From e0bf17930b1c4ed607aa204b7e29928e668fb4f1 Mon Sep 17 00:00:00 2001 From: WindSoilder Date: Thu, 15 Dec 2022 02:25:32 +0800 Subject: [PATCH] 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 | 41 ++-------- crates/nu-engine/src/eval.rs | 85 +-------------------- crates/nu-protocol/src/pipeline_data.rs | 85 +++++++++++++++++++++ 3 files changed, 94 insertions(+), 117 deletions(-) diff --git a/crates/nu-command/src/core_commands/try_.rs b/crates/nu-command/src/core_commands/try_.rs index 1d83f6145b..992a2cbf50 100644 --- a/crates/nu-command/src/core_commands/try_.rs +++ b/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::{ - Category, Example, ListStream, PipelineData, Signature, SyntaxShape, Type, Value, -}; +use nu_protocol::{Category, Example, 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 { - stdout: None, - stderr, - 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 = 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 { + Ok(pipeline) => { + let (pipeline, external_failed) = pipeline.is_external_failed(); + if external_failed { 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 { - stdout: None, - stderr, - exit_code: exit_code_stream, - span, - metadata, - trim_end_newline, - }) + Ok(pipeline) } } - Ok(output) => Ok(output), } } diff --git a/crates/nu-engine/src/eval.rs b/crates/nu-engine/src/eval.rs index 085fe9f7f1..34d60cf874 100644 --- a/crates/nu-engine/src/eval.rs +++ b/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, - PipelineData, Range, RawStream, ShellError, Span, Spanned, Unit, Value, VarId, ENV_VARIABLE_ID, + Config, HistoryFileFormat, IntoInterruptiblePipelineData, IntoPipelineData, PipelineData, + 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. -// 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. +// Try to catch and detect if external command runs to failed. fn might_consume_external_result(input: PipelineData) -> (PipelineData, bool) { - let mut runs_to_failed = false; - 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 = 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) - } + input.is_external_failed() } pub fn eval_element_with_input( diff --git a/crates/nu-protocol/src/pipeline_data.rs b/crates/nu-protocol/src/pipeline_data.rs index 76e9b8fcb6..625456f0a9 100644 --- a/crates/nu-protocol/src/pipeline_data.rs +++ b/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 = 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.