fix(native): compute complexity/CFG for prototype method definitions#1345
Conversation
|
Claude finished @carlos-alm's task —— View job PR Review: fix(native): compute complexity/CFG for prototype method definitionsTasks
Root Cause AnalysisProblem Being Solved: The native Rust extractor was emitting
This is a real parity bug: the WASM engine correctly computed complexity/CFG for these patterns but the native engine silently dropped them. This directly caused incorrect output in the native engine for any JavaScript/TypeScript codebase using prototype-based patterns. Why This Approach: The fix is precisely targeted — replace Risk Assessment: Very low. The fix follows an established, tested pattern. Both call sites pass nodes of the correct type: Phase 2: Configuration Change DetectionNo linting rules changed, no test timeouts modified, no build config altered. Single source file changed. Clean. Phase 3: Test IntegrityThree new unit tests added (lines 2507–2544):
Tests use One minor observation: Phase 4: Code QualityThe fix is minimal and correct:
Backlog Compliance
Final Recommendation
Optional follow-up (not blocking): Consider adding a test for |
Greptile SummaryThis PR fixes two gaps in the native JavaScript extractor where
Confidence Score: 5/5Safe to merge — two isolated one-line additions that follow an established pattern, each covered by a dedicated new unit test. The fix is narrowly scoped to two call sites in the JavaScript extractor. Both additions mirror the identical pattern already used by handle_method_def and handle_var_decl, reducing the risk of behavioral drift. Four new unit tests directly verify the changed paths, and the full 353-test suite passes. No side effects on other extraction paths are expected. No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["Prototype assignment node"] --> B{Pattern?}
B -->|"Foo.prototype.method = rhs"| C["emit_js_prototype_method"]
B -->|"Foo.prototype = { ... }"| D["extract_js_prototype_object_literal"]
C --> E{rhs.kind?}
E -->|"function_expression / arrow_function"| F["extract_js_parameters(rhs) NEW\ncompute_all_metrics + build_function_cfg\n→ Definition with children"]
E -->|identifier| G["push_type_map_entry"]
D --> H{child.kind?}
H -->|method_definition| I["extract_js_parameters(child) NEW\ncompute_all_metrics + build_function_cfg\n→ Definition with children"]
H -->|shorthand_property_identifier| J["push_type_map_entry"]
H -->|pair| K["emit_js_prototype_method → recurses"]
Reviews (17): Last reviewed commit: "fix(native): extract parameters for prot..." | Re-trigger Greptile |
Codegraph Impact Analysis6 functions changed → 3 callers affected across 1 files
|
|
Addressed Greptile P2 feedback:
Also addressed Claude's optional follow-up: added a test for the |
|
Claude finished @carlos-alm's task —— View job Re-review: fix(native): compute complexity/CFG for prototype method definitionsTasks
Updated ReviewAll Greptile P2 feedback and the Claude optional follow-up have been fully addressed. Parameter extraction ( Parameter extraction ( New arrow_function test ( Test suite consistency: All four prototype tests now assert the full triple: One observation (non-blocking): The arrow test only checks for parameter Rating: ⭐⭐⭐⭐⭐ (5/5) The PR is now fully consistent with |
|
Resolved merge conflicts with |
|
Addressed Greptile feedback:
|
|
Claude finished @carlos-alm's task —— View job Re-review: fix(native): compute complexity/CFG for prototype method definitionsTasks
Final VerificationThe last two open items from the previous review are both resolved: Missing PR description accuracy: Updated to correctly state 4 new unit tests. ✓ All four prototype tests now assert the full triple ( Rating: ⭐⭐⭐⭐⭐ (5/5) No remaining issues. This PR is complete and ready to merge. |
|
Resolved all merge conflicts with latest
JavaScript benchmark: 100.0% precision, 100.0% recall. Build and lint clean. |
|
Synced with base branch ( The Windows CI failure ( |
8ad9fb2 to
0650ce9
Compare
|
Merged latest from Root cause: The merge at 4ac7cbe incorrectly removed The base branch already had the correct fix in commit d387644 ( Pre-existing test failures (not caused by this PR):
|
| ? { returnTypeMap: Array.from(symbols.returnTypeMap.entries()) } | ||
| : {}), | ||
| ...(symbols.callAssignments?.length ? { callAssignments: symbols.callAssignments } : {}), |
There was a problem hiding this comment.
paramBindings silently dropped across the worker boundary
The PR removes paramBindings from serializeExtractorOutput (here) and from deserializeResult in wasm-worker-pool.ts. The TypeScript extractor produces paramBindings via extractParamBindingsWalk, serialises it into the returned ExtractorOutput, and the worker uses that output as symbols. After this change, every symbols.paramBindings array is discarded before being sent to the main thread.
build-edges.ts (line 542-561) has an explicit pts post-pass that runs only when symbols.paramBindings is non-empty; its own comment reads: "The native Rust engine builds call edges without knowledge of paramBindings, so fn() calls inside higher-order functions are not resolved to their concrete targets. This JS post-pass … adds only the parameter-flow pts edges that the native engine missed." With paramBindings always absent, that pass is permanently skipped and higher-order call edges are silently missing for every JS/TS file processed through the worker pool.
There was a problem hiding this comment.
Fixed — paramBindings was briefly removed during a merge conflict but restored in commit d387644 (fix(wasm-worker): restore paramBindings in SerializedExtractorOutput and remove duplicate copy in pool). The current code correctly serializes it in wasm-worker-entry.ts (line 808), declares it in wasm-worker-protocol.ts (line 73), and deserializes it in wasm-worker-pool.ts (line 110) — the pts post-pass in build-edges.ts receives paramBindings as intended.
|
Resolved merge conflict with main (commit 1967fcd — feat/prototype-resolver-1317). The conflict was in |
emit_js_prototype_method and extract_js_prototype_object_literal were emitting children: None for function_expression/arrow_function/method_definition RHS nodes, unlike all other method handlers (handle_method_def, handle_var_decl) which call extract_js_parameters and populate the children field. Call extract_js_parameters at both sites and pass the result through opt_children, completing parity with handle_method_def and handle_var_decl. Add 4 new unit tests verifying non-None complexity, CFG, and children (including all declared parameters) for every affected code shape: direct function assignment, direct arrow assignment, shorthand method definition, and key-value function pair. Closes #1345
480f9f1 to
9ab4fee
Compare
|
Force-pushed to fix commitlint CI failure. The branch had accumulated messy merge history from multiple syncs with
The branch now contains a single clean commit on top of main:
The change is identical to what was reviewed and approved: |
Summary
Extends the prototype-method complexity fix (already in main via #1347) by also extracting parameter children for prototype method definitions:
Test plan
Closes #1345