feat(resolver): phase 8.3d — object property write tracking in points-to analysis#1295
Conversation
…-to analysis
Walk assignment_expression nodes in extractTypeMapWalk (WASM) and
match_js_type_map (native). When LHS is a simple member_expression
(obj.prop) and RHS is an identifier, seed typeMap['obj.prop'] = { type:
fn, confidence: 0.85 }. This covers patterns like:
handlers.auth = authMiddleware;
router.use(handlers.auth); // -> edge: caller -> authMiddleware
Extend resolveByMethodOrGlobal (TS) and resolve_call_targets (Rust) with
a composite key lookup (step 4.5): when a call has receiver + name, check
typeMap['receiver.name'] for a direct pts target before falling through to
the no-match return. Skips BUILTIN_GLOBALS / builtin objects (console,
Math, etc.) and chained writes (a.b.c = x).
Closes #1292
|
Claude finished @carlos-alm's task —— View job PR Review: feat(resolver): phase 8.3d — object property write tracking in points-to analysisTasks
Root Cause AnalysisProblem Being Solved: Before this PR, Why This Approach: The fix is minimal and architecturally sound. Rather than a new pass, it hooks into the existing Risk Assessment: Low. The new behavior only fires when a call has a receiver + name that exactly matches a previously-seen property write. The 0.85 confidence level is appropriate (below explicit type annotations at 0.9, above factory heuristics at 0.7). The Phase 2: Configuration Change Detection✅ No biome/lint rules relaxed, no TypeScript strict flags disabled, no test timeouts increased, no coverage thresholds lowered. The two minor style changes ( Phase 3: Test Integrity✅ No assertions removed or weakened. Five focused unit tests cover the key edge cases (simple write, chained write, non-identifier RHS, builtin guard, idempotency). Integration test builds a real graph and asserts on actual DB edges — solid end-to-end coverage. Phase 4: Code QualityBug: BUILTIN_GLOBALS parity gap between enginesThe Rust This is an engine parity bug: Per CLAUDE.md policy: "If two engines produce different results, that is a bug in the less-accurate engine". The TS engine is less restrictive here (a false positive) and needs the four names added to Minor: No kind filter on pts resolution targets
Observation:
|
Greptile SummaryThis PR adds phase 8.3d of the points-to analysis: object property write tracking. It seeds the type map with
Confidence Score: 5/5Safe to merge — the new resolution step is narrowly scoped to property-write pts keys, falls through cleanly when no match is found, and does not alter existing steps 1–4. The composite key lookup is inserted correctly inside the receiver guard in both engines, BUILTIN_GLOBALS filtering prevents noise entries from being seeded, and higher-confidence-wins deduplication is consistent between TypeScript and Rust. Unit and integration tests directly exercise the before/after behaviour. No files require special attention. Important Files Changed
Reviews (5): Last reviewed commit: "fix(extractor): sync is_js_builtin_globa..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| it('higher-confidence entry wins when same key appears twice', () => { | ||
| // First write seeds at 0.85; a later type annotation on handlers.auth should win | ||
| const parser = parsers.get('typescript'); | ||
| const tree = parser.parse(` | ||
| handlers.auth = firstMiddleware; | ||
| handlers.auth = secondMiddleware; | ||
| `); | ||
| const symbols = extractSymbols(tree, 'test.ts'); | ||
| // Second write at same confidence (0.85) does not overwrite the first | ||
| expect(symbols.typeMap.get('handlers.auth')?.type).toBe('firstMiddleware'); | ||
| }); |
There was a problem hiding this comment.
The test is titled "higher-confidence entry wins when same key appears twice," but it exercises the equal-confidence case: two 0.85 writes, first-write wins. The higher-confidence path (a 0.9 annotation overwriting the first 0.85 write) is never asserted. This means the invariant that setTypeMapEntry promotes on strictly higher confidence is untested by this suite.
The inner comment ("a later type annotation on handlers.auth should win") describes a scenario that is completely absent from the test body — there is no type annotation, and the assertion expects firstMiddleware (i.e., the first write wins, not the annotation).
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 — renamed the test to 'first-write wins when same key appears twice at equal confidence', removed the stale inner comment, and added a new explicit test 'higher-confidence entry promotes over lower-confidence entry' that directly calls setTypeMapEntry to assert the 0.85 → 0.9 promotion path. The BUILTIN_GLOBALS test was also extended to cover process/window/document/globalThis.
| const obj = lhsN.childForFieldName('object'); | ||
| const prop = lhsN.childForFieldName('property'); | ||
| if (!obj || !prop) return; | ||
| if (obj.type !== 'identifier') return; // skip chained: a.b.c = x | ||
|
|
||
| const objName = obj.text; | ||
| if (BUILTIN_GLOBALS.has(objName)) return; | ||
|
|
||
| setTypeMapEntry(typeMap, `${objName}.${prop.text}`, rhsN.text, 0.85); |
There was a problem hiding this comment.
fnRefBindings handling
The existing fnRefBindings block (lines 1419–1422) explicitly checks prop.type === 'property_identifier' || prop.type === 'identifier' before using prop.text, with a comment explaining that computed subscript expressions can produce unexpected text. handlePropWriteTypeMap skips this guard and calls prop.text directly. While tree-sitter's member_expression should always yield a property_identifier for the property field (bracket-notation produces subscript_expression and is therefore excluded by the earlier lhsN.type !== 'member_expression' check), the missing guard is a defensive inconsistency that could produce malformed composite keys if the grammar ever diverges.
| const obj = lhsN.childForFieldName('object'); | |
| const prop = lhsN.childForFieldName('property'); | |
| if (!obj || !prop) return; | |
| if (obj.type !== 'identifier') return; // skip chained: a.b.c = x | |
| const objName = obj.text; | |
| if (BUILTIN_GLOBALS.has(objName)) return; | |
| setTypeMapEntry(typeMap, `${objName}.${prop.text}`, rhsN.text, 0.85); | |
| const obj = lhsN.childForFieldName('object'); | |
| const prop = lhsN.childForFieldName('property'); | |
| if (!obj || !prop) return; | |
| if (obj.type !== 'identifier') return; // skip chained: a.b.c = x | |
| if (prop.type !== 'property_identifier' && prop.type !== 'identifier') return; | |
| const objName = obj.text; | |
| if (BUILTIN_GLOBALS.has(objName)) return; | |
| setTypeMapEntry(typeMap, `${objName}.${prop.text}`, rhsN.text, 0.85); |
There was a problem hiding this comment.
Fixed — added the prop.type !== 'property_identifier' && prop.type !== 'identifier' guard in handlePropWriteTypeMap, consistent with the adjacent fnRefBindings block. Added an explanatory comment matching the pattern used there.
Codegraph Impact Analysis9 functions changed → 23 callers affected across 7 files
|
…BALS (#1295) Adds the four names present in Rust's is_js_builtin_global but absent from the TypeScript BUILTIN_GLOBALS set, restoring dual-engine parity for property-write pts tracking. Also adds prop.type guard in handlePropWriteTypeMap consistent with the adjacent fnRefBindings block.
…romotion test (#1295) Renames 'higher-confidence entry wins' to accurately describe equal-confidence first-write behavior. Removes stale inner comment. Adds explicit test for strict-higher-confidence promotion via setTypeMapEntry. Extends BUILTIN_GLOBALS test to cover process/window/document/globalThis.
|
Addressed the BUILTIN_GLOBALS engine-parity bug: added |
|
Claude finished @carlos-alm's task —— View job Re-review: feat(resolver): phase 8.3d — object property write tracking in points-to analysisTasks
Verified FixesAll three requested changes are correctly applied: 1. BUILTIN_GLOBALS parity ( 2. // Guard: only static property access (property_identifier or identifier), not
// computed subscript expressions — consistent with the adjacent fnRefBindings block.
if (prop.type !== 'property_identifier' && prop.type !== 'identifier') return;Exact pattern from the Greptile suggestion, with appropriate comment. 3. Test corrections (
Remaining Finding:
|
|
Addressed remaining reviewer findings:
|
|
Claude finished @carlos-alm's task —— View job Re-review: feat(resolver): phase 8.3d — object property write tracking in points-to analysisTasks
Verified Fixes (Round 3)All remaining findings from the previous review are correctly addressed: 1. Step 4.5 fall-through fix ( if (ptsTarget) {
const resolved = lookup
.byName(ptsTarget)
.filter((t) => computeConfidence(relPath, t.file, null) >= 0.5);
if (resolved.length > 0) return resolved; // ← non-empty guard added
}The unconditional return is gone. Now matches the Rust pattern exactly ( 2. The Rust guard now contains all 43 entries from the TypeScript
Cross-checking both sets: identical. Parity guarantee restored. 3. Correctly added, with explanatory comment matching the adjacent 4. Test fixes (
No Remaining FindingsAll three rounds of review findings are fully addressed:
Final Recommendation
|
1 participant
Summary
typeMap['obj.prop']fromassignment_expressionnodes (handlers.auth = authMiddleware) in both WASM extractor (extractTypeMapWalk) and native Rust extractor (match_js_type_map), with confidence 0.85resolveByMethodOrGlobal(TS) andresolve_call_targets(Rust) with a step-4.5 composite pts key lookup: when a call hasreceiver + name, checktypeMap['receiver.name']for a direct function target before falling throughconsole,Math, etc.) and chained writes (a.b.c = x)Before:
router.use(handlers.auth)produced no edge toauthMiddlewareAfter: edge
setupRoutes → authMiddlewareis emitted (dynamic=1)Test plan
tests/parsers/javascript.test.tsunderPhase 8.3d: property write pts trackingtests/integration/issue-1292-property-write-pts.test.tsverifies full pipeline with acceptance criteria from feat(resolver): phase 8.3d — object property write tracking in points-to analysis #1292cargo check— 0 errors, 6 pre-existing warnings)Closes #1292