Skip to content

Commit b0144ea

Browse files
committed
fix: correct self-contradictory value-ref filter docs (docs check acknowledged)
The filter is keyed on dynamicKind, not on which extraction site fired, so adding class for instanceof also extends the #1771 object-literal and #1776 Lua sites to accept class-kind targets. The prior wording claimed those two sites stayed function/method-only, which the code contradicts. Impact: 1 functions changed, 3 affected
1 parent 9bb778a commit b0144ea

4 files changed

Lines changed: 26 additions & 20 deletions

File tree

crates/codegraph-core/src/domain/graph/builder/stages/build_edges.rs

Lines changed: 8 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -791,13 +791,14 @@ fn process_file<'a>(
791791
// of these positions is as likely to be a plain data reference
792792
// (`{ name: SOME_CONSTANT }`) as a real function/class, so drop any
793793
// other-kind match rather than fabricating a "calls" edge to a
794-
// constant. `class` is included alongside function/method because
795-
// `instanceof`'s right operand is always a class/constructor
796-
// (#1784) — unlike the original #1771 object-literal case, which is
797-
// function/method only. Applied once here (after all
798-
// resolve_call_targets tiers), mirroring the
799-
// `dynamicKind === 'value-ref'` filter in resolveFallbackTargets
800-
// (stages/build-edges.ts).
794+
// constant. `class` was added because `instanceof`'s right operand
795+
// is always a class/constructor (#1784). The filter is keyed on
796+
// `dynamic_kind`, not on which site produced the call, so the #1771
797+
// object-literal and #1776 Lua sites also gain class-kind
798+
// resolution as a side effect — not because either idiom commonly
799+
// names a class. Applied once here (after all resolve_call_targets
800+
// tiers), mirroring the `dynamicKind === 'value-ref'` filter in
801+
// resolveFallbackTargets (stages/build-edges.ts).
801802
if call.dynamic_kind.as_deref() == Some("value-ref") {
802803
targets.retain(|t| t.kind == "function" || t.kind == "method" || t.kind == "class");
803804
}

docs/architecture/decisions/002-dynamic-call-resolution.md

Lines changed: 11 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -83,8 +83,10 @@ export type DynamicKind =
8383
// global/builtin identifier (e.g. `require = tracedRequire`,
8484
// #1776), or the right operand of an `instanceof` check
8585
// (e.g. `err instanceof CodegraphError`, #1784) —
86-
// resolvable against function/method-kind targets, plus
87-
// class-kind for the instanceof site
86+
// resolvable against function/method/class-kind targets;
87+
// class was added for instanceof, but the filter is
88+
// per-kind rather than per-site, so all three sites
89+
// share the same allow-list
8890
```
8991

9092
`dynamic?: boolean` is kept to avoid churning every `call.dynamic ? 1 : 0` site.
@@ -106,11 +108,13 @@ is the correct substitute for real alias tracking), and the right operand of
106108
an `instanceof` check (#1784, `err instanceof CodegraphError``instanceof`
107109
evaluates its right operand as a value, never calls it) are three independent
108110
extraction sites feeding the same resolution/filtering logic downstream. The
109-
`instanceof` site is the first to resolve against `class`-kind targets (the
110-
other two are function/method only, since `instanceof`'s operand is always a
111-
class/constructor) — the resolver-side filter is keyed on `dynamicKind`, not
112-
on which site produced the call, so this is a per-kind allowed-target-kind
113-
set rather than a per-site one. New languages/positions can add a fourth
111+
`instanceof` site is the motivation for adding `class` to the allowed
112+
target kinds, since its operand is always a class/constructor — but the
113+
resolver-side filter is keyed on `dynamicKind`, not on which site produced
114+
the call, so this is a per-kind allowed-target-kind set rather than a
115+
per-site one: object-literal and Lua value-ref sites also gain class-kind
116+
resolution as a side effect of this change, not because either idiom
117+
commonly names a class. New languages/positions can add a fourth
114118
without touching `build-edges.ts` / `incremental.ts` / `build_edges.rs`
115119
again, beyond widening the allowed-target-kind set if the new site's operand
116120
isn't a function/method/class.

src/domain/graph/builder/stages/build-edges.ts

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1378,11 +1378,12 @@ function resolveFallbackTargets(
13781378
// these positions is as likely to be a plain data reference
13791379
// (`{ name: SOME_CONSTANT }`) as a real function/class, so drop any
13801380
// other-kind match rather than fabricating a "calls" edge to a constant.
1381-
// `class` is included alongside function/method because `instanceof`'s
1382-
// right operand is always a class/constructor (#1784) — unlike the
1383-
// original #1771 object-literal case, which is function/method only.
1384-
// Applied once here, after every fallback tier above, so it covers
1385-
// whichever tier produced the match.
1381+
// `class` was added because `instanceof`'s right operand is always a
1382+
// class/constructor (#1784). The filter is keyed on `dynamicKind`, not on
1383+
// which site produced the call, so the #1771 object-literal and #1776 Lua
1384+
// sites also gain class-kind resolution as a side effect — not because
1385+
// either idiom commonly names a class. Applied once here, after every
1386+
// fallback tier above, so it covers whichever tier produced the match.
13861387
if (call.dynamicKind === 'value-ref') {
13871388
// `targets` is typed without `kind` when it flows straight through from
13881389
// resolveCallTargets (call-resolver.ts's declared return type omits it),

src/types.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -488,7 +488,7 @@ export type DynamicKind =
488488
| 'reflection' // .call/.apply/.bind / Reflect.* / callable-ref — resolved when target is in codebase; sink edge emitted if unresolved
489489
| 'eval' // eval() / new Function() — undecidable; always flagged
490490
| 'unresolved-dynamic' // any other detected dynamic pattern; flagged
491-
| 'value-ref'; // bare identifier used as a value reference rather than a call site — object-literal property value (dispatch-table pattern, e.g. `{ resolve: someFn }`, #1771), assignment to a Lua global/builtin identifier (e.g. `require = tracedRequire`, #1776), or the right operand of an `instanceof` check (e.g. `err instanceof CodegraphError`, #1784) — resolved only against function/method/class-kind targets (class only relevant for the instanceof site); unresolved (e.g. plain data references) are dropped silently, NOT flagged
491+
| 'value-ref'; // bare identifier used as a value reference rather than a call site — object-literal property value (dispatch-table pattern, e.g. `{ resolve: someFn }`, #1771), assignment to a Lua global/builtin identifier (e.g. `require = tracedRequire`, #1776), or the right operand of an `instanceof` check (e.g. `err instanceof CodegraphError`, #1784) — resolved against function/method/class-kind targets; class was added for instanceof, but the filter is per-kind rather than per-site, so all three sites share the same allow-list; unresolved (e.g. plain data references) are dropped silently, NOT flagged
492492

493493
/** A function/method call detected by an extractor. */
494494
export interface Call {

0 commit comments

Comments
 (0)