From b50903cf58a067b78815be620372e8a3d5cbdcae Mon Sep 17 00:00:00 2001 From: Devyn Cairns Date: Mon, 3 Jun 2024 00:28:45 -0700 Subject: [PATCH] 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 | 35 +++-- crates/nu-parser/tests/test_parser.rs | 215 ++++++++++++++++++++++++++ 2 files changed, 239 insertions(+), 11 deletions(-) diff --git a/crates/nu-parser/src/parser.rs b/crates/nu-parser/src/parser.rs index ad5e7d4f42..830d8b81e0 100644 --- a/crates/nu-parser/src/parser.rs +++ b/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(contents, span); - if let Some(err) = err { - working_set.error(err) - } - String::from_utf8_lossy(&contents).to_string() - } else { - String::from_utf8_lossy(contents).to_string() - }; + let (contents, err) = unescape_string_preserving_quotes(contents, span); + if let Some(err) = err { + working_set.error(err); + } 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 (String, Option) { + 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"); diff --git a/crates/nu-parser/tests/test_parser.rs b/crates/nu-parser/tests/test_parser.rs index ef5d968280..2646b3cc90 100644 --- a/crates/nu-parser/tests/test_parser.rs +++ b/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();