Repository navigation
Conversation
|
Reproduced on 1.4.3-canary.1+367d939d9 and on main at 6d504dd (Linux x64). The line before printf 'import { $ } from "bun";\n$.throws(false);\nconst r = await $`if true; then\n\techo tabbed\nfi`.quiet();\nconsole.log(JSON.stringify(r.stdout.toString()), JSON.stringify(r.stderr.toString()), r.exitCode);\n' > tab.ts && bun tab.ts
PR: #43861. It is stacked on #43922, which is stacked on #43921. |
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: oven-sh/bun/.coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan provides up to 10 included reviews per hour; 2 remain after this review. WalkthroughThe shell lexer now treats unquoted tabs as word delimiters, like spaces. New tests cover tab handling in lexer input and shell scripts, including cases where tabs remain literal inside quotes or after a backslash. ChangesShell tab delimiters
Suggested reviewers: Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to The tab-delimiter change appears ready to merge after normal checks. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
There was a problem hiding this comment.
Beyond the inline findings, I also checked: quoted and backslash-escaped tabs stay literal because the arm is gated on CharState::Normal (covered by the three literal lex cases); the ASCII fast path already stops at a tab since SPECIAL_CHARS_TABLE sets it, and the new const assert pins that; and the behaviour change for an unquoted tab used as data (cut -d<TAB>) matches bash and is called out in the description, so I did not flag it.
Extended reasoning...
One-line lexer change in src/shell_parser/parse.rs widening the word-break guard from space to space-or-tab, plus lex and end-to-end shell tests; no security-sensitive surface. The inline findings concern pre-existing parser bugs (trailing-comment newline, unsupported for-loops) that tab-indented scripts now reach rather than a defect in the new arm itself.
|
Updated 8:30 PM PT - Sep 24th, 2026
✅ @robobun, your commit ba1b441d09bf3fc338a460893125e2752efd7702 passed in 🧪 To try this PR locally: bunx bun-pr 43861That installs a local version of the PR into your bun-43861 --bun |
0dc1a0a to
e2d9ce5
Compare
61e9254 to
fbb1dae
Compare
e2d9ce5 to
1225bcd
Compare
There was a problem hiding this comment.
LGTM — the rebase onto the reserved-word parse failures closes the body-execution concern from the earlier review, and the lexer change itself is a two-line extension of the space arm.
What was reviewed:
- The guard at src/shell_parser/parse.rs:2931 sits inside the
!escapedbranch, so\<TAB>and quoted tabs still reachappend_char_to_str_pool;b'\t'is inSPECIAL_CHARS(line 3957), so the new const-assert holds and the ASCII fast path already stopped at tabs. - Checked the other space-only sites (
[[/]]at 2522/2568 already accept\t;eat_varends a name on any non-identifier char) — nothing left inconsistent. - Interpolated
${"a\tb"}goes through the JS-string-ref path before the match, so the existing assertion at bunshell.test.ts:188 is unaffected; the new tests pin that tab-indentedfor/while/!/brace/heredoc bodies fail the parse instead of running.
Extended reasoning...
The change adds b'\t' to the word-breaker arm of Lexer::lex in src/shell_parser/parse.rs plus a matching compile-time assert, and adds lex and TestBuilder tests covering tabs between words, around operators and redirects, in $(...), in quotes, and after a backslash. It touches no security-sensitive surface. The earlier red finding (tab-indented loop bodies running with an empty variable) is addressed by stacking on the commits that fail the parse on unsupported reserved words, and new tests assert that outcome. The diff is small, mirrors the existing arm exactly, and the behaviour change (unquoted tab splits a word, as in bash) is stated in the PR; no CODEOWNER covers the file.
fbb1dae to
b29d9fc
Compare
1225bcd to
69a47a3
Compare
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.
LGTM — the rebase onto the reserved-word rejection resolves the earlier concern about tab-indented loop bodies executing, and the remaining inline comment is a test nit only.
Extended reasoning...
The change adds b'\t' to the single word-breaker arm of Lexer::lex in src/shell_parser/parse.rs (one line plus a compile-time table assertion), with lexer and end-to-end tests covering tabs between words, at line boundaries, around operators/redirects, inside $(...), and the literal cases in quotes and after a backslash. It touches no security-sensitive surface; the shell lexer is parsing user-authored scripts and the new behavior matches POSIX/bash. I checked the other space-only site in the same file (the variable-name terminator list at ~line 3518) and it already stops on any non-identifier byte, so a tab terminates a $NAME there without needing the change; the other whitespace sites at 2535-2584 and 3747 already include tab. The prior flagged risk (for/while bodies running once with an empty variable) is now closed by the stacked base rejecting those reserved words, which the new test.each pins. The one surviving finding is a redundant here-document test row, not a correctness issue.
b29d9fc to
440ca75
Compare
69a47a3 to
28d317d
Compare
There was a problem hiding this comment.
LGTM — a one-line lexer change that makes an unquoted tab a word breaker like a space, with the here-document row from the earlier nit now dropped.
What was reviewed:
- The new arm at src/shell_parser/parse.rs:2947 only breaks the word in
CharState::Normal; quoted and backslash-escaped tabs still fall through toappend_char_to_str_pool, and the sibling[[/]]peek arms andis_whitespacealready treated a tab as blank, so no other lexer site needed the same change. - Checked that the
for/while/!/{rows in the newtest.eachonly reach the reserved-word error via the tab split (base commits 440ca75/fe3bfa63 supply the rejection), so each row fails without the lexer change. - Grepped the shell test suite for existing tab assertions: the only ones (bunshell.test.ts:185-189) cover an interpolated
${"a\tb"}value, which is escaped before lexing and is unaffected.
Extended reasoning...
The PR changes one match arm in the Bun shell lexer (src/shell_parser/parse.rs) so b'\t' joins b' ' as a word breaker, adds a compile-time assert that tab is in SPECIAL_CHARS_TABLE, and adds 16 lex-level and 11 behavioral tests across test/js/bun/shell/lex.test.ts and bunshell.test.ts. It touches no security-sensitive surface beyond the shell lexer itself, and the behavior it introduces matches POSIX token recognition and bash. The production change is a single guard condition whose quoting/escaping guards are the pre-existing ones for space, the tests assert exact token streams and exact stdout, the changed path is not covered by CODEOWNERS, and the latest commit removed the only test row flagged in the prior review, which decided approve over defer.
440ca75 to
70b1dac
Compare
28d317d to
d96a671
Compare
There was a problem hiding this comment.
LGTM — the one-line lexer change is correct and the concern from my earlier review is now covered by the base commits.
What was reviewed:
- The word-breaker arm at src/shell_parser/parse.rs:2940 is the only site that accepted a space but not a tab; the
[[/]]detection (:2534, :2580),is_whitespace(:3746) andSPECIAL_CHARS(:3972) already include\t, so the fast-path scan and comment detection agree with the new arm. - Backslash-escaped tabs take the
escapedbranch and stay literal; quoted tabs are gated byCharState::Normal— both pinned by the new lex and end-to-end cases. - Tab-indented
for/while/!/{bodies now fail the parse via the reserved-word table at :1990 (landed in the two base commits), and the newtest.eachin bunshell.test.ts asserts the exact message; the previously non-failing<<-EOFrow was dropped.
Extended reasoning...
The diff adds b'\t' to the unquoted word-breaker guard in the Bun Shell lexer (src/shell_parser/parse.rs, 2 lines) and adds lexer equality-vs-space and end-to-end execution tests in test/js/bun/shell/lex.test.ts and bunshell.test.ts. It touches no security-sensitive surface; the shell lexer is user-input parsing but the change only widens a whitespace check to match POSIX and the existing sibling tab sites. Approve was decided by the change being a single-guard fix with every sibling whitespace site already consistent, the earlier blocking concern (loop bodies running with an empty variable) being closed by the reserved-word rejection now on the base branch and pinned by the new tests, and no CODEOWNER covering the changed paths. The documented behaviour change (an unquoted tab inside a word now splits, as in bash) is intentional and stated in the PR.
70b1dac to
2e45e47
Compare
d96a671 to
9237af7
Compare
The lexer's word-break arm matched only a space. An unquoted tab fell through to the default arm and joined the current word, so a tab-indented script failed with "command not found: \techo" and `echo a<TAB>b` passed one argument. The arm now matches a tab too. A tab inside quotes, or after a backslash, stays a literal character.
2e45e47 to
cee8742
Compare
9237af7 to
ba1b441
Compare
Stacked on #43922 (which is stacked on #43921). They fail the parse for the constructs that Bun Shell does not support, so a tab-indented body of one cannot run.
Problem
if true; then\n\techo tabbed\nfiprintsbun: command not found: \techoand exits 1.echo a<TAB>bpasses one argument.Lexer::lex(src/shell_parser/parse.rs) matches onlyb' '. An unquoted tab joins the word.Fix
b'\t'too. A quoted or backslash-escaped tab stays literal.test/js/bun/shell/lex.test.ts(13 of 16 new cases fail on the base branch),bunshell.test.ts(10 of 11).Background
SPECIAL_CHARS_TABLEhas it. Only the match arm was missing.${"a\tb"}) is data. Its tab stays literal.Downsides
cut -d<TAB> -f2) now splits it, as in bash. Quote the tab to keep it.Lexer::lex: +4 instructions, no branch, per space and per quoted character. Binary: +0 bytes.Notes
No GitHub issue reports this. A tab-indented
ifbody failed withcommand not found: \techo, and the search for a report found none.Cases checked against bash 5.2 (each
<T>is a real tab):if true; then\n<T>echo tabbed\nficommand not found: \techotabbedecho a<T>ba<T>ba becho a<T>|<T>catcommand not found: \tcataecho a<T>&&<T>echo bcommand not found: \techoa,bFOO=bar<T>printenv FOOcommand not found: FOObarecho a<T>><T>/dev/nullNo such file or directory: \t/dev/nullif [[<T>-n a<T>]]; then echo y; fiUnknown conditional expression operation: a\tyecho 'a<T>b' "a<T>b" a\<T>bThe interaction with the open bugs of space-indented scripts:
echo a<T># c\n<T>echo bprintsa echo b. The comment swallows the newline. shell: end the statement at the newline that ends a comment #41468 makes ita,b. The new lex casebefore a comment that ends a lineholds before and after shell: end the statement at the newline that ends a comment #41468.for d in x y; do\n<T>echo "body:$d/"\ndoneprintedbody:/once when this change stood alone. The base branch of this PR now fails the parse onfor. The same holds forwhile,!and a brace group, and the newdoes not run the body of an unsupported constructcases pin it. shell: reject!, brace groups, here-documents and stray then/elif/else/fi at parse time #43922 pins the tab-indented here-document.How the cost was measured: release builds of the base branch and of this PR, instruction counts from gdb single-stepping over
createParsedShellScriptonbun-profile(second call).valgrind,perfandstraceare not available in the build container.echo a; echo b | cat; true && echo chas 9 spaces: 72,347 to 72,383 (+36). A script with 71 characters inside quotes: 68,370 to 68,654 (+284). Both are 4 instructions per character that reaches the fall-through block of the lexer. The release binary has the same shape as a release-profile compile of the crate: two jump tables, then acmp $0x20; jneblock. That block becomescmp $32; setne; cmp $9; setne; test; jne..text+.rodata(size -A): 77,982,896 before and after.Also measured, as static counts from a release-profile compile of the crate:
lexhas 1757 instructions (ASCII) and 1679 (WTF-8) before, 1761 and 1683 with the guard. A constant pattern arm (SPACE | TAB =>) in place of the guard puts both bytes in the jump table: 1755 and 1677, with 2 instructions fewer on the fall-through path, but about 139 bytes more for the larger tables. The guard form is kept because every other arm of this match is a guard.Sites that accept a space and not a tab, and stay out of this change:
post_subshell_expansion(src/runtime/shell/states/Expansion.rs:548) does not split command substitution output on a tab:echo $(printf 'a\tb')keeps the tab. A tab split there today makes[[ -f $(printf 'my\tfile') ]]false, because[[ ]]operands are split. Bun Shell: the output of a command substitution is not split on a tab #43859 tracks both. shell: honor IFS when field-splitting command substitutions #33552 was an earlier attempt.replace_package_manager_run(src/install/lifecycle_script_runner.rs:126) rewritesnpm runtobun runonly after a space or a quote. After a tab the script still runs, through npm. It is a rewrite ofpackage.jsonscripts, not the shell..shfiles.echo a 2>&1<T>bnow behaves asecho a 2>&1 bdoes, which takesbas a file target. shell: don't consume the next word as a file target for2>&1/1>&2#34904 is open for that.no test proof · iteration 1 · 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