Skip to content

fix(codegen): emit newline after legal-comment orphan flush#22304

Merged
graphite-app[bot] merged 1 commit into
mainfrom
fix/codegen-legal-comment-orphan-idempotency
May 11, 2026
Merged

fix(codegen): emit newline after legal-comment orphan flush#22304
graphite-app[bot] merged 1 commit into
mainfrom
fix/codegen-legal-comment-orphan-idempotency

Conversation

@Dunqing

@Dunqing Dunqing commented May 11, 2026

Copy link
Copy Markdown
Member

print_legal_orphans_before carried the source-side followed_by_newline = false for inline orphans (e.g. 33/*!*/ ||) over into the flush position. After printing the orphan, print_comments set print_next_indent_as_space = true, which then turned the closing-} indent into a single space — producing \t/*!*/ } on pass 1.

Pass 2 saw the comment at }-position, picked it up via FunctionBody's comments_at_end branch (which calls clear_pending_indent_space()), and emitted \t/*!*/} without the space. The two passes diverged and monitor-oxc's codegen idempotency check failed on every cjs-module-lexer / es-module-lexer build.

Force followed_by_newline = true on the last orphan before the flush so print_comments takes its existing newline-emitting branch. Orphans now land block-level on both passes — matching the pass-2 comments_at_end path that already calls clear_pending_indent_space().

The orphan-preservation behavior added in #21575 (issue #19750) is unaffected: //! some license whose anchor is DCE'd is still emitted at the next surviving statement.

Test plan

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

Dunqing commented May 11, 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 11, 2026

Copy link
Copy Markdown

Merging this PR will not alter performance

✅ 48 untouched benchmarks
⏩ 3 skipped benchmarks1


Comparing fix/codegen-legal-comment-orphan-idempotency (58390d8) with main (b5d970f)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 (6b50f23) during the generation of this report, so b5d970f 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 May 11, 2026 08:36
@Dunqing Dunqing added the 0-merge Merge with Graphite Merge Queue label May 11, 2026

Dunqing commented May 11, 2026

Copy link
Copy Markdown
Member Author

Merge activity

`print_legal_orphans_before` carried the source-side `followed_by_newline = false` for inline orphans (e.g. `33/*!*/ ||`) over into the flush position. After printing the orphan, `print_comments` set `print_next_indent_as_space = true`, which then turned the closing-`}` indent into a single space — producing `\t/*!*/ }` on pass 1.

Pass 2 saw the comment at `}`-position, picked it up via `FunctionBody`'s `comments_at_end` branch (which calls `clear_pending_indent_space()`), and emitted `\t/*!*/}` without the space. The two passes diverged and [monitor-oxc's codegen idempotency check](https://github.com/oxc-project/monitor-oxc/actions/runs/25616176953/job/75194649734) failed on every cjs-module-lexer / es-module-lexer build.

Force `followed_by_newline = true` on the last orphan before the flush so `print_comments` takes its existing newline-emitting branch. Orphans now land block-level on both passes — matching the pass-2 `comments_at_end` path that already calls `clear_pending_indent_space()`.

The orphan-preservation behavior added in #21575 (issue #19750) is unaffected: `//! some license` whose anchor is DCE'd is still emitted at the next surviving statement.

## Test plan
- [x] `cargo test -p oxc_codegen` (105 tests, includes new `test_inline_legal_comment_in_function_body_is_idempotent`)
- [x] `cargo test -p oxc_minifier --tests` (492 tests, includes #21575's `preserve_legal_comment_when_dce_removes_anchor`)
- [x] Re-ran codegen idempotency on all 4 originally failing files (`[email protected] / @2.2.0`, `[email protected] / @2.0.0`) — all `same = true`.
@graphite-app graphite-app Bot force-pushed the fix/codegen-legal-comment-orphan-idempotency branch from 58390d8 to 6a8852d Compare May 11, 2026 08:44
@graphite-app graphite-app Bot merged commit 6a8852d into main May 11, 2026
28 checks passed
@graphite-app graphite-app Bot removed the 0-merge Merge with Graphite Merge Queue label May 11, 2026
@graphite-app graphite-app Bot deleted the fix/codegen-legal-comment-orphan-idempotency branch May 11, 2026 08:48
camc314 added a commit that referenced this pull request May 11, 2026
### 🚀 Features

- 66c9b01 transformer/typescript: Debug_assert that `enum_eval` ran in
semantic (#22252) (Dunqing)
- ffe6475 minifier: Fold `Array` constructor with safe spreads (#22215)
(camc314)

### 🐛 Bug Fixes

- d3d0b18 traverse: Handle `ChainElement::TSNonNullExpression` in
`GatherNodeParts` (#22247) (leaysgur)
- 4e880de transformer/object-rest-spread: Declare temp vars for computed
keys (#22284) (camc314)
- a7c3e22 semantic: Clear member write target for computed keys (#22302)
(camc314)
- 6a8852d codegen: Emit newline after legal-comment orphan flush
(#22304) (Dunqing)
- 5da9fda transformer/explicit-resource-management: Preserve class names
(#22306) (Dunqing)
- b5d970f transformer/explicit-resource-management: Preserve class names
(#22290) (camc314)
- bc54fd4 minifier: Keep function / class names if direct eval is
present in the scope (#22241) (sapphi-red)
- 7a810c0 minifier: Refresh direct eval flags after DCE (#21787)
(Dunqing)
- dd88726 transformer/legacy-decorator: Preserve accessor type
annotation for emitDecoratorMetadata (#21966) (Dunqing)
- 29a3cd7 codegen: Swap mapping/indent order for top-level decls
(#22206) (Dunqing)
- 73b4f40 minifier: Preserve catch binding with direct eval (#22221)
(camc314)
- 0e13d17 minifier: Preserve optional chain base side effects (#22219)
(camc314)
- 0c7c01c transformer/typescript: Inline optional-chain enum member
access (#21834) (Dunqing)
- a6aff7e codegen: Emit block/array/object end mapping at close char
(#22200) (Dunqing)
- a099b03 codegen: Emit call end mapping at `)` position, not past it
(#22199) (Dunqing)
- 5753774 minifier: Cap if-return ternary collapse for firefox (#21841)
(Gurupungav Narayanan)
- 2493bdd codegen: Correct sourcemap end mappings for closing delimiters
(#22001) (Mark Dalgleish)
- 3b385e2 minifier: Bail optimizing `Array` with unknown arg count
(#22188) (camc314)
- 9fa2122 parser: Parse array computed class keys (#22159) (camc314)

### 📚 Documentation

- a4a6892 napi/parser: Correct code comment (#22278) (overlookmotel)
- 9305373 oxc: Update README (#22178) (camc314)

Co-authored-by: Cameron <[email protected]>
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