From 15600ac07a43e3d4d66ba7e3add2b1f87faa6d22 Mon Sep 17 00:00:00 2001 From: megaboich Date: Sat, 18 Jul 2026 12:50:56 +0200 Subject: [PATCH] Fix underscore-pileup collisions with scope-aware resolution The collision loop deduped final names globally across the whole file and disambiguated by prepending `_`. Because a local model returns the same generic word for hundreds of unrelated variables, each repeat collided with the previous one and the loop kept prepending `_`, producing names with up to ~320 leading underscores (e.g. `___...___index`). This is over-conservative: two variables named `index` in disjoint / sibling scopes are perfectly legal JS and never conflict. Only names visible in the same scope chain need disambiguation. Replace the file-global `taken: HashSet` with a `CollisionResolver` that checks a candidate only against names actually visible in the binding's scope chain: - same scope / ancestors, via `Scoping::find_binding` (walks up), and - descendants, via an Euler-tour interval over the scope tree plus a live `name -> enter-indices` index (O(log n) range query), so an outer symbol is never renamed to a name a nested scope would capture. Genuine conflicts get a numeric suffix instead of an underscore prefix. If the candidate already ends in a number, that number is incremented rather than a second one appended: `index` -> `index2` -> `index3`, and `foo2` -> `foo3` (never `foo22`). Tests: identical names in sibling scopes stay bare; same-scope and shadowing conflicts get exactly one suffix; no pileup across many disjoint scopes; descendant capture is prevented; and suffix increment-vs-append rules. --- src/rename/collision.rs | 251 ++++++++++++++++++++++++++++++++++++++++ src/rename/mod.rs | 1 + src/rename/test_dsl.rs | 22 ++++ src/rename/walker.rs | 183 +++++++++++++++++++++++++---- 4 files changed, 433 insertions(+), 24 deletions(-) create mode 100644 src/rename/collision.rs diff --git a/src/rename/collision.rs b/src/rename/collision.rs new file mode 100644 index 00000000..97a30025 --- /dev/null +++ b/src/rename/collision.rs @@ -0,0 +1,251 @@ +use std::collections::{BTreeSet, HashMap}; + +use oxc_semantic::{ScopeId, Scoping, SymbolId}; +use oxc_str::Ident; + +/// Split a trailing run of ASCII decimal digits off an identifier. +/// +/// `"index3"` -> `("index", Some(3))`, `"iterator"` -> `("iterator", None)`. +/// An all-digit string (impossible for a valid identifier) yields `None`. +fn split_numeric_suffix(name: &str) -> (&str, Option) { + let bytes = name.as_bytes(); + let mut i = bytes.len(); + while i > 0 && bytes[i - 1].is_ascii_digit() { + i -= 1; + } + if i == 0 || i == bytes.len() { + (name, None) + } else { + (&name[..i], name[i..].parse::().ok()) + } +} + +/// Resolve `base` to a free name given an `is_taken` predicate, appending or +/// **incrementing** a numeric suffix. +/// +/// If `base` already ends in a number, that number is incremented rather than a +/// new one appended — so `userAccountManager2` becomes `userAccountManager3` +/// (not `userAccountManager22`), and `index3` becomes `index4`. Names without +/// a trailing number start at `2` (`index` -> `index2` -> `index3`). +/// +/// Returns the resolved name and whether any suffixing was needed. +pub(crate) fn dedupe bool>(base: &str, is_taken: F) -> (String, bool) { + if !is_taken(base) { + return (base.to_string(), false); + } + let (stem, start) = split_numeric_suffix(base); + let mut n = start.map(|x| x.saturating_add(1)).unwrap_or(2); + loop { + let candidate = format!("{stem}{n}"); + if !is_taken(&candidate) { + return (candidate, true); + } + n += 1; + } +} + +/// Scope-aware collision resolver. +/// +/// Replaces the old file-global `taken` set (which forced whole-file name +/// uniqueness and produced the `________index` underscore pileup). A candidate +/// name only collides when it is actually visible in the same scope chain: +/// +/// 1. **Same scope or an ancestor** — detected via `Scoping::find_binding`, +/// which walks up the scope tree. This prevents same-scope redeclaration +/// and prevents shadowing an outer binding the new name would capture. +/// 2. **A descendant scope** — detected via an Euler-tour interval over the +/// scope tree. Renaming an outer symbol to a name already bound in a nested +/// scope would capture the outer symbol's references that occur inside that +/// nested scope (e.g. renaming outer `a` to `x` in +/// `var a; function f() { var x; return a; }` would silently rebind the +/// `return a` to the inner `x`), so it must count as a collision. The +/// descendant index is seeded from every existing binding and kept live by +/// `commit`, so this holds no matter what order symbols are processed in. +/// +/// Two identifiers in *disjoint* scopes (e.g. sibling functions) may freely +/// share a name — exactly as JavaScript allows — so no underscores accrue. +pub(crate) struct CollisionResolver { + /// Euler-tour enter index per scope (order of first visit in a DFS). + enter: HashMap, + /// Euler-tour exit index per scope. Scope `d` is a descendant of scope `a` + /// iff `enter[a] < enter[d] < exit[a]`. + exit: HashMap, + /// For each currently-bound name, the sorted set of enter-indices of the + /// scopes that bind it. Enables an O(log n) "is this name bound in any + /// descendant of scope X?" range query. + name_scopes: HashMap>, +} + +impl CollisionResolver { + /// Build the scope tree Euler tour and seed the name index from every + /// existing binding (including names that will never be renamed, so that + /// descendant kept-names are still respected). + pub(crate) fn new(scoping: &Scoping) -> Self { + // Build children adjacency from parent links. + let mut children: HashMap> = HashMap::new(); + for scope_id in scoping.scope_descendants_from_root() { + if let Some(parent) = scoping.scope_parent_id(scope_id) { + children.entry(parent).or_default().push(scope_id); + } + } + + // Iterative DFS from the root, assigning enter/exit indices so that a + // scope's descendants occupy a contiguous open interval of enter-indices. + let mut enter: HashMap = HashMap::new(); + let mut exit: HashMap = HashMap::new(); + let mut counter: u32 = 0; + let root = scoping.root_scope_id(); + // Stack holds (scope, entered?). On first pop we record enter and push + // the scope back as "entered" plus its children; on second pop we record + // exit. + let mut stack: Vec<(ScopeId, bool)> = vec![(root, false)]; + while let Some((scope_id, entered)) = stack.pop() { + if entered { + exit.insert(scope_id, counter); + continue; + } + enter.insert(scope_id, counter); + counter += 1; + stack.push((scope_id, true)); + if let Some(kids) = children.get(&scope_id) { + for &kid in kids { + stack.push((kid, false)); + } + } + } + + // Seed the name index from all current bindings. + let mut name_scopes: HashMap> = HashMap::new(); + for (scope_id, bindings) in scoping.iter_bindings() { + let Some(&idx) = enter.get(&scope_id) else { + continue; + }; + for name in bindings.keys() { + name_scopes.entry(name.to_string()).or_default().insert(idx); + } + } + + CollisionResolver { + enter, + exit, + name_scopes, + } + } + + /// Returns true if `candidate` cannot be assigned to `symbol` (declared in + /// `decl_scope`) without clashing with a binding visible in the same scope + /// chain (ancestor, same scope, or descendant). + pub(crate) fn collides( + &self, + scoping: &Scoping, + decl_scope: ScopeId, + symbol: SymbolId, + candidate: &str, + ) -> bool { + // (1) Same scope or ancestor: find_binding walks up from decl_scope. + // A hit on the symbol itself is not a real collision (candidate == its + // own current name, i.e. a no-op rename). + if let Some(found) = scoping.find_binding(decl_scope, Ident::from(candidate)) { + if found != symbol { + return true; + } + } + + // (2) Descendant scopes: is `candidate` bound anywhere strictly inside + // decl_scope's subtree? Query the open enter-index interval. + if let (Some(&lo), Some(&hi)) = (self.enter.get(&decl_scope), self.exit.get(&decl_scope)) { + if let Some(indices) = self.name_scopes.get(candidate) { + // Descendants have enter-index in (lo, hi). + if indices.range((lo + 1)..hi).next().is_some() { + return true; + } + } + } + + false + } + + /// Record that `decl_scope`'s binding changed from `old_name` to `new_name`, + /// keeping the descendant index in sync for subsequent `collides` queries. + pub(crate) fn commit(&mut self, decl_scope: ScopeId, old_name: &str, new_name: &str) { + let Some(&idx) = self.enter.get(&decl_scope) else { + return; + }; + if let Some(set) = self.name_scopes.get_mut(old_name) { + set.remove(&idx); + if set.is_empty() { + self.name_scopes.remove(old_name); + } + } + self.name_scopes + .entry(new_name.to_string()) + .or_default() + .insert(idx); + } +} + +#[cfg(test)] +mod tests { + use std::collections::HashSet; + + use super::{dedupe, split_numeric_suffix}; + + #[test] + fn split_no_trailing_digits() { + assert_eq!(split_numeric_suffix("iterator"), ("iterator", None)); + } + + #[test] + fn split_trailing_digits() { + assert_eq!(split_numeric_suffix("index3"), ("index", Some(3))); + assert_eq!( + split_numeric_suffix("userAccountManager2"), + ("userAccountManager", Some(2)) + ); + } + + #[test] + fn split_all_digits_is_treated_as_no_suffix() { + assert_eq!(split_numeric_suffix("123"), ("123", None)); + } + + #[test] + fn dedupe_free_name_unchanged() { + let taken: HashSet<&str> = HashSet::new(); + assert_eq!( + dedupe("index", |c| taken.contains(c)), + ("index".to_string(), false) + ); + } + + #[test] + fn dedupe_appends_when_no_trailing_number() { + let taken: HashSet = ["index".to_string()].into_iter().collect(); + assert_eq!( + dedupe("index", |c| taken.contains(c)), + ("index2".to_string(), true) + ); + } + + #[test] + fn dedupe_increments_existing_trailing_number() { + // The bug the user reported: `...Manager2` colliding must become + // `...Manager3`, not `...Manager22`. + let taken: HashSet = ["userAccountManager2".to_string()].into_iter().collect(); + assert_eq!( + dedupe("userAccountManager2", |c| taken.contains(c)), + ("userAccountManager3".to_string(), true) + ); + } + + #[test] + fn dedupe_increments_until_free() { + let taken: HashSet = ["index3".to_string(), "index4".to_string()] + .into_iter() + .collect(); + assert_eq!( + dedupe("index3", |c| taken.contains(c)), + ("index5".to_string(), true) + ); + } +} diff --git a/src/rename/mod.rs b/src/rename/mod.rs index 43da7776..d64dca4a 100644 --- a/src/rename/mod.rs +++ b/src/rename/mod.rs @@ -1,3 +1,4 @@ +mod collision; mod safe_name; #[cfg(test)] pub mod test_dsl; diff --git a/src/rename/test_dsl.rs b/src/rename/test_dsl.rs index ac92ca88..f5a267e3 100644 --- a/src/rename/test_dsl.rs +++ b/src/rename/test_dsl.rs @@ -32,6 +32,19 @@ impl Renamer for IdentityRenamer { } } +/// Renames based on the original name via a lookup table; unmapped names are +/// left unchanged. Order-independent, unlike `queue`, which makes it convenient +/// for testing scope-aware collision behaviour regardless of traversal order. +pub struct MapRenamer(std::collections::HashMap); +impl Renamer for MapRenamer { + fn rename(&mut self, original: &str, _: &str) -> String { + self.0 + .get(original) + .cloned() + .unwrap_or_else(|| original.to_string()) + } +} + pub struct RecordingRenamer { suffix: String, pub log: CallLog, @@ -87,6 +100,15 @@ pub fn recording(sfx: &str) -> RecordingRenamer { } } +pub fn mapping(pairs: &[(&str, &str)]) -> MapRenamer { + MapRenamer( + pairs + .iter() + .map(|(a, b)| (a.to_string(), b.to_string())) + .collect(), + ) +} + // --- ScenarioBuilder --- pub struct ScenarioBuilder { diff --git a/src/rename/walker.rs b/src/rename/walker.rs index 7ab3cf92..6feeacc9 100644 --- a/src/rename/walker.rs +++ b/src/rename/walker.rs @@ -7,6 +7,7 @@ use oxc_semantic::{AstNodes, NodeId, Scoping, SemanticBuilder, SymbolId}; use oxc_span::{GetSpan, SourceType}; use oxc_str::Ident; +use super::collision::CollisionResolver; use super::{RenameError, Renamer}; pub fn rename_all_identifiers( @@ -73,7 +74,7 @@ pub fn rename_all_identifiers( entries.sort_by(|a, b| b.1.cmp(&a.1).then(a.2.cmp(&b.2))); let mut visited: HashSet = HashSet::new(); - let mut taken: HashSet = HashSet::new(); + let mut collisions = CollisionResolver::new(semantic.scoping()); for (sym_id, _, _) in &entries { let sym_id = *sym_id; @@ -115,27 +116,23 @@ pub fn rename_all_identifiers( continue; } - // Apply safe-name normalization. - let mut safe = super::safe_name::to_identifier(&new_name); + // Apply safe-name normalization to get the base candidate. + let base = super::safe_name::to_identifier(&new_name); - // Collision loop: prefix with '_' until name is free. + // Resolve collisions in this scope chain with a numeric suffix instead + // of the old underscore-prefix loop (which piled up `________index` + // when a local model returned the same generic word many times). If the + // model's name already ends in a number, that number is incremented + // rather than a second one appended — so `...Iterator2` -> `...Iterator3`, + // never `...Iterator22`. let scope_id = semantic.scoping().symbol_scope_id(sym_id); - loop { - let already_taken = taken.contains(&safe); - let pre_existing = { - let scoping = semantic.scoping(); - let ident = Ident::from(safe.as_str()); - scoping.find_binding(scope_id, ident).is_some() - }; - if !already_taken && !pre_existing { - break; - } - safe = format!("_{safe}"); - } - - taken.insert(safe.clone()); + let (safe, _collided) = super::collision::dedupe(&base, |cand| { + collisions.collides(semantic.scoping(), scope_id, sym_id, cand) + }); - // Rename in the symbol table (codegen reads from here via with_scoping). + // Rename in the symbol table (codegen reads from here via with_scoping), + // and keep the collision index in sync for later symbols. + collisions.commit(scope_id, &original_name, &safe); let new_ident = Ident::from(allocator.alloc_str(&safe)); semantic .scoping_mut() @@ -247,7 +244,7 @@ fn ceil_char_boundary(source: &str, index: usize) -> usize { mod tests { use std::collections::HashSet; - use super::super::test_dsl::{fixed, identity, queue, recording, scenario, suffix}; + use super::super::test_dsl::{fixed, identity, mapping, queue, recording, scenario, suffix}; use super::*; #[test] @@ -561,8 +558,8 @@ mod tests { out.output() ); assert!( - out.output().contains("const _foo = 1"), - "expected _foo: {}", + out.output().contains("const foo2 = 1"), + "expected numeric-suffixed foo2: {}", out.output() ); } @@ -573,8 +570,8 @@ mod tests { .with_context_size(500) .renamed_with(fixed("bar")); assert!( - out.output().contains("_bar"), - "expected _bar for renamed foo: {}", + out.output().contains("bar2"), + "expected bar2 for renamed foo: {}", out.output() ); assert!( @@ -584,6 +581,144 @@ mod tests { ); } + // --- scope-aware collision handling --- + + /// Counts occurrences of `name` as a whole identifier token (bounded by + /// non-identifier characters), so `count_ident(s, "shared")` does not match + /// inside `_shared`, and `count_ident(s, "_shared")` does not match inside + /// `__shared`. + fn count_ident(hay: &str, name: &str) -> usize { + let is_word = |c: char| c.is_ascii_alphanumeric() || c == '_' || c == '$'; + let bytes = hay.as_bytes(); + let mut n = 0; + let mut start = 0; + while let Some(pos) = hay[start..].find(name) { + let i = start + pos; + let before_ok = i == 0 || !is_word(hay[..i].chars().next_back().unwrap()); + let after = i + name.len(); + let after_ok = after >= bytes.len() || !is_word(hay[after..].chars().next().unwrap()); + if before_ok && after_ok { + n += 1; + } + start = i + name.len(); + } + n + } + + #[test] + fn sibling_scopes_may_reuse_the_same_name_without_suffix() { + // Two locals the model names identically, in disjoint sibling functions. + // JS allows this, so neither should accrue a suffix. + let out = scenario("function f() { var a = 1; } function g() { var b = 2; }") + .with_context_size(500) + .renamed_with(mapping(&[("a", "shared"), ("b", "shared")])); + let o = out.output(); + assert_eq!( + count_ident(o, "shared"), + 2, + "both sibling locals should be bare `shared`: {o}" + ); + assert!( + !o.contains("shared2"), + "sibling reuse must not add a numeric suffix: {o}" + ); + } + + #[test] + fn same_scope_conflict_gets_numeric_suffix() { + // Two bindings in the SAME scope cannot share a name; the second becomes + // `shared2` rather than `_shared`. + let out = scenario("const a = 1; const b = 2;") + .with_context_size(500) + .renamed_with(mapping(&[("a", "shared"), ("b", "shared")])); + let o = out.output(); + assert_eq!(count_ident(o, "shared"), 1, "one bare `shared`: {o}"); + assert_eq!(count_ident(o, "shared2"), 1, "one `shared2`: {o}"); + assert!(!o.contains("shared3"), "only two colliding names: {o}"); + assert!(!o.contains("_shared"), "must not underscore-prefix: {o}"); + } + + #[test] + fn shadowing_conflict_across_nesting_gets_numeric_suffix() { + // An outer binding and a nested binding cannot both be `shared`, or a + // reference would be captured. Exactly one becomes `shared2`, regardless + // of traversal order. + let out = scenario("var a = 1; function f() { var b = 2; return a; }") + .with_context_size(500) + .renamed_with(mapping(&[("a", "shared"), ("b", "shared")])); + let o = out.output(); + assert!(o.contains("shared2"), "one conflict becomes `shared2`: {o}"); + assert!(!o.contains("shared3"), "only two colliding names: {o}"); + assert!(!o.contains("_shared"), "must not underscore-prefix: {o}"); + } + + #[test] + fn no_suffix_pileup_across_many_disjoint_scopes() { + // Regression for the `________index` pileup: many disjoint scopes each + // naming their local `index` must all stay bare `index`. + let mut src = String::new(); + for i in 0..10 { + src.push_str(&format!("function f{i}() {{ var v{i} = {i}; }} ")); + } + let pairs: Vec<(String, String)> = (0..10) + .map(|i| (format!("v{i}"), "index".to_string())) + .collect(); + let pair_refs: Vec<(&str, &str)> = pairs + .iter() + .map(|(a, b)| (a.as_str(), b.as_str())) + .collect(); + let out = scenario(&src) + .with_context_size(500) + .renamed_with(mapping(&pair_refs)); + let o = out.output(); + assert_eq!( + count_ident(o, "index"), + 10, + "all ten locals should be bare: {o}" + ); + assert!(!o.contains("index2"), "no numeric-suffix pileup: {o}"); + } + + #[test] + fn descendant_capture_is_prevented_with_numeric_suffix() { + // Renaming the OUTER `a` to a name already bound in a nested scope would + // capture the `return a` reference (it would resolve to the inner `x`). + // The descendant check must force a suffix even though `find_binding` + // (which only walks *up*) can't see the nested binding. Outer becomes + // `x2`, and the inner `x` and the outer reference stay correct. + let out = scenario("var a = 1; function f() { var x = 2; return a + x; }") + .with_context_size(500) + .renamed_with(mapping(&[("a", "x")])); + let o = out.output(); + assert_eq!( + count_ident(o, "x2"), + 2, + "outer decl + its reference should both be `x2`: {o}" + ); + assert_eq!( + count_ident(o, "x"), + 2, + "inner decl + its reference should stay bare `x`: {o}" + ); + assert!(!o.contains("x3"), "only one conflict: {o}"); + } + + #[test] + fn same_scope_numeric_names_increment_not_append() { + // Three same-scope bindings the model all names `item2`: the suffix must + // increment (`item2` -> `item3` -> `item4`), never append a second digit + // (`item22`) or fall back to an underscore. + let out = scenario("const a = 1; const b = 2; const c = 3;") + .with_context_size(500) + .renamed_with(mapping(&[("a", "item2"), ("b", "item2"), ("c", "item2")])); + let o = out.output(); + assert_eq!(count_ident(o, "item2"), 1, "one `item2`: {o}"); + assert_eq!(count_ident(o, "item3"), 1, "one `item3`: {o}"); + assert_eq!(count_ident(o, "item4"), 1, "one `item4`: {o}"); + assert!(!o.contains("item22"), "must increment, not append: {o}"); + assert!(!o.contains("_item"), "must not underscore-prefix: {o}"); + } + #[test] fn does_not_crash_on_arguments_assign() { let out = scenario("function foo() { arguments = '??'; }")