From 6e1118681deb507670994508b648117dc435867b Mon Sep 17 00:00:00 2001 From: Solomon Date: Sun, 17 Nov 2024 17:01:52 -0700 Subject: [PATCH] make command signature parsing more strict (#14309) # User-Facing Changes The parser now errors on more invalid command signatures: ```nushell # expected parameter or flag def foo [ bar: int: ] {} # expected type def foo [ bar: = ] {} def foo [ bar: ] {} # expected default value def foo [ bar = ] {} ``` --- crates/nu-command/tests/commands/def.rs | 29 ++++++++++++++++++++++++- crates/nu-parser/src/parser.rs | 25 ++++++++++++++++----- 2 files changed, 47 insertions(+), 7 deletions(-) diff --git a/crates/nu-command/tests/commands/def.rs b/crates/nu-command/tests/commands/def.rs index c66869b479..0705c0085f 100644 --- a/crates/nu-command/tests/commands/def.rs +++ b/crates/nu-command/tests/commands/def.rs @@ -2,6 +2,13 @@ use nu_test_support::nu; use nu_test_support::playground::Playground; use std::fs; +#[test] +fn def_with_trailing_comma() { + let actual = nu!("def test-command [ foo: int, ] { $foo }; test-command 1"); + + assert!(actual.out == "1"); +} + #[test] fn def_with_comment() { Playground::setup("def_with_comment", |dirs, _| { @@ -72,6 +79,13 @@ fn def_errors_with_comma_before_equals() { assert!(actual.err.contains("expected parameter")); } +#[test] +fn def_errors_with_colon_before_equals() { + let actual = nu!("def test-command [ foo: = 1 ] {}"); + + assert!(actual.err.contains("expected type")); +} + #[test] fn def_errors_with_comma_before_colon() { let actual = nu!("def test-command [ foo, : int ] {}"); @@ -85,7 +99,6 @@ fn def_errors_with_multiple_colons() { assert!(actual.err.contains("expected type")); } -#[ignore = "This error condition is not implemented yet"] #[test] fn def_errors_with_multiple_types() { let actual = nu!("def test-command [ foo:int:string ] {}"); @@ -93,6 +106,20 @@ fn def_errors_with_multiple_types() { assert!(actual.err.contains("expected parameter")); } +#[test] +fn def_errors_with_trailing_colon() { + let actual = nu!("def test-command [ foo: int: ] {}"); + + assert!(actual.err.contains("expected parameter")); +} + +#[test] +fn def_errors_with_trailing_default_value() { + let actual = nu!("def test-command [ foo: int = ] {}"); + + assert!(actual.err.contains("expected default value")); +} + #[test] fn def_errors_with_multiple_commas() { let actual = nu!("def test-command [ foo,,bar ] {}"); diff --git a/crates/nu-parser/src/parser.rs b/crates/nu-parser/src/parser.rs index e4d3765e17..7db2e112bf 100644 --- a/crates/nu-parser/src/parser.rs +++ b/crates/nu-parser/src/parser.rs @@ -3392,6 +3392,7 @@ pub fn parse_signature_helper(working_set: &mut StateWorkingSet, span: Span) -> Arg, AfterCommaArg, Type, + AfterType, DefaultValue, } @@ -3425,7 +3426,9 @@ pub fn parse_signature_helper(working_set: &mut StateWorkingSet, span: Span) -> let mut args: Vec = vec![]; let mut parse_mode = ParseMode::Arg; - for token in &output { + for (index, token) in output.iter().enumerate() { + let last_token = index == output.len() - 1; + match token { Token { contents: crate::TokenContents::Item | crate::TokenContents::AssignmentOperator, @@ -3437,10 +3440,12 @@ pub fn parse_signature_helper(working_set: &mut StateWorkingSet, span: Span) -> // The : symbol separates types if contents == b":" { match parse_mode { + ParseMode::Arg if last_token => working_set + .error(ParseError::Expected("type", Span::new(span.end, span.end))), ParseMode::Arg => { parse_mode = ParseMode::Type; } - ParseMode::AfterCommaArg => { + ParseMode::AfterCommaArg | ParseMode::AfterType => { working_set.error(ParseError::Expected("parameter or flag", span)); } ParseMode::Type | ParseMode::DefaultValue => { @@ -3452,9 +3457,15 @@ pub fn parse_signature_helper(working_set: &mut StateWorkingSet, span: Span) -> // The = symbol separates a variable from its default value else if contents == b"=" { match parse_mode { - ParseMode::Type | ParseMode::Arg => { + ParseMode::Arg | ParseMode::AfterType if last_token => working_set.error( + ParseError::Expected("default value", Span::new(span.end, span.end)), + ), + ParseMode::Arg | ParseMode::AfterType => { parse_mode = ParseMode::DefaultValue; } + ParseMode::Type => { + working_set.error(ParseError::Expected("type", span)); + } ParseMode::AfterCommaArg => { working_set.error(ParseError::Expected("parameter or flag", span)); } @@ -3467,7 +3478,9 @@ pub fn parse_signature_helper(working_set: &mut StateWorkingSet, span: Span) -> // The , symbol separates params only else if contents == b"," { match parse_mode { - ParseMode::Arg => parse_mode = ParseMode::AfterCommaArg, + ParseMode::Arg | ParseMode::AfterType => { + parse_mode = ParseMode::AfterCommaArg + } ParseMode::AfterCommaArg => { working_set.error(ParseError::Expected("parameter or flag", span)); } @@ -3480,7 +3493,7 @@ pub fn parse_signature_helper(working_set: &mut StateWorkingSet, span: Span) -> } } else { match parse_mode { - ParseMode::Arg | ParseMode::AfterCommaArg => { + ParseMode::Arg | ParseMode::AfterCommaArg | ParseMode::AfterType => { // Long flag with optional short form following with no whitespace, e.g. --output, --age(-a) if contents.starts_with(b"--") && contents.len() > 2 { // Split the long flag from the short flag with the ( character as delimiter. @@ -3790,7 +3803,7 @@ pub fn parse_signature_helper(working_set: &mut StateWorkingSet, span: Span) -> } } } - parse_mode = ParseMode::Arg; + parse_mode = ParseMode::AfterType; } ParseMode::DefaultValue => { if let Some(last) = args.last_mut() {