Skip to content

feat(codegen): rework printing normal / legal / annotation comments#10997

Merged
Boshen merged 1 commit intomainfrom
05-12-fix_codegen_correct_print_normal___legal___annotation_comments
May 13, 2025
Merged

feat(codegen): rework printing normal / legal / annotation comments#10997
Boshen merged 1 commit intomainfrom
05-12-fix_codegen_correct_print_normal___legal___annotation_comments

Conversation

@Boshen
Copy link
Member

@Boshen Boshen commented May 13, 2025

fixes #10948

This PR changes how comment options are interpreted.

CodegenOptions::comments, CodegenOptions::annotation_comments and CodegenOptions::legal_comments are now independent of each other.

CodegenOptions::comments

Controls printing of normal comments. At present, only some leading comments are preserved. It does not control legal and annotation comments.

CodegenOptions::annotation_comments

Controls printing of annotation comments

  • jsdoc: /** jsdoc */
  • pure: /* #__PURE__ */ and /* #__NO_SIDE_EFFECTS__ */
  • webpack: /* webpackChunkName */
  • vite: /* @vite-ignore */
  • coverage: v8 ignore, c8 ignore, node:coverage, istanbul ignore

CodegenOptions::legal_comments

Controls printing of legal comments

  • starts with //! or /*!.
  • contains /* @license */ or /* @preserve */

Comment generation is also changed:

codegen first builds a comments map by keeping only the needed comments.
Printing functions will then print out comments in their respective positions.


This PR reveals some incorrect comment interpretation:

Copilot AI review requested due to automatic review settings May 13, 2025 10:39
@graphite-app
Copy link
Contributor

graphite-app bot commented May 13, 2025

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 hot fixes, skip the queue and merge this PR next

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.

@github-actions github-actions bot added A-ast Area - AST A-transformer Area - Transformer / Transpiler A-codegen Area - Code Generation A-isolated-declarations Isolated Declarations C-enhancement Category - New feature or request labels May 13, 2025
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR reworks comment printing by decoupling normal, annotation, and legal comments, updating both snapshot tests and code generation logic.

  • Updated snapshot tests to reflect separate comment types and improved clarity of expected outputs.
  • Refactored the codegen implementation by removing cached comment flags and using inline options calls, as well as updating documentation and helper methods for printing comments.
  • Adjusted enum discriminant values in the AST for comment annotations.

Reviewed Changes

Copilot reviewed 29 out of 29 changed files in this pull request and generated no comments.

File Description
crates/oxc_isolated_declarations/tests/snapshots/*.snap Updated snapshots with explicit markers for correct/incorrect comment interpretations.
crates/oxc_codegen/tests/integration/* Introduced #[track_caller] and switched to helper constructors, improving debugging and consistency.
crates/oxc_codegen/src/* Refactored comment printing logic and options handling; updated inline documentation.
crates/oxc_ast/src/ast/comment.rs Reordered enum variant values for comment annotations.
Comments suppressed due to low confidence (2)

crates/oxc_codegen/src/lib.rs:140

  • [nitpick] Removing cached boolean flags in favor of directly calling the options methods improves maintainability, but please verify that the repeated option accessor calls do not introduce any noticeable performance overhead in critical codegen paths.
/* removed cached print_any_comment, print_legal_comment, print_annotation_comment */

crates/oxc_ast/src/ast/comment.rs:55

  • Changing the numeric discriminant for enum variants (swapping Legal and Jsdoc values) might be a breaking change if these values are assumed for serialization or pattern matching. Please confirm that this change is intentional and that all downstream consumers are updated accordingly.
Legal = 1,

@codspeed-hq
Copy link

codspeed-hq bot commented May 13, 2025

CodSpeed Instrumentation Performance Report

Merging #10997 will not alter performance

Comparing 05-12-fix_codegen_correct_print_normal___legal___annotation_comments (ef3fce2) with main (13dbcc6)

Summary

✅ 33 untouched benchmarks
🆕 3 new benchmarks

Benchmarks breakdown

Benchmark BASE HEAD Change
🆕 linter[RadixUIAdoptionSection.jsx] N/A 2.8 ms N/A
🆕 linter[cal.com.tsx] N/A 1.2 s N/A
🆕 linter[checker.ts] N/A 3.1 s N/A

@Boshen
Copy link
Member Author

Boshen commented May 13, 2025

Tested in Rolldown, things are working as intended.

@Boshen Boshen merged commit 1673ffb into main May 13, 2025
28 checks passed
@Boshen Boshen deleted the 05-12-fix_codegen_correct_print_normal___legal___annotation_comments branch May 13, 2025 11:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-ast Area - AST A-codegen Area - Code Generation A-isolated-declarations Isolated Declarations A-transformer Area - Transformer / Transpiler C-enhancement Category - New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

CodegenOptions::print_legal_comment doesn't make any effect

2 participants