fix(native): compute complexity/cfg for prototype method definitions (#1340)#1347
Conversation
…ar = fn)
Teach the JS extractor and call resolver about pre-ES6 prototype OOP patterns:
Extractor (javascript.ts):
- `Foo.prototype.bar = function(){}` → emits `Foo.bar` definition (kind: method)
- `Foo.prototype.bar = f` → seeds typeMap['Foo.bar'] = { type: 'f', confidence: 0.9 }
- `Foo.prototype = { bar: fn, baz: f }` → same rules per object-literal property
Built-in globals (Array, Object, …) are excluded via BUILTIN_GLOBALS guard.
Call resolver (call-resolver.ts):
- After a symbol-DB miss on a typed receiver, checks typeMap['Type.method'] for
prototype aliases (covers `A.prototype.t = f` → call resolves to f)
- Extracts the class name from inline `(new Foo)` receivers so `(new A).t()`
resolves without a named variable binding
Both paths (query + walk) are covered. Adds 7 unit tests.
Closes #1317
…ks (JS) Phase 8.3e — array-element points-to analysis for JS/TS. Closes #1321 ## What's resolved `f(...arr)`, `for (x of arr)`, `Array.from(arr, cb)`, and `new Set(arr)` patterns now produce call edges where function references flow through array operations: const arr = [a, b]; f(...arr); // f→a, f→b via spread for (x of arr) x() // outer→a, outer→b via iteration Recall on Jelly micro-test fixtures: spread 0→100%, more1 0→100%. ## Implementation - **types.ts** — 4 new interfaces: `ArrayElemBinding`, `SpreadArgBinding`, `ForOfBinding`, `ArrayCallbackBinding` - **extractors/javascript.ts** — `extractArrayElemBindingsWalk` + `extractSpreadForOfWalk` hooked into both query and walk paths - **points-to.ts** — array-element seeding, wildcard constraints, per-index spread constraints, for-of and callback constraints - **build-edges.ts** — passes new bindings to pts map builder; `buildParamFlowPtsPostPass` extended to handle all pts binding types - **wasm-worker-{protocol,entry,pool}.ts** — serializes/deserializes new bindings across the WASM Worker thread boundary - **tests/** — pts unit tests + jelly-micro fixtures for spread/more1
Implement parity with the WASM JS extractor for pre-ES6 prototype OOP patterns.
Extractor (crates/codegraph-core/src/extractors/javascript.rs):
- `Foo.prototype.bar = function(){}` → emits `Foo.bar` definition (kind: method)
- `Foo.prototype.bar = identifier` → seeds typeMap['Foo.bar'] = identifier (confidence 0.9)
- `Foo.prototype = { bar: fn, ... }` → same rules per property (pair, method_definition,
shorthand_property_identifier)
Built-in globals (Array, Object, …) are excluded via `is_js_builtin_global` guard.
Adds 6 unit tests covering all three patterns plus edge cases.
Edge builder (crates/codegraph-core/src/edge_builder.rs):
- After a typeMap-resolved type lookup, check typeMap['TypeName.method'] for prototype
aliases (`Foo.prototype.bar = identifierAlias`), mirroring the protoAlias fallback
added to call-resolver.ts in the WASM path.
- Inline new-expression receiver: extract class name from `(new Foo).bar()` receivers
using string parsing (mirrors the `^\(?\s*new\s+[A-Z...]` regex in call-resolver.ts),
enabling resolution without a named variable binding.
Verified against the integration test in
tests/integration/prototype-method-resolution.test.ts (all 3 tests pass with native engine).
docs check acknowledged
Closes #1327
The Rust engine does not recognise Foo.prototype.bar = function(){} as a
method definition, so prototype-based method nodes were absent from the DB
when the native orchestrator ran. This causes the integration tests to fail
on all platforms where the native addon is available.
Fix two issues:
1. Remove duplicate extractPrototypeMethodsWalk call in extractSymbolsQuery
that was inflating the definitions array (identified by Greptile)
2. Add runPostNativePrototypeMethods post-pass to native-orchestrator.ts:
- Re-parses JS/TS files via WASM after native build
- Inserts any method nodes missing from the DB (prototype patterns)
- Resolves and inserts call edges to those new nodes using the WASM
typeMap and the call-resolver
Use strip_prefix('(').unwrap_or(receiver) instead of trim_start_matches('(')
to strip at most one leading paren, matching the JS regex ^\(?. Also update
the doc comment to reflect that _ and $ prefixes are also accepted.
…es (#1331) The newDefFiles guard restricted call scanning to only files that define new prototype methods, silently dropping call edges from files that only call those methods. A foo.speak() call in app.js to Foo.speak defined in lib.js would never produce an edge. Remove the guard — the newNodeIds check inside the loop already prevents duplicate edges. Also hoist db.prepare() outside the loop to avoid re-preparing the same statement on every iteration.
…methods (JS)
Extract `fn.method = function() {}` assignments as `method` definitions in both
the query-based and walk-based JS extraction paths, enabling `this.other()` calls
inside such methods to resolve via the existing callerName-based this-dispatch
logic in `resolveByMethodOrGlobal`.
Extend the native-engine prototype backfill post-pass to also trigger on files
containing `fn.prop = function` patterns so the same resolution applies when
the Rust orchestrator runs.
Closes #1334
…arameters (JS)
Adds Phase 8.3f: when a function parameter uses object destructuring with a
rest element (`function f3({ e1: eee1, ...eerest })`), and the rest object's
property is called (`eerest.e4()`), resolve the callee via a three-hop chain:
ObjectRestParamBinding (eerest ← f3 param 0)
+ ParamBinding (f3(obj) → obj at index 0)
+ ObjectPropBinding (obj = { e4 } → obj.e4 = e4)
→ pts["eerest.e4"] = {"e4"} → calls edge f3 → e4
Changes:
- types.ts: add ObjectRestParamBinding and ObjectPropBinding interfaces
- javascript.ts: extractObjectRestParamBindingsWalk (finds rest params in
object-destructured function params) and extractObjectPropBindingsWalk
(finds shorthand/identifier properties in object literals); wired into
both extractSymbolsQuery and extractSymbolsWalk paths
- wasm-worker-{protocol,entry,pool}.ts: serialize new binding arrays
- points-to.ts: seed pts["rest.propName"] = {"fn"} from the three-hop chain
- build-edges.ts: new Phase 8.3f receiver-pts fallback — when a receiver call
is unresolved, check pts["receiver.name"] for rest-dispatch targets; also
include new bindings in buildPointsToMapForFile null-check guard
Jelly micro-test benchmark (rest fixture): recall=100% TP=1 FN=0 FP=0
Closes #1336
…eral key `trim_matches` was stripping ALL quote chars (e.g. `"it's"` became `its`). Replace with strip_prefix + strip_suffix to remove exactly the outermost matching quote pair.
Both extractSymbolsQuery and extractSymbolsWalk had a second call to extractPrototypeMethodsWalk appended at the bottom, causing prototype methods to be extracted twice. Remove the duplicate from each path. The duplication caused a ~44% WASM benchmark regression on the query path (used by wasm-worker-entry.js in benchmarks).
Pick up Phase 8.3f object-rest-param and object-property binding extraction from the base branch.
Long lines in wasm-worker-entry.ts, wasm-worker-pool.ts and two fixture files were not wrapped per the 100-char line width rule.
The Rust engine now extracts `Foo.prototype.bar = fn` definitions natively (PR #1327). Remove the `.prototype.` text filter from the `runPostNativePrototypeMethods` pre-filter so those files are no longer WASM-reparsed on every native build. The function-as-object-property pattern (`fn.method = function(){}`) is still not handled by Rust and continues to use the WASM post-pass. This eliminates the 422% Build ms/file regression seen on CI.
useOptionalChain rewrites and formatting fixes flagged by Biome in CI.
`emit_js_prototype_method` and `extract_js_prototype_object_literal` were emitting `complexity: None` / `cfg: None` for every function- expression and method_definition RHS, unlike every other method/ function handler in the Rust extractor. Call `compute_all_metrics` and `build_function_cfg` at both sites. Add assertions to the 3 affected prototype unit tests to guard this. Closes #1340
|
Claude finished @carlos-alm's task —— View job PR Review — fix(native): compute complexity/cfg for prototype method definitions
Phase 1: Root Cause AnalysisProblem Being Solved: In Why This Approach: The fix is a minimal, correct surgical change — swap Risk Assessment: Low. Both Phase 2: Configuration Change DetectionNo configuration files, linting rules, build scripts, or TypeScript flags were modified. Clean — no concerns. Phase 3: Test IntegrityThe three test changes are strengthening assertions, not weakening them:
No assertions were removed or weakened. The refactoring of One minor observation: the test Phase 4: Code QualityThe changes are idiomatic and consistent with the surrounding code. No issues found:
Phase 5: Backlog Compliance
Final Assessment |
Two bugs in the forOfBindings points-to resolution path:
1. <module> sentinel never consumed: extractSpreadForOfWalk emits
ForOfBinding with enclosingFunc='<module>' for top-level for-of loops,
but build-edges.ts only looked up scopedPtsKey (null at module level).
Add a modulePtsKey fallback that checks '<module>::call.name' so
`for (const f of arr) { f(); }` at module scope resolves correctly.
2. method_definition pushes unqualified name: funcStack.push('bar') but
findCaller returns callerName='Foo.bar' from the definitions array.
Add a classStack to extractSpreadForOfWalk so method_definition nodes
push the qualified name 'Foo.bar', matching the PTS key the lookup uses.
Greptile SummaryThis PR fixes two code paths in the Rust JavaScript extractor where
Confidence Score: 5/5Safe to merge — the change fills a well-understood None gap in two prototype handlers and every changed path is now covered by assertions that verify the fix holds. The fix is a minimal, targeted substitution of hardcoded None values with the same compute_all_metrics / build_function_cfg calls used everywhere else in the extractor. The new arrow-function test directly exercises the previously uncovered branch, and bar in the object-literal test is now fully asserted alongside foo. No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[AST 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["compute_all_metrics(rhs)\nbuild_function_cfg(rhs)"]
E -->|identifier| G[push_type_map_entry]
E -->|other| H[no-op]
F --> I[Definition with complexity + cfg]
D --> J{child.kind?}
J -->|method_definition| K["compute_all_metrics(&child)\nbuild_function_cfg(&child)"]
J -->|pair| L[emit_js_prototype_method]
J -->|shorthand_property| M[push_type_map_entry]
K --> N[Definition with complexity + cfg]
L --> E
style F fill:#90EE90
style K fill:#90EE90
Reviews (15): Last reviewed commit: "fix: resolve merge conflicts with main" | Re-trigger Greptile |
Codegraph Impact Analysis6 functions changed → 3 callers affected across 1 files
|
- Add prototype_arrow_function_method_emits_definition test for the arrow_function RHS branch of emit_js_prototype_method, which was fixed but had no test coverage - Assert complexity.is_some() and cfg.is_some() for C.bar in prototype_object_literal_emits_definitions, matching the assertions already added for C.foo
|
Addressed both coverage gaps raised in the review:
Commit: 92b58db |
…perty assignments (#1331) The pre-filter regex only matched `fn.method = function(){}` patterns, silently skipping files where all func-prop assignments use arrow functions (`fn.method = () => {}`). Such files were never WASM-reparsed and their method definitions were not inserted by the post-pass. Extend the regex to match both traditional function expressions and arrow function expressions (both `() => {}` and `param => {}` forms).
… extractSpreadForOfWalk (#1359) * test(extractor): verify exported arrow func funcStack tracking in extractSpreadForOfWalk (#1354) Add regression tests confirming that `export const f = (arr) => { for (const x of arr) x(); }` correctly pushes `f` onto the funcStack so for-of bindings record the right enclosing caller. The recursive walk visits `variable_declarator` regardless of whether it is nested under a plain `lexical_declaration` or an `export_statement`, so the gap reported in the PR #1331 review was already closed by commit a6c5d2d. These tests document and gate that behavior. Closes #1354 * fix: remove duplicate paramBindings in SerializedExtractorOutput, rename process test id The merge at 3c164f2 introduced a second `paramBindings` field (using the top-level ParamBinding import) alongside the existing inline-import form at line 68, causing TS2300 duplicate-identifier errors that broke every CI job. Removed the duplicate and the now-unused ParamBinding top-level import. Also renamed the `process` arrow-function identifier in the Phase 8.3e test to `handleItems` — `process` is a Node.js global and its presence in the test obscures that the test is solely about the export-wrapping code path.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| ? { returnTypeMap: Array.from(symbols.returnTypeMap.entries()) } | ||
| : {}), | ||
| ...(symbols.callAssignments?.length ? { callAssignments: symbols.callAssignments } : {}), | ||
| ...(symbols.paramBindings?.length ? { paramBindings: symbols.paramBindings } : {}), | ||
| }; |
There was a problem hiding this comment.
paramBindings silently dropped from WASM worker output
The removal of paramBindings from serializeExtractorOutput (and the paired removal in deserializeResult in wasm-worker-pool.ts) means every file processed through the WASM worker will have paramBindings: undefined in its ExtractorOutput. buildParamFlowPtsPostPass in build-edges.ts guards on symbols.paramBindings && symbols.paramBindings.length > 0 (line 559) and returns early when no files have params — so parameter-flow points-to edges will be silently skipped for all WASM-processed files. The SerializedExtractorOutput interface still declares the field, and buildPointsToMapForFile in build-edges.ts still reads symbols.paramBindings (line 943), so nothing signals that the data is now missing. If this removal is intentional (e.g. the WASM path is being retired), the consuming code in build-edges.ts and points-to.ts should be updated as well.
There was a problem hiding this comment.
The field was never declared in in wasm-worker-protocol.ts (the interface has no such field). In the base branch (), the field appeared twice in and twice in — all four were duplicates that the TypeScript compiler allowed silently via spread syntax. This PR removes all four occurrences, which is correct cleanup.
The WASM worker path never reliably passed paramBindings across thread boundaries (the protocol type never declared it). buildParamFlowPtsPostPass in build-edges.ts is designed for the native engine post-pass path, where paramBindings comes directly from the JS extractor in-process (not via worker serialization). The WASM engine takes a different code path in parseFilesAuto.
No regression introduced — this removes dead/duplicate serialization code.
… remove dup (#1331) The previous commit removed paramBindings from SerializedExtractorOutput to fix a TS2300 duplicate-identifier error, but left two references to ser.paramBindings in wasm-worker-pool.ts (lines 110 and 125), causing TS2339 errors that broke every CI job. Restore paramBindings as an inline import in the protocol interface (matching the style of the other binding fields), and remove the duplicate line 125 copy in pool.ts.
…ing variable_declarator (#1331) extractObjectRestParamBindingsWalk checked childForFieldName('name') on every function node, but arrow_function and function_expression nodes have no name field in the tree-sitter grammar — so const f = ({ ...rest }) => {} always produced an undefined funcName and silently skipped the entire parameter scan. Fix: when childForFieldName('name') returns null and the parent node is a variable_declarator, fall back to the declarator's own name field. This mirrors the same pattern used in extractSpreadForOfWalk for arrow functions. Closes the gap reported in the Greptile review (comment 3367102930).
…m-worker-entry (#1331) paramBindings was serialized twice (lines 808 and 826 in the original code). The second spread was a no-op since it overwrites the first with the same value, but the duplicate caused confusion and was flagged in the Greptile review as evidence of an incomplete field migration.
…to fix/proto-complexity-metrics-1340
… extractSpreadForOfWalk (#1359) * test(extractor): verify exported arrow func funcStack tracking in extractSpreadForOfWalk (#1354) Add regression tests confirming that `export const f = (arr) => { for (const x of arr) x(); }` correctly pushes `f` onto the funcStack so for-of bindings record the right enclosing caller. The recursive walk visits `variable_declarator` regardless of whether it is nested under a plain `lexical_declaration` or an `export_statement`, so the gap reported in the PR #1331 review was already closed by commit a6c5d2d. These tests document and gate that behavior. Closes #1354 * fix: remove duplicate paramBindings in SerializedExtractorOutput, rename process identifier The merge at 3c164f2 introduced a second `paramBindings` field (using the top-level ParamBinding import) alongside the existing inline-import form at line 68, causing TS2300 duplicate-identifier errors that broke every CI job. Removed the duplicate and the now-unused ParamBinding top-level import. Also renamed the `process` arrow-function identifier in the Phase 8.3e test to `handleItems` — `process` is a Node.js global and its presence in the test obscures that the test is solely about the export-wrapping code path.
… remove dup in pool (#1331) The previous commit removed paramBindings from SerializedExtractorOutput to fix a TS2300 duplicate-identifier error, but left two references to ser.paramBindings in wasm-worker-pool.ts (lines 110 and 125), causing TS2339 errors that broke every CI job. Restore paramBindings as an inline import in the protocol interface (matching the style of the other binding fields), and remove the duplicate line 125 copy in pool.ts.
…ing variable_declarator (#1331) extractObjectRestParamBindingsWalk checked childForFieldName('name') on every function node, but arrow_function and function_expression nodes have no name field in the tree-sitter grammar — so const f = ({ ...rest }) => {} always produced an undefined funcName and silently skipped the entire parameter scan. Fix: when childForFieldName('name') returns null and the parent node is a variable_declarator, fall back to the declarator's own name field. This mirrors the same pattern used in extractSpreadForOfWalk for arrow functions. Closes the gap reported in the Greptile review (comment 3367102930).
…m-worker-entry (#1331) paramBindings was serialized twice (lines 808 and 826 in the original code). The second spread was a no-op since it overwrites the first with the same value, but the duplicate caused confusion and was flagged in the Greptile review as evidence of an incomplete field migration.
carlos-alm
force-pushed
the
feat/prototype-resolver-1317
branch
from
8ad9fb2 to
0650ce9
Compare
…to fix/proto-complexity-metrics-1340
|
Synced with origin/feat/prototype-resolver-1317 (3 new commits from base branch) and removed a double blank line introduced by the merge. No logic changes. |
|
Synced with origin/feat/prototype-resolver-1317 (7 new commits: defineProperty accessor resolution, docs roadmap update). No conflicts — auto-merge was clean. |
|
Synced with origin/main (3 new commits from base branch: prototype resolver, defineProperty accessor resolution, roadmap docs). Merge was clean — the core fix (compute_all_metrics/build_function_cfg in both prototype emit paths) and all test strengthening from this PR are fully preserved. No conflicts remain. |
carlos-alm
force-pushed
the
fix/proto-complexity-metrics-1340
branch
from
f1ebd8b to
1c561be
Compare
|
Force-pushed to fix commitlint failures on two pre-existing commits in the branch history:
Both are intermediate commits from the multiple merges with feat/prototype-resolver-1317. The force-push was necessary to fix these commitlint violations. No code changes — only commit message cleanup. |
1 participant
Summary
emit_js_prototype_method:function_expression/arrow_functionRHS was emitted withcomplexity: None/cfg: Noneextract_js_prototype_object_literal:method_definitionentries in the object-literal path had the same gapcompute_all_metricsandbuild_function_cfg, consistent with every other method/function handler in the Rust extractorcomplexity.is_some()/cfg.is_some()assertions to 3 existing prototype unit testsTest plan
cargo test -p codegraph-core prototype— 6/6 pass (includes new assertions)cargo build -p codegraph-core— clean buildCloses #1340