fix redirection, all tests passing

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

@@ -1,5 +1,5 @@
 use nu_protocol::{
-    ast::{Block, Pipeline, PipelineRedirection, RedirectionSource},
+    ast::{Block, Pipeline, PipelineRedirection, RedirectionSource, RedirectionTarget},
     engine::StateWorkingSet,
     ir::{Instruction, IrBlock, RedirectMode},
     CompileError, IntoSpanned, RegId, Span,
@@ -116,13 +116,13 @@ fn compile_pipeline(
             let mut modes = redirect_modes_of_expression(working_set, &next_element.expr, span)?;
 
             // If there's a next element with no inherent redirection we always pipe out *unless*
-            // this is a single redirection to stderr (e>|)
+            // this is a single redirection of stderr to pipe (e>|)
             if modes.out.is_none()
                 && !matches!(
                     element.redirection,
                     Some(PipelineRedirection::Single {
                         source: RedirectionSource::Stderr,
-                        ..
+                        target: RedirectionTarget::Pipe { .. }
                     })
                 )
             {
@@ -139,7 +139,7 @@ fn compile_pipeline(
 
         let spec_redirect_modes = match &element.redirection {
             Some(PipelineRedirection::Single { source, target }) => {
-                let mode = redirection_target_to_mode(working_set, builder, target, false)?;
+                let mode = redirection_target_to_mode(working_set, builder, target)?;
                 match source {
                     RedirectionSource::Stdout => RedirectModes {
                         out: Some(mode),
@@ -156,8 +156,19 @@ fn compile_pipeline(
                 }
             }
             Some(PipelineRedirection::Separate { out, err }) => {
-                let out = redirection_target_to_mode(working_set, builder, out, true)?;
+                // In this case, out and err must not both be Pipe
-                let err = redirection_target_to_mode(working_set, builder, err, true)?;
+                assert!(
+                    !matches!(
+                        (out, err),
+                        (
+                            RedirectionTarget::Pipe { .. },
+                            RedirectionTarget::Pipe { .. }
+                        )
+                    ),
+                    "for Separate redirection, out and err targets must not both be Pipe"
+                );
+                let out = redirection_target_to_mode(working_set, builder, out)?;
+                let err = redirection_target_to_mode(working_set, builder, err)?;
                 RedirectModes {
                     out: Some(out),
                     err: Some(err),

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

@@ -40,7 +40,6 @@ pub(crate) fn redirection_target_to_mode(
     working_set: &StateWorkingSet,
     builder: &mut BlockBuilder,
     target: &RedirectionTarget,
-    separate: bool,
 ) -> Result<Spanned<RedirectMode>, CompileError> {
     Ok(match target {
         RedirectionTarget::File {
@@ -68,12 +67,7 @@ pub(crate) fn redirection_target_to_mode(
             )?;
             RedirectMode::File { file_num }.into_spanned(*redir_span)
         }
-        RedirectionTarget::Pipe { span } => (if separate {
+        RedirectionTarget::Pipe { span } => RedirectMode::Pipe.into_spanned(*span),
-            RedirectMode::Capture
-        } else {
-            RedirectMode::Pipe
-        })
-        .into_spanned(*span),
     })
 }
 
@@ -106,14 +100,24 @@ pub(crate) fn finish_redirection(
             item: RedirectMode::File { file_num },
             span,
         }) => {
-            builder.push(
+            // If out is a file and err is a pipe, we must not consume the expression result -
-                Instruction::WriteFile {
+            // that is actually the err, in that case.
-                    file_num,
+            if !matches!(
-                    src: out_reg,
+                modes.err,
-                }
+                Some(Spanned {
-                .into_spanned(span),
+                    item: RedirectMode::Pipe { .. },
-            )?;
+                    ..
-            builder.load_empty(out_reg)?;
+                })
+            ) {
+                builder.push(
+                    Instruction::WriteFile {
+                        file_num,
+                        src: out_reg,
+                    }
+                    .into_spanned(span),
+                )?;
+                builder.load_empty(out_reg)?;
+            }
             builder.push(Instruction::CloseFile { file_num }.into_spanned(span))?;
         }
         _ => (),

crates/nu-engine/src/eval_ir.rs

@@ -308,19 +308,7 @@ fn eval_instruction<D: DebugContext>(
         }
         Instruction::Drain { src } => {
             let data = ctx.take_reg(*src);
-            if let Some(exit_status) = data.drain()? {
+            drain(ctx, data)
-                ctx.stack.add_env_var(
-                    "LAST_EXIT_CODE".into(),
-                    Value::int(exit_status.code() as i64, *span),
-                );
-                if exit_status.code() == 0 {
-                    Ok(Continue)
-                } else {
-                    Ok(ExitCode(exit_status.code()))
-                }
-            } else {
-                Ok(Continue)
-            }
         }
         Instruction::LoadVariable { dst, var_id } => {
             let value = get_var(ctx, *var_id, *span)?;
@@ -463,11 +451,14 @@ fn eval_instruction<D: DebugContext>(
                     msg: format!("Tried to write to file #{file_num}, but it is not open"),
                     span: Some(*span),
                 })?;
-            let mut stack = ctx
+            let result = {
-                .stack
+                let mut stack = ctx
-                .push_redirection(Some(Redirection::File(file)), None);
+                    .stack
-            src.write_to_out_dests(ctx.engine_state, &mut stack)?;
+                    .push_redirection(Some(Redirection::File(file)), None);
-            Ok(Continue)
+                src.write_to_out_dests(ctx.engine_state, &mut stack)?
+            };
+            // Abort execution if there's an exit code from a failed external
+            drain(ctx, result)
         }
         Instruction::CloseFile { file_num } => {
             if ctx.files[*file_num as usize].take().is_some() {
@@ -1198,6 +1189,25 @@ fn collect(data: PipelineData, fallback_span: Span) -> Result<PipelineData, Shel
     Ok(PipelineData::Value(value, metadata))
 }
 
+/// Helper for drain behavior. Returns `Ok(ExitCode)` on failed external.
+fn drain(ctx: &mut EvalContext<'_>, data: PipelineData) -> Result<InstructionResult, ShellError> {
+    use self::InstructionResult::*;
+    let span = data.span().unwrap_or(Span::unknown());
+    if let Some(exit_status) = data.drain()? {
+        ctx.stack.add_env_var(
+            "LAST_EXIT_CODE".into(),
+            Value::int(exit_status.code() as i64, span),
+        );
+        if exit_status.code() == 0 {
+            Ok(Continue)
+        } else {
+            Ok(ExitCode(exit_status.code()))
+        }
+    } else {
+        Ok(Continue)
+    }
+}
+
 enum RedirectionStream {
     Out,
     Err,

crates/nu-protocol/src/pipeline/pipeline_data.rs

@@ -132,7 +132,8 @@ impl PipelineData {
     /// without consuming input and without writing anything.
     ///
     /// For the other [`OutDest`]s, the given `PipelineData` will be completely consumed
-    /// and `PipelineData::Empty` will be returned.
+    /// and `PipelineData::Empty` will be returned, unless the data is from an external stream,
+    /// in which case an external stream containing only that exit code will be returned.
     pub fn write_to_out_dests(
         self,
         engine_state: &EngineState,
@@ -140,7 +141,11 @@ impl PipelineData {
     ) -> Result<PipelineData, ShellError> {
         match (self, stack.stdout()) {
             (PipelineData::ByteStream(stream, ..), stdout) => {
-                stream.write_to_out_dests(stdout, stack.stderr())?;
+                if let Some(exit_status) = stream.write_to_out_dests(stdout, stack.stderr())? {
+                    return Ok(PipelineData::new_external_stream_with_only_exit_code(
+                        exit_status.code(),
+                    ));
+                }
             }
             (data, OutDest::Pipe | OutDest::Capture) => return Ok(data),
             (PipelineData::Empty, ..) => {}