From b943cbedffe8453dd29d3e532d5bc7bdf152e22a Mon Sep 17 00:00:00 2001
From: Horasal <1991933+horasal@users.noreply.github.com>
Date: Thu, 31 Aug 2023 03:24:13 +0900
Subject: [PATCH] skip comments and eols while parsing pipeline (#10149)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

This pr
- fixes https://github.com/nushell/nushell/issues/10143
- fixes https://github.com/nushell/nushell/issues/5559

# Description

Current `lite_parse` does not handle multiple line comments and eols in
pipeline.
When parsing the following tokens:


| `"abcdefg"` | ` \|` | `# foobar` | ` \n` | `split chars` |
| ------------- | ------------- |------------- |-------------
|------------- |
| [Command] | [Pipe] | [Comment] | [Eol] | [Command] |
| | | Last Token |Current Token | |

`TokenContent::Eol` handler only checks if `last_token` is `Pipe` but it
will be broken if there exist any other thing, e.g. extra `[Comment]` in
this example.

This pr make the following change:

- While parsing `[Eol]`, try to find the last non-comment token as
`last_token`
- Comment is supposed as `[Comment]+` or `([Comment] [Eol])+`
- `[Eol]+` is still parsed just like current nu (i.e. generates
`nothing`).

Notice that this pr is just a quick patch if more comment/eol related
issue occures, `lite_parser` may need a rewrite.

# User-Facing Changes

Now the following pipeline works:

```bash
1 | # comment
each { |it| $it + 2 } | # comment
math sum
```

Comment will not end the pipeline in interactive mode:

```bash
❯ 1 | # comment   (now enter multiple line mode instead of end)
▶▶ # foo
▶▶ 2
```

# Tests + Formatting

- :green_circle: `toolkit fmt`
- :green_circle: `toolkit clippy`
- :green_circle: `toolkit test`
- :green_circle: `toolkit test stdlib`

# After Submitting

None

---------

Co-authored-by: Horasal <horsal@horsal.dev>
---
 crates/nu-parser/src/lite_parser.rs | 26 ++++++++++++++++++---
 src/tests/test_parser.rs            | 35 +++++++++++++++++++++++++++++
 2 files changed, 58 insertions(+), 3 deletions(-)

diff --git a/crates/nu-parser/src/lite_parser.rs b/crates/nu-parser/src/lite_parser.rs
index 881aaa19d8..6313b01d17 100644
--- a/crates/nu-parser/src/lite_parser.rs
+++ b/crates/nu-parser/src/lite_parser.rs
@@ -185,6 +185,20 @@ impl LiteBlock {
     }
 }
 
+fn last_non_comment_token(tokens: &[Token], cur_idx: usize) -> Option<TokenContents> {
+    let mut expect = TokenContents::Comment;
+    for token in tokens.iter().take(cur_idx).rev() {
+        // skip ([Comment]+ [Eol]) pair
+        match (token.contents, expect) {
+            (TokenContents::Comment, TokenContents::Comment)
+            | (TokenContents::Comment, TokenContents::Eol) => expect = TokenContents::Eol,
+            (TokenContents::Eol, TokenContents::Eol) => expect = TokenContents::Comment,
+            (token, _) => return Some(token),
+        }
+    }
+    None
+}
+
 pub fn lite_parse(tokens: &[Token]) -> (LiteBlock, Option<ParseError>) {
     let mut block = LiteBlock::new();
     let mut curr_pipeline = LitePipeline::new();
@@ -203,7 +217,7 @@ pub fn lite_parse(tokens: &[Token]) -> (LiteBlock, Option<ParseError>) {
 
     let mut error = None;
 
-    for token in tokens.iter() {
+    for (idx, token) in tokens.iter().enumerate() {
         match &token.contents {
             TokenContents::PipePipe => {
                 error = error.or(Some(ParseError::ShellOrOr(token.span)));
@@ -294,7 +308,13 @@ pub fn lite_parse(tokens: &[Token]) -> (LiteBlock, Option<ParseError>) {
                 last_connector_span = Some(token.span);
             }
             TokenContents::Eol => {
-                if last_token != TokenContents::Pipe && last_token != TokenContents::OutGreaterThan
+                // Handle `[Command] [Pipe] ([Comment] | [Eol])+ [Command]`
+                //
+                // `[Eol]` branch checks if previous token is `[Pipe]` to construct pipeline
+                // and so `[Comment] | [Eol]` should be ignore to make it work
+                let actual_token = last_non_comment_token(tokens, idx);
+                if actual_token != Some(TokenContents::Pipe)
+                    && actual_token != Some(TokenContents::OutGreaterThan)
                 {
                     if !curr_command.is_empty() {
                         match last_connector {
@@ -451,7 +471,7 @@ pub fn lite_parse(tokens: &[Token]) -> (LiteBlock, Option<ParseError>) {
         block.push(curr_pipeline);
     }
 
-    if last_token == TokenContents::Pipe {
+    if last_non_comment_token(tokens, tokens.len()) == Some(TokenContents::Pipe) {
         (
             block,
             Some(ParseError::UnexpectedEof(
diff --git a/src/tests/test_parser.rs b/src/tests/test_parser.rs
index 57e24aed53..49289a7cf2 100644
--- a/src/tests/test_parser.rs
+++ b/src/tests/test_parser.rs
@@ -135,6 +135,41 @@ fn comment_skipping_2() -> TestResult {
     )
 }
 
+#[test]
+fn comment_skipping_in_pipeline_1() -> TestResult {
+    run_test(
+        r#"[1,2,3] | #comment
+        each { |$it| $it + 2 } | # foo
+        math sum #bar"#,
+        "12",
+    )
+}
+
+#[test]
+fn comment_skipping_in_pipeline_2() -> TestResult {
+    run_test(
+        r#"[1,2,3] #comment
+        | #comment2
+        each { |$it| $it + 2 } #foo
+        | # bar
+        math sum #baz"#,
+        "12",
+    )
+}
+
+#[test]
+fn comment_skipping_in_pipeline_3() -> TestResult {
+    run_test(
+        r#"[1,2,3] | #comment
+        #comment2
+        each { |$it| $it + 2 } #foo
+        | # bar
+        #baz
+        math sum #foobar"#,
+        "12",
+    )
+}
+
 #[test]
 fn bad_var_name() -> TestResult {
     fail_test(r#"let $"foo bar" = 4"#, "can't contain")