From 51e1782ad753a13dd3783f797dfb2ef9fa22029a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Robert=20S=C3=B6semann?= Date: Wed, 26 Aug 2026 12:43:46 +0200 Subject: [PATCH] fix(apex): don't treat referenced types as definitions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A type a file only references (`extends Bar`, `[SELECT Id FROM Account]`) was stamped with the referencing file's source_file, so it read as a definition and got its id salted per referencing file. One class referenced from five files became five unconnected nodes and no edge reached the real definition — the #1402/#2324 pattern already fixed for SQL and OCaml. Referenced types now get a sourceless placeholder with no contains edge, so _rewire_unique_stub_nodes collapses it onto the unique real definition. No origin_file: _node_disambiguation_source_key falls back to it and would re-introduce the same per-file salting. Also match class modifiers order-independently. Apex allows them in any order and any number, so `public abstract with sharing class Foo` produced no node at all under the fixed access/sharing/modifier sequence. --- graphify/extractors/apex.py | 87 +++++++++++++++++++----------- tests/fixtures/apex_modifiers.cls | 18 +++++++ tests/test_apex_type_resolution.py | 70 ++++++++++++++++++++++++ tests/test_languages.py | 47 ++++++++++++++++ 4 files changed, 191 insertions(+), 31 deletions(-) create mode 100644 tests/fixtures/apex_modifiers.cls create mode 100644 tests/test_apex_type_resolution.py diff --git a/graphify/extractors/apex.py b/graphify/extractors/apex.py index 928923a640..5592d2fba7 100644 --- a/graphify/extractors/apex.py +++ b/graphify/extractors/apex.py @@ -34,6 +34,40 @@ def add_node(nid: str, label: str, line: int) -> None: "source_location": f"L{line}", }) + def add_stub(nid: str, label: str) -> None: + """Node for a type or SObject this file only REFERENCES, not declares. + + The definition lives elsewhere, so the placeholder must stay + SOURCELESS: a stamped ``source_file`` reads as a definition, and + ``_disambiguate_colliding_node_ids`` then salts the id per referencing + file, so one class becomes N unconnected nodes and no edge reaches the + real definition (#1402/#2324, same fix as the SQL extractor). No + ``contains`` edge either, for the same reason. + + Unlike the SQL/CommonLisp stubs this sets no ``origin_file``: + ``_node_disambiguation_source_key`` falls back to it, which would + re-introduce exactly the per-file salting. + """ + if nid not in seen_ids: + seen_ids.add(nid) + nodes.append({ + "id": nid, + "label": label, + "file_type": "code", + "source_file": "", + "source_location": "", + }) + + def type_ref(name: str) -> str: + """Id for a referenced type: this file's own declaration when it has + one, otherwise a sourceless stub the corpus-level rewire resolves.""" + local_nid = _make_id(stem, name) + if local_nid in seen_ids: + return local_nid + nid = _make_id(name) + add_stub(nid, name) + return nid + def add_edge(src: str, tgt: str, relation: str, line: int, confidence: str = "EXTRACTED") -> None: edges.append({ @@ -50,23 +84,30 @@ def add_edge(src: str, tgt: str, relation: str, line: int, lines = source.splitlines() - _ACCESS = r"(?:public|private|protected|global|webService)?" - _SHARING = r"(?:\s+(?:with|without|inherited)\s+sharing)?" - _MOD = r"(?:\s+(?:abstract|virtual|override|static|final|transient|testMethod))?" _ANNOTATION = r"(?:\s*@\w+(?:\s*\([^)]*\))?\s*)*" + # Apex puts no ordering constraint on modifiers: `public abstract with + # sharing class Foo` and `public with sharing abstract class Foo` are both + # legal, as is any number of them. Matching a fixed + # access -> sharing -> modifier sequence silently dropped every declaration + # written in another order, so the type produced no node at all. + _MODIFIERS = ( + r"(?:(?:public|private|protected|global|webService" + r"|abstract|virtual|override|static|final|transient|testMethod)\s+" + r"|(?:with|without|inherited)\s+sharing\s+)*" + ) cls_re = _re.compile( - rf"^{_ANNOTATION}\s*{_ACCESS}{_SHARING}{_MOD}\s*class\s+(\w+)" + rf"^{_ANNOTATION}\s*{_MODIFIERS}class\s+(\w+)" rf"(?:\s+extends\s+(\w+))?(?:\s+implements\s+([\w,\s]+))?\s*\{{?", _re.IGNORECASE, ) iface_re = _re.compile( - rf"^{_ANNOTATION}\s*{_ACCESS}{_SHARING}{_MOD}\s*interface\s+(\w+)" + rf"^{_ANNOTATION}\s*{_MODIFIERS}interface\s+(\w+)" rf"(?:\s+extends\s+([\w,\s]+))?\s*\{{?", _re.IGNORECASE, ) enum_re = _re.compile( - rf"^{_ANNOTATION}\s*{_ACCESS}{_SHARING}{_MOD}\s*enum\s+(\w+)\s*\{{?", + rf"^{_ANNOTATION}\s*{_MODIFIERS}enum\s+(\w+)\s*\{{?", _re.IGNORECASE, ) trigger_re = _re.compile( @@ -74,7 +115,7 @@ def add_edge(src: str, tgt: str, relation: str, line: int, _re.IGNORECASE, ) method_re = _re.compile( - rf"^{_ANNOTATION}\s*{_ACCESS}{_MOD}\s*(?:static\s+)?[\w<>\[\]]+\s+(\w+)\s*\([^)]*\)\s*(?:throws\s+\w+\s*)?\{{?", + rf"^{_ANNOTATION}\s*{_MODIFIERS}[\w<>\[\]]+\s+(\w+)\s*\([^)]*\)\s*(?:throws\s+\w+\s*)?\{{?", _re.IGNORECASE, ) annotation_re = _re.compile(r"@(\w+)", _re.IGNORECASE) @@ -105,9 +146,7 @@ def add_edge(src: str, tgt: str, relation: str, line: int, trig_nid = _make_id(stem, trig_name) add_node(trig_nid, trig_name, lineno) add_edge(file_nid, trig_nid, "contains", lineno) - sob_nid = _make_id(sobject) - if sob_nid not in seen_ids: - add_node(sob_nid, sobject, lineno) + sob_nid = type_ref(sobject) add_edge(trig_nid, sob_nid, "uses", lineno, confidence="INFERRED") current_class_nid = trig_nid pending_annotations = [] @@ -124,22 +163,14 @@ def add_edge(src: str, tgt: str, relation: str, line: int, add_edge(file_nid, class_nid, "contains", lineno) if cm.group(2): base = cm.group(2).strip() - base_nid = _make_id(stem, base) - if base_nid not in seen_ids: - base_nid = _make_id(base) - if base_nid not in seen_ids: - add_node(base_nid, base, lineno) - add_edge(class_nid, base_nid, "extends", lineno, confidence="INFERRED") + add_edge(class_nid, type_ref(base), "extends", lineno, + confidence="INFERRED") if cm.group(3): for iface in cm.group(3).split(","): iface = iface.strip() if iface: - iface_nid = _make_id(stem, iface) - if iface_nid not in seen_ids: - iface_nid = _make_id(iface) - if iface_nid not in seen_ids: - add_node(iface_nid, iface, lineno) - add_edge(class_nid, iface_nid, "implements", lineno, confidence="INFERRED") + add_edge(class_nid, type_ref(iface), "implements", + lineno, confidence="INFERRED") current_class_nid = class_nid pending_annotations = [] continue @@ -158,12 +189,8 @@ def add_edge(src: str, tgt: str, relation: str, line: int, for parent in im.group(2).split(","): parent = parent.strip() if parent: - parent_nid = _make_id(stem, parent) - if parent_nid not in seen_ids: - parent_nid = _make_id(parent) - if parent_nid not in seen_ids: - add_node(parent_nid, parent, lineno) - add_edge(iface_nid, parent_nid, "extends", lineno, confidence="INFERRED") + add_edge(iface_nid, type_ref(parent), "extends", + lineno, confidence="INFERRED") pending_annotations = [] continue @@ -198,9 +225,7 @@ def add_edge(src: str, tgt: str, relation: str, line: int, for sm in soql_re.finditer(line_text): sobject = sm.group(1) - sob_nid = _make_id(sobject) - if sob_nid not in seen_ids: - add_node(sob_nid, sobject, lineno) + sob_nid = type_ref(sobject) src = current_class_nid or file_nid add_edge(src, sob_nid, "uses", lineno, confidence="INFERRED") diff --git a/tests/fixtures/apex_modifiers.cls b/tests/fixtures/apex_modifiers.cls new file mode 100644 index 0000000000..3c4d916add --- /dev/null +++ b/tests/fixtures/apex_modifiers.cls @@ -0,0 +1,18 @@ +// Apex allows class modifiers in any order, and any number of them. Every +// declaration below must produce a node; `BaseBuilder`, `Runnable` and +// `Account` are only referenced here, so they must stay sourceless. +public abstract with sharing class ReportBuilder extends BaseBuilder implements Runnable { + + global without sharing virtual class Nested { + } + + public with sharing class Legacy { + } + + private static final class Constants { + } + + public override void build() { + List rows = [SELECT Id, Name FROM Account]; + } +} diff --git a/tests/test_apex_type_resolution.py b/tests/test_apex_type_resolution.py new file mode 100644 index 0000000000..0aed0891de --- /dev/null +++ b/tests/test_apex_type_resolution.py @@ -0,0 +1,70 @@ +from __future__ import annotations + +from pathlib import Path + +from graphify.extract import extract + + +def _write(path: Path, text: str) -> Path: + path.parent.mkdir(parents=True, exist_ok=True) + path.write_text(text, encoding="utf-8") + return path + + +def _node_by_id(result: dict, nid: str) -> dict | None: + return next((n for n in result["nodes"] if n.get("id") == nid), None) + + +def test_apex_cross_file_extends_resolves_to_real_def(tmp_path: Path): + base = _write( + tmp_path / "classes/DataLibrary.cls", + "public virtual with sharing class DataLibrary {\n" + " public void retrieve() {}\n" + "}\n", + ) + sub = _write( + tmp_path / "classes/MockDataLibrary.cls", + "@IsTest\npublic class MockDataLibrary extends DataLibrary {}\n", + ) + result = extract([base, sub], cache_root=tmp_path) + + extends = [e for e in result["edges"] if e["relation"] == "extends"] + assert extends, "expected an extends edge" + for e in extends: + tgt = _node_by_id(result, e["target"]) + assert tgt is not None, f"extends target {e['target']} is not a node" + assert Path(tgt["source_file"]).name == "DataLibrary.cls", ( + f"extends landed on {e['target']} instead of the real definition" + ) + + +def test_apex_one_base_referenced_twice_stays_one_node(tmp_path: Path): + # The base class must not fragment into a node per referencing file, which is + # what made "what depends on this class" come back empty. + base = _write(tmp_path / "classes/Base.cls", "public with sharing class Base {}\n") + one = _write(tmp_path / "classes/One.cls", "public class One extends Base {}\n") + two = _write(tmp_path / "classes/Two.cls", "public class Two extends Base {}\n") + result = extract([base, one, two], cache_root=tmp_path) + + bases = [n for n in result["nodes"] if n["label"] == "Base"] + assert len(bases) == 1, f"Base fragmented into {len(bases)} nodes" + targets = {e["target"] for e in result["edges"] if e["relation"] == "extends"} + assert targets == {bases[0]["id"]} + + +def test_apex_unknown_base_does_not_bind_to_an_unrelated_type(tmp_path: Path): + # Negative control: `Queueable` is defined nowhere in the corpus, so it must + # stay a sourceless leaf rather than absorb the same-named local variable type + # or any other node. + job = _write( + tmp_path / "classes/Job.cls", + "public with sharing class Job implements Queueable {}\n", + ) + other = _write(tmp_path / "classes/Other.cls", "public with sharing class Other {}\n") + result = extract([job, other], cache_root=tmp_path) + + impl = [e for e in result["edges"] if e["relation"] == "implements"] + assert len(impl) == 1 + tgt = _node_by_id(result, impl[0]["target"]) + assert tgt is not None and tgt["label"] == "Queueable" + assert not tgt["source_file"], "an absent type must not claim a definition file" diff --git a/tests/test_languages.py b/tests/test_languages.py index 6fd3a45c55..c7e61bbe2b 100644 --- a/tests/test_languages.py +++ b/tests/test_languages.py @@ -3246,6 +3246,53 @@ def test_apex_no_dangling_edges(): assert e["source"] in node_ids, f"dangling source in {fixture}: {e}" assert e["target"] in node_ids, f"dangling target in {fixture}: {e}" +def test_apex_modifiers_in_any_order_are_declarations(): + r = extract_apex(FIXTURES / "apex_modifiers.cls") + labels = _labels(r) + for name in ("ReportBuilder", "Nested", "Legacy", "Constants"): + assert name in labels, f"{name} produced no node" + +def test_apex_referenced_types_are_sourceless(tmp_path): + source = tmp_path / "Handler.cls" + source.write_text( + "public with sharing class Handler extends BaseHandler implements Queueable {\n" + " void run() {\n" + " List rows = [SELECT Id FROM Account];\n" + " }\n" + "}\n" + ) + r = extract_apex(source) + by_label = {n["label"]: n for n in r["nodes"]} + # Only referenced here, so they are placeholders for definitions elsewhere. + for name in ("BaseHandler", "Queueable", "Account"): + assert by_label[name]["source_file"] == "", f"{name} claims to be defined here" + # Actually declared here, so it keeps this file as its source. + assert by_label["Handler"]["source_file"].endswith("Handler.cls") + +def test_apex_referenced_type_is_not_contained_by_referencing_file(tmp_path): + """Negative control: a referenced type must NOT get a `contains` edge. + + A contained node reads as a definition owned by this file, which is what + made one class fragment into a node per referencing file.""" + source = tmp_path / "Handler.cls" + source.write_text("public class Handler extends BaseHandler {}\n") + r = extract_apex(source) + base = next(n for n in r["nodes"] if n["label"] == "BaseHandler") + contained = {e["target"] for e in r["edges"] if e["relation"] == "contains"} + assert base["id"] not in contained + +def test_apex_local_declaration_beats_stub(tmp_path): + """A base class declared in the SAME file binds to that declaration.""" + source = tmp_path / "Outer.cls" + source.write_text( + "public with sharing class Base {}\n" + "public with sharing class Outer extends Base {}\n" + ) + r = extract_apex(source) + base = [n for n in r["nodes"] if n["label"] == "Base"] + assert len(base) == 1 + assert base[0]["source_file"].endswith("Outer.cls") + # -- SystemVerilog -------------------------------------------------------------