Repository: reubeno/brush
Pull request #359 (2025-01-25 06:01:47)

fix(redirection): make sure redirection fd + operator are contiguous
Use token location information to confirm that the optional file descriptor that precedes a redirection operator wasn't separated by space that's since been elided by tokenization.

Also adds more tests for redirection and extends the compat test harness to display the actual diff between temp files found to differ.

(Contributes to one of the root causes of #358.)

github-actions[bot]: ## Test Results
    2 files      9 suites   1m 28s ⏱️
  649 tests   649 ✅ 0 💤 0 ❌
1 284 runs  1 284 ✅ 0 💤 0 ❌

Results for commit 66043cf0.

[test-results]:data:application/gzip;base64,H4sIACaBlGcC/13MSw7CIBSF4a00jB1wKSC4GYNQEmJbDI9R4969Ja22Dr//JGchPoxDJreOXTqSaygNGuFqMiXEGakUGpeybpLrXfdcrf1Pz/DCRL/BmzCewpBSTFtJdV4/gSm+af88pd9n8+Gy+fho4zSFgiBSUt5bT62SwlwBhHU9Ez113AmmGDDwD1AayPsD4GTV2gYBAAA=

github-actions[bot]: # Performance Benchmark Report
| Benchmark name                         | Baseline (μs) |  Test/PR (μs) |    Delta (μs) | Delta %         |
| -------------------------------------- | ------------- | ------------- | ------------- | --------------- |
| `clone_shell_object                  ` | `   16.43 μs` | `   16.32 μs` | `   -0.11 μs` | `🟢 -0.66%    ` |
| `eval_arithmetic                     ` | `    0.18 μs` | `    0.17 μs` | `   -0.01 μs` | `🟢 -5.49%    ` |
| `expand_one_string                   ` | `    1.55 μs` | `    1.55 μs` | `    0.01 μs` | `⚪  Unchanged` |
| `for_loop                            ` | `   33.46 μs` | `   33.80 μs` | `    0.34 μs` | `🟠 +1.02%    ` |
| `function_call                       ` | `    3.37 μs` | `    3.22 μs` | `   -0.14 μs` | `🟢 -4.19%    ` |
| `instantiate_shell                   ` | `   49.46 μs` | `   48.18 μs` | `   -1.28 μs` | `🟢 -2.60%    ` |
| `instantiate_shell_with_init_scripts ` | `22083.50 μs` | `21778.20 μs` | ` -305.31 μs` | `🟢 -1.38%    ` |
| `parse_bash_completion               ` | ` 1678.12 μs` | ` 1686.27 μs` | `    8.15 μs` | `⚪  Unchanged` |
| `parse_sample_script                 ` | `    1.75 μs` | `    1.77 μs` | `    0.02 μs` | `🟠 +1.37%    ` |
| `run_echo_builtin_command            ` | `   16.30 μs` | `   15.70 μs` | `   -0.60 μs` | `⚪  Unchanged` |
| `run_one_external_command            ` | ` 2208.43 μs` | ` 1663.28 μs` | ` -545.16 μs` | `🟢 -24.69%   ` |
| `tokenize_sample_script              ` | `    2.86 μs` | `    2.86 μs` | `    0.00 μs` | `⚪  Unchanged` |

# Code Coverage Report: Only Changed Files listed

| Package | Base Coverage | New Coverage | Difference |
| ------- | ------------- | ------------ | ---------- |
| brush-core/src/interp.rs | 🟢 91.18% | 🟢 91.47% | 🟢 0.29% |
| brush-parser/src/parser.rs | 🟢 99.16% | 🟢 99.17% | 🟢 0.01% |
| **Overall Coverage** | **🟢 75.16%** | **🟢 75.2%** | **🟢 0.04%** |

_Minimum allowed coverage is_ `70%`_, this run produced_ `75.2%`
<!-- Sticky Pull Request Comment -->

Patch:
diff --git a/brush-parser/src/parser.rs b/brush-parser/src/parser.rs
index 292eed1..2fce40b 100644
--- a/brush-parser/src/parser.rs
+++ b/brush-parser/src/parser.rs
@@ -771,10 +771,16 @@ peg::parser! {
             linebreak() [Token::Word(e, _)] linebreak() { e }
 
         // N.B. An I/O number must be a string of only digits, and it must be
-        // followed by a '<' or '>' character (but not consume them).
+        // followed by a '<' or '>' character (but not consume them). We also
+        // need to make sure that there was no space between the number and the
+        // redirection operator; unfortunately we don't have the space anymore
+        // but we can infer it by looking at the tokens' locations.
         rule io_number() -> u32 =
-            [Token::Word(w, _) if w.chars().all(|c: char| c.is_ascii_digit())]
-            &([Token::Operator(o, _) if o.starts_with('<') || o.starts_with('>')]) {
+            [Token::Word(w, num_loc) if w.chars().all(|c: char| c.is_ascii_digit())]
+            &([Token::Operator(o, redir_loc) if
+                    o.starts_with(['<', '>']) &&
+                    locations_are_contiguous(num_loc, redir_loc)]) {
+
                 w.parse().unwrap()
             }
 
@@ -881,6 +887,13 @@ fn add_pipe_extension_redirection(c: &mut ast::Command) -> Result<(), &'static s
     Ok(())
 }
 
+fn locations_are_contiguous(
+    loc_left: &crate::TokenLocation,
+    loc_right: &crate::TokenLocation,
+) -> bool {
+    loc_left.end.index == loc_right.start.index
+}
+
 fn parse_array_assignment(
     word: &str,
     elements: &[&String],


Test patch:
diff --git a/brush-shell/tests/cases/redirection.yaml b/brush-shell/tests/cases/redirection.yaml
index 15de255..5fa2d97 100644
--- a/brush-shell/tests/cases/redirection.yaml
+++ b/brush-shell/tests/cases/redirection.yaml
@@ -29,6 +29,20 @@ cases:
     stdin: |
       echo hi >&2
 
+  - name: "Redirecting an fd"
+    stdin: |
+      echo hi-out 1>stdout.txt
+      echo hi-error 2>stderr.txt >&2
+
+  - name: "Almost redirecting but not actually redirecting"
+    stdin: |
+      # Checks to make sure the '1' isn't treated as a file descriptor
+      echo 1 >&2
+
+  - name: "Redirection with arbitrary fd number"
+    stdin: |
+      echo hi 4>file.txt >&4
+
   - name: "Process substitution: basic"
     stdin: |
       shopt -u -o posix
@@ -76,3 +90,24 @@ cases:
     stdin: |
       shopt -u -o posix
       cp <(echo hi) >(cat)
+
+  - name: "Redirection in command substitution"
+    stdin: |
+      echo $(echo hi >&2) 2>stderr.txt
+
+  - name: "Redirection in command substitution to non-standard fd"
+    ignore_stderr: true
+    stdin: |
+      echo $(echo hi >&3) 3>output.txt
+
+  - name: "Redirection in command substitution in braces to non-standard fd"
+    known_failure: true # https://github.com/reubeno/brush/issues/358
+    ignore_stderr: true
+    stdin: |
+      { : $(echo hi >&3); } 3>output.txt
+
+  - name: "Redirection in command substitution in subshell to non-standard fd"
+    known_failure: true # https://github.com/reubeno/brush/issues/358
+    ignore_stderr: true
+    stdin: |
+      ( : $(echo hi >&3); ) 3>output.txt
diff --git a/brush-shell/tests/compat_tests.rs b/brush-shell/tests/compat_tests.rs
index 0d75fb5..4521792 100644
--- a/brush-shell/tests/compat_tests.rs
+++ b/brush-shell/tests/compat_tests.rs
@@ -715,12 +715,38 @@ impl TestCaseResult {
                 for entry in entries {
                     const INDENT: &str = "        ";
                     match entry {
-                        DirComparisonEntry::Different(left, right) => {
+                        DirComparisonEntry::Different(
+                            left_path,
+                            left_contents,
+                            right_path,
+                            right_contents,
+                        ) => {
                             writeln!(
                                 writer,
                                 "{INDENT}oracle file {} differs from test file {}",
-                                left.to_string_lossy(),
-                                right.to_string_lossy()
+                                left_path.to_string_lossy(),
+                                right_path.to_string_lossy()
+                            )?;
+
+                            writeln!(
+                                writer,
+                                "{INDENT}{}",
+                                "------ Oracle <> Test: file ---------------------------------"
+                                    .cyan()
+                            )?;
+
+                            write_diff(
+                                &mut writer,
+                                8,
+                                left_contents.as_str(),
+                                right_contents.as_str(),
+                            )?;
+
+                            writeln!(
+                                writer,
+                                "        {}",
+                                "---------------------------------------------------------------"
+                                    .cyan()
                             )?;
                         }
                         DirComparisonEntry::LeftOnly(p) => {
@@ -1227,7 +1253,7 @@ impl StringComparison {
 enum DirComparisonEntry {
     LeftOnly(PathBuf),
     RightOnly(PathBuf),
-    Different(PathBuf, PathBuf),
+    Different(PathBuf, String, PathBuf, String),
 }
 
 enum DirComparison {
@@ -1265,7 +1291,9 @@ fn diff_dirs(oracle_path: &Path, test_path: &Path) -> Result<DirComparison> {
             }
             dir_cmp::full::DirCmpEntry::Both(l, r, _) => DirComparisonEntry::Different(
                 pathdiff::diff_paths(l, oracle_path).unwrap(),
+                std::fs::read_to_string(l).unwrap(),
                 pathdiff::diff_paths(r, test_path).unwrap(),
+                std::fs::read_to_string(r).unwrap(),
             ),
         })
         .collect();