From dac32557cdf85018aefc970b7f44820dbb534055 Mon Sep 17 00:00:00 2001
From: Horasal <1991933+horasal@users.noreply.github.com>
Date: Mon, 4 Sep 2023 09:21:45 +0900
Subject: [PATCH] prevent crash when use redirection with let/mut (#10139)

Fix #9992

I mistakenly messed up https://github.com/nushell/nushell/pull/10118 and
this is a cleaned version.

# Description

* This pr changes the panic to errors while parsing `let`, now user will
get the following errors:
<img width="395" alt="scr"
src="https://github.com/nushell/nushell/assets/1991933/4b39ac14-cd1f-47b3-9490-81009ca42717">
<img width="394" alt="scr"
src="https://github.com/nushell/nushell/assets/1991933/71ce33ad-f4d0-4132-828f-9674b9603556">
<img width="440" alt="scr"
src="https://github.com/nushell/nushell/assets/1991933/257eab4d-1a72-42db-b09e-f42bef33d2ec">

* `out+err>` is cached by `parse_expression` but not this, which may be
a potential problem.
* `Commond(None, ..)` remains panic for future bug report because I
don't actually know when/how does it happen

# User-Facing Changes

Nushell won't crash when user typing `let a = 1 err> ...`

# Tests + Formatting

- `cargo fmt --all -- --check` : OK
- `cargo clippy --workspace -- -D warnings -D clippy::unwrap_used` : OK
- `cargo test --workspace` : OK
- `cargo run -- -c "use std testing; testing run-tests --path
crates/nu-std"` : OK

# After Submitting

None

Co-authored-by: Horasal <horsal@horsal.dev>
---
 crates/nu-parser/src/parser.rs        | 11 +++++++++++
 crates/nu-parser/tests/test_parser.rs | 24 ++++++++++++++++++++++++
 crates/nu-protocol/src/parse_error.rs |  8 ++++++++
 3 files changed, 43 insertions(+)

diff --git a/crates/nu-parser/src/parser.rs b/crates/nu-parser/src/parser.rs
index ae1bb6a048..530ca63767 100644
--- a/crates/nu-parser/src/parser.rs
+++ b/crates/nu-parser/src/parser.rs
@@ -5439,6 +5439,17 @@ pub fn parse_pipeline(
                                 new_command.comments.extend_from_slice(&command.comments);
                                 new_command.parts.extend_from_slice(&command.parts);
                             }
+                            LiteElement::Redirection(span, ..) => {
+                                working_set.error(ParseError::RedirectionInLetMut(*span, None))
+                            }
+                            LiteElement::SeparateRedirection { out, err } => {
+                                working_set.error(ParseError::RedirectionInLetMut(
+                                    out.0.min(err.0),
+                                    Some(out.0.max(err.0)),
+                                ))
+                            }
+                            LiteElement::SameTargetRedirection { redirection, .. } => working_set
+                                .error(ParseError::RedirectionInLetMut(redirection.0, None)),
                             _ => panic!("unsupported"),
                         }
                     }
diff --git a/crates/nu-parser/tests/test_parser.rs b/crates/nu-parser/tests/test_parser.rs
index 8278f702ab..901f339844 100644
--- a/crates/nu-parser/tests/test_parser.rs
+++ b/crates/nu-parser/tests/test_parser.rs
@@ -781,6 +781,30 @@ fn test_nothing_comparison_eq() {
         )
     ))
 }
+#[rstest]
+#[case(b"let a = 1 err> /dev/null", "RedirectionInLetMut")]
+#[case(b"let a = 1 out> /dev/null", "RedirectionInLetMut")]
+#[case(b"mut a = 1 err> /dev/null", "RedirectionInLetMut")]
+#[case(b"mut a = 1 out> /dev/null", "RedirectionInLetMut")]
+// This two cases cause AssignInPipeline instead of RedirectionInLetMut
+#[case(b"let a = 1 out+err> /dev/null", "AssignInPipeline")]
+#[case(b"mut a = 1 out+err> /dev/null", "AssignInPipeline")]
+fn test_redirection_with_letmut(#[case] phase: &[u8], #[case] expected: &str) {
+    let engine_state = EngineState::new();
+    let mut working_set = StateWorkingSet::new(&engine_state);
+    let _block = parse(&mut working_set, None, phase, true);
+    match expected {
+        "RedirectionInLetMut" => assert!(matches!(
+            working_set.parse_errors.first(),
+            Some(ParseError::RedirectionInLetMut(_, _))
+        )),
+        "AssignInPipeline" => assert!(matches!(
+            working_set.parse_errors.first(),
+            Some(ParseError::AssignInPipeline(_, _, _, _))
+        )),
+        _ => panic!("unexpected pattern"),
+    }
+}
 
 #[test]
 fn test_nothing_comparison_neq() {
diff --git a/crates/nu-protocol/src/parse_error.rs b/crates/nu-protocol/src/parse_error.rs
index 14e2939e16..95f1e665eb 100644
--- a/crates/nu-protocol/src/parse_error.rs
+++ b/crates/nu-protocol/src/parse_error.rs
@@ -466,6 +466,13 @@ pub enum ParseError {
         #[label("{label}")]
         span: Span,
     },
+
+    #[error("Redirection can not be used with let/mut.")]
+    #[diagnostic()]
+    RedirectionInLetMut(
+        #[label("Not allowed here")] Span,
+        #[label("...and here")] Option<Span>,
+    ),
 }
 
 impl ParseError {
@@ -550,6 +557,7 @@ impl ParseError {
             ParseError::UnknownOperator(_, _, s) => *s,
             ParseError::InvalidLiteral(_, _, s) => *s,
             ParseError::LabeledErrorWithHelp { span: s, .. } => *s,
+            ParseError::RedirectionInLetMut(s, _) => *s,
         }
     }
 }