From 02f968d02d39e3d53b02ab5a4485ebc148a5ae80 Mon Sep 17 00:00:00 2001 From: Boshen <1430279+Boshen@users.noreply.github.com> Date: Thu, 19 Dec 2024 14:16:28 +0000 Subject: [PATCH] refactor(semantic): change `Bindings` to a plain `FxHashMap` (#8019) `IndexMap` was needed for the insertion order requirement in mangler. Bindings (symbol ids) are monotonically increasing by binding positions, which means we can get insertion order by sorting the symbol ids in mangler. Previous attempt: https://github.com/oxc-project/oxc/pull/4228 --- Cargo.lock | 1 - .../no_unsafe_declaration_merging.rs | 9 ++++---- crates/oxc_mangler/src/lib.rs | 6 ++++-- crates/oxc_semantic/Cargo.toml | 5 +---- crates/oxc_semantic/src/builder.rs | 21 ++++++------------- crates/oxc_semantic/src/scope.rs | 15 ++++++------- crates/oxc_semantic/tests/main.rs | 3 ++- crates/oxc_wasm/src/lib.rs | 4 ++-- tasks/transform_checker/src/lib.rs | 4 +++- 9 files changed, 29 insertions(+), 39 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 680ae0ab7..a6ccd065c 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1914,7 +1914,6 @@ name = "oxc_semantic" version = "0.42.0" dependencies = [ "assert-unchecked", - "indexmap", "insta", "itertools", "oxc_allocator", diff --git a/crates/oxc_linter/src/rules/typescript/no_unsafe_declaration_merging.rs b/crates/oxc_linter/src/rules/typescript/no_unsafe_declaration_merging.rs index 4d45d4f4f..60a5a7b55 100644 --- a/crates/oxc_linter/src/rules/typescript/no_unsafe_declaration_merging.rs +++ b/crates/oxc_linter/src/rules/typescript/no_unsafe_declaration_merging.rs @@ -43,9 +43,10 @@ impl Rule for NoUnsafeDeclarationMerging { match node.kind() { AstKind::Class(decl) => { if let Some(ident) = decl.id.as_ref() { - for (_, &symbol_id) in ctx.semantic().scopes().get_bindings(node.scope_id()) { + for symbol_id in ctx.semantic().scopes().get_bindings(node.scope_id()).values() + { if let AstKind::TSInterfaceDeclaration(scope_interface) = - get_symbol_kind(symbol_id, ctx) + get_symbol_kind(*symbol_id, ctx) { check_and_diagnostic(ident, &scope_interface.id, ctx); } @@ -53,8 +54,8 @@ impl Rule for NoUnsafeDeclarationMerging { } } AstKind::TSInterfaceDeclaration(decl) => { - for (_, &symbol_id) in ctx.semantic().scopes().get_bindings(node.scope_id()) { - if let AstKind::Class(scope_class) = get_symbol_kind(symbol_id, ctx) { + for symbol_id in ctx.semantic().scopes().get_bindings(node.scope_id()).values() { + if let AstKind::Class(scope_class) = get_symbol_kind(*symbol_id, ctx) { if let Some(scope_class_ident) = scope_class.id.as_ref() { check_and_diagnostic(&decl.id, scope_class_ident, ctx); } diff --git a/crates/oxc_mangler/src/lib.rs b/crates/oxc_mangler/src/lib.rs index 1eac2ef47..92b209f73 100644 --- a/crates/oxc_mangler/src/lib.rs +++ b/crates/oxc_mangler/src/lib.rs @@ -110,8 +110,10 @@ impl Mangler { let mut slot = parent_max_slot; if !bindings.is_empty() { - // `bindings` are stored in order, traverse and increment slot - for symbol_id in bindings.values().copied() { + // Sort `bindings` in declaration order. + let mut bindings = bindings.values().copied().collect::>(); + bindings.sort_unstable(); + for symbol_id in bindings { slots[symbol_id] = slot; slot += 1; } diff --git a/crates/oxc_semantic/Cargo.toml b/crates/oxc_semantic/Cargo.toml index 1b915140a..0afe38e35 100644 --- a/crates/oxc_semantic/Cargo.toml +++ b/crates/oxc_semantic/Cargo.toml @@ -30,16 +30,13 @@ oxc_span = { workspace = true } oxc_syntax = { workspace = true } assert-unchecked = { workspace = true } -indexmap = { workspace = true } itertools = { workspace = true } phf = { workspace = true, features = ["macros"] } rustc-hash = { workspace = true } [dev-dependencies] -oxc_parser = { workspace = true } - -indexmap = { workspace = true } insta = { workspace = true, features = ["glob"] } +oxc_parser = { workspace = true } phf = { workspace = true, features = ["macros"] } rustc-hash = { workspace = true } serde_json = { workspace = true } diff --git a/crates/oxc_semantic/src/builder.rs b/crates/oxc_semantic/src/builder.rs index 93aeac214..049addc0a 100644 --- a/crates/oxc_semantic/src/builder.rs +++ b/crates/oxc_semantic/src/builder.rs @@ -23,18 +23,9 @@ use oxc_syntax::{ }; use crate::{ - binder::Binder, - checker, - class::ClassTableBuilder, - diagnostics::redeclaration, - jsdoc::JSDocBuilder, - label::UnusedLabels, - node::AstNodes, - scope::{Bindings, ScopeTree}, - stats::Stats, - symbol::SymbolTable, - unresolved_stack::UnresolvedReferencesStack, - JSDocFinder, Semantic, + binder::Binder, checker, class::ClassTableBuilder, diagnostics::redeclaration, + jsdoc::JSDocBuilder, label::UnusedLabels, node::AstNodes, scope::ScopeTree, stats::Stats, + symbol::SymbolTable, unresolved_stack::UnresolvedReferencesStack, JSDocFinder, Semantic, }; macro_rules! control_flow { @@ -713,10 +704,10 @@ impl<'a> Visit<'a> for SemanticBuilder<'a> { if self.scope.get_flags(parent_scope_id).is_catch_clause() { let parent_bindings = self.scope.get_bindings_mut(parent_scope_id); if !parent_bindings.is_empty() { - let parent_bindings = parent_bindings.drain(..).collect::(); - parent_bindings.values().for_each(|&symbol_id| { + let parent_bindings = mem::take(parent_bindings); + for &symbol_id in parent_bindings.values() { self.symbols.set_scope_id(symbol_id, self.current_scope_id); - }); + } *self.scope.get_bindings_mut(self.current_scope_id) = parent_bindings; } } diff --git a/crates/oxc_semantic/src/scope.rs b/crates/oxc_semantic/src/scope.rs index aada47ae4..b1d5d0667 100644 --- a/crates/oxc_semantic/src/scope.rs +++ b/crates/oxc_semantic/src/scope.rs @@ -1,7 +1,6 @@ use std::mem; -use indexmap::IndexMap; -use rustc_hash::{FxBuildHasher, FxHashMap}; +use rustc_hash::FxHashMap; use oxc_index::IndexVec; use oxc_span::CompactStr; @@ -12,9 +11,7 @@ use oxc_syntax::{ symbol::SymbolId, }; -type FxIndexMap = IndexMap; - -pub(crate) type Bindings = FxIndexMap; +pub(crate) type Bindings = FxHashMap; pub type UnresolvedReferences = FxHashMap>; /// Scope Tree @@ -203,7 +200,7 @@ impl ScopeTree { /// Check if a symbol is declared in a certain scope. pub fn has_binding(&self, scope_id: ScopeId, name: &str) -> bool { - self.bindings[scope_id].get(name).is_some() + self.bindings[scope_id].contains_key(name) } /// Get the symbol bound to an identifier name in a scope. @@ -330,13 +327,13 @@ impl ScopeTree { /// Remove an existing binding from a scope. pub fn remove_binding(&mut self, scope_id: ScopeId, name: &CompactStr) { - self.bindings[scope_id].shift_remove(name); + self.bindings[scope_id].remove(name); } /// Move a binding from one scope to another. pub fn move_binding(&mut self, from: ScopeId, to: ScopeId, name: &str) { let from_map = &mut self.bindings[from]; - if let Some((name, symbol_id)) = from_map.swap_remove_entry(name) { + if let Some((name, symbol_id)) = from_map.remove_entry(name) { self.bindings[to].insert(name, symbol_id); } } @@ -365,7 +362,7 @@ impl ScopeTree { // Remove old entry. `swap_remove` swaps the last entry into the place of removed entry. // We just inserted the new entry as last, so the new entry takes the place of the old. // Order of entries is same as before, only the key has changed. - let old_symbol_id = bindings.swap_remove(old_name); + let old_symbol_id = bindings.remove(old_name); debug_assert_eq!(old_symbol_id, Some(symbol_id)); } diff --git a/crates/oxc_semantic/tests/main.rs b/crates/oxc_semantic/tests/main.rs index 18f3dee01..4de4a3e9b 100644 --- a/crates/oxc_semantic/tests/main.rs +++ b/crates/oxc_semantic/tests/main.rs @@ -46,7 +46,8 @@ fn get_scope_snapshot(semantic: &Semantic, scopes: impl Iterator .as_str(), ); result.push_str("\"symbols\": "); - let bindings = scope_tree.get_bindings(scope_id); + let mut bindings = scope_tree.get_bindings(scope_id).iter().collect::>(); + bindings.sort_unstable_by_key(|&(_, symbol_id)| symbol_id); result.push('['); bindings.iter().enumerate().for_each(|(index, (name, &symbol_id))| { if index != 0 { diff --git a/crates/oxc_wasm/src/lib.rs b/crates/oxc_wasm/src/lib.rs index fe6cf2c70..7516acaec 100644 --- a/crates/oxc_wasm/src/lib.rs +++ b/crates/oxc_wasm/src/lib.rs @@ -387,10 +387,10 @@ impl Oxc { let bindings = self.scopes.get_bindings(scope_id); if !bindings.is_empty() { self.write_line("Bindings: {"); - bindings.iter().for_each(|(name, &symbol_id)| { + for (name, &symbol_id) in bindings { let symbol_flags = self.symbols.get_flags(symbol_id); self.write_line(format!(" {name} ({symbol_id:?} {symbol_flags:?})",)); - }); + } self.write_line("}"); } } diff --git a/tasks/transform_checker/src/lib.rs b/tasks/transform_checker/src/lib.rs index 1d957f2ef..cae83c7f2 100644 --- a/tasks/transform_checker/src/lib.rs +++ b/tasks/transform_checker/src/lib.rs @@ -346,10 +346,12 @@ impl PostTransformChecker<'_, '_> { if binding_names.is_mismatch() { self.errors.push_mismatch("Bindings mismatch", scope_ids, binding_names); } else { - let symbol_ids = self.get_pair(scope_ids, |scoping, scope_id| { + let mut symbol_ids = self.get_pair(scope_ids, |scoping, scope_id| { scoping.scopes.get_bindings(scope_id).values().copied().collect::>() }); if self.remap_symbol_ids_sets(&symbol_ids).is_mismatch() { + symbol_ids.after_transform.sort_unstable(); + symbol_ids.rebuilt.sort_unstable(); self.errors.push_mismatch("Binding symbols mismatch", scope_ids, symbol_ids); } }