From dae5f628b0d2f8a424b5dabfba2ea870396aba6b Mon Sep 17 00:00:00 2001 From: Dunqing Date: Wed, 3 Jan 2024 12:57:03 +0800 Subject: [PATCH] perf(semantic): check duplicate parameters in Binder of FormalParameters (#1840) --- .../src/rules/unicorn/no_array_for_each.rs | 4 +- .../src/snapshots/no_array_for_each.snap | 4 +- crates/oxc_semantic/src/binder.rs | 27 +++++++- crates/oxc_semantic/src/checker/javascript.rs | 26 +------- crates/oxc_semantic/src/checker/typescript.rs | 22 ++++++- tasks/coverage/parser_test262.snap | 24 +++---- tasks/coverage/parser_typescript.snap | 66 +++++++++---------- 7 files changed, 95 insertions(+), 78 deletions(-) diff --git a/crates/oxc_linter/src/rules/unicorn/no_array_for_each.rs b/crates/oxc_linter/src/rules/unicorn/no_array_for_each.rs index 7c4f5721b..9eb3115b1 100644 --- a/crates/oxc_linter/src/rules/unicorn/no_array_for_each.rs +++ b/crates/oxc_linter/src/rules/unicorn/no_array_for_each.rs @@ -118,8 +118,8 @@ fn test() { r"array.forEach(({foo}, index = foo) => {})", r"array.forEach((element, {bar = element}) => {})", r"array.forEach(({foo}, {bar = foo}) => {})", - r"foo.forEach(function(element, element) {})", - r"foo.forEach(function element(element, element) {})", + r"foo.forEach(function(element, element1) {})", + r"foo.forEach(function element(element, element1) {})", r"this._listeners.forEach((listener: () => void) => listener());", r"return foo.forEach(element => {bar(element)});", ]; diff --git a/crates/oxc_linter/src/snapshots/no_array_for_each.snap b/crates/oxc_linter/src/snapshots/no_array_for_each.snap index 913fe2e17..ed68b1904 100644 --- a/crates/oxc_linter/src/snapshots/no_array_for_each.snap +++ b/crates/oxc_linter/src/snapshots/no_array_for_each.snap @@ -74,14 +74,14 @@ expression: no_array_for_each ⚠ eslint-plugin-unicorn(no-array-for-each): Do not use `Array#forEach` ╭─[no_array_for_each.tsx:1:1] - 1 │ foo.forEach(function(element, element) {}) + 1 │ foo.forEach(function(element, element1) {}) · ─────── ╰──── help: Replace it with a for` loop. For loop is faster, more readable, and you can use `break` or `return` to exit early. ⚠ eslint-plugin-unicorn(no-array-for-each): Do not use `Array#forEach` ╭─[no_array_for_each.tsx:1:1] - 1 │ foo.forEach(function element(element, element) {}) + 1 │ foo.forEach(function element(element, element1) {}) · ─────── ╰──── help: Replace it with a for` loop. For loop is faster, more readable, and you can use `break` or `return` to exit early. diff --git a/crates/oxc_semantic/src/binder.rs b/crates/oxc_semantic/src/binder.rs index 82791187a..a306c4467 100644 --- a/crates/oxc_semantic/src/binder.rs +++ b/crates/oxc_semantic/src/binder.rs @@ -2,7 +2,10 @@ #[allow(clippy::wildcard_imports)] use oxc_ast::ast::*; -use oxc_ast::{syntax_directed_operations::BoundNames, AstKind}; +use oxc_ast::{ + syntax_directed_operations::{BoundNames, IsSimpleParameterList}, + AstKind, +}; use oxc_span::{Atom, SourceType}; use crate::{scope::ScopeFlags, symbol::SymbolFlags, SemanticBuilder, VariableInfo}; @@ -182,9 +185,29 @@ impl<'a> Binder for Function<'a> { } impl<'a> Binder for FormalParameters<'a> { + // Binds the formal parameters of a function or method. fn bind(&self, builder: &mut SemanticBuilder) { let includes = SymbolFlags::FunctionScopedVariable; - let excludes = SymbolFlags::FunctionScopedVariableExcludes; + + let is_not_allowed_duplicate_parameters = matches!( + self.kind, + // ArrowFormalParameters: UniqueFormalParameters + FormalParameterKind::ArrowFormalParameters | + // UniqueFormalParameters : FormalParameters + // * It is a Syntax Error if BoundNames of FormalParameters contains any duplicate elements. + FormalParameterKind::UniqueFormalParameters + ) || + // Multiple occurrences of the same BindingIdentifier in a FormalParameterList is only allowed for functions which have simple parameter lists and which are not defined in strict mode code. + builder.strict_mode() || + // FormalParameters : FormalParameterList + // * It is a Syntax Error if IsSimpleParameterList of FormalParameterList is false and BoundNames of FormalParameterList contains any duplicate elements. + !self.is_simple_parameter_list(); + + let excludes = if is_not_allowed_duplicate_parameters { + SymbolFlags::FunctionScopedVariable | SymbolFlags::FunctionScopedVariableExcludes + } else { + SymbolFlags::FunctionScopedVariableExcludes + }; let is_signature = self.kind == FormalParameterKind::Signature; self.bound_names(&mut |ident| { if !is_signature { diff --git a/crates/oxc_semantic/src/checker/javascript.rs b/crates/oxc_semantic/src/checker/javascript.rs index 306cfa803..ecf10feda 100644 --- a/crates/oxc_semantic/src/checker/javascript.rs +++ b/crates/oxc_semantic/src/checker/javascript.rs @@ -1,7 +1,7 @@ #[allow(clippy::wildcard_imports)] use oxc_ast::{ ast::*, - syntax_directed_operations::{BoundNames, IsSimpleParameterList, PropName}, + syntax_directed_operations::{IsSimpleParameterList, PropName}, AstKind, }; use oxc_diagnostics::{ @@ -15,7 +15,6 @@ use oxc_syntax::{ NumberBase, }; use phf::{phf_set, Set}; -use rustc_hash::FxHashMap; use crate::{builder::SemanticBuilder, diagnostics::Redeclaration, scope::ScopeFlags, AstNode}; @@ -157,15 +156,6 @@ fn check_module_record(ctx: &SemanticBuilder<'_>) { } } -fn check_duplicate_bound_names(bound_names: &T, ctx: &SemanticBuilder<'_>) { - let mut idents: FxHashMap = FxHashMap::default(); - bound_names.bound_names(&mut |ident| { - if let Some(old_span) = idents.insert(ident.name.clone(), ident.span) { - ctx.error(Redeclaration(ident.name.clone(), old_span, ident.span)); - } - }); -} - #[derive(Debug, Error, Diagnostic)] #[error("Cannot use await in class static initialization block")] #[diagnostic()] @@ -936,20 +926,6 @@ fn check_formal_parameters<'a>( ctx.error(ARestParameterCannotHaveAnInitializer(pat.span)); } } - - if params.is_empty() { - return; - } - - // Note: all other cases forbid duplicate parameter names. - if params.kind == FormalParameterKind::FormalParameter - && !ctx.strict_mode() - && params.is_simple_parameter_list() - { - return; - } - - check_duplicate_bound_names(params, ctx); } fn check_array_pattern(pattern: &ArrayPattern, ctx: &SemanticBuilder<'_>) { diff --git a/crates/oxc_semantic/src/checker/typescript.rs b/crates/oxc_semantic/src/checker/typescript.rs index 8d0ceec8c..80d58e252 100644 --- a/crates/oxc_semantic/src/checker/typescript.rs +++ b/crates/oxc_semantic/src/checker/typescript.rs @@ -1,12 +1,14 @@ +use oxc_ast::syntax_directed_operations::BoundNames; #[allow(clippy::wildcard_imports)] use oxc_ast::{ast::*, AstKind}; use oxc_diagnostics::{ miette::{self, Diagnostic}, thiserror::Error, }; -use oxc_span::{GetSpan, Span}; +use oxc_span::{Atom, GetSpan, Span}; +use rustc_hash::FxHashMap; -use crate::{builder::SemanticBuilder, AstNode}; +use crate::{builder::SemanticBuilder, diagnostics::Redeclaration, AstNode}; pub struct EarlyErrorTypeScript; @@ -18,11 +20,27 @@ impl EarlyErrorTypeScript { #[allow(clippy::single_match)] match kind { AstKind::SimpleAssignmentTarget(target) => check_simple_assignment_target(target, ctx), + AstKind::FormalParameters(params) => check_formal_parameters(params, ctx), _ => {} } } } +fn check_formal_parameters(params: &FormalParameters, ctx: &SemanticBuilder<'_>) { + if !params.is_empty() && params.kind == FormalParameterKind::Signature { + check_duplicate_bound_names(params, ctx); + } +} + +fn check_duplicate_bound_names(bound_names: &T, ctx: &SemanticBuilder<'_>) { + let mut idents: FxHashMap = FxHashMap::default(); + bound_names.bound_names(&mut |ident| { + if let Some(old_span) = idents.insert(ident.name.clone(), ident.span) { + ctx.error(Redeclaration(ident.name.clone(), old_span, ident.span)); + } + }); +} + fn check_simple_assignment_target<'a>( target: &SimpleAssignmentTarget<'a>, ctx: &SemanticBuilder<'a>, diff --git a/tasks/coverage/parser_test262.snap b/tasks/coverage/parser_test262.snap index e2b3e8a52..84d666282 100644 --- a/tasks/coverage/parser_test262.snap +++ b/tasks/coverage/parser_test262.snap @@ -14096,9 +14096,9 @@ Expect Syntax Error: "language/import/import-attributes/json-named-bindings.js" ╭─[language/expressions/function/param-duplicated-strict-3.js:21:1] 21 │ 22 │ (function (param, param, param) { }); - · ──┬── ──┬── - · │ ╰── It can not be redeclared here - · ╰── `param` has already been declared here + · ──┬── ──┬── + · │ ╰── It can not be redeclared here + · ╰── `param` has already been declared here ╰──── × Identifier `param` has already been declared @@ -14132,9 +14132,9 @@ Expect Syntax Error: "language/import/import-attributes/json-named-bindings.js" ╭─[language/expressions/function/param-duplicated-strict-body-3.js:20:1] 20 │ 21 │ (function (param, param, param) { 'use strict'; }); - · ──┬── ──┬── - · │ ╰── It can not be redeclared here - · ╰── `param` has already been declared here + · ──┬── ──┬── + · │ ╰── It can not be redeclared here + · ╰── `param` has already been declared here ╰──── × Cannot assign to 'eval' in strict mode @@ -31656,9 +31656,9 @@ Expect Syntax Error: "language/import/import-attributes/json-named-bindings.js" ╭─[language/statements/function/param-duplicated-strict-3.js:21:1] 21 │ 22 │ function _13_1_7_fun(param, param, param) { } - · ──┬── ──┬── - · │ ╰── It can not be redeclared here - · ╰── `param` has already been declared here + · ──┬── ──┬── + · │ ╰── It can not be redeclared here + · ╰── `param` has already been declared here ╰──── × Identifier `param` has already been declared @@ -31692,9 +31692,9 @@ Expect Syntax Error: "language/import/import-attributes/json-named-bindings.js" ╭─[language/statements/function/param-duplicated-strict-body-3.js:21:1] 21 │ 22 │ function _13_1_28_fun(param, param, param) { 'use strict'; } - · ──┬── ──┬── - · │ ╰── It can not be redeclared here - · ╰── `param` has already been declared here + · ──┬── ──┬── + · │ ╰── It can not be redeclared here + · ╰── `param` has already been declared here ╰──── × Cannot assign to 'eval' in strict mode diff --git a/tasks/coverage/parser_typescript.snap b/tasks/coverage/parser_typescript.snap index a7170a20b..847335d00 100644 --- a/tasks/coverage/parser_typescript.snap +++ b/tasks/coverage/parser_typescript.snap @@ -5905,9 +5905,9 @@ Expect to Parse: "conformance/salsa/plainJSRedeclare3.ts" ╭─[compiler/duplicateIdentifierBindingElementInParameterDeclaration1.ts:5:1] 5 │ function f2({b}, {b}) { } 6 │ function f3([c,[c],[[c]]]) { } - · ┬ ┬ - · │ ╰── It can not be redeclared here - · ╰── `c` has already been declared here + · ┬ ┬ + · │ ╰── It can not be redeclared here + · ╰── `c` has already been declared here 7 │ function f4({d, d:{d}}) { } ╰──── @@ -5935,9 +5935,9 @@ Expect to Parse: "conformance/salsa/plainJSRedeclare3.ts" ╭─[compiler/duplicateIdentifierBindingElementInParameterDeclaration1.ts:7:1] 7 │ function f4({d, d:{d}}) { } 8 │ function f5({e, e: {e}}, {e}, [d,e, [[e]]], ...e) { } - · ┬ ┬ - · │ ╰── It can not be redeclared here - · ╰── `e` has already been declared here + · ┬ ┬ + · │ ╰── It can not be redeclared here + · ╰── `e` has already been declared here 9 │ function f6([f, ...f]) { } ╰──── @@ -5945,9 +5945,9 @@ Expect to Parse: "conformance/salsa/plainJSRedeclare3.ts" ╭─[compiler/duplicateIdentifierBindingElementInParameterDeclaration1.ts:7:1] 7 │ function f4({d, d:{d}}) { } 8 │ function f5({e, e: {e}}, {e}, [d,e, [[e]]], ...e) { } - · ┬ ┬ - · │ ╰── It can not be redeclared here - · ╰── `e` has already been declared here + · ┬ ┬ + · │ ╰── It can not be redeclared here + · ╰── `e` has already been declared here 9 │ function f6([f, ...f]) { } ╰──── @@ -5955,9 +5955,9 @@ Expect to Parse: "conformance/salsa/plainJSRedeclare3.ts" ╭─[compiler/duplicateIdentifierBindingElementInParameterDeclaration1.ts:7:1] 7 │ function f4({d, d:{d}}) { } 8 │ function f5({e, e: {e}}, {e}, [d,e, [[e]]], ...e) { } - · ┬ ┬ - · │ ╰── It can not be redeclared here - · ╰── `e` has already been declared here + · ┬ ┬ + · │ ╰── It can not be redeclared here + · ╰── `e` has already been declared here 9 │ function f6([f, ...f]) { } ╰──── @@ -5965,9 +5965,9 @@ Expect to Parse: "conformance/salsa/plainJSRedeclare3.ts" ╭─[compiler/duplicateIdentifierBindingElementInParameterDeclaration1.ts:7:1] 7 │ function f4({d, d:{d}}) { } 8 │ function f5({e, e: {e}}, {e}, [d,e, [[e]]], ...e) { } - · ┬ ┬ - · │ ╰── It can not be redeclared here - · ╰── `e` has already been declared here + · ┬ ┬ + · │ ╰── It can not be redeclared here + · ╰── `e` has already been declared here 9 │ function f6([f, ...f]) { } ╰──── @@ -6035,9 +6035,9 @@ Expect to Parse: "conformance/salsa/plainJSRedeclare3.ts" ╭─[compiler/duplicateIdentifierBindingElementInParameterDeclaration2.ts:6:1] 6 │ function f2({b}, {b}) { } 7 │ function f3([c, [c], [[c]]]) { } - · ┬ ┬ - · │ ╰── It can not be redeclared here - · ╰── `c` has already been declared here + · ┬ ┬ + · │ ╰── It can not be redeclared here + · ╰── `c` has already been declared here 8 │ function f4({d, d: {d}}) { } ╰──── @@ -6065,9 +6065,9 @@ Expect to Parse: "conformance/salsa/plainJSRedeclare3.ts" ╭─[compiler/duplicateIdentifierBindingElementInParameterDeclaration2.ts:8:1] 8 │ function f4({d, d: {d}}) { } 9 │ function f5({e, e: {e}}, {e}, [d, e, [[e]]], ...e) { } - · ┬ ┬ - · │ ╰── It can not be redeclared here - · ╰── `e` has already been declared here + · ┬ ┬ + · │ ╰── It can not be redeclared here + · ╰── `e` has already been declared here 10 │ function f6([f, ...f]) { } ╰──── @@ -6075,9 +6075,9 @@ Expect to Parse: "conformance/salsa/plainJSRedeclare3.ts" ╭─[compiler/duplicateIdentifierBindingElementInParameterDeclaration2.ts:8:1] 8 │ function f4({d, d: {d}}) { } 9 │ function f5({e, e: {e}}, {e}, [d, e, [[e]]], ...e) { } - · ┬ ┬ - · │ ╰── It can not be redeclared here - · ╰── `e` has already been declared here + · ┬ ┬ + · │ ╰── It can not be redeclared here + · ╰── `e` has already been declared here 10 │ function f6([f, ...f]) { } ╰──── @@ -6085,9 +6085,9 @@ Expect to Parse: "conformance/salsa/plainJSRedeclare3.ts" ╭─[compiler/duplicateIdentifierBindingElementInParameterDeclaration2.ts:8:1] 8 │ function f4({d, d: {d}}) { } 9 │ function f5({e, e: {e}}, {e}, [d, e, [[e]]], ...e) { } - · ┬ ┬ - · │ ╰── It can not be redeclared here - · ╰── `e` has already been declared here + · ┬ ┬ + · │ ╰── It can not be redeclared here + · ╰── `e` has already been declared here 10 │ function f6([f, ...f]) { } ╰──── @@ -6095,9 +6095,9 @@ Expect to Parse: "conformance/salsa/plainJSRedeclare3.ts" ╭─[compiler/duplicateIdentifierBindingElementInParameterDeclaration2.ts:8:1] 8 │ function f4({d, d: {d}}) { } 9 │ function f5({e, e: {e}}, {e}, [d, e, [[e]]], ...e) { } - · ┬ ┬ - · │ ╰── It can not be redeclared here - · ╰── `e` has already been declared here + · ┬ ┬ + · │ ╰── It can not be redeclared here + · ╰── `e` has already been declared here 10 │ function f6([f, ...f]) { } ╰──── @@ -12342,9 +12342,9 @@ Expect to Parse: "conformance/salsa/plainJSRedeclare3.ts" ╭─[conformance/es6/destructuring/destructuringParameterDeclaration1ES6.ts:96:1] 96 │ 97 │ function e6({x: [number, number, number]}) { } // error, duplicate identifier; - · ───┬── ───┬── - · │ ╰── It can not be redeclared here - · ╰── `number` has already been declared here + · ───┬── ───┬── + · │ ╰── It can not be redeclared here + · ╰── `number` has already been declared here 98 │ ╰────