fix(resolver): require invocation evidence for object-literal value-ref liveness#2034
Conversation
…ef liveness
Object-literal property-value references (`{ resolve: someFn }` bare
identifiers and `{ someFn }` shorthand) previously granted a function
liveness merely by existing as a property value, regardless of whether
that property was ever actually invoked (`table.resolve(...)`)
anywhere in the codebase. A dispatch-table entry that was wired up but
never read escaped `roles --role dead` detection as a false negative.
Value-ref calls originating from an object-literal property now carry
the property's key name (Call.keyExpr / CallInfo.key_expr), distinct
from the referenced value's own name. The resolver only honors the
value-ref as a `calls` edge when that key is independently confirmed
to be invoked via member-call syntax somewhere in the files being
processed (full build: the whole codebase; incremental: the current
file), mirrored identically in both the WASM/TS and native Rust
engines.
Impact: 10 functions changed, 32 affected
Greptile SummaryThis PR tightens the dead-code false-negative introduced by the #1771 dispatch-table value-ref mechanism: a function referenced as an object-literal property value (
Confidence Score: 4/5Safe to merge for the full-build path; the incremental path now aggressively drops value-ref edges when the invocation is cross-file, which can surface dead-code findings during watch rebuilds that a subsequent full rebuild will correct. The core fix is correct and well-tested on both engines for the full-build case. The only structural concern is that the incremental TS path scopes
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["Object-literal extractor\n{ resolve: someFn }"] -->|"sets keyExpr = resolve\nname = someFn"| B[Call record\ndynamicKind=value-ref]
B --> C{Full build or\nIncremental?}
C -->|Full build via build-edges.ts| D["collectInvokedPropertyNames\nfrom all files in fileSymbols"]
C -->|Incremental via incremental.ts| E["collectInvokedPropertyNames\nfrom current file only"]
D --> F{"keyExpr in\ninvokedPropertyNames?"}
E --> F
F -->|Yes| G["Keep calls edge\nsomeFn is LIVE"]
F -->|No| H["Drop targets\nsomeFn is DEAD"]
style G fill:#d4edda,stroke:#28a745
style H fill:#f8d7da,stroke:#dc3545
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
A["Object-literal extractor\n{ resolve: someFn }"] -->|"sets keyExpr = resolve\nname = someFn"| B[Call record\ndynamicKind=value-ref]
B --> C{Full build or\nIncremental?}
C -->|Full build via build-edges.ts| D["collectInvokedPropertyNames\nfrom all files in fileSymbols"]
C -->|Incremental via incremental.ts| E["collectInvokedPropertyNames\nfrom current file only"]
D --> F{"keyExpr in\ninvokedPropertyNames?"}
E --> F
F -->|Yes| G["Keep calls edge\nsomeFn is LIVE"]
F -->|No| H["Drop targets\nsomeFn is DEAD"]
style G fill:#d4edda,stroke:#28a745
style H fill:#f8d7da,stroke:#dc3545
Reviews (1): Last reviewed commit: "fix(resolver): require invocation eviden..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| // #1895: scoped to this file's own calls only — see collectInvokedPropertyNames | ||
| // doc comment (call-resolver.ts) for why incremental rebuilds use a narrower, | ||
| // same-file view rather than a full-codebase one. | ||
| const invokedPropertyNames = collectInvokedPropertyNames([symbols.calls]); |
There was a problem hiding this comment.
collectInvokedPropertyNames([symbols.calls]) only sees calls extracted from the single file currently being rebuilt. If factory.js defines { resolve: neverCalled } and the only table.resolve(1) site lives in consumer.js, an incremental rebuild triggered solely on factory.js will not find resolve in invokedPropertyNames and will drop the edge — classifying neverCalled as dead even though it is genuinely reachable. Before this fix the edge was unconditionally kept, so this is a new false-positive direction introduced on the incremental path.
This is documented as the same scoping trade-off accepted elsewhere, and a full rebuild will correct it. Worth confirming the team is comfortable with incremental builds potentially surfacing "dead" findings that disappear on the next full run, since that could affect CI dead-code checks that run on incremental watches.
| export function collectInvokedPropertyNames( | ||
| callsList: Iterable<Iterable<{ name: string; receiver?: string }>>, | ||
| ): Set<string> { | ||
| const names = new Set<string>(); | ||
| for (const calls of callsList) { | ||
| for (const call of calls) { | ||
| if (call.receiver) names.add(call.name); | ||
| } | ||
| } | ||
| return names; |
There was a problem hiding this comment.
collectInvokedPropertyNames adds a property name whenever ANY call in the processed files has a non-empty receiver and that name. There is no check that the receiver is plausibly the same dispatch-table object. A completely unrelated logger.resolve() or promise.resolve() call anywhere in the codebase will add resolve to the set and prevent { resolve: neverCalled } from being flagged dead, even if the actual dispatch table is never read. This is the conservative direction of error (false negative for dead-code), so it won't misclassify live code as dead, but it can cause real dead-code entries to escape detection. Given the existing comment explicitly scopes this as "one hop further" heuristic evidence rather than precise analysis, this is understood — just worth noting for future precision work.
Codegraph Impact Analysis10 functions changed → 30 callers affected across 4 files
|
1 participant
Summary
codegraph roles --role deadtreated a function referenced as an object-literal property value ({ resolve: someFn }bare identifiers,{ someFn }shorthand — the #1771 "value-ref" dispatch-table mechanism) as sufficient evidence of liveness on its own, without checking whether the property is ever actually accessed and invoked (table.resolve(...)) anywhere. A dispatch-table entry that is wired up but genuinely never read escaped dead-code detection as a false negative.Reproduced and confirmed against this repo's own built
dist/cli.js(the published npm binary predates this feature and can't be used to test it — seecrates/codegraph-corestaleness note below):Before this fix,
neverCalledwas classifiedleaf/core(neverdead-*) purely because it appeared asresolve's value — identical treatment torealHandler, which genuinely is invoked. After the fix,neverCalledis correctly classifieddead-unresolved;realHandleris unaffected.Fix
Value-ref calls originating from an object-literal property now carry the property's key name (
Call.keyExpr/CallInfo.key_expr— an existing field, previously only used for reflection-style resolution, unambiguously safe to reuse since every consumer of it is gated ondynamicKind === 'reflection'), distinct from the referenced value's own declared name. The resolver only honors the value-ref as acallsedge when that key is independently confirmed to be invoked via member-call syntax (x.keyExpr(...)) somewhere in the files currently being processed — full builds see the whole codebase; incremental rebuilds see the file(s) being rebuilt (the same scoping trade-off this codebase already accepts elsewhere for incremental classification, e.g.hasActiveFileSiblings/exported-via-reexport/median fan-in inroles.rs's incremental path).Mirrored identically in both engines:
collectObjectLiteralValueRefCall+ shorthand collector (src/extractors/javascript.ts) now setkeyExpr;resolveFallbackTargets(stages/build-edges.ts) andbuildCallEdges(incremental.ts) apply the new check via a sharedcollectInvokedPropertyNameshelper (call-resolver.ts, the module already shared between the full-build and incremental paths).handle_object_literal_pair_value_ref+ shorthand handler (extractors/javascript.rs) now setkey_expr;EdgeContext(build_edges.rs) precomputes the sameinvoked_property_namesset fromfilesandprocess_file's value-ref filter applies the identical check.instanceof/Lua-builtin-reassignment value-refs never setkeyExpr(no property-key concept applies to them), so they're unaffected by this stricter check.Scope note (partial fix — see follow-ups)
This closes the false negative for the literal mechanism named in #1895's title (bare-identifier/shorthand object-literal property values). While investigating, I found the issue's own concrete motivating example (
Partition.deltaCPMetc. from #1770) is actually caused by two different, deeper mechanisms that this PR does not touch:roles --role deaduses non-transitive fan-in: a function called only by another function that is itself unreachable is never flagged dead, regardless of whether that caller is itself dead. Fixing this requires reachability analysis from confirmed-live roots, not a fan-in check — a substantially larger classifier redesign.returned rather than assigned viaconst x = {...}(extractObjectLiteralFunctionsonly handles the latter).Both are filed as separate, correctly-scoped issues per repo convention rather than expanding this PR's diff — closing the loop on the original #1770 concrete case needs all three fixes together. Given the value-ref liveness gap described by #1895's title and suggested-fix-direction is fully and correctly closed here, this PR uses Refs #1895 rather than Fixes #1895, following the precedent set by PR #1943.
Test plan
tests/parsers/javascript.test.tsunit tests forkeyExprcapture (bare identifier, string-literal key, computed string-literal key, non-string computed key, shorthand, instanceof-unaffected) — mirrored in Rust unit tests inextractors/javascript.rs.tests/integration/issue-1895-value-ref-invocation-check.test.ts— full build-through-classification integration test, both WASM and native engines, confirming a never-invoked dispatch-table entry is flagged dead while a genuinely-invoked sibling entry is not.build_call_edgestest (value_ref_edge_requires_key_invoked_elsewhere) directly exercising the new filter.tests/integration/issue-1771-dispatch-table-value-ref.test.ts(the original Dispatch-table function references (resolve: fn) inconsistently flagged dead-unresolved depending on unrelated fanOut #1771 fixture, which genuinely calls.matches(...)/.resolve(...)on its dispatch table) still passes unchanged on both engines — confirming the stricter check doesn't regress legitimately-invoked dispatch tables.npm test: 3969 passed, 0 failed (full suite, both engines).cargo test --release(crate): 591 passed, 0 failed.npm run lint: clean (one pre-existing, unrelated warning in a different fixture file)..nodeaddon locally from this branch's Rust source (the published npm/prebuilt binary predates the Dispatch-table function references (resolve: fn) inconsistently flagged dead-unresolved depending on unrelated fanOut #1771/Lua: function reassigned to global builtin (require = traced_require) flagged as false-positive dead-unresolved #1776/codegraph exports does not credit instanceof ClassName checks as consumers #1784 value-ref mechanisms entirely) to verify native/WASM parity end-to-end.Refs #1895