argotorg / argotorg/solidity

Extend shadowing warning to cover local variables that shadow inherited state variables

Open
#16,698 0 comments 4 reactions 1 assignee Claimed by @nikola-matic View on GitHub
feature low effort low impact should have
Dominant language
C++
Stars
25.7k
Forks
6.2k
Avg merge
2d 19h
Merged PRs (30d)
29

Description

## Motivation

The existing warning already fires for direct shadowing:

```solidity
contract A {
uint256 public x = 1;
function f() public pure returns (uint256) {
uint256 x = 2; // Warning: This declaration shadows an existing declaration.
return x;
}
}
```

However it does not fire when the shadowed variable is inherited, regardless of whether the base is concrete, abstract, or multiple levels up the linearization chain:

```solidity
// SPDX-License-Identifier: MIT
pragma solidity ^0.8.34;

// Case 1: concrete base, no warning
contract ConcreteBase {
uint256 public value = 10;
}
contract ChildConcrete is ConcreteBase {
function f() public pure returns (uint256) {
uint256 value = 99;
return value;
}
}

// Case 2: abstract base, no warning
abstract contract AbstractBase {
uint256 public balance = 100;
}
contract ChildAbstract is AbstractBase {
function f() public pure returns (uint256) {
uint256 balance = 0;
return balance;
}
}

// Case 3: transitive ancestor (2 levels up), no warning
abstract contract A { uint256 public x = 1; }
abstract contract B is A {}
contract C is B {
function f() public pure returns (uint256) {
uint256 x = 99;
return x;
}
}
```

`solc 0.8.35` produces no output for any of the three cases.

A local variable shadowing an inherited state variable is confusing and should trigger a warning as a direct case.

## Specification

Extend the check that produces the "This declaration shadows an existing declaration" warning so that it also fires when a local variable declaration (including named return variables) has the same name as a state variable reachable via the contract's linearized base list.

The warning format should be identical to the existing one, with the Note pointing at the inherited declaration site:

```
Warning: This declaration shadows an existing declaration.
--> Child.sol:8:9:
|
8 | uint256 balance = 0;
| ^^^^^^^^^^^^^^^
Note: The shadowed declaration is here:
--> Base.sol:2:5:
|
2 | uint256 public balance = 100;
| ^^^^^^^^^^^^^^^^^^^^^^
```

## Backwards Compatibility

This change introduces new warnings on previously warning-free code. It is not a breaking change in the sense that existing code continues to compile and behave identically, no semantics are altered. Projects that treat warnings as errors (`--error-codes`) may need to rename shadowing locals, which is the intended outcome.

## Related

- #2563: Shadowing of inherited state variables **between contracts** (state-level redeclaration)
- #973: Original discussion on shadowing warnings.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.