Skip to content

feat(codegen): preserve comments between CatchClause's param and body#16167

Merged
graphite-app[bot] merged 1 commit intomainfrom
copilot/preserve-comments-in-catch-clause
Dec 5, 2025
Merged

feat(codegen): preserve comments between CatchClause's param and body#16167
graphite-app[bot] merged 1 commit intomainfrom
copilot/preserve-comments-in-catch-clause

Conversation

Copy link
Contributor

Copilot AI commented Nov 26, 2025

Fixes #16076

@graphite-app
Copy link
Contributor

graphite-app bot commented Nov 26, 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.

Copilot AI changed the title [WIP] Fix codegen to preserve comments in CatchClause feat(codegen): preserve comments between CatchClause's param and body Nov 26, 2025
Copilot AI requested a review from sapphi-red November 26, 2025 14:31
@sapphi-red sapphi-red marked this pull request as ready for review November 27, 2025 08:27
Copilot AI review requested due to automatic review settings November 27, 2025 08:27
@github-actions github-actions bot added A-parser Area - Parser A-codegen Area - Code Generation C-enhancement Category - New feature or request labels Nov 27, 2025
@codspeed-hq
Copy link

codspeed-hq bot commented Nov 27, 2025

CodSpeed Performance Report

Merging #16167 will not alter performance

Comparing copilot/preserve-comments-in-catch-clause (4faf829) with main (cfdfe7d)1

Summary

✅ 42 untouched
⏩ 3 skipped2

Footnotes

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

  2. 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.

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 adds support for preserving comments between a catch clause's parameter and body, enabling code coverage tools that use ignore hints (like /* v8 ignore next */) to properly instrument catch blocks.

Key Changes:

  • Modified comment parsing to treat line comments after ) as leading rather than trailing
  • Added comment printing logic in CatchClause codegen before the block body
  • Added manual handling of the print_next_indent_as_space flag to ensure proper spacing

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File Description
crates/oxc_parser/src/lexer/trivia_builder.rs Updated trailing comment detection to exclude Kind::RParen, making line comments after ) be treated as leading comments attached to the next token
crates/oxc_codegen/src/gen.rs Added print_comments_at(self.body.span.start) before printing catch block body, with manual handling of spacing flag for inline comments
crates/oxc_codegen/tests/integration/comments.rs Added three test cases covering inline block comments, multiple comments, and line comments between catch parameter and body
crates/oxc_codegen/tests/integration/snapshots/coverage.snap Added snapshot expectations for the three new test cases showing proper comment preservation

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@sapphi-red sapphi-red marked this pull request as draft November 27, 2025 09:53
Copilot AI requested a review from sapphi-red November 27, 2025 10:00
@sapphi-red sapphi-red force-pushed the copilot/preserve-comments-in-catch-clause branch from 78c730b to bc81ea4 Compare November 27, 2025 10:18
@sapphi-red
Copy link
Member

@copilot fix lint

Copy link
Contributor Author

Copilot AI commented Dec 5, 2025

@copilot fix lint

Fixed in 4faf829. Removed unused imports (CodegenOptions, CommentOptions, and snapshot_options) from comments.rs.

@sapphi-red sapphi-red marked this pull request as ready for review December 5, 2025 06:18
@sapphi-red sapphi-red requested review from Boshen and removed request for sapphi-red December 5, 2025 06:18
@sapphi-red sapphi-red assigned Boshen and unassigned sapphi-red and Copilot Dec 5, 2025
@Boshen Boshen added the 0-merge Merge with Graphite Merge Queue label Dec 5, 2025
Copy link
Member

Boshen commented Dec 5, 2025

Merge activity

@graphite-app graphite-app bot force-pushed the copilot/preserve-comments-in-catch-clause branch from 4faf829 to a607cc4 Compare December 5, 2025 06:29
@graphite-app graphite-app bot merged commit a607cc4 into main Dec 5, 2025
20 checks passed
@graphite-app graphite-app bot deleted the copilot/preserve-comments-in-catch-clause branch December 5, 2025 06:35
@graphite-app graphite-app bot removed the 0-merge Merge with Graphite Merge Queue label Dec 5, 2025
overlookmotel pushed a commit that referenced this pull request Dec 8, 2025
### 💥 BREAKING CHANGES

- 083fea9 napi/parser: [**BREAKING**] Represent empty optional fields on
JS side as `null` (#16411) (overlookmotel)

### 🚀 Features

- 7a2afee parser: Add TS1174 error for classes extending multiple base
classes (#15993) (sapphi-red)
- da87812 semantic: Add TS2309 error for export assignment with other
exports (#15992) (sapphi-red)
- d6d2bcd minifier: Remove unused function calls that are marked by
`manual_pure_functions` (#16534) (sapphi-red)
- c90f053 minifier: Support `.` separated values for
`compress.treeshake.manualPureFunctions` (#16529) (sapphi-red)
- a607cc4 codegen: Preserve comments between CatchClause's param and
body (#16167) (copilot-swe-agent)
- 8c10694 semantic: Expose get_comment_at method (#16439) (camc314)
- 3981e7a ast: Add get_comment_at to lookup a comment by span (#16438)
(camc314)

### 🐛 Bug Fixes

- 699406a napi/parser: Move `ExportEntry::module_request` field to first
(#16412) (overlookmotel)
- 12bd794 napi/parser: Move `ExportEntry::module_request` field to last
(#16403) (overlookmotel)

### ⚡ Performance

- 790beeb napi/parser: Do not remove extraneous options on JS side
(#16447) (overlookmotel)

Co-authored-by: Boshen <1430279+Boshen@users.noreply.github.com>
Copilot AI pushed a commit that referenced this pull request Dec 10, 2025
### 💥 BREAKING CHANGES

- 083fea9 napi/parser: [**BREAKING**] Represent empty optional fields on
JS side as `null` (#16411) (overlookmotel)

### 🚀 Features

- 7a2afee parser: Add TS1174 error for classes extending multiple base
classes (#15993) (sapphi-red)
- da87812 semantic: Add TS2309 error for export assignment with other
exports (#15992) (sapphi-red)
- d6d2bcd minifier: Remove unused function calls that are marked by
`manual_pure_functions` (#16534) (sapphi-red)
- c90f053 minifier: Support `.` separated values for
`compress.treeshake.manualPureFunctions` (#16529) (sapphi-red)
- a607cc4 codegen: Preserve comments between CatchClause's param and
body (#16167) (copilot-swe-agent)
- 8c10694 semantic: Expose get_comment_at method (#16439) (camc314)
- 3981e7a ast: Add get_comment_at to lookup a comment by span (#16438)
(camc314)

### 🐛 Bug Fixes

- 699406a napi/parser: Move `ExportEntry::module_request` field to first
(#16412) (overlookmotel)
- 12bd794 napi/parser: Move `ExportEntry::module_request` field to last
(#16403) (overlookmotel)

### ⚡ Performance

- 790beeb napi/parser: Do not remove extraneous options on JS side
(#16447) (overlookmotel)

Co-authored-by: Boshen <1430279+Boshen@users.noreply.github.com>
taearls pushed a commit to taearls/oxc that referenced this pull request Dec 11, 2025
### 💥 BREAKING CHANGES

- 083fea9 napi/parser: [**BREAKING**] Represent empty optional fields on
JS side as `null` (oxc-project#16411) (overlookmotel)

### 🚀 Features

- 7a2afee parser: Add TS1174 error for classes extending multiple base
classes (oxc-project#15993) (sapphi-red)
- da87812 semantic: Add TS2309 error for export assignment with other
exports (oxc-project#15992) (sapphi-red)
- d6d2bcd minifier: Remove unused function calls that are marked by
`manual_pure_functions` (oxc-project#16534) (sapphi-red)
- c90f053 minifier: Support `.` separated values for
`compress.treeshake.manualPureFunctions` (oxc-project#16529) (sapphi-red)
- a607cc4 codegen: Preserve comments between CatchClause's param and
body (oxc-project#16167) (copilot-swe-agent)
- 8c10694 semantic: Expose get_comment_at method (oxc-project#16439) (camc314)
- 3981e7a ast: Add get_comment_at to lookup a comment by span (oxc-project#16438)
(camc314)

### 🐛 Bug Fixes

- 699406a napi/parser: Move `ExportEntry::module_request` field to first
(oxc-project#16412) (overlookmotel)
- 12bd794 napi/parser: Move `ExportEntry::module_request` field to last
(oxc-project#16403) (overlookmotel)

### ⚡ Performance

- 790beeb napi/parser: Do not remove extraneous options on JS side
(oxc-project#16447) (overlookmotel)

Co-authored-by: Boshen <1430279+Boshen@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-codegen Area - Code Generation C-enhancement Category - New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

codegen: preserve comments between CatchClause's param and body

4 participants