Skip to content

Commit

Permalink
refactor(semantic): var hoisting (#4379)
Browse files Browse the repository at this point in the history 
close: #4323

This PR refactors the var hoisting logic to avoid inserting the same symbol into every scope that can be hosted.
  • Loading branch information
@Dunqing
Dunqing committed Jul 25, 2024
1 parent fd363d1 commit 7cd53f3
Show file tree
Hide file tree
Showing 8 changed files with 97 additions and 90 deletions.
2 changes: 1 addition & 1 deletion crates/oxc_linter/src/rules/eslint/no_import_assign.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
16 changes: 0 additions & 16 deletions crates/oxc_mangler/src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down
92 changes: 53 additions & 39 deletions crates/oxc_semantic/src/binder.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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 };
Expand All @@ -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();
Expand Down Expand Up @@ -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();
Expand All @@ -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();
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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,
Expand All @@ -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,
Expand All @@ -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 };
Expand All @@ -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() {
Expand All @@ -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
Expand All @@ -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,
Expand Down
20 changes: 13 additions & 7 deletions crates/oxc_semantic/src/builder.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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,
Expand Down Expand Up @@ -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>,
Expand Down Expand Up @@ -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(),
Expand Down Expand Up @@ -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));
Expand Down Expand Up @@ -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::<Bindings>();
*self.scope.get_bindings_mut(self.current_scope_id) = parent_bindings;
}

self.unresolved_references.increment_scope_depth();
Expand Down
2 changes: 1 addition & 1 deletion crates/oxc_semantic/src/scope.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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>>;

Expand Down
10 changes: 1 addition & 9 deletions crates/oxc_semantic/tests/fixtures/typescript-eslint/catch/inherited-scope.snap
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)",
Expand Down
17 changes: 1 addition & 16 deletions crates/oxc_semantic/tests/fixtures/typescript-eslint/catch/scope.snap
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)",
Expand Down
Loading

0 comments on commit 7cd53f3

Please sign in to comment.