Repository: kdr-aus/ogma (rust)

Issue:
[BUG] `last` is getting inferred as `Str` input
## Current Behaviour

`ls | grp name | fold 0 { \$row | get value | last get:Num size }` results in an error suggesting that `last` has too many arguments. This is incorrect, the type inference has incorrectly assumed the input of `last` is `Str` and is using that case to work the inference.

## Expected Behaviour

The error message should be more about the types, the guess of `Str` is overly optimistic, instead it should suggest `Str` or `Table`.

## Reproduction Steps
1. 

### Additional information


Pull request description:
Alter op target inference to check variable existence
Fixes #116

Patch:
diff --git a/RELEASE.md b/RELEASE.md
index e089528..e169bfc 100644
--- a/RELEASE.md
+++ b/RELEASE.md
@@ -8,6 +8,7 @@
 - Fix variable type inferencing when passing variables to `def`s (https://github.com/kdr-aus/ogma/pull/115)
 - Detect trailing command in `let` and suggest a pipe (https://github.com/kdr-aus/ogma/pull/113)
 - Fix error where stronger type guarantees were present (https://github.com/kdr-aus/ogma/pull/117)
+- Fix def not inferring correct input in `last` (https://github.com/kdr-aus/ogma/pull/147)
 
 **✨ Other Updates**
 - `ogma` crate API documentation is now published at https://kdr-aus.github.io/ogma/ogma/ (https://github.com/kdr-aus/ogma/commit/cf5cc7979c399b609e2e0605ffe176e70e474ac2)
diff --git a/ogma/src/eng/comp/infer.rs b/ogma/src/eng/comp/infer.rs
index fe14377..4dcc462 100644
--- a/ogma/src/eng/comp/infer.rs
+++ b/ogma/src/eng/comp/infer.rs
@@ -22,14 +22,17 @@ impl<'d> Compiler<'d> {
         // we get _lowest_ nodes that are:
         // 1. Op variants,
         // 2. are not compiled,
-        // 3. have unknown inputs
+        // 3. have multiple inputs
+        // 4. is sealed, or contains no variables
         let infer_nodes = self.ag.sinks(|n| {
             // 1. op variants
             self.ag[n].op().is_some() &&
             // 2. not compiled
             !self.compiled_ops.contains_key(&n.index()) &&
             // 3. has multiple valid types
-            self.tg[n].input.is_multiple()
+            self.tg[n].input.is_multiple() &&
+            // 4. is sealed, or contains no variables
+            (self.lg.sealed(n) || !self.ag.detect_var(OpNode(n)))
         });
 
         let mut chgs = Vec::new();
@@ -57,8 +60,6 @@ impl<'d> Compiler<'d> {
                 .filter(|n| self.tg[n.idx()].input.is_multiple())
                 .collect::<HashSet<_>>();
 
-            dbg!(&set);
-
             set.iter()
                 .copied()
                 // get shallowest node if it shows up in path only once
@@ -72,8 +73,6 @@ impl<'d> Compiler<'d> {
                 .collect::<Vec<_>>()
         };
 
-        dbg!(&infer_nodes);
-
         let mut success = false;
         let mut err = None;
 
@@ -143,46 +142,34 @@ enum Error {
 }
 
 /// Tries to compile the `op` with each type in the inferred types set.
-/// Unsuccesful compilations will add a `RemoveInput` change into the `chgs`.
+/// Pushes `RemoveInput` if failed.
 fn trial_inferred_types(op: OpNode, compiler: &Compiler, chgs: Chgs) {
     let _sink1 = &mut Default::default();
-    let _sink2 = &mut Default::default();
 
     let set = compiler.tg[op.idx()]
         .input
         .tys()
         .expect("`op` is expected to be an inferred input node with multiple types");
 
-    let cmps = set
+    let failed = set
         .iter()
         .cloned()
-        .map(|ty| {
+        .filter(|ty| {
+            let infer_output = &mut false;
             let compiled = compiler
-                .compile_block(op, ty.clone(), _sink1, _sink2)
+                .compile_block(op, ty.clone(), _sink1, infer_output)
                 .is_ok();
-            (compiled, ty)
+            // remove input if failed compilation, and
+            // does not infer output
+            // we keep in if infer_output is true, since we need more information about types
+            // before we can compile this block, and removing input inferences will just reduce it
+            // to an empty set.
+            !compiled && !*infer_output
         })
-        .collect::<Vec<_>>();
-
-    // apply changes
-    if cmps.iter().filter(|(x, _)| *x).count() == 1 {
-        // only a single compilation succeeded, going to use this as an indication that this type
-        // is the correct one (a bit of an assumption)
-        chgs.push(
-            cmps.into_iter()
-                .find_map(|(x, ty)| x.then(|| ObligeInput(op.into(), ty)))
-                .unwrap()
-                .into(),
-        );
-    } else if compiler.lg.sealed(op.idx()) {
-        // if it is sealed, there won't be any variable updates which means this is pretty much
-        // as good as it gets for compilation, reduce the inferred set to what compiled
-        chgs.extend(
-            cmps.into_iter()
-                .filter_map(|(x, ty)| (!x).then(|| RemoveInput(op.into(), ty)))
-                .map(Into::into),
-        );
-    }
+        .map(|ty| RemoveInput(op.into(), ty))
+        .map(Into::into);
+
+    chgs.extend(failed)
 }
 
 fn input_via_expr_compilation<'a>(
diff --git a/ogma/src/eng/comp/mod.rs b/ogma/src/eng/comp/mod.rs
index 4842771..6d49e94 100644
--- a/ogma/src/eng/comp/mod.rs
+++ b/ogma/src/eng/comp/mod.rs
@@ -420,12 +420,8 @@ impl<'d> Compiler<'d> {
             }
         }
 
-        dbg!(&chgs);
-
         let chgd = self.apply_graph_chgs(chgs.into_iter())?;
 
-        dbg!(chgd);
-
         (goto_resolve | chgd)
             .then(|| ())
             .ok_or_else(|| err.expect("error should be some if unsuccesful"))
diff --git a/ogma/src/eng/graphs/astgraph.rs b/ogma/src/eng/graphs/astgraph.rs
index f38ab30..62626ad 100644
--- a/ogma/src/eng/graphs/astgraph.rs
+++ b/ogma/src/eng/graphs/astgraph.rs
@@ -635,6 +635,15 @@ impl AstGraph {
         self.node_indices()
             .filter_map(move |n| self[n].def().map(|_| DefNode(n)))
     }
+
+    /// Detect if there are variable references below `op`.
+    pub fn detect_var(&self, op: OpNode) -> bool {
+        op.debug_assert_is_op_node(self);
+
+        self.sinks(|n| self[n].var().is_some())
+            .into_iter()
+            .any(|var| petgraph::algo::has_path_connecting(&self.0, op.idx(), var, None))
+    }
 }
 
 #[cfg(debug_assertions)]
diff --git a/ogma/src/eng/graphs/mod.rs b/ogma/src/eng/graphs/mod.rs
index ee651f2..205c61d 100644
--- a/ogma/src/eng/graphs/mod.rs
+++ b/ogma/src/eng/graphs/mod.rs
@@ -774,4 +774,22 @@ mod tests {
         assert_eq!(ExprNode(21.into()).parent(g), Some(OpNode(7.into())));
         assert_eq!(ExprNode(25.into()).parent(g), Some(OpNode(24.into())));
     }
+
+    #[test]
+    fn variable_detection() {
+        let (ag, _tg) = init_graphs("\\ 3 | let $a | + { - 4 } | - $a { > 3 }");
+
+        dbg!(&ag);
+
+        assert!(!ag.detect_var(OpNode(1.into()))); // \ 3
+        assert!(ag.detect_var(OpNode(3.into()))); // let
+        assert!(!ag.detect_var(OpNode(5.into()))); // +
+        assert!(ag.detect_var(OpNode(7.into()))); // - (second one)
+        assert!(!ag.detect_var(OpNode(10.into()))); // -
+        assert!(ag.detect_var(OpNode(12.into()))); // >
+        assert!(ag.detect_var(OpNode(23.into()))); // cmp $rhs
+        assert!(ag.detect_var(OpNode(25.into()))); // =
+        assert!(!ag.detect_var(OpNode(27.into()))); // Ord::Gt
+        assert!(ag.detect_var(OpNode(36.into()))); // eq $rhs
+    }
 }
diff --git a/ogma/src/eng/graphs/tygraph.rs b/ogma/src/eng/graphs/tygraph.rs
index 0be13be..ded98de 100644
--- a/ogma/src/eng/graphs/tygraph.rs
+++ b/ogma/src/eng/graphs/tygraph.rs
@@ -394,14 +394,11 @@ impl TypeGraph {
             .filter(|e| !completed_indices.contains(&e.index()))
             .collect::<Vec<_>>();
 
-        dbg!(&completed_indices);
-
         for edge in edges {
             let flow = &self[edge];
             let (from_idx, to_idx) = self
                 .edge_endpoints(edge)
                 .expect("edge would exist in graph");
-            dbg!(edge, (from_idx, to_idx));
             let from = &self[from_idx];
             let to = &self[to_idx];
             let known_out = from.output.has_ty();
@@ -415,8 +412,6 @@ impl TypeGraph {
                 conflict,
             };
 
-            dbg!(&flow);
-
             let x = match flow {
                 // Directed flow of input -> input
                 Flow::II if known_in => {
@@ -767,7 +762,8 @@ impl Knowledge {
             // An obliged source can flow into an unknown or any dest
             (Obliged(_), Any) => Ok(()),
             // An obliged source can flow into itself or lower ranked items if the types match
-            (Obliged(t1), Obliged(t2)) if t1 == t2 => Ok(()),
+            // It can also flow into a Known type, again if the types agree
+            (Obliged(t1), Known(t2) | Obliged(t2)) if t1 == t2 => Ok(()),
             (Obliged(t1), Inferred(ts)) if ts.contains(t1) => Ok(()),
 
             // An inferred source can flow into an unknown or any dest


Test patch:
diff --git a/ogma/tests/commands/pipeline.rs b/ogma/tests/commands/pipeline.rs
index ec4987b..94b9de4 100644
--- a/ogma/tests/commands/pipeline.rs
+++ b/ogma/tests/commands/pipeline.rs
@@ -324,6 +324,27 @@ expected `String`, found `Number`
     assert_eq!(x, Ok(Value::Num((0 + 1 - 30 + 100 + 100 + 100).into())));
 }
 
+#[test]
+fn get_test_with_default_as_another_get() {
+    let defs = &Definitions::new();
+    let x = process_w_table("fold 0 { \\ $row | get snd { get first } }", defs)
+        .unwrap_err()
+        .to_string();
+    println!("{}", x);
+    assert_eq!(
+        &x,
+        r#"Semantics Error: `get` does not support `Nil` input data
+--> shell:28
+ | fold 0 { \ $row | get snd { get first } }
+ |                             ^^^
+--> help: use `get --help` to view requirements. consider implementing `def get`
+"#
+    );
+
+    let x = process_w_table("fold 0 { \\ $row | get snd $row.first:Num }", defs);
+    assert_eq!(x, Ok(Value::Num(100.into())));
+}
+
 // ------ Input ----------------------------------------------------------------
 #[test]
 fn input_help_msg() {
@@ -453,6 +474,18 @@ fn last_testing() {
     assert_eq!(x, Ok(Value::Str("o".into())));
 }
 
+#[test]
+fn last_testing_bug_116() {
+    let defs = &Definitions::new();
+
+    let x = process_w_nil(
+        r#"ls | grp name | fold 0 { \$row | get value | last get:Num size }"#,
+        defs,
+    );
+    dbg!(&x);
+    assert!(matches!(x, Ok(Value::Num(_))));
+}
+
 // ------ Length ---------------------------------------------------------------
 #[test]
 fn len_help_msg() {