diff --git a/.claude/SCHEMA_DECISIONS.md b/.claude/SCHEMA_DECISIONS.md index 1ad68e4d..3037da0b 100644 --- a/.claude/SCHEMA_DECISIONS.md +++ b/.claude/SCHEMA_DECISIONS.md @@ -60,11 +60,13 @@ found not to). Expose `--precision {rta,0-cfa,0-1-cfa}`. Coarse heap precision conservative but sound-leaning semantic `ddg`. ### D7 — L4 summary edges: own summary pass -Compute `summary` (actual_in→actual_out) edges via a dedicated pass — hammock regions -composed bottom-up over the SCC-condensation DAG (Tarjan), k-limited to a monotone -fixpoint — mirroring `codeanalyzer-python` (`summaries.py`/`scc.py`). WALA's HRB -summaries are lazily computed inside its Slicer and not cleanly exposable. Heaviest -L4 unit; lands last. +Compute `summary` (actual_in→actual_out) edges via a dedicated pass — composed +bottom-up over the SCC-condensation DAG (Tarjan), k-limited to a monotone fixpoint. +The Python pilot operates at statement granularity, not via region decomposition; +region decomposition remains an open refinement for either analyzer. Both persist +`cfg` and `cdg` on the callable and compute post-dominators; what remains is the +region decomposition itself. WALA's HRB summaries are lazily computed inside its +Slicer and not cleanly exposable. Heaviest L4 unit; lands last. ### D8 — Identity: `can://java////` Java analog of the pilot's `can://python/…`; built from the existing `signatureOf()`. diff --git a/docs/design/plans/2026-08-28-l3-entry-defs.md b/docs/design/plans/2026-08-28-l3-entry-defs.md index b4cb9a3d..c0610c05 100644 --- a/docs/design/plans/2026-08-28-l3-entry-defs.md +++ b/docs/design/plans/2026-08-28-l3-entry-defs.md @@ -268,7 +268,7 @@ D7 currently reads, in both places, that the summary pass mirrors `codeanalyzer- Rewrite both mentions to say what is true: the pass composes bottom-up over the SCC condensation with a monotone fixpoint (which both analyzers do), that `codeanalyzer-python` reaches its transfer relation at statement granularity rather than by region decomposition, and that region decomposition remains an open refinement for either analyzer rather than an existing precedent. Keep the rest of D7 — SCC condensation, k-limiting, fixpoint, "heaviest unit, sequenced last" — as written; only the parity claim is wrong. -Add one sentence recording that Java is better positioned than the reference for a future region pass, since it already persists both `cfg` and `cdg` on the callable and has Cooper–Harvey–Kennedy post-dominators in `CdgBuilder`. +Add one sentence stating that both analyzers persist `cfg` and `cdg` on the callable and compute post-dominators, so what remains to implement is the region decomposition itself. - [ ] **Step 2: Verify no other document repeats the claim** @@ -286,23 +286,368 @@ git commit -m "docs(spec): correct D7 — the reference analyzer has no hammock ## Measurement -*Task 2 fills this in. Leave the headings; replace the placeholder text.* +Measured at `228f270` ("after") against its parent `53a4029` ("before"). The "before" jar was +built from a throwaway worktree rather than by stashing, so the working branch was never mutated: + +```bash +mkdir -p /tmp/m +git worktree add /tmp/l3-before 228f270~1 +(cd /tmp/l3-before && ./gradlew fatJar -x test) # before jar +./gradlew fatJar -x test # after jar +# note: build/libs also holds a stale codeanalyzer-2.4.1.jar, so name the jar explicitly +# rather than globbing codeanalyzer-*.jar +java -jar /tmp/l3-before/build/libs/codeanalyzer-3.0.0.jar \ + -i src/test/resources/test-applications/l4-sdg-test -a 4 --no-build -o /tmp/m/before +java -jar build/libs/codeanalyzer-3.0.0.jar \ + -i src/test/resources/test-applications/l4-sdg-test -a 4 --no-build -o /tmp/m/after +git worktree remove /tmp/l3-before # when finished +``` + +**What this measurement does not cover.** There is no `gradle` on PATH in this environment, so +every CLI run needs `--no-build` and the WALA half degrades wherever the target has no compiled +classes. On `l4-sdg-test` that is total: both documents carry **zero** `prov:["points-to"]` edges, +so nothing below says anything about the WALA engine on that fixture. On `daytrader8` it happens +*not* to degrade — that project ships a `target/classes` (155 `.class` files, untracked, left by an +earlier local run), so WALA did run there and emitted 1231 `points-to` edges, **identically on both +sides** (1231 before, 1231 after). That identity is the only positive evidence here that the change +is AST-engine-only as the Global Constraints require; a clean clone without a built `daytrader8` +will not reproduce it and will see the points-to half absent on both sides instead. + +Also uncovered: the shadowing risk this plan's own self-review flags (a catch or lambda parameter +sharing a formal's spelling). Nothing below tests it. The 43 spurious `summary` edges found on +daytrader8 are all attributable to a different mechanism, established by construction — restoring +per-parameter seeding removes exactly those 43 and nothing else — but no claim is made here about +whether any of the 1646 new `ddg` edges is itself a shadowing artifact, because that was not +measured. ### Fixture deltas, per callable -*(new `ddg` edges as full tuples; `summary` edges gained)* +Eleven `ddg` edges added, **none lost, none changed**. Every one is rooted at `@entry` with +`prov:["ssa"]`, as the additivity constraint requires. `Heap.get()` takes no parameters and is +correctly untouched. + +| Callable | new `ddg` edge | `dst` node kind | +| --- | --- | --- | +| `Chain.a(int)` | `{src:"@entry", dst:"5:9", var:"x", prov:["ssa"]}` | `return` | +| `Chain.b(int)` | `{src:"@entry", dst:"9:9", var:"y", prov:["ssa"]}` | `return` | +| `Chain.c(int)` | `{src:"@entry", dst:"13:9", var:"z", prov:["ssa"]}` | `return` | +| `Heap.put(int)` | `{src:"@entry", dst:"7:9", var:"v", prov:["ssa"]}` | `statement` (`this.box = v;`) | +| `Heap.roundTrip(int)` | `{src:"@entry", dst:"15:9", var:"v", prov:["ssa"]}` | `call` (`put(v);`) | +| `Loops.callFirst(int[])` | `{src:"@entry", dst:"15:9", var:"a", prov:["ssa"]}` | `return` | +| `Loops.first(int[])` | `{src:"@entry", dst:"8:9", var:"q", prov:["ssa"]}` | `loop` (the for-each container) | +| `Mutual.even(int)` | `{src:"@entry", dst:"5:9", var:"n", prov:["ssa"]}` | `branch` (`if (n == 0)`) | +| `Mutual.even(int)` | `{src:"@entry", dst:"8:9", var:"n", prov:["ssa"]}` | `return` (`return odd(n - 1);`) | +| `Mutual.odd(int)` | `{src:"@entry", dst:"12:9", var:"n", prov:["ssa"]}` | `branch` | +| `Mutual.odd(int)` | `{src:"@entry", dst:"15:9", var:"n", prov:["ssa"]}` | `return` | + +Aggregate: `ddg` 2 → 13; `summary` 5 → 5; `param_in` 6 → 6; `param_out` 6 → 6. + +**`summary` edges: no change at all.** The two documents carry the identical five-edge set — +`Chain.a` `5:16/actual_in:0 → 5:16/actual_out`, `Chain.b` `9:16/actual_in:0 → 9:16/actual_out`, +`Loops.callFirst` `15:16/actual_in:0 → 15:16/actual_out`, `Mutual.even` +`8:16/actual_in:0 → 8:16/actual_out`, `Mutual.odd` `15:16/actual_in:0 → 15:16/actual_out`. Nothing +gained, nothing lost. + +**Therefore `L4GateTest`'s pinned counts did not move** and the file is unmodified by this task: +`param_in` 6, `param_out` 6, `summary` 5, exactly as pinned at #203. On this fixture semantic +seeding reproduces the textual result precisely. `Loops.first` is the interesting case: `L4GateTest`'s +javadoc singles it out as the one summary edge that *needs* container-node text seeding, because +`q` is named nowhere but the for-each header. It no longer needs it — `{@entry → 8:9, var q}` +composes with the pre-existing `{8:9 → 9:13, var x}` to reach the `return` sink semantically. + +Read that "no change" carefully, though. **Every callable in `l4-sdg-test` has arity 0 or 1** — the +fixture's maximum parameter count is one. The daytrader8 run below shows the change *does* move +`summary` edges, by a mechanism that needs two parameters in one callable to fire. So the stable +count here is not evidence that the change is summary-neutral; it is evidence that this fixture +cannot see the difference. That is a gap in the fixture, and the follow-on should close it. (Closed: +`com/l4/Arity.java` adds a two-parameter callable of which only one parameter flows out, which moves +the fixture's totals to `param_in` 9, `param_out` 7, `summary` 6.) ### Seeding-rule redundancy -*(per rule: what disappears when it is disabled)* +Each rule was put behind a system property in a scratch worktree +(`git worktree add /tmp/l3-scratch 228f270`) so one jar covers every combination: +`-Dseed.rule1=0` / `-Dseed.rule2=0` / `-Dseed.rule3=0` disable `seedsFor`'s ddg-rooted, call-argument +text, and whole-body span text rules respectively. With no property set the instrumented jar emits a +**set-identical** `summary` payload to the clean jar on both corpora, so the harness is +behaviour-neutral. The scratch edits live only in `/tmp/l3-scratch`; nothing in this repo changed. + +```bash +java -Dseed.rule2=0 -Dseed.rule3=0 -jar /tmp/l3-scratch/build/libs/codeanalyzer-3.0.0.jar \ + -i src/test/resources/test-applications/daytrader8 -a 4 --no-build -o /tmp/m/dt-R1only +``` + +Summary-edge totals (`l4-sdg-test` / `daytrader8`): + +| variant | fixture | daytrader8 | vs. all-rules-on | +| --- | --- | --- | --- | +| all three rules (= shipped) | 5 | 329 | — | +| rule 1 disabled | 5 | 283 | loses 46 on daytrader8 | +| rule 2 disabled | 5 | 329 | loses nothing | +| rule 3 disabled | 5 | 329 | loses nothing | +| **rule 1 only** | **5** | **329** | **set-identical to all three** | +| rule 1 only, *before* the change | **0** | — | — | + +Read straight, that is the answer the follow-on wants: **rules 2 and 3 are dead on both corpora. +Disabling either loses zero edges, and rule 1 alone reproduces the entire emitted set — all 5 on the +fixture and all 329 on daytrader8.** And the last row is what the change bought: at `53a4029`, rule 1 +alone produced **zero** summary edges on the fixture, because no `ddg` edge could be rooted at a +parameter, so the ddg-rooted rule had nothing to match. It was inert for parameter seeding; the +textual rules were carrying the pass entirely. + +Rule 1 was not *entirely* inert before, though. On daytrader8, `dt-noR1` emits 283 edges against +`dt-before`'s 286, so three edges came from rule 1 matching an ordinary (non-entry) `ddg` edge whose +access-path base is a parameter name — e.g. a parameter reassigned in the body, whose reassignment +node then roots an edge. Those three are: + +- `TradeDirect.buy(String, String, double, int)` — `338:11/actual_in:1 → 338:11/actual_out` +- `TradeDirect.completeOrder(Integer, boolean)` — `524:19/actual_in:1 → 524:19/actual_out` +- `TradeDirect.sell(String, Integer, int)` — `445:11/actual_in:1 → 445:11/actual_out` + +#### The 46-edge gap is not a win — rule 1 now over-approximates across parameters + +Rule 1 disabled loses 46 edges but only 3 of those existed before the change, so the change *added* +43 summary edges on daytrader8 (286 → 329, none lost). **All 43 are spurious.** The mechanism: + +```java +for (JDdgEdge e : ddg) { + if (name.equals(base(e.getVar()))) { + seeds.add(e.getSrc()); // SummaryPass.java:244 + } +} +``` + +For an ordinary edge, `src` is the node that *defines* the variable, and seeding there is right. For +an entry-rooted edge `src` is `@entry` — the node that defines **every** formal. So the seed set for +*any* parameter collapses to the single shared node `@entry`, and `reaches()`'s BFS then follows +`@entry`'s out-edges for *all* the other parameters too. If one parameter reaches the return, every +parameter with an entry-rooted edge is credited with reaching it. + +Confirmed on a purpose-built discriminator rather than inferred. Drop this in a throwaway project +(a `build.gradle` copied from `l4-sdg-test` plus `src/main/java/com/x/T.java`) and run both jars over +it at `-a 4 --no-build`. The layout is verbatim — the node ids cited below are line:column, so +reformatting it renumbers them: + +```java +package com.x; + +public class T { + // Only `p` reaches the return. `q` is used by a void call and goes nowhere. + public int leak(int p, int q) { + int t = p; + sink(q); + return t; + } + + public void sink(int z) { + } + + public int caller(int m, int n) { + return leak(m, n); + } +} +``` + +`leak`'s `ddg` after the change is `{@entry → 6:9, var p}`, `{@entry → 7:9, var q}`, +`{6:9 → 8:9, var t}`. Seeding `q` at `@entry` walks the `p` edge to `6:9`, then to the `8:9` return +sink, so `flows(leak)` becomes `{0,1}` and `caller` gains +`{src:"15:16/actual_in:1", dst:"15:16/actual_out"}` — a claim that `n` comes back out of `leak`, +which it cannot: `q` is only handed to a `void` callee. Before the change this edge did not exist, +because the textual rules seeded `q` at `7:9` and at `7:9/actual_in:0`, both dead ends (`sink` is +`void`, so that site has no `actual_out` to bridge to). + +`KeySequenceDirect.getNextID` is the same shape in real code. It gains exactly six edges — +`39:13/actual_in:{1,2,3}` and `45:19/actual_in:{1,2,3}`, each `→ actual_out` — at its two +`allocNewBlock(conn, keyName, inSession, inGlobalTxn)` sites. Argument 0 is credited on *both* sides, +so `conn` already flowed to `allocNewBlock`'s return before the change; arguments 1–3 appear only +after it. Reading the callee, `conn` reaches the return through +`stmt = conn.prepareStatement(…)` → `rs = stmt.executeQuery()` → `keyVal = rs.getInt(…)` → +`block = new KeyBlock(keyVal, …)` → `return block`, and `keyName`, `inSession` and `inGlobalTxn` +are swept along with it at the shared `@entry` seed. The first sentence of that is observed; the +def-use chain is read off the source, not extracted from the document. + +This is over-approximation, which the L4 weak-update posture explicitly accepts, and no real flow is +lost — so it is not a defect against the plan's constraints. It is a **precision regression in +`SummaryPass`**, not in `DdgBuilder`, and it matters here because it lands on exactly the axis the +retirement decision turns on. + +#### A one-line correction removes it + +Seeding the *use* site instead of the shared def when the source is `@entry` restores per-parameter +resolution. Tested as a fourth toggle (`-Dseed.entryfix=1`) over +`seeds.add(ENTRYFIX && "@entry".equals(e.getSrc()) ? e.getDst() : e.getSrc())`: + +| variant | fixture | daytrader8 | +| --- | --- | --- | +| corrected rule 1 + rules 2,3 | 5 | 286 — **set-identical to `dt-before`** | +| **corrected rule 1 alone** | **5** | **286 — set-identical to `dt-before`** | + +All 43 spurious edges disappear, the discriminator's `caller` drops back to one summary edge, and +corrected rule 1 *on its own* reproduces the exact pre-change edge set across 1229 callables. This is +observed, not argued. The reasoning for why it should hold in general — reach from `@entry` along the +`var == name` edges is exactly `{dst} ∪ reach(dst)`, so seeding `dst` is the same set minus the other +parameters' edges — is inference, and the follow-on should re-derive it rather than take it from here. + +#### Exact-set restoration, shown rather than asserted + +The claim above was originally recorded as counts. Re-derived end to end while shipping the fix, as +**sets** — the extractor emits one `callable-id ⇥ src ⇥ dst` line per summary edge, sorted, so an +empty `diff` is set (indeed sorted-multiset) identity and the matching digest is independent +corroboration. Run from the repo root: + +```bash +git worktree add /tmp/l3-seedfix 228f270~1 +(cd /tmp/l3-seedfix && ./gradlew fatJar -x test -q) +./gradlew fatJar -x test -q + +mkdir -p /tmp/m4 +java -jar /tmp/l3-seedfix/build/libs/codeanalyzer-3.0.0.jar \ + -i src/test/resources/test-applications/daytrader8 -a 4 --no-build -o /tmp/m4/before +java -jar build/libs/codeanalyzer-3.0.0.jar \ + -i src/test/resources/test-applications/daytrader8 -a 4 --no-build -o /tmp/m4/after + +cat > /tmp/m4/summaries.py <<'PY' +import json, sys +def walk(types, out): + for t in types.values(): + walk(t.get("types") or {}, out) + for c in (t.get("callables") or {}).values(): + walk(c.get("types") or {}, out) + for e in c.get("summary") or []: + out.append("%s\t%s\t%s" % (c["id"], e["src"], e["dst"])) +d, out = json.load(open(sys.argv[1])), [] +for f in d["application"]["symbol_table"].values(): + walk(f.get("types") or {}, out) +print("\n".join(sorted(out))) +PY +python3 /tmp/m4/summaries.py /tmp/m4/before/analysis.json > /tmp/m4/before.txt +python3 /tmp/m4/summaries.py /tmp/m4/after/analysis.json > /tmp/m4/after.txt +wc -l /tmp/m4/before.txt /tmp/m4/after.txt +diff /tmp/m4/before.txt /tmp/m4/after.txt && echo "EXACT SET MATCH" +shasum -a 256 /tmp/m4/before.txt /tmp/m4/after.txt +git worktree remove /tmp/l3-seedfix +``` + +``` +Preparing worktree (detached HEAD 53a4029) +HEAD is now at 53a4029 feat(l4): interprocedural SDG — param_in/param_out, semantic DDG, summary edges, graph 2.1.0 (#203) + 286 /tmp/m4/before.txt + 286 /tmp/m4/after.txt + 572 total +EXACT SET MATCH +be6b830272fee1fc2af9125ccc1b9bec6dbdffa718d93065d23978f3926d198a /tmp/m4/before.txt +be6b830272fee1fc2af9125ccc1b9bec6dbdffa718d93065d23978f3926d198a /tmp/m4/after.txt +``` + +The same comparison on `l4-sdg-test` (now carrying the two-parameter `Arity` discriminator) is 6 +edges on both sides with an empty `diff`. And with rules 2 and 3 disabled in a scratch worktree — +the toggles re-applied on top of the corrected rule 1 — **corrected rule 1 alone** is also 286 and +also `diff`-empty against `before.txt`, which is the row the retirement decision actually rests on: + +```bash +java -Dseed.rule2=0 -Dseed.rule3=0 -jar /tmp/l3-r1only/build/libs/codeanalyzer-3.0.0.jar \ + -i "$PWD/src/test/resources/test-applications/daytrader8" -a 4 --no-build -o /tmp/m4/dt-R1only +python3 /tmp/m4/summaries.py /tmp/m4/dt-R1only/analysis.json > /tmp/m4/dt-R1only.txt +wc -l /tmp/m4/before.txt /tmp/m4/dt-R1only.txt +diff /tmp/m4/before.txt /tmp/m4/dt-R1only.txt && echo IDENTICAL +``` + +``` + 286 /tmp/m4/before.txt + 286 /tmp/m4/dt-R1only.txt + 572 total +IDENTICAL +``` + +**The correction must stay scoped to `@entry`.** Generalising it — `seeds.add(e.getDst())` for +*every* rule-1 edge, which the inference above invites — was measured and **loses three edges**, all +at `TradeDirect.completeOrder(Connection, Integer)` argument 1 (`buy` `338:11`, `completeOrder(Integer, +boolean)` `524:19`, `sell` `445:11`): 283 against `before.txt`'s 286. The mechanism is the *same* +conflation, arriving through a WALA edge instead of a synthetic one — `completeOrder`'s `ddg` carries +`{566:5 → …, var orderID, prov:["points-to"]}`, and `566:5` is the node that defines the returned +`orderData`, so seeding `orderID` there borrows `orderData`'s reach to the `return`. + +**A later read of these three edges found they are not a real flow.** The `var` match is nominal, not +semantic: `orderID` here names the field `OrderDataBean.orderID` (`OrderDataBean.java:68`), read off +the returned bean at the `orderData.getOrderID()` call sites this edge's `dst` reaches +(`TradeDirect.java:597,610,623,626,634`) — not `completeOrder(Connection, Integer)`'s own parameter of +the same spelling. That parameter is bound into a JDBC call instead (`stmt.setInt(1, +orderID.intValue())` at `:557`), and the row the query returns only *happens* to carry a column also +named `orderID`; neither engine models that connection. So generalising the correction would not have +dropped a real flow here — it would have removed a same-named-field conflation of exactly the class +the `@entry` fix exists to remove, arriving by name collision with an unrelated field rather than by a +shared synthetic node. + +That changes *why* the correction stays scoped to `@entry`, not whether it should. This branch is a +regression fix: its job is restoring the `53a4029` baseline it perturbed — the 286-edge set these +three already belonged to — not auditing every other instance of the same over-seeding. And the L4 +posture's asymmetry still applies on its own terms: it forbids under-approximation, and "measured safe +on `l4-sdg-test` and daytrader8" is not the same claim as "safe everywhere" — widening `e.getDst()` to +every rule-1 edge, not just entry-rooted ones, is a precision reform whose burden of proof (that it +drops no real flow on some corpus neither of these two happens to contain) belongs to a follow-on that +can gather it, not to this fix riding in on two corpora's worth of evidence. So `@entry` gets the +correction and ordinary def sites keep `src`, for now. ### Real-application aggregates -*(edge deltas and wall-clock, on a fixture of realistic size)* +`daytrader8`, 1229 callables, 141 source files. Same two jars, `-a 4 --no-build`. + +| | before (`53a4029`) | after (`228f270`) | delta | +| --- | --- | --- | --- | +| `ddg` total | 3829 | 5475 | **+1646, 0 lost** | +| — of which `prov:["ssa"]` | 2598 | 4244 | +1646 | +| — of which `prov:["points-to"]` | 1231 | 1231 | **0** | +| — of which rooted at `@entry` | 0 | 1646 | +1646 | +| `summary` total | 286 | 329 | +43, 0 lost (all 43 spurious — above) | +| `param_in` | 1943 | 1943 | 0 | +| `param_out` | 914 | 914 | 0 | + +Every added edge is `prov:["ssa"]` and `@entry`-rooted; the WALA-derived half is untouched, which is +the additivity and engine-scope constraint holding on real code. + +**Wall clock — no measurable cost.** Three alternating warm runs per side, `/usr/bin/time -p`: + +| | run 1 | run 2 | run 3 | median real | median user | +| --- | --- | --- | --- | --- | --- | +| before | 29.98 | 29.98 | 30.14 | **29.98 s** | 62.45 s | +| after | 31.07 | 29.66 | 30.28 | **30.28 s** | 61.59 s | + ++0.30 s (+1.0%) on median wall clock, inside the 1.4 s spread of the "after" arm, and median *user* +CPU is slightly **lower** after. A first, discarded pair read 92.99 s (before) against 52.81 s +(after) — that ordering is backwards and both numbers are cold-cache artifacts of first-touching a +35 MB jar and a 20 MB output file; the gap closes entirely once warm. The check this run exists to +make — that seeding `@entry` with every formal does not blow up the reaching-definitions fixpoint on +real code — passes: 43% more `ddg` edges for ~1% more wall clock. ### Recommendation for the follow-on -*(which rules to retire, and what must stay)* +**Retire rules 2 and 3, but only together with a one-line correction to rule 1 — not before it.** +The redundancy evidence is unambiguous: on both corpora, disabling the call-argument-text rule +(`SummaryPass.java:248-257`) or the whole-body-span-text rule (`:258-262`) loses zero summary edges, +and rule 1 alone reproduces the entire emitted set. That is the retirement case, and entry defs are +what created it — the same experiment run against `53a4029` gives rule 1 alone **zero** edges on the +fixture. But +rule 1 as currently written seeds the shared `@entry` node, which conflates all of a callable's +formals and manufactured 43 false summary edges on daytrader8; retiring the textual rules on top of +that would trade a documented, bounded textual over-approximation for an undocumented semantic one +that is *worse* on exactly the multi-parameter callables the summary pass is most used on. With +`seeds.add(e.getDst())` for entry-rooted edges, corrected rule 1 alone reproduces `daytrader8`'s +pre-change 286-edge set and the fixture's 5-edge set exactly — so the follow-on's order of work +should be: fix the seed, pin the corrected behaviour with a regression test built on the +two-parameter discriminator above, and only then delete `:248-262`. Nothing in the suite can +currently distinguish the two: every test that runs `SummaryPass` — `SummaryPassTest.analyzed()`, +`L4GateTest`, `V2Neo4jSchemaConformanceTest` — runs it over `l4-sdg-test`, and every callable in that +fixture has arity 0 or 1. The fixture needs a multi-parameter callable before the assertion is worth +writing. + +Three gaps bound this recommendation. First, both text rules are exercised here only where rule 1 +already succeeds; neither corpus contains a case where source text is unavailable to `SummaryPass` +(a module with a null `source` would silently disable rule 3 wholesale — `index()` handles that +case, so it is reachable), so "dead" means dead on the 1243 callables measured, not proven +unreachable. Second, `l4-sdg-test` produced no `points-to` edges at all, so nothing here says how the +rules interact with WALA-derived `ddg` edges beyond the observation that daytrader8's 1231 of them +are unchanged on both sides. Third, the corrected-rule-1 result is measured on two corpora, not +proven; the follow-on should re-derive the argument before relying on it. --- @@ -311,4 +656,4 @@ git commit -m "docs(spec): correct D7 — the reference analyzer has no hammock - **Spec coverage:** #204's three goals map to Tasks 1 (entry defs + k-limit parity), 2 (measurement), 3 (spec correction). Its fourth DoD item — `L3 ⊆ L4` still holding and the L4 gate's counts updated — is Task 2 Step 5. - **Placeholder scan:** the Measurement section is deliberately a template Task 2 fills; every other step carries runnable commands or complete code. Task 2 Step 2's per-rule findings cannot be pre-written because they are the experiment's output, but the procedure and the stop condition (a lost summary edge is a defect) are concrete. - **Type consistency:** `build(cfg, fieldDepth, formals)` and `L3Overlays.build(..., formals)` use `List` in both the interface block and the code; `JParameter::getName` matches the model; `ControlFlowGraph.ENTRY` is the existing constant, not a new literal. -- **Known risk not designed away:** Task 1 seeds defs from parameter *names*, which collides with a local of the same name in an inner scope — the reaching-definitions kill at the reassignment handles the common case (covered by `aLocalShadowingTheFormalKillsTheEntryDefinition`), but a catch parameter or a lambda parameter shadowing a formal is not covered by any test here and is called out in #204's caveats. If Task 2's measurement shows spurious edges from shadowing, that is a finding for the follow-on, not a reason to hold Task 1. +- **Known risk not designed away:** Task 1 seeds defs from parameter *names*, which collides with a local of the same name in an inner scope — the reaching-definitions kill at the reassignment handles the common case (covered by `aReassignmentKillsTheEntryDefinition`), but a catch parameter or a lambda parameter shadowing a formal is not covered by any test here and is called out in #204's caveats. If Task 2's measurement shows spurious edges from shadowing, that is a finding for the follow-on, not a reason to hold Task 1. diff --git a/docs/design/specs/schema-v2-l3-l4-design.md b/docs/design/specs/schema-v2-l3-l4-design.md index 33f0b0b5..f38602ef 100644 --- a/docs/design/specs/schema-v2-l3-l4-design.md +++ b/docs/design/specs/schema-v2-l3-l4-design.md @@ -77,7 +77,7 @@ Recorded in [`.claude/SCHEMA_DECISIONS.md`](../../../.claude/SCHEMA_DECISIONS.md | D4 | Type kinds | **Single `kind`** ∈ `class\|interface\|enum\|record\|annotation` + `nesting:{parent?,is_local?}` | Replaces the `is_interface/is_enum/is_record/is_nested/...` boolean pile. | | D5 | L3 CFG engine & granularity | **WALA engine → project to source-statement `line:col` nodes** | WALA computes SSACFG/dominance/def-use (heap-ready for L4); project each SSA instruction to its enclosing source statement via `IMethod.getSourcePosition` + JavaParser statement spans. **Fallback:** if source-fidelity proves unresolvable, revisit hand-building the CFG from the JavaParser AST (how Python/TS/Go do it). | | D6 | L4 points-to precision | **RTA default + `--precision {rta,0-cfa,0-1-cfa}`** | RTA is proven to scale (0-1-CFA was found not to); coarse heap precision ⇒ conservative semantic `ddg`. Precision tunable per project. | -| D7 | L4 summary edges | **Own summary pass** (region/bottom-up over the SCC condensation, k-limited) | Parity with `codeanalyzer-python` (`summaries.py`/`scc.py`); keystone-conformant. Heaviest L4 unit; lands last. | +| D7 | L4 summary edges | **Own summary pass** (bottom-up over the SCC condensation, k-limited, monotone fixpoint) | Bottom-up composition with monotone fixpoint over SCC-condensation DAG, mirroring the Python pilot's approach (which operates at statement granularity, not region decomposition); keystone-conformant. Heaviest L4 unit; lands last. | | D8 | `can://` scheme for Java | `can://java////` | Java analog of the pilot's `can://python/…`; built from the existing `signatureOf()`. | | D9 | Neo4j namespace | Keep the **`J_`** relationship prefix | Existing convention (`J_CALLS`, …); dual-label `JSymbol` merge pattern already present. | @@ -148,7 +148,7 @@ Global/static state modeled as **extra** formal/actual vertices (rides the same **Semantic DDG:** `DataDependenceOptions.FULL` + `ModRef` yields alias/heap-derived def-use, emitted as **additional** `ddg` edges tagged `prov:["points-to"]`. L3's `prov:["ssa"]` edges are untouched — this preserves `L3 ⊆ L4` (weak-update / over-approximate posture; no strong updates that would remove an edge). -**Summary pass (D7):** a dedicated pass mirroring `codeanalyzer-python` — hammock-region summaries composed bottom-up over the SCC-condensation DAG (Tarjan), k-limited, iterated to a monotone fixpoint within each SCC. Produces the `summary` (actual_in→actual_out) edges that make later SDK slicing/taint context-sensitive without re-descending into callees. Heaviest unit; sequenced last. +**Summary pass (D7):** a dedicated pass composing bottom-up over the SCC-condensation DAG (Tarjan), k-limited, iterated to a monotone fixpoint within each SCC. The Python pilot reaches its transfer relation at statement granularity rather than via region decomposition; region decomposition remains an open refinement for either analyzer. Both analyzers persist `cfg` and `cdg` on the callable and compute post-dominators; what remains to implement is the region decomposition itself. Produces the `summary` (actual_in→actual_out) edges that make later SDK slicing/taint context-sensitive without re-descending into callees. Heaviest unit; sequenced last. **Cost controls:** flag-gated (nothing at L4 runs unless `-a 4`); k-limiting mandatory for termination; summaries content-hashed/cached with recorded dependency metadata (incremental re-analysis aspirational); parallel-by-construction wavefront over the SCC DAG, `-j N` byte-identical to `-j 1`. diff --git a/src/main/java/com/ibm/cldk/syntactic_analysis/CallableBuilder.java b/src/main/java/com/ibm/cldk/syntactic_analysis/CallableBuilder.java index b3f35ab5..f8430acb 100644 --- a/src/main/java/com/ibm/cldk/syntactic_analysis/CallableBuilder.java +++ b/src/main/java/com/ibm/cldk/syntactic_analysis/CallableBuilder.java @@ -116,7 +116,10 @@ public JCallable build( // cfg/cdg/ddg overlays. The BlockStmt and symbol solver are live only here. // Skipped under the wala engine — L3WalaOverlays handles that path post-build. if (ctx.getAnalysisLevel() >= 3 && "ast".equals(ctx.getL3Engine())) { - L3Overlays.L3Result l3 = L3Overlays.build(b, callable.getBody(), ctx, ctx.getGraphFieldDepth()); + L3Overlays.L3Result l3 = L3Overlays.build(b, callable.getBody(), ctx, + ctx.getGraphFieldDepth(), + callable.getParameters().stream().map(JParameter::getName) + .collect(Collectors.toList())); callable.setBody(l3.body()); callable.setCfg(l3.cfg()); callable.setCdg(l3.cdg()); diff --git a/src/main/java/com/ibm/cldk/syntactic_analysis/L3Overlays.java b/src/main/java/com/ibm/cldk/syntactic_analysis/L3Overlays.java index 32124fab..b966a58a 100644 --- a/src/main/java/com/ibm/cldk/syntactic_analysis/L3Overlays.java +++ b/src/main/java/com/ibm/cldk/syntactic_analysis/L3Overlays.java @@ -56,11 +56,22 @@ public List ddg() { } } + /** Existing callers keep working; a callable with no declared formals behaves exactly as before. */ public static L3Result build(BlockStmt body, Map existingBody, L1BuildContext ctx, int fieldDepth) { + return build(body, existingBody, ctx, fieldDepth, List.of()); + } + + /** + * @param formals the callable's declared parameter names, in declaration order — passed through to + * {@link DdgBuilder#build(ControlFlowGraph, int, List)} so a formal is defined at {@code @entry} + * and its own dataflow can root a dependence at a parameter. + */ + public static L3Result build(BlockStmt body, Map existingBody, L1BuildContext ctx, + int fieldDepth, List formals) { ControlFlowGraph cfg = CfgBuilder.build(body, existingBody, ctx); List cdg = CdgBuilder.build(cfg); - List ddg = DdgBuilder.build(cfg, fieldDepth); + List ddg = DdgBuilder.build(cfg, fieldDepth, formals); return new L3Result(cfg.nodes(), cfg.toCfgEdges(), cdg, ddg); } } diff --git a/src/main/java/com/ibm/cldk/syntactic_analysis/dataflow/DdgBuilder.java b/src/main/java/com/ibm/cldk/syntactic_analysis/dataflow/DdgBuilder.java index ecea0cbd..82ec8c33 100644 --- a/src/main/java/com/ibm/cldk/syntactic_analysis/dataflow/DdgBuilder.java +++ b/src/main/java/com/ibm/cldk/syntactic_analysis/dataflow/DdgBuilder.java @@ -71,6 +71,11 @@ public int hashCode() { } } + /** Existing callers keep working; a callable with no declared formals behaves exactly as before. */ + public static List build(ControlFlowGraph cfg, int fieldDepth) { + return build(cfg, fieldDepth, List.of()); + } + /** * Compute the data-dependence edges for one callable, in three phases: * @@ -85,8 +90,13 @@ public int hashCode() { * * Edges are deduped and sorted for determinism; each is one def→use pair for a {@code fieldDepth}- * limited access path. + * + * @param formals the callable's declared parameter names, in declaration order. They are defined + * at {@code @entry}: a formal has no defining statement, but it is live on entry, and without + * that def no dependence can root at a parameter — which is what forced downstream consumers + * to recover parameter flow by matching source text. */ - public static List build(ControlFlowGraph cfg, int fieldDepth) { + public static List build(ControlFlowGraph cfg, int fieldDepth, List formals) { // Phase 1: per-node gen sets (defs) and the paths each node reads (uses). List nodes = reachableFromEntry(cfg); Map> defs = new HashMap<>(); @@ -95,6 +105,11 @@ public static List build(ControlFlowGraph cfg, int fieldDepth) { Set d = new LinkedHashSet<>(); Set u = new LinkedHashSet<>(); collect(cfg.astNode(n), d, u, fieldDepth); + if (ControlFlowGraph.ENTRY.equals(n)) { + // A formal's access path is its bare name — a base segment, which AccessPath never + // truncates, so this is k-independent by construction. + d.addAll(formals); + } defs.put(n, d); uses.put(n, u); } diff --git a/src/main/java/com/ibm/cldk/syntactic_analysis/dataflow/SummaryPass.java b/src/main/java/com/ibm/cldk/syntactic_analysis/dataflow/SummaryPass.java index 310427ba..164ac181 100644 --- a/src/main/java/com/ibm/cldk/syntactic_analysis/dataflow/SummaryPass.java +++ b/src/main/java/com/ibm/cldk/syntactic_analysis/dataflow/SummaryPass.java @@ -9,6 +9,7 @@ import com.ibm.cldk.schema.JParameter; import com.ibm.cldk.schema.JType; import com.ibm.cldk.schema.Span; +import com.ibm.cldk.syntactic_analysis.controlflow.ControlFlowGraph; import java.nio.charset.StandardCharsets; import java.util.ArrayDeque; import java.util.ArrayList; @@ -36,13 +37,15 @@ *

The relation is derived syntactically, in the weak-update (may-flow, never-drop) * posture the L4 design takes: a parameter reaches the return if the callable's {@code ddg} carries * it there, if a call site passes it on and that callee's own summary returns it, or if the - * parameter is simply named in a body node's source text. That last rule is what makes the - * pass useful at all on ordinary code, because {@link DdgBuilder} gives a parameter no defining node - * — nothing in the {@code ddg} is ever rooted at one. Without it {@code int c(int z) { return z - 3; - * }} has no def-use to follow at all, and {@code int m(int q) { int t = q; return t; }} has only - * {@code (decl → ret, var "t")}, which no seed reaches. The cost is counting a parameter merely + * parameter is simply named in a body node's source text. That last rule is what made the + * pass useful at all on ordinary code back when {@link DdgBuilder} gave a parameter no defining node, + * so nothing in the {@code ddg} could be rooted at one: {@code int c(int z) { return z - 3; }} had no + * def-use to follow at all, and {@code int m(int q) { int t = q; return t; }} had only + * {@code (decl → ret, var "t")}, which no seed reached. The cost is counting a parameter merely * mentioned in a statement as flowing out of it. Over-approximating there is the accepted - * trade. + * trade. {@code DdgBuilder} now defines the formals at {@code @entry}, so the ddg rule reaches these + * shapes on its own and the two text rules have been measured redundant on both corpora; retiring + * them is a separate change. */ public final class SummaryPass { @@ -204,11 +207,12 @@ private static Fn facts(JCallable c, byte[] source) { } /** - * Where a parameter's value is visible, syntactically: the def end of any {@code ddg} edge whose - * access path is rooted at it, any call-site {@code actual_in} vertex whose argument text names - * it, and every body node whose source text names it (see the class javadoc — a parameter - * has no defining node in the {@code ddg}, so text is the only thing that can put it on the map at - * all). + * Where a parameter's value is visible, syntactically: an end of any {@code ddg} edge whose + * access path is rooted at it — the def end for an ordinary edge, the use end for one + * rooted at {@code @entry} (see the comment on that rule) — any call-site {@code actual_in} + * vertex whose argument text names it, and every body node whose source text names it + * (see the class javadoc for why the two text rules exist and why they now have nothing left to + * add). * *

Every kind with a span is seedable, containers included. A {@code branch}/{@code loop}/ * {@code switch} span swallows the statements nested inside it, so seeding one on a name mentioned @@ -241,7 +245,37 @@ private static Set seedsFor(Fn fn, String name, Map span if (ddg != null) { for (JDdgEdge e : ddg) { if (name.equals(base(e.getVar()))) { - seeds.add(e.getSrc()); + // For an entry-rooted edge, the *use* end — the obvious `e.getSrc()` is wrong + // there. A seed is a node id, and the graph `facts` builds is keyed on node + // identity alone (it discards `var`), so seeding a def node also hands this + // parameter the out-edges of every *other* variable defined at that node. + // `@entry` is where that bites hardest: `DdgBuilder` defines all of a callable's + // formals at that one synthetic node, so `getSrc()` collapses every parameter's + // seed set to `{@entry}` and lets one parameter's reach credit them all — 43 + // spurious summary edges on daytrader8, every one of them on a callable of arity + // greater than 1. The dst is where *this* parameter's value is observed, which + // is exactly what the `var`-filtered edge licenses and no more. + // + // A real def site keeps its `src`. The same conflation happens there in + // miniature, but it is the bounded may-flow over-approximation this pass already + // accepts. Doing this unconditionally was measured against the pre-change + // baseline and found to cost three daytrader8 edges at + // `TradeDirect.completeOrder(Connection, Integer)`, all under the `var` label + // `orderID` — but that label is nominal, not semantic: those edges are rooted at + // the node defining `orderData` and reach a `getOrderID()` call on it, so `var` + // names the *field* `OrderDataBean.orderID`, not this callable's own parameter of + // the same spelling. The parameter reaches the return, if at all, only through + // JDBC that neither this engine nor WALA models, so the three are a conflation of + // the same class this fix removes, not a real flow it would drop. + // + // The scoping still stops at `@entry`, though — for a narrower reason than "it + // would cost something real". This branch is a regression fix: its job is + // restoring the baseline it perturbed, not auditing every other instance of this + // same over-seeding. And the L4 posture forbids under-approximation on evidence + // this thin — widening `src` to `dst` for every rule-1 edge, not just entry-rooted + // ones, is measured safe on only two corpora, not enough to rule out dropping a + // real flow somewhere neither has been run. That burden belongs to a follow-on. + seeds.add(ControlFlowGraph.ENTRY.equals(e.getSrc()) ? e.getDst() : e.getSrc()); } } } diff --git a/src/test/java/com/ibm/cldk/schema/L3DifferentialGateTest.java b/src/test/java/com/ibm/cldk/schema/L3DifferentialGateTest.java index db663b5c..71dacc2b 100644 --- a/src/test/java/com/ibm/cldk/schema/L3DifferentialGateTest.java +++ b/src/test/java/com/ibm/cldk/schema/L3DifferentialGateTest.java @@ -16,6 +16,8 @@ import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertTrue; +import com.github.javaparser.StaticJavaParser; +import com.github.javaparser.ast.body.MethodDeclaration; import com.github.javaparser.ast.stmt.BlockStmt; import com.github.javaparser.ast.stmt.Statement; import com.ibm.cldk.CodeAnalyzer; @@ -214,7 +216,18 @@ void differentialGateCfgCdgAgreeDdgDeltaReported(@TempDir Path tmp) throws Excep // ---- AST engine (reference oracle) -------------------------------------------------- ControlFlowGraph astG = CfgBuilder.build(body, new LinkedHashMap<>(), ctx); List astCdg = CdgBuilder.build(astG); - List astDdg = DdgBuilder.build(astG, 3); + // Every TARGET_METHODS entry takes at least one parameter, so building the reference + // oracle's ddg without formals (the legacy two-arg overload) would leave it blind to the + // @entry-rooted edges the shipped engine now emits — exactly the divergence this gate + // exists to compare against WALA. Extract the formals the same way + // DdgBuilderEntryDefsTest.ddgOf does and use the three-arg overload instead. + MethodDeclaration astMethod = StaticJavaParser.parse(FIXTURE) + .findAll(MethodDeclaration.class).stream() + .filter(m -> m.getNameAsString().equals(methodName)) + .findFirst().orElseThrow(); + List formals = astMethod.getParameters().stream() + .map(p -> p.getNameAsString()).collect(Collectors.toList()); + List astDdg = DdgBuilder.build(astG, 3, formals); // ---- WALA engine ------------------------------------------------------------------- WalaAnalysis.MethodIr mir = findMethod(wala, methodName); diff --git a/src/test/java/com/ibm/cldk/schema/L4GateTest.java b/src/test/java/com/ibm/cldk/schema/L4GateTest.java index 9d28876a..6ba4504f 100644 --- a/src/test/java/com/ibm/cldk/schema/L4GateTest.java +++ b/src/test/java/com/ibm/cldk/schema/L4GateTest.java @@ -69,37 +69,40 @@ void paramEdgeAritiesMatchAndNothingDangles() { } /** - * §14's "arity matches" as an actual count. Hand-derived from the four fixture files and + * §14's "arity matches" as an actual count. Hand-derived from the five fixture files and * confirmed against this run: * *

    - *
  • {@code param_in} = 6 — one per argument at each of the six in-project call sites with - * arguments: {@code a→b}, {@code b→c}, {@code even→odd}, {@code odd→even}, - * {@code roundTrip→put}, {@code callFirst→first}. {@code roundTrip→get()} passes none, so - * it contributes none. - *
  • {@code param_out} = 6 — one per site whose callee returns a value: the same four - * {@code Chain}/{@code Mutual} sites, plus {@code callFirst→first} and - * {@code roundTrip→get()}; {@code put} is {@code void}, so that site has no - * {@code actual_out} to reach. - *
  • {@code summary} = 5 — {@code Chain.a}, {@code Chain.b}, {@code Mutual.even}, - * {@code Mutual.odd} and {@code Loops.callFirst}, one shortcut each. + *
  • {@code param_in} = 9 — one per argument at each in-project call site that passes any: + * {@code a→b}, {@code b→c}, {@code even→odd}, {@code odd→even}, {@code roundTrip→put}, + * {@code callFirst→first} and {@code leak→sink} contribute one each; {@code caller→leak} + * contributes two. {@code roundTrip→get()} passes none, so it contributes none. + *
  • {@code param_out} = 7 — one per site whose callee returns a value: the same four + * {@code Chain}/{@code Mutual} sites, plus {@code callFirst→first}, + * {@code roundTrip→get()} and {@code caller→leak}; {@code put} and {@code sink} are + * {@code void}, so those sites have no {@code actual_out} to reach. + *
  • {@code summary} = 6 — {@code Chain.a}, {@code Chain.b}, {@code Mutual.even}, + * {@code Mutual.odd}, {@code Loops.callFirst} and {@code Arity.caller}, one shortcut each. * {@code Heap.roundTrip}'s two sites are a void callee and a no-arg callee, so neither can - * carry one. {@code Loops.callFirst}'s is the one that needs container nodes to be - * seedable: {@code first}'s parameter is named only in its {@code for}-each header. + * carry one, and neither can {@code Arity.leak}'s single void site. {@code Loops.callFirst}'s + * is the one that needs container nodes to be seedable: {@code first}'s parameter is named + * only in its {@code for}-each header. {@code Arity.caller}'s is the one that needs + * per-parameter seeding: its site passes two arguments and only the first comes back, so a + * seed shared across formals would make it two ({@code SummaryPassTest} pins which). *
*/ @Test void overlayCountsAreExactlyWhatTheFixtureImplies() { JsonObject app = root.getAsJsonObject("application"); - assertEquals(6, app.getAsJsonArray("param_in").size(), "param_in: one per argument at a resolved site"); - assertEquals(6, app.getAsJsonArray("param_out").size(), "param_out: one per value-returning site"); + assertEquals(9, app.getAsJsonArray("param_in").size(), "param_in: one per argument at a resolved site"); + assertEquals(7, app.getAsJsonArray("param_out").size(), "param_out: one per value-returning site"); int summaries = 0; for (JsonObject c : callablesById(root).values()) { JsonArray summary = c.getAsJsonArray("summary"); summaries += summary == null ? 0 : summary.size(); } - assertEquals(5, summaries, "summary: one shortcut per pass-through call site"); + assertEquals(6, summaries, "summary: one shortcut per pass-through call site"); } @Test diff --git a/src/test/java/com/ibm/cldk/syntactic_analysis/dataflow/DdgBuilderEntryDefsTest.java b/src/test/java/com/ibm/cldk/syntactic_analysis/dataflow/DdgBuilderEntryDefsTest.java new file mode 100644 index 00000000..88fd2970 --- /dev/null +++ b/src/test/java/com/ibm/cldk/syntactic_analysis/dataflow/DdgBuilderEntryDefsTest.java @@ -0,0 +1,79 @@ +package com.ibm.cldk.syntactic_analysis.dataflow; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import com.github.javaparser.StaticJavaParser; +import com.github.javaparser.ast.body.MethodDeclaration; +import com.github.javaparser.ast.stmt.BlockStmt; +import com.ibm.cldk.schema.JDdgEdge; +import com.ibm.cldk.syntactic_analysis.L1BuildContext; +import com.ibm.cldk.syntactic_analysis.L3Overlays; +import java.util.LinkedHashMap; +import java.util.List; +import java.util.stream.Collectors; +import org.junit.jupiter.api.Test; + +/** + * A callable's formals are defined at {@code @entry}, so its own dataflow can see them. Before this, + * {@code @entry} had no AST node and therefore no defs, and no ddg edge could root at a parameter. + */ +class DdgBuilderEntryDefsTest { + + /** Build the AST-engine L3 overlays for one method's body, with its formals declared. */ + private static List ddgOf(String source, String methodName) { + MethodDeclaration md = StaticJavaParser.parse(source) + .findAll(MethodDeclaration.class).stream() + .filter(m -> m.getNameAsString().equals(methodName)) + .findFirst().orElseThrow(); + BlockStmt body = md.getBody().orElseThrow(); + List formals = md.getParameters().stream() + .map(p -> p.getNameAsString()).collect(Collectors.toList()); + L1BuildContext ctx = new L1BuildContext("can://java/t", "T.java", source, 3, 3, "ast"); + return L3Overlays.build(body, new LinkedHashMap<>(), ctx, 3, formals).ddg(); + } + + private static boolean hasEntryEdge(List ddg, String var) { + return ddg.stream().anyMatch(e -> "@entry".equals(e.getSrc()) && var.equals(e.getVar())); + } + + @Test + void aParameterUsedInTheReturnGetsAnEntryRootedEdge() { + List ddg = ddgOf("class T { int m(int q) { return q; } }", "m"); + assertTrue(hasEntryEdge(ddg, "q"), + "the formal is defined at @entry and used by the return: " + ddg); + assertEquals(1, ddg.stream().filter(e -> "@entry".equals(e.getSrc())).count(), + "exactly one entry-rooted edge for one used formal: " + ddg); + } + + @Test + void anUnusedParameterProducesNoEdge() { + List ddg = ddgOf("class T { int m(int q) { return 1; } }", "m"); + assertTrue(ddg.stream().noneMatch(e -> "@entry".equals(e.getSrc())), + "a formal nothing reads yields no edge — a def with no use is not a dependence: " + ddg); + } + + @Test + void aReassignmentKillsTheEntryDefinition() { + // `q` is reassigned before the read, so the read depends on the assignment, not on @entry. + // (A local can't shadow a formal — redeclaring `q` in its own method body doesn't compile — + // this is a plain reassignment, which is a distinct kill case worth its own regression test.) + List ddg = ddgOf("class T { int m(int q) { q = 5; return q; } }", "m"); + assertTrue(ddg.stream().noneMatch(e -> "@entry".equals(e.getSrc())), + "the reassignment kills the entry def before any use reaches it: " + ddg); + } + + @Test + void twoFormalsBothUsedEachGetTheirOwnEdge() { + List ddg = ddgOf("class T { int m(int a, int b) { return a + b; } }", "m"); + assertTrue(hasEntryEdge(ddg, "a") && hasEntryEdge(ddg, "b"), ddg.toString()); + } + + @Test + void everyEntryEdgeCarriesSsaProvenance() { + List ddg = ddgOf("class T { int m(int q) { return q; } }", "m"); + ddg.stream().filter(e -> "@entry".equals(e.getSrc())) + .forEach(e -> assertEquals(List.of("ssa"), e.getProv(), + "an entry def is syntactic, not points-to derived")); + } +} diff --git a/src/test/java/com/ibm/cldk/syntactic_analysis/dataflow/SummaryPassTest.java b/src/test/java/com/ibm/cldk/syntactic_analysis/dataflow/SummaryPassTest.java index cb63dd51..fd6989cf 100644 --- a/src/test/java/com/ibm/cldk/syntactic_analysis/dataflow/SummaryPassTest.java +++ b/src/test/java/com/ibm/cldk/syntactic_analysis/dataflow/SummaryPassTest.java @@ -9,6 +9,7 @@ import com.ibm.cldk.schema.JCallEdge; import com.ibm.cldk.schema.JCallable; import com.ibm.cldk.schema.JDdgEdge; +import com.ibm.cldk.schema.JIdEdge; import com.ibm.cldk.schema.JModule; import com.ibm.cldk.schema.JParameter; import com.ibm.cldk.schema.JType; @@ -152,10 +153,14 @@ void anUnresolvedWrapperDoesNotBreakComposition() { * the real pipeline, because that is the point: {@link com.ibm.cldk.syntactic_analysis.controlflow.BodyNodeBuilder} * gives a for-each exactly one {@code loop} node spanning the whole statement (the header never * gets a node of its own), and {@link DdgBuilder} attributes the iterated expression and the loop - * variable to that node. So the loop node has a real outgoing ddg edge to {@code return x;}, but - * the only thing that can ever seed it for {@code q} is its own span text — a parameter has no ddg - * def site. With container kinds excluded from seeding, {@code flows(first)} was empty and the - * caller lost a summary edge for a flow that genuinely exists. + * variable to that node. So the loop node has a real outgoing ddg edge to {@code return x;}, and + * {@code q} is now defined at {@code @entry} too, so {@code DdgBuilder} roots a second edge there + * — {@code @entry → loop node} — which the ddg-rooted seeding rule alone now rides to the return. + * Before {@code @entry} defined the formals, a parameter had no ddg def site at all and the loop + * node's own span text was the only thing that could ever seed it; that text route still finds it + * too, redundantly. With container kinds excluded from seeding altogether, both routes vanish and + * {@code flows(first)} would again be empty — the caller would lose a summary edge for a flow that + * genuinely exists. */ @Test void aParameterUsedOnlyInALoopHeaderStillReachesTheReturn() throws Exception { @@ -167,10 +172,35 @@ void aParameterUsedOnlyInALoopHeaderStillReachesTheReturn() throws Exception { assertTrue(callFirst.getSummary().get(0).getDst().endsWith("/actual_out")); } + /** + * Two parameters in one callable, only one of which flows out — the discriminator no other + * fixture callable can be, since every other has arity 0 or 1. {@code Arity.leak(p, q)} returns a + * copy of {@code p} and hands {@code q} to a {@code void} callee, so {@code caller} may shortcut + * its {@code actual_in:0} and must not shortcut its {@code actual_in:1}. + * + *

The negative half is the one with teeth. Seeding a parameter at the def end of its + * {@code ddg} edge seeds every formal at the one shared {@code @entry} node — the reachability + * graph is keyed on node identity, not on {@code var} — so the search follows {@code p}'s edge to + * the return on {@code q}'s behalf and {@code caller} carries two summary edges instead of one. + */ + @Test + void onlyTheParameterThatActuallyFlowsIsShortcut() throws Exception { + Map modules = analyzed(); + JCallable caller = callable(modules, "/Arity/caller(int, int)"); + assertNotNull(caller.getSummary(), "p flows out of leak, so caller shortcuts that argument"); + List shortcut = new ArrayList<>(); + for (JIdEdge e : caller.getSummary()) { + shortcut.add(e.getSrc().substring(e.getSrc().lastIndexOf('/') + 1)); + } + assertEquals(List.of("actual_in:0"), shortcut, "q reaches only a void callee — argument 1 is not a flow"); + } + /** * A parameter that reaches the return through a local — {@code int m(int q) { int t = q; - * return t; }} — the commonest Java shape after {@code return q;} itself. Nothing in the ddg is - * rooted at {@code q}: {@code DdgBuilder} gives a parameter no defining node, so the only edge is + * return t; }} — the commonest Java shape after {@code return q;} itself. The fixture is built by + * hand with no edge rooted at {@code q}: {@code DdgBuilder} now defines every formal at + * {@code @entry}, but this test exists to pin the textual fallback in isolation, so it + * constructs the pre-{@code @entry} shape on purpose — the only edge is * {@code (decl → ret, var "t")}; there is no call site; and the return text names {@code t}, not * {@code q}. The seed therefore has to come from the declaration statement's own text, * which is what the widened word-boundary rule supplies — the existing {@code decl → ret} edge @@ -384,9 +414,9 @@ void summaryEndpointsAreExistingLocalBodyNodes() throws Exception { edges[0]++; })); // Without this the endpoint check passes vacuously on a regression that empties every - // summary. Five is the fixture's whole set: Chain.a→b, Chain.b→c, Mutual.even→odd, - // Mutual.odd→even, Loops.callFirst→first. Heap's two sites cannot carry one (void callee, - // no-arg callee). - assertEquals(5, edges[0], "every fixture summary edge is checked, and there are five of them"); + // summary. Six is the fixture's whole set: Chain.a→b, Chain.b→c, Mutual.even→odd, + // Mutual.odd→even, Loops.callFirst→first and Arity.caller→leak. Heap's two sites cannot + // carry one (void callee, no-arg callee), and neither can Arity.leak→sink (void callee). + assertEquals(6, edges[0], "every fixture summary edge is checked, and there are six of them"); } } diff --git a/src/test/resources/test-applications/l4-sdg-test/src/main/java/com/l4/Arity.java b/src/test/resources/test-applications/l4-sdg-test/src/main/java/com/l4/Arity.java new file mode 100644 index 00000000..ea82e9b2 --- /dev/null +++ b/src/test/resources/test-applications/l4-sdg-test/src/main/java/com/l4/Arity.java @@ -0,0 +1,24 @@ +package com.l4; + +/** + * The fixture's only callable with two parameters, and the only shape that can tell per-parameter + * seeding apart from seeding the shared node every formal is defined at. Only `p` reaches `leak`'s + * return; `q` is handed to a void callee and goes nowhere. Seed a parameter at the def end of its + * ddg edge and both collapse onto `@entry`, `q` walks `p`'s edges to the return, and `caller` gains + * a second summary edge claiming `n` comes back out of `leak`. + */ +public class Arity { + + public int leak(int p, int q) { + int t = p; + sink(q); + return t; + } + + public void sink(int z) { + } + + public int caller(int m, int n) { + return leak(m, n); + } +}