fix(extract): disambiguate unresolved base-class stubs by file#1707
Open
mallyskies wants to merge 1 commit into
Open
fix(extract): disambiguate unresolved base-class stubs by file#1707mallyskies wants to merge 1 commit into
mallyskies wants to merge 1 commit into
Conversation
ensure_named_node() tags the sourceless stub it creates for an unresolved reference with origin_file, so _disambiguate_colliding_node_ids can tell one file's unresolved reference apart from another's instead of merging every file's same-named reference onto one shared bare id (which can then collide with an unrelated same-named real definition anywhere else in the corpus, since ids are case-normalized and global). Five call sites duplicated that stub-creation logic inline instead of calling ensure_named_node() -- Ruby's `Class.new(Super)` and `class Foo < Base` inheritance, Python inheritance, Kotlin delegation -specifier inheritance/conformance, and C++ base_class_clause inheritance -- and none of them were updated when origin_file was added, so all five still produce the un-disambiguated bare-id stub the fix was meant to eliminate. Four of the five sit directly inside _extract_generic, where ensure_named_node() is already in scope as a closure, so they're switched to call it directly. The fifth (Ruby's `Class.new(Super)`) is handled by a separate helper, _ruby_extra_walk(), which doesn't have that closure in scope; that one gets the same origin_file tag added directly to its own inline stub dict, matching what ensure_named_node() already does, without changing the helper's signature. (A sixth occurrence of the same inline pattern exists in extract_apex(), a fully separate regex-based extractor with no ensure_named_node() equivalent of its own -- left out of this fix, which is scoped to the shared _extract_generic path and its one directly-affiliated helper.) Added a regression test: two different C++ files each inheriting from the same undefined base class must produce two distinct stub nodes, not one shared one. Fails on main (one shared 'base' id for both files), passes with this fix.
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
1 participant
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's wrong
When a class extends a base class that isn't defined in the same file, graphify creates a placeholder "stub" node for that unknown base class.
ensure_named_node()— the helper that creates these stubs — tags each one withorigin_file, so a later pass (_disambiguate_colliding_node_ids) can tell "file A's reference toBase" apart from "file B's reference toBase".Five places in the extractor build this same kind of stub inline, bypassing
ensure_named_node()entirely — and none of them setorigin_file. Without that tag, every file's reference to the same-named base class gets merged into one shared stub instead of staying separate. That shared stub can then collide with an unrelated real class somewhere else in the codebase that happens to have the same name.Affected inheritance handlers:
base_class_clauseclass Foo < BaseClass.new(Super)The fix
Four of the five sit directly inside
_extract_generic, whereensure_named_node()is already available — those now just call it instead of duplicating its logic.The fifth (Ruby's
Class.new(Super)) lives in a separate helper,_ruby_extra_walk(), that doesn't haveensure_named_node()in scope. Rather than change that helper's signature, it gets the sameorigin_filetag added directly to its own stub dict.A sixth, similar occurrence exists in
extract_apex(), a fully separate regex-based extractor with noensure_named_node()of its own. Left out here to keep this PR scoped to the shared_extract_genericpath and its one directly-related helper.How we found this
We hit a concrete case: ~163 different C++ files each had a class extending
mTimer, a real, shared C++ base class. Because none of those references were disambiguated by file, they all collapsed onto one stub — and that stub's label happened to exact-case-match an unrelated class also namedMTimerin a different language entirely. Result: 218 wronginheritsedges from unrelated C++ server code into that unrelated class.Testing
Added a regression test: two different C++ files, each inheriting from the same undefined base class, must produce two distinct stub nodes — not one shared one. It fails on
v8(both files share one'base'id) and passes with this fix.Full
tests/test_extract.pysuite: 94 passing (was 93; +1 new test), same 19 pre-existing failures before and after this change (unrelated — missing optional tree-sitter grammar packages in this environment).