fix(parser): try string last in oneof so oneof<string, list<string>> parses [a b] as a list (#19238)

## Description

The order of the types in a `oneof` changed what a literal argument
parsed as:

```nushell
> def show [x: oneof<string, list<string>>] { $x | describe }
> show [foo bar]
string
> def show [x: oneof<list<string>, string>] { $x | describe }
> show [foo bar]
list<string>
```

`parse_oneof` returns the first shape that parses without error, and the
`string` shape accepts nearly any single token as a bare word. With
`string` listed first, every type after it was unreachable from a
literal:

| `oneof<string, X>` | literal | parsed as (before) | parsed as (after)
|
| --- | --- | --- | --- |
| `list<string>` | `[foo bar]` | `"[foo bar]"` | `[foo, bar]` |
| `table` | `[[a]; [1]]` | `"[[a]; [1]]"` | `table<a: int>` |
| `int` | `5` | `"5"` | `5` |
| `number` | `1.5` | `"1.5"` | `1.5` |
| `duration` | `5sec` | `"5sec"` | `5sec` |
| `filesize` | `5kb` | `"5kb"` | `5kb` |
| `range` | `1..5` | `"1..5"` | `1..5` |
| `datetime` | `2024-01-01` | `"2024-01-01"` | `2024-01-01` |
| `binary` | `0x[ff]` | `"0x[ff]"` | `0x[FF]` |

`oneof<path, list<path>>` and `oneof<glob, list<glob>>` had the same
problem, and so did parameter defaults and list elements: `[x:
oneof<string, int> = 5]` defaulted to the string `"5"`, and
`list<oneof<string, int>>` parsed `[1 a]` as `["1", a]`. Both now give
ints. The type check doesn't depend on order, so `let x = [foo bar];
show $x` already passed a list.

## User-facing changes (Release notes)

In `oneof<...>` parameter types, `string`, `path`, `directory`, `glob`,
`cell-path`, `external_arg` and `any` no longer claim a literal that
another listed type matches, whatever order the types are listed in.
`oneof<string, list<string>>` now parses `[foo bar]` as a list, and
`oneof<string, int>` parses `5` as the int `5` instead of the string
`"5"`. A bare word that no other listed type matches still parses as a
string, and arguments starting with `$`, `(` or `{` parse as before.

A literal that looks like one of the other types now gets that type, so
quote it if you need the text. With `oneof<string, int>`, `007` arrives
as `7` and `0x10` as `16`. With `oneof<string, number>`, `1.10` arrives
as `1.1` and `nan` as a float. With `oneof<string, list<string>>`, a
character class like `[0-9]` arrives as the list `["0-9"]`. This also
applies to `extern` parameters, which pass the converted value to the
external command.

Parameters declared as plain `string` are unchanged: `[foo bar]` passed
to one is still the string `"[foo bar]"`.

## Additional notes

closes https://github.com/nushell/nushell/issues/16445

crates/nu-parser/src/parse_calls.rs

@@ -717,6 +717,22 @@ fn calculate_end_span(
     }
 }
 
+/// Check if a syntax shape accepts nearly any bare word without error. `string`, `path` and
+/// `directory` take `5` or `5sec` as a string, `glob` and `external_arg` as a glob, `cell-path`
+/// takes `5` as `$.5`, and `any` falls back to a string. `parse_oneof` tries these shapes last.
+fn accepts_any_bareword(shape: &SyntaxShape) -> bool {
+    matches!(
+        shape,
+        SyntaxShape::String
+            | SyntaxShape::Filepath
+            | SyntaxShape::Directory
+            | SyntaxShape::GlobPattern
+            | SyntaxShape::CellPath
+            | SyntaxShape::ExternalArgument
+            | SyntaxShape::Any
+    )
+}
+
 pub(crate) fn parse_oneof(
     working_set: &mut StateWorkingSet,
     spans: &[Span],
@@ -728,10 +744,35 @@ pub(crate) fn parse_oneof(
     let starting_spans_idx = *spans_idx;
     let mut best_guess = None;
     let mut best_guess_errors = Vec::new();
+    let mut best_guess_warnings = Vec::new();
+    let mut best_guess_scopes = Vec::new();
     let mut max_first_error_offset = 0;
     let mut propagate_error = false;
-    for shape in possible_shapes {
+    // The first shape that parses without error wins. Shapes that accept any bare word go
+    // last (keeping their declared order among themselves), the same way `SyntaxShape::Any`
+    // tries `String` last, so `oneof<string, list<string>>` parses `[a b]` as a list and
+    // `oneof<string, int>` parses `5` as an int.
+    // `parse_value` handles a value starting with `$`, `(` or `{` (variable, subexpression,
+    // interpolation, record, closure) by its first byte under nearly every shape, so no shape
+    // claims it the way `string` claims `5`. Such a value keeps the declared order, which still
+    // decides the few cases where the shape matters: `oneof<glob, list<glob>>` takes
+    // `$"*.($ext)"` as a glob, and `oneof<any, closure>` takes `{...$r}` as a record.
+    let defer_barewords = !matches!(
+        working_set
+            .get_span_contents(spans[starting_spans_idx])
+            .first(),
+        Some(b'$' | b'(' | b'{')
+    );
+    let deferred = |shape: &SyntaxShape| defer_barewords && accepts_any_bareword(shape);
+    let ordered_shapes = possible_shapes
+        .iter()
+        .filter(|shape| !deferred(shape))
+        .chain(possible_shapes.iter().filter(|shape| deferred(shape)));
+    let starting_scope_count = working_set.delta.scope.len();
+    for shape in ordered_shapes {
         let starting_error_count = working_set.parse_errors.len();
+        let starting_warning_count = working_set.parse_warnings.len();
+        let starting_compile_error_count = working_set.compile_errors.len();
         *spans_idx = starting_spans_idx;
         let value = match multispan {
             true => parse_multispan_value(working_set, spans, spans_idx, shape, input_type),
@@ -744,6 +785,13 @@ pub(crate) fn parse_oneof(
             return value;
         };
 
+        // A closure with no closing `}` leaves its scope open. Take this attempt's open scopes
+        // off the stack, so the next shape doesn't resolve names through them and later
+        // definitions don't land in them. Only the best guess's go back, below, because
+        // completions inside the unfinished closure need them.
+        let scopes = &mut working_set.delta.scope;
+        let open_scopes = scopes.split_off(starting_scope_count.min(scopes.len()));
+
         if first_error_offset > max_first_error_offset {
             // while trying the possible shapes, ignore Expected type errors
             // unless they're inside a block, closure, or expression
@@ -761,14 +809,28 @@ pub(crate) fn parse_oneof(
             best_guess = Some(value);
             best_guess_errors.clear();
             best_guess_errors.extend_from_slice(new_errors);
+            best_guess_warnings.clear();
+            best_guess_warnings
+                .extend_from_slice(&working_set.parse_warnings[starting_warning_count..]);
+            best_guess_scopes = open_scopes;
         }
+        // Discard this attempt's errors, warnings and compile errors. A later shape parses the
+        // same code again, or the attempt's closures go unused, so keeping them would report
+        // duplicate or spurious problems. If every shape fails, the best guess's errors and
+        // warnings are restored below.
         working_set.parse_errors.truncate(starting_error_count);
+        working_set.parse_warnings.truncate(starting_warning_count);
+        working_set
+            .compile_errors
+            .truncate(starting_compile_error_count);
     }
 
     // if best_guess results in new errors further than current span, then accept it
     // or propagate_error is marked as true for it
     if max_first_error_offset > spans[starting_spans_idx].start || propagate_error {
         working_set.parse_errors.extend(best_guess_errors);
+        working_set.parse_warnings.extend(best_guess_warnings);
+        working_set.delta.scope.extend(best_guess_scopes);
         best_guess.expect("best_guess should not be None here!")
     } else {
         working_set.error(ParseError::ExpectedWithStringMsg(

crates/nu-protocol/src/syntax_shape.rs

@@ -91,7 +91,9 @@ pub enum SyntaxShape {
     /// Only a numeric (integer or float) value is allowed
     Number,
 
-    /// One of a list of possible items, checked in order
+    /// One of a list of possible items, checked in order, except that for an argument not
+    /// starting with `$`, `(` or `{`, shapes accepting any bare word (string, path, directory,
+    /// glob, cell-path, external-argument, any) are checked last
     OneOf(Vec<SyntaxShape>),
 
     /// An operator, eg `+`

tests/repl/test_signatures.rs

@@ -364,6 +364,33 @@ fn oneof_annotations(
 #[case::table_record("oneof<record, table>", "{item: 1}", Type::Record(vec![("item".to_string(), Type::Int)].into()))]
 #[case::table_list_of_record("oneof<record, table>", "[{item: 1}]", Type::Table(vec![("item".to_string(), Type::Int)].into()))]
 #[case::table_table("oneof<record, table>", "[[item]; [1]]", Type::Table(vec![("item".to_string(), Type::Int)].into()))]
+// For a literal, shapes that accept any bare word are tried after the others, in their
+// declared order, so they only take what no other listed type matches.
+#[case::list_before_string(
+    "oneof<string, list<string>>",
+    "[foo bar]",
+    Type::List(Box::new(Type::String))
+)]
+#[case::table_before_string("oneof<string, table>", "[[item]; [1]]", Type::Table(vec![("item".to_string(), Type::Int)].into()))]
+#[case::int_before_string("oneof<string, int>", "5", Type::Int)]
+#[case::duration_before_string("oneof<string, duration>", "5sec", Type::Duration)]
+#[case::range_before_string("oneof<string, range>", "1..5", Type::Range)]
+#[case::list_before_path("oneof<path, list<path>>", "[a b]", Type::List(Box::new(Type::String)))]
+#[case::list_before_glob("oneof<glob, list<glob>>", "[a b]", Type::List(Box::new(Type::Glob)))]
+#[case::int_before_cell_path("oneof<cell-path, int>", "5", Type::Int)]
+#[case::string_fallback("oneof<string, int>", "foo", Type::String)]
+#[case::string_before_cell_path("oneof<string, cell-path>", "a.b", Type::String)]
+#[case::glob_before_any("oneof<glob, any>", "[a b]", Type::Glob)]
+#[case::int_before_directory("oneof<directory, int>", "5", Type::Int)]
+#[case::string_before_external_arg("oneof<string, external_arg>", "foo", Type::String)]
+#[case::int_before_external_arg("oneof<external_arg, int>", "5", Type::Int)]
+// A failed `list<int>` attempt compiles the closure, but its compile error must not leak.
+#[case::failed_attempt_compile_error("oneof<string, list<int>>", "[{|| break} a]", Type::String)]
+// `$`, `(` and `{` values keep the declared order: an interpolation still becomes a glob and a
+// spread still makes a record.
+#[case::glob_interpolation_before_list("oneof<glob, list<glob>>", r#"$"*.(1)""#, Type::Glob)]
+#[case::glob_paren_interpolation_before_int("oneof<glob, int>", "(1)*.rs", Type::Glob)]
+#[case::record_spread_before_closure("oneof<any, closure>", "{...{a: 1}}", Type::Record(vec![("a".to_string(), Type::Int)].into()))]
 #[case::closure_cell_path("oneof<cell-path, closure>", "foo", Type::CellPath)]
 #[case::closure_closure_with_bars("oneof<cell-path, closure>", "{|| get foo }", Type::Closure)]
 #[case::closure_closure_without_bars("oneof<cell-path, closure>", "{ get foo }", Type::Closure)]
@@ -385,6 +412,22 @@ fn oneof_positional_named(
     Ok(())
 }
 
+// The `list<closure>` attempt opens a scope for `{...}.foo?`, which has no closing `}`, and then
+// fails. `string` takes the argument, and that scope must reach neither the rest of the
+// argument nor the code after it.
+#[rstest]
+#[case::later_definition(
+    "f [{}.foo?]; def g [] { 5 }; scope commands | where name == g | length",
+    "1"
+)]
+#[case::name_in_same_argument(r#"let y = "outer"; f [{|y| 1}.foo? ($y)]"#, "[{|y| 1}.foo? outer]")]
+fn oneof_failed_attempt_scope(#[case] code: &str, #[case] expected: &str) -> TestResult {
+    run_test(
+        &format!("def f [x: oneof<string, list<closure>>] {{ $x }}; {code}"),
+        expected,
+    )
+}
+
 #[rstest]
 #[case::correct_type_(run_test, "{a: 1}", "")]
 #[case::correct_type_(run_test, "{a: null}", "")]