feat(resolver): resolve this-dispatch inside Object.defineProperty accessor functions (JS)#1346
Conversation
…cessor functions (JS)
When a function is registered as a getter or setter via
Object.defineProperty(obj, "bar", { get: getter }), `this` inside that
function refers to the target object. This commit tracks that binding and
resolves this.method() calls against the registered object's type.
Closes #1335
|
Claude finished @carlos-alm's task —— View job PR Review: feat(resolver): resolve this-dispatch inside Object.defineProperty accessor functions (JS)
Root Cause AnalysisProblem Being Solved: When a function is registered as a Why This Approach: The approach is correct and consistent with existing patterns — it mirrors Risk Assessment: Low. The feature is additive; the new Phase 2: Configuration Change DetectionNo configuration, linting, TypeScript, or test threshold changes. ✓ Phase 3: Test IntegrityThe test fixture and expected-edges are correctly updated — a new fixture block is added to The benchmark test file adds Phase 4: Code Quality
The function is clean, but there is one subtle concern: the inner // line 1473-1475
for (let i = 0; i < node.childCount; i++) {
walk(node.child(i)!, depth + 1); // ← ! on potentially null child
}Existing walks throughout
The lookup strategy at lines 778–784 tries a typeMap lookup first, then falls back to same-file name lookup. The same-file fallback (line 782–784) is quite broad: it returns any definition named The WASM path in
The post-pass uses
The guard
...(symbols.definePropertyReceivers?.size
? { definePropertyReceivers: Array.from(symbols.definePropertyReceivers.entries()) }
: {}),Correct — conditionally includes the tuple array only when non-empty. ✓
Well-documented JSDoc comment, consistent with other optional fields. ✓
Correct tuple-array wire format, consistent with other Maps (e.g. Phase 5: Backlog Compliance
Critical Concerns
Final Recommendation
|
Greptile SummaryAdds
Confidence Score: 5/5Safe to merge — the change is additive, both resolution paths are gated on The WASM and native post-pass paths mirror each other correctly, serialization through the worker boundary is consistent with the existing No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["AST Walk\nextractDefinePropertyReceiversWalk"] -->|"funcName → receiverVar\nMap<string,string>"| B["ExtractorOutput\n.definePropertyReceivers"]
B -->|WASM path| C["serializeExtractorOutput\nMap → Array<[string,string]>"]
C --> D["SerializedExtractorOutput\nwasm-worker-protocol"]
D --> E["deserializeResult\nArray → Map<string,string>"]
E --> F["buildFileCallEdges\nWASM path inline fallback"]
B -->|Native path| G["buildDefinePropertyPostPass\nJS post-pass"]
F -->|"this.X() with no prior target"| H{"typeMap has\nreceiverVar type?"}
G -->|"this.X() with no prior target"| H
H -->|Yes| I["lookup.byNameAndFile\ntypeName.methodName"]
H -->|No| J["lookup.byNameAndFile\ncall.name same-file fallback"]
I -->|found| K["Emit edge\ncaller → target"]
I -->|not found| J
J --> K
Reviews (9): Last reviewed commit: "Merge remote-tracking branch 'origin/mai..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if (typeName) { | ||
| const qualifiedName = `${typeName}.${call.name}`; | ||
| const qualified = lookup.byNameAndFile(qualifiedName, relPath).filter(() => true); | ||
| if (qualified.length > 0) { | ||
| targets = qualified; | ||
| } | ||
| } | ||
| // If still no targets, search for any definition named `call.name` in | ||
| // the same file — handles plain object literals where the method isn't | ||
| // qualified (e.g. `const obj = { baz() {} }` defines `baz` directly). | ||
| if (targets.length === 0) { | ||
| const sameFile = lookup.byNameAndFile(call.name, relPath).filter(() => true); | ||
| if (sameFile.length > 0) { | ||
| targets = sameFile; | ||
| } | ||
| } |
There was a problem hiding this comment.
.filter(() => true) is a no-op — it always returns every element unchanged. It was likely copy-pasted from the same-class fallback above which uses .filter((n) => n.kind === 'method'). Leaving these in place makes the code misleading and could confuse a future reader into thinking an intentional filter criterion was meant to be here.
| if (typeName) { | |
| const qualifiedName = `${typeName}.${call.name}`; | |
| const qualified = lookup.byNameAndFile(qualifiedName, relPath).filter(() => true); | |
| if (qualified.length > 0) { | |
| targets = qualified; | |
| } | |
| } | |
| // If still no targets, search for any definition named `call.name` in | |
| // the same file — handles plain object literals where the method isn't | |
| // qualified (e.g. `const obj = { baz() {} }` defines `baz` directly). | |
| if (targets.length === 0) { | |
| const sameFile = lookup.byNameAndFile(call.name, relPath).filter(() => true); | |
| if (sameFile.length > 0) { | |
| targets = sameFile; | |
| } | |
| } | |
| if (typeName) { | |
| const qualifiedName = `${typeName}.${call.name}`; | |
| const qualified = lookup.byNameAndFile(qualifiedName, relPath); | |
| if (qualified.length > 0) { | |
| targets = qualified; | |
| } | |
| } | |
| // If still no targets, search for any definition named `call.name` in | |
| // the same file — handles plain object literals where the method isn't | |
| // qualified (e.g. `const obj = { baz() {} }` defines `baz` directly). | |
| if (targets.length === 0) { | |
| const sameFile = lookup.byNameAndFile(call.name, relPath); | |
| if (sameFile.length > 0) { | |
| targets = sameFile; | |
| } | |
| } |
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 — removed both .filter(() => true) no-ops from the defineProperty WASM path in buildFileCallEdges. The qualified-name lookup and same-file fallback now call lookup.byNameAndFile() directly without filtering.
There was a problem hiding this comment.
Fixed — but used spread syntax ([...qualified] and [...sameFile]) rather than direct assignment. The original no-op .filter() was inadvertently serving as a ReadonlyArray-to-mutable-Array conversion: .filter() on a ReadonlyArray returns a new mutable Array. Removing it caused TS4104 errors at both lines (ReadonlyArray cannot be assigned to the mutable targets). Spreading into a new array is the clean fix.
| val?.type === 'identifier' && | ||
| !BUILTIN_GLOBALS.has(val.text) | ||
| ) { | ||
| out.set(val.text, targetName); |
There was a problem hiding this comment.
out.set(val.text, targetName) silently overwrites a previous entry if the same function is passed as an accessor to multiple Object.defineProperty calls (e.g. Object.defineProperty(obj1, 'a', { get: getter }) followed by Object.defineProperty(obj2, 'b', { get: getter })). Only the last binding is remembered, so this-calls inside getter would be resolved against the wrong object for the earlier registration. This is an unusual pattern, but given no warning is emitted or comment acknowledging the limitation, it may produce silently wrong edges in production codebases.
There was a problem hiding this comment.
Acknowledged with a comment. A proper fix (allowing multiple receiver bindings per accessor function) would require changing Map<string, string> to Map<string, string[]> and updating all consumers. Since this is an unusual pattern (sharing one function across multiple Object.defineProperty calls), the trade-off isn't worth it for this PR. Added an inline comment documenting the limitation so future readers are aware.
| // Same-file fallback for plain object-literal methods | ||
| if (targets.length === 0) { | ||
| targets = lookup.byNameAndFile(call.name, relPath); | ||
| } |
There was a problem hiding this comment.
When the typeMap holds no type annotation for the receiver variable, both the WASM path (buildFileCallEdges) and the native post-pass (buildDefinePropertyPostPass) fall back to lookup.byNameAndFile(call.name, relPath), which returns every definition named call.name in the file — not just members of the receiver object. In a file that has a top-level function sharing a name with a method on the accessor target, both would be emitted as call targets. The test fixture avoids this collision, but the JavaScript benchmark precision floor is held at 1.0, so any future fixture that triggers this path would immediately break CI.
There was a problem hiding this comment.
This is an intentional design decision consistent with the existing WASM path. Added a comment to buildFileCallEdges explicitly documenting that the same-file fallback is intentionally broad and mirrors the buildDefinePropertyPostPass behaviour. The JS benchmark precision floor at 1.0 will catch any future fixture regression caused by name collisions.
Codegraph Impact Analysis13 functions changed → 20 callers affected across 8 files
|
|
Addressed all three items from the review:
|
|
Claude finished @carlos-alm's task —— View job Follow-up Review: Addressing Previous Feedback
All three items from the previous review are addressed in commit Item 1 — Null guard ( for (let i = 0; i < node.childCount; i++) {
const child = node.child(i);
if (child) walk(child, depth + 1);
}Matches the pattern used throughout the rest of the file. Item 2 — No-op Item 3a — Last-write-wins comment ( Item 3b — Same-file fallback breadth comment ( Rating: ⭐⭐⭐⭐½ (4.5/5) — All critical items resolved. The one remaining minor item (technique tag Recommendation: APPROVE — the code is clean, the fixes are correct, all 171 benchmark tests pass, and the review feedback was handled appropriately. |
…accessor-this-dispatch-1335
…y fallback (#1346) Assigning lookup.byNameAndFile() result (ReadonlyArray) directly to the mutable targets variable caused TS4104 type errors after the no-op .filter() was removed. Spread into new arrays with [...qualified] and [...sameFile].
|
Fixed TypeScript type error introduced by removing the no-op .filter() calls:
|
…is-dispatch-1335' into feat/define-property-accessor-this-dispatch-1335
|
Fixed the edge count comment — updated the JS expected-edge count from 33 to 34 to account for the new |
…accessor-this-dispatch-1335
|
Merged origin/main (docs/roadmap update from #1338) to bring the branch fully up to date. CI re-triggered. Note on the duplicate: PR #1351 (feat/defineProperty-accessor-this-dispatch-1335) is a successor with additional fixes for object literal scope guards, Rust native extractor improvements, and jelly-micro fixture coverage. Both PRs target the same feature but #1351 has a broader implementation. The original functionality in this PR (#1346) is correct and all review concerns are addressed. |
1 participant
Summary
Object.defineProperty(obj, "bar", { get: getter })patterns during AST walking, recording thatgetterhasobjas itsthisreceiverbuildFileCallEdges(WASM path): whenthis.method()has no targets and the caller function is a defineProperty-registered accessor, resolves through the receiver objectbuildDefinePropertyPostPass(native path): post-pass that adds the same edges for the Rust enginedefinePropertyReceiversthrough the WASM worker protocol (SerializedExtractorOutput) so it survives the worker-thread boundarygetter → baztest edge to the JS benchmark fixture via thedefine-propertymodeCloses #1335
Test evidence
Resolution benchmark:
define-property: 1/1 (100% recall)—getter → baznow resolves viathis.baz()withthis === accessorTargetAll 171 benchmark tests pass.