From 18772b73b3e5e384a7a225ec8d3dd1773fefc444 Mon Sep 17 00:00:00 2001
From: Devyn Cairns <devyn.cairns@gmail.com>
Date: Mon, 12 Aug 2024 01:24:23 -0700
Subject: [PATCH] Add parse error for external commands used in assignment
 without caret (#13585)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

# Description

As per our Wednesday meeting, this adds a parse error when something
that would be parsed as an external call is present at the top level,
unless the head of the external call begins with a caret (to make it
explicit).

I tried to make the error quite descriptive about what should be done.

# User-Facing Changes
These now cause a parse error:

```nushell
$foo = bar
$foo = `bar`
```

These would have been interpreted as strings before this version, but
now they'd be interpreted as external calls. This behavior is consistent
with `let`/`mut` (which is unaffected by this change).

Here is an example of the error:

```
Error:   × External command calls must be explicit in assignments
   ╭─[entry #3:1:8]
 1 │ $foo = bar
   ·        ─┬─
   ·         ╰── add a caret (^) before the command name if you intended to run and capture its output
   ╰────
  help: the parsing of assignments was changed in 0.97.0, and this would have previously been treated as a string.
        Alternatively, quote the string with single or double quotes to avoid it being interpreted as a command name. This
        restriction may be removed in a future release.
```

# Tests + Formatting

Tests added to cover the change. Note made about it being temporary.
---
 crates/nu-parser/src/parser.rs | 24 ++++++++++++++++++++++++
 tests/repl/test_parser.rs      | 26 ++++++++++++++++++++++++++
 2 files changed, 50 insertions(+)

diff --git a/crates/nu-parser/src/parser.rs b/crates/nu-parser/src/parser.rs
index 8332d2f383..3470b07e14 100644
--- a/crates/nu-parser/src/parser.rs
+++ b/crates/nu-parser/src/parser.rs
@@ -4904,6 +4904,30 @@ pub fn parse_assignment_expression(
     trace!("parsing: assignment right-hand side subexpression");
     let rhs_block = parse_block(working_set, &rhs_tokens, rhs_span, false, true);
     let rhs_ty = rhs_block.output_type();
+
+    // TEMP: double-check that if the RHS block starts with an external call, it must start with a
+    // caret. This is to mitigate the change in assignment parsing introduced in 0.97.0 which could
+    // result in unintentional execution of commands.
+    if let Some(Expr::ExternalCall(head, ..)) = rhs_block
+        .pipelines
+        .first()
+        .and_then(|pipeline| pipeline.elements.first())
+        .map(|element| &element.expr.expr)
+    {
+        let contents = working_set.get_span_contents(Span {
+            start: head.span.start - 1,
+            end: head.span.end,
+        });
+        if !contents.starts_with(b"^") {
+            working_set.parse_errors.push(ParseError::LabeledErrorWithHelp {
+                error: "External command calls must be explicit in assignments".into(),
+                label: "add a caret (^) before the command name if you intended to run and capture its output".into(),
+                help: "the parsing of assignments was changed in 0.97.0, and this would have previously been treated as a string. Alternatively, quote the string with single or double quotes to avoid it being interpreted as a command name. This restriction may be removed in a future release.".into(),
+                span: head.span,
+            });
+        }
+    }
+
     let rhs_block_id = working_set.add_block(Arc::new(rhs_block));
     let mut rhs = Expression::new(
         working_set,
diff --git a/tests/repl/test_parser.rs b/tests/repl/test_parser.rs
index ea983ff82c..a65e2c9a0f 100644
--- a/tests/repl/test_parser.rs
+++ b/tests/repl/test_parser.rs
@@ -311,6 +311,32 @@ fn append_assign_takes_pipeline() -> TestResult {
     )
 }
 
+#[test]
+fn assign_bare_external_fails() {
+    let result = nu!("$env.FOO = nu --testbin cococo");
+    assert!(!result.status.success());
+    assert!(result.err.contains("must be explicit"));
+}
+
+#[test]
+fn assign_bare_external_with_caret() {
+    let result = nu!("$env.FOO = ^nu --testbin cococo");
+    assert!(result.status.success());
+}
+
+#[test]
+fn assign_backtick_quoted_external_fails() {
+    let result = nu!("$env.FOO = `nu` --testbin cococo");
+    assert!(!result.status.success());
+    assert!(result.err.contains("must be explicit"));
+}
+
+#[test]
+fn assign_backtick_quoted_external_with_caret() {
+    let result = nu!("$env.FOO = ^`nu` --testbin cococo");
+    assert!(result.status.success());
+}
+
 #[test]
 fn string_interpolation_paren_test() -> TestResult {
     run_test(r#"$"('(')(')')""#, "()")