Skip to content

Commit 7f1bca3

Browse files
committed
fix(go): make the two backends agree on a colliding signature and a stand-in
Two bugs found by review against the live graph; both were backend disagreements invisible from either side alone. A Go signature is not always a unique address -- several files in one package may each declare their own init(), so one signature names two callables with different bodies (3 such collisions on the analyzer's own corpus). The local backend keyed its candidate list by signature, collapsing them into one entry and hiding the ambiguity from the shared policy: it raised a bare ValueError where the graph raised AmbiguousName, and in_module= naming a file narrowed on neither. Both now hand the policy one candidate per declaration, and ids.pick_declaration re-applies the caller's keywords to choose among the declarations behind the resolved signature -- which the Neo4j side previously discarded by re-looking-up on the bare signature and re-colliding. get_cfg/get_cdg/get_ddg on a library stand-in raised AttributeError locally (GoExternalSymbol has no cfg field) while the graph returned a clean empty page. Empty is the honest answer: a stand-in has no statements, only formal_in/formal_out ports. That is distinct from the level gate's D7 case, where empty would be indistinguishable from "no dependence" because the pass never ran; both the ABC docstring and the tests now say so. Verified on the live graph: 11 of 11 probes that differed now agree, and the 66-check parity suite still passes. Refs #436
1 parent 2dee1a0 commit 7f1bca3

6 files changed

Lines changed: 247 additions & 43 deletions

File tree

‎cldk/analysis/go/backend.py‎

Lines changed: 21 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -235,9 +235,19 @@ def resolve_callable(self, name: str, *, in_class: str | None = None, in_module:
235235
in_module: Disambiguate by module — the repo-relative ``.go`` path, or the package's
236236
dotted name.
237237
238+
**A Go signature is not always a unique address.** Several files in one package may each
239+
declare their own ``init()``, so one signature can name several *different* callables with
240+
different bodies and different dataflow (3 such collisions on the analyzer's own corpus).
241+
Both backends hand the shared policy one candidate **per declaration**, so the ambiguity is
242+
reported rather than silently resolved to whichever won, and ``in_module=`` naming one of
243+
the declaring paths is what picks one.
244+
238245
Raises:
239246
AmbiguousName: More than one callable matched; the candidates are carried.
240247
SelectorNotInGraph: Nothing matched, or a keyword excluded every match.
248+
ValueError: The resolved signature is declared in several files and the keywords given
249+
do not single one out. The message names the paths, because passing one of them is
250+
the way out.
241251
"""
242252

243253
@abstractmethod
@@ -292,11 +302,20 @@ def get_cfg(self, callable: str, *, in_class: str | None = None, page_size: int
292302
Endpoints are the body nodes' own ``can://`` ids, the spelling :meth:`get_source`
293303
accepts.
294304
305+
**An empty page for a library stand-in is an answer, not a gap.** ``callable`` may
306+
resolve to one (``database/sql.DB.Query``), and a stand-in has no statements — the
307+
analyzer synthesizes only its ``formal_in``/``formal_out`` ports — so all three graphs
308+
are legitimately empty there. That is distinct from the level gate below, where empty
309+
*would* be indistinguishable from "no dependence" because the pass never ran; here the
310+
pass ran and the absence is a fact about what a stand-in is. Its ports are still real
311+
vertices, so the slices and :meth:`taint` do cross it.
312+
295313
Raises:
296314
AmbiguousName: ``callable`` named more than one callable.
297315
SelectorNotInGraph: Nothing matched.
298-
ValueError: ``page_size`` below 1, or ``cursor`` not from a previous page of this
299-
accessor and this callable.
316+
ValueError: ``page_size`` below 1, ``cursor`` not from a previous page of this accessor
317+
and this callable, or the signature is declared in several files (see
318+
:meth:`resolve_callable`).
300319
CodeanalyzerUsageException: (local backend) built below ``-a 3``.
301320
"""
302321

‎cldk/analysis/go/codeanalyzer/codeanalyzer.py‎

Lines changed: 54 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -68,7 +68,7 @@
6868
VIA,
6969
GoAnalysisBackend,
7070
)
71-
from cldk.analysis.go.ids import body_node_id, callable_match_names, external_signature, go_value_names
71+
from cldk.analysis.go.ids import body_node_id, callable_match_names, external_signature, go_value_names, pick_declaration
7272
from cldk.models.go import (
7373
GoAnalysis,
7474
GoApplication,
@@ -526,7 +526,7 @@ def _slice_node(self, ref: str) -> SliceNode | None:
526526

527527
# -----[ addressing ]-----
528528
@cached_property
529-
def _candidates(self) -> Tuple[List[CallableCandidate], Dict[str, Tuple[object, str | None, str]], FrozenSet[str]]:
529+
def _candidates(self) -> Tuple[List[CallableCandidate], Dict[str, List[Tuple[object, str | None, str]]]]:
530530
"""``(candidates, {signature: owner}, colliding signatures)`` for :meth:`resolve_callable`.
531531
532532
**The library stand-ins are in the domain**, and they have to be: spec §7's own worked
@@ -542,28 +542,33 @@ def _candidates(self) -> Tuple[List[CallableCandidate], Dict[str, Tuple[object,
542542
than matching one through a path derivation that would be meaningless here (the same
543543
distinction Java's J-2 records).
544544
545-
Two callables sharing a signature collapse into one entry, and the resolver would then hand
546-
back whichever won with no signal — so the collisions are recorded and a name resolving onto
547-
one raises. Checked on the *answer* rather than eagerly, so a duplicate elsewhere does not
548-
break every unrelated resolution.
545+
**One candidate per declaration, not per signature.** A Go signature is not a unique
546+
address: several files in one package may each declare their own ``init()``, so
547+
``example.com/x/initclosure.init`` names two different callables with different bodies (3
548+
such collisions on the analyzer's own corpus). Keying the candidate list by signature
549+
collapsed those into one entry, which hid the ambiguity from the shared policy — so it
550+
could neither report it (the Neo4j backend, whose rows are per declaration, raised
551+
``AmbiguousName`` where this raised a bare ``ValueError``) nor let ``in_module=`` narrow it.
552+
Emitting one candidate per declaration is what makes the two backends hand the policy the
553+
same domain, which is the only way they can agree on both the answer and the failure.
554+
555+
``declarations`` therefore maps a signature to *every* callable carrying it, and
556+
:func:`~cldk.analysis.go.ids.pick_declaration` re-applies the caller's keywords to choose
557+
among them once the policy has resolved the signature.
549558
"""
550-
found: Dict[str, Tuple[object, str | None, str]] = {}
551-
collisions: set = set()
559+
declarations: Dict[str, List[Tuple[object, str | None, str]]] = {}
552560
for c, tid, path in self._iter_callables():
553-
if c.signature in found:
554-
collisions.add(c.signature)
555-
found[c.signature] = (c, tid, path)
561+
declarations.setdefault(c.signature, []).append((c, tid, path))
556562
cands = [
557563
CallableCandidate(sig, tid, path, match_names=callable_match_names(sig), module_names=(pkg,) if (pkg := self._package_of(path)) else ())
558-
for sig, (_c, tid, path) in found.items()
564+
for sig, entries in declarations.items()
565+
for _c, tid, path in entries
559566
]
560567
for sym in self.application.external_symbols.values():
561568
sig = external_signature(sym.module, sym.signature, sym.name)
562-
if sig in found:
563-
collisions.add(sig)
564-
found[sig] = (sym, None, "")
569+
declarations.setdefault(sig, []).append((sym, None, ""))
565570
cands.append(CallableCandidate(sig, None, "", match_names=callable_match_names(sig), module_names=()))
566-
return cands, found, frozenset(collisions)
571+
return cands, declarations
567572

568573
def _package_of(self, path: str) -> str | None:
569574
"""The ``package`` clause of the module at ``path`` — what ``in_module=`` matches a Go
@@ -585,11 +590,14 @@ def resolve_callable(self, name: str, *, in_class: str | None = None, in_module:
585590
"both backends resolve over the same set". The Go receiver spellings come from
586591
:func:`~cldk.analysis.go.ids.callable_match_names`, which is shared with the Neo4j backend.
587592
"""
588-
cands, found, collisions = self._candidates
593+
cands, declarations = self._candidates
589594
sig = resolve_callable_signature(name, cands, in_class=in_class, in_module=in_module, by_full_name="more of the import path")
590-
if sig in collisions:
591-
raise ValueError(f"{sig!r} is carried by more than one analysed callable; neither can be addressed unambiguously")
592-
c, _tid, path = found[sig]
595+
entries = declarations[sig]
596+
# A signature can name several declarations (same-package multi-file ``init()``), so the
597+
# keywords are re-applied to choose among them -- see ``pick_declaration`` for why the
598+
# policy's own answer is one step short of an address here.
599+
chosen = pick_declaration(sig, [(path, tid) for _c, tid, path in entries], in_class=in_class, in_module=in_module)
600+
c, _tid, path = next((obj, tid, p) for obj, tid, p in entries if p == chosen)
593601
if not c.id:
594602
raise KeyError(f"{sig!r} carries no usable address (id={c.id!r})")
595603
# A stand-in has no file and no span, so it is described as an external rather than given a
@@ -629,17 +637,31 @@ def resolve_value(self, name: str, *, within: str) -> SliceNode:
629637
return SliceNode(file=owner.file, line=owner.line, callable=owner.callable, kind=kind, name=leaf, defined_in=defined_in, source=None, ref=body_node_id(c.id, key))
630638

631639
# -----[ per-callable graphs ]-----
632-
def _graphs_of(self, name: str, in_class: str | None, page_size: int) -> GoCallable:
633-
"""The callable ``name`` resolves to, once this backend is deep enough to have dataflow.
640+
def _graphs_of(self, name: str, in_class: str | None, page_size: int) -> Tuple[str, GoCallable | None]:
641+
"""``(signature, callable)`` for ``name``, once this backend is deep enough to have dataflow.
634642
635643
``page_size`` is validated **first**, before the level guard and before resolution, so a
636644
malformed argument is a ``ValueError`` before anything else — the order the Neo4j backend
637645
applies too, so the two cannot answer the same bad call with different exceptions.
646+
647+
**The callable is ``None`` for a library stand-in**, which is a real resolution target here
648+
(spec §7's own example, ``database/sql.DB.Query``, is one) and has no intraprocedural flow
649+
to report: the analyzer synthesizes only ``formal_in``/``formal_out`` ports for it, so
650+
:class:`~cldk.models.go.GoExternalSymbol` carries no ``cfg``/``cdg``/``ddg`` field at all.
651+
Returning ``None`` is what lets the three accessors answer with an **empty page**, which is
652+
both the honest answer — the pass ran and a stand-in has no statements — and the answer the
653+
Neo4j backend already gives, since its Cypher simply matches no rows on an external node.
654+
Reading the field blindly raised ``AttributeError`` instead, which was neither.
655+
656+
This is **not** the D7 case the level gate guards: there, empty would be indistinguishable
657+
from "no dependence" because the pass never ran. Here the pass ran and the absence is a
658+
fact about what a stand-in *is*.
638659
"""
639660
check_page_size(page_size)
640661
self._require_dataflow()
641662
node = self.resolve_callable(name, in_class=in_class)
642-
return self._dataflow_owners[node.ref][0]
663+
owner = self._dataflow_owners[node.ref][0]
664+
return node.callable, owner if isinstance(owner, GoCallable) else None
643665

644666
def get_cfg(self, callable: str, *, in_class: str | None = None, page_size: int = DEFAULT_PAGE_SIZE, cursor: str | None = None) -> EdgePage[GoCfgEdge]:
645667
"""One page of control flow within one callable (see :meth:`GoAnalysisBackend.get_cfg`).
@@ -650,21 +672,21 @@ def get_cfg(self, callable: str, *, in_class: str | None = None, page_size: int
650672
the one the Neo4j backend would return. The join happens *before* the sort, because the
651673
order is over the ids a caller sees, not over the local keys.
652674
"""
653-
c = self._graphs_of(callable, in_class, page_size)
654-
edges = [GoCfgEdge(src=body_node_id(c.id, e.src), dst=body_node_id(c.id, e.dst), kind=e.kind) for e in c.cfg or []]
655-
return edge_page(GoCfgEdge, c.signature, edges, CFG_ORDER, page_size, cursor)
675+
sig, c = self._graphs_of(callable, in_class, page_size)
676+
edges = [] if c is None else [GoCfgEdge(src=body_node_id(c.id, e.src), dst=body_node_id(c.id, e.dst), kind=e.kind) for e in c.cfg or []]
677+
return edge_page(GoCfgEdge, sig, edges, CFG_ORDER, page_size, cursor)
656678

657679
def get_cdg(self, callable: str, *, in_class: str | None = None, page_size: int = DEFAULT_PAGE_SIZE, cursor: str | None = None) -> EdgePage[GoCdgEdge]:
658680
"""One page of control dependence within one callable (see :meth:`GoAnalysisBackend.get_cdg`)."""
659-
c = self._graphs_of(callable, in_class, page_size)
660-
edges = [GoCdgEdge(src=body_node_id(c.id, e.src), dst=body_node_id(c.id, e.dst)) for e in c.cdg or []]
661-
return edge_page(GoCdgEdge, c.signature, edges, CDG_ORDER, page_size, cursor)
681+
sig, c = self._graphs_of(callable, in_class, page_size)
682+
edges = [] if c is None else [GoCdgEdge(src=body_node_id(c.id, e.src), dst=body_node_id(c.id, e.dst)) for e in c.cdg or []]
683+
return edge_page(GoCdgEdge, sig, edges, CDG_ORDER, page_size, cursor)
662684

663685
def get_ddg(self, callable: str, *, in_class: str | None = None, page_size: int = DEFAULT_PAGE_SIZE, cursor: str | None = None) -> EdgePage[GoDdgEdge]:
664686
"""One page of data dependence within one callable (see :meth:`GoAnalysisBackend.get_ddg`)."""
665-
c = self._graphs_of(callable, in_class, page_size)
666-
edges = [GoDdgEdge(src=body_node_id(c.id, e.src), dst=body_node_id(c.id, e.dst), var=e.var, prov=list(e.prov or [])) for e in c.ddg or []]
667-
return edge_page(GoDdgEdge, c.signature, edges, DDG_ORDER, page_size, cursor)
687+
sig, c = self._graphs_of(callable, in_class, page_size)
688+
edges = [] if c is None else [GoDdgEdge(src=body_node_id(c.id, e.src), dst=body_node_id(c.id, e.dst), var=e.var, prov=list(e.prov or [])) for e in c.ddg or []]
689+
return edge_page(GoDdgEdge, sig, edges, DDG_ORDER, page_size, cursor)
668690

669691
# -----[ slicing and reachability ]-----
670692
def _reach(self, ref: str, direction: str, depth: int | None) -> set:

‎cldk/analysis/go/ids.py‎

Lines changed: 48 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,7 @@
2828

2929
from typing import Iterable, List, Sequence, Tuple
3030

31-
from cldk.analysis.commons.resolve import value_candidate
31+
from cldk.analysis.commons.resolve import segment_match, value_candidate
3232

3333
#: The marker the analyzer puts in a library stand-in's **callable** id, before any vertex ``@``:
3434
#: ``can://<ns>/<app>/@external/bytes/Buffer.Read``. Its leading ``@`` is why
@@ -196,6 +196,53 @@ def external_signature(module: str, signature: str, name: str) -> str:
196196
return f"{module}.{signature or name}"
197197

198198

199+
def pick_declaration(sig: str, declarations: Sequence[Tuple[str, "str | None"]], *, in_class: "str | None", in_module: "str | None") -> str:
200+
"""The one declaration path of ``sig``, re-narrowed by the keywords that resolved it.
201+
202+
**Why this exists.** Go lets several files in one package each declare their own ``init()``, so
203+
one signature can name several *different* callables with different bodies and different
204+
dataflow — ``example.com/x/initclosure.init`` is declared in both ``a.go`` and ``b.go`` on the
205+
analyzer's own corpus (3 such collisions there). A signature is therefore not a unique address
206+
in Go the way it is in Python, and both backends have to agree on that.
207+
208+
:func:`~cldk.analysis.commons.resolve.resolve_callable_signature` returns a *signature string*,
209+
which is one step short of an address when the signature collides: the keyword that narrowed the
210+
candidate set is not re-applied to the declarations behind it, so a caller who wrote
211+
``in_module="a.go"`` and got past the policy would still face two declarations. This re-applies
212+
exactly the policy's own predicates — :func:`segment_match` on the owning type for ``in_class``,
213+
and on the repo-relative path with ``/`` for ``in_module`` — so a keyword that disambiguated the
214+
candidates disambiguates the declaration too.
215+
216+
Args:
217+
sig: The signature the policy resolved to.
218+
declarations: ``(path, owning type id)`` for every callable carrying ``sig``.
219+
in_class / in_module: The keywords as the caller passed them.
220+
221+
Returns:
222+
The single matching declaration's path.
223+
224+
Raises:
225+
ValueError: ``sig`` is carried by more than one declaration and the keywords given do not
226+
single one out. The message names the paths, because the way out is to pass one of
227+
them — a caller cannot act on "it is ambiguous" alone.
228+
"""
229+
if len(declarations) == 1:
230+
return declarations[0][0]
231+
kept = list(declarations)
232+
if in_class is not None:
233+
kept = [(path, tid) for path, tid in kept if tid and segment_match(in_class, tid)]
234+
if in_module is not None:
235+
kept = [(path, tid) for path, tid in kept if segment_match(in_module, path, sep="/")]
236+
if len(kept) == 1:
237+
return kept[0][0]
238+
paths = sorted(path for path, _tid in (kept or declarations))
239+
raise ValueError(
240+
f"{sig!r} is declared in {len(paths)} files ({', '.join(paths)}) — Go allows several files in one "
241+
f"package to declare the same signature, so it is not a unique address here. Narrow it with "
242+
f"in_module= naming one of those paths."
243+
)
244+
245+
199246
def value_display(var: str) -> Tuple[str, str, "str | None"]:
200247
"""``(kind, leaf, defined_in)`` for a ``formal_in`` vertex's ``var``, in the caller's vocabulary.
201248

‎cldk/analysis/go/neo4j/neo4j_backend.py‎

Lines changed: 15 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -74,7 +74,7 @@
7474
VIA,
7575
GoAnalysisBackend,
7676
)
77-
from cldk.analysis.go.ids import callable_match_names, external_signature, go_value_names
77+
from cldk.analysis.go.ids import callable_match_names, external_signature, go_value_names, pick_declaration
7878
from cldk.analysis.go.neo4j import reconstruct as R
7979
from cldk.models.go import (
8080
GoApplication,
@@ -600,30 +600,37 @@ def resolve_callable(self, name: str, *, in_class: str | None = None, in_module:
600600
The shared policy runs over the same unfiltered domain the local backend hands it, which is
601601
the strongest form of "both backends resolve over the same set"; the Go receiver spellings
602602
come from the shared :func:`~cldk.analysis.go.ids.callable_match_names`.
603+
604+
A signature that names several declarations (same-package multi-file ``init()``) is
605+
narrowed by :func:`~cldk.analysis.go.ids.pick_declaration`, the same function and the same
606+
keywords the local backend applies. Re-looking-up on the bare signature and refusing on
607+
``len(rows) > 1`` — what this did before — threw away the narrowing the policy had just
608+
done, so ``in_module=`` could not disambiguate even when it had matched.
603609
"""
604610
sig = resolve_callable_signature(name, self._callable_candidates, in_class=in_class, in_module=in_module, by_full_name="more of the import path")
605611
if sig in self._external_signatures:
606612
ref = self._external_signatures[sig]
607613
return SliceNode(file="", line=0, callable=sig, kind="external", name=sig.rpartition(".")[2], source=None, ref=ref)
608614
rows = self._run(
609-
"MATCH (c:GoCallable {signature:$sig}) WHERE c.id STARTS WITH $pre RETURN c.id AS ref, c.span_json AS span",
615+
"MATCH (c:GoCallable {signature:$sig}) WHERE c.id STARTS WITH $pre "
616+
"OPTIONAL MATCH (t:GoType)-[:GO_HAS_METHOD]->(c) "
617+
"RETURN c.id AS ref, c.span_json AS span, t.id AS type_id ORDER BY c.id",
610618
sig=sig,
611619
pre=self._scope_prefix,
612620
)
613621
if not rows:
614622
raise KeyError(f"{sig!r} resolved but no :GoCallable node carries it in application {self.application_name!r}")
615-
if len(rows) > 1:
616-
raise ValueError(f"{sig!r} is carried by more than one analysed callable; neither can be addressed unambiguously")
617-
ref = rows[0]["ref"]
618-
span = json.loads(rows[0]["span"]) if rows[0]["span"] else None
623+
chosen = pick_declaration(sig, [(self._path_of(r["ref"]), r["type_id"]) for r in rows], in_class=in_class, in_module=in_module)
624+
row = next(r for r in rows if self._path_of(r["ref"]) == chosen)
625+
span = json.loads(row["span"]) if row["span"] else None
619626
return SliceNode(
620-
file=self._path_of(ref),
627+
file=chosen,
621628
line=span["start"][0] if span else 0,
622629
callable=sig,
623630
kind="callable",
624631
name=sig.rpartition(".")[2],
625632
source=None,
626-
ref=ref,
633+
ref=row["ref"],
627634
)
628635

629636
def resolve_value(self, name: str, *, within: str) -> SliceNode:

0 commit comments

Comments
 (0)