refactor(semantic): var hoisting (#4379)

close: #4323

This PR refactors the var hoisting logic to avoid inserting the same symbol into every scope that can be hosted.

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

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;
                 }

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> {
+impl<'a> Binder<'a> for VariableDeclarator<'a> {
-    fn bind(&self, builder: &mut SemanticBuilder) {
+    fn bind(&self, builder: &mut SemanticBuilder<'a>) {
-        let current_scope_id = builder.current_scope_id;
         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) {
+        // 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);
-                // 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() {
+                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, span);
-                    builder.add_redeclare_variable(symbol_id, ident.span);
+                    declared_symbol_id = Some(symbol_id);
-                    return;
+
+                    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 =
+            // If a variable is already declared in the hoisted scopes,
-                builder.declare_symbol_on_scope(span, name, current_scope_id, includes, excludes);
+            // 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,

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<SymbolId>,
     current_reference_flag: ReferenceFlag,
+    pub hoisting_variables: FxHashMap<ScopeId, FxHashMap<Atom<'a>, 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<SymbolId> {
-        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.
+            // Move all bindings from parent scope to current scope
-            // to make it easier to check redeclare errors.
+            // to make it easier to resole references and check redeclare errors.
-            let bindings = self.scope.get_bindings(parent_scope_id).clone();
+            let parent_bindings =
-            self.scope.get_bindings_mut(self.current_scope_id).extend(bindings);
+                self.scope.get_bindings_mut(parent_scope_id).drain(..).collect::<Bindings>();
+            *self.scope.get_bindings_mut(self.current_scope_id) = parent_bindings;
         }
 
         self.unresolved_references.increment_scope_depth();

crates/oxc_semantic/src/scope.rs

@@ -11,7 +11,7 @@ use crate::{symbol::SymbolId, AstNodeId};
 
 type FxIndexMap<K, V> = IndexMap<K, V, BuildHasherDefault<FxHasher>>;
 
-type Bindings = FxIndexMap<CompactStr, SymbolId>;
+pub(crate) type Bindings = FxIndexMap<CompactStr, SymbolId>;
 pub(crate) type UnresolvedReference = (ReferenceId, ReferenceFlag);
 pub type UnresolvedReferences = FxHashMap<CompactStr, Vec<UnresolvedReference>>;
 

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": [
+        "symbols": []
-          {
-            "flag": "SymbolFlags(FunctionScopedVariable | CatchVariable)",
-            "id": 1,
-            "name": "e",
-            "node": "CatchParameter",
-            "references": []
-          }
-        ]
       }
     ],
     "flag": "ScopeFlags(StrictMode | Top)",

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": [
+        "symbols": []
-          {
-            "flag": "SymbolFlags(FunctionScopedVariable | CatchVariable)",
-            "id": 0,
-            "name": "e",
-            "node": "CatchParameter",
-            "references": [
-              {
-                "flag": "ReferenceFlag(Read)",
-                "id": 0,
-                "name": "e",
-                "node_id": 8
-              }
-            ]
-          }
-        ]
       }
     ],
     "flag": "ScopeFlags(StrictMode | Top)",

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 │ {