Skip to content

Commit 805a81b

Browse files
committed
refactor: lift reach() into commons, fixing it for all four languages
The frontier fix belonged in the language-independent code, not in Go's copy. _reach was byte-identical in the python, java, typescript and go local backends -- four copies of a graph algorithm over an adjacency of strings, which is the same argument that lifted shortest_walks in leg 2.5b. Four copies were four places to drift, and that is precisely how one bug came to live in all of them. commons.graphs.reach now owns the walk and the reason for its shape; each backend's _reach is a one-liner over it, keeping only `direction` -- which half of the cached adjacency to read -- as a backend concern. So the fix now covers Python, Java and TypeScript too, where the same unbounded walk was non-terminating on a sufficiently connected graph and depth=None is likewise the default on paths_between, flows_to_call, flows_to_argument and taint. Tests move to the shared suite beside call_reaches, the other lifted graph helper: a diamond chain where 55 nodes carry 262,144 paths pins expansion counting (1,048,573 frontier expansions before, 55 now), plus depth monotonicity, the fixed point, a cycle, an unknown seed, and that no backend keeps a copy. 2030 pass; the one failure predates the branch. Refs #436
1 parent 10617e7 commit 805a81b

6 files changed

Lines changed: 176 additions & 69 deletions

File tree

‎cldk/analysis/commons/graphs.py‎

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -422,6 +422,53 @@ def flow_path(nodes: Sequence[SliceNode], edges: Sequence[Tuple[str, "str | None
422422
return FlowPath(hops=[PathHop(frm=nodes[i], to=nodes[i + 1], via=via[rel], var=var, prov=list(prov or [])) for i, (rel, var, prov) in enumerate(edges)])
423423

424424

425+
def reach(edges: Mapping[str, Mapping[str, Sequence[tuple]]], src: str, depth: "int | None") -> set:
426+
"""The node ids reachable from ``src`` over ``edges`` in at most ``depth`` hops.
427+
428+
``edges`` is one direction of the ``{src: {dst: [label]}}`` adjacency each local backend
429+
builds -- only the keys are read, so the labels are ignored here. ``depth`` of ``None`` is the
430+
whole cone.
431+
432+
Level-by-level rather than a plain stack, because ``depth`` is a hop budget and a depth-first
433+
walk cannot count hops without revisiting. Every local backend's slices and flow predicates go
434+
through this, so "reachable" means one thing per language *and* across them.
435+
436+
**A node enters the next frontier once**, which is why ``seen`` is added to inside the level
437+
rather than after it. The obvious spelling builds the level as a comprehension and updates
438+
``seen`` afterwards::
439+
440+
nxt = [d for src in frontier for d in edges.get(src, ()) if d not in seen]
441+
seen.update(nxt)
442+
443+
That is correct and unusable. ``seen`` does not yet contain the level being built, so a node
444+
with *k* predecessors in the current frontier is appended *k* times and the next level expands
445+
every copy: the frontier tracks the number of **paths**, not the number of nodes. A set cannot
446+
hold duplicates, so the answer never changes -- which is exactly why it survived in four
447+
separate copies of this function until an evaluation against a real application measured the
448+
cost. On the ``memos`` corpus (208,293 source nodes, 743,154 edges), over 250 ports: identical
449+
reachable sets at every depth, and 151,417,783 frontier expansions at depth 8 against 670,450
450+
-- 83.65s against 0.11s -- while unbounded it did not terminate at all. ``depth=None`` is the
451+
default on ``paths_between`` / ``flows_to_call`` / ``flows_to_argument`` / ``taint``, so the
452+
unusable case was the default one (python-sdk#436).
453+
454+
Lifted out of the four language backends for :func:`shortest_walks`' reason, and with its
455+
history as the argument: it is a graph algorithm over an adjacency of strings and knows no
456+
language, and four copies were four places to drift -- which is how one bug came to live in
457+
all of them.
458+
"""
459+
seen, frontier, hops = {src}, [src], 0
460+
while frontier and (depth is None or hops < depth):
461+
nxt = []
462+
for node in frontier:
463+
for d in edges.get(node, ()):
464+
if d not in seen:
465+
seen.add(d)
466+
nxt.append(d)
467+
frontier = nxt
468+
hops += 1
469+
return seen
470+
471+
425472
def shortest_walks(
426473
edges: Mapping[str, Mapping[str, Sequence[tuple]]],
427474
src: str,

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

Lines changed: 7 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -49,7 +49,7 @@
4949
check_page_size,
5050
edge_page,
5151
)
52-
from cldk.analysis.commons.graphs import call_reaches, cone_sinks, flow_path, shortest_walks, slice_resolved, under_callable
52+
from cldk.analysis.commons.graphs import call_reaches, cone_sinks, flow_path, reach, shortest_walks, slice_resolved, under_callable
5353
from cldk.analysis.commons.levels import analyzer_level
5454
from cldk.analysis.commons.resolve import (
5555
CallableCandidate,
@@ -727,39 +727,13 @@ def get_ddg(self, callable: str, *, in_class: str | None = None, page_size: int
727727
def _reach(self, ref: str, direction: str, depth: int | None) -> set:
728728
"""The node ids reachable from ``ref`` in at most ``depth`` hops.
729729
730-
Level-by-level rather than a plain stack, because ``depth`` is a hop budget and a
731-
depth-first walk cannot count hops without revisiting. Shared by the slices and by the two
732-
flow predicates, so "reachable" means one thing on this backend.
733-
734-
**A node enters the next frontier once, which is the whole point of adding to ``seen``
735-
inside the loop rather than after it.** The obvious spelling builds the level as a
736-
comprehension and updates ``seen`` afterwards::
737-
738-
nxt = [d for src in frontier for d in edges.get(src, ()) if d not in seen]
739-
seen.update(nxt)
740-
741-
That is correct but not usable: ``seen`` does not yet contain the level being built, so a
742-
node with *k* predecessors in the current frontier is appended *k* times and the next level
743-
expands every copy. The frontier then tracks the number of **paths** rather than the number
744-
of nodes. Measured on memos (208,293 source nodes, 743,154 edges), over 250 ports: identical
745-
answers at every depth — duplicates cannot change a set — but 151,417,783 frontier
746-
expansions at depth 8 against 670,450, which is 83.65s against 0.11s, and unbounded it does
747-
not terminate at all. Since ``depth=None`` is the default on
748-
:meth:`paths_between`, :meth:`flows_to_call`, :meth:`flows_to_argument` and :meth:`taint`,
749-
the unusable case was the default one.
730+
One line over :func:`~cldk.analysis.commons.graphs.reach`, which is where the walk and the
731+
reason for its shape live: it is a graph algorithm over an adjacency of strings and knows no
732+
language. This method stays because the *direction* is a backend concern — which half of the
733+
cached adjacency to read — and because the slices and both flow predicates call it, so
734+
"reachable" means one thing here.
750735
"""
751-
edges = self._sdg[0][direction]
752-
seen, frontier, hops = {ref}, [ref], 0
753-
while frontier and (depth is None or hops < depth):
754-
nxt = []
755-
for src in frontier:
756-
for d in edges.get(src, ()):
757-
if d not in seen:
758-
seen.add(d)
759-
nxt.append(d)
760-
frontier = nxt
761-
hops += 1
762-
return seen
736+
return reach(self._sdg[0][direction], ref, depth)
763737

764738
def _slice_from(self, root: SliceNode, direction: str, depth: int | None, max_nodes: int) -> Slice:
765739
""":meth:`_reach`'s closure from ``root``, described and capped as the graph's is.

‎cldk/analysis/java/codeanalyzer/codeanalyzer.py‎

Lines changed: 10 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -41,7 +41,7 @@
4141
from cldk.analysis.commons.identity import resolve_identity, validate_identity, cache_matches
4242
from cldk.analysis import AnalysisLevel
4343
from cldk.analysis.commons.bounds import DEFAULT_PAGE_SIZE, check_page_size, edge_page
44-
from cldk.analysis.commons.graphs import flow_path, shortest_walks, slice_resolved, under_callable
44+
from cldk.analysis.commons.graphs import flow_path, reach, shortest_walks, slice_resolved, under_callable
4545
from cldk.analysis.commons.levels import ANALYZER_LEVELS, LEVEL_NAMES, analyzer_level
4646
from cldk.analysis.commons.results import Diagnostic, EdgePage, FlowPath, FlowPaths, Slice, SliceNode
4747
from cldk.analysis.java.backend import (
@@ -547,17 +547,15 @@ def link(src: str, dst: str, label: tuple) -> None:
547547
return self._sdg_cache
548548

549549
def _reach(self, ref: str, direction: str, depth: int | None) -> set:
550-
"""The set of node ids reachable from ``ref`` in at most ``depth`` hops. Level by level
551-
rather than a plain stack, because ``depth`` is a hop budget and a depth-first walk cannot
552-
count hops without revisiting."""
553-
edges = self._sdg()[0][direction]
554-
seen, frontier, hops = {ref}, [ref], 0
555-
while frontier and (depth is None or hops < depth):
556-
nxt = [d for src in frontier for d in edges.get(src, ()) if d not in seen]
557-
seen.update(nxt)
558-
frontier = nxt
559-
hops += 1
560-
return seen
550+
"""The set of node ids reachable from ``ref`` in at most ``depth`` hops.
551+
552+
The walk is :func:`~cldk.analysis.commons.graphs.reach`, lifted there with its
553+
measurements (python-sdk#436). The level-by-level shape and the reason a node must enter
554+
the frontier exactly once live with it; the spelling that was here appended a node once per
555+
predecessor, which left an unbounded walk non-terminating on a real application while every
556+
answer it did return was correct. ``direction`` stays a backend concern.
557+
"""
558+
return reach(self._sdg()[0][direction], ref, depth)
561559

562560
def _value_slice(self, root: SliceNode, *, backward: bool, depth: int | None, max_nodes: int) -> Slice:
563561
"""See :meth:`JavaAnalysisBackend._value_slice`. The whole closure is computed and then cut,

‎cldk/analysis/python/codeanalyzer/codeanalyzer.py‎

Lines changed: 11 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -64,7 +64,7 @@
6464
from cldk.analysis.commons.keys import module_scope
6565
from cldk.analysis.commons.identity import resolve_identity, validate_identity
6666
from cldk.analysis import AnalysisLevel
67-
from cldk.analysis.commons.graphs import call_reaches, under_callable
67+
from cldk.analysis.commons.graphs import call_reaches, reach, under_callable
6868
from cldk.analysis.commons.levels import ANALYZER_LEVELS, LEVEL_NAMES, analyzer_level
6969
from cldk.analysis.commons.resolve import CallableCandidate, body_node_kind, resolve_callable_signature, resolve_value_name, resolve_within, value_candidate
7070
from cldk.analysis.commons.results import BodyRef, CallableRef, Diagnostic, EdgePage, EntrypointCoverage, FlowPath, FlowPaths, LocateResult, ModuleRef, Slice, SliceNode, TypeRef
@@ -1333,18 +1333,17 @@ def link(src: str, dst: str, label: tuple) -> None:
13331333
def _reach(self, ref: str, direction: str, depth: int | None) -> set:
13341334
"""The set of node ids reachable from ``ref`` in at most ``depth`` hops.
13351335
1336-
Level-by-level rather than a plain stack, because ``depth`` is a hop budget and a
1337-
depth-first walk cannot count hops without revisiting. Shared by the slices and by the two
1338-
mixed flow queries so "reachable" means one thing on this backend.
1336+
Shared by the slices and by the two mixed flow queries so "reachable" means one thing on
1337+
this backend.
1338+
1339+
The walk is :func:`~cldk.analysis.commons.graphs.reach`, lifted there with its
1340+
measurements (python-sdk#436). The level-by-level shape and the reason a node must enter
1341+
the frontier exactly once live with it; the spelling that was here appended a node once per
1342+
predecessor, which left an unbounded walk non-terminating on a real application while every
1343+
answer it did return was correct. ``direction`` stays a backend concern -- which half of the
1344+
cached adjacency to read.
13391345
"""
1340-
edges = self._sdg()[0][direction]
1341-
seen, frontier, hops = {ref}, [ref], 0
1342-
while frontier and (depth is None or hops < depth):
1343-
nxt = [d for src in frontier for d in edges.get(src, ()) if d not in seen]
1344-
seen.update(nxt)
1345-
frontier = nxt
1346-
hops += 1
1347-
return seen
1346+
return reach(self._sdg()[0][direction], ref, depth)
13481347

13491348
def _slice_from(self, root: SliceNode, direction: str, depth: int | None, max_nodes: int) -> Slice:
13501349
""":meth:`_reach`'s closure from ``root``, described and capped like the graph's.

‎cldk/analysis/typescript/codeanalyzer/codeanalyzer.py‎

Lines changed: 10 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -53,7 +53,7 @@
5353
check_page_size,
5454
edge_page,
5555
)
56-
from cldk.analysis.commons.graphs import call_reaches, cone_sinks, flow_path, shortest_walks, slice_resolved, under_callable
56+
from cldk.analysis.commons.graphs import call_reaches, cone_sinks, flow_path, reach, shortest_walks, slice_resolved, under_callable
5757
from cldk.analysis.commons.keys import body_key_column, resolve_module_key
5858
from cldk.analysis.commons.levels import ANALYZER_LEVELS, LEVEL_NAMES, analyzer_level
5959
from cldk.analysis.commons.resolve import CallableCandidate, resolve_callable_signature, resolve_value_name, resolve_within
@@ -1186,18 +1186,16 @@ def link(src: str, dst: str, label: tuple) -> None:
11861186
def _reach(self, ref: str, direction: str, depth: int | None) -> set:
11871187
"""The set of node ids reachable from ``ref`` in at most ``depth`` hops.
11881188
1189-
Level-by-level rather than a plain stack, because ``depth`` is a hop budget and a
1190-
depth-first walk cannot count hops without revisiting. Shared by the slices and by the two
1191-
flow predicates so "reachable" means one thing on this backend.
1189+
Shared by the slices and by the two flow predicates so "reachable" means one thing on this
1190+
backend.
1191+
1192+
The walk is :func:`~cldk.analysis.commons.graphs.reach`, lifted there with its
1193+
measurements (python-sdk#436). The level-by-level shape and the reason a node must enter
1194+
the frontier exactly once live with it; the spelling that was here appended a node once per
1195+
predecessor, which left an unbounded walk non-terminating on a real application while every
1196+
answer it did return was correct. ``direction`` stays a backend concern.
11921197
"""
1193-
edges = self._sdg[0][direction]
1194-
seen, frontier, hops = {ref}, [ref], 0
1195-
while frontier and (depth is None or hops < depth):
1196-
nxt = [d for src in frontier for d in edges.get(src, ()) if d not in seen]
1197-
seen.update(nxt)
1198-
frontier = nxt
1199-
hops += 1
1200-
return seen
1198+
return reach(self._sdg[0][direction], ref, depth)
12011199

12021200
def _slice_node(self, ref: str) -> SliceNode:
12031201
"""One reached body node in the caller's vocabulary.

‎tests/analysis/commons/test_lifted_helpers.py‎

Lines changed: 91 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -486,3 +486,94 @@ def test_the_local_acceptance_domain_is_delimited_too(module, cls):
486486
src = inspect.getsource(getattr(importlib.import_module(module), cls)._edge_vars_in)
487487
assert "under_callable(" in src, "the acceptance domain stopped delimiting; a sibling callable's vars leak in"
488488
assert ".startswith(" not in src, "a bare prefix accepts a variable the delimited cut cannot sever"
489+
490+
491+
# -----[ reach: the walk every local backend shares (python-sdk#436) ]-----
492+
#
493+
# Lifted out of four byte-identical copies, which is how one bug came to live in all of them:
494+
# each built the level as a comprehension and updated ``seen`` afterwards, so a node with k
495+
# predecessors in the current frontier was appended k times and the next level expanded every
496+
# copy. The reachable SET never changed -- a set cannot hold duplicates -- so only cost showed it.
497+
498+
499+
def _diamond_chain(levels):
500+
"""``n0 -> {a,b} -> n1 -> {a,b} -> n2 ...``: two predecessors per node, every level.
501+
502+
The shape the bug is quadratic-or-worse on, and small enough to assert exactly: ``1 + 3*levels``
503+
nodes, but ``2**levels`` distinct paths from the head, so a frontier that counts paths rather
504+
than nodes grows exponentially while the answer stays the same size.
505+
"""
506+
edges = {}
507+
for lvl in range(levels):
508+
a, b, nxt = f"a{lvl}", f"b{lvl}", f"n{lvl + 1}"
509+
edges[f"n{lvl}"] = {a: [()], b: [()]}
510+
edges[a] = {nxt: [()]}
511+
edges[b] = {nxt: [()]}
512+
return edges, 1 + 3 * levels
513+
514+
515+
def test_reach_expands_each_node_at_most_once():
516+
"""The invariant, pinned by counting lookups rather than by timing.
517+
518+
18 levels is 55 nodes and 262,144 paths; the old walk made 1,048,573 frontier expansions for
519+
them where this makes 55. Counting is the non-flaky way to say that.
520+
"""
521+
from cldk.analysis.commons.graphs import reach
522+
523+
edges, expected = _diamond_chain(18)
524+
525+
class Counting(dict):
526+
def __init__(self, inner):
527+
super().__init__(inner)
528+
self.fetches = {}
529+
530+
def get(self, key, default=None):
531+
self.fetches[key] = self.fetches.get(key, 0) + 1
532+
return super().get(key, default)
533+
534+
counting = Counting(edges)
535+
got = reach(counting, "n0", None)
536+
assert len(got) == expected
537+
repeats = {k: n for k, n in counting.fetches.items() if n > 1}
538+
assert not repeats, f"reach re-expanded {len(repeats)} node(s), e.g. {next(iter(repeats.items()))}"
539+
540+
541+
def test_reach_is_bounded_monotone_and_reaches_a_fixed_point():
542+
"""``depth`` is a hop budget: each bound contains the shallower ones, and past the graph's
543+
diameter the answer stops growing. Unbounded must equal the fixed point, not exceed it."""
544+
from cldk.analysis.commons.graphs import reach
545+
546+
edges, expected = _diamond_chain(6)
547+
sizes = [len(reach(edges, "n0", d)) for d in range(1, 14)]
548+
assert sizes == sorted(sizes), f"reach is not monotone in depth: {sizes}"
549+
assert sizes[-1] == expected == len(reach(edges, "n0", None))
550+
for d in range(1, 14):
551+
assert reach(edges, "n0", d) <= reach(edges, "n0", None)
552+
553+
554+
def test_reach_handles_a_cycle_and_an_unknown_seed():
555+
"""A cycle must terminate (``seen`` is the guard, not the depth bound), and a seed with no
556+
outgoing edges is its own reach rather than an error -- ``reaches(x, x)``'s zero-hop rule is
557+
``call_reaches``' business, not this walk's."""
558+
from cldk.analysis.commons.graphs import reach
559+
560+
cyclic = {"a": {"b": [()]}, "b": {"c": [()]}, "c": {"a": [()]}}
561+
assert reach(cyclic, "a", None) == {"a", "b", "c"}
562+
assert reach({}, "lonely", None) == {"lonely"}
563+
assert reach(cyclic, "a", 1) == {"a", "b"}
564+
565+
566+
def test_every_local_backend_delegates_to_the_shared_walk():
567+
"""No backend may keep its own copy: four copies were four places to drift, and they did."""
568+
import inspect
569+
570+
from cldk.analysis.go.codeanalyzer import GoCodeanalyzer
571+
from cldk.analysis.java.codeanalyzer import JCodeanalyzer
572+
from cldk.analysis.python.codeanalyzer import PyCodeanalyzer
573+
from cldk.analysis.typescript.codeanalyzer import TSCodeanalyzer
574+
575+
for cls in (PyCodeanalyzer, JCodeanalyzer, TSCodeanalyzer, GoCodeanalyzer):
576+
src = inspect.getsource(cls._reach)
577+
code = [ln.strip() for ln in src.splitlines() if ln.strip()]
578+
assert any(ln.startswith("return reach(") for ln in code), f"{cls.__name__}._reach does not delegate"
579+
assert not any("while frontier" in ln for ln in code), f"{cls.__name__}._reach still carries its own walk"

0 commit comments

Comments
 (0)