fix(extractors): recognize cross-file ES6 getter/setter accessor reads - #2252
Conversation
Sorry, something went wrong.
#1893 fixed bare (non-call) property reads on ES6 get/set accessors (`obj.isReady`, no call parens) never producing a `calls` edge, but only for the same-file case: the accessor's declaring class had to be declared in the same file as the read site, since that was the only way to confirm at extraction time that a bare property read really targets an accessor rather than an unrelated same-named method/field. Adds an `accessor_kind` ('get'/'set'/NULL) column on `nodes`, set on every method-kind node that's an ES6 accessor declaration. A property read whose receiver type isn't declared in the reading file now still emits a candidate call, tagged with the accessor kind it needs (`accessorRead`) and the resolved class name as its receiver. `resolveCallTargets` (and the native `resolve_call_targets_core` mirror) short-circuits on this tag: it resolves only against a candidate whose `accessor_kind` matches exactly, bypassing the usual directory-proximity confidence gate (kind-plus-exact-name match is a strictly stronger signal), and drops the call outright when nothing matches rather than falling through to an unrelated same-named non-accessor declaration. Also recognizes `instanceof` narrowing (`if (x instanceof Y) { x.prop }`, including `&&` chains) so a receiver's declared type doesn't shadow a more specific narrowed type when the accessor lives only on the narrowed subclass — the shape of the issue's own repro. The same-file case (#1893) is unchanged: a same-file-confirmed call still flows through untagged, and the `this` receiver (whose class is always same-file) never takes the cross-file path. Fixes #2030 Impact: 21 functions changed, 68 affected
Greptile SummaryThe PR adds cross-file ES6 accessor-read recognition across the TypeScript/WASM and Rust/native graph-building paths.
Confidence Score: 5/5The PR appears safe to merge. The previously reported aliasing and global-source-scoping failures are addressed in both resolver implementations, with renamed imports looked up under their local binding while qualified names use the original declaration name; no blocking failure remains. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart LR
Source["JavaScript / TypeScript source"] --> Extract["Extract accessor declaration and property read"]
Extract --> Persist["Persist node accessor_kind"]
Extract --> Tag["Tag cross-file call with accessorRead and receiver class"]
Persist --> Resolve["Resolve qualified Class.property in imported defining file"]
Tag --> Resolve
Resolve --> Filter["Filter by get/set kind"]
Filter --> Edge["Persist calls edge"]
Reviews (5): Last reviewed commit: "merge: bring branch up to date with main" | Re-trigger Greptile |
Sorry, something went wrong.
| if (call.accessorRead && call.receiver) { | ||
| const targets = lookup | ||
| .byName(`${call.receiver}.${call.name}`) | ||
| .filter((n) => n.accessorKind === call.accessorRead); | ||
| return { targets: [...targets], importedFrom: undefined }; |
There was a problem hiding this comment.
Imported accessor aliases miss targets
When a receiver is typed, constructed, or narrowed through a renamed import such as Original as Alias, extraction records Alias and this branch looks up Alias.prop instead of the persisted Original.prop, causing the expected calls edge to be silently omitted.
Knowledge Base Used:
Sorry, something went wrong.
There was a problem hiding this comment.
Fixed in 371bc73 (de-alias via importedOriginalNames before building the qualified lookup key, mirroring the existing #1730 general-cascade behavior) and ee6b886 (correct which key importedNames is looked up under — see reply below). Scoped the lookup to the class's own imported file via byNameAndFile rather than the unscoped global byName. Added regression tests on both engines (tests/unit/call-resolver.test.ts, native build_edges.rs call_edge_tests) covering the renamed-alias case with a competing same-qualified-name node in an unrelated file.
Sorry, something went wrong.
| const targets = lookup | ||
| .byName(`${call.receiver}.${call.name}`) | ||
| .filter((n) => n.accessorKind === call.accessorRead); | ||
| return { targets: [...targets], importedFrom: undefined }; |
There was a problem hiding this comment.
Global lookup creates false edges
When separate modules declare the same qualified accessor name, this global lookup returns every matching accessor without using the receiver import's source file, causing the property read to gain calls edges to unrelated declarations.
Knowledge Base Used:
Sorry, something went wrong.
There was a problem hiding this comment.
Fixed alongside the above in 371bc73: the qualified lookup now prefers lookup.byNameAndFile(qualified, importedFrom) when the class is a known import, committing to that specific file (an empty/wrong-kind result there is authoritative, not a cue to keep searching globally) — only falling back to the unscoped byName when the class isn't a known import at all. Regression test: prefers the file the class is imported from over an unrelated same-named global match in call-resolver.test.ts, and the Rust mirror cross_file_accessor_read_prefers_imported_file_over_unrelated_same_named_global_match.
Sorry, something went wrong.
Codegraph Impact Analysis16 functions changed → 68 callers affected across 11 files
|
Sorry, something went wrong.
Two false-positive/negative risks in the #2030 accessor-read short-circuit, flagged by review: - The resolved class name can itself be a renamed import binding (`import { Original as Alias }`), so the tagged call's receiver may be the local alias, not the name the accessor's own file declares it under. De-alias via importedOriginalNames before the qualified lookup, mirroring the general cascade's existing #1730 behavior. - The qualified lookup was unscoped (global byName), so two unrelated files that happen to declare the same `ClassName.prop` accessor (same kind) would both "confirm" a read that only targets one of them. When the class is a known import, resolution now commits to that specific file (byNameAndFile) instead of falling through to the global map — an empty or wrong-kind result there is authoritative ("no"), not "keep looking elsewhere". The unscoped global lookup is now reserved for classes that aren't a known import in the reading file at all. Mirrored in both engines (call-resolver.ts, build_edges.rs's resolve_call_targets_core), which required threading the file's imported_names map into the Rust resolver alongside the existing imported_original_names parameter. Impact: 1 functions changed, 15 affected
| // wrong-kind) match here means "no", not "keep looking elsewhere" — the | ||
| // unscoped global fallback below is reserved for when the class isn't a | ||
| // known import in this file at all (e.g. an ambient/global type). | ||
| const accessorImportedFrom = importedNames.get(dealiasedClassName); |
There was a problem hiding this comment.
Renamed imports bypass source scoping
When an accessor receiver uses a renamed import such as SqliteRepository as SR, this lookup uses the de-aliased name even though importedNames is keyed by SR. The lookup therefore falls through to the global name index, causing unrelated accessors with the same qualified name to gain false calls edges.
Knowledge Base Used:
Sorry, something went wrong.
There was a problem hiding this comment.
Good catch — 371bc73's fix looked up importedNames under the de-aliased name, but importedNames is keyed by the local alias as written in the import statement, so it always missed for a renamed import and fell through to the unscoped global lookup. Fixed in ee6b886: look up importedNames using the original (possibly-aliased) receiver text, and use the de-aliased name only for the qualified lookup key itself. Strengthened the existing alias test (and added a new one) to include a competing same-qualified-name node in an unrelated file, so this class of regression can't pass silently again — mirrored in the native build_edges.rs test too.
Sorry, something went wrong.
…ased name
The previous fix's import-scoping lookup (importedNames.get(dealiasedClassName))
missed for every renamed import: importedNames is keyed by the local
binding as written in the file's own import statement (e.g. 'Alias' for
import { Original as Alias }), not the de-aliased original name. Looking
it up under the de-aliased name always returned nothing and silently fell
through to the unscoped global lookup — reintroducing the exact
same-qualified-name-in-a-different-file false edge the scoping was added
to prevent, specifically for the aliased-import case.
Look up importedNames using the original (possibly-aliased) receiver
text instead, and use the de-aliased name only for the qualified lookup
key itself. Mirrored in both engines. Strengthened both engines' tests
(and added a new one) to include a competing same-qualified-name node in
an unrelated file, so an accidental fallback-to-global no longer passes
silently — the previous versions of these tests didn't populate
importedNames with the alias key at all, so they couldn't have caught
this.
Impact: 1 functions changed, 15 affected
Sorry, something went wrong.
Summary
#1893 fixed bare (non-call) property reads on ES6 get/set accessors (obj.isReady, no call parens) never producing a calls edge, but only for the same-file case: the accessor's declaring class had to live in the same file as the read site, since that was the only way to confirm at extraction time that the read really targets an accessor rather than an unrelated same-named method/field.
This PR covers the cross-file case: the accessor's class is declared in a different file than the read site (the issue's own repro — SqliteRepository.db read from src/features/sequence.ts after instanceof SqliteRepository narrowing).
Approach
The same-file case is unchanged: a same-file-confirmed call still flows through untagged (no accessor_kind filtering needed), and this reads never take the cross-file path since this's class is always declared in the same file.
Because a getter and setter sharing a name are now distinguishable by accessor_kind, this also incidentally fixes ambiguous get+set disambiguation for the cross-file case (same-file get+set pairs keep #1893's original "skip, don't guess" behavior — left unchanged as a separate, deliberately out-of-scope improvement, see follow-up issue below).
Verification
Closes #2030