Skip to content

Conversation

@camc314
Copy link
Contributor

@camc314 camc314 commented Nov 24, 2025

Fixes #16077

@github-actions github-actions bot added A-parser Area - Parser C-bug Category - Bug labels Nov 24, 2025
Copy link
Contributor Author

camc314 commented Nov 24, 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.

This stack of pull requests is managed by Graphite. Learn more about stacking.

@camc314 camc314 marked this pull request as ready for review November 24, 2025 23:21
Copilot AI review requested due to automatic review settings November 24, 2025 23:21
Copilot finished reviewing on behalf of camc314 November 24, 2025 23:24
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 fixes the TypeScript parser to properly reject parameter properties that use binding patterns (array or object destructuring) instead of simple identifiers. The fix addresses issue #16077 by implementing TypeScript error TS(1187).

Key changes:

  • Added validation logic to reject parameter properties with destructuring patterns
  • Added comprehensive test cases for both valid and invalid scenarios
  • Updated coverage snapshots to reflect the newly passing TypeScript conformance tests

Reviewed changes

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

Show a summary per file
File Description
crates/oxc_parser/src/js/function.rs Implements validation to reject parameter properties with binding patterns when modifiers (public, private, protected, readonly, override) are present
crates/oxc_parser/src/diagnostics.rs Adds the TS(1187) diagnostic message for invalid parameter property patterns
tasks/coverage/misc/pass/oxc-16077.ts Test cases for valid parameter properties with simple identifiers
tasks/coverage/misc/fail/oxc-16077.ts Test cases for invalid parameter properties with destructuring patterns that should be rejected
tasks/coverage/snapshots/*.snap Updated coverage statistics showing improved TypeScript conformance (6 additional test cases now passing)

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

You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.

@codspeed-hq
Copy link

codspeed-hq bot commented Nov 24, 2025

CodSpeed Performance Report

Merging #16083 will not alter performance

Comparing c/11-24-fix_parser_reject_invalid_modifiers_on_parameter_properties_with_binding_patterns (371c7d1) with main (c773046)1

Summary

✅ 42 untouched
⏩ 3 skipped2

Footnotes

  1. No successful run was found on main (5bb3435) during the generation of this report, so c773046 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.

@Boshen Boshen added the 0-merge Merge with Graphite Merge Queue label Nov 25, 2025
Copy link
Member

Boshen commented Nov 25, 2025

Merge activity

  • Nov 25, 6:25 AM UTC: The merge label '0-merge' was detected. This PR will be added to the Graphite merge queue once it meets the requirements.
  • Nov 25, 6:25 AM UTC: This pull request can not be added to the Graphite merge queue. Please try rebasing and resubmitting to merge when ready.
  • Nov 25, 6:25 AM UTC: Graphite disabled "merge when ready" on this PR due to: a merge conflict with the target branch; resolve the conflict and try again..
  • Nov 25, 6:25 AM UTC: The merge label '0-merge' was detected. This PR will be added to the Graphite merge queue once it meets the requirements.
  • Nov 25, 7:53 AM UTC: Boshen added this pull request to the Graphite merge queue.
  • Nov 25, 7:53 AM UTC: The Graphite merge queue couldn't merge this PR because it had merge conflicts.
  • Nov 25, 9:01 AM UTC: The merge label '0-merge' was detected. This PR will be added to the Graphite merge queue once it meets the requirements.
  • Nov 25, 9:36 AM UTC: Graphite rebased this pull request, because this pull request is set to merge when ready.
  • Nov 25, 9:43 AM UTC: camc314 added this pull request to the Graphite merge queue.
  • Nov 25, 9:48 AM UTC: Merged by the Graphite merge queue.

@graphite-app graphite-app bot removed the 0-merge Merge with Graphite Merge Queue label Nov 25, 2025
@camc314 camc314 force-pushed the c/11-24-fix_parser_reject_invalid_modifiers_on_parameter_properties_with_binding_patterns branch from 28f865f to 371c7d1 Compare November 25, 2025 09:01
@camc314 camc314 added the 0-merge Merge with Graphite Merge Queue label Nov 25, 2025
@camc314 camc314 changed the base branch from main to graphite-base/16083 November 25, 2025 09:36
@camc314 camc314 force-pushed the c/11-24-fix_parser_reject_invalid_modifiers_on_parameter_properties_with_binding_patterns branch from 371c7d1 to a5ab05e Compare November 25, 2025 09:36
@camc314 camc314 changed the base branch from graphite-base/16083 to c/11-25-chore_all_format_generated_code November 25, 2025 09:36
@graphite-app graphite-app bot changed the base branch from c/11-25-chore_all_format_generated_code to main November 25, 2025 09:36
@github-actions github-actions bot added A-linter Area - Linter A-cli Area - CLI A-linter-plugins Area - Linter JS plugins labels Nov 25, 2025
@graphite-app graphite-app bot force-pushed the c/11-24-fix_parser_reject_invalid_modifiers_on_parameter_properties_with_binding_patterns branch from a5ab05e to 1199cee Compare November 25, 2025 09:43
@graphite-app graphite-app bot merged commit 1199cee into main Nov 25, 2025
21 checks passed
@graphite-app graphite-app bot deleted the c/11-24-fix_parser_reject_invalid_modifiers_on_parameter_properties_with_binding_patterns branch November 25, 2025 09:48
@graphite-app graphite-app bot removed the 0-merge Merge with Graphite Merge Queue label Nov 25, 2025
overlookmotel pushed a commit that referenced this pull request Dec 1, 2025
### 💥 BREAKING CHANGES

- 74cf572 ast: [**BREAKING**] Make `source` field of `TSImportType` a
`StringLiteral` (#16114) (copilot-swe-agent)
- 43156ae ast: [**BREAKING**] Rename `TSImportType` `argument` field to
`source` (#16110) (overlookmotel)
- 934d873 napi: [**BREAKING**] Drop `armv7-unknown-linux-musleabihf`
support (#16105) (Boshen)

### 🚀 Features

- 669afe0 ast: Add `Expression::is_jsx` method (#16154) (Dunqing)
- 17a8caa parser: Add diagnostic for JSX identifiers with hyphens
(#16133) (camchenry)
- 0549ae5 parser: Add diagnostic for expected ident after optional chain
(#16132) (camchenry)
- db839ae parser: Improve diagnostic for unexpected optional
declarations (#16131) (camchenry)
- bab4bc8 napi/parser: Add type annotations to parse-raw-worker test
(#15998) (camc314)

### 🐛 Bug Fixes

- 35ed36c traverse: Fix panic when truncating non-ASCII variable names
(#16265) (peter)
- 9149a26 linter/plugins, napi/parser: Deep freeze visitor keys (#16293)
(overlookmotel)
- 6b54dab minifier: Incorrect non-null object condition simplification
with `&&` and `||` (#16161) (sapphi-red)
- 9cc20a1 minifier: Avoid merging side effectful expressions to next
assignment statement if the side effect may change the left hand side
reference (#16165) (sapphi-red)
- 91eb3f2 ast/estree: Convert `TSImportType` `argument` field to
`Literal` (#16109) (overlookmotel)
- 1199cee parser: Reject invalid modifiers on parameter properties with
binding patterns (#16083) (camc314)
- f376325 traverse: Remove `console.log` from build script (#16049)
(overlookmotel)

### ⚡ Performance

- 82d784f lexer: Reduce bounds checks in `Lexer::get_string` (#16317)
(overlookmotel)
- cc2f352 span: Add `#[inline]` to `Atom` methods (#16311)
(overlookmotel)
- ffca070 span: Add `#[repr(transparent)]` to `Atom` (#16310)
(overlookmotel)
- 02bdf90 linter/plugins, napi/parser: Reuse arrays in visitor keys
(#16294) (overlookmotel)

### 📚 Documentation

- 891e0b4 parser: Add note about falling back to parse TSType in
TSImportType (#16119) (camc314)

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-cli Area - CLI A-linter Area - Linter A-linter-plugins Area - Linter JS plugins A-parser Area - Parser C-bug Category - Bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

parser: Reject invalid modifiers on paramter properties

3 participants