Skip to content

Comments

feat(parser): report more invalid modifier locations#13368

Merged
Boshen merged 2 commits intooxc-project:mainfrom
ulrichstark:report-invalid-modifiers
Aug 29, 2025
Merged

feat(parser): report more invalid modifier locations#13368
Boshen merged 2 commits intooxc-project:mainfrom
ulrichstark:report-invalid-modifiers

Conversation

@ulrichstark
Copy link
Contributor

Changes

  • Added all 22 examples of Forbid invalid modifiers #11713 to tasks/coverage/misc/fail
  • Added Default and Export to modifiers
  • Added three new TS diagnostics: 1029, 1031, and 1070
  • Split off FunctionKind::Constructor from FunctionKind::ClassMethod to differentiate when verifying modifiers of formal parameter
  • Added multiple checks to verify modifiers

Results

  • "Negative Passed" percentage of parser_babel.snap and parser_typescript.snap increased
  • Better diagnostic messages in both snapshots
  • All new failing examples of original issue do really fail
  • Closes Forbid invalid modifiers #11713

Copilot AI review requested due to automatic review settings August 28, 2025 21:51
@github-actions github-actions bot added A-parser Area - Parser C-enhancement Category - New feature or request labels Aug 28, 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 enhances the TypeScript parser to better validate modifier locations and report more specific diagnostic errors when modifiers are used incorrectly. The changes implement stricter checks for where modifiers can be placed in TypeScript code.

  • Added support for tracking Default and Export modifiers in the modifier system
  • Introduced new TypeScript diagnostic error codes (1029, 1031, 1070) for invalid modifier usage
  • Split constructor functions from class methods to enable more precise modifier validation

Reviewed Changes

Copilot reviewed 7 out of 32 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
crates/oxc_parser/src/modifiers.rs Added Default and Export modifier variants with validation logic
crates/oxc_parser/src/lexer/kind.rs Extended modifier detection to include Default and Export tokens
crates/oxc_parser/src/js/mod.rs Added Constructor as a distinct function kind
crates/oxc_parser/src/js/function.rs Updated parameter modifier validation with constructor-specific rules
crates/oxc_parser/src/js/class.rs Added modifier validation for class elements and updated constructor parsing
crates/oxc_parser/src/ts/statement.rs Added modifier validation for type members
crates/oxc_parser/src/diagnostics.rs Added three new TypeScript diagnostic error functions

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@codspeed-hq
Copy link

codspeed-hq bot commented Aug 28, 2025

CodSpeed Instrumentation Performance Report

Merging #13368 will not alter performance

Comparing ulrichstark:report-invalid-modifiers (a4b8707) with main (72468f9)

Summary

✅ 38 untouched benchmarks

@ulrichstark ulrichstark force-pushed the report-invalid-modifiers branch from 31be6d7 to d45da2d Compare August 28, 2025 22:00
@ulrichstark
Copy link
Contributor Author

I'm unsure about the failing allocations CI check... I did run the command locally, committed my changes but it still failed. Just reset back and force pushed to remove the attempt of making this CI check happy.

@Boshen Boshen added the 0-merge Merge with Graphite Merge Queue label Aug 29, 2025
@graphite-app
Copy link
Contributor

graphite-app bot commented Aug 29, 2025

Merge activity

  • Aug 29, 2:59 AM UTC: @ulrichstark we removed the merge queue label because we could not find a Graphite account associated with your GitHub profile.

You must have a Graphite account in order to use the merge queue. Create an account and try again using this link

@graphite-app graphite-app bot removed the 0-merge Merge with Graphite Merge Queue label Aug 29, 2025
@Boshen
Copy link
Member

Boshen commented Aug 29, 2025

Great work!

@Boshen Boshen merged commit 903a150 into oxc-project:main Aug 29, 2025
25 checks passed
@ulrichstark ulrichstark deleted the report-invalid-modifiers branch August 29, 2025 05:02
Boshen pushed a commit that referenced this pull request Sep 9, 2025
Follow up to #13368.
Closes #11713 by fixing five more cases mentioned in
#11713 (comment).

Other changes:
- Added TS(1024) diagnostic
- Fixed the broken oxc-11713-13.ts test file
- Made TS(1070) not report redundantly if it's actually the
responsibility of TS(1071)
- Branch off the more specific TS(1028) diagnostic from the general
TS(1030) diagnostic like TypeScript does
- Remove incorrect code to parse modifiers on typescript this parameter
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-parser Area - Parser C-enhancement Category - New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Forbid invalid modifiers

2 participants