Skip to content

refactor(parser): shorten code handling illegal modifiers#20781

Merged
graphite-app[bot] merged 1 commit intomainfrom
om/03-26-refactor_parser_shorten_code_handling_illegal_modifiers
Mar 29, 2026
Merged

refactor(parser): shorten code handling illegal modifiers#20781
graphite-app[bot] merged 1 commit intomainfrom
om/03-26-refactor_parser_shorten_code_handling_illegal_modifiers

Conversation

@overlookmotel
Copy link
Copy Markdown
Member

@overlookmotel overlookmotel commented Mar 26, 2026

illegal_modifier_error contained a large amount of repetitious code, and was hard to read. Convert it to a shorter version which makes the logic clearer, reusing the lookup table introduced in #20748.

This change also reduces the size of generated code considerably and makes it faster, but #20748 moved this code onto a cold path, so that doesn't really matter. The main benefit is greater readability.

One test case produces a slightly different error after this change.

class D extends B {
  constructor(override readonly public foo: string) {}
}
- TS(1029): 'public' modifier must precede 'override' modifier.
+ TS(1029): 'public' modifier must precede 'readonly' modifier.

Both errors are correct, and which gets flagged was arbitrary before - it wasn't dependent on the order of modifiers in the source code, only the order of if branches in illegal_modifier_error. So IMO this change isn't important - it's just a different arbitrary order now.

Copy link
Copy Markdown
Member Author

overlookmotel commented Mar 26, 2026


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
Copy link
Copy Markdown

codspeed-hq bot commented Mar 26, 2026

Merging this PR will not alter performance

✅ 48 untouched benchmarks
⏩ 8 skipped benchmarks1


Comparing om/03-26-refactor_parser_shorten_code_handling_illegal_modifiers (bdb5960) with om/03-23-perf_parser_fast_path_for_no_syntax_errors_when_checking_modifiers (1e19687)

Open in CodSpeed

Footnotes

  1. 8 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.

@overlookmotel overlookmotel marked this pull request as ready for review March 26, 2026 23:47
Copilot AI review requested due to automatic review settings March 26, 2026 23:47
Copy link
Copy Markdown
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

Refactors the parser’s illegal_modifier_error logic to reduce repetition and improve readability, with a corresponding snapshot update for the one changed diagnostic message.

Changes:

  • Added ModifierKinds::intersection helper for bitset operations.
  • Rewrote illegal_modifier_error to compute illegal-preceding sets and emit diagnostics more compactly.
  • Updated the Babel parser snapshot to reflect the updated TS1029 message in one fixture.

Reviewed changes

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

File Description
tasks/coverage/snapshots/parser_babel.snap Updates expected TS1029 diagnostic text for one invalid-modifier-order fixture.
crates/oxc_parser/src/modifiers.rs Refactors illegal modifier error generation and adds a ModifierKinds intersection helper.

@overlookmotel overlookmotel force-pushed the om/03-23-perf_parser_fast_path_for_no_syntax_errors_when_checking_modifiers branch from b26e94c to 548a6ba Compare March 26, 2026 23:55
@overlookmotel overlookmotel force-pushed the om/03-26-refactor_parser_shorten_code_handling_illegal_modifiers branch from 1bfb403 to d924cd6 Compare March 26, 2026 23:55
@overlookmotel overlookmotel force-pushed the om/03-23-perf_parser_fast_path_for_no_syntax_errors_when_checking_modifiers branch from 548a6ba to 1e19687 Compare March 28, 2026 11:03
@overlookmotel overlookmotel force-pushed the om/03-26-refactor_parser_shorten_code_handling_illegal_modifiers branch from d924cd6 to bdb5960 Compare March 28, 2026 11:03
@overlookmotel overlookmotel requested a review from Copilot March 28, 2026 15:38
Copy link
Copy Markdown
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

Copilot reviewed 1 out of 2 changed files in this pull request and generated no new comments.

@camc314 camc314 added the 0-merge Merge with Graphite Merge Queue label Mar 29, 2026
Copy link
Copy Markdown
Contributor

camc314 commented Mar 29, 2026

Merge activity

  • Mar 29, 3:30 PM UTC: The merge label '0-merge' was detected. This PR will be added to the Graphite merge queue once it meets the requirements.
  • Mar 29, 3:31 PM UTC: camc314 added this pull request to the Graphite merge queue.
  • Mar 29, 3:35 PM UTC: The Graphite merge queue couldn't merge this PR because it had merge conflicts.
  • Mar 29, 3:36 PM UTC: The merge label '0-merge' was detected. This PR will be added to the Graphite merge queue once it meets the requirements.
  • Mar 29, 3:37 PM UTC: Graphite rebased this pull request, because this pull request is set to merge when ready.
  • Mar 29, 3:39 PM UTC: camc314 added this pull request to the Graphite merge queue.
  • Mar 29, 3:47 PM UTC: Merged by the Graphite merge queue.

@graphite-app graphite-app bot changed the base branch from om/03-23-perf_parser_fast_path_for_no_syntax_errors_when_checking_modifiers to graphite-base/20781 March 29, 2026 15:30
@graphite-app graphite-app bot changed the base branch from graphite-base/20781 to main March 29, 2026 15:34
@graphite-app graphite-app bot removed the 0-merge Merge with Graphite Merge Queue label Mar 29, 2026
@camc314 camc314 added the 0-merge Merge with Graphite Merge Queue label Mar 29, 2026 — with Graphite App
@camc314 camc314 force-pushed the om/03-26-refactor_parser_shorten_code_handling_illegal_modifiers branch from bdb5960 to 60b3c4e Compare March 29, 2026 15:36
`illegal_modifier_error` contained a large amount of repetitious code, and was hard to read. Convert it to a shorter version which makes the logic clearer, reusing the lookup table introduced in #20748.

This change also reduces the size of generated code considerably and makes it faster, but #20748 moved this code onto a cold path, so that doesn't really matter. The main benefit is greater readability.

One test case produces a slightly different error after this change.

```ts
class D extends B {
  constructor(override readonly public foo: string) {}
}
```

```diff
- TS(1029): 'public' modifier must precede 'override' modifier.
+ TS(1029): 'public' modifier must precede 'readonly' modifier.
```

Both errors are correct, and which gets flagged was arbitrary before - it wasn't dependent on the order of modifiers in the source code, only the order of `if` branches in `illegal_modifier_error`. So IMO this change isn't important - it's just a *different* arbitrary order now.
@graphite-app graphite-app bot force-pushed the om/03-26-refactor_parser_shorten_code_handling_illegal_modifiers branch from 60b3c4e to 2529cfb Compare March 29, 2026 15:41
@graphite-app graphite-app bot merged commit 2529cfb into main Mar 29, 2026
27 checks passed
@graphite-app graphite-app bot removed the 0-merge Merge with Graphite Merge Queue label Mar 29, 2026
@graphite-app graphite-app bot deleted the om/03-26-refactor_parser_shorten_code_handling_illegal_modifiers branch March 29, 2026 15:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-parser Area - Parser C-cleanup Category - technical debt or refactoring. Solution not expected to change behavior

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants