From 7aa2a574349433b50ae543c898cb117d58e58085 Mon Sep 17 00:00:00 2001 From: Leon Date: Sat, 31 Dec 2022 21:18:53 +1000 Subject: [PATCH] `def`: make various punctuation misuses into errors (#7624) Closes https://github.com/nushell/nushell/issues/7604 --- crates/nu-command/tests/commands/def.rs | 91 +++++++++++++++++++++++++ crates/nu-parser/src/errors.rs | 2 +- crates/nu-parser/src/parser.rs | 80 +++++++++++++++++----- src/tests/test_engine.rs | 5 +- 4 files changed, 159 insertions(+), 19 deletions(-) diff --git a/crates/nu-command/tests/commands/def.rs b/crates/nu-command/tests/commands/def.rs index 5123c1b7bb..9088d548b6 100644 --- a/crates/nu-command/tests/commands/def.rs +++ b/crates/nu-command/tests/commands/def.rs @@ -19,6 +19,24 @@ export def e [arg] {echo $arg} }); } +#[test] +fn def_with_param_comment() { + Playground::setup("def_with_param_comment", |dirs, _| { + let data = r#" +export def e [ +param:string #My cool attractive param +] {echo $param}; + "#; + fs::write(dirs.root().join("def_test"), data).expect("Unable to write file"); + let actual = nu!( + cwd: dirs.root(), + "use def_test e; help e" + ); + + assert!(actual.out.contains(r#"My cool attractive param"#)); + }) +} + #[test] fn def_errors_with_multiple_short_flags() { let actual = nu!( @@ -31,6 +49,79 @@ fn def_errors_with_multiple_short_flags() { assert!(actual.err.contains("expected only one short flag")); } +#[test] +fn def_errors_with_comma_before_alternative_short_flag() { + let actual = nu!( + cwd: ".", pipeline( + r#" + def test-command [ --long, (-l) ] {} + "# + )); + + assert!(actual.err.contains("expected parameter")); +} + +#[test] +fn def_errors_with_comma_before_equals() { + let actual = nu!( + cwd: ".", pipeline( + r#" + def test-command [ foo, = 1 ] {} + "# + )); + + assert!(actual.err.contains("expected parameter")); +} + +#[test] +fn def_errors_with_comma_before_colon() { + let actual = nu!( + cwd: ".", pipeline( + r#" + def test-command [ foo, : int ] {} + "# + )); + + assert!(actual.err.contains("expected parameter")); +} + +#[test] +fn def_errors_with_multiple_colons() { + let actual = nu!( + cwd: ".", pipeline( + r#" + def test-command [ foo::int ] {} + "# + )); + + assert!(actual.err.contains("expected type")); +} + +#[ignore = "This error condition is not implemented yet"] +#[test] +fn def_errors_with_multiple_types() { + let actual = nu!( + cwd: ".", pipeline( + r#" + def test-command [ foo:int:string ] {} + "# + )); + + assert!(actual.err.contains("expected parameter")); +} + +#[test] +fn def_errors_with_multiple_commas() { + let actual = nu!( + cwd: ".", pipeline( + r#" + def test-command [ foo,,bar ] {} + "# + )); + + assert!(actual.err.contains("expected parameter")); +} + #[test] fn def_fails_with_invalid_name() { let err_msg = "command name can't be a number, a filesize, or contain a hash # or caret ^"; diff --git a/crates/nu-parser/src/errors.rs b/crates/nu-parser/src/errors.rs index a4be345148..52232e632b 100644 --- a/crates/nu-parser/src/errors.rs +++ b/crates/nu-parser/src/errors.rs @@ -19,7 +19,7 @@ pub enum ParseError { #[diagnostic(code(nu::parser::extra_positional), url(docsrs), help("Usage: {0}"))] ExtraPositional(String, #[label = "extra positional argument"] Span), - #[error("Require positional parameter after optional parameter")] + #[error("Required positional parameter after optional parameter")] #[diagnostic(code(nu::parser::required_after_optional), url(docsrs))] RequiredAfterOptional( String, diff --git a/crates/nu-parser/src/parser.rs b/crates/nu-parser/src/parser.rs index f143e8a798..d9b9cfa2e9 100644 --- a/crates/nu-parser/src/parser.rs +++ b/crates/nu-parser/src/parser.rs @@ -3230,6 +3230,7 @@ pub fn parse_signature_helper( #[allow(clippy::enum_variant_names)] enum ParseMode { ArgMode, + AfterCommaArgMode, TypeMode, DefaultValueMode, } @@ -3247,8 +3248,8 @@ pub fn parse_signature_helper( let (output, err) = lex( source, span.start, - &[b'\n', b'\r', b','], - &[b':', b'='], + &[b'\n', b'\r'], + &[b':', b'=', b','], false, ); error = error.or(err); @@ -3271,6 +3272,11 @@ pub fn parse_signature_helper( ParseMode::ArgMode => { parse_mode = ParseMode::TypeMode; } + ParseMode::AfterCommaArgMode => { + error = error.or_else(|| { + Some(ParseError::Expected("parameter or flag".into(), span)) + }); + } ParseMode::TypeMode | ParseMode::DefaultValueMode => { // We're seeing two types for the same thing for some reason, error error = @@ -3284,6 +3290,11 @@ pub fn parse_signature_helper( ParseMode::ArgMode | ParseMode::TypeMode => { parse_mode = ParseMode::DefaultValueMode; } + ParseMode::AfterCommaArgMode => { + error = error.or_else(|| { + Some(ParseError::Expected("parameter or flag".into(), span)) + }); + } ParseMode::DefaultValueMode => { // We're seeing two default values for some reason, error error = error.or_else(|| { @@ -3291,10 +3302,30 @@ pub fn parse_signature_helper( }); } } + } + // The , symbol separates params only + else if contents == b"," { + match parse_mode { + ParseMode::ArgMode => parse_mode = ParseMode::AfterCommaArgMode, + ParseMode::AfterCommaArgMode => { + error = error.or_else(|| { + Some(ParseError::Expected("parameter or flag".into(), span)) + }); + } + ParseMode::TypeMode => { + error = + error.or_else(|| Some(ParseError::Expected("type".into(), span))); + } + ParseMode::DefaultValueMode => { + error = error.or_else(|| { + Some(ParseError::Expected("default value".into(), span)) + }); + } + } } else { match parse_mode { - ParseMode::ArgMode => { - // Long flag with optional short form, e.g. --output, --age (-a) + ParseMode::ArgMode | ParseMode::AfterCommaArgMode => { + // 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. // The trailing ) is removed further down. @@ -3313,7 +3344,7 @@ pub fn parse_signature_helper( if !is_variable(&variable_name) { error = error.or_else(|| { Some(ParseError::Expected( - "valid variable name".into(), + "valid variable name for this long flag".into(), span, )) }) @@ -3374,7 +3405,7 @@ pub fn parse_signature_helper( if !is_variable(&variable_name) { error = error.or_else(|| { Some(ParseError::Expected( - "valid variable name".into(), + "valid variable name for this short flag".into(), span, )) }) @@ -3403,6 +3434,7 @@ pub fn parse_signature_helper( }); } } + parse_mode = ParseMode::ArgMode; } // Mandatory short flag, e.g. -e (must be one character) else if contents.starts_with(b"-") && contents.len() > 1 { @@ -3423,7 +3455,7 @@ pub fn parse_signature_helper( if !is_variable(&variable_name) { error = error.or_else(|| { Some(ParseError::Expected( - "valid variable name".into(), + "valid variable name for this short flag".into(), span, )) }) @@ -3441,10 +3473,16 @@ pub fn parse_signature_helper( var_id: Some(var_id), default_value: None, })); + parse_mode = ParseMode::ArgMode; } - // Short flag alias for long flag, e.g. --b, (-a) - // This is the same as --b (-a) + // Short flag alias for long flag, e.g. --b (-a) + // This is the same as the short flag in --b(-a) else if contents.starts_with(b"(-") { + if matches!(parse_mode, ParseMode::AfterCommaArgMode) { + error = error.or_else(|| { + Some(ParseError::Expected("parameter or flag".into(), span)) + }); + } let short_flag = &contents[2..]; let short_flag = if !short_flag.ends_with(b")") { @@ -3496,7 +3534,8 @@ pub fn parse_signature_helper( if !is_variable(&contents) { error = error.or_else(|| { Some(ParseError::Expected( - "valid variable name".into(), + "valid variable name for this optional parameter" + .into(), span, )) }) @@ -3514,7 +3553,8 @@ pub fn parse_signature_helper( default_value: None, }, false, - )) + )); + parse_mode = ParseMode::ArgMode; } // Rest param else if let Some(contents) = contents.strip_prefix(b"...") { @@ -3524,7 +3564,7 @@ pub fn parse_signature_helper( if !is_variable(&contents_vec) { error = error.or_else(|| { Some(ParseError::Expected( - "valid variable name".into(), + "valid variable name for this rest parameter".into(), span, )) }) @@ -3540,6 +3580,7 @@ pub fn parse_signature_helper( var_id: Some(var_id), default_value: None, })); + parse_mode = ParseMode::ArgMode; } // Normal param else { @@ -3549,7 +3590,7 @@ pub fn parse_signature_helper( if !is_variable(&contents_vec) { error = error.or_else(|| { Some(ParseError::Expected( - "valid variable name".into(), + "valid variable name for this parameter".into(), span, )) }) @@ -3568,7 +3609,8 @@ pub fn parse_signature_helper( default_value: None, }, true, - )) + )); + parse_mode = ParseMode::ArgMode; } } ParseMode::TypeMode => { @@ -3648,7 +3690,7 @@ pub fn parse_signature_helper( Arg::RestPositional(..) => { error = error.or_else(|| { Some(ParseError::AssignmentMismatch( - "Rest parameter given default value".into(), + "Rest parameter was given a default value".into(), "can't have default value".into(), expression.span, )) @@ -3680,8 +3722,12 @@ pub fn parse_signature_helper( if t != &expression_ty { error = error.or_else(|| { Some(ParseError::AssignmentMismatch( - "Default value wrong type".into(), - format!("default value not {}", t), + "Default value is the wrong type" + .into(), + format!( + "default value should be {}", + t + ), expression_span, )) }) diff --git a/src/tests/test_engine.rs b/src/tests/test_engine.rs index 49f19c164e..dfed46dd7e 100644 --- a/src/tests/test_engine.rs +++ b/src/tests/test_engine.rs @@ -316,7 +316,10 @@ fn default_value11() -> TestResult { #[test] fn default_value12() -> TestResult { - fail_test(r#"def foo [--x:int = "a"] { $x }"#, "default value not int") + fail_test( + r#"def foo [--x:int = "a"] { $x }"#, + "default value should be int", + ) } #[test]