refactor: introduce is_external_failed to PipelineData, and simplify try logic (#7476)

# Description

Just spot that there are some duplicate code about checking external
runs to failed, is pr is trying to refactor it and reduce lines of code

# User-Facing Changes

NaN

# Tests + Formatting

Don't forget to add tests that cover your changes.

Make sure you've run and fixed any issues with these commands:

- `cargo fmt --all -- --check` to check standard code formatting (`cargo
fmt --all` applies these changes)
- `cargo clippy --workspace -- -D warnings -D clippy::unwrap_used -A
clippy::needless_collect` to check that you're using the standard code
style
- `cargo test --workspace` to check that all tests pass

# After Submitting

If your PR had any user-facing changes, update [the
documentation](https://github.com/nushell/nushell.github.io) after the
PR is merged, if necessary. This will help us keep the docs up to date.

crates/nu-command/src/core_commands/try_.rs

@@ -1,9 +1,7 @@
 use nu_engine::{eval_block, CallExt};
 use nu_protocol::ast::Call;
 use nu_protocol::engine::{Block, Closure, Command, EngineState, Stack};
-use nu_protocol::{
+use nu_protocol::{Category, Example, PipelineData, Signature, SyntaxShape, Type, Value};
-    Category, Example, ListStream, PipelineData, Signature, SyntaxShape, Type, Value,
-};
 
 #[derive(Clone)]
 pub struct Try;
@@ -80,30 +78,9 @@ impl Command for Try {
                 }
             }
             // external command may fail to run
-            Ok(PipelineData::ExternalStream {
+            Ok(pipeline) => {
-                stdout: None,
+                let (pipeline, external_failed) = pipeline.is_external_failed();
-                stderr,
+                if external_failed {
-                mut exit_code,
-                span,
-                metadata,
-                trim_end_newline,
-            }) => {
-                let exit_code = exit_code.take();
-                let mut failed_to_run = false;
-                let mut exit_code_stream = None;
-                if let Some(stream) = exit_code {
-                    let ctrlc = stream.ctrlc.clone();
-                    let exit_code: Vec<Value> = stream.into_iter().collect();
-                    if let Some(Value::Int { val: code, .. }) = exit_code.last() {
-                        // if exit_code is not 0, it indicates error occured, return back Err.
-                        if *code != 0 {
-                            failed_to_run = true;
-                        }
-                    }
-                    exit_code_stream = Some(ListStream::from_stream(exit_code.into_iter(), ctrlc));
-                }
-
-                if failed_to_run {
                     if let Some(catch_block) = catch_block {
                         let catch_block = engine_state.get_block(catch_block.block_id);
 
@@ -126,17 +103,9 @@ impl Command for Try {
                         Ok(PipelineData::empty())
                     }
                 } else {
-                    Ok(PipelineData::ExternalStream {
+                    Ok(pipeline)
-                        stdout: None,
-                        stderr,
-                        exit_code: exit_code_stream,
-                        span,
-                        metadata,
-                        trim_end_newline,
-                    })
                 }
             }
-            Ok(output) => Ok(output),
         }
     }
 

crates/nu-engine/src/eval.rs

@@ -6,8 +6,8 @@ use nu_protocol::{
         Operator, PathMember, PipelineElement, Redirection,
     },
     engine::{EngineState, Stack},
-    Config, HistoryFileFormat, IntoInterruptiblePipelineData, IntoPipelineData, ListStream,
+    Config, HistoryFileFormat, IntoInterruptiblePipelineData, IntoPipelineData, PipelineData,
-    PipelineData, Range, RawStream, ShellError, Span, Spanned, Unit, Value, VarId, ENV_VARIABLE_ID,
+    Range, ShellError, Span, Spanned, Unit, Value, VarId, ENV_VARIABLE_ID,
 };
 use nu_utils::stdout_write_all_and_flush;
 use std::collections::HashMap;
@@ -698,86 +698,9 @@ pub fn eval_expression_with_input(
     Ok(might_consume_external_result(input))
 }
 
-// if the result is ExternalStream without redirecting output.
+// Try to catch and detect if external command runs to failed.
-// that indicates we have no more commands to execute currently.
-// we can try to catch and detect if external command runs to failed.
-//
-// This is useful to commands with semicolon, we can detect errors early to avoid
-// commands after semicolon running.
 fn might_consume_external_result(input: PipelineData) -> (PipelineData, bool) {
-    let mut runs_to_failed = false;
+    input.is_external_failed()
-    if let PipelineData::ExternalStream {
-        stdout: None,
-        stderr,
-        mut exit_code,
-        span,
-        metadata,
-        trim_end_newline,
-    } = input
-    {
-        let exit_code = exit_code.take();
-
-        // 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 = stderr.map(|stderr_stream| {
-            let stderr_ctrlc = stderr_stream.ctrlc.clone();
-            let stderr_span = stderr_stream.span;
-            let stderr_bytes = match stderr_stream.into_bytes() {
-                Err(_) => vec![],
-                Ok(bytes) => bytes.item,
-            };
-            RawStream::new(
-                Box::new(vec![Ok(stderr_bytes)].into_iter()),
-                stderr_ctrlc,
-                stderr_span,
-            )
-        });
-
-        match exit_code {
-            Some(exit_code_stream) => {
-                let ctrlc = exit_code_stream.ctrlc.clone();
-                let exit_code: Vec<Value> = exit_code_stream.into_iter().collect();
-                if let Some(Value::Int { val: code, .. }) = exit_code.last() {
-                    // if exit_code is not 0, it indicates error occured, return back Err.
-                    if *code != 0 {
-                        runs_to_failed = true;
-                    }
-                }
-                (
-                    PipelineData::ExternalStream {
-                        stdout: None,
-                        stderr,
-                        exit_code: Some(ListStream::from_stream(exit_code.into_iter(), ctrlc)),
-                        span,
-                        metadata,
-                        trim_end_newline,
-                    },
-                    runs_to_failed,
-                )
-            }
-            None => (
-                PipelineData::ExternalStream {
-                    stdout: None,
-                    stderr,
-                    exit_code: None,
-                    span,
-                    metadata,
-                    trim_end_newline,
-                },
-                runs_to_failed,
-            ),
-        }
-    } else {
-        (input, false)
-    }
 }
 
 pub fn eval_element_with_input(

crates/nu-protocol/src/pipeline_data.rs

@@ -474,6 +474,91 @@ impl PipelineData {
         }
     }
 
+    /// Try to catch external stream exit status and detect if it runs to failed.
+    ///
+    /// This is useful to commands with semicolon, we can detect errors early to avoid
+    /// commands after semicolon running.
+    ///
+    /// Returns self and a flag indicates if the external stream runs to failed.
+    /// If `self` is not Pipeline::ExternalStream, the flag will be false.
+    pub fn is_external_failed(self) -> (Self, bool) {
+        let mut failed_to_run = false;
+        // Only need ExternalStream without redirecting output.
+        // It indicates we have no more commands to execute currently.
+        if let PipelineData::ExternalStream {
+            stdout: None,
+            stderr,
+            mut exit_code,
+            span,
+            metadata,
+            trim_end_newline,
+        } = self
+        {
+            let exit_code = exit_code.take();
+
+            // 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 = stderr.map(|stderr_stream| {
+                let stderr_ctrlc = stderr_stream.ctrlc.clone();
+                let stderr_span = stderr_stream.span;
+                let stderr_bytes = match stderr_stream.into_bytes() {
+                    Err(_) => vec![],
+                    Ok(bytes) => bytes.item,
+                };
+                RawStream::new(
+                    Box::new(vec![Ok(stderr_bytes)].into_iter()),
+                    stderr_ctrlc,
+                    stderr_span,
+                )
+            });
+
+            match exit_code {
+                Some(exit_code_stream) => {
+                    let ctrlc = exit_code_stream.ctrlc.clone();
+                    let exit_code: Vec<Value> = exit_code_stream.into_iter().collect();
+                    if let Some(Value::Int { val: code, .. }) = exit_code.last() {
+                        // if exit_code is not 0, it indicates error occured, return back Err.
+                        if *code != 0 {
+                            failed_to_run = true;
+                        }
+                    }
+                    (
+                        PipelineData::ExternalStream {
+                            stdout: None,
+                            stderr,
+                            exit_code: Some(ListStream::from_stream(exit_code.into_iter(), ctrlc)),
+                            span,
+                            metadata,
+                            trim_end_newline,
+                        },
+                        failed_to_run,
+                    )
+                }
+                None => (
+                    PipelineData::ExternalStream {
+                        stdout: None,
+                        stderr,
+                        exit_code: None,
+                        span,
+                        metadata,
+                        trim_end_newline,
+                    },
+                    failed_to_run,
+                ),
+            }
+        } else {
+            (self, false)
+        }
+    }
+
     /// Consume and print self data immediately.
     ///
     /// `no_newline` controls if we need to attach newline character to output.