Repository navigation
Conversation
…nd command After a compound command (an if clause, a subshell, a [[ ]] test) the parser started another command of the same statement, so the word that closes or continues the enclosing if clause became a command name. parse_stmt now gets the words that the enclosing clause waits for from parse_if_body. One of them ends the statement when it follows a compound command. parse_if_clause consumes the word as before.
|
Updated 12:19 PM PT - Oct 21st, 2026
✅ @robobun, your commit 58975ee3654d9b23b2680905cb14098fc7f8e65d passed in 🧪 To try this PR locally: bunx bun-pr 45904That installs a local version of the PR into your bun-45904 --bun |
|
Status: the fix is in this PR (#45904). How to reproduce on main, with bash for comparison: printf 'if false; then (echo a) else echo b; fi\n' > a.sh
bun a.sh # prints nothing, exit 0. bash prints "b"
printf 'if true; then if true; then echo hi; fi fi\n' > b.sh
bun b.sh # error: Failed to run b.sh due to error Expected "else", "elif", or "fi" but got: Eof. bash prints "hi"With this PR both scripts print what bash prints. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
WalkthroughThe parser now stops before an enclosing ChangesIf-clause terminator handling
Suggested reviewers: Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation |
|---|---|---|
| Title check |  |
The title clearly and concisely describes the main parser change: ending statements at if-clause words that follow compound commands. |
| Linked Issues check | |
Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | |
Check skipped because no linked issues were found for this pull request. |
| Description check | |
The pull request description is complete and relevant. It explains the problem, cause, fix, scope, limitations, performance impact, and verification results. It does not use the template headings exac… |
Full details: Description check
-
Explanation
The pull request description is complete and relevant. It explains the problem, cause, fix, scope, limitations, performance impact, and verification results. It does not use the template headings exactly, but it provides the required information in equivalent sections.
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline finding, I also checked two paths: a stray clause word after a compound command inside a nested ( ) or $( ) still runs as a command because parse_impl passes &[], but an if nested inside that subshell still forwards its own until through parse_if_body, so only the shapes bash itself rejects are affected; and interpolated ${...} values lex as non-Text tokens, so peek_any_ifclausetok cannot treat them as fi/else, which the new bunshell.test.ts case pins. parse_stmt has only the two shell-parser callers, both updated.
Extended reasoning...
The diff adds Expr::ends_in_compound_cmd and a single early break in parse_stmt of src/shell_parser/parse.rs, threaded via a new until parameter from parse_if_body, plus tests in three files; no security-sensitive surface beyond the shell parser consuming user script text, and the new break only ever shortens a statement at a keyword the enclosing clause already waits for. One inline finding concerns pre-existing lexer handling of trailing comments that the new comment tests lean on rather than the fix itself, so a human should weigh whether those cases belong in this PR.
…l-if-keyword-after-compound # Conflicts: # src/shell_parser/parse.rs
There was a problem hiding this comment.
I re-reviewed the current revision and found no bugs. Because this changes which Bun Shell scripts parse (and how a few bash-rejected scripts now behave, per the Notes), a maintainer's look at the semantics is still worthwhile before merging.
What was reviewed:
- The new
parse_stmtbreak: it fires only when the previousExprends in a subshell/if/[[ ]]and the next token passes the samepeek_any_ifclausetokpredicateparse_if_bodyalready uses (plainToken::Text, delimited, not interpolated), so"fi",fi$Xand${"fi"}stay words; top-levelparsepasses&[], so nothing changes outside anifclause. - Token flow after an inner
fi/): the lexer emitsDelimitbefore;/newline andmatch_if_clausetokconsumes only theDelimit, so the existing separator-based exit still runs; the new break only matters when no separator token remains (none written, or a trailing#comment ate the newline). ends_in_compound_cmdis exhaustive overExprandPipelineItem, and everyparse_stmtcaller received theuntilparameter.- Tests: the pair matrix compares against the
;-spelled AST (which fails if either side throws), the negative cases go through a helper that returnsundefinedon success, and the run-shell subprocess test drains stdout/stderr/exit concurrently.
Extended reasoning...
The diff adds 24 lines to src/shell_parser/parse.rs (an until: &[IfClauseTok] parameter on parse_stmt, one early break in its expression loop, and an exhaustive Expr::ends_in_compound_cmd helper) plus about 220 lines of tests across parse.test.ts, bunshell.test.ts and run-shell.test.ts. It touches no security-sensitive surface; the parser error paths remain recoverable ParseErrors with no new unwrap/panic. Both prior inline threads were author-resolved after follow-up commits; I re-checked the .error() case and it can fail either way (stdout mismatch if the clause closes, thrown ShellError if fi runs as a command), and the comment-newline issue is pre-existing and out of scope. The decision to defer rather than approve rests on this being a grammar-semantics change with author-documented behavioral tradeoffs for bash-invalid scripts, which a maintainer should weigh; no CODEOWNER covers these paths and the hunt ran dry with no findings.
Problem
if false; then (echo a) else echo b; fiprints nothing and exits 0 (bash printsb). Afterfi,)or]], the nextthen,elif,elseorfiruns as a command or fails the parse:Expected "else", "elif", or "fi" but got: Eof.Parser::parse_stmt(src/shell_parser/parse.rs:844) starts another command there.Fix
parse_if_bodypasses the words its clause waits for toparse_stmt. One of them ends the statement afterfi,)or]].parse_if_bodystops. bash 5.2 accepts each test script.test/js/bun/shell/parse.test.ts(170 new cases fail on main),bunshell.test.ts,test/cli/run/run-shell.test.ts, all oftest/js/bun/shell/.Background
echo hi fi).)orfi(shell: end a comment before its newline, accept a line break after&&,||,|and inside[[ ]]#41468, Notes).Downsides
ifbody, of about 1,900), no allocation..text: +256 bytes.Notes
Before and after. Release builds of main (017b243) and of this PR. bash 5.2.37 gives the result of the last column for each valid script. dash does too, except for the
[[ ]]row: dash has no[[.if false; then (echo a) else echo b; fibif true; then (echo a) else⏎ echo b; fia,bun: command not found: else,baif true; then if true; then echo hi; fi else echo no; fihi,bun: command not found: else, exit 1hiif true; then if true; then echo hi; fi fiExpected "else", "elif", or "fi" but got: Eofhiif true; then (echo hi) fihiif (true) then echo yes; fiExpected "then" but got: Eofyesif [[ a == a ]] then echo yes; fiExpected "then" but got: Eofyesif true; then⏎ if true; then⏎ echo x⏎ fi # inner⏎fiExpected "else", "elif", or "fi" but got: Eofxif false; then⏎ if true; then⏎ echo a⏎ fi # inner⏎else⏎ echo b⏎fibif true; then echo hi fifiis an argument ofecho)(echo hi) fihi, thenbun: command not found: fiWhat changes, by class. The probe corpus has 45 scripts that bash accepts with a word of the clause directly after a compound command, 21 scripts that work on main, and 51 scripts that bash rejects. Each script went through
shellInternals.parseon both builds.Wrong result on main: 7 of the 45. 4 skip the
elseorelifbranch with exit 0 and no message. 3 runelseas a command. All 7 now parse to the tree of their spelling with;.Parse error on main for a valid script: 38 of the 45. All 38 now parse to the tree of their spelling with
;.No change for a script that works: 21 of 21 parse to the same tree. The 633 script templates without interpolation in
test/js/bun/shell/andtest/cli/run/run-shell.test.tson main give the same parse result. So do 11,886 shell sources on the build host (1,135 shell files and 10,751package.jsonscripts).A script that bash rejects and that exited 0 on main now fails. Two kinds:
if false; then (echo a) elif true; fi. On mainelif truewas a command in a branch that did not run. It is now a parse error:Expected "then" but got: Eof.if false; then (echo b) fi; fi. On main the firstfiwas a command in a branch that did not run. Now the firstficloses the clause and the second one runs:bun: command not found: fi, exit 1.A script that bash rejects and that now parses. Main rejects 26 of the 51 and this PR rejects 19. The 7:
if true; then (echo a) (echo b) fi,if true; then echo a [[ b == b ]] fiandif true; then echo a > /dev/null (echo b) fi. These are two commands with no separator. At top level main runs both commands. bash rejects them by the statement rule (below).if true; then (echo hi) fi fi,if true; then ( (echo a) fi ) fiandif true; then (echo a) else (echo b) elif true; then echo c; fi. A stray word of a clause runs as a command, as it does at the start of a statement on main. shell: reject!, brace groups, here-documents and stray then/elif/else/fi at parse time #43922 makes it a parse error.if true; then (echo hi) \fi. The lexer drops the backslash, so\fiisfiin each position on main. shell: fail the parse when for/while/until/select/case/esac/function/do/done is used as a command #43921 records the escaped characters.if (true) then fiandif (true) then (echo a) else fiwere parse errors and now run. Main accepts an empty body in the spelling with;(if (true); then fi).A valid script that was a parse error on main and that now runs into another open bug. The parse error hid that bug. Main does the same for the spelling with
;. Two examples:if (( 11 > 10 )) then echo yes; fi. Bun Shell has no(( )). The script printsbun: command not found: 11, takes no branch and exits 0.if (true) then⏎echo a # c⏎echo b⏎fi. The comment swallows its newline (shell: end a comment before its newline, accept a line break after&&,||,|and inside[[ ]]#41468). The script printsa echo bwith exit 0 and no message. bash printsaandb.A valid script that parses on main and is a parse error here. It needs two things in one clause: a word of the clause directly after a compound command, and a comment after a simple command on the line before
thenorfi(the bug of shell: end a comment before its newline, accept a line break after&&,||,|and inside[[ ]]#41468). On main the two bugs cancel:main and bash print
z. On mainelifis a command in a branch that does not run, andthenis an argument offalse. Hereelifis the word of the clause,thenis still an argument offalse, and the parse fails withExpected "then" but got: Eof.Reach. No GitHub issue exists for this bug, and the error text appears in no issue. No local shell source changes (above). The reason to change it without a report is the first row of the table: a valid script takes the wrong branch with no message.
The statement rule. In bash a statement holds one command: a second word after
fi,)or]]is a syntax error. Theparse_stmtof Bun Shell loops and runs that word as a second command. #34901 had a rewrite ofparse_stmtwith the bash rule. It removed the rule on 2026-10-20 and keeps the word loop ofparse_simple_cmd, which makes the words after a redirection arguments.The rule stops valid scripts today. In
(echo a) # note⏎echo bthe comment swallows its newline, soecho bfollows)with no separator. Main and this PR run both commands, as bash does. The rule needs the lexer change of #41468 or #29670 first, so it is not in this PR.When the rule comes, this change is one condition on it:
if !self.match_stmt_end() && !self.peek_any_ifclausetok(until), with theuntilparameter fromparse_if_body.Expr::ends_in_compound_cmdcan go whenparse_simple_cmdtakes each word after a redirection (the word loop of #34901).Order with #43922. #43922 turns a stray
then,elif,elseorfiat the start of a command intoUnexpected token. By a read of that branch, without this PR it rejects all 45 valid scripts, and the 7 wrong results of main become parse errors. With this PR the statement ends beforeparse_compound_cmdsees the word, so the two compose with no code change.Conformance cases in the tree.
bunshell.test.tshas six.todocases from posix-shell-testsif.tst("nest between ..."). They test this rule with{ }and!, which Bun Shell does not have, so they stay.todo. Their( )forms are in the new tests.Tests.
parse.test.ts: 150 generated pairs (10 positions of a clause, 3 compound commands, 5 list shapes) and 19 pairs written by hand. Each pair compares the tree of a script with the tree of its spelling with;. 13 scripts must fail with an exact message. 2 tests pin trees.bunshell.test.ts: 20 scripts run through$. One more pins an interpolatedfi.run-shell.test.ts: one script throughbun script.sh.first()orany()forlast(), each arm ofends_in_compound_cmdwidened or narrowed, and each wrong set of words fromparse_implandparse_if_body.fiis a plain word,echo hi fiis one command, and a word that no clause waits for does not end the statement.Measurements. Release builds of main (017b243) and of this PR.
perf,valgrind,straceandbloatyare not available in the build container. The instruction, branch and allocator counts come from a ptrace single-step of the main thread, from the entry of the firstParser::parse_implcall to its return. Each script ends in an unclosedif true; then, so the parse fails and no command runs. A per-statement number is the count for 200 statements minus the count for 100 statements, divided by 100.echo a b cifbodyifbodymi_heap_malloccalls forecho a; echo b; echo c,if true; then echo a; fi,if true; then (echo a); fiand a 20-line scriptmi_heap_malloccalls forif true; then (echo a) fi;if_clause_tok_atfor the same four scripts (debug counter)if_clause_tok_atforif true; then (echo a) fi.textof the stripped binary (llvm-size -A)parse_stmt,parse_if_body,peek_any_ifclausetokin bytes (llvm-nm -S)26 of the 36 instructions in an
ifbody are a call.peek_any_ifclausetoknow has two callers, so the compiler no longer inlines it into the loop ofparse_if_body. The same test written withIfClauseTok::from_tokkeeps it inlined and measures +13 instructions and +1 branch for each kind of statement. This PR keeps the one shared predicate.The new test ran once in the 515 parses of
parse.test.ts,bunshell.test.tsandlex.test.tsas they are on main (debug counter). The script isFOO="" [[ -n $FOO ]] && echo yes!, a statement of two commands.Considered and not taken.
Not changed here.
if true; then echo a; fi echo afterprintsaandafter. No open PR changes that: it is the statement rule above.echo hi # note⏎fipassesfitoecho. shell: end a comment before its newline, accept a line break after&&,||,|and inside[[ ]]#41468 owns that. This PR only helps a comment afterfi,)or]].Suites. Debug (ASAN) build of the branch with main (9634520) merged:
parse.test.ts,bunshell.test.ts,lex.test.ts,exec.test.tsandtest/cli/run/run-shell.test.tspass. That merge brings the stack check of #46021 intoparse_stmt. The measurements above are from before the merge and were not repeated. Release build of this PR before the merge: the 44 files oftest/js/bun/shell/andrun-shell.test.tspass. On the build host, some tests that start a debug binary or delete a large tree time out in one run. They pass when run again.no test proof · iteration 0 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/bun/shell/bunshell.test.ts, test/cli/run/run-shell.test.ts