extern/nushell
1
0
Fork 1
mirror of https://github.com/nushell/nushell.git synced 2025-01-10 16:28:50 +01:00
Code Issues Packages Projects Releases Wiki Activity

def: make various punctuation misuses into errors (#7624)

Browse Source 
Closes https://github.com/nushell/nushell/issues/7604
This commit is contained in:
Leon 2022-12-31 21:18:53 +10:00 committed by GitHub
parent 9b88ea5b60
commit 7aa2a57434
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
4 changed files with 159 additions and 19 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

91
crates/nu-command/tests/commands/def.rs
View File

@ -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] #[test]
fn def_errors_with_multiple_short_flags() { fn def_errors_with_multiple_short_flags() {
let actual = nu!( let actual = nu!(
@ -31,6 +49,79 @@ fn def_errors_with_multiple_short_flags() {
assert!(actual.err.contains("expected only one short flag")); 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] #[test]
fn def_fails_with_invalid_name() { fn def_fails_with_invalid_name() {
let err_msg = "command name can't be a number, a filesize, or contain a hash # or caret ^"; let err_msg = "command name can't be a number, a filesize, or contain a hash # or caret ^";

2
crates/nu-parser/src/errors.rs
View File

@ -19,7 +19,7 @@ pub enum ParseError {
#[diagnostic(code(nu::parser::extra_positional), url(docsrs), help("Usage: {0}"))] #[diagnostic(code(nu::parser::extra_positional), url(docsrs), help("Usage: {0}"))]
ExtraPositional(String, #[label = "extra positional argument"] Span), 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))] #[diagnostic(code(nu::parser::required_after_optional), url(docsrs))]
RequiredAfterOptional( RequiredAfterOptional(
String, String,

80
crates/nu-parser/src/parser.rs
View File

@ -3230,6 +3230,7 @@ pub fn parse_signature_helper(
#[allow(clippy::enum_variant_names)] #[allow(clippy::enum_variant_names)]
enum ParseMode { enum ParseMode {
ArgMode, ArgMode,
AfterCommaArgMode,
TypeMode, TypeMode,
DefaultValueMode, DefaultValueMode,
} }
@ -3247,8 +3248,8 @@ pub fn parse_signature_helper(
let (output, err) = lex( let (output, err) = lex(
source, source,
span.start, span.start,
&[b'\n', b'\r', b','], &[b'\n', b'\r'],
&[b':', b'='], &[b':', b'=', b','],
false, false,
); );
error = error.or(err); error = error.or(err);
@ -3271,6 +3272,11 @@ pub fn parse_signature_helper(
ParseMode::ArgMode => { ParseMode::ArgMode => {
parse_mode = ParseMode::TypeMode; parse_mode = ParseMode::TypeMode;
} }
ParseMode::AfterCommaArgMode => {
error = error.or_else(|| {
Some(ParseError::Expected("parameter or flag".into(), span))
});
}
ParseMode::TypeMode | ParseMode::DefaultValueMode => { ParseMode::TypeMode | ParseMode::DefaultValueMode => {
// We're seeing two types for the same thing for some reason, error // We're seeing two types for the same thing for some reason, error
error = error =
@ -3284,6 +3290,11 @@ pub fn parse_signature_helper(
ParseMode::ArgMode | ParseMode::TypeMode => { ParseMode::ArgMode | ParseMode::TypeMode => {
parse_mode = ParseMode::DefaultValueMode; parse_mode = ParseMode::DefaultValueMode;
} }
ParseMode::AfterCommaArgMode => {
error = error.or_else(|| {
Some(ParseError::Expected("parameter or flag".into(), span))
});
}
ParseMode::DefaultValueMode => { ParseMode::DefaultValueMode => {
// We're seeing two default values for some reason, error // We're seeing two default values for some reason, error
error = error.or_else(|| { 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 { } else {
match parse_mode { match parse_mode {
ParseMode::ArgMode => { ParseMode::ArgMode | ParseMode::AfterCommaArgMode => {
// Long flag with optional short form, e.g. --output, --age (-a) // Long flag with optional short form following with no whitespace, e.g. --output, --age(-a)
if contents.starts_with(b"--") && contents.len() > 2 { if contents.starts_with(b"--") && contents.len() > 2 {
// Split the long flag from the short flag with the ( character as delimiter. // Split the long flag from the short flag with the ( character as delimiter.
// The trailing ) is removed further down. // The trailing ) is removed further down.
@ -3313,7 +3344,7 @@ pub fn parse_signature_helper(
if !is_variable(&variable_name) { if !is_variable(&variable_name) {
error = error.or_else(|| { error = error.or_else(|| {
Some(ParseError::Expected( Some(ParseError::Expected(
"valid variable name".into(), "valid variable name for this long flag".into(),
span, span,
)) ))
}) })
@ -3374,7 +3405,7 @@ pub fn parse_signature_helper(
if !is_variable(&variable_name) { if !is_variable(&variable_name) {
error = error.or_else(|| { error = error.or_else(|| {
Some(ParseError::Expected( Some(ParseError::Expected(
"valid variable name".into(), "valid variable name for this short flag".into(),
span, span,
)) ))
}) })
@ -3403,6 +3434,7 @@ pub fn parse_signature_helper(
}); });
} }
} }
parse_mode = ParseMode::ArgMode;
} }
// Mandatory short flag, e.g. -e (must be one character) // Mandatory short flag, e.g. -e (must be one character)
else if contents.starts_with(b"-") && contents.len() > 1 { else if contents.starts_with(b"-") && contents.len() > 1 {
@ -3423,7 +3455,7 @@ pub fn parse_signature_helper(
if !is_variable(&variable_name) { if !is_variable(&variable_name) {
error = error.or_else(|| { error = error.or_else(|| {
Some(ParseError::Expected( Some(ParseError::Expected(
"valid variable name".into(), "valid variable name for this short flag".into(),
span, span,
)) ))
}) })
@ -3441,10 +3473,16 @@ pub fn parse_signature_helper(
var_id: Some(var_id), var_id: Some(var_id),
default_value: None, default_value: None,
})); }));
parse_mode = ParseMode::ArgMode;
} }
// Short flag alias for long flag, e.g. --b, (-a) // Short flag alias for long flag, e.g. --b (-a)
// This is the same as --b (-a) // This is the same as the short flag in --b(-a)
else if contents.starts_with(b"(-") { 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 = &contents[2..];
let short_flag = if !short_flag.ends_with(b")") { let short_flag = if !short_flag.ends_with(b")") {
@ -3496,7 +3534,8 @@ pub fn parse_signature_helper(
if !is_variable(&contents) { if !is_variable(&contents) {
error = error.or_else(|| { error = error.or_else(|| {
Some(ParseError::Expected( Some(ParseError::Expected(
"valid variable name".into(), "valid variable name for this optional parameter"
.into(),
span, span,
)) ))
}) })
@ -3514,7 +3553,8 @@ pub fn parse_signature_helper(
default_value: None, default_value: None,
}, },
false, false,
)) ));
parse_mode = ParseMode::ArgMode;
} }
// Rest param // Rest param
else if let Some(contents) = contents.strip_prefix(b"...") { else if let Some(contents) = contents.strip_prefix(b"...") {
@ -3524,7 +3564,7 @@ pub fn parse_signature_helper(
if !is_variable(&contents_vec) { if !is_variable(&contents_vec) {
error = error.or_else(|| { error = error.or_else(|| {
Some(ParseError::Expected( Some(ParseError::Expected(
"valid variable name".into(), "valid variable name for this rest parameter".into(),
span, span,
)) ))
}) })
@ -3540,6 +3580,7 @@ pub fn parse_signature_helper(
var_id: Some(var_id), var_id: Some(var_id),
default_value: None, default_value: None,
})); }));
parse_mode = ParseMode::ArgMode;
} }
// Normal param // Normal param
else { else {
@ -3549,7 +3590,7 @@ pub fn parse_signature_helper(
if !is_variable(&contents_vec) { if !is_variable(&contents_vec) {
error = error.or_else(|| { error = error.or_else(|| {
Some(ParseError::Expected( Some(ParseError::Expected(
"valid variable name".into(), "valid variable name for this parameter".into(),
span, span,
)) ))
}) })
@ -3568,7 +3609,8 @@ pub fn parse_signature_helper(
default_value: None, default_value: None,
}, },
true, true,
)) ));
parse_mode = ParseMode::ArgMode;
} }
} }
ParseMode::TypeMode => { ParseMode::TypeMode => {
@ -3648,7 +3690,7 @@ pub fn parse_signature_helper(
Arg::RestPositional(..) => { Arg::RestPositional(..) => {
error = error.or_else(|| { error = error.or_else(|| {
Some(ParseError::AssignmentMismatch( Some(ParseError::AssignmentMismatch(
"Rest parameter given default value".into(), "Rest parameter was given a default value".into(),
"can't have default value".into(), "can't have default value".into(),
expression.span, expression.span,
)) ))
@ -3680,8 +3722,12 @@ pub fn parse_signature_helper(
if t != &expression_ty { if t != &expression_ty {
error = error.or_else(|| { error = error.or_else(|| {
Some(ParseError::AssignmentMismatch( Some(ParseError::AssignmentMismatch(
"Default value wrong type".into(), "Default value is the wrong type"
format!("default value not {}", t), .into(),
format!(
"default value should be {}",
t
),
expression_span, expression_span,
)) ))
}) })

5
src/tests/test_engine.rs
View File

@ -316,7 +316,10 @@ fn default_value11() -> TestResult {
#[test] #[test]
fn default_value12() -> TestResult { 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] #[test]