fix(l3): @entry defines the formals, so dataflow seeding is semantic instead of textual - #205
Merged
Conversation
Measurement only; no analyzer behaviour changes and no scratch edits land. L4GateTest's pinned counts did not move and the file is untouched: param_in 6, param_out 6, summary 5 on l4-sdg-test, before and after. That is not evidence of neutrality, though — every callable in that fixture has arity 0 or 1, and the mechanism that does move summary edges needs two parameters in one callable to fire. On the fixture: 11 ddg edges added, all @entry-rooted with prov ["ssa"], none lost. On daytrader8: ddg 3829 -> 5475 (+1646, 0 lost), points-to half unchanged at 1231 on both sides, wall clock +1.0% (median 29.98s -> 30.28s), so entry defs do not blow up the reaching-definitions fixpoint on real code. Seeding-rule redundancy: rules 2 and 3 lose zero edges when disabled on either corpus, and rule 1 alone reproduces the whole set (5 on the fixture, 329 on daytrader8). Against the parent commit, rule 1 alone produced zero — the ddg-rooted rule was inert for parameter seeding. Rule 1 is not yet a safe replacement, however. It seeds e.getSrc(), which for an entry-rooted edge is the shared @entry node, so BFS follows every formal's out-edges and one parameter's reach credits them all. That manufactured 43 false summary edges on daytrader8 — six on KeySequenceDirect.getNextID alone. Seeding the use site instead restores the exact pre-change set on both corpora. The recommendation is therefore: fix the seed and pin it with a multi-parameter fixture case first, retire the text rules second.
`seedsFor`'s ddg rule filtered edges by `var` per parameter and then threw that precision away. It seeded `e.getSrc()`, which for an entry-rooted edge is the single `@entry` node where `DdgBuilder` defines every formal, and the reachability graph `facts` builds discards `var` entirely — it is keyed on node identity alone. So one parameter reaching the return credited every formal with reaching it. On daytrader8 that manufactured 43 summary edges (286 -> 329, none lost), all on callables of arity greater than one, which is why nothing in the fixture could see it. Seeding `e.getDst()` for entry-rooted edges restores the exact pre-change set: sorted (callable, src, dst) listings of all 286 daytrader8 summary edges are byte-identical against a jar built from 228f270~1 (sha256 be6b8302..d198a on both sides), and the fixture's 6 likewise. With both text rules disabled, corrected rule 1 alone reproduces the same 286-edge set — the result the follow-on retirement of those rules rests on. Evidence recorded in the plan's Measurement section, whose Recommendation paragraph also regains the "on the fixture" qualifier it dropped. Scoped to `@entry` deliberately. Generalising it to every rule-1 edge loses three daytrader8 edges, all at TradeDirect.completeOrder(Connection, Integer) argument 1, where `orderID` reaches the return only through a WALA points-to edge rooted at the node defining the returned `orderData`. Dropping a may-flow is the one direction the L4 posture forbids. Fixture: com/l4/Arity.java adds the first callable with two parameters — `leak(p, q)` returns a copy of `p` and hands `q` to a void callee — reached through `caller(m, n)` so the assertion lands on a summary edge. Without the fix `caller` carries both actual_in:0 and actual_in:1; with it, only actual_in:0. L4GateTest's pinned counts move with that fixture addition: - param_in 6 -> 9: `caller -> leak` contributes two arguments, `leak -> sink` one. - param_out 6 -> 7: `caller -> leak` returns a value; `leak -> sink` is void. - summary 5 -> 6: `Arity.caller` gains a shortcut; `Arity.leak`'s only site is void. SummaryPassTest's whole-fixture endpoint count moves 5 -> 6 for that same edge.
A later review established that the three daytrader8 edges cited as the reason the src/dst correction stays scoped to @entry were never a real flow. Their `orderID` var label names the field OrderDataBean.orderID, read off the bean TradeDirect.completeOrder(Connection, Integer) returns (OrderDataBean.java:68 declares it; TradeDirect.java:597,610, 623,626,634 are all `orderData.getOrderID()` call sites) — not that callable's own `orderID` parameter, which is bound into a JDBC call instead and reaches the return, if at all, only through database semantics neither engine models. So generalising the correction would not have dropped a real flow. It would have removed a same-named-field conflation of exactly the class the @entry fix already removes, arriving by name collision with a field rather than a shared synthetic node. The correction still stays scoped to @entry, but for a narrower reason: this is a regression-fix branch restoring the 53a4029 baseline it perturbed, and the L4 posture's bar for tolerating under-approximation isn't met by evidence from two corpora — not because these three edges are real flows. Corrects SummaryPass.java's seedsFor comment and the plan's "must stay scoped to @entry" section to say this; the mechanism explanation in both — why a shared @entry node conflates formals, why ordinary def sites keep `src` — is unchanged.
L3DifferentialGateTest built its AST-engine reference oracle with the legacy two-argument DdgBuilder.build(astG, 3), which passes no formals. Every TARGET_METHODS callable takes a parameter, so the oracle never carried the @entry-rooted edges the shipped engine now emits — exactly the divergence entry defs created — leaving the gate blind to it. Extracts each method's parameter names the same way DdgBuilderEntryDefsTest.ddgOf does and calls the three-argument overload instead. The DDG section is a report, not an equality assertion, so the widened delta (7 more AST-only edges across the four target methods, all @entry-rooted) is expected, not a failure. Also corrects two SummaryPassTest javadocs left over from before @entry defined the formals: a parameter does now have a ddg def site, and Loops.first's summary edge no longer depends solely on textual seeding (the ddg-rooted rule reaches it too, redundantly). The hand-built fixtures the comments sit above are still valid regression coverage for the textual rules and are unchanged. Renames DdgBuilderEntryDefsTest's aLocalShadowingTheFormalKillsTheEntryDefinition to aReassignmentKillsTheEntryDefinition: its case is `q = 5;`, a reassignment, not shadowing — a local can't shadow a formal in Java, since redeclaring it doesn't compile.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #204. Plan: docs/design/plans/2026-08-28-l3-entry-defs.md, whose Measurement section this branch fills in.
The gap
DdgBuilderderives every node's def/use set from its AST node, and the synthetic@entrynode never has one —ControlFlowGraph.recordAstis only called for statements. Socollect(null, …)returned immediately,@entryhad an empty def set, and no data-dependence edge could ever root at a parameter. A method's own formals were invisible to its dataflow.That is why the L4
SummaryPassseeds parameter reachability by matching a parameter's name against body-node source text: it had nothing semantic to seed from. Its own comment enumerated the cost —return a.z;crediting a parameterz,return "x marks";crediting a parameterxfrom inside a string literal.What lands
@entrydefines the formals (AST engine), threaded fromCallableBuilderthrough newL3Overlays.build/DdgBuilder.buildoverloads. Both pre-existing signatures remain, delegating with an empty list, so a callable with no formals emits exactly what it did before.@entrynode, whose def set holds every formal, while the reachability graph is keyed on node identity withvardiscarded. One parameter's search therefore followed every formal's out-edges, fabricating 43 spurious summary edges on daytrader8. Entry-rooted edges now seed the use end.Arity.javalocks it — the previous fixture's maximum arity was 1, so the repo's own gate structurally could not have caught this.codeanalyzer-python's hammock-region summaries, "parity withcodeanalyzer-python". That repo has no region machinery; its posture is deliberately statement-granularity. Corrected in the spec and in.claude/SCHEMA_DECISIONS.md.Why the seed fix is scoped to
@entryrather than applied generallyThe broader form — seeding the use end for every matching edge — was implemented and measured. It loses 3 daytrader8 edges, so it did not ship. Those 3 turn out to be name collisions (
var orderIDnaming a field on a returned bean, not the parameter ofcompleteOrder(Connection, Integer)), i.e. conflation artifacts of the same class as the 43 the fix removes — so the narrow scoping rests on restoring the measured pre-change baseline and on the posture's asymmetry under two-corpus evidence, not on those edges being real flows. The in-tree comment and the plan both say so. Whether the broader form is right is the follow-on's question, and it needs more than two corpora to answer.Verification
@entryhas no predecessors in a union-only reaching-definitions framework, andparam_in/param_out/points-tocounts are unchanged (1943/914/1231) with every pre-existing edge byte-identical.diffover sorted per-edge listings plus a matching sha256, with the literal commands and output in the plan.L3DifferentialGateTestbuilt its AST-engine oracle through the legacy two-argument call with no formals, so it was comparing WALA against a configuration the product no longer ships. Fixed; its DDG delta now reports 7@entry-rooted AST-only edges where it reported 0.CodeAnalyzerIntegrationTest, which needs a Docker daemon and fails identically onmain.#204's shadowing caveat closes as impossible by language: a catch parameter, a lambda parameter, and a lambda-body local reusing a formal's name are all compile errors under JLS 6.4, and the one legal rebinding is structurally unreachable by
DdgBuilder.Follow-ups, not in this PR
Retiring the two textual seeding rules (the decision this branch's measurement exists to inform); whether the broader use-end seeding is correct; and
l4-sdg-test/Loops.java's now-stale comment, left because touching it shifts byte offsets the gate asserts against.One caveat on the numbers: daytrader8 emits
points-toedges despite--no-buildonly because an untrackedtarget/classesis present, so a clean clone reproduces different figures. Identical on both sides of every comparison here, so no conclusion depends on it.