diff --git a/graphify/extract.py b/graphify/extract.py index 9761a1d2..cb2fcc8e 100644 --- a/graphify/extract.py +++ b/graphify/extract.py @@ -1846,9 +1846,15 @@ def _get_cpp_func_name(node, source: bytes) -> str | None: if node.type in ("field_identifier", "destructor_name", "operator_name"): return _read_text(node, source) if node.type == "qualified_identifier": - name_node = node.child_by_field_name("name") - if name_node: - return _read_text(name_node, source) + # An out-of-class DEFINITION (`void Foo::bar() {}`) carries a + # qualified_identifier declarator. Retaining the `Foo::` qualifier makes + # _make_id(stem, "Foo::bar") normalize to the same id as the in-class + # member _make_id(class_nid, "bar"), so the decl in Foo.h and the def in + # Foo.cpp resolve to ONE method node instead of two (#1547). The full + # qualified text also handles nested scopes (`A::B::bar`). Free functions + # never have a qualified_identifier here, so their bare-name ids are + # unchanged; only qualified definitions shift onto their owning class. + return _read_text(node, source) decl = node.child_by_field_name("declarator") if decl: return _get_cpp_func_name(decl, source) @@ -9345,6 +9351,144 @@ def _resolve_cross_file_imports( return new_edges +# Header / implementation file-extension pairing for the decl/def class merge. +_DECLDEF_HEADER_SUFFIXES = frozenset({".h", ".hpp", ".hh", ".hxx"}) +_DECLDEF_IMPL_SUFFIXES = frozenset({".m", ".mm", ".cpp", ".cc", ".cxx", ".c"}) + + +def _decldef_class_stem(source_file: str) -> tuple[str, str] | None: + """Return ``(dir, base_stem)`` for a header/impl source file, else None. + + The base stem strips an ObjC category suffix (``Foo+Cat.m`` -> ``Foo``) so a + category implementation pairs with its ``Foo.h`` declaration. Files with an + extension that is neither a header nor an impl extension return None and are + never considered for the merge. + """ + if not source_file: + return None + p = Path(source_file) + suffix = p.suffix.lower() + if suffix not in _DECLDEF_HEADER_SUFFIXES and suffix not in _DECLDEF_IMPL_SUFFIXES: + return None + stem = p.stem.split("+", 1)[0] # ObjC category: Foo+Cat -> Foo + if not stem: + return None + return (str(p.parent), stem) + + +def _merge_decl_def_classes( + all_nodes: list[dict], + all_edges: list[dict], +) -> None: + """Merge a class (and its methods) declared in a header with its definition in + a sibling impl file into ONE node, for C/C++/ObjC (#1547, #1556). + + A class declared in ``Foo.h`` (``class Foo`` / ``@interface Foo``) and defined + in the sibling ``Foo.cpp`` / ``Foo.m`` (``@implementation Foo``, plus — after + the C++ qualified-name fix — out-of-class method definitions ``Foo::bar``) + produces TWO nodes per symbol. Both are keyed off the file *stem*, and + ``_file_stem`` drops the extension, so the header symbol and its impl + counterpart get the IDENTICAL id and differ only in ``source_file`` and label + (the C++ def label is ``Foo::bar()`` vs the decl's ``bar``; the ObjC impl class + label equals the interface's). Left alone, ``_disambiguate_colliding_node_ids`` + SPLITS those id-collisions apart by path, fragmenting one class into two def + nodes — which then trips every resolver's single-definition god-node guard + (``len(defs) != 1`` -> bail), cascading into lost .h<->.m/.cpp linkage and dead + cross-file calls. + + This pass runs BEFORE disambiguation and collapses each such id-collision to + ONE node — the header (declaration) variant, consistent with the #1475 + header_remaps direction — so disambiguation sees a single source_file per id + and leaves it alone, and the downstream resolvers see ONE definition. Because + the colliding nodes already share an id, no edge re-pointing is needed: every + edge that referenced the impl symbol already points at the surviving id. We + only drop the redundant duplicate node and prefer the header's label. + + GOD-NODE GUARDS (false merges are the main risk): + + * Collapse fires ONLY when every node in an id-collision group comes from a + SIBLING header/impl set — same directory, same base stem (ObjC categories + ``Foo+Cat.m`` compare by the stem before ``+``), header extension paired + with impl extension — AND the group contains exactly ONE header file. + * Two unrelated ``class Logger`` in DIFFERENT directories never collide on id + (the id embeds the full file stem / directory path), so they are never + grouped and never merge. Two same-named classes in the SAME directory but + different base stems likewise key to different ids. Any id-collision that + is NOT a clean single-header sibling set is left untouched for + disambiguation to split (the conservative default). + + The class and its method/field members fold in together: members are keyed + ``_make_id(class_id, name)`` (ObjC) or, for an out-of-class C++ definition, + ``_make_id(stem, "Foo::bar")`` which normalizes to the same id as the in-class + member ``_make_id(class_id, "bar")``. So every decl/def member pair is itself an + id-collision across the same sibling file set and collapses by the same rule. + """ + # Group every code node by id, recording the distinct source files involved. + by_id: dict[str, list[dict]] = {} + for n in all_nodes: + if n.get("file_type") != "code": + continue + nid = n.get("id") + sf = str(n.get("source_file", "")) + if not isinstance(nid, str) or not nid or not sf: + continue + by_id.setdefault(nid, []).append(n) + + # Identify, per surviving id, which node to keep (header preferred). We can't + # mutate all_nodes mid-scan, so collect a set of node object ids to drop. + drop_objs: set[int] = set() + for nid, group in by_id.items(): + if len(group) < 2: + continue + # The distinct source files of this collision must form a clean sibling + # header/impl set with exactly one header. Each file must parse as a + # header/impl file (others -> bail), share one directory + base stem. + sibling_keys: set[tuple[str, str]] = set() + headers: list[dict] = [] + ok = True + for node in group: + sf = str(node.get("source_file", "")) + ds = _decldef_class_stem(sf) + if ds is None: + ok = False + break + sibling_keys.add(ds) + if Path(sf).suffix.lower() in _DECLDEF_HEADER_SUFFIXES: + headers.append(node) + if not ok: + continue + # All from one (dir, base_stem) sibling family, with a UNIQUE header. + if len(sibling_keys) != 1 or len(headers) != 1: + continue + keeper = headers[0] + for node in group: + if node is not keeper: + drop_objs.add(id(node)) + + if not drop_objs: + return + + # Drop the redundant duplicate nodes. The surviving (header) node keeps its + # own label/source_file; edges are unchanged because the id is identical. Then + # de-dup any now-identical edges (e.g. the impl file's `contains`/`method` + # edge that duplicates the header's after the collapse). + all_nodes[:] = [n for n in all_nodes if id(n) not in drop_objs] + + seen_keys: set[tuple] = set() + rewritten: list[dict] = [] + for e in all_edges: + src = e.get("source") + tgt = e.get("target") + if src == tgt: + continue + k = (src, tgt, e.get("relation"), e.get("context")) + if k in seen_keys: + continue + seen_keys.add(k) + rewritten.append(e) + all_edges[:] = rewritten + + def _merge_swift_extensions( per_file: list[dict], all_nodes: list[dict], @@ -13486,7 +13630,14 @@ _DISPATCH: dict[str, Any] = { # belongs to extract_objc, not extract_c). `@property` is deliberately excluded: it # doubles as a Doxygen comment command and ObjC properties only ever live inside an # @interface/@protocol anyway, so the stronger directives already cover them. -_OBJC_HEADER_MARKERS = (b"@interface", b"@protocol", b"@implementation", b"@import") +# +# `#import` is included because an ObjC *bridging* header is often nothing but +# `#import "X.h"` lines with no @interface (#1556). Routed to extract_c it parses +# `#import` as a `preproc_call` (not `preproc_include`), so every import edge is +# dropped and the header is isolated. `#import` is an ObjC-only directive (illegal +# in C and C++), so this won't hijack genuine C/C++ headers, and extract_objc +# resolves quoted imports via _resolve_c_include_path. +_OBJC_HEADER_MARKERS = (b"@interface", b"@protocol", b"@implementation", b"@import", b"#import") def _is_objc_header(path: Path) -> bool: @@ -13504,6 +13655,35 @@ def _is_objc_header(path: Path) -> bool: return any(marker in head for marker in _OBJC_HEADER_MARKERS) +# C++-only signals. None of these are valid in a plain C header, so finding one +# in a `.h` is a high-confidence signal the header is C++ (#1547). The C grammar +# has no class_specifier, so a `class Foo { ... };` header routed to extract_c +# loses the class and its method prototypes (a junk `foo_foo` node + a sourceless +# `class` stub); routing to extract_cpp recovers the real type. Kept CONSERVATIVE: +# a plain C header with none of these stays on extract_c. ObjC sniffing keeps +# priority (an ObjC header can legitimately contain `::`/`class` inside an inline +# C++ block when compiled as Objective-C++). +_CPP_HEADER_MARKERS = ( + b"class ", b"namespace ", b"template", b"::", + b"public:", b"private:", b"protected:", +) + + +def _is_cpp_header(path: Path) -> bool: + """Whether a `.h` file is C++ rather than plain C (#1547). + + Mirrors `_is_objc_header`: sniffs for a C++-only token. Used only to reroute + a `.h` from extract_c to extract_cpp when no ObjC marker is present (ObjC has + priority). Conservative by construction — a plain C header matches nothing + here and keeps its existing extract_c routing. + """ + try: + head = path.read_bytes()[:256 * 1024] + except OSError: + return False + return any(marker in head for marker in _CPP_HEADER_MARKERS) + + def _get_extractor(path: Path) -> Any | None: """Return the correct extractor function for a file, or None if unsupported.""" if path.name.endswith(".blade.php"): @@ -13520,8 +13700,15 @@ def _get_extractor(path: Path) -> Any | None: return extract_package_manifest # `.h` is C/C++/ObjC-ambiguous; route Objective-C headers to extract_objc # (the suffix map sends `.h` to extract_c, which can't read @interface etc.). - if path.suffix == ".h" and _is_objc_header(path): - return extract_objc + # ObjC sniffing has priority over the C++ sniff: an Objective-C++ header can + # contain both `@interface` and inline C++ (`::`), and it must parse as ObjC. + if path.suffix == ".h": + if _is_objc_header(path): + return extract_objc + # A C++ class header routed to extract_c loses the class entirely (the C + # grammar has no class_specifier). Reroute to extract_cpp (#1547). + if _is_cpp_header(path): + return extract_cpp return _DISPATCH.get(path.suffix) @@ -13794,6 +13981,14 @@ def extract( _augment_symbol_resolution_edges(paths, all_nodes, all_edges, root) + # Merge a header-declared class (and its methods) with its sibling-impl + # definition into ONE node (C/C++/ObjC #1547/#1556). Runs BEFORE the id-remap + # below: a header symbol and its impl counterpart share an id only while both + # still carry the raw file-stem prefix; the per-file prefix remap then diverges + # them (foo_h vs foo_cpp), so the collapse must happen first. Collapsing here + # also means disambiguation sees one source_file per id and won't split them. + _merge_decl_def_classes(all_nodes, all_edges) + # Remap file node IDs from absolute-path-derived to the canonical # {parent_dir}_{stem} spec form so (a) graph.json edge endpoints are stable # across machines (#502) and (b) AST file nodes match the IDs semantic diff --git a/tests/fixtures/cpp_logger/a/Logger.cpp b/tests/fixtures/cpp_logger/a/Logger.cpp new file mode 100644 index 00000000..ee35493e --- /dev/null +++ b/tests/fixtures/cpp_logger/a/Logger.cpp @@ -0,0 +1,3 @@ +#include "Logger.h" + +void Logger::log() {} diff --git a/tests/fixtures/cpp_logger/a/Logger.h b/tests/fixtures/cpp_logger/a/Logger.h new file mode 100644 index 00000000..d858d800 --- /dev/null +++ b/tests/fixtures/cpp_logger/a/Logger.h @@ -0,0 +1,4 @@ +class Logger { +public: + void log(); +}; diff --git a/tests/fixtures/cpp_logger/b/Logger.cpp b/tests/fixtures/cpp_logger/b/Logger.cpp new file mode 100644 index 00000000..ee35493e --- /dev/null +++ b/tests/fixtures/cpp_logger/b/Logger.cpp @@ -0,0 +1,3 @@ +#include "Logger.h" + +void Logger::log() {} diff --git a/tests/fixtures/cpp_logger/b/Logger.h b/tests/fixtures/cpp_logger/b/Logger.h new file mode 100644 index 00000000..d858d800 --- /dev/null +++ b/tests/fixtures/cpp_logger/b/Logger.h @@ -0,0 +1,4 @@ +class Logger { +public: + void log(); +}; diff --git a/tests/fixtures/cpp_paired/Foo.cpp b/tests/fixtures/cpp_paired/Foo.cpp new file mode 100644 index 00000000..0fc283e0 --- /dev/null +++ b/tests/fixtures/cpp_paired/Foo.cpp @@ -0,0 +1,5 @@ +#include "Foo.h" + +void Foo::bar() { + value = 1; +} diff --git a/tests/fixtures/cpp_paired/Foo.h b/tests/fixtures/cpp_paired/Foo.h new file mode 100644 index 00000000..60998e93 --- /dev/null +++ b/tests/fixtures/cpp_paired/Foo.h @@ -0,0 +1,10 @@ +#ifndef FOO_H +#define FOO_H + +class Foo { +public: + void bar(); + int value; +}; + +#endif diff --git a/tests/fixtures/cpp_paired/Main.cpp b/tests/fixtures/cpp_paired/Main.cpp new file mode 100644 index 00000000..265c1936 --- /dev/null +++ b/tests/fixtures/cpp_paired/Main.cpp @@ -0,0 +1,7 @@ +#include "Foo.h" + +int main() { + Foo f; + f.bar(); + return 0; +} diff --git a/tests/fixtures/cpp_samedir/Alpha.h b/tests/fixtures/cpp_samedir/Alpha.h new file mode 100644 index 00000000..c9875916 --- /dev/null +++ b/tests/fixtures/cpp_samedir/Alpha.h @@ -0,0 +1,4 @@ +class Dup { +public: + void a(); +}; diff --git a/tests/fixtures/cpp_samedir/Beta.h b/tests/fixtures/cpp_samedir/Beta.h new file mode 100644 index 00000000..7ba422cd --- /dev/null +++ b/tests/fixtures/cpp_samedir/Beta.h @@ -0,0 +1,4 @@ +class Dup { +public: + void b(); +}; diff --git a/tests/fixtures/cpp_samedir/plain.h b/tests/fixtures/cpp_samedir/plain.h new file mode 100644 index 00000000..ef951746 --- /dev/null +++ b/tests/fixtures/cpp_samedir/plain.h @@ -0,0 +1,7 @@ +#ifndef PLAIN_H +#define PLAIN_H + +int add(int a, int b); +struct Point { int x; int y; }; + +#endif diff --git a/tests/fixtures/objc_mixed/Bridging-Header.h b/tests/fixtures/objc_mixed/Bridging-Header.h new file mode 100644 index 00000000..a46f910e --- /dev/null +++ b/tests/fixtures/objc_mixed/Bridging-Header.h @@ -0,0 +1 @@ +#import "Widget.h" diff --git a/tests/fixtures/objc_mixed/Widget.h b/tests/fixtures/objc_mixed/Widget.h new file mode 100644 index 00000000..6544b015 --- /dev/null +++ b/tests/fixtures/objc_mixed/Widget.h @@ -0,0 +1,4 @@ +@interface Widget +- (void)render; +- (void)refresh; +@end diff --git a/tests/fixtures/objc_mixed/Widget.m b/tests/fixtures/objc_mixed/Widget.m new file mode 100644 index 00000000..fe4fff3b --- /dev/null +++ b/tests/fixtures/objc_mixed/Widget.m @@ -0,0 +1,9 @@ +#import "Widget.h" + +@implementation Widget +- (void)render { + [self refresh]; +} +- (void)refresh { +} +@end diff --git a/tests/fixtures/objc_mixed/WidgetExtras.swift b/tests/fixtures/objc_mixed/WidgetExtras.swift new file mode 100644 index 00000000..39501b81 --- /dev/null +++ b/tests/fixtures/objc_mixed/WidgetExtras.swift @@ -0,0 +1,5 @@ +extension Widget { + func describe() -> String { + return "widget" + } +} diff --git a/tests/test_languages.py b/tests/test_languages.py index fe000d8e..3393ab16 100644 --- a/tests/test_languages.py +++ b/tests/test_languages.py @@ -2582,3 +2582,168 @@ def test_systemverilog_no_dangling_edges(): for e in r["edges"]: assert e["source"] in node_ids, f"dangling source: {e}" assert e["target"] in node_ids, f"dangling target: {e}" + + +# ── Header/impl class merge + .h routing (#1547 C++, #1556 ObjC/Swift) ───────── +from graphify.extract import ( + extract as _extract_corpus, + _get_extractor, + _is_cpp_header, + _is_objc_header, +) + + +def _corpus(*relpaths): + """Run the full extract() pipeline on fixture files (absolute, resolved + paths so the per-file id-remap behaves like real usage), no shared cache.""" + import tempfile + paths = [(FIXTURES / rp).resolve() for rp in relpaths] + with tempfile.TemporaryDirectory() as td: + return _extract_corpus(paths, cache_root=Path(td)) + + +def _nodes_with_label(r, label): + return [n for n in r["nodes"] if n["label"] == label] + + +def _assert_no_dangling(r): + ids = {n["id"] for n in r["nodes"]} + for e in r["edges"]: + assert e["source"] in ids, f"dangling source: {e}" + assert e["target"] in ids, f"dangling target: {e}" + + +# --- #1547: C++ paired header/impl -------------------------------------------- + +def test_cpp_header_routes_to_cpp_extractor(): + """A `.h` with a C++ class must route to extract_cpp, not extract_c (which has + no class_specifier and would drop the class entirely).""" + p = (FIXTURES / "cpp_paired" / "Foo.h").resolve() + assert _get_extractor(p).__name__ == "extract_cpp" + assert _is_cpp_header(p) + + +def test_plain_c_header_stays_on_c_extractor(): + """A plain C header (no C++ signal) must keep its extract_c routing.""" + p = (FIXTURES / "cpp_samedir" / "plain.h").resolve() + assert not _is_cpp_header(p) + assert _get_extractor(p).__name__ == "extract_c" + + +def test_cpp_paired_single_class_node(): + """Foo.h (class) + Foo.cpp (Foo::bar def) + Main.cpp must yield exactly ONE + Foo class node — not a foo_h + foo_cpp pair, and no junk `class` stub.""" + r = _corpus("cpp_paired/Foo.h", "cpp_paired/Foo.cpp", "cpp_paired/Main.cpp") + foos = _nodes_with_label(r, "Foo") + assert len(foos) == 1, f"expected one Foo, got {[n['id'] for n in foos]}" + assert not _nodes_with_label(r, "class"), "no sourceless `class` stub should exist" + assert not _nodes_with_label(r, "foo_foo") + + +def test_cpp_paired_method_decl_and_def_are_one_node(): + """`void bar();` in Foo.h and `void Foo::bar() {}` in Foo.cpp must collapse to + ONE method node owned by the single Foo class.""" + r = _corpus("cpp_paired/Foo.h", "cpp_paired/Foo.cpp", "cpp_paired/Main.cpp") + foo = _nodes_with_label(r, "Foo")[0]["id"] + method_targets = { + e["target"] for e in r["edges"] + if e["source"] == foo and e["relation"] in ("method", "defines", "contains") + } + bar_nodes = [n for n in r["nodes"] if n["id"] in method_targets and n["label"] in ("bar", "Foo::bar()")] + # There must be exactly one node representing bar (decl and def merged). + bar_ids = {n["id"] for n in r["nodes"] if n["label"] in ("bar", "Foo::bar()")} + assert len(bar_ids) == 1, f"bar decl/def should be one node, got {bar_ids}" + assert bar_nodes, "the merged bar node should be a member of Foo" + + +def test_cpp_paired_includes_resolve_to_real_header(): + """Foo.cpp and Main.cpp `#include "Foo.h"` must resolve to the real Foo.h file + node (no dangling import).""" + r = _corpus("cpp_paired/Foo.h", "cpp_paired/Foo.cpp", "cpp_paired/Main.cpp") + ids = {n["id"] for n in r["nodes"]} + foo_h = _nodes_with_label(r, "Foo.h")[0]["id"] + imports = [e for e in r["edges"] if e["relation"] == "imports"] + assert len(imports) >= 2 + for e in imports: + assert e["target"] in ids, f"dangling import target: {e}" + assert any(e["target"] == foo_h for e in imports), "includes should target Foo.h" + + +def test_cpp_paired_no_dangling_edges(): + r = _corpus("cpp_paired/Foo.h", "cpp_paired/Foo.cpp", "cpp_paired/Main.cpp") + _assert_no_dangling(r) + + +# --- #1556: ObjC paired header/impl + bridging header ------------------------- + +def test_objc_header_with_import_routes_to_objc(): + """A bridging header that is only `#import "X.h"` (no @interface) must route to + extract_objc; extract_c parses `#import` as preproc_call and drops the edge.""" + p = (FIXTURES / "objc_mixed" / "Bridging-Header.h").resolve() + assert _is_objc_header(p) + assert _get_extractor(p).__name__ == "extract_objc" + + +def test_objc_paired_single_class_methods_not_duplicated(): + """Widget.h (@interface) + Widget.m (@implementation) -> ONE Widget class node + with its methods present once each.""" + r = _corpus("objc_mixed/Widget.h", "objc_mixed/Widget.m") + widgets = _nodes_with_label(r, "Widget") + assert len(widgets) == 1, f"expected one Widget, got {[n['id'] for n in widgets]}" + render = _nodes_with_label(r, "-render") + refresh = _nodes_with_label(r, "-refresh") + assert len(render) == 1, f"-render duplicated: {render}" + assert len(refresh) == 1, f"-refresh duplicated: {refresh}" + + +def test_objc_bridging_header_not_isolated(): + """A bridging header of only `#import "Widget.h"` must produce an imports edge + to the real Widget.h node (not be an isolated node).""" + r = _corpus("objc_mixed/Widget.h", "objc_mixed/Widget.m", "objc_mixed/Bridging-Header.h") + bridge = _nodes_with_label(r, "Bridging-Header.h")[0]["id"] + widget_h = _nodes_with_label(r, "Widget.h")[0]["id"] + out = [e for e in r["edges"] if e["source"] == bridge and e["relation"] == "imports"] + assert out, "bridging header should emit an imports edge" + assert any(e["target"] == widget_h for e in out), "bridging import should target Widget.h" + + +def test_objc_paired_no_dangling_edges(): + r = _corpus("objc_mixed/Widget.h", "objc_mixed/Widget.m", "objc_mixed/Bridging-Header.h") + _assert_no_dangling(r) + + +# --- #1556: Swift extension folds onto canonical ObjC class ------------------- + +def test_swift_extension_folds_onto_objc_class(): + """`extension Widget` in Swift over an ObjC `Widget` must fold onto the single + canonical Widget node, with its members anchored there.""" + r = _corpus("objc_mixed/Widget.h", "objc_mixed/Widget.m", "objc_mixed/WidgetExtras.swift") + widgets = _nodes_with_label(r, "Widget") + assert len(widgets) == 1, f"expected one Widget, got {[n['id'] for n in widgets]}" + wid = widgets[0]["id"] + method_targets = {e["target"] for e in r["edges"] if e["relation"] == "method" and e["source"] == wid} + labels = {n["label"] for n in r["nodes"] if n["id"] in method_targets} + assert any("describe" in l for l in labels), f"Swift extension method should anchor on Widget, got {labels}" + _assert_no_dangling(r) + + +# --- god-node guard negatives ------------------------------------------------- + +def test_decldef_merge_does_not_merge_across_directories(): + """Two unrelated `class Logger` in DIFFERENT directories (each its own .h/.cpp) + must NOT merge — assert TWO distinct Logger nodes.""" + r = _corpus( + "cpp_logger/a/Logger.h", "cpp_logger/a/Logger.cpp", + "cpp_logger/b/Logger.h", "cpp_logger/b/Logger.cpp", + ) + loggers = _nodes_with_label(r, "Logger") + assert len(loggers) == 2, f"cross-dir Loggers must stay distinct, got {[n['id'] for n in loggers]}" + assert len({n["id"] for n in loggers}) == 2 + + +def test_decldef_merge_does_not_merge_same_name_same_dir_distinct_files(): + """Two same-named `class Dup` in the SAME dir but different base stems + (Alpha.h, Beta.h) must stay distinct (no unique header/impl sibling pair).""" + r = _corpus("cpp_samedir/Alpha.h", "cpp_samedir/Beta.h") + dups = _nodes_with_label(r, "Dup") + assert len(dups) == 2, f"same-dir distinct Dups must stay distinct, got {[n['id'] for n in dups]}"