From 01891d637d7b28cff1985bc3211dc4f544996c64 Mon Sep 17 00:00:00 2001 From: Devyn Cairns Date: Sun, 21 Jul 2024 01:32:36 -0700 Subject: [PATCH] Make parsing for unknown args in known externals like normal external calls (#13414) # Description This corrects the parsing of unknown arguments provided to known externals to behave exactly like external arguments passed to normal external calls. I've done this by adding a `SyntaxShape::ExternalArgument` which triggers the same parsing rules. Because I didn't like how the highlighting looked, I modified the flattener to emit `ExternalArg` flat shapes for arguments that have that syntax shape and are plain strings/globs. This is the same behavior that external calls have. Aside from passing the tests, I've also checked manually that the completer seems to work adequately. I can confirm that specified positional arguments get completion according to their specified type (including custom completions), and then anything remaining gets filepath style completion, as you'd expect from an external command. Thanks to @OJarrisonn for originally finding this issue. # User-Facing Changes - Unknown args are now parsed according to their specified syntax shape, rather than `Any`. This may be a breaking change, though I think it's extremely unlikely in practice. - The unspecified arguments of known externals are now highlighted / flattened identically to normal external arguments, which makes it more clear how they're being interpreted, and should help the completer function properly. - Known externals now have an implicit rest arg if not specified named `args`, with a syntax shape of `ExternalArgument`. # Tests + Formatting Tests added for the new behaviour. Some old tests had to be corrected to match. - :green_circle: `toolkit fmt` - :green_circle: `toolkit clippy` - :green_circle: `toolkit test` - :green_circle: `toolkit test stdlib` # After Submitting - [ ] release notes (bugfix, and debatable whether it's a breaking change) --- crates/nu-parser/src/flatten.rs | 46 +++++++++++++++++++++-- crates/nu-parser/src/parse_keywords.rs | 10 +++++ crates/nu-parser/src/parser.rs | 51 +++++++++++++++++++++----- crates/nu-protocol/src/syntax_shape.rs | 8 ++++ tests/repl/test_custom_commands.rs | 8 +++- tests/repl/test_known_external.rs | 36 ++++++++++++++++++ 6 files changed, 144 insertions(+), 15 deletions(-) diff --git a/crates/nu-parser/src/flatten.rs b/crates/nu-parser/src/flatten.rs index 8a05edaef4..2f71ccea28 100644 --- a/crates/nu-parser/src/flatten.rs +++ b/crates/nu-parser/src/flatten.rs @@ -5,7 +5,7 @@ use nu_protocol::{ RecordItem, }, engine::StateWorkingSet, - DeclId, Span, VarId, + DeclId, Span, SyntaxShape, VarId, }; use std::fmt::{Display, Formatter, Result}; @@ -166,6 +166,22 @@ fn flatten_pipeline_element_into( } } +fn flatten_positional_arg_into( + working_set: &StateWorkingSet, + positional: &Expression, + shape: &SyntaxShape, + output: &mut Vec<(Span, FlatShape)>, +) { + if matches!(shape, SyntaxShape::ExternalArgument) + && matches!(positional.expr, Expr::String(..) | Expr::GlobPattern(..)) + { + // Make known external arguments look more like external arguments + output.push((positional.span, FlatShape::ExternalArg)); + } else { + flatten_expression_into(working_set, positional, output) + } +} + fn flatten_expression_into( working_set: &StateWorkingSet, expr: &Expression, @@ -249,16 +265,40 @@ fn flatten_expression_into( } } Expr::Call(call) => { + let decl = working_set.get_decl(call.decl_id); + if call.head.end != 0 { // Make sure we don't push synthetic calls output.push((call.head, FlatShape::InternalCall(call.decl_id))); } + // Follow positional arguments from the signature. + let signature = decl.signature(); + let mut positional_args = signature + .required_positional + .iter() + .chain(&signature.optional_positional); + let arg_start = output.len(); for arg in &call.arguments { match arg { - Argument::Positional(positional) | Argument::Unknown(positional) => { - flatten_expression_into(working_set, positional, output) + Argument::Positional(positional) => { + let positional_arg = positional_args.next(); + let shape = positional_arg + .or(signature.rest_positional.as_ref()) + .map(|arg| &arg.shape) + .unwrap_or(&SyntaxShape::Any); + + flatten_positional_arg_into(working_set, positional, shape, output) + } + Argument::Unknown(positional) => { + let shape = signature + .rest_positional + .as_ref() + .map(|arg| &arg.shape) + .unwrap_or(&SyntaxShape::Any); + + flatten_positional_arg_into(working_set, positional, shape, output) } Argument::Named(named) => { if named.0.span.end != 0 { diff --git a/crates/nu-parser/src/parse_keywords.rs b/crates/nu-parser/src/parse_keywords.rs index 4ac1d20699..7c41f8a78d 100644 --- a/crates/nu-parser/src/parse_keywords.rs +++ b/crates/nu-parser/src/parse_keywords.rs @@ -783,6 +783,16 @@ pub fn parse_extern( working_set.get_block_mut(block_id).signature = signature; } } else { + if signature.rest_positional.is_none() { + // Make sure that a known external takes rest args with ExternalArgument + // shape + *signature = signature.rest( + "args", + SyntaxShape::ExternalArgument, + "all other arguments to the command", + ); + } + let decl = KnownExternal { name: external_name, usage, diff --git a/crates/nu-parser/src/parser.rs b/crates/nu-parser/src/parser.rs index bc48b9bd0c..1fb0aee01d 100644 --- a/crates/nu-parser/src/parser.rs +++ b/crates/nu-parser/src/parser.rs @@ -221,6 +221,22 @@ pub(crate) fn check_call( } } +/// Parses an unknown argument for the given signature. This handles the parsing as appropriate to +/// the rest type of the command. +fn parse_unknown_arg( + working_set: &mut StateWorkingSet, + span: Span, + signature: &Signature, +) -> Expression { + let shape = signature + .rest_positional + .as_ref() + .map(|arg| arg.shape.clone()) + .unwrap_or(SyntaxShape::Any); + + parse_value(working_set, span, &shape) +} + /// Parses a string in the arg or head position of an external call. /// /// If the string begins with `r#`, it is parsed as a raw string. If it doesn't contain any quotes @@ -427,11 +443,7 @@ fn parse_external_string(working_set: &mut StateWorkingSet, span: Span) -> Expre fn parse_external_arg(working_set: &mut StateWorkingSet, span: Span) -> ExternalArgument { let contents = working_set.get_span_contents(span); - if contents.starts_with(b"$") || contents.starts_with(b"(") { - ExternalArgument::Regular(parse_dollar_expr(working_set, span)) - } else if contents.starts_with(b"[") { - ExternalArgument::Regular(parse_list_expression(working_set, span, &SyntaxShape::Any)) - } else if contents.len() > 3 + if contents.len() > 3 && contents.starts_with(b"...") && (contents[3] == b'$' || contents[3] == b'[' || contents[3] == b'(') { @@ -441,7 +453,19 @@ fn parse_external_arg(working_set: &mut StateWorkingSet, span: Span) -> External &SyntaxShape::List(Box::new(SyntaxShape::Any)), )) } else { - ExternalArgument::Regular(parse_external_string(working_set, span)) + ExternalArgument::Regular(parse_regular_external_arg(working_set, span)) + } +} + +fn parse_regular_external_arg(working_set: &mut StateWorkingSet, span: Span) -> Expression { + let contents = working_set.get_span_contents(span); + + if contents.starts_with(b"$") || contents.starts_with(b"(") { + parse_dollar_expr(working_set, span) + } else if contents.starts_with(b"[") { + parse_list_expression(working_set, span, &SyntaxShape::Any) + } else { + parse_external_string(working_set, span) } } @@ -998,7 +1022,7 @@ pub fn parse_internal_call( && signature.allows_unknown_args { working_set.parse_errors.truncate(starting_error_count); - let arg = parse_value(working_set, arg_span, &SyntaxShape::Any); + let arg = parse_unknown_arg(working_set, arg_span, &signature); call.add_unknown(arg); } else { @@ -1040,7 +1064,7 @@ pub fn parse_internal_call( && signature.allows_unknown_args { working_set.parse_errors.truncate(starting_error_count); - let arg = parse_value(working_set, arg_span, &SyntaxShape::Any); + let arg = parse_unknown_arg(working_set, arg_span, &signature); call.add_unknown(arg); } else { @@ -1135,6 +1159,10 @@ pub fn parse_internal_call( call.add_positional(Expression::garbage(working_set, arg_span)); } else { let rest_shape = match &signature.rest_positional { + Some(arg) if matches!(arg.shape, SyntaxShape::ExternalArgument) => { + // External args aren't parsed inside lists in spread position. + SyntaxShape::Any + } Some(arg) => arg.shape.clone(), None => SyntaxShape::Any, }; @@ -1196,7 +1224,7 @@ pub fn parse_internal_call( call.add_positional(arg); positional_idx += 1; } else if signature.allows_unknown_args { - let arg = parse_value(working_set, arg_span, &SyntaxShape::Any); + let arg = parse_unknown_arg(working_set, arg_span, &signature); call.add_unknown(arg); } else { @@ -4670,7 +4698,8 @@ pub fn parse_value( | SyntaxShape::Signature | SyntaxShape::Filepath | SyntaxShape::String - | SyntaxShape::GlobPattern => {} + | SyntaxShape::GlobPattern + | SyntaxShape::ExternalArgument => {} _ => { working_set.error(ParseError::Expected("non-[] value", span)); return Expression::garbage(working_set, span); @@ -4747,6 +4776,8 @@ pub fn parse_value( Expression::garbage(working_set, span) } + SyntaxShape::ExternalArgument => parse_regular_external_arg(working_set, span), + SyntaxShape::Any => { if bytes.starts_with(b"[") { //parse_value(working_set, span, &SyntaxShape::Table) diff --git a/crates/nu-protocol/src/syntax_shape.rs b/crates/nu-protocol/src/syntax_shape.rs index bece58d59a..66bd77ddca 100644 --- a/crates/nu-protocol/src/syntax_shape.rs +++ b/crates/nu-protocol/src/syntax_shape.rs @@ -47,6 +47,12 @@ pub enum SyntaxShape { /// A general expression, eg `1 + 2` or `foo --bar` Expression, + /// A (typically) string argument that follows external command argument parsing rules. + /// + /// Filepaths are expanded if unquoted, globs are allowed, and quotes embedded within unknown + /// args are unquoted. + ExternalArgument, + /// A filepath is allowed Filepath, @@ -145,6 +151,7 @@ impl SyntaxShape { SyntaxShape::DateTime => Type::Date, SyntaxShape::Duration => Type::Duration, SyntaxShape::Expression => Type::Any, + SyntaxShape::ExternalArgument => Type::Any, SyntaxShape::Filepath => Type::String, SyntaxShape::Directory => Type::String, SyntaxShape::Float => Type::Float, @@ -238,6 +245,7 @@ impl Display for SyntaxShape { SyntaxShape::Signature => write!(f, "signature"), SyntaxShape::MatchBlock => write!(f, "match-block"), SyntaxShape::Expression => write!(f, "expression"), + SyntaxShape::ExternalArgument => write!(f, "external-argument"), SyntaxShape::Boolean => write!(f, "bool"), SyntaxShape::Error => write!(f, "error"), SyntaxShape::CompleterWrapper(x, _) => write!(f, "completable<{x}>"), diff --git a/tests/repl/test_custom_commands.rs b/tests/repl/test_custom_commands.rs index 1f36b0e90c..1710b35913 100644 --- a/tests/repl/test_custom_commands.rs +++ b/tests/repl/test_custom_commands.rs @@ -186,8 +186,12 @@ fn help_present_in_def() -> TestResult { #[test] fn help_not_present_in_extern() -> TestResult { run_test( - "module test {export extern \"git fetch\" []}; use test `git fetch`; help git fetch | ansi strip", - "Usage:\n > git fetch", + r#" + module test {export extern "git fetch" []}; + use test `git fetch`; + help git fetch | find help | to text | ansi strip + "#, + "", ) } diff --git a/tests/repl/test_known_external.rs b/tests/repl/test_known_external.rs index 96ea677d37..c0278cc863 100644 --- a/tests/repl/test_known_external.rs +++ b/tests/repl/test_known_external.rs @@ -137,3 +137,39 @@ fn known_external_aliased_subcommand_from_module() -> TestResult { String::from_utf8(output.stdout)?.trim(), ) } + +#[test] +fn known_external_arg_expansion() -> TestResult { + run_test( + r#" + extern echo []; + echo ~/foo + "#, + &dirs::home_dir() + .expect("can't find home dir") + .join("foo") + .to_string_lossy(), + ) +} + +#[test] +fn known_external_arg_quoted_no_expand() -> TestResult { + run_test( + r#" + extern echo []; + echo "~/foo" + "#, + "~/foo", + ) +} + +#[test] +fn known_external_arg_internally_quoted_options() -> TestResult { + run_test( + r#" + extern echo []; + echo --option="test" + "#, + "--option=test", + ) +}