Skip to content

js_parser: map an inlined const to its use site in the source map - #45581

Open
robobun wants to merge 5 commits into
mainfrom
robobun/648e8b81/inlined-const-use-site-loc
Open

robobun wants to merge 5 commits into
mainfrom
robobun/648e8b81/inlined-const-use-site-loc

Conversation

@robobun

@robobun robobun commented Oct 16, 2026

Copy link
Copy Markdown
Collaborator

Fixes #45218

Problem

  • bun test --coverage reports 0 hits for every line from an inlined const declaration down to an uncalled function that uses it. A stack frame for NOT_A_FUNCTION() inside a function points at the const line too.
  • handle_identifier (src/js_parser/p.rs:2195) returns the stored const value with the loc of the declaration, so the source map points the literal inside the function at that line. generate_report_from_blocks (src/sourcemap_jsc/CodeCoverage.rs:938) takes the min mapped line of the function as its start.

Fix

  • handle_identifier returns the const value with the loc of the use. An enum-valued const (EInlinedEnum) gets a new node, because the printer maps the inner value.
  • Every other substitution in that function uses the use loc, and so does esbuild (ConstValueToExpr(loc, value)). The printed JavaScript does not change.
  • The runtime transpiler cache version moves to 36. The cache stores the source map for files of 4 KiB or more.
  • Verified: new tests in test/cli/test/coverage.test.ts and test/js/bun/sourcemap/internal-sourcemap.test.ts, both fail on 1.4.3. Self-reviewed: 3 concerns, 2 addressed (Notes).

Background

  • The runtime transpiler runs with inlining on. visit_decl stores each const x = <literal> in const_values. Each later read of x becomes that stored Expr. Expr.loc is the position the printer writes into the source map.
  • Considered a clamp in CodeCoverage.rs. It hides a wrong mapping in one consumer, and stack traces stay wrong. Make line coverage follow the innermost JSC block #41528 also rewrites that code.

Downsides

  • Each use of an enum-valued const allocates one E::InlinedEnum node in the parser arena. A literal const allocates nothing new.
  • The cache version bump transpiles every cached file again on the first run after the upgrade.
  • A const bound to a macro result object or array still inlines the inner literals with the positions the macro gave them.
Notes

Repro from the issue, bun test --coverage --coverage-reporter=lcov, before:

DA:1,12
DA:2,0
DA:3,0
DA:4,0
DA:5,0

After:

DA:1,12
DA:2,12
DA:3,12
DA:5,0

Stack trace probe, const NOT_A_FUNCTION = null; on line 1 and NOT_A_FUNCTION(); on line 5 inside call():

before: at call (idx.ts:1:24)
after:  at call (idx.ts:5:3)

const C = Color.Red; function call() { C(); } prints "red" /* Red */();. Before, the frame pointed at the declaration line. After, at the call line.

const B = Number(2), let, var, 1n, and "a" + "b" are not inlined, so they never showed the bug.

The lcov range of the uncalled function still ends one line early (DA:6 for the return line is absent). That is the existing min_line..max_line exclusive range in generate_report_from_blocks, not touched here.

Open PR #41580 (tail-call frames under the runtime's minify-syntax, for #24789) carries the same literal-case hunk in handle_identifier as a side fix. It does not handle the enum-valued const and has no coverage test. That hunk drops out of #41580 on rebase once this lands.

Self-review concerns: (1) cache version not bumped, fixed. (2) #41580 overlap not named, fixed above. (3) Macro-valued object and array consts keep the positions the macro stamped on them (Macro.rs). That is a separate path and is listed under Downsides.

Other suites run: transpiler.test.js, bundler_minify, bundler_edgecase, bundler_compile, esbuild/ts, esbuild/default, test/js/bun/sourcemap, regression/issue/30887. Pre-existing timeouts on this slow debug machine, with and without the change: bun-build-api.test.ts bytecode tests, esbuild/default TopLevelAwaitForbiddenRequireSourceMap*.

handle_identifier returned the stored const value with the loc of the
declaration. The printer then mapped the literal inside the using
function back to the declaration line. Coverage treated that line as
part of an uncalled function, and stack traces pointed at the const.
Stamp the use site loc, as every other substitution in this function
does and as esbuild does. An enum-valued const gets a new inlined enum
node so the inner value carries the use loc too.
@robobun

robobun commented Oct 16, 2026 •

Copy link
Copy Markdown
Collaborator Author
Updated 5:41 PM PT - Oct 17th, 2026

✅ @robobun, your commit 84c7dfcb7abdc40e7f42bc8ea4a0457680fe9660 passed in Build #127728! 🎉


🧪   To try this PR locally:

bunx bun-pr 45581

That installs a local version of the PR into your bun-45581 executable, so you can run:

bun-45581 --bun

Comment thread src/js_parser/p.rs Outdated
@coderabbitai

coderabbitai Bot commented Oct 16, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository: oven-sh/bun/.coderabbit.yaml
  • Review profile: ASSERTIVE
  • Plan: Advanced
  • Run ID: 77644136-398e-4e0c-86ef-be4245db7148
📥 Commits

Reviewing files that changed from the base of the PR and between 21620e2 and bf353f9.

📒 Files selected for processing: 4
  • src/js_parser/p.rs
  • src/jsc/RuntimeTranspilerCache.rs
  • test/cli/test/coverage.test.ts
  • test/js/bun/sourcemap/internal-sourcemap.test.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 2 remain after this review.

Walkthrough

The parser assigns inlined constant replacements to their identifier use-site locations. The runtime transpiler cache version increases to 36. New regression tests check LCOV coverage and stack-frame source positions.

Changes

Inlined constant locations

Layer / File(s) Summary
Use-site locations for inlined constants
src/js_parser/p.rs, src/jsc/RuntimeTranspilerCache.rs
Identifier replacements use the identifier’s location. Inlined enum replacements retain their comment wrapper, and the runtime transpiler cache version advances to 36.
Coverage and stack-map regressions
test/cli/test/coverage.test.ts, test/js/bun/sourcemap/internal-sourcemap.test.ts
The LCOV test checks executed module-load lines and an uncovered function line. The source-map test checks stack-frame positions for literal and enum-valued constants.

Suggested reviewers: jarred-sumner

Priority: ➖ Normal

Severity of issue fixed: Medium

Merge Risk: ⚪ Minimal · up to bf353

Inlined constants now map to their use site in source maps, which should correct coverage and stack-trace positions. No merge-blocking risk remains.

Pre-merge checks: Passed 4
Check name Status Explanation
Title check Passed The title clearly and concisely describes the main change: mapping inlined constants to their use sites in source maps.
Description check Passed The description explains the problem, fix, impact, limitations, and verification results. It does not use the template headings exactly, but it provides the required information in equivalent sections…
Linked Issues check Passed Issue #45218 requires executed module lines to remain covered when an uncalled function uses an inlined const, while the uncalled function remains uncovered. P::handle_identifier now assigns inlined…
Out of Scope Changes check Passed The changes stay within issue #45218. The parser change fixes the source location used by coverage and stack traces. The runtime transpiler cache version bump supports the changed source-map data. The…
Full details: Description check

Explanation

The description explains the problem, fix, impact, limitations, and verification results. It does not use the template headings exactly, but it provides the required information in equivalent sections.

Full details: Linked Issues check

Explanation

Issue #45218 requires executed module lines to remain covered when an uncalled function uses an inlined const, while the uncalled function remains uncovered. P::handle_identifier now assigns inlined replacements the identifier use location. Enum replacements also receive a new use-located node. The added coverage regression test checks this coverage split. The source-map regression test checks literal and enum stack frames at their use sites.

Full details: Out of Scope Changes check

Explanation

The changes stay within issue #45218. The parser change fixes the source location used by coverage and stack traces. The runtime transpiler cache version bump supports the changed source-map data. The two added tests verify coverage and source-map behavior. No unrelated implementation change is shown.

✨ Finishing Touches: 💡 1
🛠️ Fix failing CI checks: 💡
  • Commit to this branch
  • Create a new PR
  • 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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.

Beyond the inline finding, I also checked for sibling sites: handle_identifier is the only reader of const_values (the bundler/linker consumers in src/bundler are commented out), so the runtime transpiler and Bun.build share this one fix. The printer's EInlinedEnum arm (src/js_printer/lib.rs:4515) maps via print_expr(e.value), which confirms the inner-value re-stamp and the per-use node allocation are both needed; the shared inner E::String StoreRef was already shared across uses before this change, so that is not new.

Extended reasoning...

The change re-stamps the use-site loc onto const-inlined replacement exprs in the parser hot path, bumps the runtime transpiler cache version to 36, and adds a coverage test and a stack-trace sourcemap test. It touches no security-sensitive surface. The one inline finding is a pre-existing gap for macro-valued object/array consts, which the PR description acknowledges; the literal and enum cases are handled in the single consumer of const_values.

Comment thread src/js_parser/p.rs

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review found no issues

No high-confidence issues detected in this change.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bun test --coverage: an uncalled function that uses an inlined const zeroes every line from that const down to the function

1 participant