diff --git a/graphify/extractors/engine.py b/graphify/extractors/engine.py index 38e9a5420..e74b6e258 100644 --- a/graphify/extractors/engine.py +++ b/graphify/extractors/engine.py @@ -813,6 +813,84 @@ def _swift_declaration_keyword(node) -> str | None: return c.type return None +def _python_pre_scan_underscore_collisions(root_node, source: bytes, stem: str) -> dict[str, set[str]]: + """Pre-scan a Python module for name-only differences that collapse to one node id. + + ``ids.py:make_id`` strips leading/trailing underscores from every part before + normalizing, so ``_get_connection`` and ``get_connection`` mint the SAME id. + ``add_node`` then silently drops whichever declaration is walked second — no + warning, exit 0 — so a public method can be entirely absent from the graph + while its private-by-convention sibling (or a dunder, which strips the same + way: ``__x``/``__x__``/``x`` all collapse too) occupies the public name (#3302). + + Returns ``{plain_nid: {raw names that would collide on it}}`` for every + module-level function and every direct method of a module-level class — the + two cases the id-minting code actually distinguishes via ``parent_class_nid``. + Deliberately does not recurse into nested classes or nested functions: this + keeps the scope key trivially exact (matching ``_make_id(stem, class_name)``, + which only holds because Python never populates ``namespace_stack``) rather + than replicating the corpus-wide id-computation rules for every nesting shape. + A collision entirely inside an unhandled nested scope is simply not caught + here — a strict miss, never a false positive, since the map is only ever + consulted for a nid this same scan actually computed. + """ + groups: dict[str, set[str]] = {} + + def _record(plain_nid: str, name: str) -> None: + groups.setdefault(plain_nid, set()).add(name) + + for child in root_node.children: + if child.type == "function_definition": + name_node = child.child_by_field_name("name") + if name_node is not None: + name = _read_text(name_node, source) + if name: + _record(_make_id(stem, name), name) + elif child.type == "class_definition": + class_name_node = child.child_by_field_name("name") + body = child.child_by_field_name("body") + if class_name_node is None or body is None: + continue + class_name = _read_text(class_name_node, source) + if not class_name: + continue + class_nid = _make_id(stem, class_name) + for member in body.children: + if member.type != "function_definition": + continue + name_node = member.child_by_field_name("name") + if name_node is None: + continue + name = _read_text(name_node, source) + if name: + _record(_make_id(class_nid, name), name) + + return {nid: names for nid, names in groups.items() if len(names) >= 2} + + +def _python_underscore_salted_nid(plain_nid: str, name: str, groups: dict[str, set[str]]) -> str: + """Resolve a Python function/method's real node id against the collision map. + + A name with no leading underscore at all is "public" per PEP 8 convention. + When a collision group has exactly one public member, that member keeps the + plain id — cross-file/documentation references overwhelmingly target the + public name, and keeping it stable means an incremental rebuild that adds or + removes a private-by-convention sibling re-points nothing. Every other + member of the group (including the public one when it is NOT unique, e.g. + `_x`/`__x` colliding with no public member at all) is salted, so the outcome + never depends on declaration order — mirrors the exported/unexported rule + the Go extractor uses for its own case-only collision (#2779). + """ + names = groups.get(plain_nid) + if not names or len(names) < 2: + return plain_nid + public = [n for n in names if not n.startswith("_")] + if len(public) == 1 and name == public[0]: + return plain_nid + salt = hashlib.sha1(name.encode("utf-8"), usedforsecurity=False).hexdigest()[:6] + return _make_id(plain_nid, salt) + + def _swift_pre_scan(root_node, source: bytes) -> tuple[set[str], set[str]]: """Pre-scan a Swift compilation unit and return (protocol_names, class_like_names).""" protocols: set[str] = set() @@ -3001,6 +3079,10 @@ def _extract_generic( if config.ts_module == "tree_sitter_swift": swift_protocol_names, swift_class_names = _swift_pre_scan(root, source) + python_underscore_groups: dict[str, set[str]] = {} + if config.ts_module == "tree_sitter_python": + python_underscore_groups = _python_pre_scan_underscore_collisions(root, source, stem) + def add_node(nid: str, label: str, line: int, *, node_type: str | None = None, metadata: dict | None = None) -> None: if nid in seen_ids: @@ -4229,10 +4311,18 @@ def scala_base_name(type_node) -> str | None: line = node.start_point[0] + 1 if parent_class_nid: func_nid = _make_id(parent_class_nid, sanitized_name) + if config.ts_module == "tree_sitter_python": + func_nid = _python_underscore_salted_nid( + func_nid, sanitized_name, python_underscore_groups + ) add_node(func_nid, f".{func_name}()", line) add_edge(parent_class_nid, func_nid, "method", line) else: func_nid = _make_id(stem, sanitized_name) + if config.ts_module == "tree_sitter_python": + func_nid = _python_underscore_salted_nid( + func_nid, sanitized_name, python_underscore_groups + ) add_node(func_nid, f"{func_name}()", line) add_edge(file_nid, func_nid, "contains", line) callable_def_nids.add(func_nid) # function / method def is callable diff --git a/tests/test_python_underscore_resolution.py b/tests/test_python_underscore_resolution.py new file mode 100644 index 000000000..3e3cd26e7 --- /dev/null +++ b/tests/test_python_underscore_resolution.py @@ -0,0 +1,220 @@ +"""Regression coverage for Python leading-underscore node-id collisions (#3302). + +``ids.py:make_id`` strips leading/trailing underscores from every part before +normalizing, so ``_get_connection``/``get_connection`` (and more broadly any +name differing only by ``_``/``__``/dunder wrapping, e.g. ``x``/``_x``/``__x``/ +``__x__``) mint the SAME node id. ``add_node`` then silently drops whichever +declaration is walked second, so a public method/function can be entirely +absent from the graph while its private-by-convention sibling occupies the +public name. +""" +from __future__ import annotations +import textwrap +from pathlib import Path + +from graphify.extract import extract_python + + +def _write_py(tmp_path: Path, code: str) -> Path: + p = tmp_path / "mod.py" + p.write_text(textwrap.dedent(code)) + return p + + +def _rationale_free_nodes(result: dict) -> list[dict]: + return [n for n in result["nodes"] if n.get("file_type") != "rationale"] + + +def test_leading_underscore_method_collision_both_extracted(tmp_path: Path) -> None: + """#3302's exact repro: both methods survive as distinct nodes.""" + path = _write_py(tmp_path, ''' + class Adapter: + def _get_connection(self, url): + return url + + def get_connection(self, url): + return self._get_connection(url) + + def unrelated(self, x): + return x + ''') + result = extract_python(path) + labels = {n["label"] for n in _rationale_free_nodes(result)} + assert "._get_connection()" in labels, f"private method missing: {labels}" + assert ".get_connection()" in labels, f"public method missing (#3302): {labels}" + assert ".unrelated()" in labels + + +def test_leading_underscore_method_collision_ids_distinct(tmp_path: Path) -> None: + path = _write_py(tmp_path, ''' + class Adapter: + def _get_connection(self, url): + return url + + def get_connection(self, url): + return self._get_connection(url) + ''') + result = extract_python(path) + by_label = {n["label"]: n["id"] for n in _rationale_free_nodes(result)} + assert by_label["._get_connection()"] != by_label[".get_connection()"] + + +def test_public_method_keeps_plain_id_private_sibling_is_salted(tmp_path: Path) -> None: + """The public member's id must equal what it would be with no private sibling + at all -- an incremental rebuild that adds/removes the private sibling must + not re-point edges already targeting the public method.""" + solo = _write_py(tmp_path, ''' + class Adapter: + def get_connection(self, url): + return url + ''') + solo_result = extract_python(solo) + solo_id = next( + n["id"] for n in _rationale_free_nodes(solo_result) + if n["label"] == ".get_connection()" + ) + + with_sibling = _write_py(tmp_path, ''' + class Adapter: + def _get_connection(self, url): + return url + + def get_connection(self, url): + return self._get_connection(url) + ''') + result = extract_python(with_sibling) + nodes = _rationale_free_nodes(result) + public_id = next(n["id"] for n in nodes if n["label"] == ".get_connection()") + private_id = next(n["id"] for n in nodes if n["label"] == "._get_connection()") + + assert public_id == solo_id, ( + f"adding a private sibling moved the public method's id: {solo_id} -> {public_id}" + ) + assert private_id != public_id + + +def test_call_edge_resolves_to_the_salted_private_method(tmp_path: Path) -> None: + """`get_connection`'s call to `self._get_connection(...)` must bind to the + salted private-method node, not dangle or bind to the public one.""" + path = _write_py(tmp_path, ''' + class Adapter: + def _get_connection(self, url): + return url + + def get_connection(self, url): + return self._get_connection(url) + ''') + result = extract_python(path) + nodes = _rationale_free_nodes(result) + public_id = next(n["id"] for n in nodes if n["label"] == ".get_connection()") + private_id = next(n["id"] for n in nodes if n["label"] == "._get_connection()") + + calls = [e for e in result["edges"] if e.get("relation") == "calls"] + assert (public_id, private_id) in {(e["source"], e["target"]) for e in calls}, ( + f"no calls edge from get_connection to the salted _get_connection: {calls}" + ) + + +def test_module_level_function_collision_both_extracted(tmp_path: Path) -> None: + """Same bug, module-scoped (not inside a class).""" + path = _write_py(tmp_path, ''' + def _helper(): + return 1 + + def helper(): + return _helper() + ''') + result = extract_python(path) + labels = {n["label"] for n in _rationale_free_nodes(result)} + assert "_helper()" in labels + assert "helper()" in labels + by_label = {n["label"]: n["id"] for n in _rationale_free_nodes(result)} + assert by_label["_helper()"] != by_label["helper()"] + calls = {(e["source"], e["target"]) for e in result["edges"] if e.get("relation") == "calls"} + assert (by_label["helper()"], by_label["_helper()"]) in calls + + +def test_no_unique_public_member_salts_every_member(tmp_path: Path) -> None: + """`_x`/`__x` collide with no fully-public name in the group -- both must be + salted (order-independent), not one arbitrarily kept plain.""" + path = _write_py(tmp_path, ''' + class C: + def _x(self): + return 1 + + def __x(self): + return 2 + ''') + result = extract_python(path) + nodes = _rationale_free_nodes(result) + labels = {n["label"] for n in nodes} + assert "._x()" in labels and ".__x()" in labels + ids = {n["label"]: n["id"] for n in nodes} + plain_class_scope_id = next(n["id"] for n in nodes if n["label"] == "C") + # Neither survivor kept the bare, unsalted `_x` id. + for label in ("._x()", ".__x()"): + assert ids[label] != f"{plain_class_scope_id}_x", ( + f"{label} kept the unsalted id despite no unique public member" + ) + + +def test_dunder_and_plain_name_collision_both_extracted(tmp_path: Path) -> None: + """`__x__`, `__x`, and `x` all strip to the same id -- the fully public `x` + must win and the dunder must still be extracted, salted.""" + path = _write_py(tmp_path, ''' + class C: + def __x__(self): + return 1 + + def x(self): + return 2 + ''') + result = extract_python(path) + nodes = _rationale_free_nodes(result) + labels = {n["label"] for n in nodes} + assert ".__x__()" in labels and ".x()" in labels + ids = {n["label"]: n["id"] for n in nodes} + assert ids[".__x__()"] != ids[".x()"] + + +def test_no_collision_ids_unaffected(tmp_path: Path) -> None: + """A file with no underscore-only collisions must extract exactly as before + -- no unnecessary salting applied to unrelated names.""" + path = _write_py(tmp_path, ''' + class D: + def public_one(self): + return 1 + + def _private_two(self): + return 2 + ''') + result = extract_python(path) + nodes = _rationale_free_nodes(result) + ids = {n["label"]: n["id"] for n in nodes} + class_id = next(n["id"] for n in nodes if n["label"] == "D") + assert ids[".public_one()"] == f"{class_id}_public_one" + assert ids["._private_two()"] == f"{class_id}_private_two" + + +def test_collision_in_one_class_does_not_salt_unrelated_class(tmp_path: Path) -> None: + """A `_foo`/`foo` collision inside class A must not touch an unrelated, + non-colliding `_foo` in class B (scope-keyed, not name-keyed).""" + path = _write_py(tmp_path, ''' + class A: + def _foo(self): + return 1 + + def foo(self): + return self._foo() + + class B: + def _foo(self): + return 3 + ''') + result = extract_python(path) + nodes = _rationale_free_nodes(result) + class_b_id = next(n["id"] for n in nodes if n["label"] == "B") + b_foo = next(n for n in nodes if n["id"].startswith(class_b_id) and n["label"] == "._foo()") + assert b_foo["id"] == f"{class_b_id}_foo", ( + "an unrelated class's non-colliding _foo was needlessly salted" + )