Dumb output inference (#101)

* pass output inference by not having variadic be flexible on input type

* split how variadic fn could use constrained input or agnostic input

* add release note

* fmt

* fmt

RELEASE.md

@@ -1,6 +1,9 @@
 **🛑 Breaking Changes**
 - [`to-str` now defaults to full number formatting with optional formatting string
     (https://github.com/kdr-aus/ogma/pull/85)
+- Output inference is somewhat smarter for variadic cmds.
+    (https://github.com/kdr-aus/ogma/pull/101)
+    It does place stricter typing constraints on `+ - * / min max`.
 
 **🔬 New Features**
 - `#b` -- Newline constant (b for _break_) (https://github.com/kdr-aus/ogma/pull/75)

docs/06 Variables.md

@@ -24,10 +24,10 @@ let {get:Num price} $price {get:Num carat} $ct | \$price | / $ct }
 
 ![](./assets/variables-1.png?raw=true)
 
-A more idiomatic way of achieving the same result without the use of variables is to use the dot
+A more idiomatic way of achieving the same result is to use the dot
 (`.`) operator:
 ```plaintext
-open diamonds.csv | append --'Price per Carat' / #i.price #i.carat
+open diamonds.csv | append --'Price per Carat' { let $i | get price | / $i.carat }
 ```
 
 ![](./assets/variables-2.gif?raw=true)

ogma/src/eng/comp/infer.rs

@@ -190,7 +190,11 @@ fn input_via_expr_compilation<'a>(
 }
 
 fn output<'a>(op: OpNode, compiler: &Compiler_<'a>) -> std::result::Result<Compiler_<'a>, Error> {
-    test_compile_types(compiler, op.idx(), op.parent(compiler.ag()), true)
+    let ag = compiler.ag();
+    let brk = op.parent(ag);
+    //     let brk = brk.parent(ag).map(|x| x.parent(ag)).unwrap_or(brk);
+
+    test_compile_types(compiler, op.idx(), brk, true)
 }
 
 fn test_compile_types<'a>(

ogma/src/eng/graphs/astgraph.rs

@@ -1014,16 +1014,34 @@ impl ExprNode {
         g[self.idx()].tag()
     }
 
-    /// Fetches the first block's op for this expression.
-    pub fn first_op(self, g: &AstGraph) -> OpNode {
+    #[inline(always)]
+    fn debug_assert_is_expr_node(self, g: &AstGraph) {
         debug_assert!(
             matches!(g[self.idx()], AstNode::Expr(_)),
             "expecting an expression node"
         );
+    }
+
+    /// Fetches the first block's op for this expression.
+    pub fn first_op(self, g: &AstGraph) -> OpNode {
+        self.debug_assert_is_expr_node(g);
 
         g.neighbors(self.idx())
             .filter_map(|n| g[n].op().map(|_| OpNode(n)))
             .last()
             .expect("all expressions have at least one block op node")
     }
+
+    /// Find the parent op node, if there is one.
+    pub fn parent(self, g: &AstGraph) -> Option<OpNode> {
+        self.debug_assert_is_expr_node(g);
+
+        g.neighbors_directed(self.idx(), Direction::Incoming)
+            .next()
+            .map(|x| match &g[x] {
+                AstNode::Op { .. } => OpNode(x),
+                AstNode::Def { .. } => DefNode(x).parent(g),
+                _ => unreachable!(),
+            })
+    }
 }

ogma/src/eng/graphs/mod.rs

@@ -668,4 +668,33 @@ mod tests {
         assert_eq!(f(19), e(&[0, 3, 4, 5, 9, 10, 13, 17, 18, 19]));
         assert_eq!(f(20), e(&[0, 3, 4, 5, 9, 10, 13, 17, 18, 19, 20]));
     }
+
+    #[test]
+    fn node_accessors() {
+        let (ag, _tg) = init_graphs("let $a | + { > 3 } | + - + 3");
+
+        assert_eq!(ag.node_count(), 32);
+
+        //         let s = &mut String::new();
+        //         ag.debug_write_flowchart(&_tg, s);
+        //         std::fs::write("foo", s).unwrap();
+
+        let g = &ag;
+
+        // OpNode
+        assert_eq!(OpNode(1.into()).next(g), Some(OpNode(3.into())));
+        assert_eq!(OpNode(3.into()).next(g), Some(OpNode(5.into())));
+        assert_eq!(OpNode(5.into()).next(g), None);
+        assert_eq!(OpNode(5.into()).prev(g), Some(OpNode(3.into())));
+        assert_eq!(OpNode(3.into()).prev(g), Some(OpNode(1.into())));
+        assert_eq!(OpNode(1.into()).prev(g), None);
+        assert_eq!(OpNode(3.into()).parent(g), ExprNode(0.into()));
+
+        // ExprNode
+        assert_eq!(ExprNode(0.into()).first_op(g), OpNode(1.into()));
+        assert_eq!(ExprNode(0.into()).parent(g), None);
+        assert_eq!(ExprNode(6.into()).parent(g), Some(OpNode(5.into())));
+        assert_eq!(ExprNode(17.into()).parent(g), Some(OpNode(7.into())));
+        assert_eq!(ExprNode(21.into()).parent(g), Some(OpNode(20.into())));
+    }
 }

ogma/src/lang/impls/intrinsics/arithmetic.rs

@@ -20,11 +20,8 @@ fn variadic_intrinsic_num<F>(blk: Block, f: F) -> Result<Step>
 where
     F: Fn(f64, f64) -> f64 + Send + Sync + 'static,
 {
-    variadic_intrinsic::<Number, _>(blk, move |prev, next| {
-        let x = prev
-            .map(|prev| f(prev.as_f64(), next.as_f64()))
-            .map(Into::into)
-            .unwrap_or(next);
+    variadic_intrinsic_in_constrained::<Number, _>(blk, move |prev, next| {
+        let x = f(prev.as_f64(), next.as_f64()).into();
         (x, false)
     })
 }
@@ -70,15 +67,9 @@ if input is a Table, concat or join additional tables
 fn add_intrinsic(blk: Block) -> Result<Step> {
     match blk.in_ty() {
         Ty::Num => variadic_intrinsic_num(blk, std::ops::Add::add),
-        Ty::Str => variadic_intrinsic::<Str, _>(blk, |prev, next| {
-            (
-                prev.map(|mut s| {
-                    s.to_mut().push_str(&next);
-                    s
-                })
-                .unwrap_or(next),
-                false,
-            )
+        Ty::Str => variadic_intrinsic_in_constrained::<Str, _>(blk, |mut prev, next| {
+            prev.to_mut().push_str(&next);
+            (prev, false)
         }),
         Ty::Tab => add_table(blk),
         x => Err(Error::wrong_op_input_type(x, blk.op_tag())),
@@ -90,9 +81,8 @@ fn add_table(mut blk: Block) -> Result<Step> {
     let byrow = !f("cols");
     let intersect = !f("union") && f("intersect");
 
-    variadic_intrinsic(blk, move |prev, next| match prev {
-        Some(prev) => (add_table2(prev, next, byrow, intersect), false),
-        None => (next, false),
+    variadic_intrinsic_in_constrained(blk, move |prev, next| {
+        (add_table2(prev, next, byrow, intersect), false)
     })
 }
 

ogma/src/lang/impls/intrinsics/cmp.rs

@@ -300,10 +300,9 @@ fn max_help() -> HelpMessage {
     )
 }
 
-fn max_intrinsic(mut blk: Block) -> Result<Step> {
-    blk.assert_output(Ty::Num);
-    variadic_intrinsic::<Number, _>(blk, |prev, next| {
-        let x = prev.map(|prev| std::cmp::max(prev, next)).unwrap_or(next);
+fn max_intrinsic(blk: Block) -> Result<Step> {
+    variadic_intrinsic_in_constrained::<Number, _>(blk, |prev, next| {
+        let x = std::cmp::max(prev, next);
         (x, false)
     })
 }
@@ -326,10 +325,9 @@ fn min_help() -> HelpMessage {
     )
 }
 
-fn min_intrinsic(mut blk: Block) -> Result<Step> {
-    blk.assert_output(Ty::Num);
-    variadic_intrinsic::<Number, _>(blk, |prev, next| {
-        let x = prev.map(|prev| std::cmp::min(prev, next)).unwrap_or(next);
+fn min_intrinsic(blk: Block) -> Result<Step> {
+    variadic_intrinsic_in_constrained::<Number, _>(blk, |prev, next| {
+        let x = std::cmp::min(prev, next);
         (x, false)
     })
 }

ogma/src/lang/impls/intrinsics/logic.rs

@@ -21,11 +21,9 @@ fn and_help() -> HelpMessage {
     )
 }
 
-fn and_intrinsic(mut blk: Block) -> Result<Step> {
-    blk.assert_output(Ty::Bool);
-    variadic_intrinsic::<bool, _>(blk, |prev, next| {
-        let x = prev.unwrap_or(true);
-        let x = x && next;
+fn and_intrinsic(blk: Block) -> Result<Step> {
+    variadic_intrinsic_in_agnostic::<bool, _>(blk, |prev, next| {
+        let x = prev && next;
         (x, !x) // short circuit if x is false
     })
 }
@@ -144,12 +142,9 @@ fn or_help() -> HelpMessage {
     )
 }
 
-fn or_intrinsic(mut blk: Block) -> Result<Step> {
-    blk.assert_output(Ty::Bool); // or returns boolean
-
-    variadic_intrinsic::<bool, _>(blk, |prev, next| {
-        let x = prev.unwrap_or(false);
-        let x = x || next;
-        (x, x)
+fn or_intrinsic(blk: Block) -> Result<Step> {
+    variadic_intrinsic_in_agnostic::<bool, _>(blk, |prev, next| {
+        let x = prev || next;
+        (x, x) // short-circuit if true!
     })
 }

ogma/src/lang/impls/intrinsics/mod.rs

@@ -61,23 +61,31 @@ type InnerTable = ::table::Table<Value>;
 /// same operation across them. For example, the 'add' intrinsic (`+`) can add more than one
 /// argument at a time.
 ///
-/// The behaviour is to INCLUDE the input if it fits the type expected of the arguments (so that the
-/// expr `\\ 5 | + 1 2` returns an expected 8 (if it were ignore for arguments > 1 it would return
-/// 3).
-///
-/// A special case is provided for TableRow inputs which cannot follow the normal cloning
-/// techniques for values.
-///
 /// The `aggfn` takes the `(prev, next)` and returns the aggregate, along with a flag to exit early
 /// if known (for example, short circuiting an `or` or `and`).
-fn variadic_intrinsic<T, F>(mut blk: Block, aggfn: F) -> Result<Step>
+///
+/// This intrinsic uses the input type as the seed.
+fn variadic_intrinsic_in_constrained<T, F>(mut blk: Block, aggfn: F) -> Result<Step>
 where
     T: AsType + Into<Value> + 'static,
     T: TryFrom<Value, Error = Error>,
-    F: Fn(Option<T>, T) -> (T, bool) + Send + Sync + 'static,
+    F: Fn(T, T) -> (T, bool) + Send + Sync + 'static,
 {
-    let len = blk.args_len();
     let ty = T::as_type();
+
+    if blk.in_ty() != &ty {
+        return Err(Error::wrong_op_input_type(blk.in_ty(), blk.op_tag()));
+    }
+
+    blk.assert_output(ty.clone());
+
+    let len = blk.args_len();
+
+    if len == 0 {
+        let err_tag = blk.blk_tag().clone();
+        return Err(Error::insufficient_args(&err_tag, 0, None));
+    }
+
     let args = {
         let mut a = Vec::with_capacity(len);
         for _ in 0..len {
@@ -91,33 +99,71 @@ where
         }
         a
     };
-    let err_tag = blk.blk_tag().clone();
-
-    // we have an interesting position here.
-    // given blk.in_ty() == ty we can assert that input: T
-    // this way we can blk.eval with a known input: T
-    // HOWEVER we would be duplicating the business logic if we went down this path. instead we use
-    // .eval_value and _cast_ to T in the eval loop (should work!)
-    let use_input = blk.in_ty() == &ty;
 
     blk.eval_o(move |input, cx| {
-        let mut prev: Option<T> = if use_input {
-            Some(input.clone().try_into()?)
-        } else {
-            None
-        };
+        let mut prev: T = input.clone().try_into()?;
 
         for arg in &args {
             let next: T = arg.resolve(|| input.clone(), &cx)?.try_into()?;
             let (x, exit) = aggfn(prev, next);
-            prev = Some(x);
+            prev = x;
+            if exit {
+                break;
+            }
+        }
+
+        Ok((prev, cx.env))
+    })
+}
+
+/// Same as [`variadic_intrinsic_in_constrained`] but does not use the input as the first previous value.
+///
+/// This makes it input agnostic.
+fn variadic_intrinsic_in_agnostic<T, F>(mut blk: Block, aggfn: F) -> Result<Step>
+where
+    T: AsType + Into<Value> + 'static,
+    T: TryFrom<Value, Error = Error>,
+    F: Fn(T, T) -> (T, bool) + Send + Sync + 'static,
+{
+    let ty = T::as_type();
+    blk.assert_output(ty.clone());
+
+    let len = blk.args_len();
+
+    if len == 0 {
+        let err_tag = blk.blk_tag().clone();
+        return Err(Error::insufficient_args(&err_tag, 0, None));
+    }
+
+    let args = {
+        let mut a = Vec::with_capacity(len);
+        for _ in 0..len {
+            // use blocks input type
+            let arg = blk
+                .next_arg()?
+                .supplied(None)?
+                .returns(ty.clone())?
+                .concrete()?;
+            a.push(arg);
+        }
+        a
+    };
+
+    blk.eval_o(move |input, cx| {
+        // we know arg.len() > 0
+        let mut prev: T = args[0].resolve(|| input.clone(), &cx)?.try_into()?;
+
+        // skip the first one -- already computed
+        for arg in &args[1..] {
+            let next: T = arg.resolve(|| input.clone(), &cx)?.try_into()?;
+            let (x, exit) = aggfn(prev, next);
+            prev = x;
             if exit {
                 break;
             }
         }
 
-        prev.ok_or_else(|| Error::insufficient_args(&err_tag, 0, None))
-            .map(|x| (x, cx.env))
+        Ok((prev, cx.env))
     })
 }
 

ogma/tests/commands/arithmetic.rs

@@ -355,8 +355,6 @@ fn div_help_msg() {
 #[test]
 fn div_testing() {
     let defs = &Definitions::new();
-    let x = process_w_nil("/ 5", defs);
-    assert_eq!(x, Ok(Value::Num((5).into())));
     let x = process_w_num("/ 6", defs);
     assert_eq!(x, Ok(Value::Num((0.5).into())));
     let x = process_w_num("let $x | \\ 30 | ÷ -5 $x", defs);
@@ -544,7 +542,7 @@ fn sub_testing() {
     let defs = &Definitions::new();
     let x = process_w_num("- 5", defs);
     assert_eq!(x, Ok(Value::Num((-2).into())));
-    let x = process_w_nil("- -2 -1", defs);
+    let x = process_w_nil("\\ -2 | - -1", defs);
     assert_eq!(x, Ok(Value::Num((-1).into())));
     let x = process_w_num("let $x | \\ 1 | - 1 2 $x", defs);
     assert_eq!(x, Ok(Value::Num((-5).into()))); // 1 - 1 - 2 - 3

ogma/tests/commands/mod.rs

@@ -164,7 +164,7 @@ fn multiline_expr() {
 
     let x = process_w_nil(
         "
-* 3 4 |
+\\ 3 | * 4 |
 if {= 36}
     #f
     #t
@@ -239,7 +239,7 @@ fn multiline_errs() {
     let d = &Definitions::new();
 
     let x = process_w_nil(
-        "* 3 4 |
+        "\\ 3 | * 4 |
     * 'foo'",
         d,
     )
@@ -720,6 +720,21 @@ fn unrecognised_literal() {
     );
 }
 
+#[test]
+fn get_output_inference_be_smarter() {
+    let x = process_w_table("append { get first | * 2 }", &Definitions::new());
+
+    check_is_table(
+        x,
+        vec![
+            vec![o("first"), o("snd"), o("Heading 3"), o("_append1")],
+            vec![n(0), n(3), o("a"), n(0)],
+            vec![n(1), n(20), o("b"), n(2)],
+            vec![n(-30), n(100), o("z"), n(-60)],
+        ],
+    );
+}
+
 #[test]
 fn locals_graph_change_bug() {
     let x = process_w_table(