Mirrors/nushell
2
0
Fork 0
mirror of https://github.com/nushell/nushell synced 2024-12-26 04:53:09 +00:00
Code Issues Projects Releases Packages Wiki Activity

Fix try printing when it is not the last pipeline element (#13992)

Browse source 
# Description

Fixes #13991. This was done by more clearly separating the case when a
pipeline is drained vs when it is being written (to a file).

I also added an `OutDest::Print` case which might not be strictly
necessary, but is a helpful addition.

# User-Facing Changes

Bug fix.

# Tests + Formatting

Added a test.

# After Submitting

There are still a few redirection bugs that I found, but they require
larger code changes, so I'll leave them until after the release.
This commit is contained in:
Ian Manske 2024-10-11 23:37:10 -07:00 committed by GitHub
parent 0e3a8c552c
commit de08b68ba8
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
14 changed files with 127 additions and 194 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

2
crates/nu-cmd-lang/src/core_commands/try_.rs
View file

@ -63,7 +63,7 @@ impl Command for Try {
let eval_block = get_eval_block(engine_state);
let result = eval_block(engine_state, stack, try_block, input)
.and_then(|pipeline| pipeline.write_to_out_dests(engine_state, stack));
.and_then(|pipeline| pipeline.drain_to_out_dests(engine_state, stack));
match result {
Err(err) => run_catch(err, head, catch_block, engine_state, stack, eval_block),

10
crates/nu-command/src/filters/tee.rs
View file

@ -163,7 +163,7 @@ use it in your pipeline."#
Ok(None)
}
OutDest::Null => copy_on_thread(tee, io::sink(), &info).map(Some),
OutDest::Inherit => {
OutDest::Print | OutDest::Inherit => {
copy_on_thread(tee, io::stderr(), &info).map(Some)
}
OutDest::File(file) => {
@ -181,7 +181,9 @@ use it in your pipeline."#
Ok(())
}
OutDest::Null => copy_pipe(stdout, io::sink(), &info),
OutDest::Inherit => copy_pipe(stdout, io::stdout(), &info),
OutDest::Print | OutDest::Inherit => {
copy_pipe(stdout, io::stdout(), &info)
}
OutDest::File(file) => copy_pipe(stdout, file.as_ref(), &info),
}?;
}
@ -198,7 +200,7 @@ use it in your pipeline."#
OutDest::Null => {
copy_pipe_on_thread(stderr, io::sink(), &info).map(Some)
}
OutDest::Inherit => {
OutDest::Print | OutDest::Inherit => {
copy_pipe_on_thread(stderr, io::stderr(), &info).map(Some)
}
OutDest::File(file) => {
@ -218,7 +220,7 @@ use it in your pipeline."#
Ok(())
}
OutDest::Null => copy(tee, io::sink(), &info),
OutDest::Inherit => copy(tee, io::stdout(), &info),
OutDest::Print | OutDest::Inherit => copy(tee, io::stdout(), &info),
OutDest::File(file) => copy(tee, file.as_ref(), &info),
}?;
}

13
crates/nu-command/tests/commands/try_.rs
View file

@ -107,13 +107,22 @@ fn exit_code_available_in_catch() {
}
#[test]
fn try_catches_exit_code_in_assignment() {
fn catches_exit_code_in_assignment() {
let actual = nu!("let x = try { nu -c 'exit 42' } catch { |e| $e.exit_code }; $x");
assert_eq!(actual.out, "42");
}
#[test]
fn try_catches_exit_code_in_expr() {
fn catches_exit_code_in_expr() {
let actual = nu!("print (try { nu -c 'exit 42' } catch { |e| $e.exit_code })");
assert_eq!(actual.out, "42");
}
#[test]
fn prints_only_if_last_pipeline() {
let actual = nu!("try { 'should not print' }; 'last value'");
assert_eq!(actual.out, "last value");
let actual = nu!("try { ['should not print'] | every 1 }; 'last value'");
assert_eq!(actual.out, "last value");
}

2
crates/nu-engine/src/compile/builder.rs
View file

@ -205,7 +205,7 @@ impl BlockBuilder {
Instruction::Span { src_dst } => allocate(&[*src_dst], &[*src_dst]),
Instruction::Drop { src } => allocate(&[*src], &[]),
Instruction::Drain { src } => allocate(&[*src], &[]),
Instruction::WriteToOutDests { src } => allocate(&[*src], &[]),
Instruction::DrainIfEnd { src } => allocate(&[*src], &[]),
Instruction::LoadVariable { dst, var_id: _ } => allocate(&[], &[*dst]),
Instruction::StoreVariable { var_id: _, src } => allocate(&[*src], &[]),
Instruction::DropVariable { var_id: _ } => Ok(()),

2
crates/nu-engine/src/compile/keyword.rs
View file

@ -473,7 +473,7 @@ pub(crate) fn compile_try(
if let Some(mode) = redirect_modes.err {
builder.push(mode.map(|mode| Instruction::RedirectErr { mode }))?;
}
builder.push(Instruction::WriteToOutDests { src: io_reg }.into_spanned(call.head))?;
builder.push(Instruction::DrainIfEnd { src: io_reg }.into_spanned(call.head))?;
builder.push(Instruction::PopErrorHandler.into_spanned(call.head))?;
// Jump over the failure case

3
crates/nu-engine/src/compile/redirect.rs
View file

@ -151,7 +151,8 @@ pub(crate) fn out_dest_to_redirect_mode(
OutDest::PipeSeparate => Ok(RedirectMode::PipeSeparate),
OutDest::Value => Ok(RedirectMode::Value),
OutDest::Null => Ok(RedirectMode::Null),
OutDest::Inherit => Ok(RedirectMode::Inherit),
OutDest::Print => Ok(RedirectMode::Print),
OutDest::Inherit => Err(CompileError::InvalidRedirectMode { span }),
OutDest::File(_) => Err(CompileError::InvalidRedirectMode { span }),
})
.transpose()

26
crates/nu-engine/src/eval.rs
View file

@ -395,13 +395,13 @@ fn eval_element_with_input_inner<D: DebugContext>(
) -> Result<PipelineData, ShellError> {
let data = eval_expression_with_input::<D>(engine_state, stack, &element.expr, input)?;
if let Some(redirection) = element.redirection.as_ref() {
let is_external = if let PipelineData::ByteStream(stream, ..) = &data {
matches!(stream.source(), ByteStreamSource::Child(..))
} else {
false
};
if let Some(redirection) = element.redirection.as_ref() {
if !is_external {
match redirection {
&PipelineRedirection::Single {
@ -437,30 +437,24 @@ fn eval_element_with_input_inner<D: DebugContext>(
}
}
let has_stdout_file = matches!(stack.pipe_stdout(), Some(OutDest::File(_)));
let data = match &data {
let data = if let Some(OutDest::File(file)) = stack.pipe_stdout() {
match &data {
PipelineData::Value(..) | PipelineData::ListStream(..) => {
if has_stdout_file {
data.write_to_out_dests(engine_state, stack)?;
data.write_to(file.as_ref())?;
PipelineData::Empty
} else {
data
}
}
PipelineData::ByteStream(stream, ..) => {
let write = match stream.source() {
ByteStreamSource::Read(_) | ByteStreamSource::File(_) => has_stdout_file,
ByteStreamSource::Child(_) => false,
};
if write {
data.write_to_out_dests(engine_state, stack)?;
PipelineData::ByteStream(..) => {
if !is_external {
data.write_to(file.as_ref())?;
PipelineData::Empty
} else {
data
}
}
PipelineData::Empty => PipelineData::Empty,
}
} else {
data
};
Ok(data)

24
crates/nu-engine/src/eval_ir.rs
View file

@ -322,13 +322,13 @@ fn eval_instruction<D: DebugContext>(
let data = ctx.take_reg(*src);
drain(ctx, data)
}
Instruction::WriteToOutDests { src } => {
Instruction::DrainIfEnd { src } => {
let data = ctx.take_reg(*src);
let res = {
let stack = &mut ctx
.stack
.push_redirection(ctx.redirect_out.clone(), ctx.redirect_err.clone());
data.write_to_out_dests(ctx.engine_state, stack)?
data.drain_to_out_dests(ctx.engine_state, stack)?
};
ctx.put_reg(*src, res);
Ok(Continue)
@ -507,14 +507,19 @@ fn eval_instruction<D: DebugContext>(
msg: format!("Tried to write to file #{file_num}, but it is not open"),
span: Some(*span),
})?;
let result = {
let mut stack = ctx
.stack
.push_redirection(Some(Redirection::File(file)), None);
src.write_to_out_dests(ctx.engine_state, &mut stack)?
let is_external = if let PipelineData::ByteStream(stream, ..) = &src {
matches!(stream.source(), ByteStreamSource::Child(..))
} else {
false
};
// Abort execution if there's an exit code from a failed external
drain(ctx, result)
if let Err(err) = src.write_to(file.as_ref()) {
if is_external {
ctx.stack.set_last_error(&err);
}
Err(err)?
} else {
Ok(Continue)
}
}
Instruction::CloseFile { file_num } => {
if ctx.files[*file_num as usize].take().is_some() {
@ -1421,6 +1426,7 @@ fn eval_redirection(
RedirectMode::Value => Ok(Some(Redirection::Pipe(OutDest::Value))),
RedirectMode::Null => Ok(Some(Redirection::Pipe(OutDest::Null))),
RedirectMode::Inherit => Ok(Some(Redirection::Pipe(OutDest::Inherit))),
RedirectMode::Print => Ok(Some(Redirection::Pipe(OutDest::Print))),
RedirectMode::File { file_num } => {
let file = ctx
.files

4
crates/nu-protocol/src/engine/stack_out_dest.rs
View file

@ -62,8 +62,8 @@ pub(crate) struct StackOutDest {
impl StackOutDest {
pub(crate) fn new() -> Self {
Self {
pipe_stdout: None,
pipe_stderr: None,
pipe_stdout: Some(OutDest::Print),
pipe_stderr: Some(OutDest::Print),
stdout: OutDest::Inherit,
stderr: OutDest::Inherit,
parent_stdout: None,

5
crates/nu-protocol/src/ir/display.rs
View file

@ -92,8 +92,8 @@ impl<'a> fmt::Display for FmtInstruction<'a> {
Instruction::Drain { src } => {
write!(f, "{:WIDTH$} {src}", "drain")
}
Instruction::WriteToOutDests { src } => {
write!(f, "{:WIDTH$} {src}", "write-to-out-dests")
Instruction::DrainIfEnd { src } => {
write!(f, "{:WIDTH$} {src}", "drain-if-end")
}
Instruction::LoadVariable { dst, var_id } => {
let var = FmtVar::new(self.engine_state, *var_id);
@ -312,6 +312,7 @@ impl fmt::Display for RedirectMode {
RedirectMode::Value => write!(f, "value"),
RedirectMode::Null => write!(f, "null"),
RedirectMode::Inherit => write!(f, "inherit"),
RedirectMode::Print => write!(f, "print"),
RedirectMode::File { file_num } => write!(f, "file({file_num})"),
RedirectMode::Caller => write!(f, "caller"),
}

7
crates/nu-protocol/src/ir/mod.rs
View file

@ -123,9 +123,9 @@ pub enum Instruction {
/// code, and invokes any available error handler with Empty, or if not available, returns an
/// exit-code-only stream, leaving the block.
Drain { src: RegId },
/// Write to the output destinations in the stack
/// Drain the value/stream in a register and discard only if this is the last pipeline element.
// TODO: see if it's possible to remove this
WriteToOutDests { src: RegId },
DrainIfEnd { src: RegId },
/// Load the value of a variable into the `dst` register
LoadVariable { dst: RegId, var_id: VarId },
/// Store the value of a variable from the `src` register
@ -290,7 +290,7 @@ impl Instruction {
Instruction::Span { src_dst } => Some(src_dst),
Instruction::Drop { .. } => None,
Instruction::Drain { .. } => None,
Instruction::WriteToOutDests { .. } => None,
Instruction::DrainIfEnd { .. } => None,
Instruction::LoadVariable { dst, .. } => Some(dst),
Instruction::StoreVariable { .. } => None,
Instruction::DropVariable { .. } => None,
@ -450,6 +450,7 @@ pub enum RedirectMode {
Value,
Null,
Inherit,
Print,
/// Use the given numbered file.
File {
file_num: u32,

94
crates/nu-protocol/src/pipeline/byte_stream.rs
View file

@ -1,7 +1,7 @@
//! Module managing the streaming of raw bytes between pipeline elements
use crate::{
process::{ChildPipe, ChildProcess},
ErrSpan, IntoSpanned, OutDest, PipelineData, ShellError, Signals, Span, Type, Value,
ErrSpan, IntoSpanned, PipelineData, ShellError, Signals, Span, Type, Value,
};
use serde::{Deserialize, Serialize};
#[cfg(unix)]
@ -13,7 +13,6 @@ use std::{
fs::File,
io::{self, BufRead, BufReader, Cursor, ErrorKind, Read, Write},
process::Stdio,
thread,
};
/// The source of bytes for a [`ByteStream`].
@ -600,80 +599,6 @@ impl ByteStream {
}
Ok(())
}
pub(crate) fn write_to_out_dests(
self,
stdout: &OutDest,
stderr: &OutDest,
) -> Result<(), ShellError> {
let span = self.span;
let signals = &self.signals;
match self.stream {
ByteStreamSource::Read(read) => {
write_to_out_dest(read, stdout, true, span, signals)?;
}
ByteStreamSource::File(file) => match stdout {
OutDest::Pipe | OutDest::PipeSeparate | OutDest::Value | OutDest::Null => {}
OutDest::Inherit => {
copy_with_signals(file, io::stdout(), span, signals)?;
}
OutDest::File(f) => {
copy_with_signals(file, f.as_ref(), span, signals)?;
}
},
ByteStreamSource::Child(mut child) => {
match (child.stdout.take(), child.stderr.take()) {
(Some(out), Some(err)) => {
// To avoid deadlocks, we must spawn a separate thread to wait on stderr.
thread::scope(|s| {
let err_thread = thread::Builder::new()
.name("stderr writer".into())
.spawn_scoped(s, || match err {
ChildPipe::Pipe(pipe) => {
write_to_out_dest(pipe, stderr, false, span, signals)
}
ChildPipe::Tee(tee) => {
write_to_out_dest(tee, stderr, false, span, signals)
}
})
.err_span(span);
match out {
ChildPipe::Pipe(pipe) => {
write_to_out_dest(pipe, stdout, true, span, signals)
}
ChildPipe::Tee(tee) => {
write_to_out_dest(tee, stdout, true, span, signals)
}
}?;
if let Ok(result) = err_thread?.join() {
result?;
} else {
// thread panicked, which should not happen
debug_assert!(false)
}
Ok::<_, ShellError>(())
})?;
}
(Some(out), None) => {
// single output stream, we can consume directly
write_to_out_dest(out, stdout, true, span, signals)?;
}
(None, Some(err)) => {
// single output stream, we can consume directly
write_to_out_dest(err, stderr, false, span, signals)?;
}
(None, None) => {}
}
child.wait()?;
}
}
Ok(())
}
}
impl From<ByteStream> for PipelineData {
@ -962,23 +887,6 @@ fn trim_end_newline(string: &mut String) {
}
}
fn write_to_out_dest(
read: impl Read,
stream: &OutDest,
stdout: bool,
span: Span,
signals: &Signals,
) -> Result<(), ShellError> {
match stream {
OutDest::Pipe | OutDest::PipeSeparate | OutDest::Value => return Ok(()),
OutDest::Null => copy_with_signals(read, io::sink(), span, signals),
OutDest::Inherit if stdout => copy_with_signals(read, io::stdout(), span, signals),
OutDest::Inherit => copy_with_signals(read, io::stderr(), span, signals),
OutDest::File(file) => copy_with_signals(read, file.as_ref(), span, signals),
}?;
Ok(())
}
#[cfg(unix)]
pub(crate) fn convert_file<T: From<OwnedFd>>(file: impl Into<OwnedFd>) -> T {
file.into().into()

12
crates/nu-protocol/src/pipeline/out_dest.rs
View file

@ -25,10 +25,16 @@ pub enum OutDest {
///
/// This will forward output to the null device for the platform.
Null,
/// Output to nushell's stdout or stderr.
/// Output to nushell's stdout or stderr (only for external commands).
///
/// This causes external commands to inherit nushell's stdout or stderr.
/// This causes external commands to inherit nushell's stdout or stderr. This also causes
/// [`ListStream`](crate::ListStream)s to be drained, but not to be printed.
Inherit,
/// Print to nushell's stdout or stderr.
///
/// This is just like `Inherit`, except that [`ListStream`](crate::ListStream)s and
/// [`Value`](crate::Value)s are also printed.
Print,
/// Redirect output to a file.
File(Arc<File>), // Arc<File>, since we sometimes need to clone `OutDest` into iterators, etc.
}
@ -52,7 +58,7 @@ impl TryFrom<&OutDest> for Stdio {
match out_dest {
OutDest::Pipe | OutDest::PipeSeparate | OutDest::Value => Ok(Self::piped()),
OutDest::Null => Ok(Self::null()),
OutDest::Inherit => Ok(Self::inherit()),
OutDest::Print | OutDest::Inherit => Ok(Self::inherit()),
OutDest::File(file) => Ok(file.try_clone()?.into()),
}
}

97
crates/nu-protocol/src/pipeline/pipeline_data.rs
View file

@ -165,66 +165,71 @@ impl PipelineData {
}
}
/// Writes all values or redirects all output to the current [`OutDest`]s in `stack`.
/// Drain and write this [`PipelineData`] to `dest`.
///
/// For [`OutDest::Pipe`] and [`OutDest::PipeSeparate`], this will return the `PipelineData` as
/// is without consuming input and without writing anything.
/// Values are converted to bytes and separated by newlines if this is a `ListStream`.
pub fn write_to(self, mut dest: impl Write) -> Result<(), ShellError> {
match self {
PipelineData::Empty => Ok(()),
PipelineData::Value(value, ..) => {
let bytes = value_to_bytes(value)?;
dest.write_all(&bytes)?;
dest.flush()?;
Ok(())
}
PipelineData::ListStream(stream, ..) => {
for value in stream {
let bytes = value_to_bytes(value)?;
dest.write_all(&bytes)?;
dest.write_all(b"\n")?;
}
dest.flush()?;
Ok(())
}
PipelineData::ByteStream(stream, ..) => stream.write_to(dest),
}
}
/// Drain this [`PipelineData`] according to the current stdout [`OutDest`]s in `stack`.
///
/// For the other [`OutDest`]s, the given `PipelineData` will be completely consumed
/// and `PipelineData::Empty` will be returned (assuming no errors).
pub fn write_to_out_dests(
/// For [`OutDest::Pipe`] and [`OutDest::PipeSeparate`], this will return the [`PipelineData`]
/// as is. For [`OutDest::Value`], this will collect into a value and return it. For
/// [`OutDest::Print`], the [`PipelineData`] is drained and printed. Otherwise, the
/// [`PipelineData`] is drained, but only printed if it is the output of an external command.
pub fn drain_to_out_dests(
self,
engine_state: &EngineState,
stack: &mut Stack,
) -> Result<PipelineData, ShellError> {
match (self, stack.stdout()) {
(PipelineData::Empty, ..) => {}
(data, OutDest::Pipe | OutDest::PipeSeparate) => return Ok(data),
(data, OutDest::Value) => {
let metadata = data.metadata();
let span = data.span().unwrap_or(Span::unknown());
return data
.into_value(span)
.map(|val| PipelineData::Value(val, metadata));
) -> Result<Self, ShellError> {
match stack.pipe_stdout().unwrap_or(&OutDest::Inherit) {
OutDest::Print => {
self.print(engine_state, stack, false, false)?;
Ok(Self::Empty)
}
(PipelineData::ByteStream(stream, ..), stdout) => {
stream.write_to_out_dests(stdout, stack.stderr())?;
OutDest::Pipe | OutDest::PipeSeparate => Ok(self),
OutDest::Value => {
let metadata = self.metadata();
let span = self.span().unwrap_or(Span::unknown());
self.into_value(span).map(|val| Self::Value(val, metadata))
}
(PipelineData::Value(..), OutDest::Null) => {}
(PipelineData::ListStream(stream, ..), OutDest::Null) => {
// we need to drain the stream in case there are external commands in the pipeline
stream.drain()?;
OutDest::File(file) => {
self.write_to(file.as_ref())?;
Ok(Self::Empty)
}
(PipelineData::Value(value, ..), OutDest::File(file)) => {
let bytes = value_to_bytes(value)?;
let mut file = file.as_ref();
file.write_all(&bytes)?;
file.flush()?;
}
(PipelineData::ListStream(stream, ..), OutDest::File(file)) => {
let mut file = file.as_ref();
// use BufWriter here?
for value in stream {
let bytes = value_to_bytes(value)?;
file.write_all(&bytes)?;
file.write_all(b"\n")?;
}
file.flush()?;
}
(data @ (PipelineData::Value(..) | PipelineData::ListStream(..)), OutDest::Inherit) => {
data.print(engine_state, stack, false, false)?;
OutDest::Null | OutDest::Inherit => {
self.drain()?;
Ok(Self::Empty)
}
}
Ok(PipelineData::Empty)
}
pub fn drain(self) -> Result<(), ShellError> {
match self {
PipelineData::Empty => Ok(()),
PipelineData::Value(Value::Error { error, .. }, ..) => Err(*error),
PipelineData::Value(..) => Ok(()),
PipelineData::ListStream(stream, ..) => stream.drain(),
PipelineData::ByteStream(stream, ..) => stream.drain(),
Self::Empty => Ok(()),
Self::Value(Value::Error { error, .. }, ..) => Err(*error),
Self::Value(..) => Ok(()),
Self::ListStream(stream, ..) => stream.drain(),
Self::ByteStream(stream, ..) => stream.drain(),
}
}