fix: detect Dart local variable shadowing a class field, in both engines - #2571
Merged
Conversation
Dart's #2319 second follow-up (PR #2477) only detected a shadowing PARAMETER, leaving a shadowing LOCAL VARIABLE declaration wrongly resolved (or dropped) against the field's own type -- a parameter is trivially in scope for the whole function body, but a local variable's scope is block-bounded and position-dependent, which is why this was scoped out at the time as needing more design work. Adds findEnclosingDartShadowingLocalName / find_enclosing_dart_ shadowing_local_name: walks up the call site's enclosing blocks one level at a time (stopping at the function_body boundary), and at each level only checks that block's siblings BEFORE the entry statement's own index for a matching local_variable_declaration. This verifies declaration order and block scope directly from the tree -- a local declared later in the same block, or in a sibling if/for block, can never falsely match -- rather than assuming anything about how Dart's own compiler treats forward references. Confirmed via parse dumps that both engines' grammars produce the same block/local_variable_declaration/initialized_variable_definition shape for this scenario, so the fix mirrors identically on both sides. docs check acknowledged. Closes #2478 Impact: 2 functions changed, 4 affected
Contributor
Greptile SummaryThe PR updates both Dart extraction engines to distinguish a block-scoped local variable from a same-named class field when extracting method-call receivers.
Confidence Score: 5/5The PR appears safe to merge, with no concrete actionable defects established in the changed paths. The mirrored extractors apply the same block- and order-aware receiver classification, and the new tests cover the primary scope transitions and end-to-end resolution behavior. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
Call["Dart receiver call: name.method()"] --> Param{"Same-named parameter?"}
Param -->|Yes| Bare["Emit bare receiver: name"]
Param -->|No| Local{"Earlier matching local in an enclosing block?"}
Local -->|Yes| Bare
Local -->|No| Field["Emit field receiver: this.name"]
Bare --> LocalType["Resolve function-scoped local type"]
Field --> FieldType["Resolve class-scoped field type"]
Reviews (1): Last reviewed commit: "fix: detect Dart local variable shadowin..." | Re-trigger Greptile |
Contributor
Codegraph Impact Analysis2 functions changed → 4 callers affected across 1 files
|
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
The #2319 second follow-up (PR #2477) only detected a shadowing parameter, explicitly scoping out a shadowing local variable declaration (#2478) as needing more design work — a parameter is trivially in scope for the whole function body, but a local variable's scope is block-bounded and position-dependent.
Rather than leaving this deferred, I worked through a design that avoids the hard part (full control-flow-order reasoning, or relying on any assumption about whether Dart's compiler itself rejects a forward reference) by checking ordering and block-scope directly from the tree:
findEnclosingDartShadowingLocalName/find_enclosing_dart_shadowing_local_namewalks up from the call site one enclosingblockat a time, stopping at thefunction_bodyboundary. At each level, only that block's siblings before the entry statement's own index are checked for a matchinglocal_variable_declaration— a local declared later in the same block is not yet in scope there, and one declared in a different branch of anif/forlives in a sibling block this walk never visits at all, so it can't falsely match either.Confirmed via parse dumps (both engines) that this produces the identical
block/local_variable_declaration/initialized_variable_definitionAST shape for this scenario, so the fix mirrors 1:1 betweenfindDartSelectorReceiver(TS) andhandle_dart_call_expression+ the legacyfind_dart_selector_receiver(Rust, kept mirrored per this file's own convention even though it's currently dead code for the pinned grammar).Two pre-existing tests (
sets_receiver_on_a_local_variable_method_call/sets receiver on a local-variable method call) asserted the old, less-precisethis.-prefixed behavior for a bare local — updated both to the new, correct bare-receiver behavior, since this is a genuine improvement these tests' own comments already flagged as imprecise ("the extractor cannot tell the two apart... prefixing is harmless here").Test plan
dart.rs(local_variable_shadows_fieldmodule) — basic shadow, sibling-method no-shadow, sibling-block no-shadow, nested-block shadow, before-declaration no-shadow, end-to-end type resolutiontests/parsers/dart.test.ts(#2478describe block) — same coverage, WASM enginetests/integration/issue-2478-dart-local-var-shadows-field.test.ts— mirrorsissue-2319-dart-parameter-shadows-field.test.ts's exact pattern (same-named method on both types, confirms the call edge targets the local's type, never the field's)npx tsc --noEmit -p .,npm run lint, fullnpm test(5485 passed)cargo fmt -- --check,cargo clippy --lib -- -D warnings,cargo test --lib(1122 passed)Closes #2478