From 5f7afafe51727b08996b27d1b9eb68e82bc019b4 Mon Sep 17 00:00:00 2001
From: Devyn Cairns <devyn.cairns@gmail.com>
Date: Sat, 27 Jul 2024 19:38:57 -0700
Subject: [PATCH] IR: fix incorrect capturing of subexpressions (#13467)

# Description


[Discovered](https://discord.com/channels/601130461678272522/614593951969574961/1266503282554179604)
by `@warp` on Discord.

The IR compiler was not properly setting redirect modes for
subexpressions because `FullCellPath` was always being compiled with
capture-out redirection. This is the correct behavior if there is a tail
to the `FullCellPath`, as we need the value in order to try to extract
anything from it (although this is unlikely to work) - however, the
parser also generates `FullCellPath`s with an empty tail quite often,
including for bare subexpressions.

Because of this, the following did not behave as expected:

```nushell
(docker run -it --rm alpine)
```

Capturing the output meant that `docker` didn't have direct access to
the terminal as a TTY.

As this is a minor bug fix, it should be okay to include in the 0.96.1
patch release.

# User-Facing Changes

- Fixes the bug as described when running with IR evaluation enabled.

# Tests + Formatting

I added a test for this, though we're not currently running all tests
with IR on the CI, but it should ensure this behaviour is consistent.
The equivalent minimum repro I could find was:

```nushell
(nu --testbin cococo); null
```

as this should cause the `cococo` message to appear on stdout, and if
Nushell is capturing the output, it would be discarded instead.
---
 crates/nu-engine/src/compile/expression.rs | 10 +++++++++-
 tests/shell/pipeline/commands/external.rs  |  6 ++++++
 2 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/crates/nu-engine/src/compile/expression.rs b/crates/nu-engine/src/compile/expression.rs
index 948eb5c238..80fc4fb3fb 100644
--- a/crates/nu-engine/src/compile/expression.rs
+++ b/crates/nu-engine/src/compile/expression.rs
@@ -444,7 +444,15 @@ pub(crate) fn compile_expression(
                     working_set,
                     builder,
                     &full_cell_path.head,
-                    RedirectModes::capture_out(expr.span),
+                    // Only capture the output if there is a tail. This was a bit of a headscratcher
+                    // as the parser emits a FullCellPath with no tail for subexpressions in
+                    // general, which shouldn't be captured any differently than they otherwise
+                    // would be.
+                    if !full_cell_path.tail.is_empty() {
+                        RedirectModes::capture_out(expr.span)
+                    } else {
+                        redirect_modes
+                    },
                     in_reg,
                     out_reg,
                 )?;
diff --git a/tests/shell/pipeline/commands/external.rs b/tests/shell/pipeline/commands/external.rs
index a6efe2b6c9..ff851a3293 100644
--- a/tests/shell/pipeline/commands/external.rs
+++ b/tests/shell/pipeline/commands/external.rs
@@ -209,6 +209,12 @@ fn run_glob_if_pass_variable_to_external() {
     })
 }
 
+#[test]
+fn subexpression_does_not_implicitly_capture() {
+    let actual = nu!("(nu --testbin cococo); null");
+    assert_eq!(actual.out, "cococo");
+}
+
 mod it_evaluation {
     use super::nu;
     use nu_test_support::fs::Stub::{EmptyFile, FileWithContent, FileWithContentToBeTrimmed};