Only warn about variables being shadowed in inline assembly. #10971
Conversation
| { | ||
| solAssert(!!declaration, ""); | ||
| if (dynamic_cast<MagicVariableDeclaration const* const>(declaration)) | ||
| // Don't warn the user for what the user did not. | ||
| continue; |
There was a problem hiding this comment.
Actually I was about to push a PR which adds distinct errors for trying to access variables visible in Solidity but not in inline assembly. Well, only for the MagicVariableDeclaration, but was wondering if it should be extended.
This PR goes strictly against that notion so perhaps it would make sense briefly discussing this import resolution bit, it has so many (seemingly random) rules.
There was a problem hiding this comment.
Never mind! I thought this is the TypeChecker, that is what I am talking about :)
| @@ -208,18 +208,18 @@ void NameAndTypeResolver::warnVariablesNamedLikeInstructions() const | |||
| for (auto const& instruction: evmasm::c_instructions) | |||
| { | |||
| string const instructionName{boost::algorithm::to_lower_copy(instruction.first)}; | |||
There was a problem hiding this comment.
Should this actually just use Dialect::reservedIdentifier?
| ); | ||
| // Only warn about variables, everything else is not reachable from | ||
| // inline assembly. | ||
| if (auto const* varDecl = dynamic_cast<VariableDeclaration const*>(declaration)) |
There was a problem hiding this comment.
Ideally we would have the resolution part as a helper, given there are so many weird rules for variables (see TypeChecker::visit(InlineAssembly))
There was a problem hiding this comment.
I would specifically not add the resolution rules here - we should warn about more than just exactly those variables that are reachable.
On the other hand: This check was introduced in a version of Solidity when yul was not yet "strict". Now, it is not possible to mistakenly use a variable in function-context and vice-versa. So maybe we could remove the check altogether?
There was a problem hiding this comment.
I think the reservedIdentifiers supposed to solve that problem in the dialect, so perhaps we do not need this check here?
There was a problem hiding this comment.
function g() public pure {
uint mload;
assembly {
}
}
Ah okay so it is for the above case.
I am not sure we have a good solution for this. This may be one of those cases where #2691 would qualify 😅
| m_errorReporter.warning( | ||
| 8261_error, | ||
| declaration->location(), | ||
| "Variable is shadowed in inline assembly by an instruction of the same name" |
There was a problem hiding this comment.
Just noticed that this warning is missing a fullstop. Will need to update the one test that gets triggered syntaxTests/inlineAssembly/shadowing/variable_by_opcode.sol
| "Variable is shadowed in inline assembly by an instruction of the same name" | |
| "Variable is shadowed in inline assembly by an instruction of the same name." |
chriseth
force-pushed
the
onlyWarnAboutVariables
branch
from
23cb87e to
142b433
Compare
test/libsolidity/syntaxTests/inlineAssembly/shadowing/variable_by_opcode.sol
Show resolved
Hide resolved
chriseth
force-pushed
the
onlyWarnAboutVariables
branch
from
142b433 to
759a432
Compare
Changelog.md
Outdated
| @@ -6,6 +6,7 @@ Language Features: | |||
| Compiler Features: | |||
| * AST: Export NatSpec comments above each statement as their documentation. | |||
| * Optimizer: Simple inlining when jumping to small blocks that jump again after a few side-effect free opcodes. | |||
| * Inline Assembly: Do not warn about shadowing by instructions if the shadowed declaration is not a variable. | |||
There was a problem hiding this comment.
Not alphabetically sorted
There was a problem hiding this comment.
Inline Assembly: Do not warn about shadowing by instructions if the shadowed declaration is not a variable.
So, iiuc, we do not warn anymore if shadowed declaration is not a variable e.g., a function? Does this mean we retain the warning for shadowed variables in inline assembly? Do we have tests for variables in inline assembly that shadow variables and/or other declaration types e.g., structs, functions etc?
There was a problem hiding this comment.
I changed it in the meantime, we removed the warning completely.
| } | ||
| } | ||
| } | ||
| // ---- |
There was a problem hiding this comment.
Could you please add a semantic test for the following if something like this does not already exist?
contract C {
uint x;
function add(uint, uint) public pure returns (uint) { return 0; }
function g() public returns (uint) {
x = add(1, 2);
assembly {
x := add(x, add(1, 2))
}
}
}
just to ensure that the intended add is called in each context.
| } | ||
| } | ||
| } | ||
| // ---- |
There was a problem hiding this comment.
Was this test producing a warning before because nameFromCurrentScope returned the user defined function declaration named mload?
There was a problem hiding this comment.
It was producing a warning that mload and the mload instruction are in conflict.
chriseth
force-pushed
the
onlyWarnAboutVariables
branch
from
759a432 to
150fa92
Compare
|
Ah, tests are failing. |
chriseth
force-pushed
the
onlyWarnAboutVariables
branch
from
150fa92 to
78a097a
Compare
In my opinion it is not too useful to warn about shadowing of declarations that cannot be referenced by inline assembly anyway.
Brought up in the forum by Amxx: https://forum.soliditylang.org/t/which-compiler-warnings-do-you-ignore/89/2?u=chriseth