fix(native): add prototype method extraction to Rust engine (#1327)#1339
Conversation
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
|
Claude finished @carlos-alm's task —— View job PR Review: fix(native): add prototype method extraction to Rust engine (#1327)
Root Cause AnalysisProblem Being Solved: The Rust/native engine was missing extraction of pre-ES6 prototype-style OOP patterns ( Why This Approach: The PR adds two coordinated pieces: (1) a Risk Assessment: Low. The changes are purely additive; existing extraction paths are unchanged. The prototype-alias fallback in Phase 1–3: No Red Flags
Phase 4: Code Quality
|
| Severity | Location | Issue |
|---|---|---|
| Minor | javascript.rs:364-374 |
Prototype method definitions skip compute_all_metrics/build_function_cfg — creates secondary complexity-metrics divergence. Parity-correct for call edges but not for codegraph complexity output. |
| Minor | edge_builder.rs:503-526 |
extract_inline_new_type: no unit tests for the string-parsing edge cases. |
| Minor | edge_builder.rs:431-439 |
Prototype-alias fallback path has no dedicated unit test. |
None of these are blockers for the stated goal (call-edge parity). The missing complexity metrics for prototype methods is the most notable gap — if codegraph complexity is expected to report on prototype-defined methods, this will silently produce null metrics. Recommend opening an issue to track it.
Final Assessment
Rating: ⭐⭐⭐⭐ (4/5)
Action: APPROVE
Reasoning: The PR correctly identifies a real parity bug, fixes it at the right layer (extraction + resolution), faithfully mirrors the TypeScript reference implementation, adds 6 unit tests + 1 integration test, and introduces zero new dependencies. The code is clean, well-commented, and architecturally appropriate. The minor gaps (missing complexity metrics, two missing unit tests) don't affect correctness of the primary goal. Recommend opening a follow-up issue for the complexity: None gap in prototype method definitions before merging if codegraph complexity is expected to cover these symbols.
Codegraph Impact Analysis25 functions changed → 16 callers affected across 9 files
|
Greptile SummaryThis PR ports the pre-ES6 prototype OOP extraction from the WASM/TypeScript path to the native Rust engine, achieving parity for
Confidence Score: 5/5Safe to merge; Rust extraction and edge resolution are correctly implemented and the WASM post-pass deduplicates via a DB existence check, preventing any double-insertion for mixed files. All three prototype patterns are handled in both the extractor and edge builder, mirroring the TS resolver faithfully. The WASM filter update is correct: the negative lookahead reliably excludes prototype patterns from WASM re-parse, and the post-pass guards against duplicates with an explicit SELECT before INSERT. The only issue found is a stale JSDoc comment that does not affect runtime behaviour. The JSDoc on runPostNativePrototypeMethods in native-orchestrator.ts is the one item worth a quick fix before merge; no correctness issues were found in the Rust files. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[JS/TS File] --> B{Rust Extractor\nmatch_js_prototype_methods}
B -->|Foo.prototype.bar = function| C[Emit method Definition\nFoo.bar]
B -->|Foo.prototype.bar = identifier| D[Seed typeMap\nFoo.bar to identifier]
B -->|Foo.prototype = object literal| E[extract_js_prototype_object_literal]
E -->|pair / arrow_function| C
E -->|pair / identifier| D
E -->|shorthand_property_identifier| D
E -->|method_definition| C
C --> F[(DB: nodes table)]
D --> G[typeMap in FileSymbols]
G --> H[edge_builder resolve_call_targets]
H -->|type_map lookup| I{Direct method lookup Foo.bar}
I -->|found| J[Return typed targets]
I -->|miss| K{Proto-alias fallback typeMap.get Foo.bar}
K -->|found| L[Resolve to standalone fn]
H -->|no type_map entry| M{extract_inline_new_type new Foo .bar}
M -->|Foo extracted| I
N[WASM Post-Pass] -->|Filter: fn.method=function only| O{DB existence check}
O -->|node missing| F
O -->|already present| P[Skip - no duplicate]
Reviews (8): Last reviewed commit: "fix: resolve merge conflicts with feat/p..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| /// Only extracts PascalCase (uppercase-initial) names to avoid false positives on | ||
| /// lowercase constructor calls (rare but present in legacy code). | ||
| fn extract_inline_new_type(receiver: &str) -> Option<String> { | ||
| let s = receiver.trim_start_matches('(').trim_start(); |
There was a problem hiding this comment.
trim_start_matches('(') removes all leading ( characters, while the JS regex it mirrors (^\(?) allows at most one. A receiver like ((new Foo)).bar() would resolve Foo in Rust but produce no match in the TS engine, creating a parity divergence.
| let s = receiver.trim_start_matches('(').trim_start(); | |
| let s = receiver.strip_prefix('(').unwrap_or(receiver).trim_start(); |
There was a problem hiding this comment.
Fixed — replaced trim_start_matches('(') with strip_prefix('(').unwrap_or(receiver) so at most one leading paren is stripped, exactly matching the JS regex ^\(?. Committed in 0c26030.
| /// Only extracts PascalCase (uppercase-initial) names to avoid false positives on | ||
| /// lowercase constructor calls (rare but present in legacy code). |
There was a problem hiding this comment.
_- and $-prefixed names. The comment should be updated to match the actual behaviour (which correctly mirrors the JS regex [A-Z_$]).
| /// Only extracts PascalCase (uppercase-initial) names to avoid false positives on | |
| /// lowercase constructor calls (rare but present in legacy code). | |
| /// Only extracts names that start with an uppercase letter, `_`, or `$` to avoid | |
| /// false positives on plain lowercase constructor calls (rare but present in legacy code). |
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 — updated the doc comment to accurately describe that names starting with uppercase letter, _, or $ are accepted, matching the JS regex [A-Z_$]. Committed in 0c26030.
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.
|
Addressed all review feedback: Greptile P2 fixes (both fixed in commit 0c26030):
Out-of-scope findings tracked as issues:
|
…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.
|
Follow-up fixes pushed in this batch: Greptile string-key stripping bug (commit 41d7811) Duplicate Benchmark regression fix (commit 5baf98c) Base branch merge (commit 2fa6d7e + 8dcc760) |
The regex /\b\w+\.\w+\s*=\s*function/ matched the substring
'prototype.bar = function' inside 'Foo.prototype.bar = function(){}',
causing prototype files to be queued for WASM re-processing even though
the Rust engine now handles those patterns natively. Added a negative
lookahead to exclude the prototype shape, matching only
function-as-object-property patterns like 'fn.method = function'.
Fixes the duplicate-node risk flagged in Greptile review of #1339.
|
Fixed the pre-filter regex in native-orchestrator.ts (commit 4e579b0): The regex Fixed by adding a negative lookahead: |
Cover the string-parsing logic in extract_inline_new_type:
(new Foo), (new Foo('arg')), no-parens form, _ and $ prefixes,
lowercase rejection, plain identifier, and the newFoo-not-a-keyword case.
|
Addressed Claude's review concerns:
|
|
Claude finished @carlos-alm's task —— View job Re-review: fix(native): add prototype method extraction to Rust engine (#1327)
All concerns from the initial review have been addressed. Here's the verification: 1. 8 unit tests added in
All 8 tests are positive assertions matching the implementation exactly. ✓ 2. Pre-filter regex fix (commit
return /\b(?!prototype\.)\w+\.\w+\s*=\s*function/.test(content);The negative lookahead 3. Doc comment fix (commit
4.
5. Open issues for out-of-scope findings ✓
No remaining concerns. All flagged issues are either fixed or appropriately tracked. The PR is correct, well-tested, and architecturally sound. Rating: ⭐⭐⭐⭐⭐ (5/5) — LGTM, ready to merge. |
Resolve conflicts: - javascript.rs: preserve both prototype extraction tests (this PR) and Phase 8.3e Object.define*/create tests (base branch) - this.js: accept base branch arrow function form for f.g
… biome lint (#1339) Commit 4ed709e's biome auto-fix renamed defProp/defProps/create to _defProp/_defProps/_create (unused-variable prefix), but the expected-edges.json manifest still referenced the old names. This caused 5 false positives and 5 false negatives in the JS benchmark, dropping precision to 84.4% (below the 100% threshold) and recall to 81.8% (below 90%). Also fixes the class-inheritance DoubleCounter fixture: the code used Counter.count() (a direct static call) but the manifest expected a class-inheritance edge via super.count(). Changed to super.count() so the fixture tests what the manifest documents. Prevent recurrence by adding a biome.json override that disables lint for tests/benchmarks/resolution/fixtures/** — fixture files are hand-written sample code that must use specific patterns (including apparently-unused functions and super calls) to exercise resolution.
|
Fixed CI failures (commit fceb2bf): The JS benchmark was failing precision (84.4% < 100%) and recall (81.8% < 90%) due to a stale Fixes applied:
Local test results: precision=100%, recall=93.9% (all JS benchmark tests pass). |
1 participant
Summary
match_js_prototype_methodstocrates/codegraph-core/src/extractors/javascript.rs— handles all three patterns (Foo.prototype.bar = fn,Foo.prototype.bar = identifier,Foo.prototype = { bar: fn })new-expression receiver extraction and prototype alias lookup tocrates/codegraph-core/src/edge_builder.rsto match thecall-resolver.tsresolver pathTest plan
javascript.rs(all pass:cargo test prototype)cargo test)tests/integration/prototype-method-resolution.test.ts— 3/3 pass with locally built native binarynpm test)Closes #1327