-
Notifications
You must be signed in to change notification settings - Fork 15
fix(resolver): require invocation evidence for object-literal value-ref liveness #2034
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
2b6a00d
b28051a
17fbcba
26918bb
be6432c
4ead840
f6326bf
590f560
d3ea4a4
c306812
8bb5893
46037b1
6a84cf9
8d936d1
11c84be
963301a
3e8035d
879635e
0fe9fc3
ccf3c19
d5b1162
8971017
056c5e4
0738171
6f2423c
4c02378
0698e16
4ac3b99
6767f09
5f6f30a
b479318
c39ac40
067674f
f6807b4
1c07a54
bf82aa2
cd02d27
980b5dc
4a873a3
8f3348a
acf804c
986c851
71e9174
d2935cc
6515186
30c491a
9db911b
70559fc
822e0eb
fcdfe7d
ee6362c
d4e6212
aec5d2b
d250f8d
75978e2
af17b6d
dd447bc
a79a8a1
d84f6b5
e7a2255
b54972a
1821a8b
d45aba7
69d8a6f
db08219
79cd761
cbfa954
c0e7eae
2dabe21
b1eb9af
4a1e279
40b5e24
1506896
2dfd6bc
ccea0cb
93ccfa3
e105c26
5d99bf0
e0ab9dc
730b49b
0d7e8e6
c5b9a34
2de6550
e5fe754
4eef28e
4090355
bf8e969
51b9e23
a62bab7
8f52d28
59220a5
2e1aa11
3d436ca
668451c
4bccd4b
25ba42f
431c0e5
ab6a685
17e265b
fdedff8
6ef1731
afca17f
801d290
a45e74d
453a768
554efbf
ef6492c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -31,6 +31,7 @@ import { | |
| } from '../resolver/points-to.js'; | ||
| import { | ||
| type CallNodeLookup, | ||
| collectInvokedPropertyNames, | ||
| findCaller, | ||
| isModuleScopedLanguage, | ||
| resolveCallTargets, | ||
|
|
@@ -1044,6 +1045,10 @@ function buildCallEdges( | |
| // JS path uses (buildPointsToMapForFile, shared via resolver/points-to.js). | ||
| const ptsMap = buildPointsToMapForFile(symbols, importedNames); | ||
| const fnRefBindingLhs = new Set(symbols.fnRefBindings?.map((b) => b.lhs) ?? []); | ||
| // #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]); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Verified this is a genuine, accepted trade-off consistent with how this exact incremental-classification subsystem already works — not a hand-wave. I independently checked the two precedents this PR's own doc comment cites ( While verifying this, I did find and fix one real inaccuracy in the doc comment itself: it also cited "median fan-in/out" as a third example of this scoping trade-off, but that's backwards — Given the underlying trade-off is real and consistent with existing convention, I'm accepting it as-is rather than building a persistent cross-file index in this PR (that would be a meaningfully larger, separate change — a new persisted evidence table with incremental insert/prune logic on both engines, not a small tweak). Tracked as a follow-up: #2087 |
||
| let edgesAdded = 0; | ||
|
|
||
| for (const call of symbols.calls) { | ||
|
|
@@ -1078,6 +1083,12 @@ function buildCallEdges( | |
| targets = targets.filter( | ||
| (t) => t.kind === 'function' || t.kind === 'method' || t.kind === 'class', | ||
| ); | ||
| // #1895: object-literal-property value-refs additionally require | ||
| // independent evidence the property is actually invoked somewhere — | ||
| // mirrors the same check in resolveFallbackTargets (stages/build-edges.ts). | ||
| if (call.keyExpr && !invokedPropertyNames.has(call.keyExpr)) { | ||
| targets = []; | ||
| } | ||
| } | ||
|
|
||
| edgesAdded += emitIncrementalCallEdges( | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
collectInvokedPropertyNamesadds a property name whenever ANY call in the processed files has a non-emptyreceiverand thatname. There is no check that the receiver is plausibly the same dispatch-table object. A completely unrelatedlogger.resolve()orpromise.resolve()call anywhere in the codebase will addresolveto 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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed this is understood and fine to leave as-is for this PR — making that judgment independently, not just deferring to your framing.
The error direction is the safe one (false negative for dead-code only — a completely unrelated
logger.resolve()/promise.resolve()can prevent a genuinely-dead{ resolve: neverCalled }from being flagged, but this check can never cause genuinely-live code to be misclassified as dead). This PR's own stated scope was "require any invocation evidence at all" — closing the prior gap where an object-literal value-ref got liveness credit with zero invocation evidence whatsoever — not "require precisely-correlated invocation evidence." Tightening this to require the receiver's inferred type/binding to plausibly match the specific dispatch-table object would mean extending the points-to/type-map machinery to correlate receiver call sites back to the originating object literal, implemented identically in both the WASM/TS and native Rust engines for parity — a real dataflow-precision feature, not a small tweak, and meaningfully larger than this PR's actual diff.Tracked as a follow-up for future precision work: #2088