diff --git a/crates/codegraph-core/src/extractors/dart.rs b/crates/codegraph-core/src/extractors/dart.rs index af07472a1..db76d3f4e 100644 --- a/crates/codegraph-core/src/extractors/dart.rs +++ b/crates/codegraph-core/src/extractors/dart.rs @@ -923,6 +923,79 @@ fn find_enclosing_dart_param_list_for_call<'a>(node: &Node<'a>) -> Option bool { + let mut current = *node; + loop { + let Some(ancestor) = current.parent() else { + return false; + }; + if ancestor.kind() == "block" { + let mut idx: i64 = -1; + for i in 0..ancestor.child_count() { + if ancestor.child(i).map(|c| c.id()) == Some(current.id()) { + idx = i as i64; + break; + } + } + let mut i = idx - 1; + while i >= 0 { + if let Some(sibling) = ancestor.child(i as usize) { + if sibling.kind() == "local_variable_declaration" { + if let Some(decl) = find_child(&sibling, "initialized_variable_definition") + { + if let Some(name_node) = decl.child_by_field_name("name") { + if node_text(&name_node, source) == name { + return true; + } + } + } + } + } + i -= 1; + } + } + if ancestor.kind() == "function_body" { + return false; + } + current = ancestor; + } +} + /// Qualified name (`ClassName.methodName`, or bare `functionName`) of the /// function/method enclosing `node`, a descendant of that function's /// `function_body` — e.g. a local variable declaration inside the body @@ -1210,6 +1283,9 @@ fn find_dart_selector_receiver(method_selector: &Node, source: &[u8]) -> Option< return Some(name.to_string()); } } + if find_enclosing_dart_shadowing_local_name(method_selector, source, name) { + return Some(name.to_string()); + } Some(format!("this.{}", name)) } "type_identifier" => Some(node_text(&prev_sibling, source).to_string()), @@ -1294,8 +1370,10 @@ fn handle_dart_call_expression(node: &Node, source: &[u8], symbols: &mut FileSym // rationale (#2319 second follow-up, Greptile finding on PR // #2477: this is likewise the LIVE path for that shadowing fix, // for the same reason it's the live path for the `this.`-prefix - // fix above). Mirrors `findDartSelectorReceiver` in - // `src/extractors/dart.ts`. + // fix above) — OR by a same-named LOCAL VARIABLE declared + // earlier in an enclosing block (#2478; see + // `find_enclosing_dart_shadowing_local_name`'s doc comment). + // Mirrors `findDartSelectorReceiver` in `src/extractors/dart.ts`. let receiver = object.and_then(|obj| { if obj.kind() != "identifier" { return None; @@ -1306,6 +1384,9 @@ fn handle_dart_call_expression(node: &Node, source: &[u8], symbols: &mut FileSym return Some(name.to_string()); } } + if find_enclosing_dart_shadowing_local_name(node, source, name) { + return Some(name.to_string()); + } Some(format!("this.{}", name)) }); @@ -1710,12 +1791,11 @@ mod tests { "missing doSomething call; got: {:?}", s.calls ); - // Also `this.`-prefixed even though `w` is a local, not a field: - // the extractor cannot tell the two apart from a bare identifier - // alone, and prefixing is harmless here — the class-scoped - // lookup it enables just finds no entry for a non-field name and - // falls through to the same bare-key lookup as before. - assert_eq!(call.unwrap().receiver.as_deref(), Some("this.w")); + // Bare, NOT `this.`-prefixed: `w` is a local variable declared + // earlier in this same block, so `find_enclosing_dart_shadowing_ + // local_name` (#2478) correctly recognizes it as a local rather + // than defaulting to a field access. + assert_eq!(call.unwrap().receiver.as_deref(), Some("w")); } #[test] @@ -1848,6 +1928,117 @@ mod tests { } } + // #2478: a LOCAL VARIABLE (not just a parameter) can also legally shadow + // a same-named class field of a different type — the counterpart to + // `parameter_shadows_field` above. + mod local_variable_shadows_field { + use super::*; + + #[test] + fn local_var_receiver_is_not_this_prefixed_when_it_shadows_a_field() { + let s = parse_dart( + "class Service {\n final Repository _repo;\n Service(this._repo);\n void run() {\n var _repo = MockRepository();\n _repo.mockOnlyMethod();\n }\n}", + ); + let call = s.calls.iter().find(|c| c.name == "mockOnlyMethod"); + assert!( + call.is_some(), + "missing mockOnlyMethod call; got: {:?}", + s.calls + ); + assert_eq!( + call.unwrap().receiver.as_deref(), + Some("_repo"), + "a local variable shadowing a field must emit the BARE receiver, not \ + `this.`-prefixed (which would wrongly activate the class-scoped FIELD lookup)" + ); + } + + #[test] + fn does_not_shadow_when_declared_in_a_different_method() { + let s = parse_dart( + "class Service {\n final Repository _repo;\n Service(this._repo);\n void run() {\n var _repo = MockRepository();\n _repo.mockOnlyMethod();\n }\n void other() {\n _repo.findById();\n }\n}", + ); + let call = s.calls.iter().find(|c| c.name == "findById"); + assert!(call.is_some(), "missing findById call; got: {:?}", s.calls); + assert_eq!(call.unwrap().receiver.as_deref(), Some("this._repo")); + } + + #[test] + fn does_not_shadow_across_sibling_blocks() { + // The local's scope ends with the `if` block that declares it — + // a call AFTER that block, back in the outer method body, must + // still resolve against the field. + let s = parse_dart( + "class Service {\n final Repository _repo;\n Service(this._repo);\n void run(bool cond) {\n if (cond) {\n var _repo = MockRepository();\n _repo.mockOnlyMethod();\n }\n _repo.findById();\n }\n}", + ); + let inner = s.calls.iter().find(|c| c.name == "mockOnlyMethod"); + assert_eq!( + inner.and_then(|c| c.receiver.as_deref()), + Some("_repo"), + "call inside the if-block must still see the local as shadowing" + ); + let outer = s.calls.iter().find(|c| c.name == "findById"); + assert_eq!( + outer.and_then(|c| c.receiver.as_deref()), + Some("this._repo"), + "call outside the if-block must NOT be shadowed by a local scoped to a sibling block" + ); + } + + #[test] + fn shadows_from_an_enclosing_block_into_a_nested_call_site() { + // The inverse of the sibling-block case: the local is declared + // in the OUTER block, and the call is nested inside an `if` + // block within that same enclosing scope — the local is still + // in scope there. + let s = parse_dart( + "class Service {\n final Repository _repo;\n Service(this._repo);\n void run(bool cond) {\n var _repo = MockRepository();\n if (cond) {\n _repo.mockOnlyMethod();\n }\n }\n}", + ); + let call = s.calls.iter().find(|c| c.name == "mockOnlyMethod"); + assert_eq!(call.and_then(|c| c.receiver.as_deref()), Some("_repo")); + } + + #[test] + fn does_not_shadow_a_call_textually_before_the_local_declaration() { + // Even in the SAME block, a local declared AFTER the call site + // must not retroactively shadow it — ordering is checked + // directly from sibling position, not merely "does a + // same-named local exist anywhere in this block". + let s = parse_dart( + "class Service {\n final Repository _repo;\n Service(this._repo);\n void run() {\n _repo.findById();\n var _repo = MockRepository();\n _repo.mockOnlyMethod();\n }\n}", + ); + let before = s.calls.iter().find(|c| c.name == "findById"); + assert_eq!( + before.and_then(|c| c.receiver.as_deref()), + Some("this._repo"), + "a call preceding the local's own declaration must still resolve against the field" + ); + let after = s.calls.iter().find(|c| c.name == "mockOnlyMethod"); + assert_eq!(after.and_then(|c| c.receiver.as_deref()), Some("_repo")); + } + + #[test] + fn end_to_end_resolution_does_not_target_the_field_type() { + // Full end-to-end regression mirroring parameter_shadows_field's + // identical test: both types define a same-named method, so a + // wrong (field-typed) resolution would be indistinguishable + // from a correct one without checking both the receiver AND the + // typeMap entry the resolver actually consumes. + let s = parse_dart( + "class Repository {\n void save() {}\n}\nclass MockRepository {\n void save() {}\n}\nclass Service {\n final Repository _repo;\n Service(this._repo);\n void run() {\n var _repo = MockRepository();\n _repo.save();\n }\n}", + ); + let call = s.calls.iter().find(|c| c.name == "save"); + assert!(call.is_some(), "missing save call; got: {:?}", s.calls); + assert_eq!(call.unwrap().receiver.as_deref(), Some("_repo")); + let scoped = s + .type_map + .iter() + .find(|e| e.name == "Service.run::_repo") + .expect("missing Service.run::_repo scoped type-map entry"); + assert_eq!(scoped.type_name, "MockRepository"); + } + } + // #2474: `var svc = UserService(repo);` never seeded a typeMap entry for // `svc`, unlike every other language extractor's identical // constructor-call-initializer convention — so a later call through it diff --git a/src/extractors/dart.ts b/src/extractors/dart.ts index 4da40c02e..3d95cd5b3 100644 --- a/src/extractors/dart.ts +++ b/src/extractors/dart.ts @@ -422,9 +422,13 @@ function handleDartFormalParamTypeMap(node: TreeSitterNode, ctx: ExtractorOutput * dependent and diverge from the (order-independent) native engine. Both * options are out of scope for this fix — tracked in #2568. * - * Deliberately does not attempt to detect a LOCAL VARIABLE shadowing a class - * field of the same name (only a shadowing PARAMETER is handled elsewhere, - * via `findDartSelectorReceiver`) — tracked separately as #2478. + * Seeds unconditionally, regardless of whether this local happens to shadow + * a same-named class field — `findDartSelectorReceiver` / + * `findEnclosingDartShadowingLocalName` (#2478) is what decides, at each + * call site, whether a bare receiver should resolve against this seeded + * local-scoped entry or the field's own class-scoped one; this function + * only needs to make the local's own type available for that later lookup + * to find. */ function handleDartLocalVarTypeMap(node: TreeSitterNode, ctx: ExtractorOutput): void { const nameNode = node.childForFieldName('name'); @@ -613,6 +617,77 @@ function findEnclosingDartParamListForCall(node: TreeSitterNode): TreeSitterNode return findChild(inner, 'formal_parameter_list'); } +/** + * Whether `name` is shadowed by a LOCAL VARIABLE declared earlier in an + * ancestor block of `node` — e.g. `_repo` in: + * + * void run() { + * var _repo = MockRepository(); + * _repo.mockOnlyMethod(); // shadowed: resolves the LOCAL, not the field + * } + * + * (#2478, the local-variable counterpart to `collectDartParamNames`'s + * parameter-shadowing check.) A parameter is trivially in scope for the + * WHOLE function body with no ordering to consider, but a local variable's + * scope is block-bounded and position-dependent: a same-named local + * declared in a DIFFERENT (sibling) block, or later in the SAME block, must + * NOT be treated as shadowing. + * + * Walks up from `node` one enclosing `block` at a time. At each level, the + * child of that block on the path up from `node` is the "entry statement" + * — only that block's siblings BEFORE the entry statement's own index are + * checked: a local variable declared AFTER it in the same block is not yet + * in scope there, and one declared inside a DIFFERENT branch of an + * `if`/`for` lives in a sibling block this walk never visits at all, so it + * can't falsely match either. This verifies the ordering directly from the + * tree rather than assuming Dart's compiler already rejects a forward + * reference. + * + * Stops at the enclosing `function_body` boundary, mirroring + * `findEnclosingDartParamListForCall`'s identical discipline against + * crossing into an outer function/class scope. + * + * Deliberately does NOT walk into nested descendant blocks the call site + * itself isn't inside (e.g. a local declared inside an `if` whose block + * doesn't contain the call) — those are simply never visited by the + * ancestor walk, so this cannot over-detect shadowing there. Nor does it + * chase the rare, unusual-style comma-separated multi-declarator local + * (`var a, b = Foo();`) — tree-sitter-dart's grammar folds the second + * declarator into an oddly-shaped `initialized_identifier` sibling rather + * than a second `initialized_variable_definition`, and this only reads the + * primary `name` field. Missing that rare shape is a conservative + * under-detection (falls through to the pre-existing `this.`-prefixed + * behavior), not a wrong one — matching this file's own "don't guess" + * convention for every other case `findDartSelectorReceiver` leaves + * unhandled. + */ +function findEnclosingDartShadowingLocalName(node: TreeSitterNode, name: string): boolean { + let current: TreeSitterNode | null = node; + while (current) { + const ancestor: TreeSitterNode | null = current.parent; + if (!ancestor) return false; + if (ancestor.type === 'block') { + let idx = -1; + for (let i = 0; i < ancestor.childCount; i++) { + if (ancestor.child(i)?.id === current.id) { + idx = i; + break; + } + } + for (let i = idx - 1; i >= 0; i--) { + const sibling = ancestor.child(i); + if (sibling?.type !== 'local_variable_declaration') continue; + const decl = findChild(sibling, 'initialized_variable_definition'); + const nameNode = decl?.childForFieldName('name'); + if (nameNode?.text === name) return true; + } + } + if (ancestor.type === 'function_body') return false; + current = ancestor; + } + return false; +} + /** * The `method_signature`/`function_signature`/`constructor_signature` node * enclosing `node`, where `node` is some descendant of that function's @@ -1012,10 +1087,16 @@ function resolveDartSelectorCall(node: TreeSitterNode): DartSelectorCall | null * parameter's own type instead — see that function's doc comment for why * the bare fallback key ALONE (i.e. simply not prefixing, with no * function-scoped seeding) is NOT sufficient to avoid resolving against the - * field's type. Only a shadowing PARAMETER is detected this way — a - * shadowing LOCAL VARIABLE declaration is a materially bigger, deliberately - * out-of-scope problem tracked in #2478. + * field's type. + * + * A shadowing LOCAL VARIABLE declaration (`var _repo = MockRepository(); + * _repo.mockOnlyMethod();` inside a class whose own `_repo` field is a + * different type) is detected the same way, via + * `findEnclosingDartShadowingLocalName` (#2478) — see that function's doc + * comment for why block-scoping and declaration order can be checked + * directly from the tree without needing full control-flow analysis. * + * A `type_identifier` sibling (a class/type name used as a static-call * receiver, e.g. `MyClass.staticMethod()`) is deliberately left UNPREFIXED — * it never denotes a field access, so there is no class-scoped key for it @@ -1047,6 +1128,9 @@ function findDartSelectorReceiver(methodSelector: TreeSitterNode): string | unde if (paramList && collectDartParamNames(paramList).has(prevSibling.text)) { return prevSibling.text; } + if (findEnclosingDartShadowingLocalName(methodSelector, prevSibling.text)) { + return prevSibling.text; + } return `this.${prevSibling.text}`; } if (prevSibling?.type === 'type_identifier') { diff --git a/tests/integration/issue-2478-dart-local-var-shadows-field.test.ts b/tests/integration/issue-2478-dart-local-var-shadows-field.test.ts new file mode 100644 index 000000000..e46b90877 --- /dev/null +++ b/tests/integration/issue-2478-dart-local-var-shadows-field.test.ts @@ -0,0 +1,128 @@ +/** + * Integration test for #2478: Dart LOCAL VARIABLE shadowing a same-named + * class field — the counterpart to #2319's second follow-up (PR #2477), + * which only handled a shadowing PARAMETER. + * + * Dart also legally allows a local variable declaration to shadow a + * same-named class field of a DIFFERENT type for the rest of its enclosing + * block: + * + * class Service { + * final Repository _repo; + * Service(this._repo); + * void run() { + * var _repo = MockRepository(); + * _repo.mockOnlyMethod(); // means the LOCAL, not the field + * } + * } + * + * Fix: `findDartSelectorReceiver` / `find_dart_selector_receiver` / + * `handle_dart_call_expression`'s receiver extraction now also checks + * `findEnclosingDartShadowingLocalName` / `find_enclosing_dart_shadowing_ + * local_name`, which walks up the call site's enclosing `block`s (stopping + * at the function body boundary) checking each block's own preceding + * siblings for a matching `local_variable_declaration` — correctly bounded + * by both block scope (a local declared in a sibling `if`/`for` block never + * matches) and declaration order (a local declared later in the same block + * never retroactively shadows an earlier call). + * + * This fixture defines a same-named `save()` method on both the field's + * type and the shadowing local's type specifically so a WRONG (field-typed) + * resolution would produce a real, distinguishable edge rather than + * silently resolving to nothing. + */ + +import fs from 'node:fs'; +import os from 'node:os'; +import path from 'node:path'; +import Database from 'better-sqlite3'; +import { afterAll, beforeAll, describe, expect, it } from 'vitest'; +import { buildGraph } from '../../src/domain/graph/builder.js'; +import { isNativeAvailable } from '../../src/infrastructure/native.js'; + +const FIXTURE = { + 'services.dart': ` +class Repository { + void save() {} +} + +class MockRepository { + void save() {} +} + +class Service { + final Repository _repo; + + Service(this._repo); + + void run() { + var _repo = MockRepository(); + _repo.save(); + } +} +`, +}; + +function writeFixture(rootDir: string) { + for (const [rel, content] of Object.entries(FIXTURE)) { + fs.writeFileSync(path.join(rootDir, rel), content); + } +} + +function readCallEdges(dbPath: string) { + const db = new Database(dbPath, { readonly: true }); + try { + return db + .prepare( + `SELECT n1.name AS src, n2.name AS tgt + FROM edges e + JOIN nodes n1 ON e.source_id = n1.id + JOIN nodes n2 ON e.target_id = n2.id + WHERE e.kind = 'calls' + ORDER BY n1.name, n2.name`, + ) + .all() as Array<{ src: string; tgt: string }>; + } finally { + db.close(); + } +} + +function runSuite(engine: 'wasm' | 'native') { + describe(`Dart local variable shadows field (#2478) — ${engine}`, () => { + let tmpDir: string; + + beforeAll(async () => { + tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), `cg-2478-dart-shadow-${engine}-`)); + writeFixture(tmpDir); + await buildGraph(tmpDir, { engine, incremental: false, skipRegistry: true }); + }, 60_000); + + afterAll(() => { + fs.rmSync(tmpDir, { recursive: true, force: true }); + }); + + it('never resolves the shadowed call against the FIELD type (Repository.save)', () => { + const dbPath = path.join(tmpDir, '.codegraph', 'graph.db'); + const edges = readCallEdges(dbPath); + expect( + edges.some((e) => e.src === 'Service.run' && e.tgt === 'Repository.save'), + `Service.run must NOT resolve to Repository.save (the field's type); got: ${JSON.stringify(edges)}`, + ).toBe(false); + }); + + it('resolves the shadowed call against the LOCAL VARIABLE type (MockRepository.save)', () => { + const dbPath = path.join(tmpDir, '.codegraph', 'graph.db'); + const edges = readCallEdges(dbPath); + expect( + edges.some((e) => e.src === 'Service.run' && e.tgt === 'MockRepository.save'), + `Service.run -> MockRepository.save edge missing; got: ${JSON.stringify(edges)}`, + ).toBe(true); + }); + }); +} + +runSuite('wasm'); + +describe.skipIf(!isNativeAvailable())('native engine parity', () => { + runSuite('native'); +}); diff --git a/tests/parsers/dart.test.ts b/tests/parsers/dart.test.ts index 5336e679a..afba0e2c6 100644 --- a/tests/parsers/dart.test.ts +++ b/tests/parsers/dart.test.ts @@ -371,13 +371,12 @@ import 'package:flutter/material.dart';`); var w = Foo(); w.doSomething(); }`); - // Also `this.`-prefixed even though `w` is a local, not a field: the - // extractor cannot tell the two apart from a bare identifier alone, - // and prefixing is harmless here — the class-scoped lookup it enables - // just finds no entry for a non-field name and falls through to the - // same bare-key lookup as before. + // Bare, NOT `this.`-prefixed: `w` is a local variable declared earlier + // in this same block, so `findEnclosingDartShadowingLocalName` (#2478) + // correctly recognizes it as a local rather than defaulting to a + // field access. expect(symbols.calls).toContainEqual( - expect.objectContaining({ name: 'doSomething', receiver: 'this.w' }), + expect.objectContaining({ name: 'doSomething', receiver: 'w' }), ); }); @@ -529,6 +528,130 @@ class Service { }); }); + // #2478: a LOCAL VARIABLE (not just a parameter) can also legally shadow + // a same-named class field of a different type — the counterpart to the + // #2319 second follow-up above. + describe('#2478: local variable shadowing a same-named class field', () => { + it('emits the bare receiver (not `this.`-prefixed) when a local shadows the field', () => { + const symbols = parseDart(`class Service { + final Repository _repo; + Service(this._repo); + void run() { + var _repo = MockRepository(); + _repo.mockOnlyMethod(); + } +}`); + expect(symbols.calls).toContainEqual( + expect.objectContaining({ name: 'mockOnlyMethod', receiver: '_repo' }), + ); + }); + + it('does not shadow a bare field access in a sibling method with no such local', () => { + const symbols = parseDart(`class Service { + final Repository _repo; + Service(this._repo); + void run() { + var _repo = MockRepository(); + _repo.mockOnlyMethod(); + } + void other() { + _repo.findById(); + } +}`); + expect(symbols.calls).toContainEqual( + expect.objectContaining({ name: 'findById', receiver: 'this._repo' }), + ); + }); + + it('does not shadow across sibling blocks', () => { + // The local's scope ends with the `if` block that declares it — a + // call AFTER that block, back in the outer method body, must still + // resolve against the field. + const symbols = parseDart(`class Service { + final Repository _repo; + Service(this._repo); + void run(bool cond) { + if (cond) { + var _repo = MockRepository(); + _repo.mockOnlyMethod(); + } + _repo.findById(); + } +}`); + expect(symbols.calls).toContainEqual( + expect.objectContaining({ name: 'mockOnlyMethod', receiver: '_repo' }), + ); + expect(symbols.calls).toContainEqual( + expect.objectContaining({ name: 'findById', receiver: 'this._repo' }), + ); + }); + + it('shadows from an enclosing block into a nested call site', () => { + // The inverse of the sibling-block case: the local is declared in the + // OUTER block, and the call is nested inside an `if` block within + // that same enclosing scope — the local is still in scope there. + const symbols = parseDart(`class Service { + final Repository _repo; + Service(this._repo); + void run(bool cond) { + var _repo = MockRepository(); + if (cond) { + _repo.mockOnlyMethod(); + } + } +}`); + expect(symbols.calls).toContainEqual( + expect.objectContaining({ name: 'mockOnlyMethod', receiver: '_repo' }), + ); + }); + + it('does not shadow a call textually before the local declaration', () => { + // Even in the SAME block, a local declared AFTER the call site must + // not retroactively shadow it — ordering is checked directly from + // sibling position, not merely "does a same-named local exist + // anywhere in this block". + const symbols = parseDart(`class Service { + final Repository _repo; + Service(this._repo); + void run() { + _repo.findById(); + var _repo = MockRepository(); + _repo.mockOnlyMethod(); + } +}`); + expect(symbols.calls).toContainEqual( + expect.objectContaining({ name: 'findById', receiver: 'this._repo' }), + ); + expect(symbols.calls).toContainEqual( + expect.objectContaining({ name: 'mockOnlyMethod', receiver: '_repo' }), + ); + }); + + it('end-to-end: does not resolve the shadowed call against the field type', () => { + const symbols = parseDart(`class Repository { + void save() {} +} +class MockRepository { + void save() {} +} +class Service { + final Repository _repo; + Service(this._repo); + void run() { + var _repo = MockRepository(); + _repo.save(); + } +}`); + expect(symbols.calls).toContainEqual( + expect.objectContaining({ name: 'save', receiver: '_repo' }), + ); + expect(symbols.typeMap.get('Service.run::_repo')).toEqual({ + type: 'MockRepository', + confidence: 0.7, + }); + }); + }); + // #2474: `var svc = UserService(repo);` never seeded a typeMap entry for // `svc`, unlike every other language extractor's identical // constructor-call-initializer convention — so a later call through it