skip comments and eols while parsing pipeline (#10149)

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

- 🟢 `toolkit fmt`
- 🟢 `toolkit clippy`
- 🟢 `toolkit test`
- 🟢 `toolkit test stdlib`

# After Submitting

None

---------

Co-authored-by: Horasal <horsal@horsal.dev>
This commit is contained in:
Horasal 2023-08-31 03:24:13 +09:00 committed by GitHub
parent 3fd1a26ec0
commit b943cbedff
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
2 changed files with 58 additions and 3 deletions

View File

@ -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>) { pub fn lite_parse(tokens: &[Token]) -> (LiteBlock, Option<ParseError>) {
let mut block = LiteBlock::new(); let mut block = LiteBlock::new();
let mut curr_pipeline = LitePipeline::new(); let mut curr_pipeline = LitePipeline::new();
@ -203,7 +217,7 @@ pub fn lite_parse(tokens: &[Token]) -> (LiteBlock, Option<ParseError>) {
let mut error = None; let mut error = None;
for token in tokens.iter() { for (idx, token) in tokens.iter().enumerate() {
match &token.contents { match &token.contents {
TokenContents::PipePipe => { TokenContents::PipePipe => {
error = error.or(Some(ParseError::ShellOrOr(token.span))); 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); last_connector_span = Some(token.span);
} }
TokenContents::Eol => { 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() { if !curr_command.is_empty() {
match last_connector { match last_connector {
@ -451,7 +471,7 @@ pub fn lite_parse(tokens: &[Token]) -> (LiteBlock, Option<ParseError>) {
block.push(curr_pipeline); block.push(curr_pipeline);
} }
if last_token == TokenContents::Pipe { if last_non_comment_token(tokens, tokens.len()) == Some(TokenContents::Pipe) {
( (
block, block,
Some(ParseError::UnexpectedEof( Some(ParseError::UnexpectedEof(

View File

@ -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] #[test]
fn bad_var_name() -> TestResult { fn bad_var_name() -> TestResult {
fail_test(r#"let $"foo bar" = 4"#, "can't contain") fail_test(r#"let $"foo bar" = 4"#, "can't contain")