fix: align enclosing-caller attribution for variable bindings (haskell, zig)#1499
fix: align enclosing-caller attribution for variable bindings (haskell, zig)#1499carlos-alm wants to merge 27 commits into
Conversation
… diff Adds snapshot-pre-bash.sh (PreToolUse Bash) + track-bash-writes.sh (PostToolUse Bash): the pre-hook captures git status --porcelain to a per-worktree temp file before each Bash call; the post-hook diffs the before/after state and appends newly modified or created files to .claude/session-edits.log. This closes the gap where files written by sed -i, printf redirects, tee, heredocs, or build tools (Cargo.lock, lockfiles) were never recorded, causing guard-git.sh to emit false-positive BLOCKED errors. Closes #1457
- clojure.rs: annotate lifetime-anchor assignment to silence false-positive - cfg.rs: remove never-called start_line_of method - complexity.rs: remove never-constructed NotHandled variant; convert irrefutable if-let patterns to plain let destructures - dataflow.rs: remove never-read callee fields from CallReturn/Destructured - incremental.rs: remove never-read lang field from CacheEntry cargo check and cargo clippy both clean after these changes.
Adds .github/workflows/perf-canary.yml — a path-filtered workflow that fires on PRs touching src/extractors/, src/domain/graph/, or crates/** and runs only the incremental-benchmark suite (full build + no-op + 1-file rebuild, both engines). Catches the class of regressions that accumulated invisibly across the Phase 8.x PRs and were only detected at v3.12.0 publish time. The regression guard gains BENCH_CANARY=1 mode: raises thresholds to 50%/100%/150% (standard/noisy/WASM) and skips the build, query, and resolution suites — only incremental checks run. This absorbs shared- runner timing variance while still blocking catastrophic regressions (+98% full build, +1827% 1-file rebuild from v3.12.0). Closes #1433
On incremental builds, runPostNativeCha previously scanned all call→qualified-method edges in the DB (~12ms flat, O(graph size)), even for 1-file changes where no hierarchy or RTA evidence changed. Add two cheap indexed gate queries. Gate A checks whether any changed file introduced a class/interface/trait/struct/record node (hierarchy may have new implementors reachable from unchanged call sites). Gate B checks whether any changed file added a call edge to a class-kind target (RTA set may have grown, enabling previously filtered expansions in unchanged callers). If neither gate fires, restrict the candidate query to src.file IN changedFiles — safe because the hierarchy and instantiated set are unchanged for all other files. Full builds (isFullBuild=true) and cases where either gate fires retain the existing full-scan behaviour. Mirrors the changed-files scoping pattern of runPostNativeThisDispatch. Closes #1441
Times each JS post-pass in tryNativeOrchestrator and exposes the
measurements in BuildResult.phases:
- gapDetectMs — dropped-language gap detection + backfill
- chaMs — CHA expansion (interface dispatch)
- thisDispatchMs — this/super dispatch WASM re-parse (was already
tracked but now properly named alongside the rest)
- reclassifyMs — scoped role re-classification after edge insertion
- techniqueBackfillMs — technique-column UPDATE on native-written edges
Previously only thisDispatchMs was reported, causing wall-clock vs
phaseSum to diverge by 1.1s+ on 1-file rebuilds and making benchmark
regressions undiagnosable from committed history.
Updates update-incremental-report.ts to render the new phases in a
collapsible details block under each engine's 1-file rebuild section.
Closes #1434
…ld for required-tier grammars The docstring claimed pool cost was "amortised over enough parse work" — measurements show IPC overhead scales linearly (~55–64ms/file pool vs ~8–10ms/file inline). The real motivation is crash safety for exotic WASM grammars (#965); JS/TS/TSX (required-tier, used in all this-dispatch backfill calls) have never triggered the V8 fatal crash class and are safe to run inline. Raise threshold 16 → 32 to keep typical this-dispatch batches (≤ 18 files on the codegraph corpus) on the inline fast path. Exotic-language drops are almost always well under 32 files and also benefit from the inline path without meaningful crash risk increase. Closes #1435
…e incremental rebuilds On 1-file native incremental builds, two JS post-passes ran unconditionally even when they had no work to do: - `backfillNativeDroppedFiles`: called whenever changedCount > 0, even when detectDroppedLanguageGap returned an empty gap. Gate now checks gap.missingAbs.length > 0 || gap.staleRel.length > 0 directly, matching backfillNativeDroppedFiles's own internal early-exit guard. - Node/edge COUNT(*) re-count: ran unconditionally after all post-passes even when none of them wrote any edges. COUNT(*) over 50K+ edge tables is non-trivial, especially via the NativeDbProxy napi-rs round-trip. Now gated on postPassWroteData (backfill | CHA edges | this-dispatch edges). Closes #1454
The post-pass it timed (runPostNativePrototypeMethods) was deleted in b5c03a2 when func-prop extraction moved to Rust (#1432). The optional field was never set by any code path that survived the deletion. Also remove the stale reference to "prototype-methods post-pass" from the parseFilesWasmForBackfill docstring — only the this-dispatch post-pass uses symbolsOnly now. Closes #1432
… collision Field type annotations (`private repo: OrderRepository`) were seeded as bare file-wide typeMap keys, causing `this.repo` inside `UserService` to resolve to `OrderRepository` when both classes had a `repo` field (issue #1458). Both extractors (TS `handleFieldDefTypeMap` and Rust `field_definition` branch) now seed `ClassName.field` keys at confidence 0.9, matching the `CallerClass.X` resolver fallback added in PR #1382. Bare keys are kept at confidence 0.6 as fallbacks for single-class files or class expressions where no enclosing class name is available. Both engines change identically — parity preserved.
…ed names
The resolution benchmark uses WASM-built graphs where the Elixir, Julia,
and Objective-C extractors emit module-qualified symbol names (Main.run,
App.main, UserService.create_user, etc.). The expected-edges manifests
were written with bare unqualified names (run, main, create_user), so
every correctly-resolved edge appeared as a false positive and every
expected edge appeared as a false negative — causing all three languages
to show 0% precision even though resolution was working correctly.
Root cause: starting in v3.12.0, cross-module call resolution began working
for these languages (via the improved receiver-dispatch and same-class
fallback in resolveByMethodOrGlobal / build-edges.ts). With 0 edges
previously resolved, the name mismatch was invisible; once edges started
resolving, the manifests showed 17 FP (elixir), 11 FP (julia), 6 FP
(objc) — all correctly resolved edges misidentified as false positives.
Fix:
- Update all three expected-edges.json manifests to use the
module-qualified names matching actual extractor output:
elixir: Main.run, UserService.create_user, Validators.validate_user, etc.
julia: App.main, Service.create_user, Repository.new_repo, etc.
objc: full ObjC selectors (createUserWithId:name:email:, isValidEmail:, etc.)
plus add main -> run (plain C call correctly resolved)
- Ratchet THRESHOLDS for all three:
elixir: precision 0.0 -> 1.0, recall 0.0 -> 0.8 (17/21 resolved)
julia: precision 0.0 -> 1.0, recall 0.0 -> 0.7 (11/15 resolved)
objc: precision 0.0 -> 1.0, recall 0.0 -> 0.4 (6/13 resolved)
Remaining FNs are genuine unresolved edges (same-file bare calls in
elixir/julia, receiver-typed message sends in objc) — not regressions.
Closes #1447
The JS C++ and CUDA extractors had no handler for 'declaration' AST nodes,
so typeMap was never seeded for statically-typed locals (e.g. 'UserService svc;').
Without a typeMap entry for 'svc', resolveReceiverEdge had nothing to look up and
silently skipped the receiver edge.
Add handleCppDeclaration / handleCudaDeclaration to both extractors. They mirror
match_c_family_type_map ('declaration' branch) from the native Rust path: extract
the type node text and seed typeMap[varName] = { type, confidence: 0.9 } for each
identifier or init_declarator child. Primitive types (int, char, bool, …) are
skipped to avoid spurious edges.
parity-compare.mjs --langs cpp,cuda --hybrid: PARITY OK (wasm = native = hybrid)
All 3044 tests pass.
… in Rust solver
Go extractor was only seeding typeMap for var_spec and parameter_declaration,
missing short_var_declaration. Added infer_short_var_types to handle:
- x := Struct{} → conf 1.0 (composite literal)
- x := &Struct{} → conf 1.0 (address-of composite)
- x := NewFoo() / x := pkg.NewFoo() → conf 0.7 (New* factory prefix)
Python extractor was only seeding typeMap for typed_parameter and
typed_default_parameter, missing plain assignment. Added
infer_py_assignment_type to handle:
- order = Order(...) → conf 1.0 (uppercase constructor)
- obj = Module.Class(...) → conf 0.7 (uppercase module prefix, non-builtin)
Both mirror the existing JS extractors exactly. Parity check for
go and python: wasm vs native/hybrid OK.
…l, zig) Both engines used different rules for attributing calls inside variable bindings: WASM: attributed to the narrowest enclosing span regardless of kind, so local variable declarations inside fn main() shadowed the enclosing function (Zig: calls attributed to repo/svc variables instead of main), and nested let-bindings inside a Haskell do-block shadowed the top-level main binding. Native: loaded allNodes from a query that excluded 'variable' kind, so top-level Haskell bind nodes (main = do …, kind='variable') never matched in defs_with_ids, causing all calls to fall back to the file node. Unified rule implemented in findCaller (TS) and find_enclosing_caller (Rust): - Function/method definitions are preferred over any variable/constant binding as the enclosing caller scope — local var declarations inside a function body never shadow the enclosing function (fixes Zig repo/svc attribution). - When no function/method encloses the call, fall back to the WIDEST (outermost) variable/constant binding — this handles Haskell where main is a top-level bind node with kind 'variable'. Widest span is used so that nested let-bindings do not shadow the outer main binding. - File node remains the absolute last resort. Also adds 'variable' to NODE_KIND_FILTER_SQL (JS) and EDGE_NODE_KIND_FILTER (Rust pipeline.rs) so top-level variable bindings are included in the allNodes set available for caller matching. parity-compare.mjs --langs haskell,zig --hybrid: PARITY OK — 2/2 fixtures.
|
Claude finished @carlos-alm's task —— View job PR Review
Root Cause AnalysisProblem Being Solved: Both engines had different, incorrect rules for attributing call edges to their enclosing caller when variable/constant bindings were involved:
Why This Approach: The two-pass strategy (1: narrowest enclosing function/method; 2: widest outermost variable/constant binding) is the correct semantic model. Function bodies are always preferred as the enclosing scope. Only when no function encloses the call does a top-level variable binding (Haskell-style) act as the caller, and the outermost one wins to prevent inner Risk Assessment: Low–medium. The logic change is additive — it makes the two engines agree, fixes confirmed false attributions (verified by the parity tool), and the strategy handles the two known edge cases (Haskell top-level bindings, Zig local var shadows). The regression guard and resolution benchmark act as safety nets. Phase 2: Configuration Change DetectionTwo new hooks added in
No linting rules relaxed, no TypeScript strict flags disabled, no test coverage thresholds lowered. ✓ Phase 3: Test Integrity
Phase 4: Code QualityCore logic —
|
Codegraph Impact Analysis55 functions changed → 182 callers affected across 49 files
|
…and test Remove unused TypeMapEntry import from cpp.ts and cuda.ts, reformat primitive-type Set literals and test expect() calls to satisfy biome line-length rules.
Java was the only fixture where all three build paths (wasm, native, hybrid) disagreed pairwise. Bug 1 — WASM typeMap pollution: `handleJavaLocalVarDecl` used last-wins Map.set(), so the local `InMemoryUserRepository repo` in the static `createDefault()` method silently overrode the constructor parameter `UserRepository repo`. This caused WASM to bypass the interface and resolve directly to the concrete class, producing no interface edge and the wrong receiver. Fix: switch to first-wins `setTypeMapEntry` to match Rust extractor semantics. First-wins preserves the interface annotation that drives correct CHA dispatch. Bug 2 — native vs wasm/hybrid confidence mismatch: `runPostNativeCha` (native orchestrator path) used `computeConfidence − CHA_DISPATCH_PENALTY = 0.7 − 0.1 = 0.6`, while `runChaPostPass` (DB post-pass used by wasm and hybrid) hardcodes 0.8. Fix: align `runPostNativeCha` to also use 0.8. Result: all three build paths now emit identical edges and confidences. `parity-compare.mjs --langs java --hybrid` passes. Updated expected-edges.json to include both the interface declaration edge (TypeRepository.X at 0.7) and the CHA-expanded impl edge (InMemoryUserRepository.X at 0.8), which are the correct semantics for an interface-typed receiver. Closes #1469
Greptile SummaryThis PR fixes enclosing-caller attribution for Haskell and Zig by replacing the single-pass narrowest-span algorithm with a two-pass strategy: prefer the narrowest enclosing
Confidence Score: 5/5Safe to merge — the two-pass caller attribution is correctly implemented in both engines, the parity check passes, and all supporting changes are well-tested. The core attribution change (two-pass findCaller / find_enclosing_caller) is logically sound: function/method always wins over variable, and among variables the widest span wins, matching the Haskell top-level-binding case. The NODE_KIND_FILTER_SQL and EDGE_NODE_KIND_FILTER additions are conservative one-word changes. The CHA confidence alignment (hardcoded 0.8 for typed-receiver dispatch) unifies three previously-divergent code paths. The symbolsOnly propagation to parseFilesWasmInline is a clean fix for a silent no-op. All changes are covered by unit tests and the parity-compare fixture suite. No files require special attention — all changed files are internally consistent and the Rust/TS implementations mirror each other correctly. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[call site at line L] --> B{Any def spans line L?}
B -- No --> F[file node as caller]
B -- Yes --> C{Is def kind function or method?}
C -- Yes --> D{span < fn_caller_span?}
D -- Yes --> E[update fn_caller - narrowest function wins]
D -- No --> C
E --> C
C -- No --> G{Is def kind variable or constant?}
G -- Yes --> H{span > var_caller_span?}
H -- Yes --> I[update var_caller - widest variable wins]
H -- No --> G
I --> G
G -- No --> B
B -- done --> J{fn_caller found?}
J -- Yes --> K[return fn_caller - narrowest function]
J -- No --> L{var_caller found?}
L -- Yes --> M[return var_caller - widest variable binding e.g. Haskell main]
L -- No --> F
Reviews (7): Last reviewed commit: "fix: resolve merge conflicts with main" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| return fields; | ||
| } | ||
|
|
||
| /** | ||
| * Primitive C/C++ types that are never class/struct receivers. Seeding these | ||
| * into typeMap would cause spurious receiver edges (e.g. `int x` → `int`). | ||
| */ | ||
| const CPP_PRIMITIVE_TYPES = new Set([ | ||
| 'int', | ||
| 'long', | ||
| 'short', | ||
| 'unsigned', | ||
| 'signed', | ||
| 'float', | ||
| 'double', | ||
| 'char', | ||
| 'bool', | ||
| 'void', | ||
| 'wchar_t', | ||
| 'auto', | ||
| 'size_t', | ||
| 'uint8_t', | ||
| 'uint16_t', | ||
| 'uint32_t', | ||
| 'uint64_t', | ||
| 'int8_t', | ||
| 'int16_t', | ||
| 'int32_t', | ||
| 'int64_t', | ||
| 'ptrdiff_t', | ||
| 'intptr_t', | ||
| 'uintptr_t', | ||
| ]); | ||
|
|
||
| function isPrimitiveCppType(typeName: string): boolean { | ||
| // Strip qualifiers like `const`, `volatile`, `unsigned` etc. | ||
| const base = typeName.split(/\s+/).pop() ?? typeName; | ||
| return CPP_PRIMITIVE_TYPES.has(base) || CPP_PRIMITIVE_TYPES.has(typeName); | ||
| } | ||
|
|
||
| function extractCppEnumEntries(enumNode: TreeSitterNode): SubDeclaration[] { | ||
| const entries: SubDeclaration[] = []; | ||
| const body = findChild(enumNode, 'enumerator_list'); |
There was a problem hiding this comment.
cpp.ts and cuda.ts
CPP_PRIMITIVE_TYPES/CUDA_PRIMITIVE_TYPES are byte-for-byte identical, and handleCppDeclaration/handleCudaDeclaration differ only in the unwrap helper (unwrapCppDeclaratorName vs extractCudaFieldName) and the primitive predicate. Any update to the type set (e.g. adding __int128, half, or template types) will need to be made in both places, which is easy to miss. A shared utility (e.g. shared/c-family.ts exporting C_FAMILY_PRIMITIVE_TYPES and a generic handleCFamilyDeclaration) would keep both extractors in sync and make it easier to extend for future C-family grammars.
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.
Agreed — the duplication is real. Filed #1507 to track the refactoring (extracting C_FAMILY_PRIMITIVE_TYPES and a shared handleCFamilyDeclaration helper). Keeping it out of scope here to keep this PR focused on the attribution fix.
|
|
||
| for (const def of definitions) { | ||
| if (def.line <= call.line) { | ||
| const end = def.endLine || Infinity; |
There was a problem hiding this comment.
endLine: 0 as unbounded. || Infinity promotes any falsy value (including 0) to infinity, while ?? Infinity only applies to null/undefined.
| const end = def.endLine || Infinity; | |
| const end = def.endLine ?? Infinity; |
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 — changed || Infinity to ?? Infinity so that endLine: 0 (a valid single-line node) is not treated as unbounded. Good catch.
The inline CHA expansion in buildCallEdges and buildChaPostPass used computeConfidence(relPath, t.file) - CHA_DISPATCH_PENALTY for all CHA targets, producing 0.6 for cross-directory interface dispatch (same-dir = 0.7, minus 0.1 penalty). runChaPostPass (helpers.ts) and runPostNativeCha (native-orchestrator.ts) both hardcode 0.8 for interface/CHA-dispatch edges. The deduplication in runChaPostPass uses the existing DB edge as-is and skips reinsertion, so the 0.6 edges from the inline pass were never upgraded to 0.8. Fix: typed-receiver (interface) dispatch branches now use hardcoded 0.8 matching the post-pass constants. The this/super branch keeps computeConfidence-based proximity scoring to remain aligned with runPostNativeThisDispatch. parity-compare.mjs --langs typescript --hybrid goes green (was 12 edge diffs). Closes #1470 docs check acknowledged
…s unbounded Replace `|| Infinity` with `?? Infinity` in findCaller so that a definition with endLine=0 (a valid single-line node at the start of a file) is not incorrectly treated as having unbounded span.
…e/constant binding The find_enclosing_caller doc-comment incorrectly said "narrowest" for Pass 2, but the code (and the TS mirror in call-resolver.ts) correctly selects the WIDEST (outermost) enclosing variable/constant binding so that nested let-bindings inside main's do-block do not shadow main.
|
Fixed the find_enclosing_caller docstring in build_edges.rs: Pass 2 now correctly reads "widest (outermost)" instead of "narrowest" — matches both the code behavior and the TS mirror in call-resolver.ts. This was the last item flagged in the Greptile review summary. |
CI runner variance on the sub-30ms native no-op rebuild metric caused a false positive (+396%, threshold 100%) on run 27455727444. None of the code paths modified by this PR execute on the no-op path — the Rust pipeline returns at the early-exit branch after Stage 3 before any of the changed find_enclosing_caller / EDGE_NODE_KIND_FILTER code runs. Same pattern as 3.11.2:No-op rebuild.
|
Fixed the CI failure (Perf canary). The no-op rebuild regression (23 → 114ms, +396%) is CI runner variance — none of the changed code paths execute on the no-op rebuild path (the Rust pipeline returns at the early-exit branch after Stage 3 before any extraction, edge building, or find_enclosing_caller runs). Added |
1 participant
Both engines used different rules for attributing calls inside variable bindings:
fn main()shadowed the enclosing function (Zig: calls attributed torepo/svcvariables instead ofmain), and nestedlet-bindings inside a Haskell do-block shadowed the top-levelmainbinding.allNodesfrom a query that excludedvariablekind, so top-level Haskell bind nodes (main = do …, kind=variable) never matched indefs_with_ids, causing all calls to fall back to the file node.Unified rule
Implemented in
findCaller(TS) andfind_enclosing_caller(Rust):repo/svcattribution).mainis a top-level bind node with kindvariable. Widest span is used so that nestedlet-bindings do not shadow the outermainbinding.Also adds
variabletoNODE_KIND_FILTER_SQL(JS) andEDGE_NODE_KIND_FILTER(Rustpipeline.rs) so top-level variable bindings are included in theallNodesset available for caller matching.kindfield added toDefWithIdstruct in Rust.Verification
All 4 Haskell calls now correctly attributed to
main(variable). ZigUserRepository.init/UserService.initcalls correctly attributed tomain(function).Closes #1468