Skip to content

fix(native): compute complexity/CFG for prototype method definitions#1345

Merged
carlos-alm merged 1 commit into
mainfrom
fix/native-prototype-complexity-1340
Jun 7, 2026
Merged

fix(native): compute complexity/CFG for prototype method definitions#1345
carlos-alm merged 1 commit into
mainfrom
fix/native-prototype-complexity-1340

Conversation

@carlos-alm

@carlos-alm carlos-alm commented Jun 6, 2026

Copy link
Copy Markdown
Contributor

Summary

Extends the prototype-method complexity fix (already in main via #1347) by also extracting parameter children for prototype method definitions:

  • `emit_js_prototype_method` was emitting `children: None` for `function_expression` and `arrow_function` RHS nodes
  • The `method_definition` arm in `extract_js_prototype_object_literal` had the same gap
  • Fix: call `extract_js_parameters` at both sites and pass the result through `opt_children`, matching the pattern in `handle_method_def` and `handle_var_decl`

Test plan

  • 4 new Rust unit tests verifying non-None complexity, CFG, and children (including all declared parameters) for direct prototype assignment (function and arrow), object-literal shorthand method, and object-literal pair-fn — all pass
  • Full Rust suite: 353/353 pass (`cargo test`)

Closes #1345

@claude

claude Bot commented Jun 6, 2026

Copy link
Copy Markdown

Claude finished @carlos-alm's task —— View job


PR Review: fix(native): compute complexity/CFG for prototype method definitions

Tasks

  • Gather context and understand the PR
  • Read and analyze changed files
  • Phase 1: Root Cause Analysis
  • Phase 2: Configuration Change Detection
  • Phase 3: Test Integrity
  • Phase 4: Code Quality
  • Phase 5: Backlog Compliance
  • Final Assessment

Root Cause Analysis

Problem Being Solved: The native Rust extractor was emitting complexity: None and cfg: None for prototype method definitions in two code paths:

  1. emit_js_prototype_method — handles Foo.prototype.bar = function() {...} and Foo.prototype.bar = () => {...}
  2. extract_js_prototype_object_literal (method_definition arm) — handles shorthand Foo.prototype = { greet() {...} }

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 None literals with the same two function calls (compute_all_metrics and build_function_cfg) used identically in handle_function_decl (line 505–506), handle_method_def (line 566–567), and handle_var_decl (line 647–648). No abstraction changes needed; this is pure omission correction.

Risk Assessment: Very low. The fix follows an established, tested pattern. Both call sites pass nodes of the correct type: rhs in emit_js_prototype_method is always a function_expression or arrow_function (guarded by the match arm), and &child in the method_definition arm is a method_definition node — the same kind handle_method_def receives. The build_function_cfg function handles the None body case gracefully (lines 1456–1461 in cfg.rs), so there's no crash risk on edge cases.


Phase 2: Configuration Change Detection

No linting rules changed, no test timeouts modified, no build config altered. Single source file changed. Clean.


Phase 3: Test Integrity

Three new unit tests added (lines 2507–2544):

  1. prototype_direct_method_has_complexity_and_cfg — covers Foo.prototype.bar = function() {...} with a branch, verifying complexity.is_some() and cfg.is_some()
  2. prototype_object_literal_method_definition_has_complexity_and_cfg — covers shorthand greet() { ... } in object literal
  3. prototype_object_literal_pair_fn_has_complexity_and_cfg — covers bar: function() {...} pair form

Tests use if (true) branches to ensure the metrics are non-trivially populated (i.e., the complexity visitor actually walks the body). Assertions are appropriately direct — not weakened. All three tests are new additions, not modifications of existing expectations.

One minor observation: arrow_function form (Foo.prototype.bar = () => {...}) is not covered by a test. The emit_js_prototype_method function handles "function_expression" | "arrow_function" in the same arm, so the fix applies to both — but only the function_expression variant is tested. Not a blocker, but worth noting for completeness.


Phase 4: Code Quality

The fix is minimal and correct:

  • emit_js_prototype_method (lines 371–372): Passes rhs directly. Since this arm only matches "function_expression" | "arrow_function", both functions receive a proper function node. build_function_cfg uses child_by_field_name("body") which works correctly on both node types. ✓

  • extract_js_prototype_object_literal method_definition arm (lines 401–402): Passes &child (a method_definition node). This matches exactly what handle_method_def does (line 566–567), where a method_definition node is passed directly. build_function_cfg can walk the body field of method_definition nodes. ✓

  • Consistency with existing pattern: All other handlers (handle_function_decl, handle_method_def, handle_var_decl, callback handlers at lines 1608–1644) use identical call signatures. This PR completes the set. ✓

  • No unnecessary changes: The pair arm correctly delegates to emit_js_prototype_method (already handles the function node), so no change is needed there. ✓


Backlog Compliance

  • Zero-dep: ✓ — No new dependencies. Calls existing compute_all_metrics and build_function_cfg already used throughout the file.
  • Foundation-aligned: ✓ — Pure local static analysis, no cloud calls, no new toolchain requirements, no agent behavior.
  • Problem-fit: 4/5 — Directly improves the accuracy of complexity/CFG data that AI agents use for blast-radius analysis. Missing complexity on prototype methods means AI agents get incomplete code health signals for JavaScript codebases using the native engine.
  • Breaking: No — purely additive. Previously None fields now have values; no schema changes, no CLI format changes, no API signature changes.
  • Tier: 1 (zero-dep ✓ + foundation-aligned ✓)

Final Recommendation

  • Rating: ⭐⭐⭐⭐ (4/5)
  • Action: APPROVE
  • Reasoning: Clean, targeted fix for a genuine parity bug. Follows the established pattern exactly, has 3 new tests covering the main cases, and the 340/340 Rust suite passes. The only missing coverage is the arrow_function variant in emit_js_prototype_method, but since it shares the same code path as function_expression the risk is negligible. No configuration relaxation, no weakened assertions, no scope creep.

Optional follow-up (not blocking): Consider adding a test for Foo.prototype.bar = (x) => x > 0 ? x : 0 to cover the arrow function variant — though the existing fix applies to it identically.

@greptile-apps

greptile-apps Bot commented Jun 6, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR fixes two gaps in the native JavaScript extractor where children (parameter metadata) was hardcoded to None for prototype-style method definitions. Both emit_js_prototype_method and the method_definition arm of extract_js_prototype_object_literal now call extract_js_parameters and wire the result through opt_children, matching the existing pattern in handle_method_def and handle_var_decl.

  • emit_js_prototype_method (function_expression / arrow_function): extract_js_parameters(rhs, source) is now called on the RHS function node, and the result replaces the previous children: None.
  • extract_js_prototype_object_literal (method_definition arm): extract_js_parameters(&child, source) is now called on the method_definition node, exactly mirroring handle_method_def.
  • Four new unit tests (direct function, direct arrow, shorthand method, pair-fn) verify non-None complexity, CFG, and parameter children; all 353 suite tests pass.

Confidence Score: 5/5

Safe 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

Filename Overview
crates/codegraph-core/src/extractors/javascript.rs Two one-line additions wire extract_js_parameters into prototype method definition sites that previously emitted children: None; four new unit tests confirm correctness. Change is well-scoped and consistent with existing handler patterns.

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"]
Loading

Reviews (17): Last reviewed commit: "fix(native): extract parameters for prot..." | Re-trigger Greptile

@github-actions

github-actions Bot commented Jun 6, 2026

Copy link
Copy Markdown
Contributor

Codegraph Impact Analysis

6 functions changed3 callers affected across 1 files

  • emit_js_prototype_method in crates/codegraph-core/src/extractors/javascript.rs:514 (3 transitive callers)
  • extract_js_prototype_object_literal in crates/codegraph-core/src/extractors/javascript.rs:543 (2 transitive callers)
  • prototype_direct_method_has_complexity_cfg_and_children in crates/codegraph-core/src/extractors/javascript.rs:2697 (0 transitive callers)
  • prototype_direct_arrow_has_complexity_cfg_and_children in crates/codegraph-core/src/extractors/javascript.rs:2717 (0 transitive callers)
  • prototype_object_literal_method_definition_has_complexity_cfg_and_children in crates/codegraph-core/src/extractors/javascript.rs:2737 (0 transitive callers)
  • prototype_object_literal_pair_fn_has_complexity_cfg_and_children in crates/codegraph-core/src/extractors/javascript.rs:2755 (0 transitive callers)

@carlos-alm

Copy link
Copy Markdown
Contributor Author

Addressed Greptile P2 feedback:

  • emit_js_prototype_method (lines 364–374): Added extract_js_parameters(rhs, source) and replaced children: None with children: opt_children(children) — matching handle_var_decl's pattern exactly.
  • extract_js_prototype_object_literal method_definition arm (lines 393–404): Added extract_js_parameters(&child, source) and replaced children: None with children: opt_children(children) — matching handle_method_def's pattern exactly.

Also addressed Claude's optional follow-up: added a test for the arrow_function variant (prototype_direct_arrow_has_complexity_cfg_and_children). All four tests now assert on children as well as complexity and cfg. Full Rust suite: 341/341 pass.

@carlos-alm

Copy link
Copy Markdown
Contributor Author

@greptileai

@carlos-alm

Copy link
Copy Markdown
Contributor Author

@claude

@claude

claude Bot commented Jun 6, 2026

Copy link
Copy Markdown

Claude finished @carlos-alm's task —— View job


Re-review: fix(native): compute complexity/CFG for prototype method definitions

Tasks

  • Gather context and understand the request
  • Read updated file changes
  • Review parameter extraction additions
  • Review new arrow_function test
  • Final assessment

Updated Review

All Greptile P2 feedback and the Claude optional follow-up have been fully addressed.

Parameter extraction (emit_js_prototype_method, line 365): extract_js_parameters(rhs, source) is now called before the Definition push, and children: opt_children(children) replaces children: None. This matches handle_var_decl exactly. ✓

Parameter extraction (extract_js_prototype_object_literal method_definition arm, line 396): Same pattern — extract_js_parameters(&child, source) called, result passed via opt_children. Matches handle_method_def exactly. ✓

New arrow_function test (prototype_direct_arrow_has_complexity_cfg_and_children, line 2531): Tests (a, b) => a > 0 ? a : b — a ternary body that ensures the complexity visitor walks the expression, not just a trivial no-branch function. Asserts complexity.is_some(), cfg.is_some(), and that parameter a is present in children. Well-formed. ✓

Test suite consistency: All four prototype tests now assert the full triple: complexity, cfg, children. The prior gap (only is_some() for metrics, None for children) is gone. ✓

One observation (non-blocking): The arrow test only checks for parameter a, not b. Both a and b are declared in the parameter list, so b should also appear in children. This is a minor coverage gap but not a correctness issue — if a is populated, b is too, since both come from the same extract_js_parameters call.


Rating: ⭐⭐⭐⭐⭐ (5/5)
Action: APPROVE

The PR is now fully consistent with handle_method_def and handle_var_decl across all three fields (complexity, cfg, children). 341/341 tests pass. Clean, targeted, complete.

View job

@carlos-alm

Copy link
Copy Markdown
Contributor Author

Resolved merge conflicts with fix/native-prototype-extraction-1327. Both test sets are preserved in the merged result: the 4 prototype complexity/CFG/children tests from this PR and the 4 Object.defineProperty/defineProperties/create type_map tests from the base branch. Rust suite: 353/353 pass.

@carlos-alm

Copy link
Copy Markdown
Contributor Author

@greptileai

Base automatically changed from fix/native-prototype-extraction-1327 to feat/prototype-resolver-1317 June 6, 2026 08:53
@carlos-alm

Copy link
Copy Markdown
Contributor Author

Addressed Greptile feedback:

  • Arrow test missing 'b' assertion (line 2689-2703): Added the missing assert!(children.iter().any(|c| c.name == "b"), ...) assertion — the test now checks both parameters a and b, matching the completeness of the other parameter tests.
  • PR description reported 3 new tests; 4 were added: Updated the PR description to correctly state 4 new unit tests.

@carlos-alm

Copy link
Copy Markdown
Contributor Author

@greptileai

@carlos-alm

Copy link
Copy Markdown
Contributor Author

@claude

@claude

claude Bot commented Jun 6, 2026

Copy link
Copy Markdown

Claude finished @carlos-alm's task —— View job


Re-review: fix(native): compute complexity/CFG for prototype method definitions

Tasks

  • Gather context and understand the request
  • Read updated test changes
  • Final assessment

Final Verification

The last two open items from the previous review are both resolved:

Missing b assertion (lines 2703–2706): assert!(children.iter().any(|c| c.name == "b"), ...) is now present in prototype_direct_arrow_has_complexity_cfg_and_children. The test now asserts both a and b, matching the completeness of all other parameter tests. ✓

PR description accuracy: Updated to correctly state 4 new unit tests. ✓

All four prototype tests now assert the full triple (complexity, cfg, children) with complete parameter coverage. 353/353 Rust tests pass.

Rating: ⭐⭐⭐⭐⭐ (5/5)
Action: APPROVE

No remaining issues. This PR is complete and ready to merge.

View job

@carlos-alm

Copy link
Copy Markdown
Contributor Author

Resolved all merge conflicts with latest feat/prototype-resolver-1317. Summary of changes pushed:

  1. Merged base branch changes (correct _defProp/_defProps/_create names in expected-edges.json, super.count() in inheritance.js)
  2. Fixed duplicate paramBindings field in SerializedExtractorOutput that resulted from the merge
  3. Added missing assertion for parameter b in the arrow function test (addressing both Claude and Greptile's non-blocking observation)
  4. Updated PR description to correctly state 4 new tests were added

JavaScript benchmark: 100.0% precision, 100.0% recall. Build and lint clean.

@carlos-alm

Copy link
Copy Markdown
Contributor Author

Synced with base branch (feat/prototype-resolver-1317): merged commit 9fb1a8b (test: exported arrow funcStack + fix duplicate paramBindings). No conflicts.

The Windows CI failure (EBUSY: resource busy or locked, graph.db) is a pre-existing flaky SQLite file-locking issue on Windows unrelated to this PR — tracked in #1344 and #1353, with a fix in PR #1356. All tests pass on Linux and macOS.

@carlos-alm carlos-alm force-pushed the feat/prototype-resolver-1317 branch from 8ad9fb2 to 0650ce9 Compare June 7, 2026 00:33
@carlos-alm

Copy link
Copy Markdown
Contributor Author

Merged latest from feat/prototype-resolver-1317 to fix all CI failures.

Root cause: The merge at 4ac7cbe incorrectly removed paramBindings from SerializedExtractorOutput in wasm-worker-protocol.ts and left two references to ser.paramBindings in wasm-worker-pool.ts. This caused TS2339 errors that broke the npm install prepare step, which in turn caused all CI jobs to fail.

The base branch already had the correct fix in commit d387644 (fix(wasm-worker): restore paramBindings in SerializedExtractorOutput and remove duplicate copy in pool). This merge brings in that fix along with two other cleanup commits (6e87234, 8ad9fb2).

Pre-existing test failures (not caused by this PR):

@carlos-alm

Copy link
Copy Markdown
Contributor Author

@greptileai

Comment on lines 823 to 825
? { returnTypeMap: Array.from(symbols.returnTypeMap.entries()) }
: {}),
...(symbols.callAssignments?.length ? { callAssignments: symbols.callAssignments } : {}),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 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.

Fix in Claude Code

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@carlos-alm

Copy link
Copy Markdown
Contributor Author

@greptileai

Base automatically changed from feat/prototype-resolver-1317 to main June 7, 2026 03:27
@carlos-alm

Copy link
Copy Markdown
Contributor Author

Resolved merge conflict with main (commit 1967fcd — feat/prototype-resolver-1317). The conflict was in crates/codegraph-core/src/extractors/javascript.rs: main had introduced the same prototype-method extraction infrastructure (with complexity: None, cfg: None, children: None) while this PR's entire purpose was to fill those None fields. Resolution: kept the PR side (computed complexity/CFG/children) in both emit sites and preserved all four new unit tests. Nothing was lost from either side.

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
@carlos-alm carlos-alm force-pushed the fix/native-prototype-complexity-1340 branch from 480f9f1 to 9ab4fee Compare June 7, 2026 04:59
@carlos-alm

Copy link
Copy Markdown
Contributor Author

Force-pushed to fix commitlint CI failure.

The branch had accumulated messy merge history from multiple syncs with feat/prototype-resolver-1317 (which was later merged to main). This left two commits in the PR's commit range that violated the 100-char header limit rule:

  • fix(wasm-worker): restore paramBindings in SerializedExtractorOutput and remove duplicate copy in pool (#1331) (110 chars)
  • test(extractor): verify exported arrow function funcStack tracking in extractSpreadForOfWalk (#1359) (footer body lines > 100 chars)

The branch now contains a single clean commit on top of main:

  • fix(native): extract parameters for prototype method definitions (#1345)

The change is identical to what was reviewed and approved: children: opt_children(children) in both emit_js_prototype_method and extract_js_prototype_object_literal, plus the 4 new unit tests. All 11 prototype tests pass.

@carlos-alm

Copy link
Copy Markdown
Contributor Author

@greptileai

@carlos-alm carlos-alm merged commit 428edb4 into main Jun 7, 2026
28 checks passed
@carlos-alm carlos-alm deleted the fix/native-prototype-complexity-1340 branch June 7, 2026 06:24
@github-actions github-actions Bot locked and limited conversation to collaborators Jun 7, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant