Repository navigation
Conversation
Bun Shell split unquoted command-substitution results on a hardcoded
{space, newline} predicate, ignoring the IFS shell variable entirely.
This gave the wrong argv arity in both directions: IFS=: / IFS=, / a
newline IFS produced one glued-together argument, tab-separated output
(the default IFS includes TAB) never split, and IFS= (empty) failed to
disable splitting.
Implement POSIX field splitting driven by IFS: default separators are
space/tab/newline, an empty IFS disables splitting, runs of IFS
whitespace collapse, and each non-whitespace IFS byte delimits a field
so consecutive ones and leading/trailing ones yield empty fields.
Command substitution still deletes trailing newlines before splitting.
|
Reproduced against stock 1.4.0 and origin/main: unquoted command substitution split on a hardcoded Also addressed in this PR following review:
Known gaps noted for follow-up (narrow, shells disagree, or need separate env-model work): CI: The shell suite is green on all lanes. The remaining red on this PR is unrelated pre-existing flake (NAPI / worker_threads JSC assertion on x64-asan / install-registry), none of which touch |
|
Warning Review limit reached
Next review available in: 24 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughThis PR rewrites shell command-substitution splitting to follow POSIX IFS rules, adds tracking for committed and empty fields, updates argv and redirect consumers to match the new semantics, and adjusts brace/glob boundary handling with new tests. ChangesShell expansion field splitting
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 12:05 AM PT - Jul 7th, 2026
❌ @robobun, your commit 714f513 has some failures in 🧪 To try this PR locally: bunx bun-pr 33552That installs a local version of the PR into your bun-33552 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
…eld handling Address three issues in the IFS field-splitting change: - Assignment values are no longer field-split. POSIX exempts assignment values, redirect targets, and [[ ]] operands from field splitting; the existing ExpansionOpts.single flag (already passed by those call sites but previously ignored) now suppresses it. Assigns now passes single. - IFS is read only from shell_env, never export_env. Like every POSIX shell, an inherited environment IFS (here process.env.IFS) must not control field splitting; a script-local IFS= assignment lands in shell_env, so the legitimate use is unaffected. - Leading empty fields are preserved for two-field results (e.g. $(echo ,a) with IFS=,). ExpansionOut now tracks whether a word has been committed instead of inferring it from buf/bounds emptiness, which could not distinguish 'no word yet' from 'one empty word committed'. The brace and glob commit loops use the same flag.
|
Thanks, all three are real. Fixed in 747b149:
Added tests for all three (assignment not split, env |
| /// whitespace) delimits one field, so consecutive ones yield empty fields. | ||
| fn ifs_split_fields<'a>(s: &'a [u8], ifs: &[u8]) -> Vec<&'a [u8]> { | ||
| let is_ifs = |b: u8| ifs.contains(&b); | ||
| let is_ifs_ws = |b: u8| matches!(b, b' ' | b'\t' | b'\n') && ifs.contains(&b); |
There was a problem hiding this comment.
🟡 nit: is_ifs_ws hardcodes {space, tab, newline}, but bash 5.2 (the PR's stated reference) treats \r/\f/\v as IFS whitespace when they appear in IFS — so IFS=$'\r'; set -- $(printf 'a\r\rb') gives 2 words in bash but 3 (with a spurious empty middle) here. Obscure trigger and matches dash, so not blocking; the fix is one line: add b'\r' | b'\x0c' | b'\x0b' to the matches!.
Extended reasoning...
What the bug is
The IFS-whitespace predicate in ifs_split_fields is:
let is_ifs_ws = |b: u8| matches!(b, b' ' | b'\t' | b'\n') && ifs.contains(&b);POSIX §2.6.5 defines IFS white space as any character that is both in IFS and in the current locale's [:space:] class — in the C/POSIX locale that is {space, tab, newline, carriage-return, form-feed, vertical-tab}. Bash 5.0+ implements exactly this (its CHANGES file documents the switch to the locale space class), and the PR description explicitly says output "matches bash 5.2 across the matrix." With this predicate, an \r, \f, or \v in IFS is classified as a non-whitespace IFS byte — a hard delimiter — instead of collapsing whitespace.
How it manifests / step-by-step proof
Take IFS = "\r" and input "a\r\rb" (after trailing-newline stripping). Tracing ifs_split_fields:
is_ifs_ws('\r')=false(not in thematches!set), so the leading-whitespace skip does nothing;i = 0.- Field scan:
i = 0..1('a'is not in IFS). Push"a".i = 1. - Delimiter consumption: the first
while is_ifs_wsloop is a no-op.is_ifs(s[1])is true → consume one byte,i = 2. The trailingwhile is_ifs_wsloop is again a no-op. - Field scan at
i = 2:s[2] = '\r'is in IFS, so the loop body doesn't advance. Push""(empty slice[2..2]). - Consume one delimiter →
i = 3. Field scan pushes"b".
Result: ["a", "", "b"] — 3 fields. Bash 5.2 gives 2:
$ bash -c 'IFS=$(printf "\r"); set -- $(printf "a\r\rb"); echo $#'
2
$ bash -c 'IFS=$(printf " \r"); set -- $(printf "\ra\r"); echo $#'
1The same divergence applies to \f (0x0c) and \v (0x0b), also verified against bash 5.2.21.
Why nothing else prevents it
The predicate is new in this PR; nothing else classifies IFS whitespace. The old post_subshell_expansion never consulted IFS at all (and its trim step happened to include \r), but that path is gone.
Impact and why this is a nit
The trigger requires deliberately putting \r/\f/\v into IFS, which is awkward in Bun Shell — there's no $'...' quoting, so it requires an external printf via command substitution. The realistic case is processing CRLF output with IFS containing \r, where a blank line (\r\n\r\n) yields a spurious empty argv word. Note that dash uses the same space/tab/newline-only interpretation and gives 3 for the test above, so POSIX shells genuinely disagree here — Bun matches dash but not bash. Given the obscurity and the split among reference shells, this shouldn't block merge.
Fix
One line — extend the whitespace set to the full C-locale [:space:] class:
let is_ifs_ws = |b: u8| matches!(b, b' ' | b'\t' | b'\n' | b'\r' | b'\x0c' | b'\x0b') && ifs.contains(&b);An unquoted command substitution whose output splits to a single empty field (e.g. $(echo ,) with IFS=,) must still produce one empty argv word, the same as bash. The previous code left that field in current_out and, with buf/bounds both empty and no has_quoted_empty marker, Cmd::child_done pushed no argument at all. ExpansionOut now carries has_empty_field, set whenever IFS splitting yields at least one field. Cmd::child_done treats it like has_quoted_empty when deciding to push an empty word, so a sole empty field yields one arg while an unset variable still yields none. The flag is separate from has_quoted_empty so it does not affect leading-tilde handling.
|
Thanks for the thorough follow-up. Triage of the four: Sole empty field → zero args (real arity regression, 1→0). Fixed in 6bf6fbc. Edge IFS whitespace against an adjacent literal (
|
| let is_ifs = |b: u8| ifs.contains(&b); | ||
| let is_ifs_ws = |b: u8| matches!(b, b' ' | b'\t' | b'\n') && ifs.contains(&b); |
There was a problem hiding this comment.
🟡 nit: is_ifs tests membership per-byte via ifs.contains(&b), so a multibyte IFS character is treated as a set of independent delimiter bytes. IFS=£ (0xC2 0xA3) makes each byte its own non-whitespace delimiter, so $(echo a£b) yields ["a","","b"] (bash: ["a","b"]), and output containing a different character sharing a byte (e.g. ¢ = 0xC2 0xA2) is torn mid-codepoint into invalid UTF-8. Unlike the deferred \r/\f/\v case this is directly reachable via a literal IFS=£ in source (no $'...' needed) and testable — matches dash but not bash; fine as a follow-up alongside the other deferred edges, worth documenting the byte-oriented semantics.
Extended reasoning...
What the bug is
ifs_split_fields tests IFS membership per-byte:
let is_ifs = |b: u8| ifs.contains(&b);so a multibyte UTF-8 character in IFS is treated as a set of independent delimiter bytes rather than one delimiter character. Each byte then acts as its own non-whitespace IFS delimiter, producing spurious empty fields between them and — worse — matching unrelated characters that share a byte.
Distinct from the deferred \r/\f/\v comment
That comment was deferred on the grounds that the trigger requires those bytes to be in IFS, which Bun Shell can't currently express (no $'...' quoting, and a literal CR is eaten by the lexer). That rationale does not apply here: a non-ASCII IFS is trivially expressible as a literal — IFS=£ in the script source (or await $`IFS=£; ...` from JS) stores the UTF-8 bytes [0xC2, 0xA3] in shell_env with no special quoting. So this path is reachable through the shell surface today and is testable.
Step-by-step: IFS=£; cmd $(printf 'a£b')
ifs = [0xC2, 0xA3], s = [0x61, 0xC2, 0xA3, 0x62].
- Leading-ws skip: no-op (
0x61not IFS-ws).i=0. - Field scan:
!is_ifs(0x61)→i=1.is_ifs(0xC2)(contained inifs) → stop. Push"a". - Delimiter:
is_ifs_ws(0xC2)=false;is_ifs(0xC2)→ consume one byte,i=2; trailing-ws no-op. - Field scan at
i=2:is_ifs(0xA3)→ stop immediately. Push"". - Delimiter: consume
0xA3,i=3. - Field scan: push
"b".
Result: ["a", "", "b"] — 3 argv words with a spurious empty middle. bash 5.2 in a UTF-8 locale gives ["a", "b"] (verified: bash -c 'IFS=£; set -- $(printf "a£b"); echo $#' → 2). For a££b bash gives 3; this gives 5.
The worse case: mid-character splitting
If the substitution output contains a different character sharing a byte with the IFS character, that byte is stripped and the remaining continuation byte becomes an argv word of invalid UTF-8. E.g. IFS=£ (0xC2 0xA3) with output "a¢b" (¢ = 0xC2 0xA2): is_ifs(0xC2) matches, so field 1 = "a", delimiter consumes 0xC2, field 2 = [0xA2, 0x62] — a lone continuation byte 0xA2 glued to b. The ¢ is silently corrupted. bash leaves "a¢b" as one word (no £ present).
Why nothing prevents it
get_ifs returns raw bytes and ifs_split_fields never groups them into characters. There is no ASCII fast-path / WTF-8 slow-path split as do_brace_expand has (Expansion.rs:319-324). Before this PR the path did not exist (IFS was ignored), so it is new to this rewrite in the sense that the arity changes from 1 (old, IFS ignored) to 3 (new) where bash gives 2.
Impact / severity
Nit: requires deliberately setting IFS to a non-ASCII delimiter, which is uncommon. dash has the same byte-wise behavior, so Bun matches dash but not bash — same disclaimer as the \r case. But unlike that case it is directly reachable and testable, and the mid-character-split variant silently produces mojibake in argv with exit 0. Shouldn't block merge given dash parity and the exotic trigger.
Fix
Either (a) document that IFS is byte-oriented (like dash / POSIX C-locale), or (b) when ifs contains a byte ≥ 0x80, decode both ifs and s as WTF-8 code points and match character-wise, mirroring the ASCII/WTF-8 branching in do_brace_expand. A test:
TestBuilder.command`IFS=£; BUN_TEST_VAR=1 ${BUN} -e ${ARGV} $(echo a£b)`
.stdout(`["a","b"]\n`)
.runAsTest("multibyte IFS char is one delimiter");| fn post_subshell_expansion(me: &mut Expansion, mut stdout: Vec<u8>) { | ||
| // Strip a single trailing newline, then convert remaining newlines | ||
| // to spaces. | ||
| if stdout.last() == Some(&b'\n') { | ||
| // Command substitution deletes trailing newlines before field splitting. | ||
| while stdout.last() == Some(&b'\n') { | ||
| stdout.pop(); | ||
| } |
There was a problem hiding this comment.
🟡 Nit: this rewrite now strips only trailing \n from unquoted $(...) (previously [ \t\r\n]), while the quoted path in child_done still trims trailing \r/space/tab — so "$(tool)" strips a CRLF-terminating \r but $(tool) keeps it, the reverse of what quoting normally implies. The new unquoted behavior is bash-correct so it shouldn't block, but it's worth deciding: either also strip trailing \r here (while matches!(stdout.last(), Some(b'\n' | b'\r'))) as a documented cross-platform deviation for Windows tools that emit CRLF (where.exe, MSVCRT-linked CLIs), or align the quoted path to POSIX (strip only trailing \n) so the two are consistent.
Extended reasoning...
What changed
The old post_subshell_expansion trimmed leading/trailing b' ' | b'\n' | b'\r' | b'\t' before splitting. The rewrite now strips only trailing newlines:
while stdout.last() == Some(&b'\n') {
stdout.pop();
}then either appends verbatim (single / empty-IFS) or field-splits on IFS (default b" \t\n", which does not contain \r). So an unquoted $(cmd) whose output ends in CRLF now yields an argv word with a trailing \r where it previously did not. This is POSIX-correct — verified: bash -c 'set -- $(printf "a\r\n"); printf "%s" "$1" | od -c' retains the \r. And bash-compat is the PR's stated goal, so on its own this would just be a "note the behavior change" item.
The internal inconsistency
The stronger point is that the quoted path in child_done was not touched and still does:
while hi > 0 && matches!(stdout[hi - 1], b' ' | b'\n' | b'\r' | b'\t') {
hi -= 1;
}So after this PR, "$(tool)" strips a trailing \r while $(tool) keeps it — the opposite of what a user would expect from adding quotes (quoting normally reduces trimming/mangling, never increases it). The two code paths now disagree about what "remove trailing newlines from a command substitution" means.
Step-by-step trace
Take cmd $(tool) where tool prints "path\r\n" (any Windows-native CLI: where.exe, PowerShell, cmd /c echo, or an MSVCRT-linked binary), with default IFS:
stdout = b"path\r\n".while stdout.last() == Some(&b'\n')→ pop once →b"path\r".\r != \n, loop stops.- Not empty, not
single, IFS unset →ifs_bytes = b" \t\n". ifs_split_fields(b"path\r", b" \t\n"):\ris not in the IFS set, so the field scan runs to end. Returns[b"path\r"].current_out = b"path\r".cmdreceives argv["path\r"].
Before this PR: after stripping the \n and converting newlines→spaces, the trim loop matched \r and dropped it → cmd received ["path"].
Compare the quoted spelling cmd "$(tool)" after this PR: child_done's while hi > 0 && matches!(stdout[hi-1], b' ' | b'\n' | b'\r' | b'\t') decrements hi twice → cmd receives ["path"]. Quoted strips more than unquoted.
The single: true path (assignments, redirect targets, [[ ]] operands — new in 747b149) hits the same shape: it appends verbatim after \n-strip only, so VAR=$(tool) and > $(tool) also gain a trailing \r they did not have before.
Why it matters (and why it's still a nit)
Bun Shell explicitly targets cross-platform scripting. On Windows, $(where.exe node), $(cmd /c echo %CD%), and any MSVCRT-linked tool print CRLF. Before this PR the \r was silently stripped and the result was directly usable as a path/arg; after, every such substitution carries an invisible \r that becomes "file not found" / "command not found" downstream, exit ≠ 0, no hint that a stray CR is the cause.
That said: (1) the new unquoted behavior is exactly bash-correct, which is what the PR set out to do; (2) the old \r-trimming was itself the deviation; (3) the quoted-path over-trim is pre-existing code the PR didn't touch. So this shouldn't block merge — it just deserves a conscious decision rather than an accident.
Fix options
Either:
- (a) Also strip trailing
\rhere as a deliberate cross-platform deviation from POSIX, matching what the quoted path already does:while matches!(stdout.last(), Some(b'\n' | b'\r')) { stdout.pop(); }. This keeps$(where.exe node)working on Windows. - (b) Accept the bash-correct behavior, and align the quoted path in
child_doneto POSIX (strip only trailing\n) so at least the two are consistent — and call out the CRLF implication in release notes.
Either way the two paths should agree.
…e words Making ExpansionOpts.single live (747b149) activated the pre-existing single: true at the redirect-target call site, so an unquoted command substitution in redirect position took the verbatim path and surrounding whitespace leaked into the filename: echo hi > $(echo " foo ") created a file named " foo " instead of foo, diverging from both released Bun and bash. bash field-splits redirect words (and errors on more than one field); only assignment values and [[ ]] operands are exempt. Pass single: false at the redirect call site so targets go through ifs_split_fields like argv words, restoring the pre-change behavior, and drop redirect targets from the single-word doc comments.
|
Thanks. Triage of this round: Redirect target regression ( Multibyte IFS byte-oriented ( Trailing non-whitespace IFS byte vs adjacent literal ( Trailing Net: the one real regression is fixed; the rest are narrow POSIX/bash refinements and a pre-existing inconsistency, tracked for a focused follow-up. |
| for field in rest { | ||
| me.current_out.extend_from_slice(field); | ||
| Self::push_current_out(me); | ||
| } | ||
| me.current_out.extend_from_slice(&s[a..]); | ||
| me.current_out.extend_from_slice(last); |
There was a problem hiding this comment.
🟣 Pre-existing (not introduced by this PR), noting for the deferred edge-delimiter follow-up: in a compound word ~$(cmd) where the substitution field-splits into ≥2 fields, the leading ~ lands on the last field instead of the first — bash gives ~$(printf "a b") → ["~a","b"], Bun gives ["a","~b"]. post_subshell_expansion flushes all-but-last fields via push_current_out mid-walk, leaving only the last field in current_out; the deferred leading-tilde post-processing in next() then operates solely on current_out. When the follow-up restructures how post_subshell_expansion reports boundaries, it should also route the tilde onto the first committed field rather than current_out.
Extended reasoning...
What the bug is
In a compound word ~$(cmd), when the unquoted $(cmd) field-splits into two or more fields, the leading ~ is attached to the last field instead of the first. bash 5.2:
$ bash -c 'set -- ~$(printf "a b"); printf "[%s]" "$@"'
[~a][b]Bun (both before and after this PR) produces ["a","~b"]. The rewrite widens reachability slightly — with the new custom-IFS support, IFS=,; ~$(printf ",a") gives ["","~a"] where bash gives ["~","a"] — but the underlying defect is the same.
Code path
next() (Expansion.rs:186-189) detects leading_tilde for a compound atom starting with SimpleAtom::Tilde and sets word_idx = 1, deferring tilde handling until after the atom walk. The walk then hits the CmdSubst atom, spawns a Script, and on child_done calls post_subshell_expansion. For a multi-field result, the for field in rest loop (Expansion.rs:656-659) calls Self::push_current_out(me) once per non-last field — flushing each into out.buf and clearing current_out — and leaves only the last field in current_out. Back in next() with word_idx >= atoms_len, the leading-tilde post-processing block (Expansion.rs:246-273) runs match me.current_out.first() and does me.current_out.insert(0, b'~') (or the HOME splice) — on current_out, which now holds only the last field. The already-committed first field in out.buf never sees the tilde.
Why nothing prevents it
The tilde post-processing was written under the assumption that current_out still holds the entire expanded word when it runs. That assumption breaks whenever post_subshell_expansion flushes mid-walk. leading_tilde is not consulted inside post_subshell_expansion, and there is no mechanism to prefix the tilde onto the first committed field rather than the residual current_out.
Step-by-step: ~$(printf "a b")
- Compound atom =
[Tilde, CmdSubst].leading_tilde = true→word_idx = 1. - CmdSubst runs, stdout =
"a b". Not quoted →post_subshell_expansion. ifs_split_fields("a b", " \t\n")→["a","b"].split_last()→last="b",rest=["a"].restloop:current_out = "a";push_current_out(me)→out.buf = "a",out.bounds = [](first commit),out.committed = true,current_outcleared.current_out.extend("b")→current_out = "b". Return.next()re-enters,word_idx = 2 >= atoms_len. Leading-tilde block:current_out.first() = Some(b'b')→Some(_)arm →current_out.insert(0, b'~')→current_out = "~b".push_current_out→out.buf = "a~b",out.bounds = [1].Cmd::child_donereconstructs["a","~b"]. Bash:["~a","b"].
Why this is pre-existing
The old post_subshell_expansion (removed in this diff) also called Self::push_current_out(me) on each interior space and left only the trailing segment in current_out via the final extend_from_slice(&s[a..]). So ~$(echo "a b") already produced ["a","~b"] on main via default-IFS whitespace splitting. The rewrite preserves the same flush-then-postprocess ordering; it does not introduce the defect.
Impact and fix
Narrow trigger: a compound word starting with a literal ~ immediately followed by an unquoted $(...) that produces multiple fields. Not blocking for this PR.
Flagging so the deferred edge-delimiter follow-up (which will restructure how post_subshell_expansion reports boundaries to its caller) can also route the deferred tilde onto the first committed field. One option: when leading_tilde is set, prepend the ~/HOME to current_out before the first push_current_out call inside post_subshell_expansion (i.e., handle it at the point the first field is about to be committed), rather than after the walk completes.
Two follow-ups from review on the IFS field-splitting change:
- Brace expansion now drops unquoted-null variants to match bash:
{,a} -> a, {a,} -> a, {,} -> nothing, {a,,b} -> a b. The earlier
committed-flag change preserved a leading empty variant, which
diverged from both bash and released Bun; empty variants are now
filtered before committing words.
- A redirect target that field-splits into more than one word is now an
ambiguous redirect (exit 1, no file created), matching bash, instead
of gluing the fields together or dropping a delimiter byte. The
ExpandingRedirect handler leaves redirection_file empty on a
multi-field result so the existing ambiguous-redirect check fires.
|
Both confirmed against bash 5.2 and fixed in 0e1f46a: Brace empty variants. Redirect multi-field → ambiguous. Right call, and it subsumes the pre-existing whitespace-glue too. A redirect target that field-splits into more than one word is now an ambiguous redirect (exit 1, no file created), matching bash, rather than gluing ( Leading |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/runtime/shell/states/Expansion.rs`:
- Around line 592-594: The IFS handling in ifs_split_fields() is dropping
separators based only on the substitution fragment, which causes
boundary-touching whitespace to be lost and words to collapse incorrectly.
Update the splitting logic in Expansion.rs to consider the in-flight
word/boundary state when deciding whether IFS whitespace should split or be
preserved, rather than trimming the fragment in isolation. Use
ifs_split_fields() and the surrounding expansion path as the fix point, and add
regression tests for both a$(printf " b") and $(printf "a ")b.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: f9e07c94-ce14-487f-8c9e-6696bb6fface
📒 Files selected for processing (4)
src/runtime/shell/states/Assigns.rssrc/runtime/shell/states/Cmd.rssrc/runtime/shell/states/Expansion.rstest/js/bun/shell/bunshell.test.ts
IFS separators at the edge of an unquoted command-substitution result now break the word against an adjacent literal atom instead of gluing onto it: a$(printf " b") -> [a, b], $(printf "a ")b -> [a, b], a$(printf " ")b -> [a, b], and with IFS=, $(echo a,)x -> [a, x]. The edge-whitespace case was pre-existing; the trailing non-whitespace case (delimiter byte dropped) was introduced by IFS splitting. Both now match bash. ifs_split_fields reports whether a separator touched each edge of the input. post_subshell_expansion flushes any literal prefix in current_out as its own word when a leading separator was consumed, and commits the last field when a trailing one was; the walk-end flush in next skips the now-empty current_out.
|
Fixed in 714f513 rather than leaving it to a follow-up, since this was flagged by two reviewers and one variant is a byte-loss I introduced.
Verified against bash 5.2 across: This also covers the "trailing non-whitespace IFS byte vs adjacent literal" comment. |
|
Closing as part of a cleanup of stale pull requests. This PR has had no new commits since 2026-07-07, it conflicts with main, and its last CI run failed. This is not a judgment on the fix itself. If the problem still reproduces on a current build, reopen this PR after a rebase or open a new one against main. |
1 participant
What
Bun Shell splits an unquoted command substitution into argv words on a hardcoded
is space || is newlinepredicate and never consults theIFSshell variable. This produces the wrong argv arity in both directions:IFSis ignored.IFS=:/IFS=,/ a newlineIFS(the standard "split this output on my delimiter" idioms) all return one glued-together argument.IFSis<space><tab><newline>, but TAB was never a separator, so tab-separated tool output (cut,awk, TSV pipelines) never split.IFS=(empty) is documented to disable field splitting, but splitting still happened on spaces.Everything was exit 0 with no diagnostic; programs just received the wrong number of arguments.
Repro
Cause
post_subshell_expansioninsrc/runtime/shell/states/Expansion.rsconverted newlines to spaces, trimmed, then split on runs of spaces. It never readIFS.Fix
Implement POSIX field splitting in the word-expansion pass:
IFSfrom the shell env (then exported env); default to" \t\n"when unset.IFSdisables field splitting.IFS) runs collapse into one delimiter and leading/trailing IFS whitespace is ignored.Representing a leading empty field required tracking whether a word had already been committed to the output buffer, rather than inferring it from buffer emptiness (an empty first word leaves the buffer empty).
Note: the
IFS=$'\n'form in the report relies on ANSI-C$'...'quoting, which Bun Shell does not implement (it leaves the bytes literal). A real newline inIFSsplits correctly; that is a separate feature gap, not part of this fix.Verification
New tests in
test/js/bun/shell/bunshell.test.ts(describeIFS field splitting) assert exact argv arity viaprocess.argv.slice(1)for custom IFS, empty IFS, default TAB, whitespace-run collapse, empty fields (leading/middle/consecutive), and the quoted-substitution control. They fail on the released binary and pass with the fix. Output matches bash 5.2 across the matrix.