pipefail: avoid try outputs value when catch exists, avoid finally runs twice, avoid finally funs before try finish. (#17764)

Fixes: #17730 
Fixes: #17715 
Fixes: #17567

This pr is going to do the following things in order to fix these issues
at once:
1. Making `Instruction::Collect` ignore `pipefail` checking, so anything
invoves *collect* won't check pipefail, for example: let, argument
passing, binary operation, match expression
2. Restrict try block from streaming if it contains `catch` block or
`finally` block. This is done by adding a new
`Instruction::CollectFailuable` after relative block. It is similar to
`Instruction::Collect`, except it checks pipefail.
3. Before running `finally` block, evaluate `PopFinallyRun` instruction
first, this avoids duplicate finally block runs.

## Release notes summary - What our users need to know
### finally block won't run twice if error produced in finally block
```
> try {
} finally {
    print "inside finally"
    error make
}
```
It prints "inside finally" once.
### finally block won't run before try/catch finished
```
try {
    ^ping -n 3 127.0.0.1   # or `^ping -c 3 127.0.0.1` on Linux/macOS
} finally {
    print "finally ran"
}
```
Finally won't run before try finished.
NOTES ABOUT STREAMING:
*try* won't streaming if `catch` or `finally` exists.
*catch* won't streaming if `finally` exists

### let ignores pipefail checking
```
> sh -c "echo 'hello\nworld' && false" | lines | let x
```
The variable `x` will be assigned.
### Try doesn't produce output if it has an error
```
> try { bash -c 'exit 1' | is-empty } catch { 'result-on-catch' }
result-on-catch
```
It returns `result-on-catch`, and redundant `true` won't be outputed.

## Tasks after submitting
NaN

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

@@ -228,11 +228,22 @@ fn get_json_error() {
 
 #[test]
 fn pipefail_works() {
+    // the print 'bbb' should not run because the previous command failed
+    // So no output should be printed
+    let actual = nu!(
+        experimental: vec!["pipefail".to_string()],
+        "nu --testbin fail | lines | length; print 'bbb'"
+    );
+    assert_eq!(actual.out, "")
+}
+
+#[test]
+fn let_ignores_pipefail() {
     let actual = nu!(
         experimental: vec!["pipefail".to_string()],
         "try { let x = nu --testbin fail | lines | length; print $x } catch {|e| print $e.exit_code}"
     );
-    assert_eq!(actual.out, "1")
+    assert_eq!(actual.out, "0")
 }
 
 #[test]
@@ -342,3 +353,54 @@ fn catch_finally_with_variable() {
     );
     assert_eq!(actual.out, "true");
 }
+
+#[test]
+fn finally_should_not_run_before_try_finished() {
+    let actual = nu!(
+        experimental: vec!["pipefail".to_string()],
+        r#"
+        with-env { FOO: 'bar' } {
+            try { nu --testbin echo_env FOO } finally { print 'bb' }
+        }
+        "#
+    );
+    assert_eq!(actual.out, "barbb")
+}
+
+#[test]
+fn finally_should_not_run_before_catch_finished() {
+    let actual = nu!(
+        experimental: vec!["pipefail".to_string()],
+        r#"
+        with-env { FOO: 'bar' } {
+            try { 1 / 0 } catch { nu --testbin echo_env FOO } finally { print 'bb' }
+        }
+        "#
+    );
+    assert_eq!(actual.out, "barbb")
+}
+
+#[test]
+fn finally_should_not_run_twice_when_error_in_finally() {
+    let actual = nu!(
+        experimental: vec!["pipefail".to_string()],
+        r#"
+        try {
+            ^true
+        } finally {
+            print "inside finally"
+            error make -u "oh no"
+        }
+        "#
+    );
+    assert_eq!(actual.out, "inside finally")
+}
+
+#[test]
+fn try_wont_generate_extra_output() {
+    let actual = nu!(
+        experimental: vec!["pipefail".to_string()],
+        "try { nu --testbin fail | is-empty } catch { 'here' }"
+    );
+    assert_eq!(actual.out, "here")
+}

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

@@ -204,6 +204,7 @@ impl BlockBuilder {
             Instruction::Move { dst, src } => allocate(&[*src], &[*dst]),
             Instruction::Clone { dst, src } => allocate(&[*src], &[*dst, *src]),
             Instruction::Collect { src_dst } => allocate(&[*src_dst], &[*src_dst]),
+            Instruction::TryCollect { src_dst } => allocate(&[*src_dst], &[*src_dst]),
             Instruction::Span { src_dst } => allocate(&[*src_dst], &[*src_dst]),
             Instruction::Drop { src } => allocate(&[*src], &[]),
             Instruction::Drain { src } => allocate(&[*src], &[]),

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

@@ -427,7 +427,7 @@ pub(crate) fn compile_try(
     //       on-error-into ERR, %io_reg           // or without
     //       finally-into  FINALLY, $io_reg       // or without
     //       %io_reg <- <...block...> <- %io_reg
-    //       write-to-out-dests %io_reg
+    //       try-collect %io_reg
     //       pop-error-handler
     //       jump END
     // ERR:  clone %err_reg, %io_reg
@@ -442,7 +442,7 @@ pub(crate) fn compile_try(
     //       on-error-into ERR, %io_reg
     //       finally-into  FINALLY, $io_reg
     //       %io_reg <- <...block...> <- %io_reg
-    //       write-to-out-dests %io_reg
+    //       try-collect %io_reg
     //       pop-error-handler
     //       jump END
     // ERR:  clone %err_reg, %io_reg
@@ -601,7 +601,12 @@ pub(crate) fn compile_try(
         builder.push(mode.map(|mode| Instruction::RedirectErr { mode }))?;
     }
 
-    if finally_type.is_none() {
+    if finally_type.is_some() || catch_type.is_some() {
+        // For `catch` clause and `finally` clause, we need to know if the `try` block
+        // runs successfully first, so we need to collect the output first to check if there
+        // is an error.
+        builder.push(Instruction::TryCollect { src_dst: io_reg }.into_spanned(call.head))?;
+    } else {
         builder.push(Instruction::DrainIfEnd { src: io_reg }.into_spanned(call.head))?;
     }
     if catch_expr.is_some() {
@@ -661,11 +666,15 @@ pub(crate) fn compile_try(
         }
     }
     if finally_type.is_some() {
-        builder.push(Instruction::PopFinallyRun.into_spanned(call.head))?;
+        builder.push(Instruction::TryCollect { src_dst: io_reg }.into_spanned(call.head))?;
     }
 
     // This is the end - whatever we succeeded or not, should jump here for finally clause.
     builder.set_label(end_label, builder.here())?;
+    if finally_type.is_some() {
+        builder.push(Instruction::PopFinallyRun.into_spanned(call.head))?;
+    }
+
     // This is the finally part.
     if let Some(finally_part) = finally_type {
         if let Some(var_id) = finally_part.var_id {

crates/nu-engine/src/eval_ir.rs

@@ -170,18 +170,21 @@ impl<'a> EvalContext<'a> {
     }
 
     /// Take and implicitly collect a register to a value
+    ///
+    /// It doesn't check exit status when collecting.
     fn collect_reg(&mut self, reg_id: RegId, fallback_span: Span) -> Result<Value, ShellError> {
-        // NOTE: in collect, it maybe good to pick the inner PipelineData
-        // directly, and drop the ExitStatus queue.
-        let data = self.take_reg(reg_id);
-        let body = data.body;
-        let span = body.span().unwrap_or(fallback_span);
-        let result = body.into_value(span);
+        // NOTE: collect_reg is used to collect the reg to a variable.
+        // So it's good to pick the inner PipelineData directly, and drop the ExitStatus queue.
         #[cfg(feature = "os")]
-        if nu_experimental::PIPE_FAIL.get() {
-            check_exit_status_future(data.exit)?
-        }
-        result
+        let body = {
+            let mut data = self.take_reg(reg_id);
+            data.exit.clear();
+            data.body
+        };
+        #[cfg(not(feature = "os"))]
+        let body = self.take_reg(reg_id).body;
+        let span = body.span().unwrap_or(fallback_span);
+        body.into_value(span)
     }
 
     /// Get a string from data or produce evaluation error if it's invalid UTF-8
@@ -366,6 +369,18 @@ fn eval_instruction<D: DebugContext>(
         }
         Instruction::Collect { src_dst } => {
             let data = ctx.take_reg(*src_dst);
+            #[cfg(feature = "os")]
+            let value = collect(data, *span, true)?;
+            #[cfg(not(feature = "os"))]
+            let value = collect(data, *span)?;
+            ctx.put_reg(*src_dst, PipelineExecutionData::from(value));
+            Ok(Continue)
+        }
+        Instruction::TryCollect { src_dst } => {
+            let data = ctx.take_reg(*src_dst);
+            #[cfg(feature = "os")]
+            let value = collect(data, *span, false)?;
+            #[cfg(not(feature = "os"))]
             let value = collect(data, *span)?;
             ctx.put_reg(*src_dst, PipelineExecutionData::from(value));
             Ok(Continue)
@@ -1598,15 +1613,21 @@ fn get_env_var_name<'a>(ctx: &mut EvalContext<'_>, key: &'a str) -> Cow<'a, str>
 /// Helper to collect values into [`PipelineData`], preserving original span and metadata
 ///
 /// The metadata is removed if it is the file data source, as that's just meant to mark streams.
-fn collect(pipe: PipelineExecutionData, fallback_span: Span) -> Result<PipelineData, ShellError> {
+///
+/// It doesn't check pipefail if `ignore_error` is true.
+fn collect(
+    pipe: PipelineExecutionData,
+    fallback_span: Span,
+    #[cfg(feature = "os")] ignore_error: bool,
+) -> Result<PipelineData, ShellError> {
     let data = pipe.body;
     let span = data.span().unwrap_or(fallback_span);
     let metadata = data.metadata().and_then(|m| m.for_collect());
-    let value = data.into_value(span)?;
     #[cfg(feature = "os")]
-    if nu_experimental::PIPE_FAIL.get() {
-        check_exit_status_future(pipe.exit)?
+    if nu_experimental::PIPE_FAIL.get() && !ignore_error {
+        check_exit_status_future(pipe.exit)?;
     }
+    let value = data.into_value(span)?;
     Ok(PipelineData::value(value, metadata))
 }
 

crates/nu-protocol/src/ir/display.rs

@@ -83,6 +83,9 @@ impl fmt::Display for FmtInstruction<'_> {
             Instruction::Collect { src_dst } => {
                 write!(f, "{:WIDTH$} {src_dst}", "collect")
             }
+            Instruction::TryCollect { src_dst } => {
+                write!(f, "{:WIDTH$} {src_dst}", "try-collect")
+            }
             Instruction::Span { src_dst } => {
                 write!(f, "{:WIDTH$} {src_dst}", "span")
             }

crates/nu-protocol/src/ir/mod.rs

@@ -109,8 +109,14 @@ pub enum Instruction {
     Move { dst: RegId, src: RegId },
     /// Copy a register (must be a collected value). Value is still in `src` after this instruction.
     Clone { dst: RegId, src: RegId },
-    /// Collect a stream in a register to a value
+    /// Collect a stream in a register to a value.
+    /// Because it collects to a value, nushell will ignore the errors in the stream.
+    /// It's important when the stream is from an external command
     Collect { src_dst: RegId },
+    /// Collect a stream in a register to a value.
+    /// But it's different from `Collect` in that if there is an error in the stream, it will be
+    /// returned as an error instead of being ignored.
+    TryCollect { src_dst: RegId },
     /// Change the span of the contents of a register to the span of this instruction.
     Span { src_dst: RegId },
     /// Drop the value/stream in a register, without draining
@@ -292,6 +298,7 @@ impl Instruction {
             Instruction::Move { dst, .. } => Some(dst),
             Instruction::Clone { dst, .. } => Some(dst),
             Instruction::Collect { src_dst } => Some(src_dst),
+            Instruction::TryCollect { src_dst } => Some(src_dst),
             Instruction::Span { src_dst } => Some(src_dst),
             Instruction::Drop { .. } => None,
             Instruction::Drain { .. } => None,