mirror of
https://github.com/safishamsi/graphify.git
synced 2026-10-21 11:21:27 +00:00
fix(js): keep class members that differ only by _, $ or # as separate nodes
make_id drops _, $ and #, so updateHash/_updateHash, close/#close and set/$set in one JS/TS class body minted one id and the second member disappeared, with its calls attributed to the survivor. Group each class body's member names by id and reuse the Python/C# collision rule (_underscore_salted_nid, now with configurable private prefixes): a unique public name keeps its id, colliding private ones get a salted id. Non-colliding members keep their ids. The TS cross-file member resolver now matches the exact member name before falling back to the normalized key, so c.set() still binds to set and not $set. Fixes #4475 Co-Authored-By: Grok Bot
This commit is contained in:
1 parent
10130e79ed
commit
03beac9d09
3 files changed
+201
-7
No files matched your search
+10
-2
@@ -5044,6 +5044,10 @@ def _resolve_typescript_member_calls(
|
||||
type_def_nids.setdefault(_key(n.get("label", "")), []).append(n["id"])
|
||||
|
||||
method_index: dict[tuple[str, str], str] = {}
|
||||
# Exact member name -> node. `_key` drops `_`/`$`/`#`, and since #4475
|
||||
# `updateHash` / `_updateHash` are two nodes of one class, so the exact
|
||||
# name is tried first and `_key` stays the fallback.
|
||||
method_exact: dict[tuple[str, str], str] = {}
|
||||
class_of_method: dict[str, str] = {}
|
||||
for e in all_edges:
|
||||
if e.get("relation") != "method":
|
||||
@@ -5052,6 +5056,7 @@ def _resolve_typescript_member_calls(
|
||||
tnode = node_by_id.get(tgt)
|
||||
if tnode is not None:
|
||||
method_index[(src, _key(tnode.get("label", "")))] = tgt
|
||||
method_exact[(src, str(tnode.get("label", "")).removesuffix("()").removeprefix("."))] = tgt
|
||||
if isinstance(src, str) and isinstance(tgt, str):
|
||||
class_of_method[tgt] = src
|
||||
|
||||
@@ -5181,7 +5186,10 @@ def _resolve_typescript_member_calls(
|
||||
if len(visible) != 1:
|
||||
continue
|
||||
type_nid = visible[0]
|
||||
method_nid = method_index.get((type_nid, _key(callee)))
|
||||
method_nid = method_exact.get((type_nid, str(callee)))
|
||||
exact = method_nid is not None
|
||||
if not exact:
|
||||
method_nid = method_index.get((type_nid, _key(callee)))
|
||||
if not method_nid:
|
||||
# Receiver typed, but the type has no such method. The old fallback
|
||||
# (a `references` edge to the type node) was another fabrication
|
||||
@@ -5192,7 +5200,7 @@ def _resolve_typescript_member_calls(
|
||||
# declares a public `foo` (the node is marked); otherwise its `foo` comes
|
||||
# from elsewhere and the private method is not the target.
|
||||
method_node = node_by_id.get(method_nid, {})
|
||||
if (str(method_node.get("label", "")).lstrip(".").startswith("#")
|
||||
if (not exact and str(method_node.get("label", "")).lstrip(".").startswith("#")
|
||||
and not str(callee).startswith("#")
|
||||
and not method_node.get("_ts_public_twin")):
|
||||
continue
|
||||
|
||||
@@ -1290,7 +1290,8 @@ def _python_pre_scan_underscore_collisions(root_node, source: bytes, stem: str)
|
||||
return {nid: names for nid, names in groups.items() if len(names) >= 2}
|
||||
|
||||
|
||||
def _underscore_salted_nid(plain_nid: str, name: str, groups: dict[str, set[str]]) -> str:
|
||||
def _underscore_salted_nid(plain_nid: str, name: str, groups: dict[str, set[str]],
|
||||
private_prefixes: tuple[str, ...] = ("_",)) -> str:
|
||||
"""Resolve a declaration's real node id against the collision map.
|
||||
|
||||
Shared by the Python and C# pre-scans (#3302, #4415). A name with no
|
||||
@@ -1303,17 +1304,63 @@ def _underscore_salted_nid(plain_nid: str, name: str, groups: dict[str, set[str]
|
||||
`_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).
|
||||
``private_prefixes`` widens what counts as non-public: JS/TS also marks
|
||||
members private with ``$`` and ``#`` (#4475).
|
||||
"""
|
||||
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("_")]
|
||||
public = [n for n in names if not n.startswith(private_prefixes)]
|
||||
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)
|
||||
|
||||
|
||||
_JS_PRIVATE_MEMBER_PREFIXES = ("_", "$", "#")
|
||||
_JS_MEMBER_METHOD_TYPES = ("method_definition", "method_signature", "abstract_method_signature")
|
||||
|
||||
|
||||
def _js_member_nid(class_nid: str, member_node, name: str, source: bytes,
|
||||
groups_cache: dict) -> str:
|
||||
"""Node id for a JS/TS class member, salted when a sibling collides (#4475).
|
||||
|
||||
``make_id`` drops ``_``, ``$`` and ``#``, so ``updateHash`` / ``_updateHash``,
|
||||
``close`` / ``#close`` and ``set`` / ``$set`` in one class body mint one id
|
||||
and the second declaration vanished. The member names of the enclosing
|
||||
class body are grouped by the id they would get (methods, signatures and
|
||||
function-valued fields, the members the walk turns into ``.name()`` nodes),
|
||||
then ``_underscore_salted_nid`` applies the Python/C# rule: a unique public
|
||||
name keeps the plain id, every other colliding name gets a salted one.
|
||||
Members that collide with nothing keep their old id. Getter/setter pairs
|
||||
and overloads share one name, so they are not a collision.
|
||||
"""
|
||||
body = member_node.parent
|
||||
if body is None:
|
||||
return _make_id(class_nid, name)
|
||||
key = (class_nid, body.start_byte, body.end_byte)
|
||||
groups = groups_cache.get(key)
|
||||
if groups is None:
|
||||
names: dict[str, set[str]] = {}
|
||||
for c in body.children:
|
||||
if c.type in _JS_MEMBER_METHOD_TYPES:
|
||||
name_node = c.child_by_field_name("name")
|
||||
elif c.type in ("field_definition", "public_field_definition"):
|
||||
value = c.child_by_field_name("value")
|
||||
if value is None or value.type not in _JS_FUNCTION_VALUE_TYPES:
|
||||
continue
|
||||
name_node = c.child_by_field_name("property") or c.child_by_field_name("name")
|
||||
else:
|
||||
continue
|
||||
text = _read_text(name_node, source) if name_node is not None else ""
|
||||
if text:
|
||||
names.setdefault(_make_id(class_nid, text), set()).add(text)
|
||||
groups = {nid: ns for nid, ns in names.items() if len(ns) >= 2}
|
||||
groups_cache[key] = groups
|
||||
return _underscore_salted_nid(_make_id(class_nid, name), name, groups,
|
||||
_JS_PRIVATE_MEMBER_PREFIXES)
|
||||
|
||||
|
||||
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()
|
||||
@@ -3927,7 +3974,7 @@ def _js_extra_walk(node, source: bytes, file_nid: str, stem: str, str_path: str,
|
||||
callable_def_nids: set | None = None,
|
||||
local_bound_names: dict | None = None,
|
||||
closure_locals_by_body: dict | None = None,
|
||||
config=None) -> bool:
|
||||
config=None, js_member_groups: dict | None = None) -> bool:
|
||||
"""Handle lexical_declaration (arrow functions, CJS requires, module-level const literals) for JS/TS. Returns True if handled."""
|
||||
# CommonJS / prototype member assignments whose value is a function:
|
||||
# exports.X = () => {} → file-contained function X()
|
||||
@@ -4065,7 +4112,8 @@ def _js_extra_walk(node, source: bytes, file_nid: str, stem: str, str_path: str,
|
||||
field_name = _read_text(prop, source)
|
||||
if field_name:
|
||||
line = node.start_point[0] + 1
|
||||
nid = _make_id(parent_class_nid, field_name)
|
||||
nid = _js_member_nid(parent_class_nid, node, field_name, source,
|
||||
js_member_groups if js_member_groups is not None else {})
|
||||
add_node_fn(nid, f".{field_name}()", line)
|
||||
add_edge_fn(parent_class_nid, nid, "method", line)
|
||||
if callable_def_nids is not None:
|
||||
@@ -5677,6 +5725,9 @@ def _extract_generic(
|
||||
if config.ts_module == "tree_sitter_swift":
|
||||
swift_protocol_names, swift_class_names = _swift_pre_scan(root, source)
|
||||
|
||||
# JS/TS class body -> member collision groups, filled lazily (#4475).
|
||||
js_member_groups: dict = {}
|
||||
|
||||
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)
|
||||
@@ -7333,6 +7384,11 @@ def _extract_generic(
|
||||
func_nid = _underscore_salted_nid(
|
||||
func_nid, sanitized_name, csharp_member_groups
|
||||
)
|
||||
elif (config.ts_module in ("tree_sitter_javascript", "tree_sitter_typescript")
|
||||
and t in _JS_MEMBER_METHOD_TYPES):
|
||||
func_nid = _js_member_nid(
|
||||
parent_class_nid, node, sanitized_name, source, js_member_groups
|
||||
)
|
||||
ruby_method_metadata = None
|
||||
if ruby_method_kind is not None:
|
||||
kinds = ruby_method_kinds.setdefault(func_nid, set())
|
||||
@@ -7923,7 +7979,8 @@ def _extract_generic(
|
||||
nodes, edges, seen_ids, function_bodies,
|
||||
parent_class_nid, add_node, add_edge,
|
||||
callable_def_nids, local_bound_names,
|
||||
closure_locals_by_body, config=config):
|
||||
closure_locals_by_body, config=config,
|
||||
js_member_groups=js_member_groups):
|
||||
return
|
||||
|
||||
# TS enum members, and namespace / module containers
|
||||
|
||||
@@ -0,0 +1,129 @@
|
||||
"""#4475: JS/TS class members that differ only by `_`, `$` or `#` stay separate nodes."""
|
||||
from __future__ import annotations
|
||||
|
||||
import warnings
|
||||
from pathlib import Path
|
||||
|
||||
import pytest
|
||||
|
||||
from graphify.extract import extract
|
||||
|
||||
|
||||
def _extract(tmp_path: Path, files: dict[str, str]) -> dict:
|
||||
for name, src in files.items():
|
||||
(tmp_path / name).write_text(src, encoding="utf-8")
|
||||
with warnings.catch_warnings():
|
||||
warnings.simplefilter("ignore")
|
||||
return extract([tmp_path / n for n in files], cache_root=tmp_path, parallel=False)
|
||||
|
||||
|
||||
def _members(r: dict) -> dict[str, str]:
|
||||
return {n["label"]: n["id"] for n in r["nodes"] if n["label"].startswith(".")}
|
||||
|
||||
|
||||
def _calls(r: dict) -> set[tuple[str, str]]:
|
||||
label = {n["id"]: n["label"] for n in r["nodes"]}
|
||||
return {(label.get(e["source"]), label.get(e["target"])) for e in r["edges"] if e["relation"] == "calls"}
|
||||
|
||||
|
||||
@pytest.mark.parametrize("fname,src,expected", [
|
||||
("a.js", "export class R { _onBegin(a) { return a; } onBegin(b) { return b; } }\n", {"._onBegin()", ".onBegin()"}),
|
||||
("a.js", "export class W { #close(a) { return a; } close(b) { return this.#close(b); } }\n", {".#close()", ".close()"}),
|
||||
("a.ts", "export declare class Q { $set(a: number): void; set(b: number): void; }\n", {".$set()", ".set()"}),
|
||||
("a.js", "export class A {\n updateHash() { return 1; }\n _updateHash() { return 2; }\n}\n", {".updateHash()", "._updateHash()"}),
|
||||
("a.ts", "export abstract class B { abstract _run(): void; abstract run(): void; }\n", {"._run()", ".run()"}),
|
||||
("a.js", "export class F { _go = () => 1; go = () => 2; }\n", {"._go()", ".go()"}),
|
||||
])
|
||||
def test_issue_cases_get_two_nodes(tmp_path, fname, src, expected):
|
||||
assert expected <= set(_members(_extract(tmp_path, {fname: src})))
|
||||
|
||||
|
||||
def test_public_member_keeps_the_plain_id_and_private_one_is_salted(tmp_path):
|
||||
m = _members(_extract(tmp_path, {"a.js": "export class W { #close(a) { return a; } close(b) { return b; } }\n"}))
|
||||
assert m[".close()"] == "a_w_close"
|
||||
assert m[".#close()"].startswith("a_w_close_") and m[".#close()"] != "a_w_close"
|
||||
|
||||
|
||||
def test_order_does_not_matter(tmp_path):
|
||||
(tmp_path / "1").mkdir()
|
||||
(tmp_path / "2").mkdir()
|
||||
a = _members(_extract(tmp_path / "1", {"a.js": "export class A { _x() {} x() {} }\n"}))
|
||||
b = _members(_extract(tmp_path / "2", {"a.js": "export class A { x() {} _x() {} }\n"}))
|
||||
assert a == b
|
||||
|
||||
|
||||
def test_no_public_member_salts_both(tmp_path):
|
||||
m = _members(_extract(tmp_path, {"a.js": "export class P { _x() { return 1; } $x() { return 2; } }\n"}))
|
||||
assert set(m) == {"._x()", ".$x()"}
|
||||
assert "a_p_x" not in m.values() and len(set(m.values())) == 2
|
||||
|
||||
|
||||
def test_non_colliding_member_ids_unchanged(tmp_path):
|
||||
m = _members(_extract(tmp_path, {"a.ts": (
|
||||
"export class C {\n run() {}\n _helper() {}\n #secret() {}\n $id() {}\n"
|
||||
" get size() { return 1; }\n set size(v) {}\n static make() {}\n"
|
||||
" over(a: number): void;\n over(a: string): void;\n over(a: any) {}\n}\n")}))
|
||||
assert m == {".run()": "a_c_run", "._helper()": "a_c_helper", ".#secret()": "a_c_secret",
|
||||
".$id()": "a_c_id", ".size()": "a_c_size", ".make()": "a_c_make", ".over()": "a_c_over"}
|
||||
|
||||
|
||||
def test_call_edges_go_to_the_right_member(tmp_path):
|
||||
r = _extract(tmp_path, {"w.js": (
|
||||
"export class W {\n #close(a) { return helper(a); }\n close(b) { return this.#close(b); }\n"
|
||||
" shut() { return this.close(1); }\n}\nfunction helper(x) { return x; }\n")})
|
||||
calls = _calls(r)
|
||||
assert (".close()", ".#close()") in calls
|
||||
assert (".shut()", ".close()") in calls
|
||||
assert (".#close()", "helper()") in calls # the private body's own call is credited to it
|
||||
assert (".close()", "helper()") not in calls
|
||||
|
||||
|
||||
@pytest.mark.parametrize("call,target", [("a.updateHash()", ".updateHash()"), ("a._updateHash()", "._updateHash()")])
|
||||
@pytest.mark.parametrize("cross_file", [False, True])
|
||||
def test_callers_reach_the_right_member(tmp_path, call, target, cross_file):
|
||||
cls = "export class A {\n updateHash() { return 1; }\n _updateHash() { return 2; }\n}\n"
|
||||
fn = f"export function f() {{ const a = new A(); return {call}; }}\n"
|
||||
files = ({"a.js": cls, "u.js": "import { A } from './a.js';\n" + fn} if cross_file else {"a.js": cls + fn})
|
||||
members = {t for s, t in _calls(_extract(tmp_path, files)) if s == "f()" and t and t.startswith(".")}
|
||||
assert members == {target}
|
||||
|
||||
|
||||
def test_cross_file_ts_typed_receiver_hits_each_member(tmp_path):
|
||||
r = _extract(tmp_path, {
|
||||
"ctx.ts": "export class Ctx {\n set(v: number): void {}\n $set(v: number): void {}\n}\n",
|
||||
"use.ts": ('import { Ctx } from "./ctx";\n'
|
||||
"export function a(c: Ctx) { c.set(1); }\nexport function b(c: Ctx) { c.$set(1); }\n"),
|
||||
})
|
||||
calls = _calls(r)
|
||||
assert ("a()", ".set()") in calls and ("a()", ".$set()") not in calls
|
||||
assert ("b()", ".$set()") in calls and ("b()", ".set()") not in calls
|
||||
|
||||
|
||||
def test_dedup_build_and_merge_keep_both(tmp_path):
|
||||
from graphify.build import build, build_merge
|
||||
from graphify.cluster import cluster
|
||||
from graphify.export import to_json
|
||||
src = {"a.js": "export class A {\n close() { return this.#close(); }\n #close() { return 1; }\n}\n"}
|
||||
r = _extract(tmp_path, src)
|
||||
G = build([r], dedup=True, root=str(tmp_path))
|
||||
labels = {G.nodes[n]["label"] for n in G.nodes}
|
||||
assert {".close()", ".#close()"} <= labels
|
||||
to_json(G, cluster(G), str(tmp_path / "graph.json"))
|
||||
G2 = build_merge([_extract(tmp_path, src)], graph_path=str(tmp_path / "graph.json"), root=str(tmp_path))
|
||||
assert set(G2.nodes) == set(G.nodes)
|
||||
|
||||
|
||||
|
||||
def test_pre_fix_ast_cache_entry_is_never_served_for_js(tmp_path):
|
||||
"""JS/TS bypass the AST cache, so a collapsed entry left by an older run can't resurface (#4475)."""
|
||||
from graphify.cache import save_cached
|
||||
from graphify.extract import cache_salt, extract_js
|
||||
src = tmp_path / "a.js"
|
||||
src.write_text("export class A {\n updateHash() { return 1; }\n _updateHash() { return 2; }\n}\n", encoding="utf-8")
|
||||
stale = extract_js(src)
|
||||
stale["nodes"] = [n for n in stale["nodes"] if n.get("label") != "._updateHash()"]
|
||||
save_cached(src, stale, root=tmp_path, cache_root=tmp_path, kind="ast", salt=cache_salt(src))
|
||||
with warnings.catch_warnings():
|
||||
warnings.simplefilter("ignore")
|
||||
r = extract([src], root=tmp_path, cache_root=tmp_path, parallel=False)
|
||||
assert {".updateHash()", "._updateHash()"} <= {n["label"] for n in r["nodes"]}
|
||||
Reference in new issue
Block a user