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 680ae0ab7fcf3..a6ccd065c2629 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 4d45d4f4ff72d..60a5a7b551ba0 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 1eac2ef473696..92b209f73c275 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 1b915140ac4f6..0afe38e353f97 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 93aeac2148f07..049addc0ab971 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 aada47ae49786..b1d5d066741fc 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 18f3dee01a107..4de4a3e9b3bf9 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 fe6cf2c7015eb..7516acaece7a8 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 1d957f2ef838d..cae83c7f27b0f 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); } }