Fix underscore-pileup collisions with scope-aware resolution#754
Open
megaboich wants to merge 1 commit into
Open
Fix underscore-pileup collisions with scope-aware resolution#754megaboich wants to merge 1 commit into
megaboich wants to merge 1 commit into
Conversation
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<String>` 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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What & why
When un-minifying with a small local model, the renamer produced identifiers with huge runs of leading underscores — up to ~320 in a real run:
The bug
The old collision loop deduped final names globally across the whole file and disambiguated by prepending
_. A small local model tends to return the same generic word for hundreds of unrelated variables (indexwas chosen ~1500× in one run, plusnode,link,iterator, …). Every repeat collided with the previous one, so the loop kept prepending_until it found a free slot.This is over-conservative. Two variables named
indexin disjoint / sibling scopes are perfectly legal JavaScript and never conflict — only names visible in the same scope chain actually need disambiguation.The fix
Replace the file-global
taken: HashSet<String>with aCollisionResolverthat checks a candidate only against names visible in the binding's scope chain:Scoping::find_binding(walks up the scope tree): catches same-scope redeclaration and shadowing that would capture an outer binding.name → enter-indicesindex (O(log n) range query): prevents renaming an outer symbol to a name a nested scope already binds, which would otherwise capture the outer symbol's references made inside that nested scope.Genuine conflicts now 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→index3foo2→foo3(neverfoo22)Behavior change
index2instead of_index. Non-colliding names are unaffected.rename_all_identifiersstill returnsResult<String, RenameError>.Scoping::rename_symbolwith the finalCodegen::with_scopingmaterialization; no AST mutation of bindings.Tests
New coverage (unit tests in
collision.rs, integration tests inwalker.rs):________index);a→xwhere a nested scope bindsxbecomesx2, preserving the reference);item2→item3→item4, neveritem22).All existing tests updated where they asserted the old
_-prefix behavior.cargo test,cargo clippy --all-targets, andcargo fmt --checkare green.