Fix external command name parsing with backslashes, and add tests (#13027)

# Description

Fixes #13016 and adds tests for many variations of external call
parsing.

I just realized @kubouch took a crack at this too (#13022) so really
whichever is better, but I think the
tests are a good addition.

crates/nu-parser/src/parser.rs

@@ -16,6 +16,7 @@ use nu_protocol::{
     IN_VARIABLE_ID,
 };
 use std::{
+    borrow::Cow,
     collections::{HashMap, HashSet},
     num::ParseIntError,
     str,
@@ -241,15 +242,10 @@ fn parse_external_arg(working_set: &mut StateWorkingSet, span: Span) -> External
         ))
     } else {
         // Eval stage trims the quotes, so we don't have to do the same thing when parsing.
-        let contents = if contents.starts_with(b"\"") {
+        let (contents, err) = unescape_string_preserving_quotes(contents, span);
-            let (contents, err) = unescape_string(contents, span);
+        if let Some(err) = err {
-            if let Some(err) = err {
+            working_set.error(err);
-                working_set.error(err)
+        }
-            }
-            String::from_utf8_lossy(&contents).to_string()
-        } else {
-            String::from_utf8_lossy(contents).to_string()
-        };
 
         ExternalArgument::Regular(Expression {
             expr: Expr::String(contents),
@@ -279,13 +275,13 @@ pub fn parse_external_call(working_set: &mut StateWorkingSet, spans: &[Span]) ->
         Box::new(arg)
     } else {
         // Eval stage will unquote the string, so we don't bother with that here
-        let (contents, err) = unescape_string(&head_contents, head_span);
+        let (contents, err) = unescape_string_preserving_quotes(&head_contents, head_span);
         if let Some(err) = err {
             working_set.error(err)
         }
 
         Box::new(Expression {
-            expr: Expr::String(String::from_utf8_lossy(&contents).into_owned()),
+            expr: Expr::String(contents),
             span: head_span,
             ty: Type::String,
             custom_completion: None,
@@ -2699,6 +2695,23 @@ pub fn unescape_unquote_string(bytes: &[u8], span: Span) -> (String, Option<Pars
     }
 }
 
+/// XXX: This is here temporarily as a patch, but we should replace this with properly representing
+/// the quoted state of a string in the AST
+fn unescape_string_preserving_quotes(bytes: &[u8], span: Span) -> (String, Option<ParseError>) {
+    let (bytes, err) = if bytes.starts_with(b"\"") {
+        let (bytes, err) = unescape_string(bytes, span);
+        (Cow::Owned(bytes), err)
+    } else {
+        (Cow::Borrowed(bytes), None)
+    };
+
+    // The original code for args used lossy conversion here, even though that's not what we
+    // typically use for strings. Revisit whether that's actually desirable later, but don't
+    // want to introduce a breaking change for this patch.
+    let token = String::from_utf8_lossy(&bytes).into_owned();
+    (token, err)
+}
+
 pub fn parse_string(working_set: &mut StateWorkingSet, span: Span) -> Expression {
     trace!("parsing: string");
 

crates/nu-parser/tests/test_parser.rs

@@ -694,6 +694,221 @@ pub fn parse_call_missing_req_flag() {
     ));
 }
 
+#[rstest]
+#[case("foo-external-call", "foo-external-call", "bare word")]
+#[case("^foo-external-call", "foo-external-call", "bare word with caret")]
+#[case(
+    "foo/external-call",
+    "foo/external-call",
+    "bare word with forward slash"
+)]
+#[case(
+    "^foo/external-call",
+    "foo/external-call",
+    "bare word with forward slash and caret"
+)]
+#[case(r"foo\external-call", r"foo\external-call", "bare word with backslash")]
+#[case(
+    r"^foo\external-call",
+    r"foo\external-call",
+    "bare word with backslash and caret"
+)]
+#[case(
+    "^'foo external call'",
+    "'foo external call'",
+    "single quote with caret"
+)]
+#[case(
+    "^'foo/external call'",
+    "'foo/external call'",
+    "single quote with forward slash and caret"
+)]
+#[case(
+    r"^'foo\external call'",
+    r"'foo\external call'",
+    "single quote with backslash and caret"
+)]
+#[case(
+    r#"^"foo external call""#,
+    r#""foo external call""#,
+    "double quote with caret"
+)]
+#[case(
+    r#"^"foo/external call""#,
+    r#""foo/external call""#,
+    "double quote with forward slash and caret"
+)]
+#[case(
+    r#"^"foo\\external call""#,
+    r#""foo\external call""#,
+    "double quote with backslash and caret"
+)]
+#[case("`foo external call`", "`foo external call`", "backtick quote")]
+#[case(
+    "^`foo external call`",
+    "`foo external call`",
+    "backtick quote with caret"
+)]
+#[case(
+    "`foo/external call`",
+    "`foo/external call`",
+    "backtick quote with forward slash"
+)]
+#[case(
+    "^`foo/external call`",
+    "`foo/external call`",
+    "backtick quote with forward slash and caret"
+)]
+#[case(
+    r"^`foo\external call`",
+    r"`foo\external call`",
+    "backtick quote with backslash"
+)]
+#[case(
+    r"^`foo\external call`",
+    r"`foo\external call`",
+    "backtick quote with backslash and caret"
+)]
+fn test_external_call_name(#[case] input: &str, #[case] expected: &str, #[case] tag: &str) {
+    let engine_state = EngineState::new();
+    let mut working_set = StateWorkingSet::new(&engine_state);
+    let block = parse(&mut working_set, None, input.as_bytes(), true);
+    assert!(
+        working_set.parse_errors.is_empty(),
+        "{tag}: errors: {:?}",
+        working_set.parse_errors
+    );
+
+    let pipeline = &block.pipelines[0];
+    assert_eq!(1, pipeline.len());
+    let element = &pipeline.elements[0];
+    match &element.expr.expr {
+        Expr::ExternalCall(name, args) => {
+            match &name.expr {
+                Expr::String(string) => {
+                    assert_eq!(expected, string);
+                }
+                other => {
+                    panic!("{tag}: Unexpected expression in command name position: {other:?}");
+                }
+            }
+            assert_eq!(0, args.len());
+        }
+        other => {
+            panic!("{tag}: Unexpected expression in pipeline: {other:?}");
+        }
+    }
+}
+
+#[rstest]
+#[case("^foo bar-baz", "bar-baz", "bare word")]
+#[case("^foo bar/baz", "bar/baz", "bare word with forward slash")]
+#[case(r"^foo bar\baz", r"bar\baz", "bare word with backslash")]
+#[case("^foo 'bar baz'", "'bar baz'", "single quote")]
+#[case("foo 'bar/baz'", "'bar/baz'", "single quote with forward slash")]
+#[case(r"foo 'bar\baz'", r"'bar\baz'", "single quote with backslash")]
+#[case(r#"^foo "bar baz""#, r#""bar baz""#, "double quote")]
+#[case(r#"^foo "bar/baz""#, r#""bar/baz""#, "double quote with forward slash")]
+#[case(r#"^foo "bar\\baz""#, r#""bar\baz""#, "double quote with backslash")]
+#[case("^foo `bar baz`", "`bar baz`", "backtick quote")]
+#[case("^foo `bar/baz`", "`bar/baz`", "backtick quote with forward slash")]
+#[case(r"^foo `bar\baz`", r"`bar\baz`", "backtick quote with backslash")]
+fn test_external_call_argument_regular(
+    #[case] input: &str,
+    #[case] expected: &str,
+    #[case] tag: &str,
+) {
+    let engine_state = EngineState::new();
+    let mut working_set = StateWorkingSet::new(&engine_state);
+    let block = parse(&mut working_set, None, input.as_bytes(), true);
+    assert!(
+        working_set.parse_errors.is_empty(),
+        "{tag}: errors: {:?}",
+        working_set.parse_errors
+    );
+
+    let pipeline = &block.pipelines[0];
+    assert_eq!(1, pipeline.len());
+    let element = &pipeline.elements[0];
+    match &element.expr.expr {
+        Expr::ExternalCall(name, args) => {
+            match &name.expr {
+                Expr::String(string) => {
+                    assert_eq!("foo", string, "{tag}: incorrect name");
+                }
+                other => {
+                    panic!("{tag}: Unexpected expression in command name position: {other:?}");
+                }
+            }
+            assert_eq!(1, args.len());
+            match &args[0] {
+                ExternalArgument::Regular(expr) => match &expr.expr {
+                    Expr::String(string) => {
+                        assert_eq!(expected, string, "{tag}: incorrect arg");
+                    }
+                    other => {
+                        panic!("Unexpected expression in command arg position: {other:?}")
+                    }
+                },
+                other @ ExternalArgument::Spread(..) => {
+                    panic!("Unexpected external spread argument in command arg position: {other:?}")
+                }
+            }
+        }
+        other => {
+            panic!("{tag}: Unexpected expression in pipeline: {other:?}");
+        }
+    }
+}
+
+#[test]
+fn test_external_call_argument_spread() {
+    let engine_state = EngineState::new();
+    let mut working_set = StateWorkingSet::new(&engine_state);
+    let block = parse(&mut working_set, None, b"^foo ...[a b c]", true);
+    assert!(
+        working_set.parse_errors.is_empty(),
+        "errors: {:?}",
+        working_set.parse_errors
+    );
+
+    let pipeline = &block.pipelines[0];
+    assert_eq!(1, pipeline.len());
+    let element = &pipeline.elements[0];
+    match &element.expr.expr {
+        Expr::ExternalCall(name, args) => {
+            match &name.expr {
+                Expr::String(string) => {
+                    assert_eq!("foo", string, "incorrect name");
+                }
+                other => {
+                    panic!("Unexpected expression in command name position: {other:?}");
+                }
+            }
+            assert_eq!(1, args.len());
+            match &args[0] {
+                ExternalArgument::Spread(expr) => match &expr.expr {
+                    Expr::List(items) => {
+                        assert_eq!(3, items.len());
+                        // that's good enough, don't really need to go so deep into it...
+                    }
+                    other => {
+                        panic!("Unexpected expression in command arg position: {other:?}")
+                    }
+                },
+                other @ ExternalArgument::Regular(..) => {
+                    panic!(
+                        "Unexpected external regular argument in command arg position: {other:?}"
+                    )
+                }
+            }
+        }
+        other => {
+            panic!("Unexpected expression in pipeline: {other:?}");
+        }
+    }
+}
+
 #[test]
 fn test_nothing_comparison_eq() {
     let engine_state = EngineState::new();