Repository navigation
Conversation
…ick substitution stays in its word The lexer keeps the word it is reading as a pending range of the string pool. A sublexer inherits that range. The backtick arm started the sublexer without a flush, so the text before the backtick became the start of the first word inside the substitution. FOO=`echo hi` ran `hi` with FOO=echo in its environment and assigned nothing. eat_subshell now flushes the pending word itself, for each kind of subshell. The `$(` arm and the `(` arm no longer flush before the call. The token streams of `$(...)` and `( ... )` do not change.
|
Updated 7:17 PM PT - Oct 14th, 2026
✅ @robobun, your commit 1485d26b3d3d6c56fb8fed3c45a446d6ccb8f6ca passed in 🧪 To try this PR locally: bunx bun-pr 45295That installs a local version of the PR into your bun-45295 --bun |
|
Status: ready for review. How it was reproduced (Linux x64, a 1.4.3 canary release build and main 9bcbf1e):
This PR supersedes #33547, which the stale job closed. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
WalkthroughThe shell parser now flushes pending text in ChangesShell substitution parsing
Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to
|
There was a problem hiding this comment.
Beyond the inline findings, I also checked the $( and ( arms for a behavior change from moving the flush into eat_subshell: the AddDelimiter chosen per kind matches the argument each removed call site passed, all three eat_subshell callers (src/shell_parser/parse.rs:2630, :2646, :2682) are covered, and the flush now preceding the MAX_SUBSHELL_DEPTH check only alters the token list on a lex that already returns an error.
Extended reasoning...
The change is a five-line move in the shell lexer's eat_subshell plus tests; it touches no auth, crypto, or permission surface. The remaining behavior changes are the ones the inline findings cover (empty backtick body, assignment-with-redirect, and assignment status in && chains), all of which the author also lists under Downsides, so a human should weigh whether those regressions are acceptable.
Findings marked 🟡 are optional suggestions and need no follow-up push.
| // The sublexer inherits `word_start` and `j`, so pending text would start its first word. | ||
| self.break_word(match kind { | ||
| SubShellKind::Dollar | SubShellKind::Backtick => AddDelimiter::No, | ||
| SubShellKind::Normal => AddDelimiter::AfterText, | ||
| })?; |
There was a problem hiding this comment.
🔴 Scripts containing an assignment-only line such as VER=git describe 2>/dev/null now fail to parse as a whole, where on the base branch the script still ran. The new flush in eat_subshell (parse.rs:3115-3119) moves VER= out of the substitution, so the parser sees assignments followed by a Redirect token, which parse_cmd_or_assigns (parse.rs:1336-1346) cannot consume; the next statement then reports expected a command or assignment but got: "Redirect". This follows from the stated purpose and is pre-existing for X=$(cmd) 2>/dev/null and X=hi 2>/dev/null. Fix: let an assignment-only command carry a redirect (parse and discard or apply it, as bash does) so all three spellings run. [also at: src/shell_parser/parse.rs:3117 - Shell scripts that redirect a backtick assignment, like X=cmd 2>/dev/null, now fail to parse and nothing in the script runs, where the base ran the script to completion.]
Why this was flagged
Input: a shell script run via bun script.sh or bun exec containing V=cmd 2>/dev/null on any line, even in a branch that never executes. After the change, eat_subshell at src/shell_parser/parse.rs:3116 emits Text("V=") before CmdSubstBegin, so the statement lexes as an assignment followed by a Redirect token. In parse_cmd_or_assigns at src/shell_parser/parse.rs:1336-1346, parse_atom returns None on Redirect, assigns is non-empty, so it returns Assigns leaving the Redirect unconsumed; the following parse reports expected a command or assignment but got: "Redirect" and nothing in the script runs, exit 1. On the base branch the same script lexes the whole word into the substitution, runs the wrong inner command but continues, exit 0. The base already rejects the $(...) and plain spellings, but the backtick spelling is the common idiom in existing bash scripts, so the population hitting the parse failure grows with this change.
Verification: The flush at src/shell_parser/parse.rs:3115-3119 emits VER= as Text("VER=") ahead of CmdSubstBegin, so parse_simple_cmd returns Assigns at 1346 without consuming the Redirect and the next parse errors expected a command or assignment but got: "Redirect"; the whole script fails, exit 1, where the base ran it. The function at parse.rs:1336-1346 is parse_simple_cmd, not parse_cmd_or_assigns.
There was a problem hiding this comment.
This is correct. The PR lists it under Downsides and in the Notes.
- The cause is in the parser, which this PR does not change. An assignment without a command name cannot take a redirect:
parse_simple_cmdreturnsCmdOrAssigns::Assignsbeforeparse_redirect()runs, andExpr::Assignhas no field for a redirect.X=$(echo hi) 2>/dev/nullandX=hi 2>/dev/nullgive the same parse error on main. - On main the backtick spelling did not work either.
X=`echo hi` 2>/dev/null; echo "[$X]"printed[]andbun: command not found: hi, because it ranhiwithX=echo. The script only looked correct when that line was in a branch that did not run. - Support for the redirect needs a decision about its semantics, so it is not in this PR. bash and dash differ: for
X=$(ls /nonexistent) 2>/dev/null, bash prints thelserror and dash does not. Both create the file forX=hi > out.txt.
If a maintainer wants that support here, I can add it.
…he next command Three results of the change to comments, found in review: - `then # note` + newline + `else` ran `else` as a command. The body loop of `parse_if_body` now skips line breaks before it tests for the word that closes the body, so an empty body over several lines parses like `then else` on one line. - A backquote inside a backquoted comment closed the substitution, and the text after it started the first word of the next substitution. `eat_subshell` now flushes the pending word for every kind of subshell (the change of #45295). - The two closers of a command substitution share one delimiter rule.
…subshell The closing arms no longer push a stray Delimit, so `$((1+2))` would now parse as a subshell that runs `1+2`. Report arithmetic expansion as not supported instead. A backtick substitution did not flush the text before it, so the text became part of the first word of its body. Bodies that end in `;`, a newline, `)` or a JS object now parse, so flush in eat_subshell for all three kinds. This is the source change of #45295.
…he tests of #45295 No lexer site pushes a Delimit after a token that is not part of a word any more, so the arm at the head of the parse_atom loop is dead: it runs 26,963 times over 112,981 probe scripts on main and 0 times here. The arithmetic expansion error now says "yet" and gives the remedy for arithmetic first. Tests: the tests of #45295 for text before a backtick, with its empty body row updated. One decision table per file in place of the form-by-body product. New rows pin each clause of the `$((` guard: `((echo a))`, an escaped paren on each side, and the forms that stay a subshell. Three todo rows name results that are wrong for a reason another change owns.
Problem
FOO=`echo hi`; echo "[$FOO]"prints[]andbun: command not found: hi.$(...)is correct.Lexer::lex(src/shell_parser/parse.rs:2630) callseat_subshellwith the word pending. The sublexer inherits it, so its first word isFOO=echo.Fix
eat_subshellflushes the pending word:AddDelimiter::Nofor$(and backticks,AddDelimiter::AfterTextfor(. TheNoforBacktickis the fixing line.$(and(arms lose their flush. That is a move: 0 of 172,474 dumped inputs without a backtick change.test/js/bun/shell/{lex,exec}.test.ts,test/cli/run/run-shell.test.ts. 31 new cases fail without the change. Also rantest/js/bun/shell/.Background
break_wordemits it as a token (a flush).$(...), backticks and(...). Onlyeat_subshellcreates one.Downsides
X=``) now fails to parse, likeX=$(). So does a redirect after an assignment. Before, both exited 0.V=`cmd` && restnow runsrest, as$(...)does.break_wordcall, +39 instructions.$(: +0.(: at most +2. Allocations, stripped size: +0.Notes
The contract. Text before an opening backtick stays in its word. The substitution body is lexed as before. This PR does not make each backtick form equal to bash (see "Not in this PR").
Symptoms on main 9bcbf1e. The exit code is 0 or 1. It depends on the body.
With this PR, and in bash and dash, the four scripts print
[hi],[hi],prempostand nothing.No issue reports this bug. It shows in a comparison of Bun Shell with bash and dash.
Tokens (
shellInternals.lex)For these two scripts, the PR tokens are the tokens that the
$(...)spelling gives on main.Interpolated values in
Bun.$.handle_js_string_refleaves an interpolated value pending in the word. On main, a value directly before a{ raw }backtick becomes the command word of the substitution:docs/runtime/shell.mdx:567says that interpolated values are literal strings. Theafter an interpolated valuetest covers this.The move changes nothing else. A script dumped
shellInternals.lexandshellInternals.parsefor 456,177 inputs, each with the ASCII lexer and the WTF-8 lexer, on debug builds of main and of this PR.a ` $ ( ) " ' = ;and space, every string of length 6 and 7 overa ` $ ( ) ", a matrix of 18,351 opener shapes, 84 sources from the shell tests, and 40 rows written by hand.CmdSubstBegin. When the whole token moves, theDelimitafter it goes away.expected a command or assignment but got: "CmdSubstEnd". The substitution body holds no command when the text is outside it: 3,397 have an empty or blank body and 389 have a body that is one unclosed".&&. Those two cases are in the next block.Behaviour changes (main → this PR, through
bun exec)Lexer::lexpush aDelimitwhen the last token isCmdSubstBegin, and the parser then expects a command.X=$()andecho ``give this error on main. bash accepts an empty substitution.X=$(echo hi) 2>/dev/nullandX=hi 2>/dev/nullgive this error on main. bash accepts them. A redirect inside the backticks works.V=$(nosuchcmd -r x) && echo "tag [$V]"printstag []on main too. bash stops with 127. On main the backtick spelling stopped only because it ran the wrong command.echo "`touch f` now"createsfon main. Single quotes or\`keep a backtick literal.Cost. Release builds of main 9bcbf1e and of this PR, one
shellInternals.lexcall per run. "Instructions" are the instructions executed insidebun_shell_parserfunctions, counted with gdb single-step. The counts repeat exactly.break_wordcallsgrow_tocallsmi_*callsecho hello worldecho $FOO barecho $(echo m)postecho pre$(echo m)post(echo m)echo a;(echo m)echo `echo m`postecho pre`echo m`postFOO=`echo hi`; echo "[$FOO]"lexx2,eat_subshellx2,break_word, fromllvm-nm -S): 16,910 → 16,862 bytes.break_wordcall sites: 54 → 54. Stripped binary: 105,604,840 → 105,604,840 bytes.valgrind,perf,bloatyandstraceare not installed where this ran.Relation to open shell PRs
break_word, give$((...))its own error #38479 changes the two closing arms and has a fix for the empty substitution. It conflicts with main.DelimitafterCmdSubstBegin, so the empty substitution stays a parse error.git apply --checkgives the same result on main and on this branch. Shell: handle CRLF, tab, comment, and backslash-newline line endings in the lexer #29670, shell: keep words after a redirection as arguments of the same command #34901, shell: break words on a tab in the lexer #43861, shell: fail the parse when for/while/until/select/case/esac/function/do/done is used as a command #43921, shell: keep a digit in its word before>or<, end the word after**, and reject<<#44532 and shell: record the last command substitution status per command and expand words first #45074 are among them, and they apply on both.Not in this PR (the same on main, different from bash)
Other checks
bun run rust:mordant: exit 0.cargo clippy -p bun_shell_parser --no-deps: no new warning.lex.test.ts, 2 inexec.test.tsand 1 inrun-shell.test.ts. They fail on a debug build of main. 4 more new cases pin the(tokens and a backtick at the start of a word. They pass on both builds.lex.test.ts75 pass,exec.test.ts21 pass,run-shell.test.ts4 pass,parse.test.ts18 pass,bunshell.test.ts450 pass.bunshell.test.tshas one group (stdin redirect from a zero-length buffer) that fails in some runs of the debug build, with and without the change.test/js/bun/shell/directory on the debug build: 29 tests fail. 28 of the 29 also fail on a debug build of main on the same machine. The other one (ls > recursive > node_modules) passes alone on both builds.no test proof · iteration 0 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/cli/run/run-shell.test.ts