Repository navigation
Conversation
The comment scanner consumed the newline but never emitted the Newline token. The next line's words were appended to the current command. A backslash inside a comment is literal and does not continue the line.
|
Warning Review limit reached
On-demand reviews are free for the next 15 days. After that, they cost $0.25 per reviewed file. Or wait 15 minutes for your next included review. View limit detailsLimit details: You’ve used all 10 included reviews currently available. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (3)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| if peeked.char == u32::from(b'\n') { | ||
| while let Some(c) = self.eat() { | ||
| if c.char == u32::from(b'\n') { | ||
| self.tokens.push(Token::Newline); |
There was a problem hiding this comment.
🔴 Emitting Token::Newline from eat_comment() regresses scripts that put a trailing comment between an operator and its right operand: cmd1 && # note\ncmd2 (and ||, |) parsed and ran on the base branch because the swallowed newline left the operator adjacent to cmd2, but now the parser sees DoubleAmpersand, Newline and parse_simple_cmd fails with "expected a command or assignment"; bash accepts this idiom. Fix: after consuming &&/|| in parse_binary() (line 875) and | in parse_pipeline() (line 908), call self.skip_newlines() so a newline — from a comment or bare — before the right operand is accepted.
Extended reasoning...
Base: echo a && # x\necho b lexes to […, DoubleAmpersand, Text:echo, Delimit, Text:b, …] because the old eat_comment consumed the \n without emitting a token; parse_binary (873) matches && at 875 and parse_pipeline immediately parses echo b — the script runs echo a && echo b, matching bash. After this change the same input lexes to […, DoubleAmpersand, Newline, Text:echo, …]; parse_binary consumes && and calls parse_pipeline → parse_compound_cmd → parse_simple_cmd, whose while-condition at 1302 sees peek()==Newline so assigns stays empty, then at_end at 1328 is true, and line 1332 raises expected a command or assignment. || shares the same path; | hits the identical failure via the loop at 908 → parse_compound_cmd. The parser has no skip_newlines() after these operators (unlike the top-level script loop at 825), so any script using the common bash pattern of annotating pipeline stages (cat f | # source\ngrep x) that ran on 1.4.3 now throws a parse error.
Verification: normal — merging breaks input that works on base and matches bash. Trace on HEAD for echo a && # x\necho b: - Lexer at parse.rs:2536 calls eat_comment(), which now pushes Token::Newline (parse.rs:3478). Token stream becomes …, DoubleAmpersand, Newline, Text("echo"), …. - parse_binary (parse.rs:875) consumes DoubleAmpersand and immediately calls parse_pipeline() (parse.rs:885)…
| TestBuilder.command /* sh */ `echo one # note \\ | ||
| echo two` | ||
| .stdout("one\ntwo\n") | ||
| .runAsTest("backslash inside a comment does not continue the line"); |
There was a problem hiding this comment.
🟡 (optional) This test does not exercise the backslash-newline path it names: Bun.$ reads TemplateStringsArray.raw, so \\ in the template reaches the shell as two backslashes followed by an unescaped LF, and eat_comment sees {char:'\\',escaped:true} then an unescaped '\n' — the same path as the first test. Reverting only the c.escaped removal in eat_comment (keeping the Newline push) would still let this test pass. Fix: send a single backslash before the newline so the lexer sees an escaped LF — write a lone \ before the literal line break (raw templates preserve \<LF> as backslash+LF), or pass the script via { raw: ["echo one # note \\\necho two"] }.
Extended reasoning...
BunShell in src/js/builtins/shell.ts calls createParsedShellScript(first.raw, ...), so the template's raw text is what the lexer parses. The source characters \\ are two backslashes in .raw; the following literal newline is an unescaped LF. In ShellCharIter::read_char the first \ peeks the second and yields {char:'\\', escaped:true} (consuming both), then the LF yields {char:'\n', escaped:false}. eat_comment therefore hits the plain-newline branch — identical to the "trailing comment ends the statement" case above — and never touches the code path guarded by the removed if peeked.escaped { continue; }. The lex.test.ts case at line 861 (a cooked JS string "... \\\n ..." → one backslash + LF) does cover it, but this end-to-end case duplicates the first bunshell case instead of covering the escaped-newline behavior its name claims, so a regression that re-introduces \-newline continuation inside comments would not be caught by bunshell.test.ts.
Verification: nit — the analysis is correct; this end-to-end case does not reach the escaped-newline branch it names, though the same path IS covered at the lexer level. Chain of evidence: 1. TestBuilder forwards the TemplateStringsArray unchanged to Bun.$: test/js/bun/shell/test_builder.ts:40 this._scriptStr = _scriptStr; and :243 Bun.$(this._scriptStr, ...). 2. Bun.$ parses .raw, not the cooked…
|
Two inputs work on main only because the comment swallows its newline. With this change alone they fail: # 1. a comment after && or |
echo a && # note
echo b
# main: prints a, b. this PR: expected a command or assignment
# 2. a comment before a closing backtick
echo `echo a # note
`
# main: prints a. this PR: expected a command or assignment but got: "CmdSubstEnd"
#34901 includes this branch, because its statement rule needs the |
This branch has not been deployed
2 participants
Problem
#comment that follows a command on the same line swallows the newline. The words on the next line are appended to the current command.rm -rf build # cleanfollowed bycp -R src buildruns the single commandrm -rf build cp -R src buildand deletessrc.eat_commentinsrc/shell_parser/parse.rs:3472. It consumes characters up to and including the\nbut never pushesToken::Newline. The lexer then continues the same simple command on the next line. A comment on its own line is not affected because the newline before it already delimited the statement.Fix
eat_commentpushesToken::Newlinewhen it consumes the newline. The next line starts a new statement, like an unquoted newline outside a comment.# note \followed by a newline ends the comment too.test/js/bun/shell/lex.test.ts(5 new token cases, 4 fail on 1.4.3) andtest/js/bun/shell/bunshell.test.ts(3 new cases, all fail on 1.4.3). Alsoparse.test.ts,shell-seq-condexpr.test.ts,bunshell-file.test.ts.Background
Token::NewlineandToken::Semicolonend a statement.Token::Delimitends a word.#starts a comment only when it is preceded by whitespace and is outside quotes. The word before the#was already delimited by that whitespace, so the comment site only needs the statement delimiter.jis not advanced by comment characters, soword_startstays correct after the comment.Notes
Repro on 1.4.3:
Before:
"one echo two\n". After:"one\ntwo\n". CRLF line endings behave the same.Consecutive
Newlinetokens (a comment on its own line now yields two) are already handled:parse_stmtreturns an empty statement and the script loop skips newlines.