From 8cca447e8c9a6f7f9a341245907ea3da5710ced4 Mon Sep 17 00:00:00 2001 From: JT <547158+jntrnr@users.noreply.github.com> Date: Thu, 24 Nov 2022 16:58:15 +1300 Subject: [PATCH] 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 | 8 ++ crates/nu-cli/src/repl.rs | 2 +- crates/nu-command/src/core_commands/do_.rs | 3 + crates/nu-command/src/system/run_external.rs | 2 +- crates/nu-command/tests/commands/do_.rs | 4 +- crates/nu-command/tests/commands/nu_check.rs | 2 +- .../nu-command/tests/commands/redirection.rs | 14 +-- crates/nu-engine/src/eval.rs | 46 ++++++---- crates/nu-parser/src/parser.rs | 88 +++++++++++++------ 9 files changed, 118 insertions(+), 51 deletions(-) diff --git a/crates/nu-cli/src/eval_file.rs b/crates/nu-cli/src/eval_file.rs index 5160d8184..bea25e9ca 100644 --- a/crates/nu-cli/src/eval_file.rs +++ b/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); diff --git a/crates/nu-cli/src/repl.rs b/crates/nu-cli/src/repl.rs index 73b3b44ea..dd2e47cd6 100644 --- a/crates/nu-cli/src/repl.rs +++ b/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 diff --git a/crates/nu-command/src/core_commands/do_.rs b/crates/nu-command/src/core_commands/do_.rs index 54a4c9169..15bb4b802 100644 --- a/crates/nu-command/src/core_commands/do_.rs +++ b/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, } diff --git a/crates/nu-command/src/system/run_external.rs b/crates/nu-command/src/system/run_external.rs index 1618390ee..47ece21dd 100644 --- a/crates/nu-command/src/system/run_external.rs +++ b/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" diff --git a/crates/nu-command/tests/commands/do_.rs b/crates/nu-command/tests/commands/do_.rs index 2791b5503..ea022581f 100644 --- a/crates/nu-command/tests/commands/do_.rs +++ b/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" "# )); diff --git a/crates/nu-command/tests/commands/nu_check.rs b/crates/nu-command/tests/commands/nu_check.rs index ec53e469d..c6482e420 100644 --- a/crates/nu-command/tests/commands/nu_check.rs +++ b/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")); }) } diff --git a/crates/nu-command/tests/commands/redirection.rs b/crates/nu-command/tests/commands/redirection.rs index 4d1e3db37..439dcf93a 100644 --- a/crates/nu-command/tests/commands/redirection.rs +++ b/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")); }) } diff --git a/crates/nu-engine/src/eval.rs b/crates/nu-engine/src/eval.rs index a9905198b..ef2d1a112 100644 --- a/crates/nu-engine/src/eval.rs +++ b/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 - || ((i < pipeline.elements.len() - 1) - && (matches!( - pipeline.elements[i + 1], - PipelineElement::Redirection(_, Redirection::Stderr, _) - | PipelineElement::Redirection(_, Redirection::StdoutAndStderr, _) - ))), - )?; - input = eval_result.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 eval_result.1 { - return Ok(input); + redirect_stderr, + ); + + match (eval_result, redirect_stderr) { + (Ok((pipeline_data, _)), true) => { + input = pipeline_data; + + // 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. + } + (Err(error), true) => input = PipelineData::Value(Value::Error { error }, None), + (output, false) => { + 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; diff --git a/crates/nu-parser/src/parser.rs b/crates/nu-parser/src/parser.rs index 7a24376c0..dd491efb8 100644 --- a/crates/nu-parser/src/parser.rs +++ b/crates/nu-parser/src/parser.rs @@ -5999,19 +5999,38 @@ pub fn lite_parse(tokens: &[Token]) -> (LiteBlock, Option) { if last_token != TokenContents::Pipe && last_token != TokenContents::OutGreaterThan { if !curr_command.is_empty() { - if last_connector == TokenContents::OutGreaterThan - || last_connector == TokenContents::ErrGreaterThan - || last_connector == TokenContents::OutErrGreaterThan - { - curr_pipeline.push(LiteElement::Redirection( - last_connector_span - .expect("internal error: redirection missing span information"), - Redirection::Stdout, - curr_command, - )); - } else { - curr_pipeline - .push(LiteElement::Command(last_connector_span, curr_command)); + match last_connector { + TokenContents::OutGreaterThan => { + curr_pipeline.push(LiteElement::Redirection( + last_connector_span.expect( + "internal error: redirection missing span information", + ), + Redirection::Stdout, + curr_command, + )); + } + TokenContents::ErrGreaterThan => { + curr_pipeline.push(LiteElement::Redirection( + 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) { } TokenContents::Semicolon => { if !curr_command.is_empty() { - if last_connector == TokenContents::OutGreaterThan - || last_connector == TokenContents::ErrGreaterThan - || last_connector == TokenContents::OutErrGreaterThan - { - curr_pipeline.push(LiteElement::Redirection( - last_connector_span - .expect("internal error: redirection missing span information"), - Redirection::Stdout, - curr_command, - )); - } else { - curr_pipeline.push(LiteElement::Command(last_connector_span, curr_command)); + match last_connector { + TokenContents::OutGreaterThan => { + curr_pipeline.push(LiteElement::Redirection( + last_connector_span + .expect("internal error: redirection missing span information"), + Redirection::Stdout, + curr_command, + )); + } + TokenContents::ErrGreaterThan => { + curr_pipeline.push(LiteElement::Redirection( + 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) { block.push(curr_pipeline); curr_pipeline = LitePipeline::new(); + last_connector = TokenContents::Pipe; + last_connector_span = None; } last_token = TokenContents::Semicolon;