-
Notifications
You must be signed in to change notification settings - Fork 6.3k
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.
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
base: develop
Are you sure you want to change the base?
Fix incorrect shadow warning for variables in non-overlapping scopes #16379
Conversation
|
Thank you for your contribution to the Solidity compiler! A team member will follow up shortly. If you haven't read our contributing guidelines and our review checklist before, please do it now, this makes the reviewing process and accepting your contribution smoother. If you have any questions or need our help, feel free to post them in the PR or talk to us directly on the #solidity-dev channel on Matrix. |
cameel
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the contribution! There are a few things we need here, most notably more complete test coverage of this and similar cases of declaration shadowing that may be affected (or work but could be broken by an incorrect fix).
| { | ||
| solAssert(outerDeclaration, ""); | ||
| if (auto const* varDecl = dynamic_cast<VariableDeclaration const*>(outerDeclaration)) | ||
| if (varDecl->isLocalVariable() && outerDeclaration->location().start >= innerLocation->start) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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/catchvariables - 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.
There was a problem hiding this comment.
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; }There was a problem hiding this comment.
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.
|
Thanks @cameel I'll get to this shortly. |
e56329e to
8c6c483
Compare
4888db0 to
58a89a9
Compare
58a89a9 to
674f171
Compare
|
The core fix and original test update are passing CI. I've had difficulty getting additional tests to pass. The fix specifically targets local variables in non-overlapping scopes - it checks if a homonym (same-name declaration in outer scope) is a local variable declared after the current declaration, and skips the warning in that case. Could you provide guidance on:
Happy to iterate on this. |
Fixes #16190
When a local variable is declared in a block that ends before another local variable with the same name is declared at function scope, the compiler incorrectly warns that the first variable shadows the second.
This PR skips shadow warnings for local variables where the outer declaration appears after the inner declaration in source order, since the outer variable was not visible when the inner one was declared.