Skip to content

Commit

Permalink
error when a closure is used as def body
Browse files Browse the repository at this point in the history
# 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 nushell#1:1:12]
 1 │ def foo [] {|bar| }
   ·            ────┬───
   ·                ╰── expected definition body { ... }
   ╰────
```
  • Loading branch information
sgvictorino committed Nov 12, 2024
1 parent 919d55f commit d81048f
Show file tree
Hide file tree
Showing 17 changed files with 47 additions and 33 deletions.
2 changes: 1 addition & 1 deletion crates/nu-cmd-lang/src/core_commands/def.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
2 changes: 1 addition & 1 deletion crates/nu-cmd-lang/src/core_commands/export_def.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
2 changes: 1 addition & 1 deletion crates/nu-cmd-lang/src/core_commands/export_module.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
2 changes: 1 addition & 1 deletion crates/nu-cmd-lang/src/core_commands/for_.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Expand Down
4 changes: 2 additions & 2 deletions crates/nu-cmd-lang/src/core_commands/if_.rs
Original file line number Diff line number Diff line change
Expand Up @@ -24,15 +24,15 @@ 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(
"else_expression",
SyntaxShape::Keyword(
b"else".to_vec(),
Box::new(SyntaxShape::OneOf(vec![
SyntaxShape::Block,
SyntaxShape::Block(true),
SyntaxShape::Expression,
])),
),
Expand Down
2 changes: 1 addition & 1 deletion crates/nu-cmd-lang/src/core_commands/loop_.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}

Expand Down
2 changes: 1 addition & 1 deletion crates/nu-cmd-lang/src/core_commands/module.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
2 changes: 1 addition & 1 deletion crates/nu-cmd-lang/src/core_commands/try_.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down
2 changes: 1 addition & 1 deletion crates/nu-cmd-lang/src/core_commands/while_.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
2 changes: 1 addition & 1 deletion crates/nu-command/src/debug/timeit.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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![
Expand Down
2 changes: 1 addition & 1 deletion crates/nu-command/src/env/export_env.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
7 changes: 3 additions & 4 deletions crates/nu-parser/src/parse_keywords.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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))
}
}
}

Expand Down
28 changes: 20 additions & 8 deletions crates/nu-parser/src/parser.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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| {
Expand Down Expand Up @@ -4289,7 +4289,11 @@ fn table_type(head: &[Expression], rows: &[Vec<Expression>]) -> (Type, Vec<Parse
(Type::Table(ty.into()), errors)
}

pub fn parse_block_expression(working_set: &mut StateWorkingSet, span: Span) -> 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);
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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)
Expand Down
6 changes: 3 additions & 3 deletions crates/nu-parser/tests/test_parser.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}

Expand Down Expand Up @@ -2228,15 +2228,15 @@ 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(
"else_expression",
SyntaxShape::Keyword(
b"else".to_vec(),
Box::new(SyntaxShape::OneOf(vec![
SyntaxShape::Block,
SyntaxShape::Block(true),
SyntaxShape::Expression,
])),
),
Expand Down
7 changes: 4 additions & 3 deletions crates/nu-protocol/src/syntax_shape.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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();
Expand Down
2 changes: 1 addition & 1 deletion crates/nu-protocol/src/ty.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
6 changes: 4 additions & 2 deletions tests/repl/test_parser.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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]
Expand Down

0 comments on commit d81048f

Please sign in to comment.