Repository navigation
Conversation
|
Updated 7:05 AM PT - Oct 21st, 2026
❌ @robobun, your commit 56e8d0a has 1 failures in
🧪 To try this PR locally: bunx bun-pr 45968That installs a local version of the PR into your bun-45968 --bun |
|
Status: reworked and pushed as two commits. It waits for CI and for a maintainer. How I reproduced it. Bun 1.4.3 and the merge base, bash 5.2.37 as the reference: echo "hello world" > greeting.txt; echo x > "my file.txt"
bun exec 'if [[ $(cat greeting.txt) == "hello world" ]]; then echo same; else echo DIFFERENT; fi' # DIFFERENT, bash: same
bun exec 'if [[ $(cat greeting.txt) == helloworld ]]; then echo EQUAL; else echo "not equal"; fi' # EQUAL, bash: not equal
bun exec 'if [[ -f $(echo my file.txt) ]]; then echo found; else echo MISSING; fi' # MISSING, bash: found
bun exec 'if [[ $(cat greeting.txt) == hello* ]]; then echo matches; else echo "NO MATCH"; fi' # no matches found + NO MATCH, bash: matchesThe same script as a One decision is open for a maintainer: commit 2 matches an unquoted |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
WalkthroughChangesShell expansion and conditional expressions
Suggested reviewers: Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation |
|---|---|---|
| Title check |  |
The title clearly describes the main changes: single-word expansion for [[ ]] operands and * matching on the right side of == and !=. |
| Description check | |
The description explains the problem, implementation, behavior changes, trade-offs, maintainer decision, and extensive verification results. It does not use the exact template headings, but it provide… |
| 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. |
Full details: Description check
-
Explanation
The description explains the problem, implementation, behavior changes, trade-offs, maintainer decision, and extensive verification results. It does not use the exact template headings, but it provides the required content.
- 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.
✨ Finishing Touches: 💡 1
🛠️ Fix failing CI checks: 💡
- Commit to this branch
- Create a new PR
![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 the deleted take_err arm in CondExpr::on_expansion_done: with WordContext::CondOperand the BraceExpand and glob transitions in Expansion.rs are gated off, so no brace/glob error can reach that arm, and Expansion::take_err still has a live caller in Cmd.rs. The two new debug_assert!s compile out and the release path reports exit 1 to the parent rather than panicking. The PR's own open "refuse or match" question for [[ $f == *.md ]] is a product decision a maintainer should settle.
Extended reasoning...
The change swaps the assign_ctx bool on Expansion for a three-variant WordContext enum, routes [[ ]] operands through a no-split/no-brace/no-glob path, and adds a refusal for an unquoted * in the right operand of ==/!=; it touches no security-sensitive surface. Findings were posted inline (notably the != refusal changing which branch existing scripts take), and the PR explicitly asks a maintainer to decide refuse-vs-match, so a human look is needed.
Findings marked 🟡 are optional suggestions and need no follow-up push.
An unquoted command substitution in a `[[ ]]` operand was field split, and `CondExpr` then joined the fields with nothing between them. So `[[ $(echo a b) == "a b" ]]` was false and `[[ $(echo a b) == ab ]]` was true. Brace expansion and pathname expansion ran on the operand in the same way. `Expansion::init` now takes a `WordContext` (argv word or redirect target, assignment value, `[[ ]]` operand) in place of the `assign_ctx` bool. A `[[ ]]` operand runs no field splitting, brace expansion or pathname expansion, as in bash, so it is always one word. A `~` that is not at its start is text. A test whose right operand of `==` or `!=` has an unquoted `*` is a pattern test in bash. It keeps the argv expansion here.
bash reads the right operand of `==` and `!=` as a pattern. The shell expanded it like an argv word, so `[[ $f == *.md ]]` compared `$f` with the file names in the directory, or printed `no matches found`. The parser now records how the right operand is read in `ast::CondPattern`. For `Star`, the operand expands in `WordContext::CondPattern`: `Expansion` returns the text between the unquoted `*` bytes as parts, and `CondExpr` matches the left operand against them. A `*` matches any run of bytes. Only a `*` typed in the script is a wildcard. A `*` from quotes, a variable, a substitution or a JS value is text. A pattern that also has an unquoted `?` or `[` is `Unsupported`. It prints a message and the test fails. The lexer marks a backslash-escaped `?` or `[` as data, so `a\?*` is a `Star` pattern.
4fda43c to
56e8d0a
Compare
robobun
changed the title
[[ ]] operand to exactly one word[[ ]] operand to one word and match * on the right of == and !=
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.
The two new commits address what the previous round raised: a *-only matcher now decides ==/!= instead of a refusal, and every row in the new condexpr suites pins .stderr(""). Beyond the inline finding, I also checked the data_range_at binary search (the lexer and its sublexers push js_string_ranges at strpool-append time, so the list is sorted and non-overlapping, and an interpolated value never extends past the Text token that holds it, so text[searched..] cannot go out of range), ** in a pattern (both * offsets are recorded, giving an empty middle part the matcher skips), and the deleted take_err arm (every ExpansionState::Err producer is brace or glob expansion, which the two [[ ]] contexts never enter).
Extended reasoning...
The change replaces a bool with a WordContext enum in the shell's word expander, adds a CondPattern discriminant to the parser AST with a token-range scan for unquoted ?/[, marks backslash-escaped ?/[ as data in the lexer, and adds a *-only matcher in CondExpr; no auth, crypto, or network surface. The parser scan and matcher index arithmetic were traced and hold, and the one surviving finding is pre-existing tilde behaviour outside [[ ]], but the diff is ~630 lines across the lexer, parser, and runtime, so a human look remains worthwhile rather than an approval.
| ast::SimpleAtom::Tilde => { | ||
| if expand_tilde { | ||
| // Only brace expansion can make a `~` after the first atom the start of a word. | ||
| if word_idx == 0 || ctx.brace_expansion() { |
There was a problem hiding this comment.
🟣 pre-existing, not blocking: pre-existing: in argv words and assignment values a ~ right after a variable or quoted text still expands to $HOME, so mv "$f" "$f"~ targets <f>/home/user instead of <f>~, as on the base branch; only the two [[ ]] contexts are fixed. Expansion.rs:572 expands a Tilde atom at any word index whenever ctx.brace_expansion() is true, and parse.rs:1675 makes a Tilde atom from every Text token that starts with ~. Fix: expand a mid-word ~ only where brace expansion can put it at the start of a word (directly after a { or , meta atom), or run tilde expansion after brace expansion, while keeping ~ at index 0 and {a,~} → a $HOME working.
A small fix can ride a push you are already making; otherwise a short reply is enough.
Why this was flagged
A script run through Bun.$ or bun script.sh uses the backup-suffix idiom cp "$f" "$f"~ or mv $f $f~. The lexer ends the Var token and sets word_start after it at src/shell_parser/parse.rs:2767, so $f~ lexes as Var(f) then Text("~"); src/shell_parser/parse.rs:1675-1677 turns that Text token into a SimpleAtom::Tilde at word index 1. At runtime Expansion::expand_simple_no_io reaches src/runtime/shell/states/Expansion.rs:572 with word_idx == 1 and ctx == WordContext::Fields (or AssignValue for A=$f~); ctx.brace_expansion() is true for both, so the arm appends $HOME and the argument becomes <f>/home/user. bash only tilde-expands a prefix at the start of a word, so it passes <f>~. The base branch passed expand_tilde: true unconditionally and had the same result for these words; this PR keeps that path and only stops the expansion for CondOperand/CondPattern. The new comment at Expansion.rs:571 says only brace expansion can make a later ~ a word start, but the arm fires for every Fields/AssignValue word, not only those with a brace group.
Verification: pre-existing. Trigger: any argv word or assignment value where a literal ~ follows a variable or quoted text. The gate at src/runtime/shell/states/Expansion.rs:572 is if word_idx == 0 || ctx.brace_expansion(), true for Fields and AssignValue, so a mid-word Tilde atom is expanded to $HOME. On the base commit the sole caller passed expand_tilde = true (base Expansion.rs:185).
This branch has not been deployed
2 participants
Problem
[[ $(echo a b) == ab ]]is true, with no error.CondExpr(the node that runs[[ ]]) expands an operand like an argv word (src/runtime/shell/states/CondExpr.rs:69) and joins the fields.[[ $f == *.md ]]compares$fwith file names.Fix
Expansion(it expands one word) takes aWordContextin place of theassign_ctxbool. A[[ ]]operand is not field split, brace expanded or globbed, as in bash.*on the right of==or!=matches any run of bytes. An unquoted?or[beside it fails the test with a message. Without commit 2, that operand expands as on main.test/js/bun/shell/{bunshell,exec,parse}.test.ts(152 new tests, 113 fail before) and the whole directory.=as==, a bash oracle test, docs.Background
assign_ctxso thatA=$(...)is not field split. For shell: remove only trailing line ends from a quoted command substitution #45339, it is nowWordContext::AssignValue.*typed in the script is a wildcard.*operand. For!=, exit code 1 reads as "it matched".Downsides
[[ a1.txt == a* ]]is true with no such file.[[ ${f} == *.md ]]is false with no message:${f}is not expanded.[[ a == a ]]runs 79 more instructions (1.2%), with the same allocations..textgrows 3,328 bytes.Notes
The decision for a maintainer: commit 2. No user reported any of this. bash reads the right operand of
==and!=as a pattern. No maintainer asked for that in the Bun shell. The two commits build and pass their tests one by one, so the answer "no" is commit 1 alone.[[ $f == *.md ]]is a match of$fagainst*.md. The directory has no part in it.*expands like on main. Besidea1.txt,[[ a1.txt == a*.txt ]]is true.[[ README != *.md ]]printsbun: no matches found: *.mdand takes the false branch. Every other[[ ]]operand is one word.*operand with a message and exit code 1. That is gone. For!=, exit code 1 reads as "it matched", soif [[ $f != *.md ]]skipped a body that bash runs.Repro. bash 5.2 prints
same,found,not equal. Bun 1.4.3 printsDIFFERENT,MISSING,EQUAL.The pattern rules of commit 2
==and!=is a pattern, and only when it has an unquoted*. The parser records this inast::CondPattern(None,Star,Unsupported).shellInternals.parseprints it.*matches any run of bytes: nothing,/, a leading.. The match is on the whole left operand, byte by byte.{,,,}, and everything that comes from$NAME,$(...)or a JS value (shell: only template-literal glob tokens act as pattern syntax #31220).Star,ExpansiongivesCondExprthe text between the*bytes as parts (WordContext::CondPattern).CondExprhas no new field.?or[beside the*isUnsupported. The shell cannot tell what it must match. The test prints the message and fails:"?",'[',\?and\[are text, so[[ $msg != *"[skip ci]"* ]]and[[ $url == *\?* ]]work. The lexer marks a backslash-escaped?or[as data for this.*in the operand,?and[...]are text, as before.[[ $REPLY != [Yy] ]]keeps its answer. Six rows pin this.What changes for a script that runs today
$(...)or backtick substitution in a[[ ]]operand gives the same text as the quoted form. Blanks and newlines inside it stay. Leading blanks stay too.{a,b}and*in a unary operand or a left operand are text.[[ -n *.nomatch ]]no longer printsno matches found, and[[ -f a1.tx* ]]tests the namea1.tx*.*in the right operand of==or!=is a pattern (commit 2).~that is not at the start of an operand is text:[[ "a"~ == "a~" ]]was false and is true, as in bash.[[ $f == *~ ]]matches a name that ends in~.! [[ ... ]]is not a negation in the Bun shell (shell: reject!, brace groups, here-documents and stray then/elif/else/fi at parse time #43922 rejects it at parse time). So a negated test changes its branch where the test changes its answer.Where the answer is the bash answer, and where it is not
$NAME,$0to$9,$(...), backticks, quoted text and JS values.*-only[[ ]]tests taken from existing shell scripts, each run with four operand values: 174 take the bash branch on all four. Main takes it on none.==always false, 8!=always true). Their operands are$@or$*(35),${arr[i]}(20),${NAME}(19) and${VERSION%%+*}(1). The shell does not expand these anywhere. Draft shell: expand${NAME}the same way as$NAME#45032 adds${NAME}. Atest.failingrow trips when it does.=is the same operator as==in bash. Here[[ a = a ]]is still the parse errorUnknown conditional expression operation: =.[[ ]]forms, one fresh directory per form: 30 to 96 take the same branch with an empty stderr. Wrong branch with no message: 54 to 17. Forms that print to stderr: 36 to 7. Of the 14 forms of an unquoted substitution, 0 to 11 are equal.Forms that moved away from bash (4)
[[ -f $(echo "my file.txt ") ]]and[[ $(echo " a ") == " a" ]]: the path that keeps a substitution whole trims trailing blanks. Before, the field split hid that.n=" 3"; [[ $(echo $n) == 3 ]]and its!=form:$nis not field split, soechoprints the blanks. Before, the field split of the substitution removed them.[[ $(echo $(echo $n)) == 3 ]]is true before and after. This is thewc -lcase on macOS. It was not run on macOS.Forms that went from a message to a silent wrong branch (3)
NAME=foo.md; [[ ${NAME} == *.md ]]and its!=form (see${NAME}above).P='a?'; [[ a1.txt == $P* ]]: the?comes from a variable, so it is text. bash says true.Still different from bash, left out on purpose
[[ -z $(echo " ") ]],[[ -n $(echo " ") ]],[[ $(echo "a ") == "a " ]]. shell: remove only trailing line ends from a quoted command substitution #45339 changes that trim for the quoted form and selects its arm withassign_ctx. When the two PRs meet,me.assign_ctxbecomesmatches!(me.ctx, WordContext::AssignValue). Then a[[ ]]operand takes the arm that removes line ends only, and these forms equal bash.[[ "$(echo "a ")" == $(echo "a ") ]]and its mirror pin that the quoted and the unquoted form stay equal.$Por$(...)is text (shell: only template-literal glob tokens act as pattern syntax #31220).[[ a == ? ]]and[[ a == [abc] ]]are text compares.**in a[[ ]]operand is a parse error before and after (shell: keep a digit in its word before>or<, end the word after**, and reject<<#44532).?and[still gives exit code 1 for!=.Open PRs that touch the same lines. None of them needs to wait for this one, and this one waits for none.
Cmd.rsandparse.rs. The second one to land makes three edits:WordContextif/else inCmd.rs, withargs[0].is_declaration_utility().let arg2_start = self.current;beforeparse_atom(WordMode::Argv).Token::JSArraySepto the arm ofhas_unquoted_question_or_bracketthat does nothing.?and[...]. Thematchinhas_unquoted_question_or_bracketnames every token kind, so the merged tree does not build until that kind has an arm.${NAME}the same way as$NAME#45032 and shell: pass a glob with no match to the command as written #43746 conflict with main today. With this PR the conflicts are theExpansion::initcall sites, the line that selects the path with no field splitting, and the test file.Measurements. Release builds of ea6a2c7 and of this diff on it, one configuration. The branch is this diff rebased onto 9b4284f. The rebase changed no line of the diff.
bun-profileunder gdb.perf,valgrindandbloatyare not installed in my container.stepi, main thread,bun execof 8 copies, counts repeat exactly).echo a: 4861 to 4858. One plain argv word: 1082 to 1081. An argv word with*, up to the start of the glob walk: 517 to 520.echo {a,b}: 8193 to 8205.echo "a"b$x: 4110 to 4113.[[ a == a ]]: 3096 to 3151.[[ -n a ]]: 2138 to 2169.[[ a == a ]], 38 are one node accessor (as_condexpr_mut, 19 instructions, 2 calls) that LLVM no longer inlines intoInterpreter::child_done.echo a: 0.echo a\ b: 6 more per escaped character.[[ a == a ]]: lexer 6 fewer, parser 30 more (LLVM copies the right operand field by field after the*check reads it).[[ -n a ]]: lexer 4 fewer, parser 3 more.[[ abc == a*c ]]: parser 193 more for the?and[scan.[[ a == a ]].echo a b c d e f g h37/5/39,echo $(echo a b)49/2/42,A=$(echo a b)39/1/34,export A=$(echo a b)48/0/40,echo hi > $(echo f)50/1/43,echo a*in a 9-file directory 68/33/88,[[ a == a ]]9/0/7,[[ -n a ]]6/0/4. Lower:[[ $(echo a b) == "a b" ]]40/0/34 to 39/0/33,[[ * == "*" ]]52/19/60 to 10/0/8,[[ {a,b} == "{a,b}" ]]17/2/16 to 10/1/8,[[ abc == a*c ]]21/3/21 to 13/0/10.catch syscall, slope between 50 and 150 copies, 9-file directory).[[ a1.txt == a*.txt ]],[[ x == *.nomatch ]]and[[ * == "*" ]]: openat 1 to 0, getdents64 2 to 0, close 1 to 0, and no write ofno matches found.[[ x == ?* ]]: the same three go to 0, write 2 to 1 (the message).size_of::<Node>()176,Expansion168,CondExpr112,WordContext1,ast::CondExpr88. The slab stride is 176 bytes in both release builds.bun):.text81,695,461 to 81,698,789 bytes..rodataand the file size do not change (22,606,796 and 107,271,904 bytes).?before the*and 4,000 interpolated array elements: 1.8 ms to 1.9 ms for the synchronous part of the$call.Tests run
bunshell,exec,shell-seq-condexpr,pipeline_stack,parse,brace,lazy,lex: 813 pass, 0 fail.lazy: 716 pass, 0 fail.test/js/bun/shell/andtest/cli/run/run-shell.test.ts: 1146 pass, 0 fail (44 files).==rows of the GNU bash port moved out ofdescribe.todo.cargo clippyforbun_shell_parserandbun_runtime, andcargo fmt --check: clean.shell-load.test.ts(30,000 spawns in 90 s) times out on the debug build in my container. It passes on the release builds with and without the diff.Self-review, not addressed (3)
=as a synonym of==: a separate change inBINARY_OPS.[[ ]].docs/runtime/shell.mdxhas none today.History. The original code passed
ExpansionOpts { single: true }for a[[ ]]operand and never read it. #35002 removed the option as dead code. #33552 (closed by a stale sweep) made[[ ]]operands exempt from field splitting as part of anIFSchange.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