Skip to content

fix(parser): set pure comment index after dedup check to handle lookahead/rewind#21570

Merged
graphite-app[bot] merged 1 commit into
mainfrom
fix/pure-comment-in-incorrect-place
Apr 20, 2026
Merged

fix(parser): set pure comment index after dedup check to handle lookahead/rewind#21570
graphite-app[bot] merged 1 commit into
mainfrom
fix/pure-comment-in-incorrect-place

Conversation

@Dunqing

@Dunqing Dunqing commented Apr 20, 2026

Copy link
Copy Markdown
Member

Fix some idempotency tests that failed in https://github.com/oxc-project/monitor-oxc/actions/runs/24652862465/job/72079552802

Summary

When /* @__PURE__ */ appears before an object literal (e.g. export const X = /* @__PURE__ */ { a: 1 }), the parser's arrow-function lookahead causes the comment to be added to the comments vec during speculative parsing. After rewind, the same comment is encountered again — the dedup check skips the push, but parse_annotation had already set pure_comment to comments.len(), which now points to a non-existent index. This causes mark_pure_comment_not_applied to silently fail, breaking codegen idempotency.

Fix

  • Move pure_comment / has_no_side_effects_comment assignment from parse_annotation to add_comment, after the dedup check
  • In the dedup branch, set the index to the existing comment (len - 1)
  • In the normal branch, set the index to where the comment will be pushed (len)
  • Extract shared logic into set_annotation_flags helper

Test plan

  • Added parser test: /* @__PURE__ */ before object literal is correctly marked PureNotApplied
  • Added parser test: /* @__PURE__ */ before call expression remains Pure after lookahead
  • Added codegen idempotency test: parse→codegen→parse→codegen produces identical output
  • cargo test -p oxc_parser --lib -- trivia_builder — 9 tests pass
  • cargo test -p oxc_codegen --test integration -- comments — all pass

Dunqing commented Apr 20, 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.

@Dunqing Dunqing requested a review from Boshen April 20, 2026 08:52
@codspeed-hq

codspeed-hq Bot commented Apr 20, 2026

Copy link
Copy Markdown

Merging this PR will degrade performance by 3.5%

❌ 1 regressed benchmark
✅ 47 untouched benchmarks
⏩ 3 skipped benchmarks1

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Performance Changes

Mode Benchmark BASE HEAD Efficiency
Simulation mangler[RadixUIAdoptionSection.jsx_keep_names] 19.7 µs 20.5 µs -3.5%

Comparing fix/pure-comment-in-incorrect-place (2a0b901) with main (385eb94)2

Open in CodSpeed

Footnotes

  1. 3 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports.

  2. No successful run was found on main (5a6dab2) during the generation of this report, so 385eb94 was used instead as the comparison base. There might be some changes unrelated to this pull request in this report.

@Dunqing Dunqing marked this pull request as ready for review April 20, 2026 09:00
@Dunqing Dunqing force-pushed the fix/pure-comment-in-incorrect-place branch 2 times, most recently from 95af6f4 to 2a0b901 Compare April 20, 2026 12:12
@Dunqing

Dunqing commented Apr 20, 2026

Copy link
Copy Markdown
Member Author

Confirmed that this fix is correct, and I tested it locally in monitor-oxc, and it worked. So I am merging it without review.

@Dunqing Dunqing added the 0-merge Merge with Graphite Merge Queue label Apr 20, 2026

Dunqing commented Apr 20, 2026

Copy link
Copy Markdown
Member Author

Merge activity

…head/rewind (#21570)

Fix some idempotency tests that failed in https://github.com/oxc-project/monitor-oxc/actions/runs/24652862465/job/72079552802

## Summary

When `/* @__PURE__ */` appears before an object literal (e.g. `export const X = /* @__PURE__ */ { a: 1 }`), the parser's arrow-function lookahead causes the comment to be added to the `comments` vec during speculative parsing. After rewind, the same comment is encountered again — the dedup check skips the push, but `parse_annotation` had already set `pure_comment` to `comments.len()`, which now points to a non-existent index. This causes `mark_pure_comment_not_applied` to silently fail, breaking codegen idempotency.

### Fix

- Move `pure_comment` / `has_no_side_effects_comment` assignment from `parse_annotation` to `add_comment`, **after** the dedup check
- In the dedup branch, set the index to the existing comment (`len - 1`)
- In the normal branch, set the index to where the comment will be pushed (`len`)
- Extract shared logic into `set_annotation_flags` helper

## Test plan

- Added parser test: `/* @__PURE__ */` before object literal is correctly marked `PureNotApplied`
- Added parser test: `/* @__PURE__ */` before call expression remains `Pure` after lookahead
- Added codegen idempotency test: parse→codegen→parse→codegen produces identical output
- `cargo test -p oxc_parser --lib -- trivia_builder` — 9 tests pass
- `cargo test -p oxc_codegen --test integration -- comments` — all pass
@graphite-app graphite-app Bot force-pushed the fix/pure-comment-in-incorrect-place branch from 2a0b901 to 67d40f8 Compare April 20, 2026 12:26
@graphite-app graphite-app Bot merged commit 67d40f8 into main Apr 20, 2026
28 checks passed
@graphite-app graphite-app Bot removed the 0-merge Merge with Graphite Merge Queue label Apr 20, 2026
@graphite-app graphite-app Bot deleted the fix/pure-comment-in-incorrect-place branch April 20, 2026 12:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants