Repository navigation
Conversation
The brace lexer now reports, for each unescaped `{`, `,` and `}`, whether
it survived as brace syntax. Before a word reaches the glob walker, the
shell drops the brace bytes the lexer demoted to text from `meta_offsets`,
so `neutralize_glob_metachars` wraps them like any other data byte. A word
with no brace hint at all drops every brace byte.
`echo {x}.*.txt` matched `x.a.txt` because the glob matcher reads every
`{...}` as a group. It now matches `{x}.a.txt`, as bash does.
|
Status: closed, superseded by #32902 (matcher fix, covers every case here) and #33423 (removes the shell path the lexer change feeds). Details in the comment below. Reproduced on bun 1.4.0 and on main: |
|
Important Review skippedThis review includes 3 billable files. This on-demand review is free during your promotion. Your included review limit has been reached. Run
⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
You can disable this status message by setting the Comment |
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. The alignment invariant between kept_as_syntax and the brace bytes in meta_offsets checks out, and the test coverage is thorough. Because it's a correctness change to the shell's brace/glob interaction with a subtle ordering invariant, a human look would still be worthwhile.
What was reviewed:
- Traced the
kept_as_syntax↔meta_offsetszip:do_brace_expand's escaping loop leaves exactly the meta-offset brace bytes unescaped, andmeta_offsetsis monotone, so the pairing is one-to-one (guarded by thedebug_assert). - Verified
kept_as_syntaxis read offtokensafter demotion androllback_bracesbut beforeflatten_tokensreorders indices. - Confirmed the no-brace-hint path (
retain_brace_syntax(me, &[])) is sound:brace_expansion_hintat parse.rs:1671 requires{,}and,, so any word the lexer would expand already takes thedo_brace_expandpath.
Extended reasoning...
Overview
The PR fixes a bash-compat bug where a comma-less brace group ({x}) that the brace lexer demotes to literal text was still handed to the glob walker as an active brace group, so {x}.* matched x.a instead of {x}.a. Three files change: src/shell_parser/braces.rs gains a kept_as_syntax: Vec<bool> on LexerOutput (one verdict per unescaped {/,/} in input order), src/runtime/shell/states/Expansion.rs gains retain_brace_syntax which drops the demoted entries from meta_offsets before neutralize_glob_metachars runs, and test/js/bun/shell/brace.test.ts gains 9 integration tests plus a 12-case Rust unit test for the new field.
Security risks
None. This is a correctness fix that narrows what the glob walker treats as pattern syntax; it cannot broaden a match. The neutralization path ([c] wrapping) is pre-existing and only receives more bytes to wrap.
Level of scrutiny
Medium-high. The fix rests on an ordering invariant: the sequence of brace bytes in meta_offsets (offsets into current_out) must line up one-to-one with the lexer's kept_as_syntax (verdicts over the escaped string). I traced this through: expand_simple_no_io pushes meta_offsets in append order (monotone), the tilde prefix adjusts them uniformly, and do_brace_expand's escaping loop backslash-escapes every {/,/}/\\ not at a meta offset — so the unescaped brace bytes the lexer sees are exactly the meta-offset brace bytes, in order. The * entries in meta_offsets are skipped by the match in retain_brace_syntax and don't consume a verdict. The debug_assert!(kept.next().is_none()) catches any drift.
I also checked the lexer side: brace_chars records Some(tok_idx) for each syntax token and None for immediate text, then re-reads the token variant after rollback_braces (which uses in-place replace_token_with_string, so indices stay valid) but before flatten_tokens (which swaps tokens). Walked the {a,{b}}, {a,{b,c},d and {a\\,b} cases by hand and they match the unit-test expectations. The }-with-empty-stack and stray-, arms correctly push None and fall through to append_char (no continue), preserving pre-existing token output.
For the no-brace-hint path (has_glob_expansion() without has_brace_expansion()), retain_brace_syntax(me, &[]) drops every recorded brace byte. This is safe because brace_expansion_hint at parse.rs:1671 is has_brace_open && has_brace_close && has_comma — any word the brace lexer could expand sets the hint, so on this path no brace can be legitimate glob-walker syntax.
Other factors
The tests are strong: each fixture creates both the {x}.* and x.* filenames so a regression to the old behavior fails observably, the {a,b},* case guards the zip pairing specifically, and the Rust unit test covers rollback, nesting, escaped chars and multi-byte text on both encodings. The PR description documents the interaction with #33423 and #32902. This is well-executed but not mechanical — the invariant spans two subsystems and someone who owns the shell expansion pipeline should confirm it.
|
Closing after self-review. The bug is real, but this is the wrong place to fix it, and the change has a regression of its own. Wrong layer:
Regression (verified with this branch):
The nine shell cases in |
Problem
{x}.a.txtandx.a.txt,Bun.$\echo {x}..txt`printsx.a.txt. bash prints{x}.a.txt.{x},.txt,{a,{x}}..txtand{a,b` fail the same way.src/shell_parser/braces.rs) treats a{...}group with no comma as text. The glob matcher still reads every{...}as a group, andneutralize_glob_metachars(src/runtime/shell/states/Expansion.rs:402) passes every template brace byte through to it.Fix
LexerOutput::kept_as_syntax: one bool per unescaped{,,,}, read off the tokens after demotion and rollback.retain_brace_syntaxdrops the demoted brace bytes frommeta_offsetsbefore the word goes to the walker. A word with no brace hint drops all of them. The neutralizer then wraps them as[{],[,],[}], as it already does for interpolated braces.test/js/bun/shell/brace.test.ts(9 new tests, 8 fail on main) and a unit test inbraces.rs. Alsobunshell.test.ts,parse.test.ts,lex.test.ts, miri, clippy.Background
Expansion.rsexpands one shell word.meta_offsetslists the bytes written by template*,**,{,,,}atoms. Only these may act as pattern syntax.neutralize_glob_metacharswraps every other metacharacter in a[c]class.{,}and,. Without it the word goes to the walker directly.do_brace_expandruns the brace lexer, pushes the variants, and also hands the original pattern to the walker, which expands the groups again (shell: pathname-expand each brace variant instead of appending the patterns #33423 changes that part). This PR edits that pattern.Notes
Before and after, with the fixtures from the tests:
echo {x}.*.txtx.a.txt{x}.a.txt {x}.b.txtecho *.{x}a.xa.{x}echo {x}/*.txtx/a.txt{x}/a.txtecho {a,b*no matches found{a,b1.txtecho {a,b}x{c*ax{c1 bx{c2echo {x},*.txtx,a.txt{x},a.txtecho {a,{x}}.*.txta.1.txt x.1.txta.1.txt {x}.1.txtThe last three words also emit the un-expanded variants as extra argv words on main. That is the pre-existing behavior #33423 removes, so those tests assert on the matches only. When #33423 lands, its test "a comma-less brace group still globs a literal *" needs
{x}.a.txtstyle fixtures.Alignment between the verdicts and
meta_offsets:do_brace_expandbackslash-escapes every{,,,}and\that is not inmeta_offsets, so the unescaped brace characters the lexer sees are exactly the recorded ones, in order.retain_brace_syntaxzips the two anddebug_asserts that both run out together. The{a,b},*.txttest guards the pairing: the stray comma is dropped, the group is kept.,and}outside any group were already literal in the matcher, so dropping them changes nothing. An unclosed{used to make the matcher fail the whole word. Now it is literal, as in bash.#32902 takes the other route and changes
Bun.Globitself. This PR keeps the shell correct either way: if #32902 lands,[{]and{in a walker pattern both mean a literal brace.Ran:
bun bd testontest/js/bun/shell/brace.test.ts,parse.test.ts,lex.test.ts,bunshell.test.ts, the glob-usingcommands/tests (threelsfailures are environmental: root user, no registry access) andtest/internal/source-lints/.cargo test -p bun_shell_parser,bun run rust:miri -p bun_shell_parser,cargo clippy --no-depsonbun_shell_parserandbun_runtime. All new fixtures were also run through bash 5.2.37.