feat(resolver): resolve this-dispatch inside Object.defineProperty accessor functions (JS)#1351
Conversation
…cessor functions (JS) Closes #1335 When a function is registered as a `get`/`set` accessor via `Object.defineProperty(obj, key, { get: getter })`, `this` inside `getter` equals `obj`. Codegraph previously emitted no edge for `this.method()` calls inside such accessors when the callee was defined as an object literal property. Three layered mechanisms introduced: 1. **Object literal function extraction** — `const obj = { baz: () => {} }` now extracts `baz` as a named `function` definition (WASM query path via `extractConstDeclarators`, walk path via `handleVariableDecl`, Rust path via `extract_object_literal_functions`). This makes the callee visible to the same-file definition lookup used by `resolveCallTargets` step 2. 2. **Composite typeMap seeding for object literal properties** — `typeMap['obj.baz'] = 'baz'` seeded for shorthand properties (`{ baz }`), identifier values (`{ baz: fn }`), and inline function/arrow values. Enables the accessor receiver path. 3. **Accessor receiver tracking** — `Object.defineProperty(obj, key, { get: getter })` seeds `typeMap['getter:this'] = 'obj'`. `resolveByMethodOrGlobal` (WASM/TS) and `resolve_call_targets` (Rust) check this entry first for plain-function callers (no class prefix), then look up `typeMap['obj.method']` to reach the concrete target — providing precision over the broad same-file fallback. Benchmark impact: - Jelly micro-test `accessors3`: 0% -> 100% recall (TP=1, FP=0, FN=0) - JS fixture `define-property-accessor.js`: new edge `accessorGetter -> accessMethod` - JS benchmark total expected edges: 33 -> 34 (precision 1.0 maintained, recall >= 0.9)
|
Claude finished @carlos-alm's task —— View job PR Review: feat(resolver): resolve this-dispatch inside Object.defineProperty accessor functions
Phase 1: Root Cause AnalysisProblem Being Solved: When a plain function is registered as a Why This Approach: The PR extends the existing
Then, in Risk Assessment: Low-to-medium. The feature is narrowly scoped to Phase 2: Configuration ChangesNo linting rules modified, no thresholds changed, no TypeScript flags disabled. No config files changed. PASS. Phase 3: Test IntegrityNo tests weakened. New fixture + expected-edges.json added. Benchmark test file gets 2 lines adding the new fixture file to the manifest scan — no assertions removed or softened. PASS. Phase 4: Code QualityConcern 1 — Flat-namespace pollution from
When The PR does not scope these definitions (e.g., as Concern 2 — WASM path injects definitions at query time but misses query-path scope guard (LOW)
Concern 3 —
Concern 4 — Rust
Concern 5 —
Correctness — The two-step lookup chain ( Phase 5: Backlog Compliance
Critical ConcernsOnly one concern worth blocking on: The flat-namespace definitions emission from The PR's fixture is minimal (one object, one property, one accessor) so precision stays at 1.0 in the benchmark. However, the benchmark fixture set is small and this effect may not be visible yet. The two-level lookup Recommendation: Either scope the emitted definitions as Final Assessment |
Codegraph Impact Analysis15 functions changed → 43 callers affected across 9 files
|
Greptile SummaryThis PR implements
Confidence Score: 5/5Safe to merge — the change is self-contained, all previous review issues are confirmed fixed, and the JS benchmark precision remains at 1.0 with recall ≥ 0.9. The two-step accessor dispatch is correctly implemented in both resolvers and is guarded against function-scoped objects on both the TS and Rust paths. Qualified definition names prevent global-index pollution. The query path and walk path are mutually exclusive, so no duplicate definitions arise. No files require special attention. Important Files Changed
Reviews (11): Last reviewed commit: "Merge branch 'main' into feat/defineProp..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| // accessor for `obj`, typeMap seeds 'callerName:this' = 'obj'. Resolve this.method() | ||
| // via typeMap['obj.method'] → the concrete definition. Runs before the broad exact-name | ||
| // lookup to avoid false positives from unrelated same-file definitions. | ||
| if call.receiver.as_deref() == Some("this") && !caller_name.contains('.') { |
There was a problem hiding this comment.
caller_name
The Rust guard !caller_name.contains('.') does not exclude empty string, whereas the TypeScript counterpart explicitly checks callerName && !callerName.includes('.'). When caller_name == "", the lookup key becomes ":this". No entry is ever seeded under that key, so no incorrect edge is produced today, but adding the guard makes the parity explicit and removes the latent risk if seeding logic ever changes.
There was a problem hiding this comment.
Fixed — added !caller_name.is_empty() to the Rust guard in edge_builder.rs:442, bringing it to exact parity with the TypeScript check callerName && !callerName.includes('.'). Commit: d2b55b6.
| return undefined; | ||
| } | ||
|
|
||
| /** | ||
| * Phase 8.3f: return the identifier text of a `get` or `set` accessor in a property descriptor. | ||
| * `{ get: getter }` → 'getter'; `{ set: setter }` → 'setter'. | ||
| */ | ||
| function findDescriptorAccessor(desc: TreeSitterNode): string | undefined { | ||
| if (desc.type !== 'object') return undefined; | ||
| for (let i = 0; i < desc.childCount; i++) { | ||
| const pair = desc.child(i); | ||
| if (pair?.type !== 'pair') continue; |
There was a problem hiding this comment.
get/set accessor per descriptor is seeded
findDescriptorAccessor returns on the first matching get or set pair and ignores any remaining accessors. For a descriptor with both { get: getter, set: setter }, only getter:this = obj (or setter:this = obj, depending on parse order) is seeded; the other function receives no this-dispatch hint and falls through to the broader exact-name lookup. The same early-return applies to the Rust counterpart in find_descriptor_accessor. If both functions call this.method(), only one will resolve precisely via Phase 8.3f; the other relies on same-file fallback and may pick up a false positive if the method name is not unique.
There was a problem hiding this comment.
Fixed — renamed findDescriptorAccessor → findDescriptorAccessors (TS) and find_descriptor_accessor → find_descriptor_accessors (Rust) to return all matching get/set entries instead of the first. The caller now iterates all results and seeds a callerName:this = obj entry for each accessor. Commit: 5ef11f6.
| } | ||
| } | ||
|
|
||
| /** | ||
| * Phase 8.3f: extract function/arrow function properties from an object literal as standalone | ||
| * definitions so that `this.method()` calls inside Object.defineProperty accessor functions can | ||
| * resolve them via the normal same-file definition lookup. | ||
| * | ||
| * `const obj = { baz: () => {} }` → emits Definition { name: 'baz', kind: 'function' } | ||
| */ | ||
| function extractObjectLiteralFunctions(objNode: TreeSitterNode, definitions: Definition[]): void { | ||
| for (let i = 0; i < objNode.childCount; i++) { | ||
| const child = objNode.child(i); | ||
| if (!child) continue; | ||
| if (child.type === 'pair') { | ||
| const keyNode = child.childForFieldName('key'); | ||
| const valueNode = child.childForFieldName('value'); | ||
| if (!keyNode || !valueNode) continue; | ||
| const keyName = | ||
| keyNode.type === 'string' ? keyNode.text.replace(/^['"]|['"]$/g, '') : keyNode.text; | ||
| if (!keyName) continue; | ||
| if ( | ||
| valueNode.type === 'arrow_function' || | ||
| valueNode.type === 'function_expression' || | ||
| valueNode.type === 'function' | ||
| ) { | ||
| definitions.push({ | ||
| name: keyName, | ||
| kind: 'function', | ||
| line: nodeStartLine(child), | ||
| endLine: nodeEndLine(valueNode), | ||
| }); | ||
| } | ||
| } else if (child.type === 'method_definition') { | ||
| const nameNode = child.childForFieldName('name'); | ||
| if (nameNode) { | ||
| definitions.push({ | ||
| name: nameNode.text, | ||
| kind: 'function', | ||
| line: nodeStartLine(child), | ||
| endLine: nodeEndLine(child), | ||
| }); | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| function handleEnumDecl(node: TreeSitterNode, ctx: ExtractorOutput): void { | ||
| const nameNode = node.childForFieldName('name'); | ||
| if (!nameNode) return; |
There was a problem hiding this comment.
extractObjectLiteralFunctions registers each function-valued property as a Definition { name: keyName, kind: 'function' } — e.g. const obj = { render: () => {} } adds a definition named render. This name is indexed under nodes_by_name['render'] alongside any top-level function render declarations, so any unqualified call to render() elsewhere in a proximate file will now include this anonymous property as a candidate.
The benchmark precision stays at 1.0 because the fixture properties (baz, accessMethod) are not shared with other call sites. In real-world codebases with common property names (init, update, start, render), this global-index pollution could produce false positive edges. The same issue exists in the Rust counterpart extract_object_literal_functions.
There was a problem hiding this comment.
Fixed — extractObjectLiteralFunctions (TS) and extract_object_literal_functions (Rust) now emit definitions under qualified names (obj.baz instead of bare baz), preventing global-index pollution from common property names. The typeMap value for function/arrow entries is also updated to the qualified name so the Phase 8.3f resolver path calls lookup.byName('obj.baz'). The method_definition branch in Rust also now computes complexity metrics for parity with the pair branch. All tests (jelly-micro, JS benchmark, parser, integration) pass. Commit: 5ef11f6.
…through worker boundary These three ExtractorOutput fields were populated by the JS/TS extractor but never included in SerializedExtractorOutput, causing them to be silently dropped when results crossed the Worker thread boundary. The main thread always received undefined for these fields regardless of what the worker computed, breaking cross-file return-type propagation, param binding resolution, and call-assignment tracking in the WASM engine. Fixes #1348
…gnments through worker boundary" This reverts commit 6402681.
…ccessor this-dispatch (#1351) Parity with the TypeScript guard: `callerName && !callerName.includes('.')`. When caller_name is empty the key would be ':this' which is never seeded, so no incorrect edge is produced today — but adding the guard removes the latent risk and makes the two engines explicitly equivalent.
…ified names for object literal defs (#1351) Two fixes: 1. findDescriptorAccessors (TS) / find_descriptor_accessors (Rust): changed from returning the first accessor to returning all of them. For { get: getter, set: setter }, both 'getter:this = obj' and 'setter:this = obj' are now seeded so both functions can resolve this-dispatch via Phase 8.3f. 2. extractObjectLiteralFunctions (TS) / extract_object_literal_functions (Rust): definitions are now emitted under qualified names ('obj.baz' instead of bare 'baz') to avoid polluting the global definition index with common property names that could produce false-positive edges via the broad exact-name fallback. The typeMap value for function/arrow entries is also updated to the qualified name so the resolver calls lookup.byName('obj.baz'). Also adds complexity metrics for method_definition nodes in the Rust branch (parity with the pair branch which already computed them).
…or this-dispatch (#1351) - Add comment to extractConstDeclarators explaining that the function-scope guard is upheld by the caller (extractConstantsWalk) — future refactors must add a hasFunctionScopeAncestor check before calling extractObjectLiteralFunctions from any other context. - Add comment at the setTypeMapEntry call noting that ':' is a reserved separator for Phase 8.3f keys and cannot collide with real JS identifier names.
|
Addressed Claude's review feedback:
|
|
Claude finished @carlos-alm's task —— View job Re-Review: feat(resolver): resolve this-dispatch inside Object.defineProperty accessor functions
All five concerns from the previous review have been addressed correctly. Here's the verification: 1. Flat namespace pollution (MEDIUM) — Fixed
2. All get/set accessors per descriptor (LOW) — Fixed
3. Rust empty-string guard on
4. Rust
5.
6. Scope guard fragility (LOW) — Fixed
Final AssessmentView job | Branch: feat/defineProperty-accessor-this-dispatch-1335 |
| }); | ||
| // Phase 8.3f: extract function/arrow properties from object literals so that | ||
| // this.method() calls inside Object.defineProperty accessors can resolve them. | ||
| if (valueN.type === 'object') { | ||
| extractObjectLiteralFunctions(valueN, nameN.text, ctx.definitions); | ||
| } |
There was a problem hiding this comment.
hasFunctionScopeAncestor guard
The inline comment added at line 501–504 explicitly warns: "Do not call extractObjectLiteralFunctions from any other context without adding a hasFunctionScopeAncestor guard first." This call in handleVariableDecl omits the guard. Because walkJavaScriptNode recurses into every child without filtering on scope — unlike extractConstantsWalk, which skips FUNCTION_SCOPE_TYPES — this branch fires for const localObj = { fn: () => {} } inside any function body. That registers localObj.fn into the global definition index, where it could be found by lookup.byName('localObj.fn') if an unrelated accessor coincidentally seeds typeMap['getter:this'] = 'localObj'.
The fix is the same guard already used in the sibling destructured-binding branch: !hasFunctionScopeAncestor(node).
There was a problem hiding this comment.
Fixed — added !hasFunctionScopeAncestor(node) to the condition at line 838 in handleVariableDecl, matching the Rust path's find_parent_of_types check and the sibling destructured-binding branch below. The walk path now correctly skips object literals inside function bodies. Also updated the comment at the query path call site to reflect that the walk path now has the guard. Commit: e0663f2.
…sors (#1351) Two fixes: 1. findDescriptorAccessors (TS) / find_descriptor_accessors (Rust): changed from returning the first accessor to returning all of them. For { get: getter, set: setter }, both 'getter:this = obj' and 'setter:this = obj' are now seeded so both functions can resolve this-dispatch via Phase 8.3f. 2. extractObjectLiteralFunctions (TS) / extract_object_literal_functions (Rust): definitions are now emitted under qualified names ('obj.baz' instead of bare 'baz') to avoid polluting the global definition index with common property names that could produce false-positive edges via the broad exact-name fallback. The typeMap value for function/arrow entries is also updated to the qualified name so the resolver calls lookup.byName('obj.baz'). Also adds complexity metrics for method_definition nodes in the Rust branch (parity with the pair branch which already computed them).
…or this-dispatch (#1351) - Add comment to extractConstDeclarators explaining that the function-scope guard is upheld by the caller (extractConstantsWalk) — future refactors must add a hasFunctionScopeAncestor check before calling extractObjectLiteralFunctions from any other context. - Add comment at the setTypeMapEntry call noting that ':' is a reserved separator for Phase 8.3f keys and cannot collide with real JS identifier names.
… expected-edges (#1351) The graph registers object literal arrow properties under qualified names (e.g. 'accessorTarget.accessMethod' not bare 'accessMethod') since extractObjectLiteralFunctions was updated to avoid polluting the global definition index. The expected edge must match the stored node name so the precision check passes for both WASM and native engines.
carlos-alm
force-pushed
the
feat/defineProperty-accessor-this-dispatch-1335
branch
from
2549010 to
6ca6d56
Compare
…cro target names (#1351) Two fixes: 1. handleVariableDecl (walk path): add !hasFunctionScopeAncestor(node) guard before calling extractObjectLiteralFunctions, matching the Rust path's find_parent_of_types check and the sibling destructured-binding branch. Without the guard, const object literals inside function bodies registered their qualified properties (e.g. localObj.fn) into the global definition index. 2. jelly-micro accessors3/expected-edges.json: update target name from bare 'baz' to qualified 'obj.baz' to match the actual node name stored by extractObjectLiteralFunctions. recall was 0% (TP=0) since qualified names were introduced; now recall=100%.
Fixes applied since last reviewCI failures fixed:
Greptile review comment fixed:
|
…ties (#1351) Method-shorthand properties (`const obj = { baz() {} }`) produced a qualified definition `obj.baz` via extractObjectLiteralFunctions but no matching typeMap entry. The two-step Phase 8.3f lookup (callerName:this → obj → obj.baz) would find the typeMap key but then lookup.byName('obj.baz') would fail because no typeMap['obj.baz'] was ever seeded. Add a method_definition branch in handleVarDeclaratorTypeMap (TS) and in extract_object_literal_functions (Rust) to seed typeMap['obj.baz'] = 'obj.baz', matching the pair (arrow/function) branch.
|
Fixed — added |
… fix phase label (#1351) - Add !hasFunctionScopeAncestor(node) guard to the object-literal seeding block in handleVarDeclaratorTypeMap, matching Rust handle_var_decl's find_parent_of_types check. Prevents function-scoped const obj = { fn: ... } from seeding typeMap entries that could shadow module-level declarations with the same property names. - Fix Phase 8.3g → Phase 8.3f label in resolution-benchmark.test.ts comment (all implementation files use Phase 8.3f; the test comment was inconsistent).
|
Addressed the two remaining issues from Greptile's summary review:
Commit: 877d536. |
1 participant
Summary
this.method()inside accessor functions can reach them via same-file lookuptypeMap['obj.propKey'] = 'propKey'for object literal properties (shorthand, identifier, and function values) to enable cross-file accessor dispatch in the futuretypeMap['getter:this'] = 'obj'whenObject.defineProperty(obj, key, { get: getter })is detected, then uses this inresolveByMethodOrGlobal(WASM) andresolve_call_targets(Rust) forthis-dispatch from plain functions registered as accessorsJelly micro-test
accessors3(the motivating case):Edge
getter → baz: 0% → 100% recall.JS resolution benchmark: +1 expected edge (
accessorGetter → accessMethodin the newdefine-property-accessor.jsfixture), precision 1.0 maintained.Test plan
npx vitest run tests/benchmarks/resolution/jelly-micro.test.ts—accessors3100% recall, generators 100%npx vitest run tests/benchmarks/resolution/resolution-benchmark.test.ts -t "javascript"— 5 passed (precision 1.0, recall ≥ 0.9)npx vitest run tests/parsers/javascript.test.ts— 75 passednpx vitest run tests/integration/— 620 passedCloses #1335