Skip to content

Commit 75a8a2c

Browse files
fixererFixerer
andauthored
Fix ir eval error on branched block redirection (Fixes nushell#16582) (nushell#16676)
### Description Attempt to fix issue [nushell#16582](nushell#16582) by avoiding duplicated `write` and `close` calls for an `out>`/`err>` redirected file for `try-catch` and `if-else` blocks. Similarly to [this PR](nushell#15617) a function to identify if a code block is being handeled, and if so skip the invocation of `finish_redirection` so that no additional writes are added. (The blocks perform redirections on their own and this then avoids the duplication). ### Release notes summary - What our users need to know The following commands will no longer raise ir-eval errors: - `if true { echo "hey" } out> /dev/null` - `if false { echo "hey" } else { echo "ho" } out> /dev/null` - `try { 1/0 } catch { echo "hi" } out> /dev/null` IR before changes: [before.txt](https://github.com/user-attachments/files/22308084/before.txt) IR after changes: [after.txt](https://github.com/user-attachments/files/22308085/after.txt) ### Tests Added 8 tests. - 🟢 `toolkit fmt` - 🟢 `toolkit clippy` - 🟢 `toolkit test` - 🟢 `toolkit test stdlib` --------- Co-authored-by: Fixerer <[email protected]>
1 parent 21a54d4 commit 75a8a2c

File tree

2 files changed

+166
-5
lines changed

2 files changed

+166
-5
lines changed

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

Lines changed: 149 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -476,7 +476,7 @@ fn pipe_redirection_in_let_and_mut(
476476

477477
#[rstest::rstest]
478478
#[case("o>", "bar")]
479-
#[case("e>", "")]
479+
#[case("e>", "baz")]
480480
#[case("o+e>", "bar\nbaz")] // in subexpression, the stderr is go to the terminal
481481
fn subexpression_redirection(#[case] redir: &str, #[case] stdout_file_body: &str) {
482482
Playground::setup("file redirection with subexpression", |dirs, _| {
@@ -485,6 +485,153 @@ fn subexpression_redirection(#[case] redir: &str, #[case] stdout_file_body: &str
485485
format!("$env.BAR = 'bar'; $env.BAZ = 'baz'; (nu --testbin echo_env_mixed out-err BAR BAZ) {redir} result.txt")
486486
);
487487
assert!(actual.status.success());
488-
assert!(file_contents(dirs.test().join("result.txt")).contains(stdout_file_body));
488+
assert_eq!(
489+
file_contents(dirs.test().join("result.txt")).trim(),
490+
stdout_file_body
491+
);
492+
})
493+
}
494+
495+
#[rstest::rstest]
496+
#[case("o>", "bar")]
497+
#[case("e>", "baz")]
498+
#[case("o+e>", "bar\nbaz")]
499+
fn file_redirection_in_if_true(#[case] redir: &str, #[case] stdout_file_body: &str) {
500+
Playground::setup("file redirection if true block", |dirs, _| {
501+
let actual = nu!(
502+
cwd: dirs.test(),
503+
format!("$env.BAR = 'bar'; $env.BAZ = 'baz'; if true {{ nu --testbin echo_env_mixed out-err BAR BAZ }} {redir} result.txt")
504+
);
505+
assert!(actual.status.success());
506+
assert_eq!(
507+
file_contents(dirs.test().join("result.txt")).trim(),
508+
stdout_file_body
509+
);
510+
})
511+
}
512+
513+
#[rstest::rstest]
514+
#[case(true, "hey")]
515+
#[case(false, "ho")]
516+
fn file_redirection_in_if_else(#[case] cond: bool, #[case] stdout_file_body: &str) {
517+
Playground::setup("file redirection if-else block", |dirs, _| {
518+
let actual = nu!(
519+
cwd: dirs.test(),
520+
format!("if {cond} {{ echo 'hey' }} else {{ echo 'ho' }} out> result.txt")
521+
);
522+
assert!(actual.status.success());
523+
assert_eq!(
524+
file_contents(dirs.test().join("result.txt")).trim(),
525+
stdout_file_body
526+
);
527+
})
528+
}
529+
530+
#[rstest::rstest]
531+
#[case("o>", "bar")]
532+
#[case("e>", "baz")]
533+
#[case("o+e>", "bar\nbaz")]
534+
fn file_redirection_in_try_catch(#[case] redir: &str, #[case] stdout_file_body: &str) {
535+
Playground::setup("file redirection try-catch block", |dirs, _| {
536+
let actual = nu!(
537+
cwd: dirs.test(),
538+
format!("$env.BAR = 'bar'; $env.BAZ = 'baz'; try {{ 1/0 }} catch {{ nu --testbin echo_env_mixed out-err BAR BAZ }} {redir} result.txt")
539+
);
540+
assert!(actual.status.success());
541+
assert_eq!(
542+
file_contents(dirs.test().join("result.txt")).trim(),
543+
stdout_file_body
544+
);
545+
})
546+
}
547+
548+
#[rstest::rstest]
549+
fn file_redirection_where_closure() {
550+
Playground::setup("file redirection where closure", |dirs, _| {
551+
let actual = nu!(
552+
cwd: dirs.test(),
553+
format!("echo foo bar | where {{|x| $x | str contains 'f'}} out> result.txt")
554+
);
555+
assert!(actual.status.success());
556+
assert_eq!(file_contents(dirs.test().join("result.txt")).trim(), "foo");
557+
})
558+
}
559+
560+
#[rstest::rstest]
561+
fn file_redirection_match_block() {
562+
Playground::setup("file redirection match block", |dirs, _| {
563+
let actual = nu!(
564+
cwd: dirs.test(),
565+
format!("match 3 {{ 1 => 'foo', 3 => 'bar' }} out> result.txt")
566+
);
567+
assert!(actual.status.success());
568+
assert_eq!(file_contents(dirs.test().join("result.txt")).trim(), "bar");
569+
})
570+
}
571+
572+
#[rstest::rstest]
573+
fn file_redirection_pattern_match_block() {
574+
Playground::setup("file redirection pattern match block", |dirs, _| {
575+
let actual = nu!(
576+
cwd: dirs.test(),
577+
format!("let foo = {{ name: 'bar' }}; match $foo {{ {{ name: 'bar' }} => 'baz' }} out> result.txt")
578+
);
579+
assert!(actual.status.success());
580+
assert_eq!(file_contents(dirs.test().join("result.txt")).trim(), "baz");
581+
})
582+
}
583+
584+
#[rstest::rstest]
585+
fn file_redirection_each_block() {
586+
Playground::setup("file redirection each block", |dirs, _| {
587+
let actual = nu!(
588+
cwd: dirs.test(),
589+
format!("[1 2 3] | each {{ $in + 1 }} out> result.txt")
590+
);
591+
assert!(actual.status.success());
592+
assert_eq!(
593+
file_contents(dirs.test().join("result.txt")).trim(),
594+
"2\n3\n4"
595+
);
596+
})
597+
}
598+
599+
#[rstest::rstest]
600+
fn file_redirection_do_block_with_return() {
601+
Playground::setup("file redirection do block with return", |dirs, _| {
602+
let actual = nu!(
603+
cwd: dirs.test(),
604+
format!("do {{|x| return ($x + 1); return $x}} 4 out> result.txt")
605+
);
606+
assert!(actual.status.success());
607+
assert_eq!(file_contents(dirs.test().join("result.txt")).trim(), "5");
608+
})
609+
}
610+
611+
#[rstest::rstest]
612+
fn file_redirection_while_block() {
613+
Playground::setup("file redirection on while", |dirs, _| {
614+
let actual = nu!(
615+
cwd: dirs.test(),
616+
format!("mut x = 0; while $x < 3 {{ $x = $x + 1; echo $x }} o> result.txt")
617+
);
618+
// Why the discrepancy to `for` behaviour?
619+
assert!(actual.status.success());
620+
assert_eq!(file_contents(dirs.test().join("result.txt")), "");
621+
})
622+
}
623+
624+
#[rstest::rstest]
625+
fn file_redirection_not_allowed_on_for() {
626+
Playground::setup("file redirection disallowed on for", |dirs, _| {
627+
let actual = nu!(
628+
cwd: dirs.test(),
629+
format!("for $it in [1 2 3] {{ echo $in + 1 }} out> result.txt")
630+
);
631+
assert!(!actual.status.success());
632+
assert!(
633+
actual.err.contains("Redirection can not be used with for"),
634+
"Should fail"
635+
);
489636
})
490637
}

crates/nu-engine/src/compile/mod.rs

Lines changed: 17 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -194,9 +194,9 @@ fn compile_pipeline(
194194
out_reg,
195195
)?;
196196

197-
// only clean up the redirection if current element is not
198-
// a subexpression. The subexpression itself already clean it.
199-
if !is_subexpression(&element.expr.expr) {
197+
// Only clean up the redirection if current element is NOT
198+
// a nested eval expression, since this already cleans it.
199+
if !has_nested_eval_expr(&element.expr.expr) {
200200
// Clean up the redirection
201201
finish_redirection(builder, redirect_modes, out_reg)?;
202202
}
@@ -207,6 +207,20 @@ fn compile_pipeline(
207207
Ok(())
208208
}
209209

210+
fn has_nested_eval_expr(expr: &Expr) -> bool {
211+
is_subexpression(expr) || is_block_call(expr)
212+
}
213+
214+
fn is_block_call(expr: &Expr) -> bool {
215+
match expr {
216+
Expr::Call(inner) => inner
217+
.arguments
218+
.iter()
219+
.any(|arg| matches!(arg.expr().map(|e| &e.expr), Some(Expr::Block(..)))),
220+
_ => false,
221+
}
222+
}
223+
210224
fn is_subexpression(expr: &Expr) -> bool {
211225
match expr {
212226
Expr::FullCellPath(inner) => {

0 commit comments

Comments
 (0)