Skip to content

fix(codegen): map call-chain punctuation for correct V8 stack columns#22789

Merged
graphite-app[bot] merged 1 commit into
mainfrom
fix/codegen-sourcemap-exclusive-end-mapping
Jun 1, 2026
Merged

fix(codegen): map call-chain punctuation for correct V8 stack columns#22789
graphite-app[bot] merged 1 commit into
mainfrom
fix/codegen-sourcemap-exclusive-end-mapping

Conversation

@Dunqing

@Dunqing Dunqing commented May 28, 2026

Copy link
Copy Markdown
Member

Summary

  • oxc_codegen produced off-by-one V8 stack-trace and coverage columns for call-chain shapes. When a (/./[ is chained directly onto a call — or any expression ending in )/] — that punctuation was left unmapped, so the lookup fell back to the closing delimiter one column to the left. Surfaced as vitest failures in vite-ecosystem-ci: test.each([1, 2])(…) reporting column 17 instead of 18, and await expect(…).resolves.toBe(4) reporting 40 instead of 41.
  • Root cause: such operands end in ) or ], so there is no trailing identifier token for a source-map consumer to anchor on. The fix maps that punctuation from print_expr — keyed on postfix precedence — back to the operand's exclusive end, exactly where V8 reports the frame. This is the trailing end-mapping that Babel and TSC apply to every node, scoped here to just the no-anchor sites. The closing )/}/] keep their existing at-delimiter mapping, which v8-to-istanbul relies on for coverage range boundaries (fix(codegen): correct sourcemap end mappings for closing delimiters #22001).
  • Only these no-anchor chain sites are mapped — not the byte past every delimiter — so the added source-map tokens stay at +0.6% over baseline. (A blanket variant that mapped every delimiter regressed codegen ~5% on CodSpeed.)
  • Verified end-to-end through node --enable-source-maps: each chained frame resolves to the correct column (the outer ( for curried calls, the . for a getter/member on a call result), and regresses by exactly one column when the mapping is disabled. Covered by the stacktrace_is_correct integration test and unit tests in sourcemap.rs.

Example

factory()() — V8 reports the outer call at the ( after the inner ):

without this fix:  at <anonymous> (input.js:7:9)    ← the inner `)`
with this fix:     at <anonymous> (input.js:7:10)   ← the outer `(`   ✓

The same one-column drift affected expect(x).resolves (the . getter access after the )).

Compared with esbuild and tsc

Same input through each tool (identical generated code), visualized with source-map-viewer:

const outer = factory()();          // outer `(`  of factory()()
const member = expect(x).resolves;  // `.`        of expect(x).resolves
const indexed = arr[i]();           // `(`        of arr[i]()
Source token oxc (this PR) tsc esbuild
outer ( of factory()()
. of expect(x).resolves
( of arr[i]()
  • vs esbuild (compare): oxc adds the three chain mappings esbuild lacks — esbuild maps the ] one column left of the (, the exact off-by-one. This PR resolves what esbuild still gets wrong.
  • vs tsc (compare): all three positions match tsc exactly. tsc maps ~10 more positions because it gives every node a trailing end-mapping; this PR maps only the no-anchor sites (+0.6% vs tsc's full density).

This PR was prepared with AI assistance; I reviewed, tested, and verified the changes.

@github-actions github-actions Bot added the A-codegen Area - Code Generation label May 28, 2026

Dunqing commented May 28, 2026

Copy link
Copy Markdown
Member Author

How to use the Graphite Merge Queue

Add either label to this PR to merge it via the merge queue:

  • 0-merge - adds this PR to the back of the merge queue
  • hotfix - for urgent changes, fast-track this PR to the front of the merge queue

You must have a Graphite account in order to use the merge queue. Sign up using this link.

An organization admin has enabled the Graphite Merge Queue in this repository.

Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.

This stack of pull requests is managed by Graphite. Learn more about stacking.

@codspeed-hq

codspeed-hq Bot commented May 28, 2026

Copy link
Copy Markdown

Merging this PR will not alter performance

✅ 57 untouched benchmarks


Comparing fix/codegen-sourcemap-exclusive-end-mapping (035c74b) with main (81b86eb)1

Open in CodSpeed

Footnotes

  1. No successful run was found on main (494c043) during the generation of this report, so 81b86eb was used instead as the comparison base. There might be some changes unrelated to this pull request in this report.

@Dunqing Dunqing force-pushed the fix/codegen-sourcemap-exclusive-end-mapping branch 2 times, most recently from 1c5f694 to cbc46f8 Compare June 1, 2026 05:30
@Dunqing Dunqing changed the title fix(codegen): map both the closing delimiter and the position past it fix(codegen): map call-chain punctuation for correct V8 stack columns Jun 1, 2026
@Dunqing Dunqing force-pushed the fix/codegen-sourcemap-exclusive-end-mapping branch 3 times, most recently from a2bc427 to 035c74b Compare June 1, 2026 13:36
@Dunqing Dunqing marked this pull request as ready for review June 1, 2026 13:44
@Dunqing Dunqing added the 0-merge Merge with Graphite Merge Queue label Jun 1, 2026

Dunqing commented Jun 1, 2026

Copy link
Copy Markdown
Member Author

Merge activity

…#22789)

## Summary

- `oxc_codegen` produced off-by-one V8 stack-trace and coverage columns for call-chain shapes. When a `(`/`.`/`[` is chained directly onto a call — or any expression ending in `)`/`]` — that punctuation was left unmapped, so the lookup fell back to the closing delimiter one column to the left. Surfaced as vitest failures in vite-ecosystem-ci: `test.each([1, 2])(…)` reporting column 17 instead of 18, and `await expect(…).resolves.toBe(4)` reporting 40 instead of 41.
- Root cause: such operands end in `)` or `]`, so there is no trailing identifier token for a source-map consumer to anchor on. The fix maps that punctuation from `print_expr` — keyed on postfix precedence — back to the operand's exclusive end, exactly where V8 reports the frame. This is the trailing end-mapping that Babel and TSC apply to every node, scoped here to just the no-anchor sites. The closing `)`/`}`/`]` keep their existing at-delimiter mapping, which `v8-to-istanbul` relies on for coverage range boundaries (#22001).
- Only these no-anchor chain sites are mapped — not the byte past every delimiter — so the added source-map tokens stay at **+0.6%** over baseline. (A blanket variant that mapped every delimiter regressed codegen ~5% on CodSpeed.)
- Verified end-to-end through `node --enable-source-maps`: each chained frame resolves to the correct column (the outer `(` for curried calls, the `.` for a getter/member on a call result), and regresses by exactly one column when the mapping is disabled. Covered by the `stacktrace_is_correct` integration test and unit tests in `sourcemap.rs`.

## Example

`factory()()` — V8 reports the outer call at the `(` after the inner `)`:

```
without this fix:  at <anonymous> (input.js:7:9)    ← the inner `)`
with this fix:     at <anonymous> (input.js:7:10)   ← the outer `(`   ✓
```

The same one-column drift affected `expect(x).resolves` (the `.` getter access after the `)`).

## Compared with esbuild and tsc

Same input through each tool (identical generated code), visualized with [source-map-viewer](https://source-map-viewer.void.app):

```js
const outer = factory()();          // outer `(`  of factory()()
const member = expect(x).resolves;  // `.`        of expect(x).resolves
const indexed = arr[i]();           // `(`        of arr[i]()
```

| Source token | oxc (this PR) | tsc | esbuild |
| --- | :--: | :--: | :--: |
| outer `(` of `factory()()` | ✓ | ✓ | ✗ |
| `.` of `expect(x).resolves` | ✓ | ✓ | ✗ |
| `(` of `arr[i]()` | ✓ | ✓ | ✗ |

- **vs esbuild** ([compare](https://source-map-viewer.void.app/compare?a=QCnJWx7H&b=Y4FwbiMW)): oxc adds the three chain mappings esbuild lacks — esbuild maps the `]` one column left of the `(`, the exact off-by-one. This PR resolves what esbuild still gets wrong.
- **vs tsc** ([compare](https://source-map-viewer.void.app/compare?a=UjsQoNTX&b=Y4FwbiMW)): all three positions match tsc exactly. tsc maps ~10 more positions because it gives every node a trailing end-mapping; this PR maps only the no-anchor sites (+0.6% vs tsc's full density).

---

This PR was prepared with AI assistance; I reviewed, tested, and verified the changes.
@graphite-app graphite-app Bot force-pushed the fix/codegen-sourcemap-exclusive-end-mapping branch from 035c74b to 7914807 Compare June 1, 2026 13:45
@graphite-app graphite-app Bot removed the 0-merge Merge with Graphite Merge Queue label Jun 1, 2026
@graphite-app graphite-app Bot merged commit 7914807 into main Jun 1, 2026
29 checks passed
@graphite-app graphite-app Bot deleted the fix/codegen-sourcemap-exclusive-end-mapping branch June 1, 2026 13:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-codegen Area - Code Generation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant