Repository: reubeno/brush
Pull request #194 (2024-10-09 00:18:36)

fix: workarounds for edge word parsing cases
* Fixes 'Binary string matching with expansion' test case
* Adds + fixes 'Command substitution with embedded parens' test case

github-actions[bot]: ## Test Results
  2 files    9 suites   55s :stopwatch:
441 tests 441 :white_check_mark: 0 :zzz: 0 :x:
876 runs  876 :white_check_mark: 0 :zzz: 0 :x:

Results for commit 41a2d2e9.

[test-results]:data:application/gzip;base64,H4sIALPNBWcC/13MSw7CIBSF4a00jB30Igh1MwYukBDbYniMjHuXklbR4fef5DyJ87NN5DrQ00BS8blhqjAlquzDWsl5dV3ytjEGh26pIP6nu3/UNH6CU37+CTbGEPcSy7p9SnHZcVz25fvY3B02938YlsXnCsJAUUPtJFEJRVHaM3BmlHZMCIqagh4FAAPyegM1wiQbBAEAAA==

github-actions[bot]: # Performance Benchmark Report
| Benchmark name                         | Baseline (μs) |  Test/PR (μs) |    Delta (μs) | Delta %         |
| -------------------------------------- | ------------- | ------------- | ------------- | --------------- |
| `expand_one_string                   ` | `    3.33 μs` | `    3.33 μs` | `    0.00 μs` | `⚪  Unchanged` |
| `instantiate_shell                   ` | `   60.45 μs` | `   59.85 μs` | `   -0.60 μs` | `⚪  Unchanged` |
| `instantiate_shell_with_init_scripts ` | `28686.28 μs` | `28646.71 μs` | `  -39.57 μs` | `⚪  Unchanged` |
| `parse_bash_completion               ` | ` 2712.21 μs` | ` 2692.57 μs` | `  -19.64 μs` | `⚪  Unchanged` |
| `parse_sample_script                 ` | `    4.07 μs` | `    4.01 μs` | `   -0.06 μs` | `⚪  Unchanged` |
| `run_echo_builtin_command            ` | `   90.79 μs` | `   91.07 μs` | `    0.28 μs` | `⚪  Unchanged` |
| `run_one_builtin_command             ` | `  108.65 μs` | `  108.90 μs` | `    0.25 μs` | `⚪  Unchanged` |
| `run_one_external_command            ` | ` 2048.63 μs` | ` 1897.67 μs` | ` -150.95 μs` | `🟢 -7.37%    ` |
| `run_one_external_command_directly   ` | ` 1008.11 μs` | ` 1006.86 μs` | `   -1.25 μs` | `⚪  Unchanged` |

# Code Coverage Report: Only Changed Files listed

| Package | Base Coverage | New Coverage | Difference |
| ------- | ------------- | ------------ | ---------- |
| brush-core/src/patterns.rs | 🟢 96.98% | 🟢 97.14% | 🟢 0.16% |
| brush-interactive/src/reedline/reedline_shell.rs | 🟢 89.84% | 🟢 89.68% | 🔴 -0.16% |
| **Overall Coverage** | **🟢 71.98%** | **🟢 72.06%** | **🟢 0.08%** |

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

Patch:
diff --git a/brush-core/src/patterns.rs b/brush-core/src/patterns.rs
index 1efa9e4..c1c6340 100644
--- a/brush-core/src/patterns.rs
+++ b/brush-core/src/patterns.rs
@@ -247,15 +247,37 @@ impl Pattern {
             regex_str.push('^');
         }
 
+        let mut current_pattern = String::new();
         for piece in &self.pieces {
-            let next_part = match piece {
+            match piece {
                 PatternPiece::Pattern(s) => {
-                    pattern_to_regex_str(s, false, false, enable_extended_globbing)?
+                    current_pattern.push_str(s);
                 }
-                PatternPiece::Literal(s) => escape_for_regex(s),
-            };
+                PatternPiece::Literal(s) => {
+                    if !current_pattern.is_empty() {
+                        let regex_piece = pattern_to_regex_str(
+                            current_pattern.as_str(),
+                            false,
+                            false,
+                            enable_extended_globbing,
+                        )?;
+                        regex_str.push_str(regex_piece.as_str());
+                        current_pattern = String::new();
+                    }
+
+                    regex_str.push_str(escape_for_regex(s).as_str());
+                }
+            }
+        }
 
-            regex_str.push_str(next_part.as_str());
+        if !current_pattern.is_empty() {
+            let regex_piece = pattern_to_regex_str(
+                current_pattern.as_str(),
+                false,
+                false,
+                enable_extended_globbing,
+            )?;
+            regex_str.push_str(regex_piece.as_str());
         }
 
         if strict_suffix_match {
diff --git a/brush-interactive/src/reedline/completer.rs b/brush-interactive/src/reedline/completer.rs
index b539b9c..385f084 100644
--- a/brush-interactive/src/reedline/completer.rs
+++ b/brush-interactive/src/reedline/completer.rs
@@ -37,7 +37,7 @@ impl ReedlineCompleter {
         insertion_index: usize,
         delete_count: usize,
     ) -> reedline::Suggestion {
-        let mut style = nu_ansi_term::Style::new().dimmed();
+        let mut style = nu_ansi_term::Style::new();
         if candidate.ends_with(std::path::MAIN_SEPARATOR) {
             style = style.fg(nu_ansi_term::Color::Green);
         }
diff --git a/brush-interactive/src/reedline/reedline_shell.rs b/brush-interactive/src/reedline/reedline_shell.rs
index a574d9e..adca0ee 100644
--- a/brush-interactive/src/reedline/reedline_shell.rs
+++ b/brush-interactive/src/reedline/reedline_shell.rs
@@ -1,3 +1,4 @@
+use nu_ansi_term::Color;
 use reedline::MenuBuilder;
 use std::sync::Arc;
 use tokio::sync::Mutex;
@@ -45,7 +46,9 @@ impl ReedlineShell {
         let completion_menu = Box::new(
             reedline::ColumnarMenu::default()
                 .with_name(COMPLETION_MENU_NAME)
-                .with_marker(""),
+                .with_marker("")
+                .with_selected_text_style(Color::Blue.bold().reverse())
+                .with_selected_match_text_style(Color::Blue.bold().reverse()),
         );
 
         // Set up key bindings.
@@ -54,11 +57,7 @@ impl ReedlineShell {
         // Set up default history-based hinter.
         let mut hinter = reedline::DefaultHinter::default();
         if !options.disable_color {
-            hinter = hinter.with_style(
-                nu_ansi_term::Style::new()
-                    .italic()
-                    .fg(nu_ansi_term::Color::DarkGray),
-            );
+            hinter = hinter.with_style(nu_ansi_term::Style::new().italic().fg(Color::DarkGray));
         }
 
         // Instantiate reedline with some defaults and hand it ownership of
diff --git a/brush-parser/src/word.rs b/brush-parser/src/word.rs
index cd9eefc..929482d 100644
--- a/brush-parser/src/word.rs
+++ b/brush-parser/src/word.rs
@@ -386,7 +386,7 @@ fn cacheable_parse(
     let pieces = expansion_parser::unexpanded_word(word.as_str(), &options)
         .map_err(|err| error::WordParseError::Word(word.to_owned(), err))?;
 
-    tracing::debug!("Parsed word '{}' => {{{:?}}}", word, pieces);
+    tracing::debug!(target: "parse", "Parsed word '{}' => {{{:?}}}", word, pieces);
 
     Ok(pieces)
 }
@@ -411,7 +411,7 @@ peg::parser! {
         pub(crate) rule unexpanded_word() -> Vec<WordPieceWithSource> = word(<![_]>)
 
         rule word<T>(stop_condition: rule<T>) -> Vec<WordPieceWithSource> =
-            tilde:tilde_prefix_with_source()? pieces:word_piece_with_source(<stop_condition()>)* {
+            tilde:tilde_prefix_with_source()? pieces:word_piece_with_source(<stop_condition()>, false /*in_command*/)* {
                 let mut all_pieces = Vec::new();
                 if let Some(tilde) = tilde {
                     all_pieces.push(tilde);
@@ -427,7 +427,7 @@ peg::parser! {
 
         rule arithmetic_word_piece<T>(stop_condition: rule<T>) =
             "(" arithmetic_word_plus_right_paren() {} /
-            word_piece(<param_rule_or_open_paren(<stop_condition()>)>) {}
+            word_piece(<param_rule_or_open_paren(<stop_condition()>)>, false /*in_command*/) {}
 
         rule param_rule_or_open_paren<T>(stop_condition: rule<T>) -> () =
             stop_condition() {} /
@@ -435,18 +435,18 @@ peg::parser! {
 
         rule arithmetic_word_plus_right_paren() =
             "(" arithmetic_word_plus_right_paren() ")" /
-            word_piece(<[')']>)* ")" {}
+            word_piece(<[')']>, false /*in_command*/)* ")" {}
 
-        rule word_piece_with_source<T>(stop_condition: rule<T>) -> WordPieceWithSource =
-            start_index:position!() piece:word_piece(<stop_condition()>) end_index:position!() {
+        rule word_piece_with_source<T>(stop_condition: rule<T>, in_command: bool) -> WordPieceWithSource =
+            start_index:position!() piece:word_piece(<stop_condition()>, in_command) end_index:position!() {
                 WordPieceWithSource { piece, start_index, end_index }
             }
 
-        rule word_piece<T>(stop_condition: rule<T>) -> WordPiece =
+        rule word_piece<T>(stop_condition: rule<T>, in_command: bool) -> WordPiece =
             arithmetic_expansion() /
             command_substitution() /
             parameter_expansion() /
-            unquoted_text(<stop_condition()>)
+            unquoted_text(<stop_condition()>, in_command)
 
         rule double_quoted_word_piece() -> WordPiece =
             arithmetic_expansion() /
@@ -455,12 +455,12 @@ peg::parser! {
             double_quoted_escape_sequence() /
             double_quoted_text()
 
-        rule unquoted_text<T>(stop_condition: rule<T>) -> WordPiece =
+        rule unquoted_text<T>(stop_condition: rule<T>, in_command: bool) -> WordPiece =
             s:double_quoted_sequence() { WordPiece::DoubleQuotedSequence(s) } /
             s:single_quoted_literal_text() { WordPiece::SingleQuotedText(s.to_owned()) } /
             s:ansi_c_quoted_text() { WordPiece::AnsiCQuotedText(s.to_owned()) } /
             normal_escape_sequence() /
-            unquoted_literal_text(<stop_condition()>)
+            unquoted_literal_text(<stop_condition()>, in_command)
 
         rule double_quoted_sequence() -> Vec<WordPieceWithSource> =
             "\"" i:double_quoted_sequence_inner()* "\"" { i }
@@ -480,18 +480,25 @@ peg::parser! {
         rule ansi_c_quoted_text() -> &'input str =
             "$\'" inner:$([^'\'']*) "\'" { inner }
 
-        rule unquoted_literal_text<T>(stop_condition: rule<T>) -> WordPiece =
-            s:$(unquoted_literal_text_piece(<stop_condition()>)+) { WordPiece::Text(s.to_owned()) }
+        rule unquoted_literal_text<T>(stop_condition: rule<T>, in_command: bool) -> WordPiece =
+            s:$(unquoted_literal_text_piece(<stop_condition()>, in_command)+) { WordPiece::Text(s.to_owned()) }
 
-        rule unquoted_literal_text_piece<T>(stop_condition: rule<T>) =
-            extglob_pattern() /
+        // TODO: Find a way to remove the special-case logic for extglob + subshell commands
+        rule unquoted_literal_text_piece<T>(stop_condition: rule<T>, in_command: bool) =
+            is_true(in_command) extglob_pattern() /
+            is_true(in_command) subshell_command() /
             !stop_condition() !normal_escape_sequence() [^'$' | '\'' | '\"'] {}
 
+        rule is_true(value: bool) = &[_] {? if value { Ok(()) } else { Err("not true") } }
+
         rule extglob_pattern() =
             ("@" / "!" / "?" / "+" / "*") "(" extglob_body_piece()* ")" {}
 
         rule extglob_body_piece() =
-            word_piece(<[')']>) {}
+            word_piece(<[')']>, true /*in_command*/) {}
+
+        rule subshell_command() =
+            "(" command() ")" {}
 
         rule double_quoted_text() -> WordPiece =
             s:double_quote_body_text() { WordPiece::Text(s.to_owned()) }
@@ -677,7 +684,7 @@ peg::parser! {
             $(command_piece()*)
 
         pub(crate) rule command_piece() -> () =
-            word_piece(<[')']>) {} /
+            word_piece(<[')']>, true /*in_command*/) {} /
             ([' ' | '\t'])+ {}
 
         rule backquoted_command() -> String =
@@ -767,4 +774,17 @@ mod tests {
 
         Ok(())
     }
+
+    #[test]
+    fn parse_extglob_with_embedded_parameter() -> Result<()> {
+        let parsed = super::parse("+([$var])", &ParserOptions::default())?;
+        assert_matches!(
+            &parsed[..],
+            [WordPieceWithSource { piece: WordPiece::Text(s1), .. },
+             WordPieceWithSource { piece: WordPiece::ParameterExpansion(ParameterExpr::Parameter { parameter: Parameter::Named(s2), .. }), ..},
+             WordPieceWithSource { piece: WordPiece::Text(s3), .. }] if s1 == "+([" && s2 == "var" && s3 == "])"
+        );
+
+        Ok(())
+    }
 }


Test patch:
diff --git a/brush-shell/tests/cases/extended_tests.yaml b/brush-shell/tests/cases/extended_tests.yaml
index 51e818f..00c4696 100644
--- a/brush-shell/tests/cases/extended_tests.yaml
+++ b/brush-shell/tests/cases/extended_tests.yaml
@@ -125,7 +125,6 @@ cases:
       [[ a* != "abc" ]] && echo "3. Pass"
 
   - name: "Binary string matching with expansion"
-    known_failure: true
     stdin: |
       exclude="0123456789"
       [[ "clue" == +([$exclude]) ]] && echo "1. Fail"
diff --git a/brush-shell/tests/cases/word_expansion.yaml b/brush-shell/tests/cases/word_expansion.yaml
index 4f02114..2eadf6b 100644
--- a/brush-shell/tests/cases/word_expansion.yaml
+++ b/brush-shell/tests/cases/word_expansion.yaml
@@ -56,6 +56,11 @@ cases:
       echo "$(var="updated"; echo ${var})"
       echo "var=${var}"
 
+  - name: "Command substitution with embedded parens"
+    stdin: |
+      x=$(echo foo | (wc -l; echo hi))
+      echo "\$x: $x"
+
   - name: "Backtick command substitution"
     stdin: |
       echo `echo hi`