From cb67de675e69fb42d9ac6c9baa3f32c61dce9c26 Mon Sep 17 00:00:00 2001
From: Yash Thakur <45539777+ysthakur@users.noreply.github.com>
Date: Wed, 14 Feb 2024 18:16:19 -0500
Subject: [PATCH] Disallow spreading lists automatically when calling externals
 (#11857)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

<!--
if this PR closes one or more issues, you can automatically link the PR
with
them by using one of the [*linking
keywords*](https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue#linking-a-pull-request-to-an-issue-using-a-keyword),
e.g.
- this PR should close #xxxx
- fixes #xxxx

you can also mention related issues, PRs or discussions!
-->

# Description
<!--
Thank you for improving Nushell. Please, check our [contributing
guide](../CONTRIBUTING.md) and talk to the core team before making major
changes.

Description of your pull request goes here. **Provide examples and/or
screenshots** if your changes affect the user experience.
-->

Spreading lists automatically when calling externals was deprecated in
0.89 (#11289), and this PR is to remove it in 0.91.

# User-Facing Changes
<!-- List of all changes that impact the user experience here. This
helps us keep track of breaking changes. -->

The new error message looks like this:

```
>  ^echo [1 2]
Error: nu::shell::cannot_pass_list_to_external

  × Lists are not automatically spread when calling external commands
   ╭─[entry #13:1:8]
 1 │  ^echo [1 2]
   ·        ──┬──
   ·          ╰── Spread operator (...) is necessary to spread lists
   ╰────
  help: Either convert the list to a string or use the spread operator, like so: ...[1 2]
```

The old error message didn't say exactly where to put the `...` and
seemed to confuse a lot of people, so hopefully this helps.

# Tests + Formatting
<!--
Don't forget to add tests that cover your changes.

Make sure you've run and fixed any issues with these commands:

- `cargo fmt --all -- --check` to check standard code formatting (`cargo
fmt --all` applies these changes)
- `cargo clippy --workspace -- -D warnings -D clippy::unwrap_used` to
check that you're using the standard code style
- `cargo test --workspace` to check that all tests pass (on Windows make
sure to [enable developer
mode](https://learn.microsoft.com/en-us/windows/apps/get-started/developer-mode-features-and-debugging))
- `cargo run -- -c "use std testing; testing run-tests --path
crates/nu-std"` to run the tests for the standard library

> **Note**
> from `nushell` you can also use the `toolkit` as follows
> ```bash
> use toolkit.nu # or use an `env_change` hook to activate it
automatically
> toolkit check pr
> ```
-->

There was one test to check that implicit spread was deprecated before,
updated that to check that it's disallowed now.

# After Submitting
<!-- If your PR had any user-facing changes, update [the
documentation](https://github.com/nushell/nushell.github.io) after the
PR is merged, if necessary. This will help us keep the docs up to date.
-->
---
 crates/nu-command/src/system/run_external.rs | 36 ++++++++------------
 crates/nu-protocol/src/shell_error.rs        | 16 +++++++++
 src/tests/test_spread.rs                     | 12 +++----
 3 files changed, 36 insertions(+), 28 deletions(-)

diff --git a/crates/nu-command/src/system/run_external.rs b/crates/nu-command/src/system/run_external.rs
index 43c81eba9b..edd00f4879 100644
--- a/crates/nu-command/src/system/run_external.rs
+++ b/crates/nu-command/src/system/run_external.rs
@@ -135,29 +135,23 @@ pub fn create_external_command(
     let mut spanned_args = vec![];
     let mut arg_keep_raw = vec![];
     for (arg, spread) in call.rest_iter(1) {
-        // TODO: Disallow automatic spreading entirely later. This match block will
-        // have to be refactored, and lists will have to be disallowed in the parser too
         match eval_expression(engine_state, stack, arg)? {
             Value::List { vals, .. } => {
-                if !spread {
-                    nu_protocol::report_error_new(
-                        engine_state,
-                        &ShellError::GenericError {
-                            error: "Automatically spreading lists is deprecated".into(),
-                            msg: "Spreading lists automatically when calling external commands is deprecated and will be removed in 0.91.".into(),
-                            span: Some(arg.span),
-                            help: Some("Use the spread operator (put a '...' before the argument)".into()),
-                            inner: vec![],
-                        },
-                    );
-                }
-                // turn all the strings in the array into params.
-                // Example: one_arg may be something like ["ls" "-a"]
-                // convert it to "ls" "-a"
-                for v in vals {
-                    spanned_args.push(value_as_spanned(v)?);
-                    // for arguments in list, it's always treated as a whole arguments
-                    arg_keep_raw.push(true);
+                if spread {
+                    // turn all the strings in the array into params.
+                    // Example: one_arg may be something like ["ls" "-a"]
+                    // convert it to "ls" "-a"
+                    for v in vals {
+                        spanned_args.push(value_as_spanned(v)?);
+                        // for arguments in list, it's always treated as a whole arguments
+                        arg_keep_raw.push(true);
+                    }
+                } else {
+                    return Err(ShellError::CannotPassListToExternal {
+                        arg: String::from_utf8_lossy(engine_state.get_span_contents(arg.span))
+                            .into(),
+                        span: arg.span,
+                    });
                 }
             }
             val => {
diff --git a/crates/nu-protocol/src/shell_error.rs b/crates/nu-protocol/src/shell_error.rs
index 664582658e..d4549766b7 100644
--- a/crates/nu-protocol/src/shell_error.rs
+++ b/crates/nu-protocol/src/shell_error.rs
@@ -1308,6 +1308,22 @@ This is an internal Nushell error, please file an issue https://github.com/nushe
         span: Span,
     },
 
+    /// Lists are not automatically spread when calling external commands
+    ///
+    /// ## Resolution
+    ///
+    /// Use the spread operator (put a '...' before the argument)
+    #[error("Lists are not automatically spread when calling external commands")]
+    #[diagnostic(
+        code(nu::shell::cannot_pass_list_to_external),
+        help("Either convert the list to a string or use the spread operator, like so: ...{arg}")
+    )]
+    CannotPassListToExternal {
+        arg: String,
+        #[label = "Spread operator (...) is necessary to spread lists"]
+        span: Span,
+    },
+
     /// Out of bounds.
     ///
     /// ## Resolution
diff --git a/src/tests/test_spread.rs b/src/tests/test_spread.rs
index f031c91988..3dceea7089 100644
--- a/src/tests/test_spread.rs
+++ b/src/tests/test_spread.rs
@@ -182,13 +182,11 @@ fn explain_spread_args() -> TestResult {
 }
 
 #[test]
-fn deprecate_implicit_spread_for_externals() {
-    // TODO: When automatic spreading is removed, test that list literals fail at parse time
-    let result = nu!(r#"nu --testbin cococo [1 2]"#);
-    assert!(result
-        .err
-        .contains("Automatically spreading lists is deprecated"));
-    assert_eq!(result.out, "1 2");
+fn disallow_implicit_spread_for_externals() -> TestResult {
+    fail_test(
+        r#"nu --testbin cococo [1 2]"#,
+        "Lists are not automatically spread",
+    )
 }
 
 #[test]