fix(es/parser): enforce UniqueFormalParameters - #12092
fix(es/parser): enforce UniqueFormalParameters#12092Felix-Ayush (Ayush7614) wants to merge 4 commits into
Conversation
🦋 Changeset detectedLatest commit: f62a934 The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c251b93b94
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "Codex (@codex) review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "Codex (@codex) address that feedback".
| }; | ||
|
|
||
| let validate_arrow_params = |p: &mut Self, params: &[Pat], is_async: bool| { | ||
| p.ensure_unique_formal_params(params.iter()); |
There was a problem hiding this comment.
Please route the other TypeScript arrow parsers through this new uniqueness check as well. This closure only runs in the main parenthesized-arrow path, but parse_paren_expr_or_arrow_fn returns earlier for the conditional-expression typed-arrow slow path, and try_parse_ts_generic_async_arrow_fn parses generic async arrows separately; in sloppy TS both still accept simple duplicates such as cond ? (a, a): number => a : 0 and async <T>(a, a) => a, so the new UniqueFormalParameters enforcement remains incomplete for arrows.
Useful? React with 👍 / 👎.
| if self.ctx().contains(Context::Strict) || !params.is_simple_parameter_list() { | ||
| self.ensure_unique_formal_params(params.iter().map(|p| &p.pat)); |
There was a problem hiding this comment.
Please don't make this depend only on the current Context::Strict for simple parameter lists. parse_program parses the file before it knows whether a later import/export will turn it into a module, and existing strict-mode checks use the module-error buffer for that reason; with this condition, function f(a, a) {} export {}; is parsed while still non-strict, so no duplicate is recorded before module mode is enabled. The same applies to module parser entry points that set Context::Module without Context::Strict, so module code can still accept simple duplicate function parameters.
Useful? React with 👍 / 👎.
Reject duplicate formal parameter bindings for methods, setters, constructors, arrows, strict-mode functions, non-simple parameter lists, and functions whose body contains a use strict directive, matching ECMA-262 early errors while still allowing simple duplicate parameters in sloppy-mode functions.
c251b93 to
d037fd7
Compare
Merging this PR will degrade performance by 4.05%
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ❌ | Simulation | typescript/fast-strip/graphile-config |
1.2 ms | 1.4 ms | -11.2% |
| ❌ | Simulation | es/parser/colors |
72.4 µs | 76.6 µs | -5.54% |
| ❌ | Simulation | es/parser/underscore |
2 ms | 2 ms | -4.01% |
| ❌ | Simulation | typescript/fast-strip-binding/graphile-config |
1.2 ms | 1.3 ms | -3.47% |
| ❌ | Simulation | es/parser/mootools |
9 ms | 9.3 ms | -3.43% |
| ❌ | Simulation | es/parser/angular |
14.3 ms | 14.8 ms | -3.37% |
| ❌ | Simulation | es/parser/typescript |
315.7 ms | 325.2 ms | -2.93% |
| ❌ | Simulation | es/parser/jquery |
11.6 ms | 11.9 ms | -2.82% |
| ❌ | Simulation | es/parser/backbone |
2.2 ms | 2.3 ms | -2.79% |
| ❌ | Simulation | es/parser/three |
55.7 ms | 57 ms | -2.33% |
| ❌ | Simulation | es/parser/jquery mobile |
17.8 ms | 18.3 ms | -2.32% |
Tip
Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.
Comparing Ayush7614:fix/unique-formal-parameters (f62a934) with main (5b758ed)
Footnotes
-
61 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. ↩
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d037fd7b23
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "Codex (@codex) review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "Codex (@codex) address that feedback".
| }; | ||
|
|
||
| let validate_arrow_params = |p: &mut Self, params: &[Pat], is_async: bool| { | ||
| p.ensure_unique_formal_params(params.iter()); |
There was a problem hiding this comment.
For non-async TypeScript generic arrows such as <T>(a, a) => a, this validation runs while parse_assignment_expr_base is being called from try_parse_ts, which sets Context::IgnoreError; emit_err therefore discards the duplicate-parameter diagnostic even though the speculative parse succeeds and returns the arrow. That leaves generic arrows with duplicate parameters accepted, so the uniqueness check needs to happen after the speculative parse commits or otherwise outside the ignored-error context.
Useful? React with 👍 / 👎.
Align no-dupe-args and test262 error snapshots with parser-emitted duplicate formal parameter diagnostics.
5e4d82b to
58fcfdf
Compare
Apply uniqueness checks after speculative TS arrow parses leave IgnoreError, buffer simple-list duplicates for auto-detected modules, and cover conditional typed / generic async arrow parsers.
58fcfdf to
b6d45d6
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b6d45d674a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "Codex (@codex) review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "Codex (@codex) address that feedback".
| } else { | ||
| self.ensure_unique_formal_params_strict(params.iter().map(|p| &p.pat)); |
There was a problem hiding this comment.
In TypeScript mode with TsSyntax { no_early_errors: true }, simple duplicate parameters in strict/module code still get reported through ensure_unique_formal_params_strict, because that path emits via emit_strict_mode_err, which bypasses the syntax().early_errors() guard used by emit_err. This makes parse_file_as_module/parse_program still surface the new DuplicateFormalParameter for inputs like function f(a, a) {} in a TS module even though callers explicitly disabled early errors, unlike the non-simple and arrow duplicate checks that go through emit_err and are suppressed.
Useful? React with 👍 / 👎.
| if let Expr::Arrow(ref arrow) = *res { | ||
| self.validate_arrow_params(&arrow.params, arrow.is_async); | ||
| } |
There was a problem hiding this comment.
Fresh evidence in the current patch is that this validation was added inside parse_assignment_expr_base, but TSX inputs that begin with < first go through the outer parse_assignment_expr Token::JSXTagStart try_parse_ts wrapper. For valid TSX generic arrows such as <T,>(a, a) => a or <T extends unknown>(a, a) => a, this line still runs while that outer wrapper has Context::IgnoreError set, so the duplicate-parameter diagnostic is discarded and the outer wrapper returns the arrow without another validation pass.
Useful? React with 👍 / 👎.
emit_strict_mode_err must respect TsSyntax::no_early_errors like emit_err. Outer TSX try_parse_ts also sets IgnoreError, so re-run validate_arrow_params after that speculative parse commits.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f62a934941
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "Codex (@codex) review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "Codex (@codex) address that feedback".
| if let Expr::Arrow(ref arrow) = *res { | ||
| self.validate_arrow_params(&arrow.params, arrow.is_async); | ||
| } |
There was a problem hiding this comment.
This post-commit check only revisits the outer generic arrow's arrow.params, but the whole speculative parse (including the arrow body) ran with Context::IgnoreError. As a result, duplicate-parameter diagnostics emitted for nested functions/arrows in the committed body are discarded; for example, const f = <T>() => (a, a) => a; or a block body containing function g([a], a) {} is accepted even though the new checks fire while errors are ignored. The committed arrow body needs the same deferred validation, or those checks need to run outside the speculative context.
Useful? React with 👍 / 👎.
2 participants
Description:
parse_unique_formal_paramswas a stub (FIXME: This is wrong) that only forwarded toparse_formal_params, so SWC accepted duplicate parameter bindings in places where ECMA-262 requires UniqueFormalParameters.This change enforces BoundNames uniqueness for:
set x({a, a}))"use strict"Sloppy-mode simple functions such as
function f(a, a) {}remain allowed, matching engines.Verified with fixture coverage under
tests/errors/unique-formal-params/**and a sloppy-mode.cjspass fixture.BREAKING CHANGE:
Invalid programs that previously parsed may now report a soft syntax error for duplicate formal parameters in UniqueFormalParameters contexts.
Related issue (if exists):
N/A (direct PR; no existing tracked issue)