fix(edge_builder): restrict same-file this-dispatch fallback to caller's own class#1343
Conversation
…r's own class When a file contains multiple unrelated classes all defining a method with the same name, the broader same-file suffix scan emitted false-positive call edges (e.g. this.area() in Shape.describe matched Calculator.area and Formatter.area). The fix: when the scan finds more than one method with the matching suffix, restrict the result to methods whose qualified name starts with the caller's own class prefix. A single unambiguous match is returned as-is (handles the CHA case of one subclass override). If multiple classes match and none is the caller's class, return nothing rather than emitting false edges. WASM tests are active; native tests marked todo pending next binary release. Closes #1324 docs check acknowledged
|
Claude finished @carlos-alm's task —— View job PR Review — fix(edge_builder): restrict same-file this-dispatch fallback to caller's own class
Root Cause AnalysisProblem Being Solved: In Why This Approach: The fix is surgical and correctly distinguishes two sub-cases:
This mirrors the WASM path's behavior: Risk Assessment: Low risk. The logic change is in a well-isolated branch (lines 474–489 of Phase 2: Configuration ChangesNone. No linting rules, build config, TypeScript flags, or test thresholds were modified. Phase 3: Test IntegrityThe
One concern worth noting: the test uses No red flags: no assertions were weakened, no coverage thresholds lowered, no existing tests modified. Phase 4: Code Quality
let suffix = format!(".{}", call.name);
if let Some(file_nodes) = ctx.nodes_by_file.get(rel_path) {
let same_file_methods: Vec<&NodeInfo> = file_nodes.iter()
.filter(|n| n.kind == "method" && n.name.ends_with(&suffix))
.copied()
.collect();
match same_file_methods.len() {
0 => {}
1 => return same_file_methods,
_ => {
if let Some(dot_pos) = caller_name.find('.') {
let caller_prefix = format!("{}.", &caller_name[..dot_pos]);
let caller_scoped: Vec<&NodeInfo> = same_file_methods.iter()
.filter(|n| n.name.starts_with(&caller_prefix))
.copied()
.collect();
if !caller_scoped.is_empty() { return caller_scoped; }
}
}
}
}Minor concern — double prefix extraction (lines 453–460 and 480–488): The caller's class prefix is extracted twice in the same branch: once for the "class-scoped exact lookup" (line 453) and again for the "broader same-file suffix scan" (line 480). The variables are Correctness of Correctness of
Backlog Compliance
Critical ConcernsNone blocking. One suggestion (non-blocking): The double Future obligation: The Final Recommendation
|
Codegraph Impact Analysis12 functions changed → 7 callers affected across 3 files
|
Greptile SummaryThis PR fixes a false-positive edge generation bug in
Confidence Score: 5/5The change is narrowly scoped to one fallback branch in the call-resolution pipeline and is safe to merge. The Rust fix is logically sound and both the multi-match and single-match false-positive paths are covered by the new integration tests. The intentional removal of cross-class CHA subclass-override detection from the suffix scan is guarded by the existing phase-8.5-cha-dispatch test suite. No files require special attention; both comments are minor observations. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["resolve_call_targets"] --> B["Steps 1-4: exact, same-file,\nmethod match, type-aware"]
B -- "found" --> Z["return targets"]
B -- "not found" --> C["receiver is this/self/super"]
C --> D["Class-scoped exact lookup\nShape.area in nodes_by_name"]
D -- "found" --> Z
D -- "not found" --> E["Same-file suffix scan\nfile_nodes ending with .area"]
E -- "empty" --> F["return empty"]
E -- "non-empty" --> G["caller_name has dot?"]
G -- "no" --> F
G -- "yes: prefix = Shape." --> H["Filter: starts_with Shape."]
H -- "non-empty" --> Z
H -- "empty" --> F
Reviews (4): Last reviewed commit: "Merge branch 'main' into fix/edge-builde..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if !same_file_methods.is_empty() { return same_file_methods; } | ||
| match same_file_methods.len() { | ||
| 0 => {} | ||
| 1 => return same_file_methods, |
There was a problem hiding this comment.
The 1 => return same_file_methods arm returns the sole match without verifying it belongs to the caller's class. This leaves the original false-positive intact for a very common real-world layout: a file with Shape (no area() method) and exactly one other class Calculator with area(). The class-scoped lookup finds nothing, the suffix scan finds 1 entry (Calculator.area), and it's returned as a Shape.describe → Calculator.area edge — the exact bug being fixed for 2+ matches. Since the surrounding if call.receiver.is_some() guard already limits us to this/self/super dispatch (so caller_name should always have a dot), applying the caller-prefix filter to the 1-match arm would close the same hole consistently. The CHA subclass-override scenario could be preserved by falling through to that arm only when the single match does start with caller_prefix.
There was a problem hiding this comment.
Fixed — the single-match arm now applies the same caller-class prefix filter as the multi-match arm. The match same_file_methods.len() block has been replaced with a unified if !same_file_methods.is_empty() + caller_prefix filter that runs for any number of matches. A new fixture tests/fixtures/this-dispatch-scope/single-sibling.ts (two classes: Caller with no area(), Sibling with area()) plus a corresponding WASM assertion verify that Caller.run → Sibling.area is not emitted.
|
|
||
| describe.each(ENGINES)('this-dispatch scope (%s)', (engine) => { | ||
| let tmpDir: string; | ||
| let callEdges: CallEdgeRow[]; |
There was a problem hiding this comment.
callEdges is declared without an initialiser. If buildGraph rejects (network hiccup, missing binary, etc.), Vitest marks all tests in the suite as failed with the beforeAll error, but if it surfaces before that, any test body reaching callEdges.find(…) throws a cryptic "variable used before assignment" instead of a clean failing assertion. Initialising to [] makes the failure mode explicit.
| let callEdges: CallEdgeRow[]; | |
| let callEdges: CallEdgeRow[] = []; |
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 to let callEdges: CallEdgeRow[] = []; so any buildGraph rejection produces a clear failing assertion rather than a cryptic "variable used before assignment" error.
…an too (#1343) The 1-match arm of the same-file suffix scan was returning any sole method regardless of whether it belonged to the caller's class. A file with Caller (no area()) and Sibling (area()) would produce a false Caller.run → Sibling.area edge — the same bug fixed for 2+ matches. Replace the three-arm match with a unified caller-prefix filter applied to all non-empty results. Add fixture single-sibling.ts and a corresponding WASM assertion to cover the single-match false-positive path. Initialize callEdges to [] for cleaner beforeAll failure mode.
|
Claude finished @carlos-alm's task —— View job PR Review — Updated State
Both Greptile issues are correctly addressed. Issue 1 — Single-match arm (edge_builder.rs:474–484): The Issue 2 — callEdges initializer (test.ts:52): let callEdges: CallEdgeRow[] = [];Confirmed initialized to New fixture One minor observation: The test at line 65 ( Both concerns are fully resolved. The PR is ready to merge. |
1 participant
Summary
edge_builder.rs, the broader same-file suffix scan forthis/self/superdispatch returned ALL methods with the matching name in the file, including from unrelated classes. A file containingShape,Calculator, andFormatterall witharea()would produce false-positive edgesShape.describe → Calculator.areaandShape.describe → Formatter.area.tests/fixtures/this-dispatch-scope/shapes.ts+ integration testthis-dispatch-scope.test.ts. WASM assertions are active; native assertions aretodountil the next binary release includes this Rust fix.Closes #1324
Test plan
npx vitest run tests/integration/this-dispatch-scope.test.ts— 4 pass, 2 todonpx vitest run tests/integration/build-parity.test.ts tests/integration/pts-parity.test.ts tests/integration/phase-8.5-cha-dispatch.test.ts— all pass, no regressionsit.todoguards and confirm native assertions pass