test(bench): add JS fixture for bare-call class-scoped lookup regression guard#1422
Conversation
…ion guard (#1407) Adds class-scope.js to the JS resolution benchmark. The fixture contains a class with a bare flush() call inside run() and a Processor.flush method that must NOT be the target of that call — JS bare calls are lexically scoped to the module, never the class. The 1.0 precision floor for the JS fixture acts as the enforcement mechanism: if the call.receiver guard in resolveByMethodOrGlobal is ever removed, the resolver would emit Processor.run → Processor.flush as a false positive, immediately failing CI. Closes #1407
|
Claude finished @carlos-alm's task —— View job PR Review
Root Cause AnalysisProblem Being Solved: Guards against a future regression where the Why This Approach: A precision-1.0 floor on the JS fixture acts as an automated regression wire. Any future resolver change that emits Risk Assessment: Low. Pure test additions with no production code changes. The fixture is minimal and unambiguous. Phase 1: Root Cause Analysis ✓The PR addresses a real, specific regression risk rather than hiding a symptom. The guard is justified by the JS language spec (no implicit Phase 2: Configuration Change Detection ✓No configuration changes. No linting rules relaxed. No thresholds lowered — the JS precision threshold remains 1.0 and recall remains 0.9. The test file modification only updates a comment explaining the edge count history (35 → 40), and adds the Phase 3: Test Integrity ✓No assertions weakened. The benchmark test file change at One observation: The Phase 4: Code Quality ✓
Minor nit: The Phase 5: Backlog Compliance
Final Recommendation
|
Greptile SummaryThis PR adds a regression guard for the JS/TS bare-call class-scoped lookup bug (#1407): a new
Confidence Score: 5/5Safe to merge — the change is a targeted, well-tested guard on the bare-call resolution path with no impact on existing C#/Java class-method dispatch or JS/TS this-receiver calls. The logic change in call-resolver.ts is minimal and precisely scoped: it adds one boolean check (isBareCall && isModuleScopedLanguage) before the same-class fallback. The new unit tests cover all four critical cases (bare .ts, bare .js, this-receiver .ts, bare .cs), and the benchmark fixture enforces the guard at precision 1.0 in CI. No existing resolution paths are altered; the C# extractor changes are documentation-only. No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["resolveByMethodOrGlobal(call, relPath)"] --> B{Has explicit receiver?}
B -- "receiver is class/type name" --> C[Direct qualified lookup\nClassName.method\n+ confidence >= 0.5]
C --> D{Found?}
D -- Yes --> E[Return targets]
D -- No --> F[typeMap / protoMap lookups]
B -- "no receiver / this / self / super" --> G[Exact name lookup\nlookup.byName call.name]
G --> H{Exact match found?}
H -- Yes --> E
H -- No --> I{isBareCall &&\nisModuleScopedLanguage?}
I -- "Yes (JS/TS bare call)" --> J[Skip same-class fallback\nReturn empty]
I -- "No (this/self/super\nor non-JS/TS)" --> K[Same-class fallback\nCallerClass.method\n+ confidence >= 0.5]
K --> L{Found?}
L -- Yes --> E
L -- No --> J
Reviews (10): Last reviewed commit: "fix(csharp): remove duplicate extract_va..." | Re-trigger Greptile |
Codegraph Impact Analysis9 functions changed → 22 callers affected across 8 files
|
…alizers in C# Both the WASM and native C# extractors were skipping variable declarations with `var`/`implicit_type` type nodes entirely, so `var service = new UserService(repo)` never added `service → UserService` to the typeMap. The call-edge resolver therefore could not resolve `service.AddUser()` or `service.GetUser()` to the qualified methods on `UserService`. The dist copy of the WASM extractor already had the fix (extractVarInitType), but the source had drifted out of sync. This commit re-introduces the logic in both engines: WASM (TypeScript): add `extractVarInitType(declarator)` that walks the variable_declarator children looking for an `object_creation_expression` (or one inside an `equals_value_clause`), then reads the `type` field via `extractCSharpTypeName`. `handleCSharpVarDecl` now sets `isVar` for `implicit_type | var_keyword` and calls `extractVarInitType` in that branch. Native (Rust): mirror the same logic in `extract_var_init_type` and update `match_csharp_type_map` to drive it when the type node is `var_keyword` or `implicit_type`. Infrastructure: add CODEGRAPH_NATIVE_ADDON_PATH env-var override to `loadNative()` for local development workflows where the published npm binary can't be loaded (its dylib install-name points to the CI build path). Tests now pass with CODEGRAPH_NATIVE_ADDON_PATH set to the locally rebuilt binary. Fixes: #1418 (build-parity test failure — native vs WASM C# edge/role divergence on `var`-typed local variables instantiated with `new`).
|
Merged origin/main into this branch. Resolved two conflicts:
Note: Local test runs show the fixture is detecting real bugs in the resolver:
These bugs are masked in CI's pre-publish benchmark gate because it runs in artifact mode (pre-computed results). Once the fixtures are rebuilt locally or the artifact is regenerated, the precision gate will fail. The fixture PR is working as designed — it added the guard, and the guard found bugs. |
|
Claude finished @carlos-alm's task —— View job PR Review — Post-Merge Analysis
Current StateAfter the merge of
Scope Issue: C# Extractor Changes Don't Belong HereThe files Per the repo's own guideline: One PR = one concern. The C# fix should be in its own PR. That said, if carlos-alm deliberately bundled this because it was blocking the class-scope fixture (it doesn't appear to be — the JS fixture is independent), then it should be explicitly noted. The C# Fix Itself (commit
|
In JS/TS, bare foo() calls inside a class method are module-scoped — there is no implicit class binding. The same-class fallback in resolveByMethodOrGlobal was incorrectly emitting class-scoped edges for bare calls with no module-level match (e.g. flush() inside Processor.run resolving to Processor.flush). Gate the fallback: skip for bare calls in JS/TS, keep for this.method() calls and for C#/Java where bare calls are class-scoped. Closes #1425.
|
Resolver fix committed (290008d): gated the same-class fallback in resolveByMethodOrGlobal on language to prevent JS/TS bare-call false positives. Both #1425 (Processor.run → Processor.flush) and #1426 (runCallThis → handler) are now fixed. Both issues closed. The class-scope.js fixture worked as designed — it found the exact regression it was intended to guard against. |
|
Claude finished @carlos-alm's task —— View job PR Review — Final Pass
Current StateThis PR now includes three distinct changes across 4 commits:
Resolver Fix —
|
|
Fixed PR description edge count discrepancy (40 → 42) and updated the resolver line reference (189 → 229). Also merged origin/main (commit 906a8b0). |
The direct qualified-name fallback in resolveByMethodOrGlobal was the only resolution path that did not apply computeConfidence >= 0.5, risking false edges from distant files in polyglot projects with name collisions. Closes #1398
|
Addressed Greptile's two review issues: Issue 1 — Missing unit test for bare-call JS/TS guard: Added 4 new unit tests to Issue 2 — Redundant Merge conflicts with |
1 participant
Summary
class-scope.jsfixture to the JS resolution benchmarkProcessor.run → processDatatoexpected-edges.jsonas the positive recall caseresolveByMethodOrGlobalon language (JS/TS bare calls no longer fall through to class-method lookup)What this guards
In JS/TS, a bare
foo()call inside a class method is lexically scoped to the module, not the class — there is no implicitthisbinding. If thecall.receiverguard inresolveByMethodOrGlobal(call-resolver.ts:229) were ever removed, the resolver would incorrectly emitProcessor.run → Processor.flush(a false positive).The fixture:
The 1.0 precision floor on the JS fixture acts as the enforcement mechanism: any regression producing
Processor.run → Processor.flushfails CI immediately.Test plan
npx vitest run -t "javascript" tests/benchmarks/resolution/resolution-benchmark.test.ts— JS precision 100%, recall 100% (42/42 edges), all 10 tests passCloses #1407