mirror of
https://github.com/safishamsi/graphify.git
synced 2026-10-21 19:31:17 +00:00
fix(python): salt module-level class ids that collide with a function or class (#2810)
The Python collision pre-scan (#3302) recorded module-level functions and class methods but not module-level class names, so a class sharing an id with a function or another class after normalization still lost one declaration to first-wins add_node: - `class Session` / `def session()` (requests/sessions.py): session() is absent from the graph, and which one survives depends on declaration order - `class Foo` / `class FOO`, `class Pair` / `class _Pair` Record module-level classes in the same scope as functions and salt the class id with the existing _underscore_salted_nid rule. Methods of a salted class are minted under the salted id, so the pre-scan now keys their groups under the class's resolved id; keying them under the plain one would drop the existing `_get`/`get` protection inside that class. Same-file base-class references go through the same salt (as C# already does), so `class Sub(Session)` binds to the real class instead of minting a sourceless `Session` stub. The pre-scan also looks through decorated_definition, so `@dataclass class Session` and the `_x` / `@property x` method idiom are covered too. Ids change only for files that contain such a collision; cached AST results for them keep the collapsed ids until the AST cache schema is bumped. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
1 parent
10130e79ed
commit
03b420df97
2 files changed
+242
-13
No files matched your search
@@ -1246,8 +1246,13 @@ def _python_pre_scan_underscore_collisions(root_node, source: bytes, stem: str)
|
||||
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``.
|
||||
module-level function, module-level class, and direct method of a
|
||||
module-level class — the cases the id-minting code actually distinguishes
|
||||
via ``parent_class_nid``. Module-level classes share the functions' scope, so
|
||||
``class Session`` / ``def session()`` and ``class Foo`` / ``class FOO``
|
||||
(casefold) collide there too (#2810). Methods are keyed under their class's
|
||||
*resolved* id: a salted class mints its methods under the salted id, so
|
||||
keying them under the plain one would miss every method collision inside it.
|
||||
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
|
||||
@@ -1261,7 +1266,15 @@ def _python_pre_scan_underscore_collisions(root_node, source: bytes, stem: str)
|
||||
def _record(plain_nid: str, name: str) -> None:
|
||||
groups.setdefault(plain_nid, set()).add(name)
|
||||
|
||||
for child in root_node.children:
|
||||
def _undecorated(node):
|
||||
# `@dataclass class X` / `@property def x` wrap the definition in a
|
||||
# decorated_definition; the walker mints the wrapped one's id unchanged.
|
||||
if node.type == "decorated_definition":
|
||||
return node.child_by_field_name("definition") or node
|
||||
return node
|
||||
|
||||
classes: list = [] # (class_name, body) for the method pass below
|
||||
for child in map(_undecorated, root_node.children):
|
||||
if child.type == "function_definition":
|
||||
name_node = child.child_by_field_name("name")
|
||||
if name_node is not None:
|
||||
@@ -1276,16 +1289,21 @@ def _python_pre_scan_underscore_collisions(root_node, source: bytes, stem: str)
|
||||
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)
|
||||
_record(_make_id(stem, class_name), class_name)
|
||||
classes.append((class_name, body))
|
||||
|
||||
module_groups = {nid: names for nid, names in groups.items() if len(names) >= 2}
|
||||
for class_name, body in classes:
|
||||
class_nid = _underscore_salted_nid(_make_id(stem, class_name), class_name, module_groups)
|
||||
for member in map(_undecorated, 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}
|
||||
|
||||
@@ -5727,6 +5745,8 @@ def _extract_generic(
|
||||
nid = _make_id(stem, ".".join(namespace_stack), name)
|
||||
if config.ts_module == "tree_sitter_c_sharp":
|
||||
nid = _underscore_salted_nid(nid, name, csharp_type_groups)
|
||||
elif config.ts_module == "tree_sitter_python":
|
||||
nid = _underscore_salted_nid(nid, name, python_underscore_groups)
|
||||
if nid in seen_ids:
|
||||
return nid
|
||||
nid = _make_id(name)
|
||||
@@ -5880,6 +5900,9 @@ def _extract_generic(
|
||||
class_nid = _underscore_salted_nid(class_nid, class_name, csharp_type_groups)
|
||||
if config.ts_module == "tree_sitter_python" and parent_class_nid:
|
||||
class_nid = _make_id(parent_class_nid, class_name)
|
||||
elif config.ts_module == "tree_sitter_python":
|
||||
# `Session` / `session()`, `Foo` / `FOO` in one module (#2810).
|
||||
class_nid = _underscore_salted_nid(class_nid, class_name, python_underscore_groups)
|
||||
line = node.start_point[0] + 1
|
||||
metadata = None
|
||||
ruby_reopened = (
|
||||
|
||||
@@ -218,3 +218,209 @@ def test_collision_in_one_class_does_not_salt_unrelated_class(tmp_path: Path) ->
|
||||
assert b_foo["id"] == f"{class_b_id}_foo", (
|
||||
"an unrelated class's non-colliding _foo was needlessly salted"
|
||||
)
|
||||
|
||||
|
||||
# --- Module-level classes join the collision map (#2810) -------------------
|
||||
#
|
||||
# The pre-scan above records module-level functions and class methods but not
|
||||
# module-level class names, so a class colliding with a function or another
|
||||
# class on one id (`Session` / `session()`, `Foo` / `FOO`, `Pair` / `_Pair`)
|
||||
# still lost one declaration to first-wins `add_node`, and which one survived
|
||||
# depended on declaration order.
|
||||
|
||||
|
||||
def _labels_to_ids(result: dict) -> dict[str, str]:
|
||||
return {n["label"]: n["id"] for n in _rationale_free_nodes(result)}
|
||||
|
||||
|
||||
def test_class_and_function_case_collision_both_extracted(tmp_path: Path) -> None:
|
||||
"""requests/sessions.py's shape: a class and its lowercase factory."""
|
||||
path = _write_py(tmp_path, '''
|
||||
class Session:
|
||||
pass
|
||||
|
||||
def session():
|
||||
return Session()
|
||||
''')
|
||||
ids = _labels_to_ids(extract_python(path))
|
||||
assert "Session" in ids, f"class missing (#2810): {sorted(ids)}"
|
||||
assert "session()" in ids, f"function missing (#2810): {sorted(ids)}"
|
||||
assert ids["Session"] != ids["session()"]
|
||||
|
||||
|
||||
def test_class_and_function_case_collision_is_order_independent(tmp_path: Path) -> None:
|
||||
"""Function declared first must not change which declarations survive or
|
||||
what ids they get."""
|
||||
class_first = _labels_to_ids(extract_python(_write_py(tmp_path, '''
|
||||
class Session:
|
||||
pass
|
||||
|
||||
def session():
|
||||
return Session()
|
||||
''')))
|
||||
func_first = _labels_to_ids(extract_python(_write_py(tmp_path, '''
|
||||
def session():
|
||||
return Session()
|
||||
|
||||
class Session:
|
||||
pass
|
||||
''')))
|
||||
assert class_first["Session"] == func_first["Session"]
|
||||
assert class_first["session()"] == func_first["session()"]
|
||||
|
||||
|
||||
def test_class_and_class_case_collision_both_extracted(tmp_path: Path) -> None:
|
||||
path = _write_py(tmp_path, '''
|
||||
class Foo:
|
||||
pass
|
||||
|
||||
class FOO:
|
||||
pass
|
||||
''')
|
||||
ids = _labels_to_ids(extract_python(path))
|
||||
assert "Foo" in ids and "FOO" in ids, f"a class was dropped: {sorted(ids)}"
|
||||
assert ids["Foo"] != ids["FOO"]
|
||||
|
||||
|
||||
def test_class_underscore_collision_public_class_keeps_plain_id(tmp_path: Path) -> None:
|
||||
"""`Pair` / `_Pair`: same unique-public-member rule as functions -- the
|
||||
public class keeps the id it has with no private sibling."""
|
||||
solo_id = _labels_to_ids(extract_python(_write_py(tmp_path, '''
|
||||
class Pair:
|
||||
pass
|
||||
''')))["Pair"]
|
||||
ids = _labels_to_ids(extract_python(_write_py(tmp_path, '''
|
||||
class _Pair:
|
||||
pass
|
||||
|
||||
class Pair:
|
||||
pass
|
||||
''')))
|
||||
assert "_Pair" in ids and "Pair" in ids
|
||||
assert ids["Pair"] == solo_id
|
||||
assert ids["_Pair"] != solo_id
|
||||
|
||||
|
||||
def test_methods_of_a_salted_class_keep_underscore_protection(tmp_path: Path) -> None:
|
||||
"""Once the class id is salted its methods are minted under the salted id,
|
||||
so the pre-scan must key their collision groups under it too -- otherwise
|
||||
`_get`/`get` inside the salted class collapse again."""
|
||||
path = _write_py(tmp_path, '''
|
||||
class Session:
|
||||
def _get(self):
|
||||
return 1
|
||||
|
||||
def get(self):
|
||||
return self._get()
|
||||
|
||||
def session():
|
||||
return Session()
|
||||
''')
|
||||
ids = _labels_to_ids(extract_python(path))
|
||||
assert "._get()" in ids and ".get()" in ids, f"a method was dropped: {sorted(ids)}"
|
||||
assert ids["._get()"] != ids[".get()"]
|
||||
assert ids[".get()"].startswith(ids["Session"]), "method not scoped under its class"
|
||||
|
||||
|
||||
def test_constructor_call_binds_to_the_salted_class(tmp_path: Path) -> None:
|
||||
"""`session()`'s `Session()` call must reach the class node, not dangle or
|
||||
bind back to `session()` itself."""
|
||||
path = _write_py(tmp_path, '''
|
||||
class Session:
|
||||
pass
|
||||
|
||||
def session():
|
||||
return Session()
|
||||
''')
|
||||
result = extract_python(path)
|
||||
ids = _labels_to_ids(result)
|
||||
pairs = {(e["source"], e["target"]) for e in result["edges"]}
|
||||
assert (ids["session()"], ids["Session"]) in pairs, (
|
||||
f"no edge from session() to the Session class: {result['edges']}"
|
||||
)
|
||||
assert (ids["session()"], ids["session()"]) not in pairs
|
||||
|
||||
|
||||
def test_cross_file_import_resolves_to_the_salted_class(tmp_path: Path) -> None:
|
||||
from graphify.build import build
|
||||
from graphify.extract import extract
|
||||
|
||||
(tmp_path / "sessions.py").write_text(textwrap.dedent('''
|
||||
class Session:
|
||||
pass
|
||||
|
||||
def session():
|
||||
return Session()
|
||||
'''))
|
||||
(tmp_path / "api.py").write_text(textwrap.dedent('''
|
||||
from sessions import Session, session
|
||||
|
||||
def make():
|
||||
return Session(), session()
|
||||
'''))
|
||||
G = build([extract(sorted(tmp_path.glob("*.py")), root=tmp_path, cache_root=tmp_path)])
|
||||
label = {n: d.get("label") for n, d in G.nodes(data=True)}
|
||||
targets = {
|
||||
label.get(d.get("_tgt", v))
|
||||
for u, v, d in G.edges(data=True)
|
||||
if label.get(d.get("_src", u)) in ("api.py", "make()")
|
||||
}
|
||||
assert "Session" in targets, f"nothing from api.py reaches the class: {targets}"
|
||||
assert "session()" in targets, f"nothing from api.py reaches the function: {targets}"
|
||||
|
||||
|
||||
def test_same_file_subclass_inherits_from_the_salted_class(tmp_path: Path) -> None:
|
||||
"""`class Sub(Session)` must point at the real (salted) class, not mint a
|
||||
second sourceless `Session` stub under the plain id."""
|
||||
path = _write_py(tmp_path, '''
|
||||
class Session:
|
||||
pass
|
||||
|
||||
def session():
|
||||
return Session()
|
||||
|
||||
class Sub(Session):
|
||||
pass
|
||||
''')
|
||||
result = extract_python(path)
|
||||
nodes = _rationale_free_nodes(result)
|
||||
assert [n["label"] for n in nodes].count("Session") == 1, (
|
||||
f"a phantom Session stub was minted: {[(n['id'], n['label']) for n in nodes]}"
|
||||
)
|
||||
ids = _labels_to_ids(result)
|
||||
inherits = {(e["source"], e["target"]) for e in result["edges"] if e.get("relation") == "inherits"}
|
||||
assert (ids["Sub"], ids["Session"]) in inherits
|
||||
|
||||
|
||||
def test_decorated_class_and_function_case_collision_both_extracted(tmp_path: Path) -> None:
|
||||
"""`@dataclass class Session` is wrapped in a decorated_definition; the
|
||||
pre-scan must see through it."""
|
||||
path = _write_py(tmp_path, '''
|
||||
from dataclasses import dataclass
|
||||
|
||||
@dataclass
|
||||
class Session:
|
||||
x: int = 0
|
||||
|
||||
def session():
|
||||
return Session()
|
||||
''')
|
||||
ids = _labels_to_ids(extract_python(path))
|
||||
assert "Session" in ids and "session()" in ids, f"a declaration was dropped: {sorted(ids)}"
|
||||
assert ids["Session"] != ids["session()"]
|
||||
|
||||
|
||||
def test_decorated_method_underscore_collision_both_extracted(tmp_path: Path) -> None:
|
||||
"""The `_x` / `@property x` idiom: a decorated method collides like a bare one."""
|
||||
path = _write_py(tmp_path, '''
|
||||
class C:
|
||||
def _x(self):
|
||||
return 1
|
||||
|
||||
@property
|
||||
def x(self):
|
||||
return self._x()
|
||||
''')
|
||||
ids = _labels_to_ids(extract_python(path))
|
||||
assert "._x()" in ids and ".x()" in ids, f"a method was dropped: {sorted(ids)}"
|
||||
assert ids["._x()"] != ids[".x()"]
|
||||
Reference in new issue
Block a user