feat(resolver): phase 8.3 + 8.3b — points-to analysis (WASM + native parity)#1296
Conversation
…ust engine (closes #1280) Ports Phase 8.2 to the native engine so returnTypeMap and callAssignments are populated for JS/TS/TSX files regardless of which engine is active. - Rust: new NativeCallAssignment NAPI struct; FileSymbols gains returnTypeMap and callAssignments fields; match_js_return_type_map and match_js_call_assignments walk passes mirror the JS extractReturnTypeMapWalk and recordCallAssignment logic exactly - JS: patchReturnTypeMap() in parser.ts converts the native array to a Map (same pattern as patchTypeMap) so propagateReturnTypesAcrossFiles sees a unified ExtractorOutput regardless of engine
…bols literals Two test-only FileSymbols struct literals in import_edges.rs and structure.rs were not updated when the new fields were added to the struct, causing cargo test --release to fail to compile on all platforms (E0063).
Three parity issues identified in review: 1. find_parent_class climbed through function scope boundaries, causing nested function declarations inside class methods to be incorrectly attributed to the enclosing class (e.g. UserService.buildQuery instead of buildQuery). Added find_parent_class_no_fn_boundary which stops the ancestor walk when it crosses a function_declaration, function_expression, arrow_function, or method_definition node, matching the WASM extractReturnTypeMapWalk which resets currentClass to null before recursing into any function body. 2. The variable_declarator branch included 'function' as a valid value kind, which has no counterpart in the TS reference and is not a valid tree-sitter value- expression node kind. Removed it, leaving only 'arrow_function' and 'function_expression' to match the WASM reference exactly. 3. patchReturnTypeMap returned undefined for files with no annotated functions, while patchTypeMap always returns a (possibly empty) Map. Changed to always assign new Map() to keep both native and WASM paths consistent and safe for future callers that iterate without optional-chaining.
…nto fix/native-return-type-parity
… (phase 8.3) Implements a per-file Andersen-style constraint solver that resolves higher-order function calls missed by the existing callback heuristic. The extractor now records FnRefBinding entries (const fn = handler, const fn = obj.method) via handleVarDeclaratorTypeMap. In the JS edge- building path, buildPointsToMapForFile seeds the pts map from local definitions and imported names, then propagates alias constraints to fixed-point (≤50 iterations). When resolveCallTargets returns empty for a dynamic, receiver-less call, resolveViaPointsTo is tried as a fallback and the resolved alias goes back through the normal resolution chain — so cross-module flows work transparently via importedNames. Confidence on pts-resolved edges is penalised by PROPAGATION_HOP_PENALTY (0.1) to reflect the indirection. The fallback is strictly additive and conservative: it only fires when primary resolution already failed, so no existing edges are affected. Expected impact: +5–10pp on caller coverage for callback-heavy code (Express/Koa middleware, Node.js EventEmitter patterns, strategy objects). Impact: 14 functions changed, 12 affected
#1289) - Guard member_expression property binding to static property access only (property_identifier | identifier), preventing computed subscript expressions like obj[expr] from producing pts keys that can never match - Add explanatory comment to the partial Call object passed to resolveCallTargets in the pts alias fallback path (only name is needed for alias resolution) - Add DEFAULTS.analysis.pointsToMaxIterations (50) and document the TODO to wire MAX_SOLVER_ITERATIONS through config alongside typePropagationDepth - Document that callable-only seeding in buildPointsToMapForFile is intentional: class aliases are handled by Phase 8.2 call-assignment propagation - Update config unit test to include the new pointsToMaxIterations field - File issue #1293 to track native Rust engine parity for fn-ref bindings
…dence direct-call edge (#1289) A pts fallback edge for caller→target (confidence C-0.1) was added to seenCallEdges before the primary loop could process a later direct call to the same target. The direct call was silently dropped by the dedup guard, leaving the graph with a penalised confidence instead of the correct full-confidence edge. Fix: pts edges are tracked in a separate ptsEdgeRows map (edgeKey → allEdgeRows index) instead of seenCallEdges. When the primary loop later resolves the same caller→target pair directly, it upgrades the existing row's confidence in-place and promotes the key to seenCallEdges.
…pts edge (#1289) Two bugs in the pts-edge upgrade path introduced by d794d47: 1. allEdgeRows[ptsIdx][3] = confidence produced TS2532 (Object is possibly undefined) because TypeScript cannot prove that ptsIdx is a valid allEdgeRows index, even though we set the index ourselves just before pushing the row. Fix: assign to a local variable with an explicit undefined guard before writing. 2. When a direct call upgrades a pts-fallback edge, only the confidence (index 3) was written back; the isDynamic flag (index 4) kept the value 1 from the original alias call. Downstream consumers (diff-impact, risk classifiers) would then misclassify a confirmed direct call as a dynamic dispatch. Fix: write isDynamic to index 4 alongside confidence.
Phase 8.3b: closes the parity gap introduced by #1289 where field-based points-to analysis only ran in the WASM/JS call-edge builder. **Rust changes (crates/codegraph-core)** - `types.rs`: add `FnRefBinding` napi struct; add `fn_ref_bindings` field to `FileSymbols` so the all-Rust orchestrator path can carry bindings through. - `extractors/javascript.rs`: extend `handle_var_decl` to detect `const alias = identifier` and `const alias = obj.method` patterns and push to `fn_ref_bindings`, mirroring the WASM extractor's `handleVarDeclaratorTypeMap`. - `edge_builder.rs`: add `build_points_to_map` / `resolve_via_points_to` (Andersen-style ≤50-iteration fixed-point solver, mirrors `points-to.ts`); add pts fallback in `process_file` for dynamic receiver-less calls with no primary targets; wire `fn_ref_bindings` into `FileEdgeInput`. - `build_pipeline.rs`: forward `FileSymbols.fn_ref_bindings` into `FileEdgeInput` for the all-Rust build pipeline path. **TypeScript changes** - `build-edges.ts`: add `fnRefBindings` to `NativeFileEntry` and serialize `symbols.fnRefBindings` when building the JS→Rust FFI entries. - `wasm-worker-entry.ts` / `wasm-worker-pool.ts` / `wasm-worker-protocol.ts`: propagate `fnRefBindings` through the WASM worker serialization boundary so the WASM pts path also works end-to-end (was silently dropped before). **Test** - `tests/integration/pts-parity.test.ts`: assert both engines emit a `processItems → handler` call edge for the `const alias = handler; arr.map(alias)` pattern.
|
Claude finished @carlos-alm's task —— View job PR Review: feat(resolver): phase 8.3 + 8.3b — points-to analysis
Root Cause AnalysisProblem Being Solved: Higher-order function calls where a named variable aliases a function (e.g. Why This Approach: An Andersen-style inclusion-based points-to analysis is the standard and correct tool for this class of problem. The field-based (non-field-sensitive) approximation is well-understood and appropriate — 99%+ precision at negligible cost, matching the scope of what a call-graph builder typically needs. The intra-file fixed-point solver is compact (110 LoC in TS, ~50 LoC in Rust) and well-bounded (≤50 iterations). The wasm-worker serialization bug fix is clearly correct: Risk Assessment: Low. The PTS path is purely additive — it runs only when Configuration Change Detection
typePropagationDepth: 3,
pointsToMaxIterations: 50,These are additive only — no existing rule was relaxed. The TODO comment acknowledges that the constants aren't yet wired through config (the hardcoded
Test IntegrityThe parity test ( const requireParity = !!process.env.CODEGRAPH_PARITY;
const describeOrSkip = requireParity || hasNative ? describe : describe.skip;In most CI environments without a native binary, this test suite silently skips (no The fix itself ( Code Quality
The One concern: const { targets: aliasTargets, importedFrom: aliasFrom } = resolveCallTargets(
lookup,
{ name: alias }, // ← only `name`, no `line`, `receiver`, or `dynamic`
...
The
The Rust
Clean. Backlog Compliance
Critical Concerns
Final Recommendation |
Greptile SummaryThis PR completes Phase 8.3/8.3b points-to analysis across both execution paths: it fixes a serialization bug that prevented
Confidence Score: 5/5Safe to merge — both engine paths produce correct and identical pts-resolved call edges, the WASM serialization bug is properly fixed across all three worker-boundary files, and the prior review's builtin-globals parity gap has been closed. The Rust pts solver mirrors the JS buildFileCallEdges logic accurately: seeding, constraint propagation, confidence penalisation, and the pts_edge_map upgrade path all behave identically to their TypeScript counterparts. The WASM worker fix correctly propagates fnRefBindings through entry, pool, and protocol layers. The integration test covers the serialization fix, the confidence-upgrade scenario, and cross-engine parity. No logic gaps or data-loss paths were found. No files require special attention. Important Files Changed
Reviews (7): Last reviewed commit: "Merge branch 'main' into feat/phase-8.3-..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| } else if name_n.kind() == "identifier" && value_n.kind() == "identifier" { | ||
| // Phase 8.3: `const alias = handler` — record for pts analysis. | ||
| symbols.fn_ref_bindings.push(FnRefBinding { | ||
| lhs: node_text(&name_n, source).to_string(), | ||
| rhs: node_text(&value_n, source).to_string(), | ||
| rhs_receiver: None, | ||
| }); |
There was a problem hiding this comment.
identifier = identifier bindings without filtering JS builtin globals, while the JS extractor guards both the identifier and member-expression cases with !BUILTIN_GLOBALS.has(valueN.text). For most code this is harmless — builtins like Array or JSON are not seeded in the pts map, so constraints referencing them produce no flow. However, if a file defines a local function whose name matches a builtin (e.g., function Array(x) {...}; const fn = Array), the Rust engine would seed Array in the pts map and produce a caller → Array edge, while the WASM engine would silently drop the binding — directly breaking the engine-parity guarantee the test suite is meant to enforce.
| } else if name_n.kind() == "identifier" && value_n.kind() == "identifier" { | |
| // Phase 8.3: `const alias = handler` — record for pts analysis. | |
| symbols.fn_ref_bindings.push(FnRefBinding { | |
| lhs: node_text(&name_n, source).to_string(), | |
| rhs: node_text(&value_n, source).to_string(), | |
| rhs_receiver: None, | |
| }); | |
| } else if name_n.kind() == "identifier" && value_n.kind() == "identifier" { | |
| // Phase 8.3: `const alias = handler` — record for pts analysis. | |
| // Mirror the JS BUILTIN_GLOBALS guard: skip well-known JS globals so | |
| // they are never seeded as pts targets (e.g. `const a = Array`). | |
| let rhs_text = node_text(&value_n, source); | |
| if !JS_BUILTIN_GLOBALS.contains(rhs_text) { | |
| symbols.fn_ref_bindings.push(FnRefBinding { | |
| lhs: node_text(&name_n, source).to_string(), | |
| rhs: rhs_text.to_string(), | |
| rhs_receiver: None, | |
| }); | |
| } |
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 — added a JS_BUILTIN_GLOBALS constant (a slice mirroring the TS BUILTIN_GLOBALS set) and guarded both the identifier and member_expression RHS cases in match_js_type_map. Patterns like const fn = Array and const fn = Math.random are now filtered out on the Rust path, closing the parity gap.
| /** Maximum fixed-point iterations for the Phase 8.3 points-to solver. */ | ||
| pointsToMaxIterations: 50, |
There was a problem hiding this comment.
pointsToMaxIterations is not read by either solver
DEFAULTS.analysis.pointsToMaxIterations and the matching CodegraphConfig interface field are defined here, but both the TS solver (MAX_SOLVER_ITERATIONS = 50 in points-to.ts) and the Rust solver (literal 50 in edge_builder.rs) use hardcoded constants instead. A user who sets this in .codegraphrc.json will see no effect. The same situation exists for typePropagationDepth, and the TODO comment above already acknowledges this, so this is a known gap — but it may be worth an explicit note in the JSDoc that the config key is reserved and not yet effective.
There was a problem hiding this comment.
Acknowledged — added a @reserved JSDoc note to both pointsToMaxIterations in config.ts and the matching field in src/types.ts clarifying that neither the WASM solver nor the Rust solver reads this value yet. The existing TODO comment above in config.ts already tracked the wiring work; the JSDoc makes it explicit at the public interface level.
Codegraph Impact Analysis17 functions changed → 21 callers affected across 9 files
|
…ers (#1296) Three test helpers in edge_builder.rs, import_edges.rs and structure.rs constructed FileEdgeInput / FileSymbols literals without the fn_ref_bindings field added in Phase 8.3, causing the native host build to fail to compile on all three CI platforms.
…constant (#1296) The Rust extractor was recording fn_ref_bindings for patterns like `const fn = Array` without the guard present in the JS extractor. This created a parity gap: if a file defines a local function whose name matches a builtin, the native engine would seed it in the pts map and produce a spurious call edge while the WASM engine would silently drop the binding. - Add JS_BUILTIN_GLOBALS constant mirroring the TS BUILTIN_GLOBALS set - Guard both the identifier and member_expression paths in match_js_type_map - Extract MAX_SOLVER_ITERATIONS named constant (was hardcoded 0..50) with a TODO noting it should be wired through config alongside typePropagationDepth
…elds (#1296) The parity test suite only ran when native was available or CODEGRAPH_PARITY was set, leaving the primary bug (fnRefBindings dropped at WASM worker boundary) unguarded in WASM-only CI. Add a dedicated describe block that unconditionally exercises the WASM engine. Also document that pointsToMaxIterations is reserved and not yet wired to either solver in config.ts and types.ts, and remove a redundant double-negation in ts-resolver.ts.
|
Addressed all four concerns from the review: 1. Missing BUILTIN_GLOBALS guard in Rust extractor (critical) — Fixed. Added a 2. Hardcoded 3. Partial 4. WASM parity test skips silently without native binary — Fixed. Added a new unconditional CI build failures — Also fixed: three test struct initializers in |
|
Claude finished @carlos-alm's task —— View job PR Re-review: feat(resolver): phase 8.3 + 8.3b — points-to analysis
Verification of All Four ConcernsFix #1 — BUILTIN_GLOBALS guard in Rust extractor ✅
Fix #2 — Named MAX_SOLVER_ITERATIONS in Rust ✅
Fix #3 — WASM-only test (always runs) ✅
Fix #4 — @reserved JSDoc in config.ts and types.ts ✅
One minor observation (non-blocking)The TS Updated AssessmentAll four concerns from the previous review have been addressed correctly. The implementation is sound:
Rating: ⭐⭐⭐⭐⭐ (5/5) — APPROVE The changes are additive, low-risk, and improve call-edge precision for higher-order function patterns. Ready to merge. |
…geRows (#1296) When a file both aliases and directly calls the same target (e.g. `arr.map(alias)` then `handler(x)`), the Rust pts fallback was inserting the edge directly into `seen_edges`, locking in the penalised confidence and silently dropping the subsequent direct call. The JS/WASM path avoids this via `ptsEdgeRows` — pts edges go into a staging map, and `emit_call_edges` upgrades confidence in-place when the direct call is found. Mirrors the same mechanism in `build-edges.ts` lines 660–700: - `pts_edge_map: HashMap<u64, usize>` (key = edge_key, value = index into `edges`) replaces the direct `seen_edges.insert` for pts-resolved edges. - `emit_call_edges` now checks `pts_edge_map` before inserting a new direct-call edge; if a pts edge exists for the same pair, it upgrades confidence and is_dynamic in-place and promotes the key to `seen_edges`. - Adds a confidence-upgrade fixture and two new test suites (WASM-only + parity) that assert the edge confidence is >= 0.9 (direct-call level) rather than the penalised 0.9 - 0.1 = 0.8 value.
…geRows (#1296) When a file both aliases and directly calls the same target (e.g. `arr.map(alias)` then `handler(x)`), the Rust pts fallback was inserting the edge directly into `seen_edges`, locking in the penalised confidence and silently dropping the subsequent direct call. The JS/WASM path avoids this via `ptsEdgeRows` — pts edges go into a staging map, and `emit_call_edges` upgrades confidence in-place when the direct call is found. Mirrors the same mechanism in `build-edges.ts` lines 660–700: - `pts_edge_map: HashMap<u64, usize>` (key = edge_key, value = index into `edges`) replaces the direct `seen_edges.insert` for pts-resolved edges. - `emit_call_edges` now checks `pts_edge_map` before inserting a new direct-call edge; if a pts edge exists for the same pair, it upgrades confidence and is_dynamic in-place and promotes the key to `seen_edges`. - Adds a confidence-upgrade fixture and two new test suites (WASM-only + parity) that assert the edge confidence is >= 0.9 (direct-call level) rather than the penalised 0.9 - 0.1 = 0.8 value.
|
Addressed the pts confidence-upgrade parity gap raised in the Greptile summary. The Rust pts fallback was inserting edges directly into Fixed by adding a |
carlos-alm
force-pushed
the
feat/phase-8.3-points-to-analysis
branch
from
4a6c108 to
d0f719b
Compare
1 participant
Summary
buildCallEdgesJSresolves higher-order function calls where a named variable aliases a function (const fn = handler; arr.map(fn)→ edge tohandler). Andersen-style fixed-point solver with ≤50 iterations.const alias = fn/const alias = obj.methodbindings;edge_builder.rsimplements the same constraint solver;build_pipeline.rsthreads bindings through.fnRefBindingswas silently dropped at the worker serialization boundary since Phase 8.3, preventing the WASM pts path from ever running. Fixed inwasm-worker-entry.ts,wasm-worker-pool.ts,wasm-worker-protocol.ts.Test plan
npx vitest run tests/integration/pts-parity.test.ts— both engines emitprocessItems → handlerfor aconst alias = handler; items.map(alias)fixturenpx vitest run tests/integration/ tests/engines/— 699 tests pass, no regressionsnpm run lint— clean (warnings only, no errors)Closes #1290