Repository: reubeno/brush
Pull request #212 (2024-10-17 05:24:28)

fix: handle expansion in here documents

github-actions[bot]: # Performance Benchmark Report
| Benchmark name                         | Baseline (μs) |  Test/PR (μs) |    Delta (μs) | Delta %         |
| -------------------------------------- | ------------- | ------------- | ------------- | --------------- |
| `expand_one_string                   ` | `    3.42 μs` | `    3.41 μs` | `   -0.00 μs` | `⚪  Unchanged` |
| `instantiate_shell                   ` | `   60.22 μs` | `   60.27 μs` | `    0.05 μs` | `⚪  Unchanged` |
| `instantiate_shell_with_init_scripts ` | `27913.94 μs` | `27831.33 μs` | `  -82.62 μs` | `⚪  Unchanged` |
| `parse_bash_completion               ` | ` 2662.28 μs` | ` 2724.61 μs` | `   62.34 μs` | `🟠 +2.34%    ` |
| `parse_sample_script                 ` | `    3.95 μs` | `    4.02 μs` | `    0.07 μs` | `🟠 +1.87%    ` |
| `run_echo_builtin_command            ` | `   90.54 μs` | `   90.28 μs` | `   -0.26 μs` | `⚪  Unchanged` |
| `run_one_builtin_command             ` | `  108.24 μs` | `  108.65 μs` | `    0.42 μs` | `⚪  Unchanged` |
| `run_one_external_command            ` | ` 1952.37 μs` | ` 1881.67 μs` | `  -70.70 μs` | `🟢 -3.62%    ` |
| `run_one_external_command_directly   ` | `  998.28 μs` | ` 1008.35 μs` | `   10.07 μs` | `⚪  Unchanged` |

# Code Coverage Report: Only Changed Files listed

| Package | Base Coverage | New Coverage | Difference |
| ------- | ------------- | ------------ | ---------- |
| brush-core/src/jobs.rs | 🔴 42.42% | 🔴 37.23% | 🔴 -5.19% |
| brush-parser/src/tokenizer.rs | 🟢 96.03% | 🟢 96.37% | 🟢 0.34% |
| **Overall Coverage** | **🟢 72.65%** | **🟢 72.65%** | **⚪ 0%** |

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

Patch:
diff --git a/brush-core/src/interp.rs b/brush-core/src/interp.rs
index 29d0700..bff9179 100644
--- a/brush-core/src/interp.rs
+++ b/brush-core/src/interp.rs
@@ -1393,8 +1393,12 @@ pub(crate) async fn setup_redirect<'a>(
             // If not specified, default to stdin (fd 0).
             let fd_num = fd_num.unwrap_or(0);
 
-            // TODO: figure out if we need to expand?
-            let io_here_doc = io_here.doc.flatten();
+            // Expand if required.
+            let io_here_doc = if io_here.requires_expansion {
+                expansion::basic_expand_word(shell, &io_here.doc).await?
+            } else {
+                io_here.doc.flatten()
+            };
 
             let f = setup_open_file_with_contents(io_here_doc.as_str())?;
 
diff --git a/brush-parser/src/ast.rs b/brush-parser/src/ast.rs
index 50e4db1..3cd0d06 100644
--- a/brush-parser/src/ast.rs
+++ b/brush-parser/src/ast.rs
@@ -785,6 +785,7 @@ impl Display for IoRedirect {
                     remove_tabs,
                     here_end,
                     doc,
+                    ..
                 },
             ) => {
                 if let Some(fd_num) = fd_num {
@@ -879,6 +880,8 @@ impl Display for IoFileRedirectTarget {
 pub struct IoHereDocument {
     /// Whether to remove leading tabs from the here document.
     pub remove_tabs: bool,
+    /// Whether to basic-expand the contents of the here document.
+    pub requires_expansion: bool,
     /// The delimiter marking the end of the here document.
     pub here_end: Word,
     /// The contents of the here document.
diff --git a/brush-parser/src/parser.rs b/brush-parser/src/parser.rs
index 59c854e..602db33 100644
--- a/brush-parser/src/parser.rs
+++ b/brush-parser/src/parser.rs
@@ -618,8 +618,24 @@ peg::parser! {
             word()
 
         rule io_here() -> ast::IoHereDocument =
-            specific_operator("<<") here_end:here_end() doc:[_] { ast::IoHereDocument { remove_tabs: false, here_end: ast::Word::from(here_end), doc: ast::Word::from(doc) } } /
-            specific_operator("<<-") here_end:here_end() doc:[_] { ast::IoHereDocument { remove_tabs: true, here_end: ast::Word::from(here_end), doc: ast::Word::from(doc) } }
+           specific_operator("<<-") here_end:here_end() doc:[_] {
+                let requires_expansion = !here_end.to_str().contains(['\'', '"', '\\']);
+                ast::IoHereDocument {
+                    remove_tabs: true,
+                    requires_expansion,
+                    here_end: ast::Word::from(here_end),
+                    doc: ast::Word::from(doc)
+                }
+            } /
+            specific_operator("<<") here_end:here_end() doc:[_] {
+                let requires_expansion = !here_end.to_str().contains(['\'', '"', '\\']);
+                ast::IoHereDocument {
+                    remove_tabs: false,
+                    requires_expansion,
+                    here_end: ast::Word::from(here_end),
+                    doc: ast::Word::from(doc)
+                }
+            }
 
         rule here_end() -> &'input Token =
             word()
diff --git a/brush-parser/src/tokenizer.rs b/brush-parser/src/tokenizer.rs
index 4a4125b..f20bb9b 100644
--- a/brush-parser/src/tokenizer.rs
+++ b/brush-parser/src/tokenizer.rs
@@ -1,3 +1,4 @@
+use std::borrow::Cow;
 use std::fmt::Display;
 use utf8_chars::BufReadCharsExt;
 
@@ -132,10 +133,6 @@ pub enum TokenizerError {
     /// An I/O error occurred while reading from the input stream.
     #[error("failed to read input")]
     ReadError(#[from] std::io::Error),
-
-    /// An unimplemented tokenization feature was encountered.
-    #[error("unimplemented tokenization: {0}")]
-    Unimplemented(&'static str),
 }
 
 impl TokenizerError {
@@ -187,6 +184,7 @@ enum HereState {
 #[derive(Clone, Debug)]
 struct HereTag {
     tag: String,
+    tag_was_escaped_or_quoted: bool,
     remove_tabs: bool,
     position: SourcePosition,
     pending_tokens_after: Vec<TokenizeResult>,
@@ -333,16 +331,12 @@ impl TokenParseState {
 
                 cross_token_state.here_state = HereState::NextLineIsHereDoc;
 
-                if self.current_token().contains('\"')
-                    || self.current_token().contains('\'')
-                    || self.current_token().contains('\\')
-                {
-                    return Err(TokenizerError::Unimplemented("quoted or escaped here tag"));
-                }
+                let tag = self.current_token();
 
                 // Include the \n in the here tag so it's easier to check against.
                 cross_token_state.current_here_tags.push(HereTag {
-                    tag: std::format!("\n{}\n", self.current_token()),
+                    tag: std::format!("\n{}\n", tag),
+                    tag_was_escaped_or_quoted: tag.contains(is_quoting_char),
                     remove_tabs,
                     position: cross_token_state.cursor.clone(),
                     pending_tokens_after: vec![],
@@ -557,9 +551,16 @@ impl<'a, R: ?Sized + std::io::BufRead> Tokenizer<'a, R> {
                     self.consume_char()?;
                     state.append_char(c);
 
+                    let next_here_tag = &self.cross_state.current_here_tags[0];
+                    let tag_str: Cow<'_, str> = if next_here_tag.tag_was_escaped_or_quoted {
+                        remove_quotes_from(next_here_tag.tag.as_str()).into()
+                    } else {
+                        next_here_tag.tag.as_str().into()
+                    };
+
                     let without_suffix = state
                         .current_token()
-                        .strip_suffix(self.cross_state.current_here_tags[0].tag.as_str())
+                        .strip_suffix(tag_str.as_ref())
                         .map(|s| s.to_owned());
 
                     if let Some(mut without_suffix) = without_suffix {
@@ -1009,6 +1010,25 @@ fn is_quoting_char(c: char) -> bool {
     matches!(c, '\\' | '\'' | '\"')
 }
 
+fn remove_quotes_from(s: &str) -> String {
+    let mut result = String::new();
+
+    let mut in_escape = false;
+    for c in s.chars() {
+        match c {
+            c if in_escape => {
+                result.push(c);
+                in_escape = false;
+            }
+            '\\' => in_escape = true,
+            c if is_quoting_char(c) => (),
+            c => result.push(c),
+        }
+    }
+
+    result
+}
+
 #[cfg(test)]
 mod tests {
     use super::*;
@@ -1369,4 +1389,12 @@ SOMETHING
         );
         Ok(())
     }
+
+    #[test]
+    fn test_quote_removal() {
+        assert_eq!(remove_quotes_from(r#""hello""#), "hello");
+        assert_eq!(remove_quotes_from(r#"'hello'"#), "hello");
+        assert_eq!(remove_quotes_from(r#""hel\"lo""#), r#"hel"lo"#);
+        assert_eq!(remove_quotes_from(r#"'hel\'lo'"#), r#"hel'lo"#);
+    }
 }


Test patch:
diff --git a/brush-shell/tests/cases/builtins/alias.yaml b/brush-shell/tests/cases/builtins/alias.yaml
index 664b22a..03a54cd 100644
--- a/brush-shell/tests/cases/builtins/alias.yaml
+++ b/brush-shell/tests/cases/builtins/alias.yaml
@@ -7,10 +7,27 @@ cases:
       alias
       myalias 'hello'
 
-  - name: "Alias referencing to alias"
+  - name: "Alias with trailing space"
     known_failure: true
+    stdin: |
+      shopt -s expand_aliases
+      alias cmd='echo '
+      alias other='replaced '
+      alias otherother='also-replaced'
+
+      cmd other otherother
+
+  - name: "Alias referencing to alias"
+    known_failure: true # Issue #57
     stdin: |
       shopt -s expand_aliases
       alias myalias=echo
       alias outeralias=myalias
       outeralias 'hello'
+
+  - name: "Alias to keywords"
+    known_failure: true
+    stdin: |
+      shopt -s expand_aliases
+      alias myalias=if
+      myalias true; then echo "true"; fi
diff --git a/brush-shell/tests/cases/here.yaml b/brush-shell/tests/cases/here.yaml
index 42beb1c..221a29c 100644
--- a/brush-shell/tests/cases/here.yaml
+++ b/brush-shell/tests/cases/here.yaml
@@ -12,7 +12,6 @@ cases:
     args: ["./script.sh"]
 
   - name: "Here doc with expansions"
-    known_failure: true # TODO: needs triage and debugging
     stdin: |
       cat <<END-MARKER
       Something here...
@@ -20,6 +19,14 @@ cases:
       $(echo "This is after")
       END-MARKER
 
+  - name: "Here doc with expansions but quoted tag"
+    stdin: |
+      cat <<"END-MARKER"
+      Something here...
+      ...and here.
+      $(echo "This is after")
+      END-MARKER
+
   - name: "Here doc with tab removal"
     stdin: |
       cat <<-END-MARKER