diff --git a/RELEASE.md b/RELEASE.md index f9539196..328b31c2 100644 --- a/RELEASE.md +++ b/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) diff --git a/docs/06 Variables.md b/docs/06 Variables.md index e90152ec..636dda02 100644 --- a/docs/06 Variables.md +++ b/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) diff --git a/ogma/src/eng/comp/infer.rs b/ogma/src/eng/comp/infer.rs index 5189b59a..5112a6bf 100644 --- a/ogma/src/eng/comp/infer.rs +++ b/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, 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>( diff --git a/ogma/src/eng/graphs/astgraph.rs b/ogma/src/eng/graphs/astgraph.rs index fd774a2f..46000510 100644 --- a/ogma/src/eng/graphs/astgraph.rs +++ b/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 { + 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!(), + }) + } } diff --git a/ogma/src/eng/graphs/mod.rs b/ogma/src/eng/graphs/mod.rs index ba904687..9a321c1f 100644 --- a/ogma/src/eng/graphs/mod.rs +++ b/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()))); + } } diff --git a/ogma/src/lang/impls/intrinsics/arithmetic.rs b/ogma/src/lang/impls/intrinsics/arithmetic.rs index 09fea835..9906fb4a 100644 --- a/ogma/src/lang/impls/intrinsics/arithmetic.rs +++ b/ogma/src/lang/impls/intrinsics/arithmetic.rs @@ -20,11 +20,8 @@ fn variadic_intrinsic_num(blk: Block, f: F) -> Result where F: Fn(f64, f64) -> f64 + Send + Sync + 'static, { - variadic_intrinsic::(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::(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 { match blk.in_ty() { Ty::Num => variadic_intrinsic_num(blk, std::ops::Add::add), - Ty::Str => variadic_intrinsic::(blk, |prev, next| { - ( - prev.map(|mut s| { - s.to_mut().push_str(&next); - s - }) - .unwrap_or(next), - false, - ) + Ty::Str => variadic_intrinsic_in_constrained::(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 { 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) }) } diff --git a/ogma/src/lang/impls/intrinsics/cmp.rs b/ogma/src/lang/impls/intrinsics/cmp.rs index b85cb131..dad01fba 100644 --- a/ogma/src/lang/impls/intrinsics/cmp.rs +++ b/ogma/src/lang/impls/intrinsics/cmp.rs @@ -300,10 +300,9 @@ fn max_help() -> HelpMessage { ) } -fn max_intrinsic(mut blk: Block) -> Result { - blk.assert_output(Ty::Num); - variadic_intrinsic::(blk, |prev, next| { - let x = prev.map(|prev| std::cmp::max(prev, next)).unwrap_or(next); +fn max_intrinsic(blk: Block) -> Result { + variadic_intrinsic_in_constrained::(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 { - blk.assert_output(Ty::Num); - variadic_intrinsic::(blk, |prev, next| { - let x = prev.map(|prev| std::cmp::min(prev, next)).unwrap_or(next); +fn min_intrinsic(blk: Block) -> Result { + variadic_intrinsic_in_constrained::(blk, |prev, next| { + let x = std::cmp::min(prev, next); (x, false) }) } diff --git a/ogma/src/lang/impls/intrinsics/logic.rs b/ogma/src/lang/impls/intrinsics/logic.rs index 2c81da62..1353abd8 100644 --- a/ogma/src/lang/impls/intrinsics/logic.rs +++ b/ogma/src/lang/impls/intrinsics/logic.rs @@ -21,11 +21,9 @@ fn and_help() -> HelpMessage { ) } -fn and_intrinsic(mut blk: Block) -> Result { - blk.assert_output(Ty::Bool); - variadic_intrinsic::(blk, |prev, next| { - let x = prev.unwrap_or(true); - let x = x && next; +fn and_intrinsic(blk: Block) -> Result { + variadic_intrinsic_in_agnostic::(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 { - blk.assert_output(Ty::Bool); // or returns boolean - - variadic_intrinsic::(blk, |prev, next| { - let x = prev.unwrap_or(false); - let x = x || next; - (x, x) +fn or_intrinsic(blk: Block) -> Result { + variadic_intrinsic_in_agnostic::(blk, |prev, next| { + let x = prev || next; + (x, x) // short-circuit if true! }) } diff --git a/ogma/src/lang/impls/intrinsics/mod.rs b/ogma/src/lang/impls/intrinsics/mod.rs index 3eccd96d..47a2058b 100644 --- a/ogma/src/lang/impls/intrinsics/mod.rs +++ b/ogma/src/lang/impls/intrinsics/mod.rs @@ -61,23 +61,31 @@ type InnerTable = ::table::Table; /// 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(mut blk: Block, aggfn: F) -> Result +/// +/// This intrinsic uses the input type as the seed. +fn variadic_intrinsic_in_constrained(mut blk: Block, aggfn: F) -> Result where T: AsType + Into + 'static, T: TryFrom, - F: Fn(Option, 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 = 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(mut blk: Block, aggfn: F) -> Result +where + T: AsType + Into + 'static, + T: TryFrom, + 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)) }) } diff --git a/ogma/tests/commands/arithmetic.rs b/ogma/tests/commands/arithmetic.rs index 1c7c7719..258c92f6 100644 --- a/ogma/tests/commands/arithmetic.rs +++ b/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 diff --git a/ogma/tests/commands/mod.rs b/ogma/tests/commands/mod.rs index 2b47c258..3d533308 100644 --- a/ogma/tests/commands/mod.rs +++ b/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( diff --git a/ogma/tests/commands/pipeline.rs b/ogma/tests/commands/pipeline.rs index aa500496..b065b494 100644 --- a/ogma/tests/commands/pipeline.rs +++ b/ogma/tests/commands/pipeline.rs @@ -500,7 +500,7 @@ fn assigning() { assert_eq!(x, Ok(Value::Bool(true))); let x = process_w_num( - "let {+ 1} $x {- 1} $y {= 3} $z | \\ $x | / $y | and {= 2} $z", + "let {+ 1} $x {- 1} $y {= 3} $z | \\ $x | / $y | = 2 | and $z", defs, ); assert_eq!(x, Ok(Value::Bool(true))); diff --git a/ogma/tests/doc-examples.rs b/ogma/tests/doc-examples.rs index 098d1368..8d3dadb8 100644 --- a/ogma/tests/doc-examples.rs +++ b/ogma/tests/doc-examples.rs @@ -196,7 +196,7 @@ let {get:Num price} $price {get:Num carat} $ct | \$price | / $ct }"#, .unwrap(); let b = process( - r#"open tests/diamonds.csv | append --'Price per Carat' / #i.price #i.carat"#, + r#"open tests/diamonds.csv | append --'Price per Carat' { let $i | get price | / $i.carat }"#, defs, ) .unwrap();