diff --git a/crates/oxc_linter/src/rules/eslint/no_import_assign.rs b/crates/oxc_linter/src/rules/eslint/no_import_assign.rs index 41cd693c36eff..dbc6200708710 100644 --- a/crates/oxc_linter/src/rules/eslint/no_import_assign.rs +++ b/crates/oxc_linter/src/rules/eslint/no_import_assign.rs @@ -124,7 +124,7 @@ fn is_argument_of_well_known_mutation_function(node_id: AstNodeId, ctx: &LintCon if ((ident.name == "Object" && OBJECT_MUTATION_METHODS.contains(property_name)) || (ident.name == "Reflect" && REFLECT_MUTATION_METHODS.contains(property_name))) - && !ctx.scopes().has_binding(current_node.scope_id(), &ident.name) + && ident.reference_id.get().is_some_and(|id| !ctx.symbols().has_binding(id)) { return expr .arguments diff --git a/crates/oxc_mangler/src/lib.rs b/crates/oxc_mangler/src/lib.rs index 2b5c23576023a..ec42f7742d7f6 100644 --- a/crates/oxc_mangler/src/lib.rs +++ b/crates/oxc_mangler/src/lib.rs @@ -104,24 +104,8 @@ impl ManglerBuilder { let mut slot = parent_max_slot; if !bindings.is_empty() { - let mut parent_bindings = None; - // `bindings` are stored in order, traverse and increment slot for symbol_id in bindings.values() { - // omit var hoisting because var symbols are added to every parent scope - if symbol_table.get_flag(*symbol_id).is_function_scoped_declaration() - && parent_bindings.is_none() - { - parent_bindings = scope_tree - .get_parent_id(scope_id) - .map(|parent_scope_id| scope_tree.get_bindings(parent_scope_id)); - } - if let Some(parent_bindings) = &parent_bindings { - if parent_bindings.values().contains(symbol_id) { - continue; - } - } - slots[*symbol_id] = slot; slot += 1; } diff --git a/crates/oxc_semantic/src/binder.rs b/crates/oxc_semantic/src/binder.rs index c095ec0f2da14..40a1347714ab3 100644 --- a/crates/oxc_semantic/src/binder.rs +++ b/crates/oxc_semantic/src/binder.rs @@ -12,14 +12,13 @@ use oxc_span::SourceType; use crate::{scope::ScopeFlags, symbol::SymbolFlags, SemanticBuilder}; -pub trait Binder { +pub trait Binder<'a> { #[allow(unused_variables)] - fn bind(&self, builder: &mut SemanticBuilder) {} + fn bind(&self, builder: &mut SemanticBuilder<'a>) {} } -impl<'a> Binder for VariableDeclarator<'a> { - fn bind(&self, builder: &mut SemanticBuilder) { - let current_scope_id = builder.current_scope_id; +impl<'a> Binder<'a> for VariableDeclarator<'a> { + fn bind(&self, builder: &mut SemanticBuilder<'a>) { let (includes, excludes) = match self.kind { VariableDeclarationKind::Const => ( SymbolFlags::BlockScopedVariable | SymbolFlags::ConstVariable, @@ -41,48 +40,63 @@ impl<'a> Binder for VariableDeclarator<'a> { return; } - // Logic for scope hoisting `var` - + // ------------------ var hosting ------------------ + let mut target_scope_id = builder.current_scope_id; let mut var_scope_ids = vec![]; - if !builder.current_scope_flags().is_var() { - for scope_id in builder.scope.ancestors(current_scope_id).skip(1) { - let flag = builder.scope.get_flags(scope_id); - // Skip the catch clause, the scope bindings have been cloned to the child block scope - if flag.is_catch_clause() { - continue; - } - var_scope_ids.push(scope_id); - if flag.is_var() { - break; - } + + // Collect all scopes where variable hoisting can occur + for scope_id in builder.scope.ancestors(target_scope_id) { + let flag = builder.scope.get_flags(scope_id); + if flag.is_var() { + target_scope_id = scope_id; + break; } + var_scope_ids.push(scope_id); } self.id.bound_names(&mut |ident| { let span = ident.span; let name = &ident.name; + let mut declared_symbol_id = None; for scope_id in &var_scope_ids { if let Some(symbol_id) = builder.check_redeclaration(*scope_id, span, name, excludes, true) { - ident.symbol_id.set(Some(symbol_id)); - builder.add_redeclare_variable(symbol_id, ident.span); - return; + builder.add_redeclare_variable(symbol_id, span); + declared_symbol_id = Some(symbol_id); + + let name = name.to_compact_str(); + // remove current scope binding and add to target scope + // avoid same symbols appear in multi-scopes + builder.scope.remove_binding(*scope_id, &name); + builder.scope.add_binding(target_scope_id, name, symbol_id); + break; } } - let symbol_id = - builder.declare_symbol_on_scope(span, name, current_scope_id, includes, excludes); + // If a variable is already declared in the hoisted scopes, + // we don't need to create another symbol with the same name + // to make sure they point to the same symbol. + let symbol_id = declared_symbol_id.unwrap_or_else(|| { + builder.declare_symbol_on_scope(span, name, target_scope_id, includes, excludes) + }); ident.symbol_id.set(Some(symbol_id)); + + // Finally, add the variable to all hoisted scopes + // to support redeclaration checks when declaring variables with the same name later. for scope_id in &var_scope_ids { - builder.scope.add_binding(*scope_id, name.to_compact_str(), symbol_id); + builder + .hoisting_variables + .entry(*scope_id) + .or_default() + .insert(name.clone(), symbol_id); } }); } } -impl<'a> Binder for Class<'a> { +impl<'a> Binder<'a> for Class<'a> { fn bind(&self, builder: &mut SemanticBuilder) { if !self.declare { let Some(ident) = &self.id else { return }; @@ -109,7 +123,7 @@ fn function_as_var(flags: ScopeFlags, source_type: SourceType) -> bool { flags.is_function() || (source_type.is_script() && flags.is_top()) } -impl<'a> Binder for Function<'a> { +impl<'a> Binder<'a> for Function<'a> { fn bind(&self, builder: &mut SemanticBuilder) { let current_scope_id = builder.current_scope_id; let scope_flags = builder.current_scope_flags(); @@ -170,7 +184,7 @@ impl<'a> Binder for Function<'a> { } } -impl<'a> Binder for BindingRestElement<'a> { +impl<'a> Binder<'a> for BindingRestElement<'a> { // Binds the FormalParameters's rest of a function or method. fn bind(&self, builder: &mut SemanticBuilder) { let parent_kind = builder.nodes.parent_kind(builder.current_node_id).unwrap(); @@ -192,7 +206,7 @@ impl<'a> Binder for BindingRestElement<'a> { } } -impl<'a> Binder for FormalParameter<'a> { +impl<'a> Binder<'a> for FormalParameter<'a> { // Binds the FormalParameter of a function or method. fn bind(&self, builder: &mut SemanticBuilder) { let parent_kind = builder.nodes.parent_kind(builder.current_node_id).unwrap(); @@ -231,7 +245,7 @@ impl<'a> Binder for FormalParameter<'a> { } } -impl<'a> Binder for CatchParameter<'a> { +impl<'a> Binder<'a> for CatchParameter<'a> { fn bind(&self, builder: &mut SemanticBuilder) { let current_scope_id = builder.current_scope_id; // https://tc39.es/ecma262/#sec-variablestatements-in-catch-blocks @@ -279,31 +293,31 @@ fn declare_symbol_for_import_specifier( ident.symbol_id.set(Some(symbol_id)); } -impl<'a> Binder for ImportSpecifier<'a> { +impl<'a> Binder<'a> for ImportSpecifier<'a> { fn bind(&self, builder: &mut SemanticBuilder) { declare_symbol_for_import_specifier(&self.local, self.import_kind.is_type(), builder); } } -impl<'a> Binder for ImportDefaultSpecifier<'a> { +impl<'a> Binder<'a> for ImportDefaultSpecifier<'a> { fn bind(&self, builder: &mut SemanticBuilder) { declare_symbol_for_import_specifier(&self.local, false, builder); } } -impl<'a> Binder for ImportNamespaceSpecifier<'a> { +impl<'a> Binder<'a> for ImportNamespaceSpecifier<'a> { fn bind(&self, builder: &mut SemanticBuilder) { declare_symbol_for_import_specifier(&self.local, false, builder); } } -impl<'a> Binder for TSImportEqualsDeclaration<'a> { +impl<'a> Binder<'a> for TSImportEqualsDeclaration<'a> { fn bind(&self, builder: &mut SemanticBuilder) { declare_symbol_for_import_specifier(&self.id, false, builder); } } -impl<'a> Binder for TSTypeAliasDeclaration<'a> { +impl<'a> Binder<'a> for TSTypeAliasDeclaration<'a> { fn bind(&self, builder: &mut SemanticBuilder) { let symbol_id = builder.declare_symbol( self.id.span, @@ -315,7 +329,7 @@ impl<'a> Binder for TSTypeAliasDeclaration<'a> { } } -impl<'a> Binder for TSInterfaceDeclaration<'a> { +impl<'a> Binder<'a> for TSInterfaceDeclaration<'a> { fn bind(&self, builder: &mut SemanticBuilder) { let symbol_id = builder.declare_symbol( self.id.span, @@ -327,7 +341,7 @@ impl<'a> Binder for TSInterfaceDeclaration<'a> { } } -impl<'a> Binder for TSEnumDeclaration<'a> { +impl<'a> Binder<'a> for TSEnumDeclaration<'a> { fn bind(&self, builder: &mut SemanticBuilder) { let is_const = self.r#const; let includes = if is_const { SymbolFlags::ConstEnum } else { SymbolFlags::RegularEnum }; @@ -341,7 +355,7 @@ impl<'a> Binder for TSEnumDeclaration<'a> { } } -impl<'a> Binder for TSEnumMember<'a> { +impl<'a> Binder<'a> for TSEnumMember<'a> { fn bind(&self, builder: &mut SemanticBuilder) { // TODO: Perf if self.id.is_expression() { @@ -362,7 +376,7 @@ impl<'a> Binder for TSEnumMember<'a> { } } -impl<'a> Binder for TSModuleDeclaration<'a> { +impl<'a> Binder<'a> for TSModuleDeclaration<'a> { fn bind(&self, builder: &mut SemanticBuilder) { // At declaration time a module has no value declaration it is only when a value declaration // is made inside a the scope of a module that the symbol is modified @@ -376,7 +390,7 @@ impl<'a> Binder for TSModuleDeclaration<'a> { } } -impl<'a> Binder for TSTypeParameter<'a> { +impl<'a> Binder<'a> for TSTypeParameter<'a> { fn bind(&self, builder: &mut SemanticBuilder) { let symbol_id = builder.declare_symbol( self.name.span, diff --git a/crates/oxc_semantic/src/builder.rs b/crates/oxc_semantic/src/builder.rs index 0fee58d625679..8481dbbd1ee86 100644 --- a/crates/oxc_semantic/src/builder.rs +++ b/crates/oxc_semantic/src/builder.rs @@ -15,6 +15,7 @@ use oxc_cfg::{ use oxc_diagnostics::OxcDiagnostic; use oxc_span::{Atom, CompactStr, SourceType, Span}; use oxc_syntax::{module_record::ModuleRecord, operator::AssignmentOperator}; +use rustc_hash::FxHashMap; use crate::{ binder::Binder, @@ -27,7 +28,7 @@ use crate::{ module_record::ModuleRecordBuilder, node::{AstNodeId, AstNodes, NodeFlags}, reference::{Reference, ReferenceFlag, ReferenceId}, - scope::{ScopeFlags, ScopeId, ScopeTree}, + scope::{Bindings, ScopeFlags, ScopeId, ScopeTree}, symbol::{SymbolFlags, SymbolId, SymbolTable}, unresolved_stack::UnresolvedReferencesStack, JSDocFinder, Semantic, @@ -66,6 +67,7 @@ pub struct SemanticBuilder<'a> { // to value like pub namespace_stack: Vec, current_reference_flag: ReferenceFlag, + pub hoisting_variables: FxHashMap, SymbolId>>, // builders pub nodes: AstNodes<'a>, @@ -114,6 +116,7 @@ impl<'a> SemanticBuilder<'a> { function_stack: vec![], namespace_stack: vec![], nodes: AstNodes::default(), + hoisting_variables: FxHashMap::default(), scope, symbols: SymbolTable::default(), unresolved_references: UnresolvedReferencesStack::new(), @@ -357,14 +360,16 @@ impl<'a> SemanticBuilder<'a> { } pub fn check_redeclaration( - &mut self, + &self, scope_id: ScopeId, span: Span, name: &str, excludes: SymbolFlags, report_error: bool, ) -> Option { - let symbol_id = self.scope.get_binding(scope_id, name)?; + let symbol_id = self.scope.get_binding(scope_id, name).or_else(|| { + self.hoisting_variables.get(&scope_id).and_then(|symbols| symbols.get(name).copied()) + })?; if report_error && self.symbols.get_flag(symbol_id).intersects(excludes) { let symbol_span = self.symbols.get_span(symbol_id); self.error(redeclaration(name, symbol_span, span)); @@ -496,10 +501,11 @@ impl<'a> Visit<'a> for SemanticBuilder<'a> { scope_id.set(Some(self.current_scope_id)); if self.scope.get_flags(parent_scope_id).is_catch_clause() { - // Clone the `CatchClause` bindings and add them to the current scope. - // to make it easier to check redeclare errors. - let bindings = self.scope.get_bindings(parent_scope_id).clone(); - self.scope.get_bindings_mut(self.current_scope_id).extend(bindings); + // Move all bindings from parent scope to current scope + // to make it easier to resole references and check redeclare errors. + let parent_bindings = + self.scope.get_bindings_mut(parent_scope_id).drain(..).collect::(); + *self.scope.get_bindings_mut(self.current_scope_id) = parent_bindings; } self.unresolved_references.increment_scope_depth(); diff --git a/crates/oxc_semantic/src/scope.rs b/crates/oxc_semantic/src/scope.rs index 4f16838d1238d..f6663ddcd0142 100644 --- a/crates/oxc_semantic/src/scope.rs +++ b/crates/oxc_semantic/src/scope.rs @@ -11,7 +11,7 @@ use crate::{symbol::SymbolId, AstNodeId}; type FxIndexMap = IndexMap>; -type Bindings = FxIndexMap; +pub(crate) type Bindings = FxIndexMap; pub(crate) type UnresolvedReference = (ReferenceId, ReferenceFlag); pub type UnresolvedReferences = FxHashMap>; diff --git a/crates/oxc_semantic/tests/fixtures/typescript-eslint/catch/inherited-scope.snap b/crates/oxc_semantic/tests/fixtures/typescript-eslint/catch/inherited-scope.snap index 791e50af8192f..777b6d3a8f86e 100644 --- a/crates/oxc_semantic/tests/fixtures/typescript-eslint/catch/inherited-scope.snap +++ b/crates/oxc_semantic/tests/fixtures/typescript-eslint/catch/inherited-scope.snap @@ -33,15 +33,7 @@ input_file: crates/oxc_semantic/tests/fixtures/typescript-eslint/catch/inherited "flag": "ScopeFlags(StrictMode | CatchClause)", "id": 2, "node": "CatchClause", - "symbols": [ - { - "flag": "SymbolFlags(FunctionScopedVariable | CatchVariable)", - "id": 1, - "name": "e", - "node": "CatchParameter", - "references": [] - } - ] + "symbols": [] } ], "flag": "ScopeFlags(StrictMode | Top)", diff --git a/crates/oxc_semantic/tests/fixtures/typescript-eslint/catch/scope.snap b/crates/oxc_semantic/tests/fixtures/typescript-eslint/catch/scope.snap index 7240bee7c46d2..18192f91af651 100644 --- a/crates/oxc_semantic/tests/fixtures/typescript-eslint/catch/scope.snap +++ b/crates/oxc_semantic/tests/fixtures/typescript-eslint/catch/scope.snap @@ -47,22 +47,7 @@ input_file: crates/oxc_semantic/tests/fixtures/typescript-eslint/catch/scope.ts "flag": "ScopeFlags(StrictMode | CatchClause)", "id": 2, "node": "CatchClause", - "symbols": [ - { - "flag": "SymbolFlags(FunctionScopedVariable | CatchVariable)", - "id": 0, - "name": "e", - "node": "CatchParameter", - "references": [ - { - "flag": "ReferenceFlag(Read)", - "id": 0, - "name": "e", - "node_id": 8 - } - ] - } - ] + "symbols": [] } ], "flag": "ScopeFlags(StrictMode | Top)", diff --git a/tasks/coverage/parser_typescript.snap b/tasks/coverage/parser_typescript.snap index 2fbee292a82f1..580511698f200 100644 --- a/tasks/coverage/parser_typescript.snap +++ b/tasks/coverage/parser_typescript.snap @@ -2,7 +2,7 @@ commit: d8086f14 parser_typescript Summary: AST Parsed : 6444/6456 (99.81%) -Positive Passed: 6422/6456 (99.47%) +Positive Passed: 6421/6456 (99.46%) Negative Passed: 1160/5653 (20.52%) Expect Syntax Error: "compiler/ClassDeclaration10.ts" Expect Syntax Error: "compiler/ClassDeclaration11.ts" @@ -4639,6 +4639,19 @@ Expect to Parse: "compiler/withStatementInternalComments.ts" 2 │ /*1*/ with /*2*/ ( /*3*/ false /*4*/ ) /*5*/ {} · ──── ╰──── +Expect to Parse: "conformance/async/es6/asyncWithVarShadowing_es6.ts" + + × Identifier `x` has already been declared + ╭─[conformance/async/es6/asyncWithVarShadowing_es6.ts:130:14] + 129 │ } + 130 │ catch ({ x }) { + · ┬ + · ╰── `x` has already been declared here + 131 │ var x; + · ┬ + · ╰── It can not be redeclared here + 132 │ } + ╰──── Expect to Parse: "conformance/classes/propertyMemberDeclarations/staticPropertyNameConflicts.ts" × Classes may not have a static property named prototype @@ -6354,6 +6367,19 @@ Expect to Parse: "conformance/salsa/typeFromPropertyAssignmentWithExport.ts" · ── ╰──── + × Identifier `x` has already been declared + ╭─[compiler/constDeclarationShadowedByVarDeclaration.ts:4:11] + 3 │ { + 4 │ const x = 0; + · ┬ + · ╰── `x` has already been declared here + 5 │ + 6 │ var x = 0; + · ┬ + · ╰── It can not be redeclared here + 7 │ } + ╰──── + × Identifier `y` has already been declared ╭─[compiler/constDeclarationShadowedByVarDeclaration.ts:12:11] 11 │ {