Repository navigation
Conversation
|
Updated 9:33 PM PT - Oct 23rd, 2026
✅ @robobun, your commit 4dcbf80c2bed8ab1d4bca8b2ceb0eeaa9afa3338 passed in 🧪 To try this PR locally: bunx bun-pr 45339That installs a local version of the PR into your bun-45339 --bun |
|
Status: ready for review. Rebased on main after #46021 took the NUL fix of #44752. Reproduced with (release build of main, bash 5.2 beside it): bun exec "echo \"a\$(printf ' ')b\""
# main: ab. bash and this PR: a b
bun exec "printf 'a\tb\n' | cut -d\"\$(printf '\t')\" -f2"
# main: cut: the delimiter must be a single character. bash and this PR: bCI: on 4dcbf80 (build 131609) 179 of 181 jobs passed, Windows and macOS aarch64 included. The two that remain are PR: #45339 |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Beyond the inline findings, I also checked whether the unquoted $(cmd) path changed: post_subshell_expansion (Expansion.rs:569-572) still trims spaces, tabs, CR and LF at both ends before field splitting, so only the quoted/assignment arm gains trailing blanks. The intentional CR-before-LF divergence from bash is documented in the helper comment, docs/runtime/shell.mdx, and pinned by the new "substitution removes trailing line ends" tests.
Extended reasoning...
The change replaces the whitespace trim in the quoted/assignment command-substitution arm of src/runtime/shell/states/Expansion.rs with a shared strip of trailing LF/CR runs, and makes the shell exit builtin accept whitespace around its number; no security-sensitive surface is touched. Three confirmed findings are posted inline, so a human should weigh the behavioral tradeoffs for unquoted variables and Windows .cmd arguments before merging.
Findings marked 🟡 are optional suggestions and need no follow-up push.
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 nit, I checked the follow-up commit: the assign_ctx branch in src/runtime/shell/states/Expansion.rs now applies the same trailing SP/TAB/CR/LF trim as the base for A=$(...) and B="$(...)", post_subshell_expansion is byte-identical to the base, and parse_exit_code in src/runtime/shell/builtin/exit.rs trims only space and tab, so 3\r, 3\n and 3\f are still rejected (pinned in exit.test.ts). The remaining behavior change is confined to "$(...)" in a command word; since that is a user-visible Bun Shell semantics change, a human look at that choice is still worthwhile.
Extended reasoning...
The PR now changes only the quoted command-substitution path in Expansion::child_done (keeps trailing blanks and a lone CR, strips trailing LF/CRLF runs) and lets the exit builtin accept space/tab around its number; assignment and unquoted paths match the base. No security-sensitive surface. The follow-up commit addressed the two substantive prior findings (assignment values handing a trailing blank to unquoted $X, and exit accepting \f/\v/\r); the open inline finding this run is a style nit, but the change is a user-facing shell semantics decision authored by a bot, so a maintainer should still weigh it.
Still open from earlier reviews (1):
- Unresolved: 1 minor or pre-existing.
bbb2bc2 to
b7f6855
Compare
robobun
changed the title
|
Review follow-up, at b7f6855 (rebased on the new head of #44752):
The title and the body now describe the narrower scope. The choice that is left for a maintainer: a quoted substitution in a command word now gives the text of bash, so |
A quoted command substitution in a command word, a redirect target or a `[[ ]]` operand lost every trailing space, tab and CR of the output. The shell now removes only the trailing newlines there, and the CRs directly before them. An assignment value keeps the old trim, because an unquoted variable is not split into fields. `exit` accepts spaces and tabs around its number, as bash does, and no other byte.
b7f6855 to
4dcbf80
Compare
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 10 minutes. View limit details
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 |
There was a problem hiding this comment.
Beyond the inline finding, I checked the two paths the new commit reworked: the exit builtin now trims only space and tab before parse_decimal, and an argument that is all blanks still fails because parse_int rejects an empty slice; the assign_ctx arm restores the base branch's full trailing trim, so an unquoted $VAR from X=$(cmd) no longer carries a trailing blank into argv.
Extended reasoning...
The commit narrows the quoted-substitution trim in Expansion.rs child_done to trailing LF/CR runs while keeping the old trim for assignments, and widens exit's accepted status to allow surrounding blanks; no security-sensitive surface is touched. The remaining inline finding concerns the resulting mismatch between an assigned value and a fresh quoted substitution of the same output, which is a semantics choice a maintainer should weigh.
| if me.assign_ctx { | ||
| // `$X` is never field split, so a value still loses these trailing bytes. | ||
| let mut hi = stdout.len(); | ||
| while hi > 0 && matches!(stdout[hi - 1], b' ' | b'\n' | b'\r' | b'\t') { | ||
| hi -= 1; | ||
| } | ||
| stdout.truncate(hi); | ||
| } else { | ||
| Self::strip_trailing_line_ends(&mut stdout); | ||
| } |
There was a problem hiding this comment.
🔴 Scripts that store a command's output in a variable and later compare it to, or reuse it beside, a fresh quoted "$(...)" of the same command now get a mismatch when the output ends in a blank, where the base branch and bash both match. In child_done at src/runtime/shell/states/Expansion.rs:667-676 the assign_ctx arm trims \t\r\n while the word arm keeps the blank, so X="$(docker ps)"; [[ "$(docker ps)" == "$X" ]] is false and F="$(cmd)"; echo hi > "$(cmd)"; cat "$F" misses the file. Fix: give both forms the same bytes while keeping ping $IP working, e.g. store the exact value and drop trailing blanks only when an unquoted $X is expanded (SimpleAtom::Var at Expansion.rs:492).
Why this was flagged
A shell script runs a command whose output ends in a blank before its newline, which hostname -I, docker ps (tabwriter-padded columns) and Windows table tools such as wmic always produce, and uses the output both through an assignment and through a quoted substitution in a word. Expansion::child_done takes the assign_ctx arm at src/runtime/shell/states/Expansion.rs:667-673 for X="$(cmd)" and truncates , \t, \r, \n, but for "$(cmd)" in a command word, redirect target or [[ ]] operand it takes the arm at Expansion.rs:674-676 and strip_trailing_line_ends at Expansion.rs:557-564 keeps the blank. On the base branch both arms ran the same trim loop (the deleted lines in this hunk), so the words were equal; bash keeps the blank in both, so they are equal there too. No existing safeguard applies: SimpleAtom::Var at Expansion.rs:492-502 copies the stored bytes verbatim and has no quoted flag to trim on.
Verification: In src/runtime/shell/states/Expansion.rs:666-677 (child_done), under no_split, the if me.assign_ctx arm (lines 667-673) truncates every trailing , \n, \r, \t, while the else arm (line 675) calls strip_trailing_line_ends (lines 557-564), which only pops \n/\r and only when the last byte is \n, so a trailing space/tab survives.
There was a problem hiding this comment.
Confirmed. The PR body lists this mismatch under Downsides, and the Notes show both cells. I did not change it, for these reasons.
The exact value in the variable was the first version of this PR. The earlier review thread found what it breaks: IP=$(hostname -I); ping $IP passes 172.30.0.2 with the space, where main and bash pass none.
The rule proposed here (exact value, and drop the blanks at an unquoted $X) must know that $X is unquoted. SimpleAtom::Var has no such flag: the lexer pushes the same Token::Var for $X and "$X" (src/shell_parser/parse.rs:2659). #44904 adds that flag (QuotedVar) and is a draft. This PR does not copy its parser change.
With the flag, that rule still has a case that matches neither main nor bash. I checked both on main and on bash 5.2:
- Trim at every unquoted
$X:X="a "; Y=b; echo $X$Yprintsa bon main and in bash. It would printab. - Trim only at the end of a word:
X=$(echo 'a '); touch $X.txtcreatesa.txton main, andaand.txtin bash. It would createa .txt.
So each partial rule keeps one such case: the exact value (an unquoted $X), this version (a variable next to a fresh quoted substitution), and the proposed rule (a padded variable followed by text in an unquoted word). Only field splitting of unquoted expansions removes all three. That is a larger change, and #44904, #45968 and #43861 each hold a part of it.
This PR keeps the version whose case is the rarest: it needs output that ends in a blank, and a script that uses that output through a variable and through a quoted substitution. If a maintainer prefers the exact value in variables, remove the assign_ctx arm here. Then the todo test passes, and the test above it pins what breaks.
|
Reply to the review of 4dcbf80: the inline finding is correct, and it is the known limit of this version (Downsides, second bullet). The thread has the reasons why the PR keeps it:
It is a choice between three limits, so the thread stays open for a maintainer. No code changed since the last push. |
Problem
echo "a$(printf ' ')b"printsab, bash printsa b.echo hi > "$(echo 'out ')"writesout, andcut -d"$(printf '\t')"fails.Expansion::child_done(src/runtime/shell/states/Expansion.rs:658). POSIX 2.6.3 removes only trailing newlines.Fix
child_doneremoves only the trailing LFs, and each CR directly before them, from a quoted substitution in a command word, a redirect target or a[[ ]]operand. bash keeps that CR.$X.exitaccepts spaces and tabs around its number, as bash does.test/js/bun/shell/bunshell.test.ts,exec.test.ts,commands/exit.test.ts(16 of 24 new tests fail without the change). Also all oftest/js/bun/shell/. Self-reviewed: 22 concerns raised, 21 addressed.Background
child_donereads the stdout of a substitution's child script. A quoted substitution or an assignment appends it to the word, an unquoted one splits it.X=$(cmd); prog $Xthen passes a trailing blank that main and bash do not pass.Downsides
ping "$(hostname -I)"now gets the trailing space, as in bash.X="$(cmd)"; [ "$X" = "$(cmd)" ]is now false for such output (main, bash: true)..text+256 bytes.Notes
Rule for a quoted substitution in a word. If the output ends in LF, the shell removes the trailing run of CR and LF bytes. It keeps spaces, tabs, and a CR with no LF after it. An interior CRLF stays.
a<SP>aa<SP>a<SP>a<TAB><LF>aa<TAB>a<TAB>a<LF><SP><LF>aa<LF><SP>a<LF><SP>a<CR>aa<CR>a<CR>a<LF><LF><LF>aaaa<CR><LF>aaa<CR>a<CR><CR><LF>aaa<CR><CR>a<SP><CR><LF>aa<SP>a<SP><CR>History. This PR was first stacked on #44752. #46021 took the NUL fix of #44752 to main, and this PR is now one commit on main.
What does not change. An assignment value (
X=$(cmd),X="$(cmd)",export X=...,X=... prog) and an unquoted substitution give the same words as before. Measured over seven forms (below): only the quoted word changes.Why assignment values keep the trim. bash stores the exact value and drops the blanks later, when it splits an unquoted
$Xinto fields. Bun Shell does not split a variable (Expansion.rs,SimpleAtom::Var), and it has no${X% }to cut the blank. The first version of this PR stored the exact value. ThenIP=$(hostname -I); ping $IP,D=$(cmd); cd $D,touch $Xandseq $Ngot a trailing blank that neither main nor bash pass. So the exact value waits for field splitting of an unquoted variable. Until thenX=$(printf 'a '); echo "[$X]"prints[a]where bash prints[a ]. To keep the blank today, use the substitution in the word:echo "[$(printf 'a ')]".Why the CR goes on every platform. The shell has removed a CR before a trailing LF on Linux, macOS and Windows since #7748. Output that ends in CRLF also reaches Linux and macOS with no Windows program involved: HTTP headers, pty output, CRLF files.
echo "[$(printf 'Location: /x\r\n\r\n' | cut -d' ' -f2)]"prints[/x]on main and here, and bash keeps<CR><LF><CR>after/x. An LF-only strip gives that word back its CR and makes"$(hostname)"end in CR on Windows. For output without NUL bytes, this CR rule is the only place where the end of a quoted substitution in a word differs from bash, apart from the empty word below. Related to #29669 (CR in a script file), which this PR does not change. Windows was not run locally. The tests feed the substitution from theechobuiltin, so they run the same code on Windows CI.A lone CR stays.
"$(printf '\r')"is the way to get a CR in a script, as"$(printf '\t')"is for a tab. bash keeps it too.What changes for a Windows program. A word changes only when a blank or a lone CR stands before the last line end.
a<SP><CR><LF>wasaand is nowa<SP>. Programs that pad their lines with spaces (table output,wmic) give such output. I did not run these programs.exit.parse_exit_codetrims spaces and tabs.exit "3 ",exit " 3"andexit "3<TAB>"give 3, as in bash and dash (main:exit: numeric argument required). A trailing CR, LF or FF is still an error, as in bash. The strip is what exposes this: on main the trim hid the blank inexit "$(echo '3 ')".Costs, with outputs (bash / main / PR).
./argv "$(hostname -I)"passes172.30.0.2<SP>/172.30.0.2/172.30.0.2<SP>. A quoted word is now the text of bash, so a command that failed on the blank in bash fails here too../argv a "$(true)" b "$(printf ' ')" cpasses<a><><b>< ><c>/<a><b><c>/<a><b>< ><c>."$(printf ' ')" echo xranechoon main. It now fails withcommand not found, and bash fails too.exit "$(printf '3\r')"gives 2 / 3 / 1: the lone CR stays in the word, andexitrejects it as bash does.X="$(echo 'a ')"; [ "$X" = "$(echo 'a ')" ] && echo T || echo FprintsT/T/F.F="$(echo 'out ')"; echo hi > "$(echo 'out ')"; cat "$F"printshi/hi/cat: out: No such file or directory.[[ "$(echo 'a ')" == $(echo 'a ') ]]is true / true / false, because an unquoted[[ ]]operand is split (shell: expand a[[ ]]operand to one word and match*on the right of == and != #45968 covers that).Not changed here, on purpose.
todotest recordsX="$(echo 'a ')"; echo "[$X]"with the result of bash.test -n "$(true)"is true (bash: false). shell: keep an empty word that has a double-quoted expansion #44904 covers it.exportoperand whose name is not literal text (export $N="$(cmd)",export "X"="$(cmd)") is a command word, not an assignment (Cmd.rs,is_assignment_word). A quoted substitution in it keeps its blanks, so the exported value has them.exit 1_0exits 10 on main, because the number parser accepts_. bash rejects it. With the blank trim,exit '1_0 'does the same..cmdor.batcommand refuses an argument that contains CR or LF (Cmd.rs,batch_arg_has_cmd_metachars). A kept lone CR, or an LF that is no longer at the end because a blank follows it, now reaches that check in a quoted word. The tree has no test for that refusal today.Scope of the gain. The fix matters for straight-line scripts: a tab or a blank from
printfused in a word, a file name with a trailing blank,[[ "$(cmd)" == "x " ]]. The comparison with bash is in the table and the measurements here and not in the tree.Measurements. Release builds, linux x64, main (6292c65) vs this PR.
mi_mallocandmi_free, slope of 200 vs 400 builtin substitutions)Interpreter::child_doneper substitution: quoted word 1,690 -> 1,693, quoted assignment 1,787 -> 1,790, unquoted assignment 1,792 -> 1,795, unquoted word 1,769 -> 1,769 (gdb single-step, callees included)size);Interpreter::child_done8,539 -> 8,593 bytes,Exit::start466 -> 615 bytes,post_subshell_expansion2,001 -> 2,001 bytes (nm -S)catch syscall, slope of 200 vs 400)Reviews. The first version kept the blanks in assignment values too. The review of that version found the
X=$(cmd); prog $Xregression, and this version keeps the trim there. A second review of this version raised 11 points. 10 are done: the unsortedlsin a test, the test names and comments, the docs wording, and the two limits above (the variable against a quoted substitution,exportwith a name that is not literal). Not done:exit 1_0, which is base behaviour. A later review comment asked forstrings::trim_rightin both trims. Not done: measured on the earlier base, the helper costs an assignment substitution +36 instructions inchild_done, against +2 with the loop that is there, and this PR does not change assignment values.Tests.
todo. 16 fail without the change. The 8 controls pin what must stay: every LF, every CRLF and every CR before the LF are removed, an assignment value (X=$(cmd),X="$(cmd)",export X="$(cmd)") still drops the trailing space,exitrejects a trailing CR, LF and FF, andexittakes a captured status.test/js/bun/shell/(44 files) andtest/cli/run/run-quote.test.tson release builds with and without the change: the same results in every file except the 16 tests above.shell-load.test.tsfails on both withToo many open filesin this container.bunshell.test.ts(480 pass),exec.test.ts(20 pass) andcommands/exit.test.ts(14 pass).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