Fix TOML multi-line string trimming - #28682
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughTrimmed initial newlines after multi-line TOML string delimiters and improved backslash-continuation handling in the lexer: line endings plus following whitespace are skipped and iterator state updated. Added regression tests covering basic and literal multi-line string behaviors and escape sequencings. Changes
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
All three previously flagged bugs have been addressed in 0b59e55. LGTM.
Extended reasoning...
Overview
This PR fixes TOML multi-line string parsing in src/interchange/toml/lexer.zig: adds trimMultilineStart() to strip the leading newline after opening """ / '' delimiters, and rewrites the backslash line-continuation logic in decodeEscapeSequences to consume all subsequent whitespace. A regression test file (test/regression/issue/28680.test.ts) covers the new behavior.
Previous Review Findings — All Addressed
My prior review flagged three issues, all resolved in commit 0b59e55:
- Bare CR trimmed incorrectly —
trimMultilineStartnow returnscontent_startunchanged for bare CR (not followed by LF), as required by spec. \t/\fescape values swapped —\tnow correctly emits0x09and\femits0x0C; a test case was added.- Outer
\rCRLF lookahead used wrong index — now correctly usesiter.i + iter.widthto peek at the next byte.
Security Risks
No security-sensitive code paths are touched. This is a TOML parser fix affecting string value decoding only.
Level of Scrutiny
Moderate — parser logic changes require care, but the scope is narrow and well-tested. The fixes follow the established iterator pattern already present in the file. The pre-existing 4-5 closing delimiter bug is not introduced or worsened by this PR.
Other Factors
The CI failure (azure-service-bus SIGILL on asan) is unrelated to these changes. All inline comments are resolved. The fix correctly handles the TOML v1.0.0 spec requirements for multi-line string parsing.
0b59e55 to
a1f5989
Compare
There was a problem hiding this comment.
All prior feedback is addressed and the new space/tab lookahead arm in f3a477d traces correctly against the Cursor semantics — deferring to a human for final sign-off since this is ~80 lines of manual iterator-state manipulation in the lexer, and build-rust is currently red on the latest commit.
Extended reasoning...
Overview
This PR fixes TOML v1.0.0 multi-line string handling in src/parsers/toml/lexer.rs: a new trim_multiline_start helper strips the leading LF/CRLF after """/''', the line-ending-backslash paths now consume all trailing whitespace, the outer \r CRLF-normalization lookahead is corrected to use iter.i + iter.width, the \t/\f escape values are unswapped, and (in f3a477d) a new ' ' | '\t' arm peeks past trailing whitespace between a backslash and the newline per the mlb-escaped-nl ABNF. Tests were moved from test/regression/issue/ into test/js/bun/resolve/toml/toml.test.js per project convention, with six new it(...) blocks.
Review history
I have reviewed this PR twice before. All five issues I raised (bare-CR trimming, \t/\f swap, broken outer-CRLF lookahead, test-file placement, and backslash-then-trailing-whitespace) have been fixed in 0b59e55 / 2c592a0 / f3a477d, and the 4/5-closing-delimiter pre-existing bug was reasonably declined as out of scope. I re-traced the f3a477d arm against CodepointIterator::next (src/bun_core/string/immutable.rs:315) — iter.i points at the start of the just-read char and next() resumes at iter.i + iter.width, so the peek = iter.i + iter.width lookahead and the iter.width = 0 reset are both correct. The fall-through path (\ + space not followed by a newline) preserves the prior lenient behavior. The bug-hunting system found nothing new this round.
Security risks
None. This is pure lexer string-decoding logic with no I/O, auth, or external-input trust boundaries beyond what already existed; all new index arithmetic is bounds-checked.
Level of scrutiny
Medium-high. The change is well-scoped and well-tested, but it is ~80 net lines of hand-rolled cursor position/width manipulation across four interacting match arms in a user-facing parser (Bun.TOML.parse and .toml imports). Off-by-one errors here are easy to introduce and hard to spot.
Other factors
The robobun CI comment shows build-rust failing on several targets for commit f3a477d (plus an agent-creation failure). I do not see anything in the diff that would obviously fail to compile, so this may be infrastructure, but I will not approve over a red build. Combined with the non-trivial iterator logic, this should get a human pass before merge.
There was a problem hiding this comment.
LGTM — all prior feedback addressed, cursor arithmetic verified against CodepointIterator::next.
Extended reasoning...
Overview
This PR fixes TOML v1.0.0 multi-line string handling in src/parsers/toml/lexer.rs: it adds trim_multiline_start to skip a leading LF/CRLF after """/''', rewrites the line-ending-backslash arms in decode_escape_sequences to consume all subsequent whitespace, adds a new arm for backslash + trailing spaces/tabs before a newline (per ABNF mlb-escaped-nl), fixes the pre-existing \t/\f value swap, and corrects the outer \r CRLF-normalization lookahead. Six new test cases were added to test/js/bun/resolve/toml/toml.test.js.
Security risks
None. This is pure-function lexing of already-loaded byte slices: no I/O, no allocation outside the existing arena, no unsafe code, and all new index arithmetic is bounds-checked before dereference. No injection, auth, or data-exposure surface.
Level of scrutiny
Moderate — parser code with manual cursor manipulation is error-prone, so I traced every new iter.i / iter.width mutation against NewCodePointIterator::next (unicode.rs:247-288), which sets cursor.i to the start of the just-decoded codepoint and cursor.width to its byte length. The PR's iter.i + iter.width lookahead and iter.width = 0 reset match the canonical pattern already used in the SIMD scan path (unicode.rs:223-225). The \r, \n, and space/tab continuation arms all advance correctly and re-enter the loop at the right byte.
Other factors
This PR has been through three review rounds; every issue I raised (bare-CR trimming, \t/\f swap, outer CRLF lookahead, test-file placement, whitespace-before-newline ABNF gap) has been fixed and is covered by a test. The one item declined as out-of-scope (4/5 closing delimiters) is a pre-existing bug unrelated to the opening-delimiter logic this PR touches. No CODEOWNERS apply to src/parsers/toml/, and the bug-hunting system found nothing new on the latest revision.
25ec3ac to
aba1bc4
Compare
There was a problem hiding this comment.
LGTM — all prior feedback addressed; the remaining inline note is a pre-existing cosmetic offset in error positions and not blocking.
Extended reasoning...
Overview
This PR fixes TOML v1.0.0 spec compliance for multi-line strings in src/parsers/toml/lexer.rs: trimming the leading newline after """/''' delimiters, consuming all whitespace after a line-ending backslash (including the escape ws newline ABNF case), fixing the pre-existing \t/\f value swap, and correcting the CRLF lookahead in the outer \r handler. Six new test cases in test/js/bun/resolve/toml/toml.test.js cover the spec examples directly. The <cassert> include in wtf-bindings.cpp is an unrelated one-line build fix for the existing assert() call in uv__tty_make_raw.
Security risks
None. This is string-content lexing for TOML config/import files — no auth, crypto, permissions, or command execution. The new code only adjusts byte-index cursors within already-bounded slices; all indexing is guarded by < text.len() checks, and trim_multiline_start bounds-checks before reading.
Level of scrutiny
Moderate. The TOML lexer feeds bunfig.toml and TOML imports, so correctness matters, but this PR has now been through four review passes. Earlier rounds caught and fixed a bare-CR over-trim, the \t/\f swap, the broken outer CRLF lookahead, the escape ws newline gap, test-file placement, and the hex-literal consistency oversight — all confirmed resolved in the current diff. The iterator-state manipulation (iter.i/iter.width = 0) is the subtle part and I've re-read it against CodepointIterator::next semantics; it's correct.
Other factors
- The one finding from this run is a pre-existing 2–4 byte skew in diagnostic column positions for malformed escapes inside
"""…"""strings — purely cosmetic, root cause predates the PR. - CI failures in the robobun comment are all in unrelated files (
HiveArrayFallbackdeprecations, FreeBSDspawn/Futexwarnings, musl build) and not caused by this change. - No CODEOWNERS for the touched paths.
- The author explicitly deferred the unrelated 4/5-closing-delimiter spec gap as out-of-scope, which is reasonable.
0c50110 to
82a86d9
Compare
There was a problem hiding this comment.
LGTM — all prior feedback addressed, iterator-reposition logic verified against Cursor::next semantics, and the new test cases cover the edge cases.
Extended reasoning...
Overview
This PR fixes TOML v1.0.0 spec compliance for multi-line strings in src/parsers/toml/lexer.rs: (1) a new trim_multiline_start helper trims a leading LF/CRLF after """ / ''', (2) the line-ending-backslash handling in decode_escape_sequences now consumes all subsequent whitespace (and accepts trailing ws between the backslash and the newline per the ABNF), (3) escape values for \t/\f are corrected and all single-char escapes converted to hex literals, and (4) slice_lo is now passed as the diagnostic base so error columns line up with the slice origin. Six new test cases were added to test/js/bun/resolve/toml/toml.test.js.
Security risks
None. This is read-only lexing of TOML input into string values; no auth, crypto, filesystem, or network surface. All new byte indexing is bounds-checked (pos < text.len() / peek < text.len()), and Rust slice indexing panics rather than reading OOB, so there is no memory-safety exposure.
Level of scrutiny
Moderate. The TOML lexer feeds bunfig.toml and .toml imports, so correctness matters, but the change is narrowly scoped to multi-line string slice bounds and a whitespace-consuming loop inside the escape decoder. The only subtle part — repositioning the cursor via iter.i = pos; iter.width = 0; so the next iterator.next() resumes at pos — I verified directly against NewCodePointIterator::next in src/bun_core/string/immutable.rs (which computes pos = cursor.i + cursor.width), and the same reposition pattern is already used elsewhere in that file.
Other factors
This PR has been through four prior review rounds; every issue raised (bare-CR trim, \t/\f swap, CRLF lookahead off-by-one, test-file placement, ws-before-newline ABNF gap, hex-literal consistency, diagnostic-base offset) has been fixed and the threads resolved. The bug-hunting pass on the current revision found nothing. Test coverage includes the no-leading-newline case, pure-continuation strings, ws-between-backslash-and-newline, and the \t/\f escape values. No CODEOWNERS entry covers src/parsers/toml/. Given the accumulated review history and verified correctness, I'm comfortable approving without further human review.
Trim the first newline after opening """ and ''' delimiters per TOML v1.0.0 spec. Fix backslash line continuation to consume all subsequent whitespace (spaces, tabs, newlines) until the next non-whitespace character. Fix swapped \t / \f escape values and the outer \r handler's CRLF lookahead in decode_escape_sequences.
Per test/CLAUDE.md: test/regression/issue/ is only for true regressions (worked in a previous release, then broke). #28680 is a TOML spec-compliance gap that was never correct, so the tests belong in test/js/bun/resolve/toml/toml.test.js.
…rings Per TOML v1.0.0 ABNF `mlb-escaped-nl = escape ws newline *( wschar / newline )`, trailing spaces/tabs between a line-ending backslash and the newline are valid and should be consumed along with the rest of the continuation.
debian-13-x64-asan-test-bun failed with exit status -1 (agent lost) on autofix commit — infrastructure flake. All build-rust steps green.
Consistency nit: the other escape pushes (\b, \n, \v, \r) use 0x0X hex, while \f and \t still used decimal 12 and 9.
…nces Error positions inside decode_escape_sequences are computed as base + iter.i, where iter.i is an offset into the sliced text. Passing `start` (position after the first quote) misaligned diagnostics for multi-line basic strings by 2-4 bytes — the difference between `start` and the actual slice origin (`start + 2` plus any trimmed LF/CRLF). Pass `slice_lo` so the column points at the real source byte.
82a86d9 to
cf642c2
Compare
There was a problem hiding this comment.
LGTM — all prior review feedback addressed; the remaining inline nit is just a CRLF test-coverage suggestion and non-blocking.
Extended reasoning...
Overview
This PR fixes two TOML v1.0.0 spec-compliance gaps in src/parsers/toml/lexer.rs: (1) trimming the leading newline immediately after """ / ''' opening delimiters via a new trim_multiline_start helper, and (2) making line-ending backslashes consume all subsequent whitespace (and, per the ABNF, trailing whitespace between the backslash and the newline). Along the way it also fixes two pre-existing bugs surfaced during review — the swapped \\t/\\f escape values and the broken CRLF lookahead in the outer \\r handler — and corrects the diagnostic base offset passed to decode_escape_sequences. Six new tests in test/js/bun/resolve/toml/toml.test.js cover the spec examples plus the escape-value fix.
Security risks
None. This is pure TOML string-content parsing with no auth, crypto, filesystem, or network surface. All new byte indexing is bounds-checked (pos < text.len(), content_start + 1 < contents.len()), and Rust slice indexing panics rather than reading out of bounds, so there is no memory-safety exposure.
Level of scrutiny
Medium — the iterator-cursor manipulation (iter.i / iter.width resets) is the only subtle part. I verified against bun_core::string::immutable::Cursor::next that next() advances to cursor.i + cursor.width and then sets cursor.i to that position, so the new code's pattern of writing iter.i = pos; iter.width = 0 correctly resumes iteration at pos. The \\r, \\n/U+2028/U+2029, and space/tab line-continuation arms each compute the resume point consistently with those semantics. trim_multiline_start is fully bounds-checked and correctly leaves bare CR untrimmed.
Other factors
This PR has been through four prior review rounds; every issue raised (bare-CR trim, \\t/\\f swap, CRLF double-newline, test-file placement, escape ws newline ABNF gap, hex-literal consistency, diagnostic base offset) has been fixed and the threads resolved. No CODEOWNERS cover these paths. The only finding from this run is a non-blocking nit suggesting explicit CRLF test cases for the new branches — worth adding but not a reason to hold the merge.
Template literals normalize to LF so the existing tests never hit the CRLF branches of trim_multiline_start or the backslash-CR lookahead.
There was a problem hiding this comment.
LGTM — all prior feedback addressed, CRLF coverage added in a9b01a2.
Extended reasoning...
Overview
This PR fixes two TOML v1.0.0 spec-compliance gaps in Bun's TOML lexer (src/parsers/toml/lexer.rs): trimming the newline immediately after """ / ''' opening delimiters, and making line-ending backslashes consume all subsequent whitespace. Along the way (in response to review), it also fixes a pre-existing \\t/\\f value swap, a CRLF lookahead off-by-one in the outer \\r handler, the diagnostic-column base for multiline escapes, and adds the mlb-escaped-nl trailing-whitespace case. Seven new tests in test/js/bun/resolve/toml/toml.test.js cover LF, CRLF, literal vs basic strings, and escape codepoints.
Security risks
None. This is pure lexer logic over byte slices with explicit bounds checks (pos < text.len(), content_start + 1 < contents.len()) on every read; no FFI, no allocation patterns that could overflow, no untrusted-path or auth surface. Worst-case impact of a defect here is a mis-parsed TOML string value.
Level of scrutiny
Moderate — it's a parser change and TOML backs bunfig.toml, but the change is narrowly scoped to multi-line string content-start/whitespace handling and doesn't touch tokenization of other value types. This PR has been through eight rounds of bot review with every finding (bare-CR trim, \\t/\\f swap, CRLF lookahead, test placement, escape ws newline ABNF gap, hex-literal consistency, slice_lo diagnostic base, CRLF test coverage) addressed and resolved. The remaining flagged item (4–5 closing delimiters) is a pre-existing, orthogonal end-position bug the author reasonably scoped out.
Other factors
The current bug-hunting pass found nothing new. The added tests directly exercise every new branch including the CRLF paths added in a9b01a2. CI failures are unrelated infrastructure (musl LTO data-layout linking) and a flaky bunx.test.ts, neither touched by this diff. No CODEOWNERS cover src/parsers/toml/. Net: well-understood, well-tested spec-compliance fix with all feedback closed out.
|
#32994 also fixes both issues here (the leading-newline trim and the line-ending backslash), validated against the official toml-test 1.0 corpus, and additionally handles the cases this diff does not: a backslash followed by trailing whitespace before the newline (valid per the |
|
The TOML parser was rewritten for v1.1.0 conformance in #32953, which landed on main (aca54d5) and closed #28680. It deletes The behavior this PR fixes is covered on current main: all 7 tests added here (first-newline trimming for basic and literal multi-line strings, line-ending backslash including whitespace before the newline, CRLF variants, and the |
1 participant
Problem
Bun's TOML parser does not properly handle multi-line strings per the TOML v1.0.0 spec:
"""or'''delimiter should be trimmed, but was kept.\is the last non-whitespace character on a line, it and all subsequent whitespace (including newlines) should be consumed until the next non-whitespace character. Only the immediate newline was skipped.Reproduction
Fix
trimMultilineStarthelper that skips\nor\r\nimmediately after the opening delimiter for both"""and'''strings.decodeEscapeSequencesto consume all subsequent whitespace characters (spaces, tabs, newlines, carriage returns) after a line-ending backslash.Verification
USE_SYSTEM_BUN=1 bun test test/regression/issue/28680.test.ts→ 3 fail, 1 passbun bd test test/regression/issue/28680.test.ts→ 4 passCloses #28680