From 1e4de6a84031a55d9c225021eb1e9a611d1a7cb2 Mon Sep 17 00:00:00 2001 From: "Heinz N. Gies" Date: Mon, 25 Sep 2023 11:53:19 +0200 Subject: [PATCH] Improved errors and feedback Signed-off-by: Heinz N. Gies --- static/language/epilog/for.md | 3 +- tests/script_runtime_error.rs | 4 +++ .../bad_fold_op/error.txt | 5 ++++ tests/script_runtime_errors/bad_fold_op/in | 1 + .../bad_fold_op/script.tremor | 3 ++ .../bad_fold_op_imut/error.txt | 5 ++++ .../script_runtime_errors/bad_fold_op_imut/in | 1 + .../bad_fold_op_imut/script.tremor | 3 ++ .../bad_fold_op_record/error.txt | 5 ++++ .../bad_fold_op_record/in | 1 + .../bad_fold_op_record/script.tremor | 3 ++ .../bad_fold_op_record_imut/error.txt | 5 ++++ .../bad_fold_op_record_imut/in | 1 + .../bad_fold_op_record_imut/script.tremor | 3 ++ tests/script_warning.rs | 1 + .../script_warnings/for_fold_op/script.tremor | 3 ++ tests/script_warnings/for_fold_op/warning.txt | 5 ++++ tremor-script/src/ast.rs | 10 +++++-- tremor-script/src/ast/raw.rs | 14 ++++++++-- tremor-script/src/grammar.lalrpop | 28 +++++++------------ tremor-script/src/interpreter/expr.rs | 25 +++++++++++------ tremor-script/src/interpreter/imut_expr.rs | 28 +++++++++++++------ 22 files changed, 116 insertions(+), 41 deletions(-) create mode 100644 tests/script_runtime_errors/bad_fold_op/error.txt create mode 100644 tests/script_runtime_errors/bad_fold_op/in create mode 100644 tests/script_runtime_errors/bad_fold_op/script.tremor create mode 100644 tests/script_runtime_errors/bad_fold_op_imut/error.txt create mode 100644 tests/script_runtime_errors/bad_fold_op_imut/in create mode 100644 tests/script_runtime_errors/bad_fold_op_imut/script.tremor create mode 100644 tests/script_runtime_errors/bad_fold_op_record/error.txt create mode 100644 tests/script_runtime_errors/bad_fold_op_record/in create mode 100644 tests/script_runtime_errors/bad_fold_op_record/script.tremor create mode 100644 tests/script_runtime_errors/bad_fold_op_record_imut/error.txt create mode 100644 tests/script_runtime_errors/bad_fold_op_record_imut/in create mode 100644 tests/script_runtime_errors/bad_fold_op_record_imut/script.tremor create mode 100644 tests/script_warnings/for_fold_op/script.tremor create mode 100644 tests/script_warnings/for_fold_op/warning.txt diff --git a/static/language/epilog/for.md b/static/language/epilog/for.md index d7fcaee358..3ed08e3b62 100644 --- a/static/language/epilog/for.md +++ b/static/language/epilog/for.md @@ -21,9 +21,10 @@ initial value is `[]` and the operator `+` is used - so the above example can al For inserting a few keys into a record we could write ```tremor + let base = {"snot": "badger"} for new_values of case (k,v) => {k: v} - into {} + into base end ``` diff --git a/tests/script_runtime_error.rs b/tests/script_runtime_error.rs index 4664021999..1ee9d677cd 100644 --- a/tests/script_runtime_error.rs +++ b/tests/script_runtime_error.rs @@ -162,6 +162,10 @@ test_cases!( subslice_no_arr, subslice_out_of_bounds, // INSERT + bad_fold_op_record, + bad_fold_op, + bad_fold_op_record_imut, + bad_fold_op_imut, bad_fold_type, bad_fold_type_imut, bad_merge2, diff --git a/tests/script_runtime_errors/bad_fold_op/error.txt b/tests/script_runtime_errors/bad_fold_op/error.txt new file mode 100644 index 0000000000..c68ab89358 --- /dev/null +++ b/tests/script_runtime_errors/bad_fold_op/error.txt @@ -0,0 +1,5 @@ +Error: + 1 | for event of + 2 | case (k, v) => v + | ^ Invalid fold operation / for arrays + 3 | into [] use / end \ No newline at end of file diff --git a/tests/script_runtime_errors/bad_fold_op/in b/tests/script_runtime_errors/bad_fold_op/in new file mode 100644 index 0000000000..3cc0ecbedf --- /dev/null +++ b/tests/script_runtime_errors/bad_fold_op/in @@ -0,0 +1 @@ +[1,2,3] diff --git a/tests/script_runtime_errors/bad_fold_op/script.tremor b/tests/script_runtime_errors/bad_fold_op/script.tremor new file mode 100644 index 0000000000..7efc595d07 --- /dev/null +++ b/tests/script_runtime_errors/bad_fold_op/script.tremor @@ -0,0 +1,3 @@ +for event of + case (k, v) => v +into [] use / end diff --git a/tests/script_runtime_errors/bad_fold_op_imut/error.txt b/tests/script_runtime_errors/bad_fold_op_imut/error.txt new file mode 100644 index 0000000000..e1b11b08c5 --- /dev/null +++ b/tests/script_runtime_errors/bad_fold_op_imut/error.txt @@ -0,0 +1,5 @@ +Error: + 1 | (for event of + 2 | case (k, v) => v + | ^ Invalid fold operation / for arrays + 3 | into [] use / end) \ No newline at end of file diff --git a/tests/script_runtime_errors/bad_fold_op_imut/in b/tests/script_runtime_errors/bad_fold_op_imut/in new file mode 100644 index 0000000000..3cc0ecbedf --- /dev/null +++ b/tests/script_runtime_errors/bad_fold_op_imut/in @@ -0,0 +1 @@ +[1,2,3] diff --git a/tests/script_runtime_errors/bad_fold_op_imut/script.tremor b/tests/script_runtime_errors/bad_fold_op_imut/script.tremor new file mode 100644 index 0000000000..412f9994b6 --- /dev/null +++ b/tests/script_runtime_errors/bad_fold_op_imut/script.tremor @@ -0,0 +1,3 @@ +(for event of + case (k, v) => v +into [] use / end) diff --git a/tests/script_runtime_errors/bad_fold_op_record/error.txt b/tests/script_runtime_errors/bad_fold_op_record/error.txt new file mode 100644 index 0000000000..acbc914a83 --- /dev/null +++ b/tests/script_runtime_errors/bad_fold_op_record/error.txt @@ -0,0 +1,5 @@ +Error: + 1 | for event of + 2 | case (k, v) => {"#{k}": v} + | ^^^^^^^^^^^ Invalid fold operation / for records + 3 | into {} use / end \ No newline at end of file diff --git a/tests/script_runtime_errors/bad_fold_op_record/in b/tests/script_runtime_errors/bad_fold_op_record/in new file mode 100644 index 0000000000..78534a5e91 --- /dev/null +++ b/tests/script_runtime_errors/bad_fold_op_record/in @@ -0,0 +1 @@ +{"k":"v"} \ No newline at end of file diff --git a/tests/script_runtime_errors/bad_fold_op_record/script.tremor b/tests/script_runtime_errors/bad_fold_op_record/script.tremor new file mode 100644 index 0000000000..66be2baa26 --- /dev/null +++ b/tests/script_runtime_errors/bad_fold_op_record/script.tremor @@ -0,0 +1,3 @@ +for event of + case (k, v) => {"#{k}": v} +into {} use / end diff --git a/tests/script_runtime_errors/bad_fold_op_record_imut/error.txt b/tests/script_runtime_errors/bad_fold_op_record_imut/error.txt new file mode 100644 index 0000000000..635711886b --- /dev/null +++ b/tests/script_runtime_errors/bad_fold_op_record_imut/error.txt @@ -0,0 +1,5 @@ +Error: + 1 | (for event of + 2 | case (k, v) => {"#{k}": v} + | ^^^^^^^^^^^ Invalid fold operation / for records + 3 | into {} use / end) \ No newline at end of file diff --git a/tests/script_runtime_errors/bad_fold_op_record_imut/in b/tests/script_runtime_errors/bad_fold_op_record_imut/in new file mode 100644 index 0000000000..78534a5e91 --- /dev/null +++ b/tests/script_runtime_errors/bad_fold_op_record_imut/in @@ -0,0 +1 @@ +{"k":"v"} \ No newline at end of file diff --git a/tests/script_runtime_errors/bad_fold_op_record_imut/script.tremor b/tests/script_runtime_errors/bad_fold_op_record_imut/script.tremor new file mode 100644 index 0000000000..9cc9d1bf6f --- /dev/null +++ b/tests/script_runtime_errors/bad_fold_op_record_imut/script.tremor @@ -0,0 +1,3 @@ +(for event of + case (k, v) => {"#{k}": v} +into {} use / end) diff --git a/tests/script_warning.rs b/tests/script_warning.rs index 643ebb000d..2b5f2622b5 100644 --- a/tests/script_warning.rs +++ b/tests/script_warning.rs @@ -66,6 +66,7 @@ test_cases!( match_imut_no_default, match_imut_multiple_default, // INSERT + for_fold_op, record_perf, array_perf, nanotime, diff --git a/tests/script_warnings/for_fold_op/script.tremor b/tests/script_warnings/for_fold_op/script.tremor new file mode 100644 index 0000000000..9c4725b63a --- /dev/null +++ b/tests/script_warnings/for_fold_op/script.tremor @@ -0,0 +1,3 @@ +for event of + case (k, v) => v +into 0 use + end diff --git a/tests/script_warnings/for_fold_op/warning.txt b/tests/script_warnings/for_fold_op/warning.txt new file mode 100644 index 0000000000..b9f420e105 --- /dev/null +++ b/tests/script_warnings/for_fold_op/warning.txt @@ -0,0 +1,5 @@ +Warning(general): + 1 | for event of + 2 | case (k, v) => v + 3 | into 0 use + end + | ^^^^^^^^^^^^^^^^ The `use ` syntax is experimental and may be deprecated or replaced \ No newline at end of file diff --git a/tremor-script/src/ast.rs b/tremor-script/src/ast.rs index c84d03b875..ee41638dcc 100644 --- a/tremor-script/src/ast.rs +++ b/tremor-script/src/ast.rs @@ -1562,9 +1562,15 @@ pub struct Merge<'script> { impl_expr!(Merge); /// Fold operation for for comprehensiosn -#[derive(Clone, Debug, PartialEq, Serialize)] +#[derive(Clone, Debug, PartialEq, Serialize, Copy)] pub struct ComprehensionFoldOp(pub(crate) BinOpKind); +impl Default for ComprehensionFoldOp { + fn default() -> Self { + Self(BinOpKind::Add) + } +} + #[derive(Clone, Debug, PartialEq, Serialize)] /// Encapsulates a structure comprehension form pub struct Comprehension<'script, Ex: Expression + 'script> { @@ -1579,7 +1585,7 @@ pub struct Comprehension<'script, Ex: Expression + 'script> { /// Initial value pub initial: ImutExpr<'script>, /// Fold operation - pub fold: ComprehensionFoldOp, + pub fold: Option, /// Case applications against target elements pub cases: ComprehensionCases<'script, Ex>, } diff --git a/tremor-script/src/ast/raw.rs b/tremor-script/src/ast/raw.rs index f667b7392a..b1a3a8e17c 100644 --- a/tremor-script/src/ast/raw.rs +++ b/tremor-script/src/ast/raw.rs @@ -1229,7 +1229,7 @@ impl<'script> Upable<'script> for MergeRaw<'script> { }) } } -#[derive(Clone, Debug, PartialEq, Serialize)] +#[derive(Clone, Debug, PartialEq, Serialize, Copy)] pub struct ComprehensionFoldOpRaw(pub BinOpKind); #[derive(Clone, Debug, PartialEq, Serialize)] @@ -1241,7 +1241,7 @@ where pub target: ImutExprRaw<'script>, pub cases: ComprehensionCasesRaw<'script, Ex>, pub initial: ImutExprRaw<'script>, - pub fold: ComprehensionFoldOpRaw, + pub fold: Option, pub(crate) mid: Box, } @@ -1256,9 +1256,17 @@ where fn up<'registry>(self, helper: &mut Helper<'script, 'registry>) -> Result { // We compute the target before shadowing the key and value + if self.fold.is_some() { + helper.warn( + self.extent().expand_lines(2), + self.extent(), + &"The `use ` syntax is experimental and may be deprecated or replaced", + warning::Class::General, + ); + } let target = self.target.up(helper)?; let initial = self.initial.up(helper)?; - let fold = ComprehensionFoldOp(self.fold.0); + let fold = self.fold.map(|f| ComprehensionFoldOp(f.0)); // We know that each case will have a key and a value as a shadowed // variable so we reserve two ahead of time so we know what id's those diff --git a/tremor-script/src/grammar.lalrpop b/tremor-script/src/grammar.lalrpop index f16664ab1a..72dae37e4d 100644 --- a/tremor-script/src/grammar.lalrpop +++ b/tremor-script/src/grammar.lalrpop @@ -1255,16 +1255,9 @@ For: ComprehensionRaw<'input, ExprRaw<'input>> = { cases, mid: NodeMeta::new_box(start, end), initial: ImutExprRaw::Literal(LiteralRaw{value: Value::array(), mid: NodeMeta::new_box(start, end)}), - fold: ComprehensionFoldOpRaw(BinOpKind::Add) + fold: None }, - "for" "of" "into" "end" => ComprehensionRaw{ - target, - cases, - mid: NodeMeta::new_box(start, end), - initial, - fold: ComprehensionFoldOpRaw(BinOpKind::Add) - }, - "for" "of" "into" "use" "end" => ComprehensionRaw{ + "for" "of" "into" "end" => ComprehensionRaw{ target, cases, mid: NodeMeta::new_box(start, end), @@ -1273,6 +1266,11 @@ For: ComprehensionRaw<'input, ExprRaw<'input>> = { }, } + +MaybeFoldOperator: Option = { + ("use" )? => <>, +} + FoldOperator: ComprehensionFoldOpRaw = { "+" => ComprehensionFoldOpRaw(BinOpKind::Add), "-" => ComprehensionFoldOpRaw(BinOpKind::Sub), @@ -1302,16 +1300,10 @@ ForImut: ComprehensionRaw<'input, ImutExprRaw<'input>> = { cases, mid: NodeMeta::new_box(start, end), initial: ImutExprRaw::Literal(LiteralRaw{value: Value::array(), mid: NodeMeta::new_box(start, end)}), - fold: ComprehensionFoldOpRaw(BinOpKind::Add) + fold: None }, - "for" "of" "into" "end" => ComprehensionRaw{ - target, - cases, - mid: NodeMeta::new_box(start, end), - initial, - fold: ComprehensionFoldOpRaw(BinOpKind::Add) - }, - "for" "of" "into" "use" "end" => ComprehensionRaw{ + + "for" "of" "into" "end" => ComprehensionRaw{ target, cases, mid: NodeMeta::new_box(start, end), diff --git a/tremor-script/src/interpreter/expr.rs b/tremor-script/src/interpreter/expr.rs index 434b88a747..88a352463c 100644 --- a/tremor-script/src/interpreter/expr.rs +++ b/tremor-script/src/interpreter/expr.rs @@ -382,8 +382,9 @@ impl<'script> Expr<'script> { } else { Value::const_null() }; + let fold = expr.fold.unwrap_or_default(); // short circuite for common cases - if expr.fold == ComprehensionFoldOp(BinOpKind::Add) { + if fold == ComprehensionFoldOp(BinOpKind::Add) { if result.is_array() { let a = result.into_array().unwrap_or_default(); return self @@ -413,27 +414,35 @@ impl<'script> Expr<'script> { if opts.result_needed { let v = v.into_owned(); if let Some(lhs) = result.as_array_mut() { - match expr.fold { + match fold { ComprehensionFoldOp(BinOpKind::Add) => { lhs.push(v); } - ComprehensionFoldOp(_) => { - return err_generic(expr, l, &"Invalid fold operation"); + ComprehensionFoldOp(op) => { + return err_generic( + expr, + l, + &format!("Invalid fold operation {op} for arrays"), + ); } } } else if let Some(lhs) = result.as_object_mut() { - match expr.fold { + match fold { ComprehensionFoldOp(BinOpKind::Add) => { for (k, v) in v.try_into_object().add_span(expr, l)? { lhs.insert(k, v); } } - ComprehensionFoldOp(_) => { - return err_generic(expr, l, &"Invalid fold operation"); + ComprehensionFoldOp(op) => { + return err_generic( + expr, + l, + &format!("Invalid fold operation {op} for records"), + ); } } } else { - result = stry!(exec_binary_numeric(self, e, expr.fold.0, &result, &v)) + result = stry!(exec_binary_numeric(self, e, fold.0, &result, &v)) .into_owned(); } } diff --git a/tremor-script/src/interpreter/imut_expr.rs b/tremor-script/src/interpreter/imut_expr.rs index d9a7ce6478..5b1e2f7f51 100644 --- a/tremor-script/src/interpreter/imut_expr.rs +++ b/tremor-script/src/interpreter/imut_expr.rs @@ -409,8 +409,10 @@ impl<'script> ImutExpr<'script> { } else { Value::const_null() }; + let fold = expr.fold.unwrap_or_default(); + // short circuite for common cases - if expr.fold == ComprehensionFoldOp(BinOpKind::Add) { + if fold == ComprehensionFoldOp(BinOpKind::Add) { if result.is_array() { let a = result.into_array().unwrap_or_default(); return self @@ -438,28 +440,36 @@ impl<'script> ImutExpr<'script> { // NOTE: We are creating a new value so we have to clone; let v = v.into_owned(); if let Some(lhs) = result.as_array_mut() { - match expr.fold { + match fold { ComprehensionFoldOp(BinOpKind::Add) => { lhs.push(v); } - ComprehensionFoldOp(_) => { - return err_generic(expr, l, &"Invalid fold operation") + ComprehensionFoldOp(op) => { + return err_generic( + expr, + l, + &format!("Invalid fold operation {op} for arrays"), + ); } } } else if let Some(lhs) = result.as_object_mut() { - match expr.fold { + match fold { ComprehensionFoldOp(BinOpKind::Add) => { for (k, v) in v.try_into_object().add_span(expr, l)? { lhs.insert(k, v); } } - ComprehensionFoldOp(_) => { - return err_generic(expr, l, &"Invalid fold operation") + ComprehensionFoldOp(op) => { + return err_generic( + expr, + l, + &format!("Invalid fold operation {op} for records"), + ); } } } else { - result = stry!(exec_binary_numeric(self, e, expr.fold.0, &result, &v)) - .into_owned(); + result = + stry!(exec_binary_numeric(self, e, fold.0, &result, &v)).into_owned(); } continue 'outer;