Skip to content

Fix incorrect shadow warning for variables in non-overlapping scopes #16379

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
heathdutton wants to merge 1 commit into
base: develop
Choose a base branch
from
+4 −1
Open

Fix incorrect shadow warning for variables in non-overlapping scopes #16379

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Changelog.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ Language Features:
Compiler Features:

Bugfixes:
* Name Resolver: Fix incorrect shadowing warning for variables declared in non-overlapping scopes.


### 0.8.33 (2025-12-18)
Expand Down
3 changes: 3 additions & 0 deletions libsolidity/analysis/NameAndTypeResolver.cpp
View file
Open in desktop
Copy link
Collaborator

@cameel cameel Jan 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PR needs a changelog entry.

Original file line number Diff line number Diff line change
Expand Up @@ -260,6 +260,9 @@ void NameAndTypeResolver::warnHomonymDeclarations() const
for (Declaration const* outerDeclaration: outerDeclarations)
{
solAssert(outerDeclaration, "");
if (auto const* varDecl = dynamic_cast<VariableDeclaration const*>(outerDeclaration))
if (varDecl->isLocalVariable() && outerDeclaration->location().start >= innerLocation->start)
Copy link
Collaborator

@cameel cameel Jan 19, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it's a good idea for semantics to be dependent on exact locations in the source file. Locations are just debug information and might be unavailable or imprecise. It's easy to mess with them e.g. in the AST import mode. We must have some more direct way to determine the order of declarations without locations.

Though I see that this is not a new thing we already have code that does this, e.g. in NameAndTypeResolver::importInheritedScope() so perhaps we can accept this for the purposes of the fix.

continue;
if (dynamic_cast<MagicVariableDeclaration const*>(outerDeclaration))
magicShadowed = true;
else if (!outerDeclaration->isVisibleInContract())
Expand Down
1 change: 0 additions & 1 deletion ...libsolidity/syntaxTests/scoping/double_variable_declaration_disjoint_scope_activation.sol
View file
Open in desktop
Copy link
Collaborator

@cameel cameel Jan 19, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR needs more tests, both for the cases that should now be fixed and the ones where we want to ensure we still have the warning:

  • Shadowing function and function pointer parameters
  • Shadowing loop variables
  • Shadowing try/catch variables
  • Shadowing variables with a tuple declaration
  • Shadowing named mapping key/value
  • Shadowing file-level constants
  • Shadowing field types in a struct
  • The reverse situation in each case

I posted some of those in #16190 (comment), #16190 (comment) and #16190 (comment).

But please also look at the existing test we have for shadowing and see if there might be some other gaps in our coverage.

Copy link
Collaborator

@cameel cameel Jan 19, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For some of those cases it would also be good to have equivalent semantic tests.

For example in the constant case from #16190 (comment) we would be interested not only whether the analysis produces the warning but also that the code generator follows the same logic and uses the right constant in calculations inside and outside of the contract:

contract C {
    uint256 constant X = 0x1234;

    function getInnerX() internal returns (uint) { return X; }
    function f() public returns (uint, uint) { return (getInnerX(), getOuterX()); }
}

uint256 constant X = 0x5678;

function getOuterX() returns (uint) { return X; }

Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,5 @@ contract test {
}
}
// ----
// Warning 2519: (57-63): This declaration shadows an existing declaration.
// Warning 2072: (57-63): Unused local variable.
// Warning 2072: (75-81): Unused local variable.