From 493850b1bf12f417a059094932173b3805561760 Mon Sep 17 00:00:00 2001 From: Ian Manske Date: Mon, 9 Sep 2024 19:44:04 -0700 Subject: [PATCH] Fix IR for `try` (#13811) # Description Fixes a bug in the IR for `try` to match that of the regular evaluator (continuing from #13515): ```nushell # without IR: try { ^false } catch { 'caught' } # == 'caught' # with IR: try { ^false } catch { 'caught' } # error, non-zero exit code ``` In this PR, both now evaluate to `caught`. For the implementation, I had to add another instruction, and feel free to suggest better alternatives. In the future, it might be possible to get rid of this extra instruction. # User-Facing Changes Bug fix, `try { ^false } catch { 'caught' }` now works in IR. --- crates/nu-command/tests/commands/try_.rs | 4 ++-- crates/nu-engine/src/compile/builder.rs | 1 + crates/nu-engine/src/compile/keyword.rs | 14 +++++++++++++- crates/nu-engine/src/eval_ir.rs | 11 +++++++++++ crates/nu-protocol/src/ir/display.rs | 3 +++ crates/nu-protocol/src/ir/mod.rs | 4 ++++ crates/nu-protocol/tests/test_config.rs | 4 ++-- 7 files changed, 36 insertions(+), 5 deletions(-) diff --git a/crates/nu-command/tests/commands/try_.rs b/crates/nu-command/tests/commands/try_.rs index 264b20796f..58d1c8f80a 100644 --- a/crates/nu-command/tests/commands/try_.rs +++ b/crates/nu-command/tests/commands/try_.rs @@ -96,12 +96,12 @@ fn can_catch_infinite_recursion() { #[test] fn exit_code_available_in_catch_env() { - let actual = nu!("try { nu -c 'exit 42'; null } catch { $env.LAST_EXIT_CODE }"); + let actual = nu!("try { nu -c 'exit 42' } catch { $env.LAST_EXIT_CODE }"); assert_eq!(actual.out, "42"); } #[test] fn exit_code_available_in_catch() { - let actual = nu!("try { nu -c 'exit 42'; null } catch { |e| $e.exit_code }"); + let actual = nu!("try { nu -c 'exit 42' } catch { |e| $e.exit_code }"); assert_eq!(actual.out, "42"); } diff --git a/crates/nu-engine/src/compile/builder.rs b/crates/nu-engine/src/compile/builder.rs index 655ffbddc5..cedebd0fb2 100644 --- a/crates/nu-engine/src/compile/builder.rs +++ b/crates/nu-engine/src/compile/builder.rs @@ -202,6 +202,7 @@ impl BlockBuilder { Instruction::Span { src_dst } => allocate(&[*src_dst], &[*src_dst]), Instruction::Drop { src } => allocate(&[*src], &[]), Instruction::Drain { src } => allocate(&[*src], &[]), + Instruction::WriteToOutDests { src } => allocate(&[*src], &[]), Instruction::LoadVariable { dst, var_id: _ } => allocate(&[], &[*dst]), Instruction::StoreVariable { var_id: _, src } => allocate(&[*src], &[]), Instruction::DropVariable { var_id: _ } => Ok(()), diff --git a/crates/nu-engine/src/compile/keyword.rs b/crates/nu-engine/src/compile/keyword.rs index 8fe939d638..f9a81ab6d1 100644 --- a/crates/nu-engine/src/compile/keyword.rs +++ b/crates/nu-engine/src/compile/keyword.rs @@ -362,6 +362,7 @@ pub(crate) fn compile_try( // // on-error-into ERR, %io_reg // or without // %io_reg <- <...block...> <- %io_reg + // write-to-out-dests %io_reg // pop-error-handler // jump END // ERR: clone %err_reg, %io_reg @@ -374,6 +375,7 @@ pub(crate) fn compile_try( // %closure_reg <- // on-error-into ERR, %io_reg // %io_reg <- <...block...> <- %io_reg + // write-to-out-dests %io_reg // pop-error-handler // jump END // ERR: clone %err_reg, %io_reg @@ -461,7 +463,17 @@ pub(crate) fn compile_try( io_reg, )?; - // Successful case: pop the error handler + // Successful case: + // - write to the current output destinations + // - pop the error handler + if let Some(mode) = redirect_modes.out { + builder.push(mode.map(|mode| Instruction::RedirectOut { mode }))?; + } + + if let Some(mode) = redirect_modes.err { + builder.push(mode.map(|mode| Instruction::RedirectErr { mode }))?; + } + builder.push(Instruction::WriteToOutDests { src: io_reg }.into_spanned(call.head))?; builder.push(Instruction::PopErrorHandler.into_spanned(call.head))?; // Jump over the failure case diff --git a/crates/nu-engine/src/eval_ir.rs b/crates/nu-engine/src/eval_ir.rs index 34ec541a14..62e9b0caad 100644 --- a/crates/nu-engine/src/eval_ir.rs +++ b/crates/nu-engine/src/eval_ir.rs @@ -317,6 +317,17 @@ fn eval_instruction( let data = ctx.take_reg(*src); drain(ctx, data) } + Instruction::WriteToOutDests { src } => { + let data = ctx.take_reg(*src); + let res = { + let stack = &mut ctx + .stack + .push_redirection(ctx.redirect_out.clone(), ctx.redirect_err.clone()); + data.write_to_out_dests(ctx.engine_state, stack)? + }; + ctx.put_reg(*src, res); + Ok(Continue) + } Instruction::LoadVariable { dst, var_id } => { let value = get_var(ctx, *var_id, *span)?; ctx.put_reg(*dst, value.into_pipeline_data()); diff --git a/crates/nu-protocol/src/ir/display.rs b/crates/nu-protocol/src/ir/display.rs index af521e162f..feac8f961d 100644 --- a/crates/nu-protocol/src/ir/display.rs +++ b/crates/nu-protocol/src/ir/display.rs @@ -94,6 +94,9 @@ impl<'a> fmt::Display for FmtInstruction<'a> { Instruction::Drain { src } => { write!(f, "{:WIDTH$} {src}", "drain") } + Instruction::WriteToOutDests { src } => { + write!(f, "{:WIDTH$} {src}", "write-to-out-dests") + } Instruction::LoadVariable { dst, var_id } => { let var = FmtVar::new(self.engine_state, *var_id); write!(f, "{:WIDTH$} {dst}, {var}", "load-variable") diff --git a/crates/nu-protocol/src/ir/mod.rs b/crates/nu-protocol/src/ir/mod.rs index 214632b44d..c2f51f514a 100644 --- a/crates/nu-protocol/src/ir/mod.rs +++ b/crates/nu-protocol/src/ir/mod.rs @@ -123,6 +123,9 @@ pub enum Instruction { /// code, and invokes any available error handler with Empty, or if not available, returns an /// exit-code-only stream, leaving the block. Drain { src: RegId }, + /// Write to the output destinations in the stack + // TODO: see if it's possible to remove this + WriteToOutDests { src: RegId }, /// Load the value of a variable into the `dst` register LoadVariable { dst: RegId, var_id: VarId }, /// Store the value of a variable from the `src` register @@ -287,6 +290,7 @@ impl Instruction { Instruction::Span { src_dst } => Some(src_dst), Instruction::Drop { .. } => None, Instruction::Drain { .. } => None, + Instruction::WriteToOutDests { .. } => None, Instruction::LoadVariable { dst, .. } => Some(dst), Instruction::StoreVariable { .. } => None, Instruction::DropVariable { .. } => None, diff --git a/crates/nu-protocol/tests/test_config.rs b/crates/nu-protocol/tests/test_config.rs index ed44b36581..f47dda3121 100644 --- a/crates/nu-protocol/tests/test_config.rs +++ b/crates/nu-protocol/tests/test_config.rs @@ -65,7 +65,7 @@ fn fancy_default_errors() { r#"force_error "My error""#, ]); - let actual = nu!(format!("try {{ {code}; null }}")); + let actual = nu!(format!("try {{ {code} }}")); assert_eq!( actual.err, @@ -89,7 +89,7 @@ fn narratable_errors() { r#"force_error "my error""#, ]); - let actual = nu!(format!("try {{ {code}; null }}")); + let actual = nu!(format!("try {{ {code} }}")); assert_eq!( actual.err,