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.

- 🟢 `toolkit fmt`
- 🟢 `toolkit clippy`
- 🟢 `toolkit test`
- 🟢 `toolkit test stdlib`

# After Submitting
- [ ] release notes (bugfix, and debatable whether it's a breaking
change)

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 {

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,

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)

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}>"),

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
+        "#,
+        "",
     )
 }
 

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",
+    )
+}