Repository navigation
Conversation
The lexer read only the bare form of a variable. For `${NAME}`, `eat_var`
returned an empty range. The `$` arm then appended a literal `$` and lexed
the braces as brace-expansion tokens.
`eat_var` now reads `{NAME}` or `{digit}` from the source bytes when an
unescaped `{` follows the `$`. It returns the same range as for the bare
form, so the `$` arm pushes the same `Var` or `VarArgv` token. Every other
`${...}` body consumes nothing and is lexed as before: the `$` is text and
the rest is ordinary shell syntax.
`ShellSrcBuilder::outbuf_ends_with_var_ref` also matches a script that
ends in `{NAME`. A value interpolated there goes to the lexer as a
reference, so it stays data and never names a variable.
|
Updated 9:33 PM PT - Oct 11th, 2026
✅ @robobun, your commit f1c62b9e5244e56f6658a023a9f9162b81761911 passed in 🧪 To try this PR locally: bunx bun-pr 45032That installs a local version of the PR into your bun-45032 --bun |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
WalkthroughThe shell parser now recognizes ChangesBraced Shell Variable Expansion
Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation |
|---|---|---|
| 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. |
| Title check | |
The title clearly and concisely identifies the main change: expanding ${NAME} in the shell like $NAME. |
| Description check | |
The description fully explains the problem, implementation, scope, limitations, risks, merge order, and verification results. It does not use the exact template headings, but it covers both required t… |
Full details: Description check
-
Explanation
The description fully explains the problem, implementation, scope, limitations, risks, merge order, and verification results. It does not use the exact template headings, but it covers both required topics: what the PR does and how it was verified.
- 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.
Beyond the inline finding, I also checked two things and ruled them out: eat_braced_var_name is entered after a peek() (cursor still on {), matches only ASCII bytes, and repositions via bump_cursor_ascii the same way the existing JS-ref caller does, so self.j and the byte cursor stay in step on both the Ascii and Unicode encodings; and the widened outbuf_ends_with_var_ref guard only converts a spliced value into a reference, which changes token boundaries but not output for brace groups like {${"a"},b} (covered by tests).
Extended reasoning...
The change adds a ${NAME}/${digit} arm to the shell lexer in src/shell_parser/parse.rs, widens the Bun.$ builder guard in src/runtime/shell/shell_body.rs so values after ${ or {NAME are spliced as references rather than text, and ships docs plus lexer, behavioral, positional, and bun exec/.sh tests. It touches the interpolation boundary of Bun.$ (where a user value could otherwise become a variable name), and the guard looks correct for that. An inline finding already flags the user-visible behavior change for "${X}" with X unset, which the author also acknowledges in the description, so a human should weigh that trade-off.
…ange) This branch is parked. #45490 has the `$?` and `$$` part. `${NAME}` is #45032. The two commits below this one are the earlier, larger shape. A review of them found these items. Do them before this work is proposed. Commit 1 (expansions): - `param_text_braces` goes up in any quote state and comes down only in the state of the word. `echo ${S:-"${Y%z}"} mid; echo second` prints `set` and drops the rest. Count only where the `}` arm would close the word. - A word with no `}` must be a lexer error (`Unclosed parameter expansion`), as for `$(`. Delete the rollback, the `param_unclosed` memo and the `Eof` sentinel: 896 KB of unclosed heads lex in 5.4 s. - A quoted word must stay in `CharState::Double` up to its `}`. An inner `"` only flips a bit that decides if `}` closes. `\}` is a `}` there. Today `"${X:-"$(echo p q)"}"` gives 2 arguments (dash and bash: 1), `"${X:-"it's done"}"` stays text, and `"${X:-a\}b}"` gives `a\b}` (dash: `a}b`). - `Step::EndField` next to `~`, `{a,b}` or `*`: `~/.cache${S:+ --force}` gives `[/.cache][~--force]`, `{a,b}${X:-c }` gives an extra empty argument, and `echo *.js${S:+ --flag}` fails with `no matches found: --flag`. Check the trailing break before the brace and glob passes, and apply the leading tilde to the first field. - `${X:-"" b}` loses the empty argument: keep a quoted-empty flag for each field. - `SubShellKind::ParamWord` makes `eat_subshell` hold an unreachable arm (mordant `param_wider_than_callers`): keep the word context in its own lexer field. `append_var_argv -> bool` folds a `self_exe_path()` failure into "not set" (mordant `error_collapsed_to_bool`). - `ParamHead.skip` and the `open_params` stack of the parser cost 5 instructions for each `parse_atom` call. `Expansion::next` can find the matching `ParamEnd` with a depth counter. - `if !head.op.falls_back_to_value() { out.truncate(len) }` is dead. - `ends_in_param_ref` copies the head grammar of the lexer by hand: add a generated test that a plain interpolated string and a string reference give the same non-text tokens after `$` and `${` plus each tail of up to 2 bytes of `${}?:-+a1\#!@`. `$\FOO${"BAR"}` expands `$FOOBAR` on main too. - Tests to add: `-` and `+` with a set, an empty and an unset parameter and with argument numbers; nested forms where the used and the unused word differ; the word guards (`[[`, `]]`, `#`, a digit before `>`, a quoted `}`); the table again in a script with a non-ASCII byte; a checked-in dash reference table. - With `${NAME}`, an unset `"${X}"` gives no word: `[ -d "${X}" ]` passes and `cd "${X}"` goes to `$HOME`. #44904 makes it one empty word, so it must land first. - A prefix assignment is not seen: `A=x B=${A:-y} cmd` gives `y`. Commit 2 (parse errors): no maintainer asked for it. If it returns: one line for each error, a kind on `LexError`, and the hint about `--shell=system` printed by the caller only for a package.json script. It also rejects text that runs today: `"${{ x }}"`, `"$#"`, `sh -c "echo hi $@" -- a b`.
|
#46691 has this change. It reads #46691 does not wait for #44904. An empty If #46691 merges, this PR can close. |
|
Closing in favor of #46691. It contains this change: it reads |
Problem
${NAME}.bun exec 'X=5; echo ${X}'prints${X}with exit 0. No issue reports this (found during shell: keep an empty word that has a double-quoted expansion #44904).Lexer::eat_var(src/shell_parser/parse.rs:3394) returns an empty range when{follows$, so the$arm appends a literal$.Fix
eat_varreads{NAME}or{digit}from the source bytes, so${NAME}and${0}..${9}give the tokens of$NAMEand$0..$9. Other$forms lex as before.outbuf_ends_with_var_refalso matches a script that ends in{NAME, so a value interpolated there stays data (Robustness and input-handling pass across install, shell, TLS/QUIC, HTTP/3, SQL and crypto #37669).test/js/bun/shell/lex.test.ts,bunshell.test.tsand three more files: 66 new tests fail on main. Also ran all 43 shell test files on release builds.$forms.Background
$NAMEinto aVartoken, which the parser and interpreter already expand.Bun.$splices a plain interpolated value into the shell source as text. Other values go in as a reference, which the lexer treats as data.$?,$$,$#, and${NAME}; reject unsupported expansions #33274 (its${NAME}tests are reused). Its lex error for other bodies breaks working scripts.Downsides
"${X}"withXnot set gives no word, as"$X"does on main. Then[ -d "${X}" ]is true,"${X}"/*is/*, andcd "${X}" && cmdrunscmdin$HOME. main passes the text${X}.Bun.$value, +11,841 for a value right after{,.text+1,280 bytes.Notes
Routes. Every route to the lexer has the defect:
bun exec,bun file.sh,bun runwith Bun's shell (the default on Windows), and{ raw }inBun.$. In aBun.$template literal,${...}is a JavaScript interpolation, so only{ raw }carries the braced form there. The package.json route has a test with--shell=bunand, for parity,--shell=system.Demand. No issue or discussion reports this. The defect was found by inspection during the work on #44904. Two places show it without a report: a transcript in #9716 (a comment of 2025-07-25) where
bun bun.shprints${FOO}, and the package.json of #17123 withrm -rf "${ZERO_REPLICA_FILE}"*. On this machine, 12 of 3,817 distinct package.json scripts hold${. This PR changes the tokens of 6 of them, for examplecommitlint --from=${CIRCLE_BRANCH} --to=${CIRCLE_SHA1}andgit commit -m 'update changelog - '${npm_package_version}. The other 6 use an operator body. The PR does not make sh scripts run: Bun's shell still has noset,[,whileorread.Scope: 2 of 23 forms. The PR reads
${NAME}and${digit}. These forms are not supported, before and after:${10}and above, the operator bodies (${X:-d},${X:=d},${X+d},${#X},${!X},${X%p},${X#p},${X/a/b}and their variants), and the bare special parameters$?,$$,$#,$@,$*,$!,$-. Related: #17396 ($$), not fixed here.$is text and the rest is lexed as ordinary shell syntax, on main and here:echo ${X:->dflt}creates the filedflt},echo ${X:-a;echo B}runs a second command, and${X%.*}fails withno matches found.[ -n "${X:-}" ]still fails open.${NAME}inside such a body now expands, as$NAMEthere does on main:${X:-${Y}}gives the same tokens as${X:-$Y}.Token::VarArgvholds au8andas_human_readablehas a 10-entry table, so${10}needs more than the lexer..envand.npmrcreaders have their own${...}grammars (dotenv skips the default of:-for an empty value, sh takes it), so no code is shared.lex.test.tsrecord the tokens of these forms under the title "not supported, tokens unchanged". sh expands or rejects each of them.#44904 and the merge order. This PR is a draft until #44904 lands. The safe order is #38002, then #44904, then this PR. #44904 makes a double-quoted expansion that is empty give one empty word. It waits for #38002, because on Windows an empty operand makes
rm -rf ""remove the files of the cwd. Alone, this PR gives"${X}"the behaviour that"$X"has on main: the word is dropped. A guard such as[ -d "${X}" ] && rm -rf "${X}"/*is safe on main only because the text${X}is not a directory. #44904 edits the three token pushes in the$arm. This PR does not touch them.git merge-treereports no conflict, and both orders give the same end state. One change is as in sh and stays with both PRs: withXnot set,rm -rf "${X}"*removes every file in the directory. On main it fails withno matches found. The lex tests compare the braced spelling with the bare spelling, so they pass before and after #44904. Three.todotests inbunshell.test.tshold the sh answer for"${UNSET}". The PR that lands second turns them on.echo a "${X}" ba ${X} ba ba ba b[ -d "${X}" ] && echo yes || echo nonoyesnonocd "${X}" && pwd$HOME85 whole-word idioms with
"${X}"andXnot set, each run withbun exec. A row matches when the exit status class, stdout and the created files equal those of bash 5.2.37. Bun's shell has no[ortestbuiltin, so the second line is what a machine with no such program sees (stock Windows).[andteston PATH[orteston PATHThe merged build predates the last change to the builder guard, which
bun execdoes not use. With[on PATH, 26 rows change from right to wrong (20 of them need[ortest) and 25 from wrong to right. Without it, 5 change to wrong (ls,rm -f,rm -rf,dirname,exit) and 18 to right. Each wrong row is wrong in the same way for"$X"on main.#29670. It edits the same line of
outbuf_ends_with_var_ref, so the PR that lands second has a conflict to resolve there. #29670 lets a name continue after a backslash-newline. The guard here looks only at the{and not at what is before it, so a value after$\<newline>{stays data in both orders. One test holds that ("after a line continuation between $ and {").Guard. It is true when the script ends in
$NAMEor{NAME. It does not look for a backslash or a quote, as before. A false positive costs one reference and changes token boundaries, not output:{${"a"},b},{pre${"a"},b}and'{${"a"}}'have tests. A false negative would let a value name a variable. With the guard line as on main, 10 guard tests fail. The guard is a copy of a lexer rule that the builder must keep in step. If every value goes in as a reference, the guard and the keyword check go away. That changesX_${v}=barand is a separate change.Measured. Release builds with LTO, same configuration, Linux x64: main 6c1eb06 and this commit on top of it. (main has two newer commits, #44955 and #45008. Neither touches the shell.) Instruction counts come from qemu-x86_64 user mode with one instruction per block, filtered to the address range of each symbol.
bloaty,valgrind,perfandstraceare not on this machine. Sizes come fromllvm-sizeandnm -S.Lexer<Ascii>::lex):$X417.01 to 412.01,$1372.01 to 373.01,$(456.01 to 456.01, literal$394.01 to 398.01 (a$ x) and 453.01 to 450.01 (a$^ x).break_wordandparse_atomdo not change. The new arm is one compare. The other differences are the layout of the recompiled function.echo ${X} x: 1,555 to 1,237 instructions inlex,break_word,parse_atomand the new helper. The bareecho $X xis 1,159.Bun.$builder: +3 instructions per plain interpolated value (157 to 160 inappend_js_value_str), reference calls unchanged at 0.\${nameor after{now goes in as a reference: +1 reference call and +2 allocations per tag call, output identical. Instructions per tag call:\${FOO+ value 76,864 to 83,921 (+7,057),{+ value 74,456 to 86,297 (+11,841). The control moved by 169. A reference costs the same on main:$+ value is 81,437 against 72,099 for a spliced value.$ { } \ " ' X 1 , ( )). All 6,562 hold an unescaped${NAME}or${0-9}. 0 other differences. 3,738 more inputs hold one and are the same lex error on both builds..text, +0 B.rodata, the stripped file does not grow.Lexer::lexAscii 7,044 to 7,073 B, Wtf8 6,774 to 6,786 B. The new helper is 464 B and 650 B.Tests.
lex.test.ts: 21 rows where the braced form gives the tokens of the bare form, 6 rows where the braces end the name, 4 rows that stay text as in sh, 12 rows for forms that are not supported, 4 rows for an interpolated value.bunshell.test.ts: 21 behaviour rows, 3.todorows for shell: keep an empty word that has a double-quoted expansion #44904, 12 guard rows (string, number, array, object withtoString,Bun.filepath, a strings array), 3 rows for a value after{. Each guard row also has a${FOO}in the source that must expand, so it fails on main too.env.positionals.test.ts,exec.test.ts,run-shell.test.ts:${N},bun exec, a.shfile, a package.json script.run-shell.test.ts: main 999 pass and 68 fail (the 66 new tests and 2lstests that need a user that is not root). This PR: 1,065 pass and 2 fail (the samelstests).leak.test.ts,shell-hang.test.ts,shell-load.test.ts,shell-blocking-pipe.test.tsand one test ofrm.test.tstime out on the debug build. They pass on both release 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/js/bun/shell/bunshell.test.ts, test/cli/run/run-shell.test.ts