[Plan] issue-2088: receiver-correlated invoked-property evidence for object-literal value-refs - #2612
[Plan] issue-2088: receiver-correlated invoked-property evidence for object-literal value-refs#2612carlos-alm wants to merge 25 commits into
Conversation
Delivery plan for #2088: replace the bare-property-name liveness check for object-literal value-refs with allocation-site correlation, gated on an escape check so escaping sites keep today's exact (conservative) behavior. Reconciles the apparent tension with ROADMAP 8.3's field-based points-to choice: field sensitivity and allocation-site abstraction are orthogonal axes, and 8.3's own Approach block already commits to the latter. Part of #2088
Greptile SummaryThe PR adds a docs-only execution plan for allocation-site-correlated object-literal invocation evidence, including TypeScript/Rust parity, persistence, escape analysis, and regression verification.
Confidence Score: 4/5The plan is not yet safe to approve for execution because parenthesized global-object subscript writes remain invisible to the proposed reassignment scan. The planned Files Needing Attention: docs/plans/issue-2088.md Important Files Changed
Reviews (30): Last reviewed commit: "docs: close round-19 under-escape gaps i..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if (owner.bindingName === null) { // `return { … }` — no binding to scan | ||
| entry.escapes = exportedNames.has(owner.enclosingFn); | ||
| continue; |
There was a problem hiding this comment.
For export const T = makeTable() with a non-exported local makeTable, this branch marks the returned literal local-closed solely from the factory's export status even though the planned call-assignment constraint propagates it into exported T. Exclusive correlated evidence then omits external calls to T, allowing live properties to be reported dead; the plan should account for destinations of returned sites before approving execution.
There was a problem hiding this comment.
Addressed in the prior round (commit 8aa0167, "fix WU-2b's return-owner escape gap"): the owner.bindingName === null branch now unconditionally sets entry.escapes = true for a returned-site owner, regardless of the factory function's own export status, since the call-assignment that actually captures the return value can land in any binding this per-file pass can't see. Confirmed still in place at the current head.
| const TRACKED_REFERENCE_PARENTS: ReadonlySet<string> = new Set([ | ||
| 'member_expression', | ||
| 'subscript_expression', | ||
| 'for_in_statement', | ||
| ]); |
There was a problem hiding this comment.
In the promised const u = T; u.alpha() case, the reference to T has a variable_declarator parent, which this allowlist rejects and therefore marks the site escaping. The test can still pass through bare-name fallback, leaving unrelated same-named calls able to preserve aliased properties and failing to verify the advertised alias correlation.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
Fixed in 899188d. allReferencesTracked's condition 3 now recognizes a variable_declarator reference as tracked when its name field is a plain identifier — i.e. const u = T (the alias shape fnRefBindings already models) — while a destructuring name such as const { k } = T still escapes, since it extracts a property rather than aliasing the reference and fnRefBindings doesn't model it. The guard mirrors the existing name?.type !== 'identifier' check in findEnclosingTableName (src/extractors/javascript.ts:4519), which already draws this exact distinction for table-name resolution.
I also strengthened WU-10's correlation test: each of the four cases now must assert escapes = 0 for its site, not just the liveness outcome — otherwise, as you noted, the alias case could keep passing via T2's bare-name fallback even if this guard regressed, without the test ever catching it.
There was a problem hiding this comment.
@_scratch_reply1.txt
There was a problem hiding this comment.
Disregard the reply directly above this one (the literal text @_scratch_reply1.txt) — that was a broken tool call from an earlier round, posted to the wrong thread by mistake, not a real comment. This thread's actual answer is the reply already above it (Fixed in 899188d7...), which stands unchanged. The intended target of that broken reply was the separate "Property extraction bypasses escape tracking" thread on this same file, which now has a proper reply.
…2612) WU-8 (and the Dual-Engine Impact table) named src/domain/graph/builder/stages/native-orchestrator.ts and a computedDispatchTableEvidence precedent there for threading objectLiteralSites across the native NAPI boundary. Neither exists: native-orchestrator.ts has zero occurrences of that field and takes no part in NAPI payload construction — it only runs tryNativeOrchestrator's post-build JS passes (CHA expansion, this-dispatch, structure, dataflow-vertices), which execute after Rust's own full-pipeline build already extracted and consumed that evidence entirely in Rust memory. FileEdgeInput, the Rust struct WU-8 cited, is Rust-only and never appears under src/; its actual TS-side counterpart is NativeFileEntry in build-edges.ts, which already carries computedDispatchTableEvidence. Corrected WU-8's Files/Input contract/Implementation to name NativeFileEntry/buildNativeFileEntry (build-edges.ts) and FileEdgeInput (build_edges.rs) explicitly, added the matching row and a seam paragraph to the Dual-Engine Impact table, and closed the verification gap the wrong citation created: a plain full-build engine comparison never reaches buildCallEdgesNative because tryNativeOrchestrator's fast path returns early first. Documented the exact mechanism by which the plan's existing --engine wasm -> --engine native verification pair already forces that path (an engine-mismatch-triggered forceFullRebuild), and marked that command order load-bearing so it isn't silently broken by a future reordering.
…tion (#2612) WU-2b's computeObjectLiteralSiteEscapes marked a return-statement-owned site (`function f() { return {...} }`) local-closed whenever the factory function itself was not exported, via exportedNames.has(owner.enclosingFn). That checks the wrong binding: the value a factory returns is captured by a call-assignment (`const X = f()`) that can land in any binding, anywhere, and WU-4's buildObjectLiteralSiteConstraints already flows the site into that binding's pts set unconditionally, with no escape check of its own. A return-owned site could therefore be marked non-escaping while a capturing binding it has no visibility into is exported or otherwise untracked, making T1 exclusive and letting a live property be reported dead -- the exact failure direction #2088's soundness requirement exists to rule out. Independently corroborated by Greptile's review comment on the PR (id 5390203404, last updated after the round-1 fix commit, so it reflects the current text, not a stale one). Fixed the branch to always mark a return-owned site escaping -- consistent with the fail-safe default this analysis already uses everywhere else -- and corrected condition 1 of the docstring, which had listed the return-statement shape as one that could reach non-escaping. Added WU-10 escape-fallback shape (d), covering `function factory(){ return {...} } export const X = factory(); X.zeta();`, asserting both liveness and escapes === 1, as the regression gate for this branch. WU-5a also justified resolveReceiverSites' caller-scoped-then-bare pts lookup by citing a function, resolveReceiverPtsAliases, that does not exist anywhere in the repo, at incremental.ts:1436 (which is a different function, emitIncrementalPtsReceiverEdges, doing a single compound-key lookup, not the two-step pattern claimed). Replaced the citation with real precedents: resolveReceiverEdge (call-resolver.ts:773-775, same file), which build-edges.ts:2123 already mirrors by name, and the ptsMap-specific scopedPtsKey fallback in emitPtsNoReceiverEdges (build-edges.ts:1965) / its incremental mirror emitIncrementalPtsNoReceiverEdges (incremental.ts:1350).
…tions (#2612) const u = T; u.alpha() previously fell through the escape allowlist (no variable_declarator case), so the alias site was marked escaping and the correlation test could only ever pass via T2's bare-name fallback -- never actually exercising the T1 correlated-evidence path the alias test case claims to cover. Add a variable_declarator case to allReferencesTracked/condition 3, guarded on the declarator's name field being a plain identifier (not a destructuring pattern) -- mirroring the existing name?.type !== 'identifier' guard in findEnclosingTableName -- so const u = T is recognized as the alias shape fnRefBindings already models, while const { k } = T (property extraction, not aliasing) still escapes, matching the escape-fallback test's own destructuring case. Also strengthen WU-10's correlation test to assert escapes = 0 for each of the four cases, not just the liveness outcome, so a future regression in the escape guard cannot pass silently through T2.
|
@greptileai please re-review. Addressed in 899188d:
|
|
@greptileai please re-review — re-posting this mention as a fresh comment (the prior comment's mention was only added via a later edit due to a Summary of what changed in 899188d, also detailed in my replies on both inline threads:
|
|
|
||
| `resolveSiteOwner` reuses the existing walk shape of `findEnclosingTableName` (variable-declarator lookup through `TABLE_NAME_PASSTHROUGH_TYPES`), extended with two extra cases — `array` parent → `` `${arrayVarName}[*]` `` (the pts key `buildArrayElemConstraints` already produces), and `return_statement` parent → `` `${enclosingFnName}::return` ``. | ||
|
|
||
| `allReferencesTracked` walks the file for identifier nodes whose text equals `bindingName`, skipping the declaration itself and any node under a scope that shadows the name — reusing `introducesShadowedBinding`, the hardened shadow detection already written for #2257 and already used by `findDeclaringScopeLine`. Every surviving reference must have a parent in `TRACKED_REFERENCE_PARENTS`; be the `value` field of a `variable_declarator` whose own `name` field is a plain `identifier` (a rebinding — `const u = T` — rejecting a destructuring `name` the same way `findEnclosingTableName` already does, since destructuring extracts a property rather than aliasing the reference); or be an `arguments`-position identifier whose callee is in `localNonExportedFns`. |
There was a problem hiding this comment.
When T is aliased with const u = T and u is then passed to an imported or otherwise unmodeled function, allReferencesTracked accepts the declarator while scanning only references to T, so the site is marked local-closed despite escaping through u. Exclusive correlated evidence then omits external property invocations and can report a live object-literal value as dead.
There was a problem hiding this comment.
Fixed in 62606d0. The rebinding branch of allReferencesTracked now recurses: accepting const u = T additionally requires allReferencesTracked to also hold for u itself, capped at 6 hops (reusing findEnclosingTableName's existing bound as defense-in-depth against a pathological alias chain, since a chain of fresh const bindings can't actually cycle). So const u = T; importedFn(u) now correctly escapes. Added case (e) to WU-10's escape-fallback test list covering exactly this shape.
You're right that the same transitivity gap exists on the paramBindings branch (a bare-identifier argument to a local non-exported function) — verified against the real, already-shipped buildParamFlowConstraints in src/domain/graph/resolver/points-to.ts, which has no escape-safety check of its own. That's a separate, currently-unreported branch of the same condition, and fixing it needs to scope the recursive check into the callee's own body rather than reuse this same file-level walk, so I filed it separately as #2617 rather than folding it into this fix.
There was a problem hiding this comment.
@_scratch_reply2.txt
There was a problem hiding this comment.
Disregard the reply directly above this one (the literal text @_scratch_reply2.txt) — that was a broken tool call from an earlier round, posted to the wrong thread by mistake, not a real comment. This thread's actual answer is the reply already above it (Fixed in 62606d04...), which stands unchanged. The intended target of that broken reply was the separate "Property extraction bypasses escape tracking" thread on this same file, which now has a proper reply.
Greptile's re-review of the round-3 fix (899188d) caught a hole in it: accepting a `const u = T` reference as tracked only checked that ONE reference to T, never following where u itself goes afterward. So `const u = T; importedFn(u); u.alpha();` could be classified local-closed even though the site escapes through u -- the same shape of gap condition 1 already documents for a return-captured binding, recurring one hop later for an alias-captured one. Make the rebinding branch of allReferencesTracked recursive: accepting `const u = T` now additionally requires condition 3 to hold, recursively, for u itself, capped at 6 hops (reusing findEnclosingTableName's existing bound) as defense-in-depth against a pathological alias chain. Add a regression case to WU-10's escape-fallback test list covering exactly this shape. The bare-identifier-argument (paramBindings) branch has the identical transitivity gap -- verified against the real, already-shipped buildParamFlowConstraints in src/domain/graph/resolver/points-to.ts, which has no escape-safety check of its own -- but fixing it requires scoping a recursive check into the callee's own body rather than reusing the same file-level walk, so it's filed separately as #2617 rather than folded in here.
|
@greptileai please re-review. Addressed in 62606d0 — see reply on the inline comment for the alias-transitivity fix and issue #2617 for the analogous param-flow branch filed separately. |
Review of the plan at 62606d0 found a structural twin of the alias-transitivity gap that commit fixed, on the parameter-passing branch of condition 3: a bare-identifier argument to a locally-defined, non-exported function was accepted as a tracked reference without ever inspecting what the callee does with the parameter. The primitive this relies on, buildParamFlowConstraints (points-to.ts), only adds pts(callee::paramName) <- pts(argName) and is documented "Scope: intra-module only" -- it has no escape check of its own. Concretely: `const P = { beta: fnB }; function use(t) { register(t); } use(P);` with `register` imported. P is unexported, its only reference is the param-pass into `use`, and `use` is local and unexported, so the old condition 3 read the site as local-closed. T1 became exclusive, T2 was skipped, and the real `t.beta()` inside the imported module produces no site token, since cross-module SITE propagation doesn't exist. fnB would be reported dead where today's exact predicate reports it live -- the false-negative-to-false-positive conversion this plan's own hard rule forbids. Apply the conservative remedy instead of recursing into the callee body (the recursive variant stays out of scope, per #2617): drop the parameter-flow branch from condition 3 entirely, so a bare-identifier argument always marks the site escaping and falls back to T2. Move WU-10's correlation case 4 (which asserted escapes = 0 for this shape) into the escape-fallback test as new case (f), asserting escapes = 1 and live via T2. Correct WU-4's "comes for free" claim and Success Criteria bullet 3, both of which advertised param-flow as part of the correlated set, and update the shape counts this ripples into (three correlation shapes, six escape-fallback shapes) throughout the doc. #2617 is re-scoped from a plan defect to a recall enhancement: extending correlation to the param-flow branch by recursing into the callee's own body remains a legitimate follow-up, just not required for this plan to be sound.
| const TRACKED_REFERENCE_PARENTS: ReadonlySet<string> = new Set([ | ||
| 'member_expression', | ||
| 'subscript_expression', | ||
| 'for_in_statement', | ||
| ]); |
There was a problem hiding this comment.
When a property value leaves through an expression such as const u = T; importedFn(u.alpha), allReferencesTracked accepts u because its direct parent is a member_expression without checking the enclosing expression's use. The site is consequently treated as local-closed, so exclusive correlated evidence can report alpha dead even though the recipient invokes it.
There was a problem hiding this comment.
Already addressed, in the commit right after this comment was posted: 9156a889 ("gate tracked reference positions on T1 visibility in WU-2").
u's reference inside u.alpha in importedFn(u.alpha) is a bare (non-call) property read, structurally identical to const f = T.k; f()'s bare read of T.k — which 9156a889's isTrackedReferencePosition narrowing now excludes explicitly: a member-expression reference is tracked only when it is itself the object of a member expression that is in turn the function of an enclosing call_expression. In importedFn(u.alpha), u.alpha's parent is the call's argument list, not u.alpha itself being called — so the grandparent-is-call_expression-with-matching-function-field check fails, isTrackedReferencePosition returns false for u's reference, and (since this is a non-vacuous, genuinely failing reference) allReferencesTracked correctly returns false for u — so T is classified escaping via the round-4 rebinding recursion, exactly as intended.
WU-10's escape-fallback case (h) (const R = { beta: fnF }; const f = R.beta; f();) is the direct-owner regression gate for this exact shape; this specific alias variant (const u = T; importedFn(u.alpha)) is one recursion hop further through the round-4 rebinding branch, and is covered by the same fix since the call-position check applies uniformly to every reference allReferencesTracked walks, direct or recursive.
Re-confirmed unaffected by this PR's latest round (round 7, five further gaps closed — see the new top-level comment): round 7 adds an isArrayOwner short-circuit that runs before this call-position check, and T here is a direct (non-array) binding, so isArrayOwner is false and this reasoning is unchanged.
(Note: two replies landed on the wrong threads on this PR around this same time, each with a literal @_scratch_replyN.txt placeholder body instead of real content — a broken tool call from an earlier round, not a comment from a human. Please disregard those two; this reply is the actual answer to this thread.)
|
@greptileai please re-review. New head since your last pass: Both of your earlier inline findings were answered against the heads that followed them, and each has a reply on its own thread:
What That branch is now dropped: a bare-identifier argument always marks the site escaping, falling back to T2 (today's exact bare-name predicate). WU-4's "parameter flow comes for free" claim is corrected, and WU-10's old correlation case 4 moved into the escape-fallback test as case (f) asserting Extending correlation to the parameter-flow shape properly (recursing into the callee body) is tracked as a follow-up recall enhancement in #2617, not deferred plan work. |
|
@greptileai please re-review. Commit 515140a fixes the parameter-flow branch of WU-2b's escape analysis (condition 3): a bare-identifier argument to a function is no longer treated as a tracked reference, so such sites always fall back to today's exact bare-name predicate instead of being credited as correlated evidence. See the inline replies on the two alias-branch threads for confirmation those fixes are unaffected, and the comment on #2617 re-scoping it from a plan defect to a follow-up recall enhancement. |
|
@greptileai please re-review. Commit 9156a88 fixes a new escape-analysis gap in WU-2 found in review after 515140a.
Fix, in order:
|
|
@greptileai please re-review. Round 7. New head:
On the vacuous- Also cleaned up on this PR: two inline replies from an earlier round landed on the wrong threads with a broken Reconciled throughout: the tracked-set doc comment, Testing Strategy's shape count (8 → 14 escape-fallback cases), the Success Criteria exclusion list, and the WU-7 Rust mirror section ( |
| const isTrackedStaticKey = | ||
| indexType === 'string' || | ||
| (indexType === 'template_string' && !indexNode!.text.includes('$')); | ||
| if (!isTrackedStaticKey) return false; |
There was a problem hiding this comment.
When an object-literal binding is invoked as T['cos$t'](), this predicate accepts the string index as tracked, but both extractors classify names containing $ as unresolved and emit no receiver. The site can therefore become local-closed with no correlated invocation evidence, causing the live property value to be reported dead.
There was a problem hiding this comment.
Fixed in 0bdfb32. isTrackedReferencePosition's subscript branch now applies one check to both string and template_string index kinds identically: strip quote/backtick characters from the index text, then require the result non-empty and free of $ — mirroring extractSubscriptCallInfo/extract_call_info exactly, the same way round 7 already intended but only implemented for the template_string arm. T['co$t']() is now rejected on the string arm too (the template-string form of the same shape, a plain backtick-quoted key containing $, was already correctly rejected by round 7's fix) — so it correctly falls through to escaping/T2 instead of being accepted as a tracked reference with zero T1 evidence behind it.
Added WU-10 escape-fallback case (p) as the regression test: const V2 = { co$t: fnA4 }; V2['co$t'](); with a cross-file decoy via ordinary dot notation (otherObj.co$t();, which needs no bracket/quote at all since $ is a valid identifier character) — asserting live + escapes = 1, so the fix is checked against exactly this shape rather than trusted by inspection alone.
| if (node.type === 'assignment_expression' || node.type === 'augmented_assignment_expression') { | ||
| const left = node.childForFieldName('left'); | ||
| if (left?.type === 'identifier' && left.text === name) return true; |
There was a problem hiding this comment.
When a mutable module-level handler is replaced through a destructuring assignment such as [run] = [function () { return this.alpha(); }], this branch ignores the write because the left side is a pattern rather than a bare identifier. Identifier resolution then trusts the original arrow as this-free, allowing exclusive correlated evidence to report a live sibling handler dead.
There was a problem hiding this comment.
Confirmed and fixed in 752405f. subtreeContainsReassignmentOf's assignment-expression branch now routes through patternBindsName(left, name) instead of the bare left?.type === 'identifier' && left.text === name check, so a destructuring target -- [run] = […], ({ run } = …), and nested/defaulted variants -- is caught, the same way blockContainsIdentifierExcluding's own assignment branch and killsBinding's already handle this exact field. Added escape-fallback cases (af)/(ag) (array- and object-destructuring reassignment respectively) to gate it, and mirrored the fix in the Rust extractor (WU-7).
One residual gap, tracked rather than silently accepted: patternBindsName itself has no case for a parenthesized_expression, so (run) = fn is still invisible to this branch -- the same gap already open at #2630 for the for-in arm, whose scope this commit extends to name the assignment arm as a fourth affected consumer. #2630 stays open.
…2 condition 4 Widens subtreeContainsReassignmentOf's assignment-expression branch to route through patternBindsName instead of a bare identifier check, so a destructuring reassignment target is no longer invisible to the write-scan (Greptile-flagged: "Destructuring writes bypass reassignment tracking"). Makes findTopLevelFunctionNodeByName fail safe when more than one top-level declaration of a name exists, rather than confidently returning the first and ignoring a later one that actually wins at runtime (var redeclaration, duplicate function declarations) -- the first round in which this function's own body, not only its caller's, changes. Also corrects two overstated doc claims surfaced by the same review pass: the round-13 Success Criteria bullet's "enforced structurally... never REASSIGNED" wording, and a stale "restrict to the simplest syntactic shape" precedent citation used to justify the now-fixed narrower scan. Adds a complexity note on the intended per-file pre-pass implementation, discloses the eval/with/globalThis-write residual gaps, and adds four escape-fallback fixtures plus the Rust mirror for both fixes. Files #2633 (duplicate-declaration fail-safe recall cost) and #2634 (globalThis-write residual gap) as follow-ups.
|
@greptileai please re-review. Round 14. New head: Two blocking findings, both surfaced by an independent critic pass that ran the plan's helpers against the real Finding 1 (also independently flagged by Greptile on this PR -- see my reply on that inline comment) -- the write-scan was narrower than the question it answers. Finding 2 -- Also this round: corrected two overstated doc claims (the round-13 Success Criteria bullet's "enforced structurally... never REASSIGNED" wording, and a stale "restrict to the simplest syntactic shape" precedent citation used to justify the pre-fix narrower scan); added a complexity note on the intended per-file pre-pass implementation (O(sites x properties x file size) as specified, versus a single whole-file pre-pass into a |
…dition 4 `findTopLevelFunctionNodeByName` only counted declarations that were direct children of `root`, but `var` is function-scoped, not block-scoped: a `var name` hoisted from inside a bare block, `if`, `for`, `try`, or `switch` body at module level is the SAME binding a direct top-level `var name` would be, and sloppy-mode Annex B extends the identical hazard to a block-level `function` declaration. Both were invisible to the round-14 count, which resolved to the FIRST declaration with full confidence instead of failing safe once a second, hoisted one existed - confirmed by running both shapes under real Node. This also corrects the round-14 scope-coverage note, which claimed no function- or block-scoped variant of "two top-level declarations" could exist at all - true of the pre-round-15 implementation, false of JS semantics. Widens the count via a new countHoistedVarScopeDeclarations helper that reuses functionScopeDeclaresVar's traversal rule, deliberately excluding let/const (a genuinely different, block-scoped binding already handled by the shadow axis) - verified by a new correlation shape proving that exclusion holds rather than merely stating it. Adds escape-fallback cases (aj)/(ak), mirrors the fix in WU-7's Rust notes, and reconciles the Testing Strategy, Risks, and Success Criteria sections accordingly. Two non-blocking items are also addressed: patternBindsName's fail-open depth-cap asymmetry gets a documented caveat, and the Testing Strategy section now spells out that cases (ai)/(ak) need a non-ESM CommonJS fixture file. Recall costs specific to this round's own fix (it never resolves a sole hoisted-only declaration, and doesn't gate the Annex-B branch on the file's strict/sloppy/module parse goal) are filed as a follow-up rather than silently accepted.
|
@greptileai please re-review. Round 15. New head: This round found that This also corrects the round-14 scope-coverage note itself, which claimed no function- or block-scoped variant of "two top-level declarations" could exist at all — true of the pre-round-15 implementation, false of JS semantics. Fixed by generalising the count to the module's own var scope: a new Also reconciled: the Testing Strategy table's shape counts (eight correlation shapes, 37 escape-fallback cases), the Risks & Mitigations and Success Criteria sections' round-by-round narratives, and a spelled-out note that cases (ai)/(ak) need a genuine non-ESM CommonJS fixture file (round 14 stated this in case (ai)'s own comment but never surfaced it at the Testing Strategy level). Two recall costs specific to this round's own fix are disclosed rather than silently accepted, and filed as a follow-up (#2635): it never resolves to a declaration that exists only inside a nested block (always fails safe even when that's the sole, unambiguous declaration), and it doesn't gate the Annex-B branch on the file's own strict/sloppy/module parse goal, so it over-counts where Annex B couldn't actually apply. Also replied to and closed an older outstanding thread from this PR ("Loop shadows bypass resolution guard") that round 11's |
…nd 15 self-review) The round-15 helper I just pushed had a real bug: it pre-filtered a CHILD's node type against FUNCTION_SCOPE_NODE_TYPES before ever recursing into it, reusing functionScopeDeclaresVar's traversal shape verbatim. That shape is safe there because the only node kind it recognizes, variable_declaration, is never itself a member of that set. It is not safe here, because this helper also recognizes function_declaration - which IS itself a member - so a nested Annex-B function_declaration would be skipped before its own name-match check ever ran, silently returning 0 instead of 1. Traced this by hand against case (ak) and confirmed it with a small simulation script before shipping the fix: case (ak) would have gone uncounted and the fix would not have fired for the one shape it exists to catch, while case (aj) and the new correlation shape's let/const exclusion would have appeared to pass regardless. Fixed by checking each node's own type for a match first, then gating recursion on that same node's type - self-check, then decide whether to descend - rather than filtering a child's type before ever visiting it. Updated the doc comment (both engines' notes) to state why this can't be a literal one-for-one reuse of the existing helper's shape, and added this as a third, concrete parity-risk hazard for WU-7 since it is exactly the kind of thing a hand-written port would compile and half-pass without ever noticing. Also files #2636: found while re-verifying that the same 'function_declaration' string match (in both the pre-existing round-14 loop and this round's own extension) does not recognize generator_function_declaration as a distinct grammar kind, so a generator function redeclaration is invisible to the count in both places. Fail-safe already, not confidently wrong, but a real detection gap - filed rather than widened inline, since fixing it means touching round 14's own already-settled test.
|
@greptileai please re-review — new head since my last comment: While re-verifying the round-15 fix by hand against case (ak) after pushing Fixed in |
#2634/#2636) An audit of the exclusion ledger found four tracked gaps mixed in among fourteen accepted over-escape recall trade-offs, but wrongly framed as the same kind of thing: #2630 (parenthesized_expression invisible to patternBindsName, three call sites), #2632 (using_declaration invisible to introducesShadowedBinding's statement_block case), #2634 (a script-scope var reassigned via globalThis.name = ... invisible to subtreeContainsReassignmentOf), and #2636 (a generator function declaration invisible to findTopLevelFunctionNodeByName's redeclaration count). All four are under-escape: a real invocation goes undetected and live code is reported dead, not a recall cost the design accepts on purpose. Close all four locally, inside condition 4's own helpers, without widening the shared patternBindsName/introducesShadowedBinding primitives other verified consumers depend on: - a small local unwrapParens() helper, called at the three parenthesized_expression call sites in subtreeContainsReassignmentOf and findResolvingScopeNode - a fourth using_declaration disjunct on findResolvingScopeNode's own walk (not on introducesShadowedBinding, since condition 3's consumer of that primitive does not need this fix) - a new isGlobalObjectQualifiedWrite check ORed onto subtreeContainsReassignmentOf's assignment branch - a generator_function_declaration branch in findTopLevelFunctionNodeByName's direct-children loop, deliberately not extended to countHoistedVarScopeDeclarations's own recursive hoisting walk (verified empirically that Annex B never hoists a generator declaration, so a nested one does not redeclare the outer binding) Mirrors all four fixes into WU-7/WU-8's Rust, verified against the real, already-shipped Rust source rather than assumed. Corrects the false framing in #2630 ("the safe direction" holds for exactly one of its four consumers, not all), #2634 (misfiled alongside genuine fail-safe trade-offs; it was a missed write, not a detected-and- declined-to-resolve-further condition), and #2636 ("fail-safe-already" holds only when every redeclaration of the name is a generator, and is confidently wrong the moment it is mixed with a plain declaration). Adds an explicit OVER-escape/UNDER-escape direction label to every tracked exclusion in Success Criteria, and a standing rule: an under-escape gap may never be filed as an accepted limitation, it must be fixed in the round that finds it. Records that #2610 is the one pre-existing exception (inherited from #2260, out of this plan's own scope, and verifiably not worsened by it). Adds six new escape-fallback fixtures (one per closed path) and four new correlation-shape guards (one per fix, proving none of them over-escapes the legitimate neighbouring shape), reconciles the Testing Strategy, Risks, and Success Criteria counts, and rewords WU-7's own round-enumeration sentence to describe the current round rather than requiring a hand-extended list every time.
| } else if (node.type === 'update_expression') { | ||
| const arg = node.childForFieldName('argument'); | ||
| if (arg?.type === 'identifier' && arg.text === name) return true; |
There was a problem hiding this comment.
When a mutable handler is updated through a parenthesized target such as (run)++, this branch compares the wrapped argument directly instead of applying unwrapParens. The scan therefore trusts stale information about the handler’s original value, allowing exclusive correlated evidence to classify a genuinely invoked sibling property as dead.
| } else if (node.type === 'update_expression') { | |
| const arg = node.childForFieldName('argument'); | |
| if (arg?.type === 'identifier' && arg.text === name) return true; | |
| } else if (node.type === 'update_expression') { | |
| const arg = node.childForFieldName('argument'); | |
| const target = arg ? unwrapParens(arg) : undefined; | |
| if (target?.type === 'identifier' && target.text === name) return true; |
There was a problem hiding this comment.
Fixed in f709b31. subtreeContainsReassignmentOf's update_expression branch now routes argument through unwrapParens before the identifier comparison, exactly matching the suggested diff and its two sibling branches.
One refinement to the framing, verified empirically rather than assumed: an update_expression performs ECMAScript's own ToNumeric coercion on its operand before writing back, so (name)++/(name)-- can never reassign name to an arbitrary new function value the way an assignment or a for-in rebind can — confirmed directly (let run = () => {}; (run)++; leaves run as NaN, never a callable this-using function). So unlike the destructuring-write and parenthesized-assignment/for-in gaps this same function's history has closed, there's no construction through this branch alone where a genuinely this-using handler was ever wrongly read as this-free because of the missing unwrapParens call — a call through the reassigned binding would throw, not silently invoke the wrong function. Fixed for structural consistency with the sibling branches (and because it's cheap and correct to do), not because a live-reported-dead repro exists — I didn't want to claim one in the plan doc's own commentary where I couldn't verify it.
Added correlation shape 18 (a parenthesized update to a different name must not perturb an unrelated table's own correlation) but no escape-fallback case, since none would demonstrate anything real. Mirrored into the Rust engine description (WU-7). Full details in docs/plans/issue-2088.md's subtreeContainsReassignmentOf doc comment and the Risks table's round-17 entry.
|
@greptileai please re-review. Round 16 closes four tracked under-escape gaps that an audit found had been drifting into the same "accepted recall trade-off" framing as this plan's genuine over-escape exclusions, even though all four are live-code-reported-dead bugs, not design boundaries:
All four are mirrored into WU-7/WU-8's Rust, verified against the real, already-shipped Rust source rather than assumed to match. Six new escape-fallback fixtures (cases (al)-(aq)) and four new correlation-shape guards (9-12, one per fix, proving none of them over-escapes a legitimate neighbouring shape) are added; Testing Strategy, Risks, and Success Criteria counts are reconciled. Also corrects three misfilings this round's own audit found: #2630's issue header called its gap uniformly "the safe direction," true for only one of its four consumers; #2634 was narrated alongside genuine fail-safe recall trade-offs in this plan's own Risks table, when it was actually a missed write; #2636's issue body claimed its gap was "fail-safe-already," true only when every redeclaration of a name is a generator, not when it's mixed with a plain declaration. Corrected via comments on the respective issues and in the plan doc itself. Adds an explicit |
…in WU-2 Round 16's using_declaration disjunct in findResolvingScopeNode is scoped to a statement_block ancestor only, matching #2632's own repro. introducesShadowedBinding's switch_body case carries the identical enumeration (no using_declaration case) that statement_block's did before this round's fix, but this was not verified either way while closing #2632 itself. Filed as #2637 rather than assumed safe, with a cross-reference from findResolvingScopeNode's own round-16 essay and from Out of Scope.
… new) Closes #2637 (introducesShadowedBinding's switch_body case carries the identical missing-using_declaration gap its statement_block case did before round 16) rather than carrying it further. Auditing every other SCOPE_NODE_TYPES member for the same gap, instead of stopping at switch_body alone, found one more instance: for_statement's own case has the identical omission, verified runnable under Node 22.18 with --js-explicit-resource-management (a using declaration in a C-style for-loop's own init clause shadows an outer decoy exactly like the switch_body case does). Both closed the same way round 16 closed statement_block: a disjunct on findResolvingScopeNode alone, never on the shared introducesShadowedBinding primitive. Two further gaps, neither previously filed, found and closed in this same round per the standing rule (an under-escape gap must be fixed in the round that finds it, never filed as an accepted limitation): - a var-kind for-of/for-in loop head (`for (var name of iter)`) rebinds the SAME module-scope binding a direct top-level var declaration created, since var is function-scoped, not block-scoped. Two independent gates both missed it: subtreeContainsReassignmentOf's for-in gate excluded ANY head carrying a kind field, var included, rather than only let/const/using (which alone create a genuinely new binding); and countHoistedVarScopeDeclarations had no case recognizing a for_in_statement as a hoisted declaration site at all, since the grammar places its kind/left fields directly under for_in_statement, never wrapped in a variable_declaration node. Both fixed independently, closing the same construct via two separate mechanisms. - isGlobalObjectQualifiedWrite (round 16, #2634) recognized only the dot spelling of a global-object-qualified write (globalThis.name = ...); the identical write spelled with bracket-subscript notation (globalThis['name'] = ...) is a subscript_expression, invisible for the identical reason the dot spelling was before round 16. Closed by a new subscript_expression arm reusing isTrackedReferencePosition's own static-key normalization verbatim. Also adds a new, unconditional with_statement disjunct to findResolvingScopeNode: no case existed anywhere in the shadow chain for a sloppy-mode `with (obj) { ... }` block, so a same-named module-level decoy resolved through it with full, unearned confidence. Corrects the Risks table's own prior framing, which grouped `with` alongside `eval` as something "no static analysis can see through" since round 14 - true of with's RESOLUTION target, false of its mere PRESENCE as an ordinary, detectable AST node. eval remains correctly Category F; with did not need to be. All five fixes are closed WITHOUT widening patternBindsName, introducesShadowedBinding, or SCOPE_NODE_TYPES themselves - the same discipline rounds 11-16 established. Mirrors all five into WU-7's Rust description, with a new round-17 parity-risk paragraph covering each fix's own porting hazard, including the two-function var-for-in gap's "passes every fixture while being half wrong on one engine" risk. Adds five new escape-fallback fixtures (cases (ar)-(av), one per closed path, (ar)/(av) both crediting #2637) and five new correlation-shape guards (13-17, one per fix, proving none of them over-escapes the legitimate neighbouring shape). Strengthens guard shape 9, which never actually invoked unwrapParens on its own source (no assignment or for-in left to read) - adds a real parenthesized write to a different name so the guard reaches the helper it names. Fixes the standing rule's own wording gap: it said "every bullet below," which scoped it to Success Criteria's own list and let #2637 (filed in Out of Scope, physically above) go unlabelled in the very commit that wrote the rule. The rule now explicitly spans both lists. Removes #2637's own Out of Scope bullet now that it is closed rather than carried. Reconciles counts throughout: 43 -> 48 escape-fallback shapes, twelve -> seventeen correlation shapes, across the Testing Strategy table, the "what no tier catches" reviewer-audit paragraph, the Risks table (all three affected rows), and Success Criteria's own contract bullet list.
|
|
||
| > **The non-vacuous-coverage requirement (ROUND 8, #2088 finding 1 — the structurally important half).** `allReferencesTracked` returns `true` only when BOTH: (1) the walk is PROVEN exhaustive over the declaring scope's subtree — it did not truncate at `MAX_WALK_DEPTH` anywhere within it; AND (2) every reference the (proven-exhaustive) walk found satisfies `isTrackedReferencePosition`, or is accepted on a recursive branch that ALSO satisfies this same two-part contract. Either an unproven walk OR a disqualifying reference makes the result `false` (escaping) — there is no third, "we're not sure, but let's call it safe" outcome. This is a STANDING RULE about the function's return contract, not a special case bolted onto the vacuous-empty-set scenario specifically: it applies identically whether the surviving set is empty, has one reference, or has a hundred. Getting this wrong toward "unproven ⇒ escapes" costs recall — the same asymmetry every other fail-safe default in this design accepts; getting it wrong the other way is precisely the class of bug this rule exists to catch structurally, in every FUTURE change to this walk, not only in the one instance found this round. | ||
|
|
||
| > **The rebinding branch recurses — accepting the `const u = T` reference is not enough on its own** (round-4 critic finding). `allReferencesTracked` must additionally hold, recursively, for the new alias name, with `isArrayOwner` UNCHANGED and `declaringScope` UNCHANGED (`allReferencesTracked(root, aliasName, objectNode, isArrayOwner, declaringScope)`) — an alias of the CONTAINER is still the container, not a single element, so it must keep whatever `isArrayOwner` value the site already has, and (round 8) the search boundary established for the original binding, since `u`'s own declaration is necessarily somewhere inside that same subtree (see the `findDeclaringScopeNode` note above) — or a site reads as local-closed while it can still escape through `u` — e.g. `const u = T; importedFn(u)`. The `name` field of the `variable_declarator` must itself be a plain `identifier`; a destructuring `name` such as `const { k } = T` is rejected the same way `findEnclosingTableName` already does, since destructuring extracts a property rather than aliasing the reference. The first cut of this branch (round-3) checked only the reference to `T` and never followed where `u` goes; that is exactly the same shape of gap condition 1 already documents for a return-captured binding, one alias hop later. The recursion depth is capped at 6, reusing `findEnclosingTableName`'s own `hops` bound rather than inventing a new one — a chain of `const a = T; const b = a; const c = b; …` cannot cycle (each step names a fresh `const` binding), so the cap is defense-in-depth, not a correctness requirement. A recursive call returning `false` — including by hitting the cap, and, as of round 8, by failing its own non-vacuous-coverage requirement — makes that reference, and so the whole site, escaping; it is not a partial result the other branches paper over. Coverage composes the same way: the OUTER call is proven-covered only if every recursive call it makes is also proven-covered. |
There was a problem hiding this comment.
When a block-scoped table is assigned to a function-scoped alias such as var u = T, the recursive alias check searches only the original table binding's block and misses later uses of u in the enclosing function. The site is consequently treated as local-closed, allowing exclusive correlated evidence to report an invoked property value dead.
There was a problem hiding this comment.
Confirmed and fixed. allReferencesTracked's rebinding recursion reused the outer call's declaringScope unconditionally for the recursive alias check — sound for a let/const alias (every case this branch had been verified against through round 17), since a lexically-scoped alias's own visibility never extends past whatever block already contains it, but not for a var one: var is function-scoped, so var u = T declared inside a block narrower than the enclosing function makes u referenceable outside that block, a region the reused boundary never reaches.
Closed by widening the recursive call's own boundary — to the alias's nearest enclosing FUNCTION_SCOPE_NODE_TYPES member (or module root), computed from the alias's own declaration position — specifically when its declarator is var-kind; a let/const alias (or a for-of loop variable) keeps reusing the outer call's boundary exactly as before. SCOPE_NODE_TYPES/introducesShadowedBinding/findDeclaringScopeNode are untouched.
Mirrored in WU-7's Rust section. New escape-fallback case (ba) and correlation shape 22 (proving the widening is a no-op when the alias's nearest enclosing function already coincides with the table's own declaring scope). Full details in allReferencesTracked's own doc-comment essay (round 18), the Risks table, and Success Criteria.
Greptile flagged (PR #2612, comment on subtreeContainsReassignmentOf's update_expression branch) that a parenthesized update target such as (run)++ is compared directly against the identifier check instead of being routed through unwrapParens first - the one branch round 16's own #2630 fix left untouched (the assignment and for-in branches both already got it). Verified the branch does read argument without unwrapParens, and fixed it the same way: route argument through unwrapParens before the identifier comparison, matching its two siblings exactly. Also verified, empirically rather than assumed, that this gap carries no soundness cost unlike every other fix landed this round: an update expression performs ECMAScript's own ToNumeric coercion on its operand, so (name)++/(name)-- can never reassign name to an arbitrary new function value the way an assignment or a for-in rebind can. Confirmed directly - `let run = () => {}; (run)++;` leaves run as NaN, never a callable this-using function. So there is no construction through this branch alone where a genuinely this-using handler was ever wrongly read as this-free because of the missing unwrapParens call. Fixed for structural consistency with the sibling branches and to close the finding, not because a live-reported-dead repro exists - and none is fabricated to manufacture one where none can exist. Adds correlation shape 18 (a parenthesized update to a different name must not perturb an unrelated table's own correlation) but no matching escape-fallback case, since no soundness repro is possible here. Mirrors the fix into WU-7's Rust description. Reconciles the correlation-shape count (seventeen -> eighteen) and adds a short, accurate note to the Risks table and Success Criteria explaining why this one closes without the under-escape framing every other round-17 fix carries. Replied to the Greptile thread with this same reasoning before triggering a re-review.
|
@greptileai please re-review. Round 17 closes #2637 — the residual round 16 itself left standing, filed but not fixed — and closes two further under-escape gaps this round's own audit found, per the standing rule that an under-escape gap must be fixed in the round that finds it:
Also adds a new, unconditional All five fixes are mirrored into WU-7's Rust description, verified against the real, already-shipped Rust source rather than assumed, with a new round-17 parity-risk paragraph. Five new escape-fallback fixtures (cases (ar)-(av)) and five new correlation-shape guards (13-17) are added; the standing rule's own wording is fixed to span both the Success Criteria and Out of Scope lists (it previously only covered the former, which is how #2637 slipped past it unlabelled in the same commit that wrote it). Also strengthens correlation shape 9, which never actually invoked Separately, replied to and fixed the "Parenthesized updates evade reassignment tracking" comment: Testing Strategy, Risks, and Success Criteria counts are reconciled throughout (43 → 48 escape-fallback shapes, twelve → eighteen correlation shapes). |
| if (left && patternBindsName(unwrapParens(left), name)) return true; | ||
| // ROUND 16 (#2088, #2634) — a script-scope `var` is also exposed as a | ||
| // property of the global object; see this function's own doc comment. | ||
| if (left && isGlobalObjectQualifiedWrite(left, name)) return true; |
There was a problem hiding this comment.
When a module-level handler is reassigned through a parenthesized target such as (globalThis.run) = function () { return this.alpha(); }, patternBindsName receives the unwrapped target but isGlobalObjectQualifiedWrite receives the original parenthesized_expression and rejects it. The analysis then trusts the handler’s original this-free value, allowing exclusive correlated evidence to report a live sibling property dead.
There was a problem hiding this comment.
Confirmed and fixed — this is the exact call-site gap: patternBindsName(unwrapParens(left), name) already unwrapped left before this comment was filed, but the very next line, isGlobalObjectQualifiedWrite(left, name), passed the ORIGINAL, still-wrapped left straight through, so (globalThis.run) = … reached isGlobalObjectQualifiedWrite as a bare parenthesized_expression, matching neither of its two arms (member_expression/subscript_expression) at all.
Closed by routing left through unwrapParens at this call site too — isGlobalObjectQualifiedWrite(unwrapParens(left), name) — matching the identical fix unwrapParens(object) now applies inside the function's own two arms for the one-layer-shallower gap ((globalThis).run = …, parens around just the identifier rather than the whole target) that a different investigation into this same function surfaced independently. Both shapes are now covered by one combined fixture: escape-fallback case (az) exercises the object-only paren first, then extends with a second property using your exact repro ((globalThis.run) = …) for the whole-target paren. Mirrored in WU-7's Rust section.
Round 17's for_statement disjunct in findResolvingScopeNode scanned for a using_declaration node that tree-sitter-javascript@0.25.0's grammar can never produce as a for_statement initializer (verified against grammar.js, node-types.json, and the real parser directly, which surfaces the broken text as an ERROR node instead) - the disjunct was dead code, and #2637 was never actually closed for that half. Reopened and re-closed by keying on the actual ERROR shape (both the plain and await-using spellings) and failing safe unconditionally, mirroring with_statement. Adds a standing rule: every fixture must be parsed with the real grammar, and the node type a fix keys on confirmed present in the tree, not inferred from runtime behavior under Node. Also closes three further under-escape gaps found while auditing this round's own scope: a getter can smuggle a this-using function through its return value with no this token in its own body (literalHasUnmodeledThisReference); allReferencesTracked's reuse of introducesShadowedBinding treats a method_definition's bare property name as a binding, spuriously pruning a genuine reference when a nested method happens to share the tracked binding's name; and a single paren layer around a global-object identifier defeats isGlobalObjectQualifiedWrite in both arms. Rebuilds correlation shapes 16 and 17 (their round-17 originals never exercised the disjuncts they claimed to guard) and adds three new correlation shapes plus four new escape-fallback cases for this round's fixes. Mirrors all changes in WU-7's Rust section. Reconciles counts, Testing Strategy, Risks table, and Success Criteria. Fixes two non-blocking nits (isTrackedReferencePosition's for-of discriminator, enclosingObjectLiteral's stale doc prose) and files #2638/#2639 for a new getter over-escape exclusion and a pre-existing collectForOfBinding bug found along the way.
Two Greptile review comments on this PR were left unreplied: "Parenthesized global writes go undetected" and "Var aliases escape the scope walk". The first is the whole-target-parenthesized variant of the paren-wrapped globalThis gap the previous commit already fixed one layer differently ((globalThis).run vs (globalThis.run)) - the same call-site unwrapParens fix in isGlobalObjectQualifiedWrite's caller closes both, confirmed and extended with Greptile's own repro as a second property on case (az). The second is new: allReferencesTracked's rebinding recursion reuses the outer call's declaringScope unconditionally for a recursive alias check. That reuse is sound only when the alias is lexically (let/const) scoped, since only then is the alias's own visibility guaranteed to stay inside the boundary that already contains it. A var-declared alias is function-scoped, so `var u = T` inside a block narrower than the enclosing function makes u referenceable outside that block - a region the reused boundary never reaches, so a genuine downstream reference is silently missed and the site reads local-closed though it can still escape through u. Closed by widening the recursive call's own boundary to the alias's nearest enclosing function (or root) specifically when its declarator is var-kind; a let/const alias is unaffected. Mirrored in WU-7's Rust section. Adds escape-fallback case (ba) and correlation shape 22, and reconciles counts throughout.
|
@greptileai please re-review. Round 18 reopens #2637: the
Two further items came directly from unreplied comments on this PR:
All fixes are mirrored into WU-7's Rust description with new round-18 parity-risk entries. Six new escape-fallback fixtures ((aw)-(ba)) and six new correlation-shape guards (16/17 rebuilt, 19-22 new) are added. Also fixes two non-blocking nits found in passing: Testing Strategy, Risks, and Success Criteria counts are reconciled throughout (48 → 53 escape-fallback shapes, eighteen → twenty-two correlation shapes). |
| const index = node.childForFieldName('index'); | ||
| const indexType = index?.type; | ||
| if (indexType !== 'string' && indexType !== 'template_string') return false; |
There was a problem hiding this comment.
When a mutable module-level handler is reassigned through globalThis[('run')] = fn, the subscript receiver is unwrapped but its index remains a parenthesized_expression, so the write is missed and stale handler information can make correlated evidence report a genuinely invoked sibling property dead.
| const index = node.childForFieldName('index'); | |
| const indexType = index?.type; | |
| if (indexType !== 'string' && indexType !== 'template_string') return false; | |
| const rawIndex = node.childForFieldName('index'); | |
| const index = rawIndex ? unwrapParens(rawIndex) : undefined; | |
| const indexType = index?.type; | |
| if (indexType !== 'string' && indexType !== 'template_string') return false; |
Three new under-escape findings, all fixed this round per the standing
rule (docs/plans/issue-2088.md):
- finding 1: a non-computed `__proto__` pair sets the table's own
[[Prototype]] (ECMA-262 Annex B.3.1) rather than an ordinary own
property, so a method reached through it binds `this` to the table
directly, with none of the extra property hop
isPositivelyThisFreeLiteral's object/array arms rely on. Fixed by a
caller-side, key-shape check in literalHasUnmodeledThisReference,
ahead of any value-shape reasoning; a computed ['__proto__'] key is
deliberately excluded, since it creates an ordinary own property and
is not given special meaning by the spec.
- finding 2: allReferencesTracked's for-of recursion reused the outer
call's declaringScope unconditionally for a var-kind loop variable,
the same gap round 18 closed for the rebinding recursion but did not
close here; round 18's own essay asserted a for-of loop variable was
"always block-scoped" without checking, which is false for `var`.
Fixed by widening the for-of recursion's own boundary the identical
way for a var-kind head, and the false parenthetical is corrected.
- finding 3: allReferencesTracked's reference-matching walk matched
`identifier` nodes only, so a binding forwarded by shorthand property
(`sink({ T })`) was invisible to the walk entirely rather than
classified untracked. Fixed by widening the filter to also match
shorthand_property_identifier; a pair's own property_identifier key
is deliberately excluded, since a key is never itself a
value-producing reference.
Also, applying the same ablation discipline to round 18's own
fixtures rather than only to this round's new ones:
- correlation shape 17 (the for_statement/malformed-using guard,
rebuilt round 18) was still vacuous after ablating the disjunct it
claims to guard: its other property resolved to a handler declared
only in a nested scope, which independently fails safe regardless of
the disjunct. Rebuilt again so the disjunct is the only thing that
can make the site escape.
- correlation shape 19's EXPECT asserted `escapes = 0` for a literal
containing a getter, contradicting this design's own U2 rule (a
get-flavoured method_definition escapes unconditionally) and its own
Success Criteria. Corrected to `escapes = 1`, and its prose ("each
property is still judged on its own shape") is corrected to reflect
that literalHasUnmodeledThisReference is a whole-literal predicate.
Adds a new standing rule alongside the existing fail-closed,
direction-label, and fixture-parse rules: every fix must be shown
load-bearing by ablation (removing it must flip its own
escape-fallback case from 1 to 0), and every guard/correlation shape
must flip the opposite way when the fix it guards is removed. This is
how both fixture defects above were found.
Non-blocking cleanup while in there:
- subtreeContainsThisKeyword is exactly as blind to
eval('this.alpha()') as subtreeContainsReassignmentOf already
discloses being to eval('name = fn'); the Category F acceptance is
now stated against both consumers, not only one.
- resolveIdentifierValueThisReference compared declaringScope to root
by object identity instead of by .id, the one node-identity
comparison in this file that did not follow the established
convention; corrected for consistency, no behavior change.
Files GH issue #2640 for a related, deliberately-not-fixed gap: a
classic-script `globalThis.T.alpha()` read is invisible to
allReferencesTracked's walk the same way the pre-round-16 write side
was. This is under-escape in direction and does not fit the plan's own
#2610-style exception (allReferencesTracked is new machinery this plan
introduces, and no other condition already bounds the cost) — recorded
in Out of Scope as a flagged departure from the DIRECTION-labels
standing rule pending explicit human sign-off, not as a quiet
exception to it.
All three findings, both fixture corrections, and the .id consistency
fix are mirrored in WU-7's Rust section one-for-one, with their own
parity-risk paragraph; the Testing Strategy, Risks & Mitigations, and
Success Criteria sections are reconciled to match. No shared primitive
(patternBindsName, introducesShadowedBinding, SCOPE_NODE_TYPES) is
widened by any of this.
|
@greptileai please re-review. Round 19 closes three further under-escape gaps found by re-auditing WU-2's condition 3/4 machinery, rebuilds one of round 18's own fixtures that turned out to still be vacuous, corrects one of round 18's own fixtures whose expectation contradicted this design's own rule, and adds a new standing rule (ablation verification) that is how both of those fixture defects were actually found.
All three findings, both fixture corrections, and the |
1 participant
Part of #2088. Docs-only — this PR adds a delivery plan, no product code. Merging it does not complete the issue; only the execute PR does.
Plan doc:
docs/plans/issue-2088.mdWhat the issue asks for
collectInvokedPropertyNames(src/domain/graph/builder/call-resolver.ts:91) reduces to:Any non-empty receiver anywhere in the processed file set credits the bare property name. So a
promise.resolve()in an unrelated file keeps{ resolve: neverCalled }from being flagged dead. Same in the native mirror,collect_invoked_property_names(crates/codegraph-core/src/domain/graph/builder/stages/build_edges.rs:861).Confirmed still present on
main@6221df16.This is the conservative error direction — a false negative for dead-code detection, never a misclassification of live code as dead. Nothing downstream produces wrong results today. This is a recall improvement to the advisory
roles --role deadcommand, not a soundness fix.The approach in one paragraph
Give every object literal a stable allocation-site identity, teach the existing Andersen points-to solver to propagate those sites into receiver variables, and credit a property as live only when a receiver that provably points at that literal invokes that key. Gate the whole thing on an escape check: a site whose identity can leave what the solver models keeps today's exact predicate. That is what stops the fix from converting a conservative false negative into a false positive.
The resolver ends up with a three-tier ladder:
site|keyT1 being exclusive rather than ORed with T2 is what produces the recall gain; the
escapesguard is what keeps it safe. Both are argued in the plan.The §8.3 tension, addressed head-on
The dispatch brief flagged a possible conflict with ROADMAP §8.3, whose approach is explicitly field-based, not field-sensitive — "treat all instances of
obj.fieldas the same abstract location regardless of whichobjinstance".There is no conflict: field sensitivity and allocation-site abstraction are orthogonal axes. Field sensitivity is about how fields are abstracted; allocation-site abstraction is about how objects are. §8.3's own Approach block already commits to the latter, in the bullet directly below the field-based one:
and §8.3's single remaining unchecked item is "Full allocation-site abstraction and constraint solver". So this plan delivers a slice of §8.3's own open item rather than deviating from it. The pts lattice stays field-based; the
site|keyevidence set is computed outside the solver, which never learns about fields. The one real extension — §8.3's allocation-site bullet does not mention object literals — is a roadmap text update in WU-9b.ADR compliance
src/domain/graph/resolver/points-to.tsand the Rustbuild_points_to_map. No new subsystem; the 50-iteration solver loop (buildCallSiteTypeMap/MAX_SOLVER_ITERATIONS) is untouched.wasm-worker-{protocol,entry,pool}.tsseam the primary parity-divergence risk, so it is its own work unit (WU-3) with its own verification. NoteCall.objectLiteralSiteneeds no protocol edit —SerializedExtractorOutput.callsis typedCall[]and passed whole (wasm-worker-protocol.ts:51); only top-levelExtractorOutputextras need explicit threading. Verified by reading the file, not assumed.pts-javascript; thejavascriptfixture's precision-1.0 floor must not move.domain/graph/resolver/directory — the Rust solver lives insidebuild_edges.rs, its pre-existing mirror location.Building on prior art, not duplicating it
collectObjectLiteralValueRefCallalready sets a value-refCall'sreceiverto the dispatch table's name, feedingcomputedDispatchTableEvidence(#2260). That is a name-correlated evidence channel and it is kept exactly as-is as T3. #2088 adds a site-correlated tier beside it. The array-literal gap in #2260's own channel is filed separately (see below) rather than folded in.Shape of the work
10 work units. Critical path is
WU-1 → WU-2 → WU-7 → WU-8 → WU-10 → WU-9b— the bottleneck is the Rust chain, since WU-7 is a line-for-line mirror that should not start until the TS escape analysis is settled, and WU-10 cannot start until both engines are done because half of what it asserts is that they agree.DB: migration v32 (current latest is v31) adds
object_literal_sitesandinvoked_property_sites, both persisted and purged per-file exactly asinvoked_property_names(#2087) is — deliberately not the in-memory-only shortcut #2260 took.Config: exactly one new
DEFAULTSkey,analysis.correlatedPropertyEvidence. Setting itfalserestores pre-#2088 behavior exactly. No new language, noLANGUAGE_REGISTRY/AST_TYPE_MAPS/LangAstConfigchange, no new runtime dependency.What no test can prove — reviewer attention needed here
The escape analysis is a judgment about completeness. The tests prove the recognised shapes are classified correctly and that the fail-safe default is
escapes: true; they cannot prove the recognised set is exhaustive.A human reviewer must read
computeObjectLiteralSiteEscapes(WU-2b) and its Rust mirror againstTRACKED_REFERENCE_PARENTSand confirm every position not in that set is genuinely treated as an escape. That review is the real gate on the plan's soundness requirement. This is called out explicitly in the plan's Testing Strategy rather than left implicit.Nine existing tests form the regression contract and must pass unedited — notably
issue-1895-value-ref-invocation-check, whose fixture literal is returned from an exportedmakeTable()and therefore escapes and resolves on T2, i.e. today's exact path.Out of scope — filed, not carried in prose
computedDispatchTableEvidenceis in-memory only, so a scoped incremental build can report a live dispatch-table property dead. Non-conservative direction, and a full-vs-incremental divergence. Its sibling channel got a durable table in follow-up: persist cross-file invoked-property-name evidence for incremental dead-code classification #2087 for exactly this reason.findEnclosingTableNamedoes not traverse array literals, soconst RESOLVERS = [{ matches, resolve }]yields noreceiverand the Computed-property (bracket-access) dispatch-table lookups lack a real calls edge, unlike dot-property value-refs #2260 pathway can never credit a handler array — the exact idiom named incollectObjectLiteralValueRefCall's own doc comment as Dispatch-table function references (resolve: fn) inconsistently flagged dead-unresolved depending on unrelated fanOut #1771's motivating case. Not closed by this plan, which leaves T3 name-keyed.-Tunder-filterstests/. Relevant only because the plan's dogfood measurement must filtertests/by hand rather than trust the raw dead-symbol count.Plan provenance
Round 1. No
plan-carry-forwardartifact exists on #2088 —gh api .../issues/2088/comments --paginatereturns zero comments carrying the sentinel, from any author, trusted or not. Everything in the plan is derived fresh from live source at6221df16.Verification status of this PR
Docs-only.
npm run lintwas run and passes (Biome is scoped tosrc//tests/, neither touched). It reports 1 pre-existing warning insrc/graph/algorithms/louvain.ts:135(ineffectivebiome-ignoresuppression) — an untouched file, left alone per CLAUDE.md's "don't clean up lint issues in files you aren't working on". Flagging it rather than silently absorbing it.✋ Human approval gate (/oversee)
docs/plans/issue-2088.md81b1be1a(round 15)oversee/plan-gate=successon81b1be1aWhat the audit found
Every round has hunted one property: can this design ever report live code as dead?
Today's
collectInvokedPropertyNamescredits any truthy receiver and structurally cannot,so the plan's whole safety argument is that it never converts that conservative false
negative into a false positive.
Along the way the plan accumulated twenty tracked "exclusion" issues, and the review
process had been treating "documented in the plan and tracked in an open issue" as making a
residual gap acceptable. That criterion was wrong, and auditing it by direction rather
than by disclosure is what surfaced the problem:
An under-escape gap is not a recall trade-off. It is the exact failure class this plan calls
"a regression against today" every time it has fixed one inline — which it did in rounds
13, 14, and 15. Fixing three instances while filing four more as accepted limitations is
self-inconsistent, and no issue number makes a false positive acceptable.
Three of the four were misfiled. #2630's own issue is headed "why it's the safe
direction" — true for one of its three consumers, false for both condition-4 consumers.
#2636 claims fail-safety on reasoning that only covers the all-generator case, and the plan
repeats that non-sequitur verbatim. #2634 sits alongside genuine fail-safe trade-offs but is
a missed write. #2632 is correctly diagnosed in its own issue as "confidently wrong" —
and is absent from the plan entirely.
Where the design actually stands
Two axes have been enumerated rather than probed case-by-case:
the plan's own walk logic. Closed, and re-verified across three independent rounds.
differ from its declaration. All classified; the remaining unsound ones are the four above.
Two structural rules came out of this and are why the findings narrowed from architectural
to mechanical: a non-vacuous coverage rule (a walk must be proven exhaustive or it
fails closed) and a fail-closed contract at the escape decision itself (every predicate
must return "escaping" for any shape it does not positively recognise, inherited
automatically by predicates added later).
What the plan gets right
ADR-002's "no new subsystem" binding is honoured — the Andersen solver loop is untouched.
Dual-engine parity is named for every added function. One
DEFAULTSkey, no stray magicnumbers, clean v32 migration. The correlation tests genuinely discriminate: shape 1 asserts a
symbol dead against a cross-file same-named decoy, an outcome the bare-name fallback
structurally cannot produce, and shape 8 proves a tightening did not over-escape rather than
merely asserting it. All 37 escape-fallback cases assert the escaping outcome rather than
certifying a bug.
The scope question, which is yours and not the reviewers'
Setting soundness aside, this plan is now considerably narrower than its first draft, and
says so. T1 correlation fires for a deliberately small core. The remaining recall gain, on an
advisory command (
roles --role dead), has to be weighed against 10 work units, a dual-enginemirror, and a v32 DB migration. That trade is a judgement no critic can make for you.
Reviewing this plan also turned up #2628 — a suspected bug in shipped product code,
unrelated to #2088.
Review the plan above. To approve it for execution, tick this box, then run
/oversee #2612: