fix(cha): CHA dispatch omits the receiver's own instantiated type - #2494
Merged
Conversation
resolveChaTargets/resolve_cha_dispatch only walked implementors (subclasses) starting from the receiver's declared type, never checking whether that type itself is instantiated. When a class is instantiated directly AND an unrelated file also declares a local subclass overriding the same method (e.g. a test double), the base class's own method was dropped from the resolved edge set while the unrelated subclass's override leaked in instead (#2348). Both engines now also resolve the receiver's own type via resolveMethodViaAncestors/resolve_method_via_ancestors, gated on strict new-expression evidence only (a new newExpressionTypes/ cha_new_expression_types set) rather than the merged instantiatedTypes/ cha_instantiated_types set (which also credits a bare high-confidence type annotation as "instantiated"). The merged set is too broad for this particular check: it would resurrect a distant interface's own bodyless method whenever some unrelated concrete subclass overrides the same method name, which regressed two existing native unit tests (cha_typed_dispatch_fallback_resolves_distant_interface_implementation and ..._respects_rta_filter) until this stricter gate was added. Adds a dual-engine regression test (issue-2348-cha-base-type-own-method.test.ts) reproducing the shape with a synthetic base class + two unrelated local override subclasses, verified to fail on both engines with the fix disabled and pass with it enabled. docs check acknowledged Impact: 6 functions changed, 0 affected
Contributor
Greptile SummaryThis follow-up scopes receiver-own-type instantiation evidence by declaring file, preventing unrelated same-named local types from producing spurious CHA targets while preserving the intended base-type edge.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains. Important Files Changed
Reviews (2): Last reviewed commit: "fix(cha): scope receiver-own-type RTA ev..." | Re-trigger Greptile |
Contributor
Codegraph Impact Analysis5 functions changed → 22 callers affected across 8 files
|
Greptile review on PR #2494: newExpressionTypes/cha_new_expression_types (added for #2348's receiver-own-type check) was a bare, project-wide set with no file scoping, unlike implementorsByFile/cha_implementors_by_file which this codebase already splits carefully for the same reason (#2237). Two unrelated files can each declare their own unrelated class with the same bare name; if only one is ever instantiated, the bare set couldn't tell them apart, and resolveMethodViaAncestors/ resolve_method_via_ancestors' own bare/global fallback could then resolve to the OTHER file's method. Both engines now also track, per file: which type names it locally declares (declaredTypeNamesByFile/cha_declared_type_names_by_file, mirroring the same local-declaration anchor recordImplements/ recordExtends already use for implementorsByFile) and which type names its own new-expression evidence names (newExpressionTypesByFile/cha_new_expression_types_by_file). The root-type check now prefers the file-scoped pair whenever the caller's file locally declares the receiver type - trusting a scoped miss as authoritative - and only falls back to the bare set when the caller's file has no local declaration to anchor against (the same accepted limitation implementorsByFile already has in that case). Adds a dual-engine regression test proving the disambiguation (three TS unit tests in cha.test.ts covering the collision, the same-file positive case, and the no-anchor fallback; one Rust integration-style unit test through build_call_edges), plus re-verifies the #2348 regression test and the real-world in-memory-repository.test.ts repro are unaffected on both engines. docs check acknowledged Impact: 7 functions changed, 0 affected
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
in-memory-repository.test.ts→BrokenRepo.findNodesForTriagein an unrelated test file).resolveChaTargets(cha.ts) /resolve_cha_dispatch(build_edges.rs) BFS-walk only ever considered the receiver's subclasses as dispatch targets — it never checked whether the receiver's own declared type is itself instantiated and should resolve to its own method. When a base class (InMemoryRepository) is instantiated directly AND an unrelated file declares a local subclass overriding the same method (test doublesBrokenRepo/InvalidOptsRepo, declared insideit()callbacks in a completely different test file), the base class's own correct method was silently dropped from the resolved edge set while the unrelated subclass's override leaked in instead.resolveMethodViaAncestors/resolve_method_via_ancestors, gated on a new strict new-expression-only evidence set (newExpressionTypes/cha_new_expression_types) rather than the mergedinstantiatedTypes/cha_instantiated_typesset (which also credits a bare high-confidence type annotation as "instantiated" — too weak for this particular check, since it would resurrect a distant interface's own bodyless method whenever an unrelated concrete subclass overrides the same method name elsewhere). This distinction was discovered because applying the naive fix against the merged set regressed two existing native unit tests.seenCallEdgesdedup makes this fix strictly additive — it can only add a previously-missing edge, never introduce a duplicate or wrong one.Test plan
tests/integration/issue-2348-cha-base-type-own-method.test.ts— synthetic fixture mirroring the real repro shape (base class instantiated directly + two unrelated local subclass overrides in separate files); verified to fail on both engines with the fix disabled, pass with it enabledtests/unit/in-memory-repository.test.ts/tests/integration/triage.test.ts):InMemoryRepository.findNodesForTriagenow correctly appears as a target for both engines (additive alongside the pre-existing, harmless spurious edges)cargo test --workspace: 1011 passed (was 1009 passed / 2 failed against the naive fix, before the strict-evidence refinement)cargo fmt --check/cargo clippy --workspace --all-targets -- -D warnings: cleannpm run lintcleanCloses #2348