Skip to content

Fix Python leading-underscore method/function id collision - #3317

Open
ayushcodes10 wants to merge 1 commit into
Graphify-Labs:v8from
ayushcodes10:fix-3302-underscore-collision
Open

Fix Python leading-underscore method/function id collision#3317
ayushcodes10 wants to merge 1 commit into
Graphify-Labs:v8from
ayushcodes10:fix-3302-underscore-collision

Conversation

@ayushcodes10

@ayushcodes10 ayushcodes10 commented Sep 3, 2026

Copy link
Copy Markdown

ids.py:make_id strips leading/trailing underscores from every name
part before normalizing, so _get_connection/get_connection (and more
broadly any name pair differing only by underscore wrapping --
x/_x/__x/x dunder variants all collapse the same way) mint the
SAME node id. add_node's first-wins dedup then silently drops
whichever declaration is walked second: a public method could be
entirely absent from the graph while its private-by-convention
sibling occupied the public name, with no warning and exit 0.

Adds a Python-scoped pre-scan (_python_pre_scan_underscore_collisions)
that groups module-level functions and direct class methods by their
would-be node id, plus a resolver (_python_underscore_salted_nid) that
salts every group member except a unique public (no leading
underscore) one -- mirroring the established convention the Go
extractor already uses for its own case-only collision (#2779/#2780):
when no unique public member exists, every member is salted, so the
outcome never depends on declaration order. The public member's id
stays exactly what it would be with no colliding sibling at all, so
adding/removing a private-by-convention sibling in an incremental
rebuild re-points nothing.

Both hooks are gated on config.ts_module == "tree_sitter_python" and
live inside the shared _extract_generic engine (Python has no
dedicated extractor file, unlike Go/Rust/SQL), following the existing
precedent for per-language pre-scans already there (csharp_interface_names,
swift_protocol_names). Call-site resolution needed no changes: label_to_nid
is rebuilt from the final nodes list after the walk completes, so it
naturally picks up whichever id (salted or not) a method actually got.

9 new tests in tests/test_python_underscore_resolution.py: the exact
repro, id stability across incremental add/remove of a private
sibling, the call edge correctly binding to the salted target, the
module-level (non-method) case, the no-unique-public-member case
(both members salted), a dunder+plain collision, an unaffected
no-collision file, and scope isolation (a collision in one class must
not touch an unrelated same-named-but-non-colliding member in another
class).

Full suite: 5117 passed, 0 failed. ruff and skillgen --check clean.

Fixes #3302.

ids.py:make_id strips leading/trailing underscores from every name
part before normalizing, so _get_connection/get_connection (and more
broadly any name pair differing only by underscore wrapping --
x/_x/__x/__x__ dunder variants all collapse the same way) mint the
SAME node id. add_node's first-wins dedup then silently drops
whichever declaration is walked second: a public method could be
entirely absent from the graph while its private-by-convention
sibling occupied the public name, with no warning and exit 0.

Adds a Python-scoped pre-scan (_python_pre_scan_underscore_collisions)
that groups module-level functions and direct class methods by their
would-be node id, plus a resolver (_python_underscore_salted_nid) that
salts every group member except a unique public (no leading
underscore) one -- mirroring the established convention the Go
extractor already uses for its own case-only collision (Graphify-Labs#2779/Graphify-Labs#2780):
when no unique public member exists, every member is salted, so the
outcome never depends on declaration order. The public member's id
stays exactly what it would be with no colliding sibling at all, so
adding/removing a private-by-convention sibling in an incremental
rebuild re-points nothing.

Both hooks are gated on config.ts_module == "tree_sitter_python" and
live inside the shared _extract_generic engine (Python has no
dedicated extractor file, unlike Go/Rust/SQL), following the existing
precedent for per-language pre-scans already there (csharp_interface_names,
swift_protocol_names). Call-site resolution needed no changes: label_to_nid
is rebuilt from the final nodes list after the walk completes, so it
naturally picks up whichever id (salted or not) a method actually got.

9 new tests in tests/test_python_underscore_resolution.py: the exact
repro, id stability across incremental add/remove of a private
sibling, the call edge correctly binding to the salted target, the
module-level (non-method) case, the no-unique-public-member case
(both members salted), a dunder+plain collision, an unaffected
no-collision file, and scope isolation (a collision in one class must
not touch an unrelated same-named-but-non-colliding member in another
class).

Note: tests/ on the current v8 HEAD has 7 pre-existing failures
unrelated to this change (test_cli_export.py, test_cross_repo_shared_types.py,
test_extract_code_only_cli.py, test_merge_graphs_cli.py) -- confirmed via
`git stash` that they fail identically on pristine v8 without this diff.
Full suite otherwise: 5116 passed. ruff and skillgen --check clean.

Fixes Graphify-Labs#3302.

@graphify-labs graphify-labs Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Graphify reviewed this change.

Worth a look — the grounded gate found no coupling regressions or blocking issues, but 4 advisory finding(s) below merit a look before merge.

Formal verification. No changes could be formally verified in this run.


Graphify review — findings

Adds a Python pre-scan that detects declarations whose node ids collapse to one after make_id strips leading/trailing underscores (e.g. _get_connection/get_connection, or __x/x), so both survive as distinct nodes instead of the second one being silently dropped. When a collision group has exactly one non-underscore-prefixed name, that member keeps the plain id (so incremental rebuilds adding/removing a private sibling don't re-point references) and every other member is salted with a name hash, making the outcome order-independent. The scan covers only module-level functions and direct methods of module-level classes — collisions buried in nested scopes are missed rather than mis-resolved.

Worth a look

  • Pre-scan uses raw name while resolution/emission use sanitized_name, so collision map is keyed on wrong stringsgraphify/extractors/engine.py:855 · Escalate · high
    • agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
  • Decorated Python functions are omitted from underscore collision pre-scangraphify/extractors/engine.py:842 · Escalate · medium
    • agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
  • Salt derived from sanitized_name loses the distinguishing information underscores carrygraphify/extractors/engine.py:883 · Escalate · medium
    • agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
  • Pre-scan keys on raw name while resolution keys on sanitized name, causing mismatched collision lookupgraphify/extractors/engine.py:4313 · Escalate · medium
    • agreed by 2 of 2 members but NOT verified (no proof, no reproducing execution) — consensus is not a verdict; needs human review
Analysis details — impact, health, verification

Impact & health

Graphify review

Impact — 655 functions depend on the 229 functions this change touches.

Health — this change adds coupling hotspots:

  • new: _extract_generic() — 18 callers, 26 callees
  • new: extract_js() — 85 callers, 4 callees
  • new: extract_xaml() — 19 callers, 17 callees
  • new: extract_objc() — 27 callers, 9 callees
  • new: extract_julia() — 17 callers, 7 callees
  • new: extract_cpp() — 27 callers, 3 callees
  • new: extract_vue() — 10 callers, 7 callees
  • new: walk() — 1 callers, 58 callees
  • …and 8 more — each is listed as a finding

Verification — 655 functions in the blast radius were not formally verified this run (proofs are advisory here).

Gate & verification

graphify gate

PASS — objectively clean (no health regressions, tests not run — proofs not run this pass (advisory)). Grounded, not self-assessed.

Advisory (not blocking):

  • verification_scope: 595 function(s) in the blast radius were not formally verified this run

Formal verification

Could not verify: Could not verify \_extract\_generic.

The verifier did not have enough to check \_extract\_generic, so it is saying so rather than guessing. No false assurance is the whole point.

Guarantee: No guarantee either way, this is an honest abstention, not a pass.

Note: Reason: parameter `path` is annotated `Path` — outside the synthesizable primitive/collection set

· 16 more finding(s) on lines outside this diff (see the check run).

@ayushcodes10

Copy link
Copy Markdown
Author

Correction: same as the note I just posted on #3315 — the "7 pre-existing failures" I reported were caused by a stale pip-installed graphifyy 0.9.38 on my machine shadowing this checkout for subprocess-based tests, not a real issue with this codebase or this change. Reinstalled editable and the full suite is clean: 5117 passed, 0 failed. Updated the PR description to match.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Python: methods differing only by a leading underscore collide on node id — the public one is silently dropped from the graph

1 participant