Repository navigation
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 (1)
Included review availability: Your plan provides up to 5 included reviews per hour; 1 remains after this review. WalkthroughThe lexer now scans comment pragmas from direct source slices and tracks absolute offsets. JSX and source-map pragma locations use the new offset model. A concurrent regression test verifies exact ChangesPragma location tracking
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Full details: Description checkExplanation The description explains the problem, implementation, behavioral impact, and verification steps. It does not use the exact template headings, but it provides the required information in equivalent sections. Comment |
|
Status: ready for review. Rebased on main after #40610 (one squashed commit, Reproduced on bun 1.4.0 with a The new test in CI on the rebased head (build 107061): 179 of 181 jobs green. The two red jobs are outside this change and CI marks both as pre-existing on main: a LeakSanitizer report in |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — corrects the doubled-offset arithmetic so JSX pragma warnings point at the argument, matching esbuild.
What was reviewed:
- Traced
chunk_startfor both callers:self.currentafterstep()in the//path, andself.start + (end_comment_text - rest.len())in the/* */path — both land on the byte after@/#. - Verified the removed
matcharm and.min()oni + 1were dead:index_of_anyonly returns in-bounds indices of@/#. - Hand-checked the test's expected offsets (41, 42, 51) and columns against the fixture strings; all three comment shapes are covered with exact assertions.
Extended reasoning...
Overview
This PR fixes incorrect source positions in JSX pragma warnings (Unsupported JSX runtime: "..."). The lexer was double-counting offsets when computing the location of a pragma argument in a comment: scan_pragma added self.start (the comment start) to a value that was already absolute in the // path, and the /* */ path additionally counted the @ index twice via i + (text.len() - rest.len()). The fix unifies both callers to pass a single chunk_start — the absolute source offset of the byte immediately after @/# — and PragmaArg::scan / scan_source_mapping_url_value add only pragma.len() + arg_index to it. Two pieces of dead code in the block-comment loop (a match on a byte that can only be @ or #, and a .min(rest.len()) on i + 1 where i < rest.len() by construction) are removed.
Security risks
None. This affects only the Range stored on pragma spans, which is used exclusively for warning locations. Pragma text (what actually configures JSX) is unchanged. No user input reaches new code paths; slice indices are all provably in bounds.
Level of scrutiny
Low-medium. The change is localized to diagnostic position arithmetic in the lexer's comment-scanning path. I traced the offset arithmetic by hand for all three test fixtures (line comment at offset 26 → argument at 41; single-line block comment → 42; multi-line block comment with a preceding @param → 51) and they match the test's indexOf("bogus") assertions. The PR description cross-checks against esbuild 0.25.1 output.
Other factors
The new test is well-constructed: it uses tempDir, covers the //, single-line /** */, and multi-line /** */ with a prior @ tag, and asserts exact line/column/offset/length/lineText values keyed by filename (order-independent via Object.fromEntries). Each fixture places the pragma after other source text so the old double-counting would land on a later line — the test would fail on Bun 1.4.0 with offsets 66/74/84 per the PR description. source_mapping_url gets the same signature change but has no reader, so it's untested (noted in the PR). The removed .min() and match are safe deletions given index_of_any's contract.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Updated 3:53 PM PT - Aug 27th, 2026
❌ @robobun, your commit f48904a has 2 failures in
🧪 To try this PR locally: bunx bun-pr 39636That installs a local version of the PR into your bun-39636 --bun |
There was a problem hiding this comment.
LGTM — the offset arithmetic checks out and the test pins exact positions.
What was reviewed:
- Traced the new
scan_comment_textloop against the doc-comment fixture (@paramthen@jsxRuntime):self.start + poslands on offset 51, matchingindexOf("bogus"). - Confirmed
&contents[chunk_start..]in the//path uses the localcontents: &'a [u8]copy, so the borrowck workaround (and its per-@StoreStrallocation) is genuinely unnecessary. remaining()has no other callers; the.min(chunk.len())clamp on the block-comment advance is preserved.
Extended reasoning...
Overview
Fixes the source location reported for JSX pragma warnings (Unsupported JSX runtime: "…"). The old scan_pragma added self.start to an offset that both callers already passed as absolute-ish, and the block-comment caller additionally counted the @ index twice. The fix collapses the two-parameter offset scheme into one chunk_start (absolute source offset of the byte after @/#), threads it unchanged through PragmaArg::scan and scan_source_mapping_url_value, rewrites the scan_comment_text pragma loop to track that offset directly, and lets the // caller slice from its local contents instead of round-tripping through StoreStr. Lexer::remaining is removed as dead. A new bun-build-api.test.ts case asserts exact offset/line/column/lineText for line, one-line block, and multi-line doc comments.
Security risks
None. This only changes the Range stored on jsx_pragma spans, which is used solely for warning locations in prepare_for_visit_pass. The pragma text (what actually configures JSX behavior) is computed from the same chunk slice as before. No untrusted-length arithmetic feeds an index — the one slice bound that could exceed the chunk (scan_pragma's return) keeps its .min(chunk.len()) clamp.
Level of scrutiny
Medium. This is inside the JS lexer, which is hot and correctness-critical, but the change is confined to diagnostic position bookkeeping and a small loop rewrite whose iteration structure is equivalent to the old one (index-of-any → advance past marker → scan → advance by consumed length). I hand-traced the doc-comment case (two @ markers, second on a later line) through the new loop and got offset 51, matching the test's indexOf("bogus"). The // path's borrow of &contents[chunk_start..] is sound because contents is a local &'a [u8] copy (line 2358), the same pattern the rest of scan_single_line_comment already uses — so dropping the StoreStr detour is safe and removes a per-@ arena copy of the rest of the file.
Other factors
The comment-cop bot flagged five long comments; all threads are resolved by 660ed00 and daa222a and the final diff carries only one-line docs. remaining() has no remaining callers (grep clean). The test uses tempDir+using, test.concurrent, and asserts exact values keyed by filename, so it fails loudly on the old offsets (66/74/84 vs 41/42/51) and covers the multi-@ block-comment path that exercises the loop's pos accumulation. PR description cross-checks against esbuild 0.25.1 output. Related bundler/transpiler suites were run per the notes.
There was a problem hiding this comment.
LGTM — the offset arithmetic is now a single, traceable formula and only diagnostic ranges change.
What was reviewed:
- Traced
chunk_startthrough both callers:self.start + posinscan_comment_textandself.currentinscan_single_line_commentboth land on the byte after@/#, andPragmaArg::scanaddspragma.len() + startto reach the argument — hand-checked against all three test fixtures. - Confirmed the local
contents: &'a [u8]inscan_single_line_commentavoids the borrowck conflict that motivated the oldStoreStrdetour, and thatremaining()had no other callers. - Checked the rewritten
scan_comment_textloop preserves the advance-and-clamp behavior (pos += i + 1is always in bounds;.min(chunk.len())on the pragma advance stays).
Extended reasoning...
Overview
This PR fixes incorrect source locations in JSX pragma warnings. The bug was double-counting: scan_pragma added self.start (the comment's absolute start) to an offset that was already absolute in one caller and mis-derived in the other. The fix collapses the two-parameter (start, offset_for_errors) model into a single chunk_start — the absolute source offset of the byte immediately after @/# — and threads it through scan_pragma, PragmaArg::scan, and scan_source_mapping_url_value. The // caller now slices directly from its local contents: &'a [u8] instead of round-tripping through an arena-allocated StoreStr, which also lets the unused Lexer::remaining() helper go. The block-comment loop in scan_comment_text is rewritten to track a single pos cursor into the comment body, dropping a dead match arm and a redundant .min().
Security risks
None. This touches only the computation of the Range attached to a pragma Span; the span's text (which drives which JSX runtime is selected) is unchanged. All indexing is into slices already bounded by the comment body or the source contents, and the one place where scan_pragma's return could theoretically exceed the chunk length is still clamped with .min(chunk.len()).
Level of scrutiny
Medium. The lexer is hot-path code, but the change is arithmetic on diagnostic offsets — the pragma text and the number of bytes the callers advance by are unchanged. I hand-verified the offset formula against all three test fixtures (line comment → 41, block comment → 42, doc comment with a preceding @param → 51) and they match indexOf("bogus") in each source string. The PR description also cross-checks against esbuild 0.25.1's 0-based columns.
Other factors
The new test in bun-build-api.test.ts asserts exact {line, column, offset, length, lineText} for three comment shapes, including a multi-line doc comment where the pragma follows another @ tag — that last case exercises the loop's advance-past-non-pragma path. The test uses tempDir, test.concurrent, and toEqual on a keyed object, matching the file's conventions. The comment-cop bot flagged over-long comments on an earlier revision; the author shortened them in 660ed00 and daa222a, and all five threads are resolved. Grep confirms remaining() has no remaining callers. The source_mapping_url path gets the same fix but is currently write-only, so no test is possible there — the PR notes this.
scan_pragma added self.start to an offset that was already absolute, so the spans stored for @jsx, @jsxFrag, @jsxRuntime, @jsxImportSource and sourceMappingURL pointed past the comment. The parser reports "Unsupported JSX runtime" at that span, so the warning named the wrong line and column. scan_pragma and PragmaArg now take the source offset of the chunk that follows the @ or # and use it as is. The single line comment scanner passes self.current and slices the chunk from its local contents, which makes Lexer::remaining unused, so it is removed. The block comment scanner tracks the chunk offset inside the comment body directly.
462eba2 to
f48904a
Compare
There was a problem hiding this comment.
LGTM — the offset arithmetic checks out and the change is diagnostic-only.
Verified chunk_start is the correct absolute source offset at both call sites: self.start + pos in scan_comment_text (since text = contents[self.start..self.end]) and self.current in scan_single_line_comment (just past the consumed @/#). PragmaArg::scan and scan_source_mapping_url_value now add only the pragma-name length and whitespace skip, which lands exactly on the argument. The removed remaining() helper had no other callers. The rewritten block-comment loop is equivalent to the old reslicing form and keeps the .min(chunk.len()) clamp. The new test pins line/column/offset/length/lineText for all three comment shapes and would fail under the old double-counted offsets.
Extended reasoning...
Overview
This PR fixes incorrect source locations in JSX pragma warnings emitted by the JS lexer. The bug was that scan_pragma added self.start (the comment's own start offset) to an offset that callers had already made absolute, so warnings like Unsupported JSX runtime: "bogus" pointed at the wrong line/column. The fix renames the parameter to chunk_start (the absolute offset of the byte after @/#), threads it through unchanged, and simplifies the block-comment pragma loop from repeated reslicing to a single pos cursor. It also drops the now-unused Lexer::remaining() and a StoreStr detour in the single-line path, and collapses chained u32::try_from casts into one usize sum. A new test in bun-build-api.test.ts builds three .jsx files (line, block, and multi-line doc comments, each with preceding source so a double-counted offset would land on a later line) and asserts exact line/column/length/offset/lineText for the warning.
Security risks
None. This is a lexer diagnostic-position fix. The pragma text extraction and application are unchanged — only the Range attached to the warning changes. No new input is parsed, no allocation size depends on the changed arithmetic, and no user-controllable value flows anywhere new.
Level of scrutiny
Moderate, because it touches src/js_parser/lexer.rs. I traced the offset arithmetic at both call sites against the actual lexer state: in scan_comment_text, text is &contents[self.start..self.end], so body[pos] sits at absolute offset self.start + pos; in scan_single_line_comment, step_with() has just consumed the @/#, leaving self.current at the following byte. Both now pass exactly that absolute offset. PragmaArg::scan then adds pragma.len() + start (whitespace skip), which is the argument's position within chunk — correct. The .min(chunk.len()) clamp on the loop advance is preserved. The single-line path now slices contents[chunk_start..] directly instead of via StoreStr::new(self.remaining()), which is behaviorally identical since remaining() was &self.contents[self.current..]. Confirmed remaining() has no other callers.
Other factors
The test is well-constructed: it uses tempDir, test.concurrent, a single .toEqual on a structured object, and indexOf("bogus") to derive expected offsets from the fixture text rather than hardcoding them. The fixtures place the pragma after earlier source lines specifically so the old double-counting would produce a visibly wrong line, satisfying the "fails for the right reason" requirement. No CODEOWNERS cover the changed files. There are no outstanding human CHANGES_REQUESTED reviews; the resolved inline threads are all from bots. The exit reason was dry_streak. A new commit landed after the earlier bot reviews, so this approval reflects the current state.
Problem
Unsupported JSX runtime: "bogus"for// @jsxRuntime boguson line 3 is reportedat rt.jsx:4:20. The argument is atrt.jsx:3:16. Block comments are off too.Lexer::scan_pragma(src/js_parser/lexer.rs:2114on main) addsself.start, the comment start, to an offset that is already absolute. The//caller also passes the offset of the@itself. The/* */caller adds the index of the@twice.Fix
scan_pragma,PragmaArg::scanandscan_source_mapping_url_valuenow take the source offset of the first byte after the@or#and use it as is.//caller passesself.currentand slices the chunk from its localcontents, as the rest of that function does. This removes theStoreStrcopy and the now unusedLexer::remaining. The/* */caller tracks the chunk offset inside the comment body.PragmaArg::scanlocates the argument relative to that byte. esbuild 0.25.1 reports the same positions. Only diagnostics change: the parser applies pragmas from the span text and uses the range only in warnings.test/bundler/bun-build-api.test.ts. Bun 1.4.0 reports offsets 66, 74 and 84 instead of 41, 42 and 51. The other suites I ran are in the notes.Background
// @jsxImportSource preact. The lexer stores the argument as aSpan: text plus sourceRange.prepare_for_visit_pass(src/js_parser/p.rs) applies the text and warns at the range.scan_single_line_commentcallsscan_pragmaat each@or#in a//comment.scan_comment_textdoes the same for a finished/* */comment.self.startis the start of the current token, here the comment.self.currentis the offset after the last consumed character.@jsxand@jsxFragspans. They get the right location through this change.Notes
Repro on bun 1.4.0:
Before:
at rt.jsx:4:20, with line 4 printed above the caret. After:at rt.jsx:3:16, caret underbogus. The block comment form/** @jsxRuntime bogus */moves from4:22(clamped to the end of the file) to3:17. A pragma on the second line of a block comment moves from6:22to4:16. esbuild 0.25.1 prints3:15,3:16and4:15for the same three files (esbuild columns are 0-based).Arithmetic for the
//case, comment at offset 26,@at 29: main passed 29,scan_pragmaaddedself.start(26), andPragmaArg::scanadded the pragma name (10) and the argument index in the text after it (1): 66. The argument is at 30 + 10 + 1 = 41.Bun.buildexposes the range start asposition.offset. The new test checks that next to line, column andlineText, for a//comment, a one line/** */comment, and a block comment where the pragma follows another@tag on a later line.source_mapping_urlgets the same treatment. The lexer writes it and nothing reads it, so there is nothing to test there.Possible follow-up, kept out of this PR: after this change the
textof each pragmaSpanis exactlysourceatrange, soJSXPragmacould hold plainRanges andprepare_for_visit_passcould read the text withSource::text_for_range, the way it already readsall_comments. That would also removejs_ast::Spanand the unreadsource_mapping_url. It touches the same lines ofp.rsas #39631, so it is better done once that PR and this one have landed. The offset fix here is needed under either shape.The same formulas were in
js_lexer.zigbefore the port, so this is not a port regression.The pragma loop in
scan_comment_textnow keepspos, the offset of the chunk inside the comment, so the source offset isself.start + pos. The old loop had amatchon the byte thatindex_of_any(rest, b"@#")found, which can only be one of those two bytes, and a.min()oni + 1, which is always in bounds. Both are gone. The clamp on the advance afterscan_pragmastays. Its old comment described a case (pragma at the end of the file) in whichscan_pragmareturns 0 or the exact length, so the comment is gone as well.Not changed here: in a block comment the argument runs to the next space, so
/**\n * @jsxRuntime classic\n */still yieldsclassic\nand warns. esbuild 0.25.1 does the same. That warning now at least points at the right line.Rebased on main after #40610, which removed the
IS_JSONbranch around the//pragma call. The conflict was in that arm and in the/* */loop. Both now carry main's shape plus the offset fix. The test file had no conflict.Suites run with the debug build:
test/bundler/bundler_jsx.test.ts,bundler_comments.test.ts,transpiler/transpiler.test.js,esbuild/dce.test.ts,esbuild/tsconfig.test.tsand all ofbun-build-api.test.ts.cargo fmtandcargo clippy -p bun_js_parserare clean.no test proof · iteration 3 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/bundler/bun-build-api.test.ts