Add stderr back when using do -i (#2309)

* Add stderr back when using do -i

* Add stderr back when using do -i

crates/nu-cli/src/commands/autoview.rs

@@ -3,7 +3,7 @@ use crate::commands::WholeStreamCommand;
 use crate::data::value::format_leaf;
 use crate::prelude::*;
 use nu_errors::ShellError;
-use nu_protocol::{hir, hir::Expression, hir::Literal, hir::SpannedExpression};
+use nu_protocol::hir::{self, Expression, ExternalRedirection, Literal, SpannedExpression};
 use nu_protocol::{Primitive, Scope, Signature, UntaggedValue, Value};
 use parking_lot::Mutex;
 use std::sync::atomic::AtomicBool;
@@ -328,7 +328,7 @@ fn create_default_command_args(context: &RunnableContextWithoutInput) -> RawComm
                 positional: None,
                 named: None,
                 span,
-                is_last: true,
+                external_redirection: ExternalRedirection::Stdout,
             },
             name_tag: context.name.clone(),
             scope: Scope::new(),

crates/nu-cli/src/commands/classified/external.rs

@@ -13,7 +13,7 @@ use futures_codec::FramedRead;
 use log::trace;
 
 use nu_errors::ShellError;
-use nu_protocol::hir::ExternalCommand;
+use nu_protocol::hir::{ExternalCommand, ExternalRedirection};
 use nu_protocol::{Primitive, Scope, ShellTypeName, UntaggedValue, Value};
 use nu_source::Tag;
 
@@ -22,7 +22,7 @@ pub(crate) async fn run_external_command(
     context: &mut Context,
     input: InputStream,
     scope: &Scope,
-    is_last: bool,
+    external_redirection: ExternalRedirection,
 ) -> Result<InputStream, ShellError> {
     trace!(target: "nu::run::external", "-> {}", command.name);
 
@@ -34,7 +34,7 @@ pub(crate) async fn run_external_command(
         ));
     }
 
-    run_with_stdin(command, context, input, scope, is_last).await
+    run_with_stdin(command, context, input, scope, external_redirection).await
 }
 
 async fn run_with_stdin(
@@ -42,7 +42,7 @@ async fn run_with_stdin(
     context: &mut Context,
     input: InputStream,
     scope: &Scope,
-    is_last: bool,
+    external_redirection: ExternalRedirection,
 ) -> Result<InputStream, ShellError> {
     let path = context.shell_manager.path();
 
@@ -122,7 +122,14 @@ async fn run_with_stdin(
         })
         .collect::<Vec<String>>();
 
-    spawn(&command, &path, &process_args[..], input, is_last, scope)
+    spawn(
+        &command,
+        &path,
+        &process_args[..],
+        input,
+        external_redirection,
+        scope,
+    )
 }
 
 fn spawn(
@@ -130,7 +137,7 @@ fn spawn(
     path: &str,
     args: &[String],
     input: InputStream,
-    is_last: bool,
+    external_redirection: ExternalRedirection,
     scope: &Scope,
 ) -> Result<InputStream, ShellError> {
     let command = command.clone();
@@ -166,10 +173,23 @@ fn spawn(
 
     // We want stdout regardless of what
     // we are doing ($it case or pipe stdin)
-    if !is_last {
+    match external_redirection {
+        ExternalRedirection::Stdout => {
             process.stdout(Stdio::piped());
             trace!(target: "nu::run::external", "set up stdout pipe");
         }
+        ExternalRedirection::Stderr => {
+            process.stderr(Stdio::piped());
+            trace!(target: "nu::run::external", "set up stderr pipe");
+        }
+        ExternalRedirection::StdoutAndStderr => {
+            process.stdout(Stdio::piped());
+            trace!(target: "nu::run::external", "set up stdout pipe");
+            process.stderr(Stdio::piped());
+            trace!(target: "nu::run::external", "set up stderr pipe");
+        }
+        _ => {}
+    }
 
     // open since we have some contents for stdin
     if !input.is_empty() {
@@ -252,7 +272,9 @@ fn spawn(
         });
 
         std::thread::spawn(move || {
-            if !is_last {
+            if external_redirection == ExternalRedirection::Stdout
+                || external_redirection == ExternalRedirection::StdoutAndStderr
+            {
                 let stdout = if let Some(stdout) = child.stdout.take() {
                     stdout
                 } else {
@@ -321,6 +343,79 @@ fn spawn(
                     }
                 }
             }
+            if external_redirection == ExternalRedirection::Stderr
+                || external_redirection == ExternalRedirection::StdoutAndStderr
+            {
+                let stderr = if let Some(stderr) = child.stderr.take() {
+                    stderr
+                } else {
+                    let _ = stdout_read_tx.send(Ok(Value {
+                        value: UntaggedValue::Error(ShellError::labeled_error(
+                            "Can't redirect the stderr for external command",
+                            "can't redirect stderr",
+                            &stdout_name_tag,
+                        )),
+                        tag: stdout_name_tag,
+                    }));
+                    return Err(());
+                };
+
+                let file = futures::io::AllowStdIo::new(stderr);
+                let stream = FramedRead::new(file, MaybeTextCodec::default());
+
+                for line in block_on_stream(stream) {
+                    match line {
+                        Ok(line) => match line {
+                            StringOrBinary::String(s) => {
+                                let result = stdout_read_tx.send(Ok(Value {
+                                    value: UntaggedValue::Error(
+                                        ShellError::untagged_runtime_error(s),
+                                    ),
+                                    tag: stdout_name_tag.clone(),
+                                }));
+
+                                if result.is_err() {
+                                    break;
+                                }
+                            }
+                            StringOrBinary::Binary(_) => {
+                                let result = stdout_read_tx.send(Ok(Value {
+                                    value: UntaggedValue::Error(
+                                        ShellError::untagged_runtime_error("<binary stderr>"),
+                                    ),
+                                    tag: stdout_name_tag.clone(),
+                                }));
+
+                                if result.is_err() {
+                                    break;
+                                }
+                            }
+                        },
+                        Err(e) => {
+                            // If there's an exit status, it makes sense that we may error when
+                            // trying to read from its stdout pipe (likely been closed). In that
+                            // case, don't emit an error.
+                            let should_error = match child.wait() {
+                                Ok(exit_status) => !exit_status.success(),
+                                Err(_) => true,
+                            };
+
+                            if should_error {
+                                let _ = stdout_read_tx.send(Ok(Value {
+                                    value: UntaggedValue::Error(ShellError::labeled_error(
+                                        format!("Unable to read from stdout ({})", e),
+                                        "unable to read from stdout",
+                                        &stdout_name_tag,
+                                    )),
+                                    tag: stdout_name_tag.clone(),
+                                }));
+                            }
+
+                            return Ok(());
+                        }
+                    }
+                }
+            }
 
             // We can give an error when we see a non-zero exit code, but this is different
             // than what other shells will do.
@@ -474,16 +569,21 @@ mod tests {
 
     #[cfg(feature = "which")]
     async fn non_existent_run() -> Result<(), ShellError> {
+        use nu_protocol::hir::ExternalRedirection;
         let cmd = ExternalBuilder::for_name("i_dont_exist.exe").build();
 
         let input = InputStream::empty();
         let mut ctx = Context::basic().expect("There was a problem creating a basic context.");
 
-        assert!(
+        assert!(run_external_command(
-            run_external_command(cmd, &mut ctx, input, &Scope::new(), false)
+            cmd,
+            &mut ctx,
+            input,
+            &Scope::new(),
+            ExternalRedirection::Stdout
+        )
         .await
-                .is_err()
+        .is_err());
-        );
 
         Ok(())
     }

crates/nu-cli/src/commands/classified/internal.rs

@@ -4,7 +4,7 @@ use crate::commands::UnevaluatedCallInfo;
 use crate::prelude::*;
 use log::{log_enabled, trace};
 use nu_errors::ShellError;
-use nu_protocol::hir::InternalCommand;
+use nu_protocol::hir::{ExternalRedirection, InternalCommand};
 use nu_protocol::{CommandAction, Primitive, ReturnSuccess, Scope, UntaggedValue, Value};
 
 pub(crate) async fn run_internal_command(
@@ -87,7 +87,7 @@ pub(crate) async fn run_internal_command(
                                                 positional: None,
                                                 named: None,
                                                 span: Span::unknown(),
-                                                is_last: false,
+                                                external_redirection: ExternalRedirection::Stdout,
                                             },
                                             name_tag: Tag::unknown_anchor(command.name_span),
                                             scope: (&*scope).clone(),

crates/nu-cli/src/commands/do_.rs

@@ -2,7 +2,9 @@ use crate::commands::classified::block::run_block;
 use crate::commands::WholeStreamCommand;
 use crate::prelude::*;
 use nu_errors::ShellError;
-use nu_protocol::{hir::Block, ReturnSuccess, Signature, SyntaxShape, Value};
+use nu_protocol::{
+    hir::Block, hir::ExternalRedirection, ReturnSuccess, Signature, SyntaxShape, Value,
+};
 
 pub struct Do;
 
@@ -61,7 +63,7 @@ async fn do_(
     registry: &CommandRegistry,
 ) -> Result<OutputStream, ShellError> {
     let registry = registry.clone();
-    let is_last = raw_args.call_info.args.is_last;
+    let external_redirection = raw_args.call_info.args.external_redirection;
 
     let mut context = Context::from_raw(&raw_args, &registry);
     let scope = raw_args.call_info.scope.clone();
@@ -72,7 +74,26 @@ async fn do_(
         },
         input,
     ) = raw_args.process(&registry).await?;
-    block.set_is_last(is_last);
+
+    let block_redirection = match external_redirection {
+        ExternalRedirection::None => {
+            if ignore_errors {
+                ExternalRedirection::Stderr
+            } else {
+                ExternalRedirection::None
+            }
+        }
+        ExternalRedirection::Stdout => {
+            if ignore_errors {
+                ExternalRedirection::StdoutAndStderr
+            } else {
+                ExternalRedirection::Stdout
+            }
+        }
+        x => x,
+    };
+
+    block.set_redirect(block_redirection);
 
     let result = run_block(
         &block,
@@ -85,6 +106,9 @@ async fn do_(
     .await;
 
     if ignore_errors {
+        // To properly ignore errors we need to redirect stderr, consume it, and remove
+        // any errors we see in the process.
+
         match result {
             Ok(mut stream) => {
                 let output = stream.drain_vec().await;

crates/nu-cli/src/commands/enter.rs

@@ -3,6 +3,7 @@ use crate::commands::WholeStreamCommand;
 use crate::context::CommandRegistry;
 use crate::prelude::*;
 use nu_errors::ShellError;
+use nu_protocol::hir::ExternalRedirection;
 use nu_protocol::{
     CommandAction, Primitive, ReturnSuccess, Signature, SyntaxShape, UntaggedValue, Value,
 };
@@ -145,7 +146,7 @@ async fn enter(
                                     positional: None,
                                     named: None,
                                     span: Span::unknown(),
-                                    is_last: false,
+                                    external_redirection: ExternalRedirection::Stdout,
                                 },
                                 name_tag: tag.clone(),
                                 scope: scope.clone(),

crates/nu-cli/src/commands/run_alias.rs

@@ -41,7 +41,7 @@ impl WholeStreamCommand for AliasCommand {
         let call_info = args.call_info.clone();
         let registry = registry.clone();
         let mut block = self.block.clone();
-        block.set_is_last(call_info.args.is_last);
+        block.set_redirect(call_info.args.external_redirection);
 
         let alias_command = self.clone();
         let mut context = Context::from_args(&args, &registry);

crates/nu-cli/src/commands/run_external.rs

@@ -62,6 +62,8 @@ impl WholeStreamCommand for RunExternalCommand {
 
         let mut positionals = positionals.into_iter();
 
+        let external_redirection = args.call_info.args.external_redirection;
+
         let name = positionals
             .next()
             .ok_or_else(|| {
@@ -130,10 +132,15 @@ impl WholeStreamCommand for RunExternalCommand {
         }
 
         let scope = args.call_info.scope.clone();
-        let is_last = args.call_info.args.is_last;
+
         let input = args.input;
-        let result =
+        let result = external::run_external_command(
-            external::run_external_command(command, &mut external_context, input, &scope, is_last)
+            command,
+            &mut external_context,
+            input,
+            &scope,
+            external_redirection,
+        )
         .await;
 
         Ok(result?.to_output_stream())

crates/nu-cli/src/commands/save.rs

@@ -1,7 +1,10 @@
 use crate::commands::{UnevaluatedCallInfo, WholeStreamCommand};
 use crate::prelude::*;
 use nu_errors::ShellError;
-use nu_protocol::{Primitive, ReturnSuccess, Signature, SyntaxShape, UntaggedValue, Value};
+use nu_protocol::{
+    hir::ExternalRedirection, Primitive, ReturnSuccess, Signature, SyntaxShape, UntaggedValue,
+    Value,
+};
 use nu_source::Tagged;
 use std::path::{Path, PathBuf};
 
@@ -226,7 +229,7 @@ async fn save(
                                 positional: None,
                                 named: None,
                                 span: Span::unknown(),
-                                is_last: false,
+                                external_redirection: ExternalRedirection::Stdout,
                             },
                             name_tag: name_tag.clone(),
                             scope,

crates/nu-cli/src/evaluate/evaluator.rs

@@ -5,7 +5,7 @@ use crate::prelude::*;
 use async_recursion::async_recursion;
 use log::trace;
 use nu_errors::{ArgumentError, ShellError};
-use nu_protocol::hir::{self, Expression, SpannedExpression};
+use nu_protocol::hir::{self, Expression, ExternalRedirection, SpannedExpression};
 use nu_protocol::{
     ColumnPath, Primitive, RangeInclusion, UnspannedPathMember, UntaggedValue, Value,
 };
@@ -194,10 +194,10 @@ async fn evaluate_invocation(
     let mut context = Context::basic()?;
     context.registry = registry.clone();
 
-    let input = InputStream::empty();
+    let input = InputStream::one(it.clone());
 
     let mut block = block.clone();
-    block.set_is_last(false);
+    block.set_redirect(ExternalRedirection::Stdout);
 
     let result = run_block(&block, &mut context, input, it, vars, env).await?;
 

crates/nu-parser/src/parse.rs

@@ -4,8 +4,8 @@ use log::trace;
 use nu_errors::{ArgumentError, ParseError};
 use nu_protocol::hir::{
     self, Binary, Block, ClassifiedBlock, ClassifiedCommand, ClassifiedPipeline, Commands,
-    Expression, Flag, FlagKind, InternalCommand, Member, NamedArguments, Operator,
+    Expression, ExternalRedirection, Flag, FlagKind, InternalCommand, Member, NamedArguments,
-    SpannedExpression, Unit,
+    Operator, SpannedExpression, Unit,
 };
 use nu_protocol::{NamedType, PositionalType, Signature, SyntaxShape, UnspannedPathMember};
 use nu_source::{Span, Spanned, SpannedItem};
@@ -498,7 +498,7 @@ fn parse_interpolated_string(
                     expr: Expression::Synthetic(hir::Synthetic::String("build-string".to_owned())),
                     span: lite_arg.span,
                 }),
-                is_last: false,
+                external_redirection: ExternalRedirection::Stdout,
                 named: None,
                 positional: Some(output),
                 span: lite_arg.span,
@@ -1359,7 +1359,11 @@ fn classify_pipeline(
                     positional: Some(args),
                     named: None,
                     span: Span::unknown(),
-                    is_last: iter.peek().is_none(),
+                    external_redirection: if iter.peek().is_none() {
+                        ExternalRedirection::None
+                    } else {
+                        ExternalRedirection::Stdout
+                    },
                 },
             }))
         } else if lite_cmd.name.item == "=" {
@@ -1387,7 +1391,11 @@ fn classify_pipeline(
                         parse_internal_command(&lite_cmd, registry, &signature, 1);
 
                     error = error.or(err);
-                    internal_command.args.is_last = iter.peek().is_none();
+                    internal_command.args.external_redirection = if iter.peek().is_none() {
+                        ExternalRedirection::None
+                    } else {
+                        ExternalRedirection::Stdout
+                    };
                     commands.push(ClassifiedCommand::Internal(internal_command));
                     continue;
                 }
@@ -1399,7 +1407,11 @@ fn classify_pipeline(
                     parse_internal_command(&lite_cmd, registry, &signature, 0);
 
                 error = error.or(err);
-                internal_command.args.is_last = iter.peek().is_none();
+                internal_command.args.external_redirection = if iter.peek().is_none() {
+                    ExternalRedirection::None
+                } else {
+                    ExternalRedirection::Stdout
+                };
                 commands.push(ClassifiedCommand::Internal(internal_command));
                 continue;
             }
@@ -1437,7 +1449,11 @@ fn classify_pipeline(
                     positional: Some(args),
                     named: None,
                     span: Span::unknown(),
-                    is_last: iter.peek().is_none(),
+                    external_redirection: if iter.peek().is_none() {
+                        ExternalRedirection::None
+                    } else {
+                        ExternalRedirection::Stdout
+                    },
                 },
             }))
         }

crates/nu-protocol/src/hir.rs

@@ -167,7 +167,7 @@ impl Commands {
                             }),
                             span: self.span,
                         }]),
-                        is_last: false, // FIXME
+                        external_redirection: ExternalRedirection::Stdout, // FIXME
                     },
                 })
             }
@@ -200,27 +200,15 @@ impl Block {
         }
     }
 
-    pub fn set_is_last(&mut self, is_last: bool) {
+    pub fn set_redirect(&mut self, external_redirection: ExternalRedirection) {
         if let Some(pipeline) = self.block.last_mut() {
             if let Some(command) = pipeline.list.last_mut() {
                 if let ClassifiedCommand::Internal(internal) = command {
-                    internal.args.is_last = is_last;
+                    internal.args.external_redirection = external_redirection;
                 }
             }
         }
     }
-
-    pub fn get_is_last(&mut self) -> Option<bool> {
-        if let Some(pipeline) = self.block.last_mut() {
-            if let Some(command) = pipeline.list.last_mut() {
-                if let ClassifiedCommand::Internal(internal) = command {
-                    return Some(internal.args.is_last);
-                }
-            }
-        }
-
-        None
-    }
 }
 
 #[derive(Debug, Ord, PartialOrd, Eq, PartialEq, Clone, Hash, Deserialize, Serialize)]
@@ -1114,13 +1102,21 @@ impl PrettyDebugWithSource for NamedValue {
     }
 }
 
+#[derive(Debug, Clone, Copy, PartialEq, PartialOrd, Eq, Ord, Hash, Serialize, Deserialize)]
+pub enum ExternalRedirection {
+    None,
+    Stdout,
+    Stderr,
+    StdoutAndStderr,
+}
+
 #[derive(Debug, Clone, PartialEq, PartialOrd, Eq, Ord, Hash, Serialize, Deserialize)]
 pub struct Call {
     pub head: Box<SpannedExpression>,
     pub positional: Option<Vec<SpannedExpression>>,
     pub named: Option<NamedArguments>,
     pub span: Span,
-    pub is_last: bool,
+    pub external_redirection: ExternalRedirection,
 }
 
 impl Call {
@@ -1193,7 +1189,7 @@ impl Call {
             positional: None,
             named: None,
             span,
-            is_last: false,
+            external_redirection: ExternalRedirection::Stdout,
         }
     }
 }