From 98f1309400dddb9aa086f2f921d642ae2214d15c Mon Sep 17 00:00:00 2001 From: Solomon Victorino Date: Tue, 12 Nov 2024 11:47:31 -0700 Subject: [PATCH] error when a closure is used as `def` body MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit # User-Facing Changes `def` now errors instead of silently ignoring closure blocks: ```nushell > def foo [] {|bar| } Error: nu::parser::parse_mismatch × Parse mismatch during operation. ╭─[entry #1:1:12] 1 │ def foo [] {|bar| } · ────┬─── · ╰── expected definition body { ... } ╰──── ``` --- crates/nu-cmd-lang/src/core_commands/def.rs | 2 +- .../src/core_commands/export_def.rs | 2 +- .../src/core_commands/export_module.rs | 2 +- crates/nu-cmd-lang/src/core_commands/for_.rs | 2 +- crates/nu-cmd-lang/src/core_commands/if_.rs | 4 +-- crates/nu-cmd-lang/src/core_commands/loop_.rs | 2 +- .../nu-cmd-lang/src/core_commands/module.rs | 2 +- crates/nu-cmd-lang/src/core_commands/try_.rs | 2 +- .../nu-cmd-lang/src/core_commands/while_.rs | 2 +- crates/nu-command/src/debug/timeit.rs | 2 +- crates/nu-command/src/env/export_env.rs | 2 +- crates/nu-parser/src/parse_keywords.rs | 7 ++--- crates/nu-parser/src/parser.rs | 28 +++++++++++++------ crates/nu-parser/tests/test_parser.rs | 6 ++-- crates/nu-protocol/src/syntax_shape.rs | 7 +++-- crates/nu-protocol/src/ty.rs | 2 +- tests/repl/test_parser.rs | 6 ++-- 17 files changed, 47 insertions(+), 33 deletions(-) diff --git a/crates/nu-cmd-lang/src/core_commands/def.rs b/crates/nu-cmd-lang/src/core_commands/def.rs index efb5bdfae2..a281e3004c 100644 --- a/crates/nu-cmd-lang/src/core_commands/def.rs +++ b/crates/nu-cmd-lang/src/core_commands/def.rs @@ -18,7 +18,7 @@ impl Command for Def { .input_output_types(vec![(Type::Nothing, Type::Nothing)]) .required("def_name", SyntaxShape::String, "Command name.") .required("params", SyntaxShape::Signature, "Parameters.") - .required("block", SyntaxShape::Closure(None), "Body of the definition.") + .required("block", SyntaxShape::Block(false), "Body of the definition.") .switch("env", "keep the environment defined inside the command", None) .switch("wrapped", "treat unknown flags and arguments as strings (requires ...rest-like parameter in signature)", None) .category(Category::Core) diff --git a/crates/nu-cmd-lang/src/core_commands/export_def.rs b/crates/nu-cmd-lang/src/core_commands/export_def.rs index 51c4c871a2..44d524560a 100644 --- a/crates/nu-cmd-lang/src/core_commands/export_def.rs +++ b/crates/nu-cmd-lang/src/core_commands/export_def.rs @@ -18,7 +18,7 @@ impl Command for ExportDef { .input_output_types(vec![(Type::Nothing, Type::Nothing)]) .required("def_name", SyntaxShape::String, "Command name.") .required("params", SyntaxShape::Signature, "Parameters.") - .required("block", SyntaxShape::Block, "Body of the definition.") + .required("block", SyntaxShape::Block(false), "Body of the definition.") .switch("env", "keep the environment defined inside the command", None) .switch("wrapped", "treat unknown flags and arguments as strings (requires ...rest-like parameter in signature)", None) .category(Category::Core) diff --git a/crates/nu-cmd-lang/src/core_commands/export_module.rs b/crates/nu-cmd-lang/src/core_commands/export_module.rs index c749c1a43f..82588b6761 100644 --- a/crates/nu-cmd-lang/src/core_commands/export_module.rs +++ b/crates/nu-cmd-lang/src/core_commands/export_module.rs @@ -20,7 +20,7 @@ impl Command for ExportModule { .required("module", SyntaxShape::String, "Module name or module path.") .optional( "block", - SyntaxShape::Block, + SyntaxShape::Block(true), "Body of the module if 'module' parameter is not a path.", ) .category(Category::Core) diff --git a/crates/nu-cmd-lang/src/core_commands/for_.rs b/crates/nu-cmd-lang/src/core_commands/for_.rs index 912d738524..5990bd49bf 100644 --- a/crates/nu-cmd-lang/src/core_commands/for_.rs +++ b/crates/nu-cmd-lang/src/core_commands/for_.rs @@ -27,7 +27,7 @@ impl Command for For { SyntaxShape::Keyword(b"in".to_vec(), Box::new(SyntaxShape::Any)), "Range of the loop.", ) - .required("block", SyntaxShape::Block, "The block to run.") + .required("block", SyntaxShape::Block(true), "The block to run.") .creates_scope() .category(Category::Core) } diff --git a/crates/nu-cmd-lang/src/core_commands/if_.rs b/crates/nu-cmd-lang/src/core_commands/if_.rs index e40bb13b82..59a2516753 100644 --- a/crates/nu-cmd-lang/src/core_commands/if_.rs +++ b/crates/nu-cmd-lang/src/core_commands/if_.rs @@ -24,7 +24,7 @@ impl Command for If { .required("cond", SyntaxShape::MathExpression, "Condition to check.") .required( "then_block", - SyntaxShape::Block, + SyntaxShape::Block(true), "Block to run if check succeeds.", ) .optional( @@ -32,7 +32,7 @@ impl Command for If { SyntaxShape::Keyword( b"else".to_vec(), Box::new(SyntaxShape::OneOf(vec![ - SyntaxShape::Block, + SyntaxShape::Block(true), SyntaxShape::Expression, ])), ), diff --git a/crates/nu-cmd-lang/src/core_commands/loop_.rs b/crates/nu-cmd-lang/src/core_commands/loop_.rs index b799d32141..95438e2912 100644 --- a/crates/nu-cmd-lang/src/core_commands/loop_.rs +++ b/crates/nu-cmd-lang/src/core_commands/loop_.rs @@ -17,7 +17,7 @@ impl Command for Loop { Signature::build("loop") .input_output_types(vec![(Type::Nothing, Type::Nothing)]) .allow_variants_without_examples(true) - .required("block", SyntaxShape::Block, "Block to loop.") + .required("block", SyntaxShape::Block(true), "Block to loop.") .category(Category::Core) } diff --git a/crates/nu-cmd-lang/src/core_commands/module.rs b/crates/nu-cmd-lang/src/core_commands/module.rs index 4ed2d5c599..ed00362016 100644 --- a/crates/nu-cmd-lang/src/core_commands/module.rs +++ b/crates/nu-cmd-lang/src/core_commands/module.rs @@ -20,7 +20,7 @@ impl Command for Module { .required("module", SyntaxShape::String, "Module name or module path.") .optional( "block", - SyntaxShape::Block, + SyntaxShape::Block(true), "Body of the module if 'module' parameter is not a module path.", ) .category(Category::Core) diff --git a/crates/nu-cmd-lang/src/core_commands/try_.rs b/crates/nu-cmd-lang/src/core_commands/try_.rs index c7242550a7..2d250dd451 100644 --- a/crates/nu-cmd-lang/src/core_commands/try_.rs +++ b/crates/nu-cmd-lang/src/core_commands/try_.rs @@ -16,7 +16,7 @@ impl Command for Try { fn signature(&self) -> nu_protocol::Signature { Signature::build("try") .input_output_types(vec![(Type::Any, Type::Any)]) - .required("try_block", SyntaxShape::Block, "Block to run.") + .required("try_block", SyntaxShape::Block(true), "Block to run.") .optional( "catch_closure", SyntaxShape::Keyword( diff --git a/crates/nu-cmd-lang/src/core_commands/while_.rs b/crates/nu-cmd-lang/src/core_commands/while_.rs index e9578ae6f5..48622caf7e 100644 --- a/crates/nu-cmd-lang/src/core_commands/while_.rs +++ b/crates/nu-cmd-lang/src/core_commands/while_.rs @@ -20,7 +20,7 @@ impl Command for While { .required("cond", SyntaxShape::MathExpression, "Condition to check.") .required( "block", - SyntaxShape::Block, + SyntaxShape::Block(true), "Block to loop if check succeeds.", ) .category(Category::Core) diff --git a/crates/nu-command/src/debug/timeit.rs b/crates/nu-command/src/debug/timeit.rs index 7bb492769c..5bf06b2107 100644 --- a/crates/nu-command/src/debug/timeit.rs +++ b/crates/nu-command/src/debug/timeit.rs @@ -17,7 +17,7 @@ impl Command for TimeIt { Signature::build("timeit") .required( "command", - SyntaxShape::OneOf(vec![SyntaxShape::Block, SyntaxShape::Expression]), + SyntaxShape::OneOf(vec![SyntaxShape::Block(true), SyntaxShape::Expression]), "The command or block to run.", ) .input_output_types(vec![ diff --git a/crates/nu-command/src/env/export_env.rs b/crates/nu-command/src/env/export_env.rs index 7c0072b5d4..0d3bc67ab4 100644 --- a/crates/nu-command/src/env/export_env.rs +++ b/crates/nu-command/src/env/export_env.rs @@ -14,7 +14,7 @@ impl Command for ExportEnv { .input_output_types(vec![(Type::Nothing, Type::Nothing)]) .required( "block", - SyntaxShape::Block, + SyntaxShape::Block(true), "The block to run to set the environment.", ) .category(Category::Env) diff --git a/crates/nu-parser/src/parse_keywords.rs b/crates/nu-parser/src/parse_keywords.rs index 3292405cab..31df7480e8 100644 --- a/crates/nu-parser/src/parse_keywords.rs +++ b/crates/nu-parser/src/parse_keywords.rs @@ -462,10 +462,9 @@ pub fn parse_def( let block = working_set.get_block_mut(*block_id); block.signature = Box::new(sig.clone()); } - _ => working_set.error(ParseError::Expected( - "definition body closure { ... }", - arg.span, - )), + _ => { + working_set.error(ParseError::Expected("definition body { ... }", arg.span)) + } } } diff --git a/crates/nu-parser/src/parser.rs b/crates/nu-parser/src/parser.rs index e4d3765e17..c66f3dd1e7 100644 --- a/crates/nu-parser/src/parser.rs +++ b/crates/nu-parser/src/parser.rs @@ -1885,8 +1885,8 @@ pub fn parse_brace_expr( // If we're empty, that means an empty record or closure if matches!(shape, SyntaxShape::Closure(_)) { parse_closure_expression(working_set, shape, span) - } else if matches!(shape, SyntaxShape::Block) { - parse_block_expression(working_set, span) + } else if let SyntaxShape::Block(capture_mutable) = shape { + parse_block_expression(working_set, capture_mutable, span) } else if matches!(shape, SyntaxShape::MatchBlock) { parse_match_block_expression(working_set, span) } else { @@ -1900,8 +1900,8 @@ pub fn parse_brace_expr( parse_full_cell_path(working_set, None, span) } else if matches!(shape, SyntaxShape::Closure(_)) { parse_closure_expression(working_set, shape, span) - } else if matches!(shape, SyntaxShape::Block) { - parse_block_expression(working_set, span) + } else if let SyntaxShape::Block(capture_mutable) = shape { + parse_block_expression(working_set, capture_mutable, span) } else if matches!(shape, SyntaxShape::MatchBlock) { parse_match_block_expression(working_set, span) } else if second_token.is_some_and(|c| { @@ -4289,7 +4289,11 @@ fn table_type(head: &[Expression], rows: &[Vec]) -> (Type, Vec Expression { +pub fn parse_block_expression( + working_set: &mut StateWorkingSet, + capture_mutable: &bool, + span: Span, +) -> Expression { trace!("parsing: block expression"); let bytes = working_set.get_span_contents(span); @@ -4344,7 +4348,15 @@ pub fn parse_block_expression(working_set: &mut StateWorkingSet, span: Span) -> let block_id = working_set.add_block(Arc::new(output)); - Expression::new(working_set, Expr::Block(block_id), span, Type::Block) + Expression::new( + working_set, + match capture_mutable { + true => Expr::Block(block_id), + false => Expr::Closure(block_id), + }, + span, + Type::Block, + ) } pub fn parse_match_block_expression(working_set: &mut StateWorkingSet, span: Span) -> Expression { @@ -4529,7 +4541,7 @@ pub fn parse_match_block_expression(working_set: &mut StateWorkingSet, span: Spa working_set, &[output[position].span], &mut 0, - &SyntaxShape::OneOf(vec![SyntaxShape::Block, SyntaxShape::Expression]), + &SyntaxShape::OneOf(vec![SyntaxShape::Block(true), SyntaxShape::Expression]), ); position += 1; working_set.exit_scope(); @@ -4805,7 +4817,7 @@ pub fn parse_value( // Be sure to return ParseError::Expected(..) if invoked for one of these shapes, but lex // stream doesn't start with '{'} -- parsing in SyntaxShape::Any arm depends on this error variant. - SyntaxShape::Block | SyntaxShape::Closure(..) | SyntaxShape::Record(_) => { + SyntaxShape::Block(_) | SyntaxShape::Closure(..) | SyntaxShape::Record(_) => { working_set.error(ParseError::Expected("block, closure or record", span)); Expression::garbage(working_set, span) diff --git a/crates/nu-parser/tests/test_parser.rs b/crates/nu-parser/tests/test_parser.rs index 11cca19d1f..f05e40f713 100644 --- a/crates/nu-parser/tests/test_parser.rs +++ b/crates/nu-parser/tests/test_parser.rs @@ -1991,7 +1991,7 @@ mod input_types { .input_output_types(vec![(Type::Nothing, Type::Nothing)]) .required("def_name", SyntaxShape::String, "definition name") .required("params", SyntaxShape::Signature, "parameters") - .required("body", SyntaxShape::Closure(None), "body of the definition") + .required("body", SyntaxShape::Block(false), "body of the definition") .category(Category::Core) } @@ -2228,7 +2228,7 @@ mod input_types { .required("cond", SyntaxShape::MathExpression, "condition to check") .required( "then_block", - SyntaxShape::Block, + SyntaxShape::Block(true), "block to run if check succeeds", ) .optional( @@ -2236,7 +2236,7 @@ mod input_types { SyntaxShape::Keyword( b"else".to_vec(), Box::new(SyntaxShape::OneOf(vec![ - SyntaxShape::Block, + SyntaxShape::Block(true), SyntaxShape::Expression, ])), ), diff --git a/crates/nu-protocol/src/syntax_shape.rs b/crates/nu-protocol/src/syntax_shape.rs index 66bd77ddca..a4732dfd93 100644 --- a/crates/nu-protocol/src/syntax_shape.rs +++ b/crates/nu-protocol/src/syntax_shape.rs @@ -18,7 +18,8 @@ pub enum SyntaxShape { Binary, /// A block is allowed, eg `{start this thing}` - Block, + /// `bool`: capture mutable variables + Block(bool), /// A boolean value, eg `true` or `false` Boolean, @@ -143,7 +144,7 @@ impl SyntaxShape { match self { SyntaxShape::Any => Type::Any, - SyntaxShape::Block => Type::Block, + SyntaxShape::Block(_) => Type::Block, SyntaxShape::Closure(_) => Type::Closure, SyntaxShape::Binary => Type::Binary, SyntaxShape::CellPath => Type::Any, @@ -209,7 +210,7 @@ impl Display for SyntaxShape { SyntaxShape::Directory => write!(f, "directory"), SyntaxShape::GlobPattern => write!(f, "glob"), SyntaxShape::ImportPattern => write!(f, "import"), - SyntaxShape::Block => write!(f, "block"), + SyntaxShape::Block(_) => write!(f, "block"), SyntaxShape::Closure(args) => { if let Some(args) = args { let arg_vec: Vec<_> = args.iter().map(|x| x.to_string()).collect(); diff --git a/crates/nu-protocol/src/ty.rs b/crates/nu-protocol/src/ty.rs index 55da6422b9..719d9a4167 100644 --- a/crates/nu-protocol/src/ty.rs +++ b/crates/nu-protocol/src/ty.rs @@ -110,7 +110,7 @@ impl Type { Type::Range => SyntaxShape::Range, Type::Bool => SyntaxShape::Boolean, Type::String => SyntaxShape::String, - Type::Block => SyntaxShape::Block, // FIXME needs more accuracy + Type::Block => SyntaxShape::Block(true), // FIXME needs more accuracy Type::Closure => SyntaxShape::Closure(None), // FIXME needs more accuracy Type::CellPath => SyntaxShape::CellPath, Type::Duration => SyntaxShape::Duration, diff --git a/tests/repl/test_parser.rs b/tests/repl/test_parser.rs index f77d431110..41872a0133 100644 --- a/tests/repl/test_parser.rs +++ b/tests/repl/test_parser.rs @@ -931,8 +931,10 @@ fn record_missing_value() -> TestResult { } #[test] -fn def_requires_body_closure() -> TestResult { - fail_test("def a [] (echo 4)", "expected definition body closure") +fn def_requires_body_block() -> TestResult { + fail_test("def a [] (echo 4)", "expected definition body")?; + fail_test("def a [] {|| }", "expected definition body")?; + fail_test("def a [] {|b: int| echo $b }", "expected definition body") } #[test]