diff --git a/crates/nu-cli/src/config_files.rs b/crates/nu-cli/src/config_files.rs index 0a1226c304..2903d26f1d 100644 --- a/crates/nu-cli/src/config_files.rs +++ b/crates/nu-cli/src/config_files.rs @@ -214,7 +214,8 @@ pub fn eval_config_contents( let prev_file = engine_state.file.take(); engine_state.file = Some(config_path.clone()); - eval_source( + // TODO: ignore this error? + let _ = eval_source( engine_state, stack, &contents, diff --git a/crates/nu-cli/src/eval_cmds.rs b/crates/nu-cli/src/eval_cmds.rs index 1459f1ef0d..02a565218b 100644 --- a/crates/nu-cli/src/eval_cmds.rs +++ b/crates/nu-cli/src/eval_cmds.rs @@ -92,11 +92,7 @@ pub fn evaluate_commands( t_mode.coerce_str()?.parse().unwrap_or_default(); } - if let Some(status) = pipeline.print(engine_state, stack, no_newline, false)? { - if status.code() != 0 { - std::process::exit(status.code()) - } - } + pipeline.print(engine_state, stack, no_newline, false)?; info!("evaluate {}:{}:{}", file!(), line!(), column!()); diff --git a/crates/nu-cli/src/eval_file.rs b/crates/nu-cli/src/eval_file.rs index bac4378d1b..2a6542cc77 100644 --- a/crates/nu-cli/src/eval_file.rs +++ b/crates/nu-cli/src/eval_file.rs @@ -118,11 +118,7 @@ pub fn evaluate_file( }; // Print the pipeline output of the last command of the file. - if let Some(status) = pipeline.print(engine_state, stack, true, false)? { - if status.code() != 0 { - std::process::exit(status.code()) - } - } + pipeline.print(engine_state, stack, true, false)?; // Invoke the main command with arguments. // Arguments with whitespaces are quoted, thus can be safely concatenated by whitespace. @@ -140,7 +136,7 @@ pub fn evaluate_file( }; if exit_code != 0 { - std::process::exit(exit_code) + std::process::exit(exit_code); } info!("evaluate {}:{}:{}", file!(), line!(), column!()); diff --git a/crates/nu-cli/src/repl.rs b/crates/nu-cli/src/repl.rs index 28a3e2cec7..35afbce2bb 100644 --- a/crates/nu-cli/src/repl.rs +++ b/crates/nu-cli/src/repl.rs @@ -97,7 +97,7 @@ pub fn evaluate_repl( Value::string("0823", Span::unknown()), ); - unique_stack.add_env_var("LAST_EXIT_CODE".into(), Value::int(0, Span::unknown())); + unique_stack.set_last_exit_code(0, Span::unknown()); let mut line_editor = get_line_editor(engine_state, nushell_path, use_color)?; let temp_file = temp_dir().join(format!("{}.nu", uuid::Uuid::new_v4())); @@ -837,7 +837,7 @@ fn do_auto_cd( "NUSHELL_LAST_SHELL".into(), Value::int(last_shell as i64, span), ); - stack.add_env_var("LAST_EXIT_CODE".into(), Value::int(0, Span::unknown())); + stack.set_last_exit_code(0, Span::unknown()); } /// diff --git a/crates/nu-cli/src/util.rs b/crates/nu-cli/src/util.rs index e912bceb5f..6a471acc9c 100644 --- a/crates/nu-cli/src/util.rs +++ b/crates/nu-cli/src/util.rs @@ -210,18 +210,19 @@ pub fn eval_source( let start_time = std::time::Instant::now(); let exit_code = match evaluate_source(engine_state, stack, source, fname, input, allow_return) { - Ok(code) => code.unwrap_or(0), + Ok(failed) => { + let code = failed.into(); + stack.set_last_exit_code(code, Span::unknown()); + code + } Err(err) => { report_error_new(engine_state, &err); - 1 + let code = err.exit_code(); + stack.set_last_error(&err); + code } }; - stack.add_env_var( - "LAST_EXIT_CODE".to_string(), - Value::int(exit_code.into(), Span::unknown()), - ); - // reset vt processing, aka ansi because illbehaved externals can break it #[cfg(windows)] { @@ -244,7 +245,7 @@ fn evaluate_source( fname: &str, input: PipelineData, allow_return: bool, -) -> Result, ShellError> { +) -> Result { let (block, delta) = { let mut working_set = StateWorkingSet::new(engine_state); let output = parse( @@ -259,7 +260,7 @@ fn evaluate_source( if let Some(err) = working_set.parse_errors.first() { report_error(&working_set, err); - return Ok(Some(1)); + return Ok(true); } if let Some(err) = working_set.compile_errors.first() { @@ -278,25 +279,23 @@ fn evaluate_source( eval_block::(engine_state, stack, &block, input) }?; - let status = if let PipelineData::ByteStream(..) = pipeline { - pipeline.print(engine_state, stack, false, false)? + if let PipelineData::ByteStream(..) = pipeline { + pipeline.print(engine_state, stack, false, false) + } else if let Some(hook) = engine_state.get_config().hooks.display_output.clone() { + let pipeline = eval_hook( + engine_state, + stack, + Some(pipeline), + vec![], + &hook, + "display_output", + )?; + pipeline.print(engine_state, stack, false, false) } else { - if let Some(hook) = engine_state.get_config().hooks.display_output.clone() { - let pipeline = eval_hook( - engine_state, - stack, - Some(pipeline), - vec![], - &hook, - "display_output", - )?; - pipeline.print(engine_state, stack, false, false) - } else { - pipeline.print(engine_state, stack, true, false) - }? - }; + pipeline.print(engine_state, stack, true, false) + }?; - Ok(status.map(|status| status.code())) + Ok(false) } #[cfg(test)] diff --git a/crates/nu-cmd-lang/src/core_commands/do_.rs b/crates/nu-cmd-lang/src/core_commands/do_.rs index bf29a2159a..f286c50644 100644 --- a/crates/nu-cmd-lang/src/core_commands/do_.rs +++ b/crates/nu-cmd-lang/src/core_commands/do_.rs @@ -1,7 +1,7 @@ use nu_engine::{command_prelude::*, get_eval_block_with_early_return, redirect_env}; use nu_protocol::{ engine::Closure, - process::{ChildPipe, ChildProcess, ExitStatus}, + process::{ChildPipe, ChildProcess}, ByteStream, ByteStreamSource, OutDest, }; use std::{ @@ -147,13 +147,7 @@ impl Command for Do { None }; - if child.wait()? != ExitStatus::Exited(0) { - return Err(ShellError::ExternalCommand { - label: "External command failed".to_string(), - help: stderr_msg, - span, - }); - } + child.wait()?; let mut child = ChildProcess::from_raw(None, None, None, span); if let Some(stdout) = stdout { diff --git a/crates/nu-cmd-lang/src/core_commands/for_.rs b/crates/nu-cmd-lang/src/core_commands/for_.rs index 36df743e5f..c7588865cf 100644 --- a/crates/nu-cmd-lang/src/core_commands/for_.rs +++ b/crates/nu-cmd-lang/src/core_commands/for_.rs @@ -93,25 +93,10 @@ impl Command for For { stack.add_var(var_id, x); match eval_block(&engine_state, stack, block, PipelineData::empty()) { - Err(ShellError::Break { .. }) => { - break; - } - Err(ShellError::Continue { .. }) => { - continue; - } - Err(err) => { - return Err(err); - } - Ok(data) => { - if let Some(status) = data.drain()? { - let code = status.code(); - if code != 0 { - return Ok( - PipelineData::new_external_stream_with_only_exit_code(code), - ); - } - } - } + Err(ShellError::Break { .. }) => break, + Err(ShellError::Continue { .. }) => continue, + Err(err) => return Err(err), + Ok(data) => data.drain()?, } } } @@ -121,25 +106,10 @@ impl Command for For { stack.add_var(var_id, x); match eval_block(&engine_state, stack, block, PipelineData::empty()) { - Err(ShellError::Break { .. }) => { - break; - } - Err(ShellError::Continue { .. }) => { - continue; - } - Err(err) => { - return Err(err); - } - Ok(data) => { - if let Some(status) = data.drain()? { - let code = status.code(); - if code != 0 { - return Ok( - PipelineData::new_external_stream_with_only_exit_code(code), - ); - } - } - } + Err(ShellError::Break { .. }) => break, + Err(ShellError::Continue { .. }) => continue, + Err(err) => return Err(err), + Ok(data) => data.drain()?, } } } diff --git a/crates/nu-cmd-lang/src/core_commands/if_.rs b/crates/nu-cmd-lang/src/core_commands/if_.rs index 8667843770..efca26b1c2 100644 --- a/crates/nu-cmd-lang/src/core_commands/if_.rs +++ b/crates/nu-cmd-lang/src/core_commands/if_.rs @@ -127,10 +127,9 @@ impl Command for If { eval_block(engine_state, stack, block, input) } else { eval_expression_with_input(engine_state, stack, else_expr, input) - .map(|res| res.0) } } else { - eval_expression_with_input(engine_state, stack, else_case, input).map(|res| res.0) + eval_expression_with_input(engine_state, stack, else_case, input) } } else { Ok(PipelineData::empty()) diff --git a/crates/nu-cmd-lang/src/core_commands/loop_.rs b/crates/nu-cmd-lang/src/core_commands/loop_.rs index f495c8d3ae..698ecc3a5a 100644 --- a/crates/nu-cmd-lang/src/core_commands/loop_.rs +++ b/crates/nu-cmd-lang/src/core_commands/loop_.rs @@ -56,23 +56,10 @@ impl Command for Loop { engine_state.signals().check(head)?; match eval_block(engine_state, stack, block, PipelineData::empty()) { - Err(ShellError::Break { .. }) => { - break; - } - Err(ShellError::Continue { .. }) => { - continue; - } - Err(err) => { - return Err(err); - } - Ok(data) => { - if let Some(status) = data.drain()? { - let code = status.code(); - if code != 0 { - return Ok(PipelineData::new_external_stream_with_only_exit_code(code)); - } - } - } + Err(ShellError::Break { .. }) => break, + Err(ShellError::Continue { .. }) => continue, + Err(err) => return Err(err), + Ok(data) => data.drain()?, } } Ok(PipelineData::empty()) diff --git a/crates/nu-cmd-lang/src/core_commands/match_.rs b/crates/nu-cmd-lang/src/core_commands/match_.rs index c3a3d61216..ac34184bfb 100644 --- a/crates/nu-cmd-lang/src/core_commands/match_.rs +++ b/crates/nu-cmd-lang/src/core_commands/match_.rs @@ -81,7 +81,7 @@ impl Command for Match { let block = engine_state.get_block(block_id); eval_block(engine_state, stack, block, input) } else { - eval_expression_with_input(engine_state, stack, expr, input).map(|x| x.0) + eval_expression_with_input(engine_state, stack, expr, input) }; } } else { diff --git a/crates/nu-cmd-lang/src/core_commands/try_.rs b/crates/nu-cmd-lang/src/core_commands/try_.rs index 2309897a1b..7f5072a3f3 100644 --- a/crates/nu-cmd-lang/src/core_commands/try_.rs +++ b/crates/nu-cmd-lang/src/core_commands/try_.rs @@ -47,6 +47,7 @@ impl Command for Try { call: &Call, input: PipelineData, ) -> Result { + let head = call.head; // This is compiled specially by the IR compiler. The code here is never used when // running in IR mode. let call = call.assert_ast_call()?; @@ -61,30 +62,15 @@ impl Command for Try { let try_block = engine_state.get_block(try_block); let eval_block = get_eval_block(engine_state); - match eval_block(engine_state, stack, try_block, input) { - Err(error) => { - let error = intercept_block_control(error)?; - let err_record = err_to_record(error, call.head); - handle_catch(err_record, catch_block, engine_state, stack, eval_block) - } + let result = eval_block(engine_state, stack, try_block, input) + .and_then(|pipeline| pipeline.write_to_out_dests(engine_state, stack)); + + match result { + Err(err) => run_catch(err, head, catch_block, engine_state, stack, eval_block), Ok(PipelineData::Value(Value::Error { error, .. }, ..)) => { - let error = intercept_block_control(*error)?; - let err_record = err_to_record(error, call.head); - handle_catch(err_record, catch_block, engine_state, stack, eval_block) - } - // external command may fail to run - Ok(pipeline) => { - let (pipeline, external_failed) = pipeline.check_external_failed()?; - if external_failed { - let status = pipeline.drain()?; - let code = status.map(|status| status.code()).unwrap_or(0); - stack.add_env_var("LAST_EXIT_CODE".into(), Value::int(code.into(), call.head)); - let err_value = Value::nothing(call.head); - handle_catch(err_value, catch_block, engine_state, stack, eval_block) - } else { - Ok(pipeline) - } + run_catch(*error, head, catch_block, engine_state, stack, eval_block) } + Ok(pipeline) => Ok(pipeline), } } @@ -109,28 +95,33 @@ impl Command for Try { } } -fn handle_catch( - err_value: Value, - catch_block: Option, +fn run_catch( + error: ShellError, + span: Span, + catch: Option, engine_state: &EngineState, stack: &mut Stack, - eval_block_fn: EvalBlockFn, + eval_block: EvalBlockFn, ) -> Result { - if let Some(catch_block) = catch_block { - let catch_block = engine_state.get_block(catch_block.block_id); + let error = intercept_block_control(error)?; + + if let Some(catch) = catch { + stack.set_last_error(&error); + let error = err_to_record(error, span); + let block = engine_state.get_block(catch.block_id); // Put the error value in the positional closure var - if let Some(var) = catch_block.signature.get_positional(0) { + if let Some(var) = block.signature.get_positional(0) { if let Some(var_id) = &var.var_id { - stack.add_var(*var_id, err_value.clone()); + stack.add_var(*var_id, error.clone()); } } - eval_block_fn( + eval_block( engine_state, stack, - catch_block, + block, // Make the error accessible with $in, too - err_value.into_pipeline_data(), + error.into_pipeline_data(), ) } else { Ok(PipelineData::empty()) @@ -143,23 +134,28 @@ fn handle_catch( /// `Err` when flow control to bubble up with `?` fn intercept_block_control(error: ShellError) -> Result { match error { - nu_protocol::ShellError::Break { .. } => Err(error), - nu_protocol::ShellError::Continue { .. } => Err(error), - nu_protocol::ShellError::Return { .. } => Err(error), + ShellError::Break { .. } => Err(error), + ShellError::Continue { .. } => Err(error), + ShellError::Return { .. } => Err(error), _ => Ok(error), } } /// Convert from `error` to [`Value::Record`] so the error information can be easily accessed in catch. fn err_to_record(error: ShellError, head: Span) -> Value { - Value::record( - record! { - "msg" => Value::string(error.to_string(), head), - "debug" => Value::string(format!("{error:?}"), head), - "raw" => Value::error(error, head), - }, - head, - ) + let exit_code = error.external_exit_code(); + + let mut record = record! { + "msg" => Value::string(error.to_string(), head), + "debug" => Value::string(format!("{error:?}"), head), + "raw" => Value::error(error, head), + }; + + if let Some(code) = exit_code { + record.push("exit_code", Value::int(code.item.into(), code.span)); + } + + Value::record(record, head) } #[cfg(test)] diff --git a/crates/nu-cmd-lang/src/core_commands/while_.rs b/crates/nu-cmd-lang/src/core_commands/while_.rs index a67c47fcab..8280c5be5f 100644 --- a/crates/nu-cmd-lang/src/core_commands/while_.rs +++ b/crates/nu-cmd-lang/src/core_commands/while_.rs @@ -73,27 +73,10 @@ impl Command for While { let block = engine_state.get_block(block_id); match eval_block(engine_state, stack, block, PipelineData::empty()) { - Err(ShellError::Break { .. }) => { - break; - } - Err(ShellError::Continue { .. }) => { - continue; - } - Err(err) => { - return Err(err); - } - Ok(data) => { - if let Some(status) = data.drain()? { - let code = status.code(); - if code != 0 { - return Ok( - PipelineData::new_external_stream_with_only_exit_code( - code, - ), - ); - } - } - } + Err(ShellError::Break { .. }) => break, + Err(ShellError::Continue { .. }) => continue, + Err(err) => return Err(err), + Ok(data) => data.drain()?, } } else { break; diff --git a/crates/nu-command/src/debug/timeit.rs b/crates/nu-command/src/debug/timeit.rs index 7a48644a6d..42755e9a67 100644 --- a/crates/nu-command/src/debug/timeit.rs +++ b/crates/nu-command/src/debug/timeit.rs @@ -59,7 +59,7 @@ impl Command for TimeIt { } else { let eval_expression_with_input = get_eval_expression_with_input(engine_state); let expression = &command_to_run.clone(); - eval_expression_with_input(engine_state, stack, expression, input)?.0 + eval_expression_with_input(engine_state, stack, expression, input)? } } else { PipelineData::empty() diff --git a/crates/nu-command/tests/commands/do_.rs b/crates/nu-command/tests/commands/do_.rs index 5f46b02c17..645421c15f 100644 --- a/crates/nu-command/tests/commands/do_.rs +++ b/crates/nu-command/tests/commands/do_.rs @@ -12,21 +12,21 @@ fn capture_errors_works() { #[test] fn capture_errors_works_for_external() { let actual = nu!("do -c {nu --testbin fail}"); - assert!(actual.err.contains("External command failed")); + assert!(actual.err.contains("exited with code")); assert_eq!(actual.out, ""); } #[test] fn capture_errors_works_for_external_with_pipeline() { let actual = nu!("do -c {nu --testbin fail} | echo `text`"); - assert!(actual.err.contains("External command failed")); + assert!(actual.err.contains("exited with code")); assert_eq!(actual.out, ""); } #[test] fn capture_errors_works_for_external_with_semicolon() { let actual = nu!(r#"do -c {nu --testbin fail}; echo `text`"#); - assert!(actual.err.contains("External command failed")); + assert!(actual.err.contains("exited with code")); assert_eq!(actual.out, ""); } diff --git a/crates/nu-engine/src/eval.rs b/crates/nu-engine/src/eval.rs index 56a85e91e7..ccb494e68e 100644 --- a/crates/nu-engine/src/eval.rs +++ b/crates/nu-engine/src/eval.rs @@ -250,7 +250,7 @@ pub fn eval_expression_with_input( stack: &mut Stack, expr: &Expression, mut input: PipelineData, -) -> Result<(PipelineData, bool), ShellError> { +) -> Result { match &expr.expr { Expr::Call(call) => { input = eval_call::(engine_state, stack, call, input)?; @@ -298,16 +298,7 @@ pub fn eval_expression_with_input( } }; - // If input an external command, - // then `might_consume_external_result` will consume `stderr` if `stdout` is `None`. - // This should not happen if the user wants to capture stderr. - if !matches!(stack.stdout(), OutDest::Pipe | OutDest::Capture) - && matches!(stack.stderr(), OutDest::Capture) - { - Ok((input, false)) - } else { - input.check_external_failed() - } + Ok(input) } fn eval_redirection( @@ -401,9 +392,8 @@ fn eval_element_with_input_inner( stack: &mut Stack, element: &PipelineElement, input: PipelineData, -) -> Result<(PipelineData, bool), ShellError> { - let (data, failed) = - eval_expression_with_input::(engine_state, stack, &element.expr, input)?; +) -> Result { + let data = eval_expression_with_input::(engine_state, stack, &element.expr, input)?; if let Some(redirection) = element.redirection.as_ref() { let is_external = if let PipelineData::ByteStream(stream, ..) = &data { @@ -473,7 +463,7 @@ fn eval_element_with_input_inner( PipelineData::Empty => PipelineData::Empty, }; - Ok((data, failed)) + Ok(data) } fn eval_element_with_input( @@ -481,20 +471,11 @@ fn eval_element_with_input( stack: &mut Stack, element: &PipelineElement, input: PipelineData, -) -> Result<(PipelineData, bool), ShellError> { +) -> Result { D::enter_element(engine_state, element); - match eval_element_with_input_inner::(engine_state, stack, element, input) { - Ok((data, failed)) => { - let res = Ok(data); - D::leave_element(engine_state, element, &res); - res.map(|data| (data, failed)) - } - Err(err) => { - let res = Err(err); - D::leave_element(engine_state, element, &res); - res.map(|data| (data, false)) - } - } + let result = eval_element_with_input_inner::(engine_state, stack, element, input); + D::leave_element(engine_state, element, &result); + result } pub fn eval_block_with_early_return( @@ -509,7 +490,7 @@ pub fn eval_block_with_early_return( } } -pub fn eval_block( +fn eval_block_inner( engine_state: &EngineState, stack: &mut Stack, block: &Block, @@ -520,8 +501,6 @@ pub fn eval_block( return eval_ir_block::(engine_state, stack, block, input); } - D::enter_block(engine_state, block); - let num_pipelines = block.len(); for (pipeline_idx, pipeline) in block.pipelines.iter().enumerate() { @@ -542,14 +521,7 @@ pub fn eval_block( (next_out.or(Some(OutDest::Pipe)), next_err), )?; let stack = &mut stack.push_redirection(stdout, stderr); - let (output, failed) = - eval_element_with_input::(engine_state, stack, element, input)?; - if failed { - // External command failed. - // Don't return `Err(ShellError)`, so nushell won't show an extra error message. - return Ok(output); - } - input = output; + input = eval_element_with_input::(engine_state, stack, element, input)?; } if last_pipeline { @@ -560,13 +532,7 @@ pub fn eval_block( (stack.pipe_stdout().cloned(), stack.pipe_stderr().cloned()), )?; let stack = &mut stack.push_redirection(stdout, stderr); - let (output, failed) = eval_element_with_input::(engine_state, stack, last, input)?; - if failed { - // External command failed. - // Don't return `Err(ShellError)`, so nushell won't show an extra error message. - return Ok(output); - } - input = output; + input = eval_element_with_input::(engine_state, stack, last, input)?; } else { let (stdout, stderr) = eval_element_redirection::( engine_state, @@ -575,40 +541,41 @@ pub fn eval_block( (None, None), )?; let stack = &mut stack.push_redirection(stdout, stderr); - let (output, failed) = eval_element_with_input::(engine_state, stack, last, input)?; - if failed { - // External command failed. - // Don't return `Err(ShellError)`, so nushell won't show an extra error message. - return Ok(output); - } - input = PipelineData::Empty; - match output { + match eval_element_with_input::(engine_state, stack, last, input)? { PipelineData::ByteStream(stream, ..) => { let span = stream.span(); - let status = stream.drain()?; - if let Some(status) = status { - stack.add_env_var( - "LAST_EXIT_CODE".into(), - Value::int(status.code().into(), span), - ); - if status.code() != 0 { - break; - } + if let Err(err) = stream.drain() { + stack.set_last_error(&err); + return Err(err); + } else { + stack.set_last_exit_code(0, span); } } - PipelineData::ListStream(stream, ..) => { - stream.drain()?; - } + PipelineData::ListStream(stream, ..) => stream.drain()?, PipelineData::Value(..) | PipelineData::Empty => {} } + input = PipelineData::Empty; } } - D::leave_block(engine_state, block); - Ok(input) } +pub fn eval_block( + engine_state: &EngineState, + stack: &mut Stack, + block: &Block, + input: PipelineData, +) -> Result { + D::enter_block(engine_state, block); + let result = eval_block_inner::(engine_state, stack, block, input); + D::leave_block(engine_state, block); + if let Err(err) = &result { + stack.set_last_error(err); + } + result +} + pub fn eval_collect( engine_state: &EngineState, stack: &mut Stack, @@ -639,8 +606,7 @@ pub fn eval_collect( expr, // We still have to pass it as input input.into_pipeline_data_with_metadata(metadata), - ) - .map(|(result, _failed)| result); + ); stack.remove_var(var_id); diff --git a/crates/nu-engine/src/eval_helpers.rs b/crates/nu-engine/src/eval_helpers.rs index 65ebc6b61d..baa7bf432b 100644 --- a/crates/nu-engine/src/eval_helpers.rs +++ b/crates/nu-engine/src/eval_helpers.rs @@ -25,12 +25,8 @@ pub type EvalBlockWithEarlyReturnFn = pub type EvalExpressionFn = fn(&EngineState, &mut Stack, &Expression) -> Result; /// Type of eval_expression_with_input() function -pub type EvalExpressionWithInputFn = fn( - &EngineState, - &mut Stack, - &Expression, - PipelineData, -) -> Result<(PipelineData, bool), ShellError>; +pub type EvalExpressionWithInputFn = + fn(&EngineState, &mut Stack, &Expression, PipelineData) -> Result; /// Type of eval_subexpression() function pub type EvalSubexpressionFn = diff --git a/crates/nu-engine/src/eval_ir.rs b/crates/nu-engine/src/eval_ir.rs index 8550cbab99..2d142e4f3d 100644 --- a/crates/nu-engine/src/eval_ir.rs +++ b/crates/nu-engine/src/eval_ir.rs @@ -207,18 +207,6 @@ fn eval_ir_block_impl( Ok(InstructionResult::Return(reg_id)) => { return Ok(ctx.take_reg(reg_id)); } - Ok(InstructionResult::ExitCode(exit_code)) => { - if let Some(error_handler) = ctx.stack.error_handlers.pop(ctx.error_handler_base) { - // If an error handler is set, branch there - prepare_error_handler(ctx, error_handler, None); - pc = error_handler.handler_index; - } else { - // If not, exit the block with the exit code - return Ok(PipelineData::new_external_stream_with_only_exit_code( - exit_code, - )); - } - } Err( err @ (ShellError::Return { .. } | ShellError::Continue { .. } @@ -281,7 +269,6 @@ enum InstructionResult { Continue, Branch(usize), Return(RegId), - ExitCode(i32), } /// Perform an instruction @@ -790,7 +777,7 @@ fn eval_instruction( } Instruction::CheckExternalFailed { dst, src } => { let data = ctx.take_reg(*src); - let (data, failed) = data.check_external_failed()?; + let (data, failed) = todo!(); // data.check_external_failed()?; ctx.put_reg(*src, data); ctx.put_reg(*dst, Value::bool(failed, *span).into_pipeline_data()); Ok(Continue) @@ -1365,20 +1352,20 @@ fn collect(data: PipelineData, fallback_span: Span) -> Result, data: PipelineData) -> Result { use self::InstructionResult::*; - let span = data.span().unwrap_or(Span::unknown()); - if let Some(exit_status) = data.drain()? { - ctx.stack.add_env_var( - "LAST_EXIT_CODE".into(), - Value::int(exit_status.code() as i64, span), - ); - if exit_status.code() == 0 { - Ok(Continue) - } else { - Ok(ExitCode(exit_status.code())) + match data { + PipelineData::ByteStream(stream, ..) => { + let span = stream.span(); + if let Err(err) = stream.drain() { + ctx.stack.set_last_error(&err); + return Err(err); + } else { + ctx.stack.set_last_exit_code(0, span); + } } - } else { - Ok(Continue) + PipelineData::ListStream(stream, ..) => stream.drain()?, + PipelineData::Value(..) | PipelineData::Empty => {} } + Ok(Continue) } enum RedirectionStream { diff --git a/crates/nu-protocol/src/engine/stack.rs b/crates/nu-protocol/src/engine/stack.rs index 7ba8268985..26e77f4444 100644 --- a/crates/nu-protocol/src/engine/stack.rs +++ b/crates/nu-protocol/src/engine/stack.rs @@ -279,6 +279,16 @@ impl Stack { } } + pub fn set_last_exit_code(&mut self, code: i32, span: Span) { + self.add_env_var("LAST_EXIT_CODE".into(), Value::int(code.into(), span)); + } + + pub fn set_last_error(&mut self, error: &ShellError) { + if let Some(code) = error.external_exit_code() { + self.set_last_exit_code(code.item, code.span); + } + } + pub fn last_overlay_name(&self) -> Result { self.active_overlays .last() diff --git a/crates/nu-protocol/src/errors/shell_error.rs b/crates/nu-protocol/src/errors/shell_error.rs index ab01ccfa54..47fd147edb 100644 --- a/crates/nu-protocol/src/errors/shell_error.rs +++ b/crates/nu-protocol/src/errors/shell_error.rs @@ -1,6 +1,6 @@ use miette::Diagnostic; use serde::{Deserialize, Serialize}; -use std::io; +use std::{io, num::NonZeroI32}; use thiserror::Error; use crate::{ @@ -639,6 +639,49 @@ pub enum ShellError { span: Span, }, + /// An external command exited with a non-zero exit code. + /// + /// ## Resolution + /// + /// Check the external command's error message. + #[error("External command had a non-zero exit code")] + #[diagnostic(code(nu::shell::non_zero_exit_code))] + NonZeroExitCode { + exit_code: NonZeroI32, + #[label("exited with code {exit_code}")] + span: Span, + }, + + #[cfg(unix)] + /// An external command exited due to a signal. + /// + /// ## Resolution + /// + /// Check why the signal was sent or triggered. + #[error("External command was terminated by a signal")] + #[diagnostic(code(nu::shell::terminated_by_signal))] + TerminatedBySignal { + signal_name: String, + signal: i32, + #[label("terminated by {signal_name} ({signal})")] + span: Span, + }, + + #[cfg(unix)] + /// An external command core dumped. + /// + /// ## Resolution + /// + /// Check why the core dumped was triggered. + #[error("External command core dumped")] + #[diagnostic(code(nu::shell::core_dumped))] + CoreDumped { + signal_name: String, + signal: i32, + #[label("core dumped with {signal_name} ({signal})")] + span: Span, + }, + /// An operation was attempted with an input unsupported for some reason. /// /// ## Resolution @@ -876,21 +919,6 @@ pub enum ShellError { span: Span, }, - #[cfg(unix)] - /// An I/O operation failed. - /// - /// ## Resolution - /// - /// This is a generic error. Refer to the specific error message for further details. - #[error("program coredump error")] - #[diagnostic(code(nu::shell::coredump_error))] - CoredumpErrorSpanned { - msg: String, - signal: i32, - #[label("{msg}")] - span: Span, - }, - /// Tried to `cd` to a path that isn't a directory. /// /// ## Resolution @@ -1395,8 +1423,22 @@ On Windows, this would be %USERPROFILE%\AppData\Roaming"# }, } -// TODO: Implement as From trait impl ShellError { + pub fn external_exit_code(&self) -> Option> { + let (item, span) = match *self { + Self::NonZeroExitCode { exit_code, span } => (exit_code.into(), span), + Self::TerminatedBySignal { signal, span, .. } + | Self::CoreDumped { signal, span, .. } => (-signal, span), + _ => return None, + }; + Some(Spanned { item, span }) + } + + pub fn exit_code(&self) -> i32 { + self.external_exit_code().map(|e| e.item).unwrap_or(1) + } + + // TODO: Implement as From trait pub fn wrap(self, working_set: &StateWorkingSet, span: Span) -> ParseError { let msg = format_error(working_set, &self); ParseError::LabeledError( diff --git a/crates/nu-protocol/src/pipeline/byte_stream.rs b/crates/nu-protocol/src/pipeline/byte_stream.rs index bd384e88c2..e2329784b8 100644 --- a/crates/nu-protocol/src/pipeline/byte_stream.rs +++ b/crates/nu-protocol/src/pipeline/byte_stream.rs @@ -1,6 +1,6 @@ //! Module managing the streaming of raw bytes between pipeline elements use crate::{ - process::{ChildPipe, ChildProcess, ExitStatus}, + process::{ChildPipe, ChildProcess}, ErrSpan, IntoSpanned, OutDest, PipelineData, ShellError, Signals, Span, Type, Value, }; use serde::{Deserialize, Serialize}; @@ -551,14 +551,14 @@ impl ByteStream { /// /// If the source of the [`ByteStream`] is [`ByteStreamSource::Child`], /// then the [`ExitStatus`] of the [`ChildProcess`] is returned. - pub fn drain(self) -> Result, ShellError> { + pub fn drain(self) -> Result<(), ShellError> { match self.stream { ByteStreamSource::Read(read) => { copy_with_signals(read, io::sink(), self.span, &self.signals)?; - Ok(None) + Ok(()) } - ByteStreamSource::File(_) => Ok(None), - ByteStreamSource::Child(child) => Ok(Some(child.wait()?)), + ByteStreamSource::File(_) => Ok(()), + ByteStreamSource::Child(child) => child.wait(), } } @@ -566,7 +566,7 @@ impl ByteStream { /// /// If the source of the [`ByteStream`] is [`ByteStreamSource::Child`], /// then the [`ExitStatus`] of the [`ChildProcess`] is returned. - pub fn print(self, to_stderr: bool) -> Result, ShellError> { + pub fn print(self, to_stderr: bool) -> Result<(), ShellError> { if to_stderr { self.write_to(&mut io::stderr()) } else { @@ -578,17 +578,15 @@ impl ByteStream { /// /// If the source of the [`ByteStream`] is [`ByteStreamSource::Child`], /// then the [`ExitStatus`] of the [`ChildProcess`] is returned. - pub fn write_to(self, dest: impl Write) -> Result, ShellError> { + pub fn write_to(self, dest: impl Write) -> Result<(), ShellError> { let span = self.span; let signals = &self.signals; match self.stream { ByteStreamSource::Read(read) => { copy_with_signals(read, dest, span, signals)?; - Ok(None) } ByteStreamSource::File(file) => { copy_with_signals(file, dest, span, signals)?; - Ok(None) } ByteStreamSource::Child(mut child) => { // All `OutDest`s except `OutDest::Capture` will cause `stderr` to be `None`. @@ -606,36 +604,33 @@ impl ByteStream { } } } - Ok(Some(child.wait()?)) + child.wait()?; } } + Ok(()) } pub(crate) fn write_to_out_dests( self, stdout: &OutDest, stderr: &OutDest, - ) -> Result, ShellError> { + ) -> Result<(), ShellError> { let span = self.span; let signals = &self.signals; match self.stream { ByteStreamSource::Read(read) => { write_to_out_dest(read, stdout, true, span, signals)?; - Ok(None) } - ByteStreamSource::File(file) => { - match stdout { - OutDest::Pipe | OutDest::Capture | OutDest::Null => {} - OutDest::Inherit => { - copy_with_signals(file, io::stdout(), span, signals)?; - } - OutDest::File(f) => { - copy_with_signals(file, f.as_ref(), span, signals)?; - } + ByteStreamSource::File(file) => match stdout { + OutDest::Pipe | OutDest::Capture | OutDest::Null => {} + OutDest::Inherit => { + copy_with_signals(file, io::stdout(), span, signals)?; } - Ok(None) - } + OutDest::File(f) => { + copy_with_signals(file, f.as_ref(), span, signals)?; + } + }, ByteStreamSource::Child(mut child) => { match (child.stdout.take(), child.stderr.take()) { (Some(out), Some(err)) => { @@ -682,9 +677,11 @@ impl ByteStream { } (None, None) => {} } - Ok(Some(child.wait()?)) + child.wait()?; } } + + Ok(()) } } diff --git a/crates/nu-protocol/src/pipeline/pipeline_data.rs b/crates/nu-protocol/src/pipeline/pipeline_data.rs index 91fd0e1315..40dfff17e3 100644 --- a/crates/nu-protocol/src/pipeline/pipeline_data.rs +++ b/crates/nu-protocol/src/pipeline/pipeline_data.rs @@ -1,12 +1,11 @@ use crate::{ ast::{Call, PathMember}, engine::{EngineState, Stack}, - process::{ChildPipe, ChildProcess, ExitStatus}, - ByteStream, ByteStreamType, Config, ErrSpan, ListStream, OutDest, PipelineMetadata, Range, - ShellError, Signals, Span, Type, Value, + ByteStream, ByteStreamType, Config, ListStream, OutDest, PipelineMetadata, Range, ShellError, + Signals, Span, Type, Value, }; use nu_utils::{stderr_write_all_and_flush, stdout_write_all_and_flush}; -use std::io::{Cursor, Read, Write}; +use std::io::Write; const LINE_ENDING_PATTERN: &[char] = &['\r', '\n']; @@ -52,16 +51,6 @@ impl PipelineData { PipelineData::Empty } - /// create a `PipelineData::ByteStream` with proper exit_code - /// - /// It's useful to break running without raising error at user level. - pub fn new_external_stream_with_only_exit_code(exit_code: i32) -> PipelineData { - let span = Span::unknown(); - let mut child = ChildProcess::from_raw(None, None, None, span); - child.set_exit_code(exit_code); - PipelineData::ByteStream(ByteStream::child(child, span), None) - } - pub fn metadata(&self) -> Option { match self { PipelineData::Empty => None, @@ -156,14 +145,10 @@ impl PipelineData { stack: &mut Stack, ) -> Result { match (self, stack.stdout()) { - (PipelineData::ByteStream(stream, ..), stdout) => { - if let Some(exit_status) = stream.write_to_out_dests(stdout, stack.stderr())? { - return Ok(PipelineData::new_external_stream_with_only_exit_code( - exit_status.code(), - )); - } - } (data, OutDest::Pipe | OutDest::Capture) => return Ok(data), + (PipelineData::ByteStream(stream, ..), stdout) => { + stream.write_to_out_dests(stdout, stack.stderr())?; + } (PipelineData::Empty, ..) => {} (PipelineData::Value(..), OutDest::Null) => {} (PipelineData::ListStream(stream, ..), OutDest::Null) => { @@ -193,15 +178,12 @@ impl PipelineData { Ok(PipelineData::Empty) } - pub fn drain(self) -> Result, ShellError> { + pub fn drain(self) -> Result<(), ShellError> { match self { - PipelineData::Empty => Ok(None), + PipelineData::Empty => Ok(()), PipelineData::Value(Value::Error { error, .. }, ..) => Err(*error), - PipelineData::Value(..) => Ok(None), - PipelineData::ListStream(stream, ..) => { - stream.drain()?; - Ok(None) - } + PipelineData::Value(..) => Ok(()), + PipelineData::ListStream(stream, ..) => stream.drain(), PipelineData::ByteStream(stream, ..) => stream.drain(), } } @@ -462,68 +444,6 @@ impl PipelineData { } } - /// Try to catch the external command exit status and detect if it failed. - /// - /// This is useful for external commands with semicolon, we can detect errors early to avoid - /// commands after the semicolon running. - /// - /// Returns `self` and a flag that indicates if the external command run failed. If `self` is - /// not [`PipelineData::ByteStream`], the flag will be `false`. - /// - /// Currently this will consume an external command to completion. - pub fn check_external_failed(self) -> Result<(Self, bool), ShellError> { - if let PipelineData::ByteStream(stream, metadata) = self { - // Preserve stream attributes - let span = stream.span(); - let type_ = stream.type_(); - let known_size = stream.known_size(); - match stream.into_child() { - Ok(mut child) => { - // Only check children without stdout. This means that nothing - // later in the pipeline can possibly consume output from this external command. - if child.stdout.is_none() { - // Note: - // In run-external's implementation detail, the result sender thread - // send out stderr message first, then stdout message, then exit_code. - // - // In this clause, we already make sure that `stdout` is None - // But not the case of `stderr`, so if `stderr` is not None - // We need to consume stderr message before reading external commands' exit code. - // - // Or we'll never have a chance to read exit_code if stderr producer produce too much stderr message. - // So we consume stderr stream and rebuild it. - let stderr = child - .stderr - .take() - .map(|mut stderr| { - let mut buf = Vec::new(); - stderr.read_to_end(&mut buf).err_span(span)?; - Ok::<_, ShellError>(buf) - }) - .transpose()?; - - let code = child.wait()?.code(); - let mut child = ChildProcess::from_raw(None, None, None, span); - if let Some(stderr) = stderr { - child.stderr = Some(ChildPipe::Tee(Box::new(Cursor::new(stderr)))); - } - child.set_exit_code(code); - let stream = ByteStream::child(child, span).with_type(type_); - Ok((PipelineData::ByteStream(stream, metadata), code != 0)) - } else { - let stream = ByteStream::child(child, span) - .with_type(type_) - .with_known_size(known_size); - Ok((PipelineData::ByteStream(stream, metadata), false)) - } - } - Err(stream) => Ok((PipelineData::ByteStream(stream, metadata), false)), - } - } else { - Ok((self, false)) - } - } - /// Try to convert Value from Value::Range to Value::List. /// This is useful to expand Value::Range into array notation, specifically when /// converting `to json` or `to nuon`. @@ -579,7 +499,7 @@ impl PipelineData { stack: &mut Stack, no_newline: bool, to_stderr: bool, - ) -> Result, ShellError> { + ) -> Result<(), ShellError> { match self { // Print byte streams directly as long as they aren't binary. PipelineData::ByteStream(stream, ..) if stream.type_() != ByteStreamType::Binary => { @@ -609,7 +529,7 @@ impl PipelineData { engine_state: &EngineState, no_newline: bool, to_stderr: bool, - ) -> Result, ShellError> { + ) -> Result<(), ShellError> { if let PipelineData::ByteStream(stream, ..) = self { // Copy ByteStreams directly stream.print(to_stderr) @@ -633,7 +553,7 @@ impl PipelineData { } } - Ok(None) + Ok(()) } } diff --git a/crates/nu-protocol/src/process/child.rs b/crates/nu-protocol/src/process/child.rs index 08da7e704c..320427a819 100644 --- a/crates/nu-protocol/src/process/child.rs +++ b/crates/nu-protocol/src/process/child.rs @@ -23,21 +23,16 @@ impl ExitStatusFuture { ExitStatusFuture::Finished(Err(err)) => Err(err.as_ref().clone()), ExitStatusFuture::Running(receiver) => { let code = match receiver.recv() { - Ok(Ok(status)) => { - #[cfg(unix)] - if let ExitStatus::Signaled { - signal, - core_dumped: true, - } = status - { - return Err(ShellError::CoredumpErrorSpanned { - msg: format!("coredump detected. received signal: {signal}"), - signal, - span, - }); - } + #[cfg(unix)] + Ok(Ok( + status @ ExitStatus::Signaled { + core_dumped: true, .. + }, + )) => { + status.check_ok(span)?; Ok(status) } + Ok(Ok(status)) => Ok(status), Ok(Err(err)) => Err(ShellError::IOErrorSpanned { msg: format!("failed to get exit code: {err:?}"), span, @@ -187,13 +182,12 @@ impl ChildProcess { Vec::new() }; - // TODO: check exit_status - self.exit_status.wait(self.span)?; + self.exit_status.wait(self.span)?.check_ok(self.span)?; Ok(bytes) } - pub fn wait(mut self) -> Result { + pub fn wait(mut self) -> Result<(), ShellError> { if let Some(stdout) = self.stdout.take() { let stderr = self .stderr @@ -229,7 +223,7 @@ impl ChildProcess { consume_pipe(stderr).err_span(self.span)?; } - self.exit_status.wait(self.span) + self.exit_status.wait(self.span)?.check_ok(self.span) } pub fn try_wait(&mut self) -> Result, ShellError> { diff --git a/crates/nu-protocol/src/process/exit_status.rs b/crates/nu-protocol/src/process/exit_status.rs index 8f3794c44f..0ea6b0e72b 100644 --- a/crates/nu-protocol/src/process/exit_status.rs +++ b/crates/nu-protocol/src/process/exit_status.rs @@ -1,3 +1,4 @@ +use crate::{ShellError, Span}; use std::process; #[derive(Debug, Clone, Copy, PartialEq, Eq)] @@ -18,6 +19,47 @@ impl ExitStatus { ExitStatus::Signaled { signal, .. } => -signal, } } + + pub fn check_ok(self, span: Span) -> Result<(), ShellError> { + match self { + ExitStatus::Exited(exit_code) => { + if let Ok(exit_code) = exit_code.try_into() { + Err(ShellError::NonZeroExitCode { exit_code, span }) + } else { + Ok(()) + } + } + #[cfg(unix)] + ExitStatus::Signaled { + signal, + core_dumped, + } => { + use nix::sys::signal::Signal; + + let sig = Signal::try_from(signal); + + if sig == Ok(Signal::SIGPIPE) { + // Processes often exit with SIGPIPE, but this is not an error condition. + Ok(()) + } else { + let signal_name = sig.map(Signal::as_str).unwrap_or("unknown signal").into(); + Err(if core_dumped { + ShellError::CoreDumped { + signal_name, + signal, + span, + } + } else { + ShellError::TerminatedBySignal { + signal_name, + signal, + span, + } + }) + } + } + } + } } #[cfg(unix)] diff --git a/crates/nu-protocol/tests/test_config.rs b/crates/nu-protocol/tests/test_config.rs index bee944d0a2..f47dda3121 100644 --- a/crates/nu-protocol/tests/test_config.rs +++ b/crates/nu-protocol/tests/test_config.rs @@ -52,7 +52,7 @@ fn filesize_format_auto_metric_false() { #[test] fn fancy_default_errors() { - let actual = nu!(nu_repl_code(&[ + let code = nu_repl_code(&[ r#"def force_error [x] { error make { msg: "oh no!" @@ -62,8 +62,10 @@ fn fancy_default_errors() { } } }"#, - r#"force_error "My error""# - ])); + r#"force_error "My error""#, + ]); + + let actual = nu!(format!("try {{ {code} }}")); assert_eq!( actual.err, @@ -73,7 +75,7 @@ fn fancy_default_errors() { #[test] fn narratable_errors() { - let actual = nu!(nu_repl_code(&[ + let code = nu_repl_code(&[ r#"$env.config = { error_style: "plain" }"#, r#"def force_error [x] { error make { @@ -85,7 +87,9 @@ fn narratable_errors() { } }"#, r#"force_error "my error""#, - ])); + ]); + + let actual = nu!(format!("try {{ {code} }}")); assert_eq!( actual.err, diff --git a/src/run.rs b/src/run.rs index 10a5043b25..e29bb9f07e 100644 --- a/src/run.rs +++ b/src/run.rs @@ -85,7 +85,7 @@ pub(crate) fn run_commands( engine_state.generate_nu_constant(); let start_time = std::time::Instant::now(); - if let Err(err) = evaluate_commands( + let result = evaluate_commands( commands, engine_state, &mut stack, @@ -95,11 +95,13 @@ pub(crate) fn run_commands( error_style: parsed_nu_cli_args.error_style, no_newline: parsed_nu_cli_args.no_newline.is_some(), }, - ) { - report_error_new(engine_state, &err); - std::process::exit(1); - } + ); perf!("evaluate_commands", start_time, use_color); + + if let Err(err) = result { + report_error_new(engine_state, &err); + std::process::exit(err.exit_code()); + } } pub(crate) fn run_file( @@ -158,29 +160,19 @@ pub(crate) fn run_file( engine_state.generate_nu_constant(); let start_time = std::time::Instant::now(); - if let Err(err) = evaluate_file( + let result = evaluate_file( script_name, &args_to_script, engine_state, &mut stack, input, - ) { - report_error_new(engine_state, &err); - std::process::exit(1); - } + ); perf!("evaluate_file", start_time, use_color); - let start_time = std::time::Instant::now(); - let last_exit_code = stack.get_env_var(&*engine_state, "LAST_EXIT_CODE"); - if let Some(last_exit_code) = last_exit_code { - let value = last_exit_code.as_int(); - if let Ok(value) = value { - if value != 0 { - std::process::exit(value as i32); - } - } + if let Err(err) = result { + report_error_new(engine_state, &err); + std::process::exit(err.exit_code()); } - perf!("get exit code", start_time, use_color); } pub(crate) fn run_repl( diff --git a/tests/shell/pipeline/commands/external.rs b/tests/shell/pipeline/commands/external.rs index ff851a3293..bef58adbc6 100644 --- a/tests/shell/pipeline/commands/external.rs +++ b/tests/shell/pipeline/commands/external.rs @@ -173,13 +173,6 @@ fn basic_outerr_pipe_works(#[case] redirection: &str) { assert_eq!(actual.out, "7"); } -#[test] -fn err_pipe_with_failed_external_works() { - let actual = - nu!(r#"with-env { FOO: "bar" } { nu --testbin echo_env_stderr_fail FOO e>| str length }"#); - assert_eq!(actual.out, "3"); -} - #[test] fn dont_run_glob_if_pass_variable_to_external() { Playground::setup("dont_run_glob", |dirs, sandbox| {