From 4851794c55e6e5ba5c5f27bc7eb819e6f5880735 Mon Sep 17 00:00:00 2001 From: Ian Manske Date: Thu, 16 May 2024 22:14:11 -0400 Subject: [PATCH] Make non-zero exit codes errors --- crates/nu-cli/src/config_files.rs | 3 +- crates/nu-cli/src/eval_cmds.rs | 6 +- crates/nu-cli/src/eval_file.rs | 8 +- crates/nu-cli/src/util.rs | 59 +++++----- crates/nu-cmd-lang/src/core_commands/do_.rs | 10 +- crates/nu-cmd-lang/src/core_commands/for_.rs | 46 ++------ crates/nu-cmd-lang/src/core_commands/if_.rs | 3 +- crates/nu-cmd-lang/src/core_commands/loop_.rs | 21 +--- .../nu-cmd-lang/src/core_commands/match_.rs | 2 +- crates/nu-cmd-lang/src/core_commands/try_.rs | 59 +++++----- .../nu-cmd-lang/src/core_commands/while_.rs | 25 +---- crates/nu-command/src/debug/timeit.rs | 2 +- crates/nu-engine/src/eval.rs | 85 ++++----------- crates/nu-engine/src/eval_helpers.rs | 8 +- crates/nu-engine/src/eval_ir.rs | 40 +++---- crates/nu-protocol/src/engine/stack.rs | 8 ++ crates/nu-protocol/src/errors/shell_error.rs | 18 ++-- .../nu-protocol/src/pipeline/byte_stream.rs | 45 ++++---- .../nu-protocol/src/pipeline/pipeline_data.rs | 102 ++---------------- crates/nu-protocol/src/process/child.rs | 3 +- crates/nu-protocol/src/process/exit_status.rs | 34 ++++++ src/run.rs | 32 +++--- 22 files changed, 218 insertions(+), 401 deletions(-) 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/util.rs b/crates/nu-cli/src/util.rs index e912bceb5f..5ee0bdd3c0 100644 --- a/crates/nu-cli/src/util.rs +++ b/crates/nu-cli/src/util.rs @@ -210,18 +210,22 @@ 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 = i32::from(failed); + stack.add_env_var( + "LAST_EXIT_CODE".into(), + Value::int(code.into(), Span::unknown()), + ); + code + } Err(err) => { report_error_new(engine_state, &err); - 1 + let code = err.exit_code(); + stack.set_last_exit_code(&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 +248,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 +263,12 @@ 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() { + report_error(&working_set, err); + // Not a fatal error, for now } if let Some(err) = working_set.compile_errors.first() { @@ -278,25 +287,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..0f83d5e456 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()?.check_ok(span)?; 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..0c6e8b174f 100644 --- a/crates/nu-cmd-lang/src/core_commands/try_.rs +++ b/crates/nu-cmd-lang/src/core_commands/try_.rs @@ -62,29 +62,16 @@ impl Command for Try { 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) - } - 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) - } - } + Err(err) => run_catch(err, call.head, catch_block, engine_state, stack, eval_block), + Ok(PipelineData::Value(Value::Error { error, .. }, ..)) => run_catch( + *error, + call.head, + catch_block, + engine_state, + stack, + eval_block, + ), + Ok(pipeline) => Ok(pipeline), } } @@ -109,28 +96,32 @@ 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 { + 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()) 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-engine/src/eval.rs b/crates/nu-engine/src/eval.rs index 56a85e91e7..9384931cf7 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( @@ -542,14 +523,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 +534,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,32 +543,20 @@ 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_exit_code(&err); + return Err(err); + } else { + stack.add_env_var("LAST_EXIT_CODE".into(), Value::int(0, span)); } } - PipelineData::ListStream(stream, ..) => { - stream.drain()?; - } + PipelineData::ListStream(stream, ..) => stream.drain()?, PipelineData::Value(..) | PipelineData::Empty => {} } + input = PipelineData::Empty; } } @@ -639,8 +595,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..d731b71ee9 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,21 @@ 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_exit_code(&err); + return Err(err); + } else { + ctx.stack + .add_env_var("LAST_EXIT_CODE".into(), Value::int(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..fd3bd70c1e 100644 --- a/crates/nu-protocol/src/engine/stack.rs +++ b/crates/nu-protocol/src/engine/stack.rs @@ -279,6 +279,14 @@ impl Stack { } } + pub fn set_last_exit_code(&mut self, error: &ShellError) { + let code = error.exit_code_spanned(); + self.add_env_var( + "LAST_EXIT_CODE".into(), + Value::int(code.item.into(), 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 e7e7e49cbf..01be884044 100644 --- a/crates/nu-protocol/src/errors/shell_error.rs +++ b/crates/nu-protocol/src/errors/shell_error.rs @@ -1437,14 +1437,18 @@ On Windows, this would be %USERPROFILE%\AppData\Roaming"# } impl ShellError { + pub fn exit_code_spanned(&self) -> Spanned { + let (item, span) = match *self { + Self::NonZeroExitCode { exit_code, span } => (exit_code, span), + Self::ProcessSignaled { signal, span, .. } + | Self::ProcessCoreDumped { signal, span, .. } => (-signal, span), + _ => (1, Span::unknown()), // TODO: better span here + }; + Spanned { item, span } + } + pub fn exit_code(&self) -> i32 { - match *self { - Self::NonZeroExitCode { exit_code, .. } => exit_code, - Self::ProcessSignaled { signal, .. } | Self::ProcessCoreDumped { signal, .. } => { - -signal - } - _ => 1, - } + self.exit_code_spanned().item } // TODO: Implement as From trait diff --git a/crates/nu-protocol/src/pipeline/byte_stream.rs b/crates/nu-protocol/src/pipeline/byte_stream.rs index bd384e88c2..2ff9c286bc 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()?.check_ok(self.span), } } @@ -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()?.check_ok(span)?; } } + 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()?.check_ok(span)?; } } + + Ok(()) } } diff --git a/crates/nu-protocol/src/pipeline/pipeline_data.rs b/crates/nu-protocol/src/pipeline/pipeline_data.rs index 91fd0e1315..48638e15ac 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, @@ -157,11 +146,7 @@ impl PipelineData { ) -> 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(), - )); - } + stream.write_to_out_dests(stdout, stack.stderr())?; } (data, OutDest::Pipe | OutDest::Capture) => return Ok(data), (PipelineData::Empty, ..) => {} @@ -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..3f287c08cf 100644 --- a/crates/nu-protocol/src/process/child.rs +++ b/crates/nu-protocol/src/process/child.rs @@ -187,8 +187,7 @@ 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) } diff --git a/crates/nu-protocol/src/process/exit_status.rs b/crates/nu-protocol/src/process/exit_status.rs index 8f3794c44f..c2b037cfac 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,39 @@ impl ExitStatus { ExitStatus::Signaled { signal, .. } => -signal, } } + + pub fn check_ok(self, span: Span) -> Result<(), ShellError> { + match self { + ExitStatus::Exited(0) => Ok(()), + ExitStatus::Exited(exit_code) => Err(ShellError::NonZeroExitCode { exit_code, span }), + #[cfg(unix)] + ExitStatus::Signaled { + signal, + core_dumped, + } => { + use nix::sys::signal::Signal; + + let signal_name = Signal::try_from(signal) + .map(Signal::as_str) + .unwrap_or("unknown signal") + .into(); + + Err(if core_dumped { + ShellError::ProcessCoreDumped { + signal_name, + signal, + span, + } + } else { + ShellError::ProcessSignaled { + signal_name, + signal, + span, + } + }) + } + } + } } #[cfg(unix)] 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(