A set of fixes for stderr redirect (#7219)

# Description

This is a set of fixes to `err>` to make it work a bit more predictably.

I've also revised the tests, which accidentally tested the wrong thing
for redirection, but should be more correct now.

# User-Facing Changes

_(List of all changes that impact the user experience here. This helps
us keep track of breaking changes.)_

# 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 -A
clippy::needless_collect` to check that you're using the standard code
style
- `cargo test --workspace` to check that all tests pass

# 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-cli/src/eval_file.rs

@@ -74,6 +74,14 @@ pub fn print_table_or_error(
     // Change the engine_state config to use the passed in configuration
     engine_state.set_config(config);
 
+    if let PipelineData::Value(Value::Error { error }, ..) = &pipeline_data {
+        let working_set = StateWorkingSet::new(engine_state);
+
+        report_error(&working_set, error);
+
+        std::process::exit(1);
+    }
+
     match engine_state.find_decl("table".as_bytes(), &[]) {
         Some(decl_id) => {
             let command = engine_state.get_decl(decl_id);

crates/nu-cli/src/repl.rs

@@ -528,7 +528,7 @@ pub fn evaluate_repl(
             Err(err) => {
                 let message = err.to_string();
                 if !message.contains("duration") {
-                    println!("Error: {:?}", err);
+                    eprintln!("Error: {:?}", err);
                     // TODO: Identify possible error cases where a hard failure is preferable
                     // Ignoring and reporting could hide bigger problems
                     // e.g. https://github.com/nushell/nushell/issues/6452

crates/nu-command/src/core_commands/do_.rs

@@ -168,6 +168,9 @@ impl Command for Do {
                 metadata,
                 trim_end_newline,
             }),
+            Ok(PipelineData::Value(Value::Error { .. }, ..)) if ignore_shell_errors => {
+                Ok(PipelineData::new(call.head))
+            }
             Err(_) if ignore_shell_errors => Ok(PipelineData::new(call.head)),
             r => r,
         }

crates/nu-command/src/system/run_external.rs

@@ -390,7 +390,7 @@ impl ExternalCommand {
                                 use std::os::unix::process::ExitStatusExt;
                                 if x.core_dumped() {
                                     let style = Style::new().bold().on(Color::Red);
-                                    println!(
+                                    eprintln!(
                                         "{}",
                                         style.paint(format!(
                                             "nushell: oops, process '{commandname}' core dumped"

crates/nu-command/tests/commands/do_.rs

@@ -9,6 +9,8 @@ fn capture_errors_works() {
         "#
     ));
 
+    eprintln!("actual.err: {:?}", actual.err);
+
     assert!(actual.err.contains("column_not_found"));
 }
 
@@ -65,7 +67,7 @@ fn ignore_shell_errors_works_for_external_with_semicolon() {
     let actual = nu!(
         cwd: ".", pipeline(
         r#"
-        do -s { fail }; `text`
+        do -s { open asdfasdf.txt }; "text"
         "#
     ));
 

crates/nu-command/tests/commands/nu_check.rs

@@ -323,7 +323,7 @@ fn parse_string_as_script() {
             "#
         ));
 
-        println!("the out put is {}", actual.err);
+        println!("the output is {}", actual.err);
         assert!(actual.err.contains("Failed to parse content"));
     })
 }

crates/nu-command/tests/commands/redirection.rs

@@ -7,10 +7,10 @@ fn redirect_err() {
     Playground::setup("redirect_err_test", |dirs, _sandbox| {
         let output = nu!(
             cwd: dirs.test(),
-            "cat asdfasdfasdf.txt err> a; cat a"
+            "cat asdfasdfasdf.txt err> a.txt; cat a.txt"
         );
 
-        assert!(output.err.contains("asdfasdfasdf.txt"));
+        assert!(output.out.contains("asdfasdfasdf.txt"));
     })
 }
 
@@ -20,10 +20,10 @@ fn redirect_err() {
     Playground::setup("redirect_err_test", |dirs, _sandbox| {
         let output = nu!(
             cwd: dirs.test(),
-            "type asdfasdfasdf.txt err> a; type a"
+            "dir missingapplication err> a; (open a | size).bytes >= 16"
         );
 
-        assert!(output.err.contains("asdfasdfasdf.txt"));
+        assert!(output.out.contains("true"));
     })
 }
 
@@ -36,7 +36,7 @@ fn redirect_outerr() {
             "cat asdfasdfasdf.txt out+err> a; cat a"
         );
 
-        assert!(output.err.contains("asdfasdfasdf.txt"));
+        assert!(output.out.contains("asdfasdfasdf.txt"));
     })
 }
 
@@ -46,10 +46,10 @@ fn redirect_outerr() {
     Playground::setup("redirect_outerr_test", |dirs, _sandbox| {
         let output = nu!(
             cwd: dirs.test(),
-            "type asdfasdfasdf.txt out+err> a; type a"
+            "dir missingapplication out+err> a; (open a | size).bytes >= 16"
         );
 
-        assert!(output.err.contains("asdfasdfasdf.txt"));
+        assert!(output.out.contains("true"));
     })
 }
 

crates/nu-engine/src/eval.rs

@@ -669,7 +669,7 @@ pub fn eval_expression(
 /// into the call and gets out the result
 /// Otherwise, invokes the expression
 ///
-/// It returns PipelineData with a boolean flag, indicate that if the external runs to failed.
+/// It returns PipelineData with a boolean flag, indicating if the external failed to run.
 /// The boolean flag **may only be true** for external calls, for internal calls, it always to be false.
 pub fn eval_expression_with_input(
     engine_state: &EngineState,
@@ -957,6 +957,14 @@ pub fn eval_block(
         let mut i = 0;
 
         while i < pipeline.elements.len() {
+            let redirect_stderr = redirect_stderr
+                || ((i < pipeline.elements.len() - 1)
+                    && (matches!(
+                        pipeline.elements[i + 1],
+                        PipelineElement::Redirection(_, Redirection::Stderr, _)
+                            | PipelineElement::Redirection(_, Redirection::StdoutAndStderr, _)
+                    )));
+
             // if eval internal command failed, it can just make early return with `Err(ShellError)`.
             let eval_result = eval_element_with_input(
                 engine_state,
@@ -971,20 +979,28 @@ pub fn eval_block(
                                 | PipelineElement::Redirection(_, Redirection::StdoutAndStderr, _)
                                 | PipelineElement::Expression(..)
                         )),
-                redirect_stderr
+                redirect_stderr,
-                    || ((i < pipeline.elements.len() - 1)
+            );
-                        && (matches!(
+
-                            pipeline.elements[i + 1],
+            match (eval_result, redirect_stderr) {
-                            PipelineElement::Redirection(_, Redirection::Stderr, _)
+                (Ok((pipeline_data, _)), true) => {
-                                | PipelineElement::Redirection(_, Redirection::StdoutAndStderr, _)
+                    input = pipeline_data;
-                        ))),
+
-            )?;
+                    // external command may runs to failed
-            input = eval_result.0;
+                    // make early return so remaining commands will not be executed.
-            // external command may runs to failed
+                    // don't return `Err(ShellError)`, so nushell wouldn't show extra error message.
-            // make early return so remaining commands will not be executed.
+                }
-            // don't return `Err(ShellError)`, so nushell wouldn't show extra error message.
+                (Err(error), true) => input = PipelineData::Value(Value::Error { error }, None),
-            if eval_result.1 {
+                (output, false) => {
-                return Ok(input);
+                    let output = output?;
+                    input = output.0;
+                    // external command may runs to failed
+                    // make early return so remaining commands will not be executed.
+                    // don't return `Err(ShellError)`, so nushell wouldn't show extra error message.
+                    if output.1 {
+                        return Ok(input);
+                    }
+                }
             }
 
             i += 1;

crates/nu-parser/src/parser.rs

@@ -5999,19 +5999,38 @@ pub fn lite_parse(tokens: &[Token]) -> (LiteBlock, Option<ParseError>) {
                 if last_token != TokenContents::Pipe && last_token != TokenContents::OutGreaterThan
                 {
                     if !curr_command.is_empty() {
-                        if last_connector == TokenContents::OutGreaterThan
+                        match last_connector {
-                            || last_connector == TokenContents::ErrGreaterThan
+                            TokenContents::OutGreaterThan => {
-                            || last_connector == TokenContents::OutErrGreaterThan
+                                curr_pipeline.push(LiteElement::Redirection(
-                        {
+                                    last_connector_span.expect(
-                            curr_pipeline.push(LiteElement::Redirection(
+                                        "internal error: redirection missing span information",
-                                last_connector_span
+                                    ),
-                                    .expect("internal error: redirection missing span information"),
+                                    Redirection::Stdout,
-                                Redirection::Stdout,
+                                    curr_command,
-                                curr_command,
+                                ));
-                            ));
+                            }
-                        } else {
+                            TokenContents::ErrGreaterThan => {
-                            curr_pipeline
+                                curr_pipeline.push(LiteElement::Redirection(
-                                .push(LiteElement::Command(last_connector_span, curr_command));
+                                    last_connector_span.expect(
+                                        "internal error: redirection missing span information",
+                                    ),
+                                    Redirection::Stderr,
+                                    curr_command,
+                                ));
+                            }
+                            TokenContents::OutErrGreaterThan => {
+                                curr_pipeline.push(LiteElement::Redirection(
+                                    last_connector_span.expect(
+                                        "internal error: redirection missing span information",
+                                    ),
+                                    Redirection::StdoutAndStderr,
+                                    curr_command,
+                                ));
+                            }
+                            _ => {
+                                curr_pipeline
+                                    .push(LiteElement::Command(last_connector_span, curr_command));
+                            }
                         }
 
                         curr_command = LiteCommand::new();
@@ -6033,18 +6052,35 @@ pub fn lite_parse(tokens: &[Token]) -> (LiteBlock, Option<ParseError>) {
             }
             TokenContents::Semicolon => {
                 if !curr_command.is_empty() {
-                    if last_connector == TokenContents::OutGreaterThan
+                    match last_connector {
-                        || last_connector == TokenContents::ErrGreaterThan
+                        TokenContents::OutGreaterThan => {
-                        || last_connector == TokenContents::OutErrGreaterThan
+                            curr_pipeline.push(LiteElement::Redirection(
-                    {
+                                last_connector_span
-                        curr_pipeline.push(LiteElement::Redirection(
+                                    .expect("internal error: redirection missing span information"),
-                            last_connector_span
+                                Redirection::Stdout,
-                                .expect("internal error: redirection missing span information"),
+                                curr_command,
-                            Redirection::Stdout,
+                            ));
-                            curr_command,
+                        }
-                        ));
+                        TokenContents::ErrGreaterThan => {
-                    } else {
+                            curr_pipeline.push(LiteElement::Redirection(
-                        curr_pipeline.push(LiteElement::Command(last_connector_span, curr_command));
+                                last_connector_span
+                                    .expect("internal error: redirection missing span information"),
+                                Redirection::Stderr,
+                                curr_command,
+                            ));
+                        }
+                        TokenContents::OutErrGreaterThan => {
+                            curr_pipeline.push(LiteElement::Redirection(
+                                last_connector_span
+                                    .expect("internal error: redirection missing span information"),
+                                Redirection::StdoutAndStderr,
+                                curr_command,
+                            ));
+                        }
+                        _ => {
+                            curr_pipeline
+                                .push(LiteElement::Command(last_connector_span, curr_command));
+                        }
                     }
 
                     curr_command = LiteCommand::new();
@@ -6054,6 +6090,8 @@ pub fn lite_parse(tokens: &[Token]) -> (LiteBlock, Option<ParseError>) {
                     block.push(curr_pipeline);
 
                     curr_pipeline = LitePipeline::new();
+                    last_connector = TokenContents::Pipe;
+                    last_connector_span = None;
                 }
 
                 last_token = TokenContents::Semicolon;