Disallow delete on types containing nested mappings. by Marenz · Pull Request #11843 · ethereum/solidity (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation29 Commits1 Checks0 Files changed
Conversation
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
[ Show hidden characters]({{ revealButtonHref }})
| @@ -1613,6 +1613,13 @@ bool TypeChecker::visit(UnaryOperation const& _operation) |
|---|
| _operation.annotation().isPure = !modifying && *_operation.subExpression().annotation().isPure; |
| _operation.annotation().isLValue = false; |
| if (op == Token::Delete && subExprType->nameable() && subExprType->containsNestedMapping()) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With the delete check, I feel that nameable condition can be removed. But not sure :(
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed it and checked isoltest locally. If the CI is happy I'd say it's safe?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I looked a bit through the code and tested some cases and it seems this is good.
| @@ -1613,6 +1613,13 @@ bool TypeChecker::visit(UnaryOperation const& _operation) |
|---|
| _operation.annotation().isPure = !modifying && *_operation.subExpression().annotation().isPure; |
| _operation.annotation().isLValue = false; |
| if (op == Token::Delete && subExprType->containsNestedMapping()) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it more convenient to move this check inside Type::unaryOperatorResult? We would only need it on reference types.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could could move it, but the error reporter is not available in the type and in this particular visit, the string part of the return value of that function is also not used for the error message
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could change the call here to use the err message returned.. it would change a few.. like this:
syntaxTests/negation.sol: FAIL
Contract:
contract test {
function f() public pure {
int x;
uint y = uint(-x);
-y;
}
}
Expected result:
TypeError 4907: (97-99): Unary operator - cannot be applied to type uint256
Obtained result:
TypeError 4907: (97-99): Unary operator - cannot be applied to type uint256: Unary negation is only allowed for signed integers.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That actually sounds good!
| * Inline Assembly: Consider functions, function parameters and return variables for shadowing checks. |
|---|
| * ``error`` is now a keyword that can only be used for defining errors. |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a rebase artifact, isn't it?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh. Yes, I must have missed that
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks fine to me.
Comment on lines +4 to +5
| * Disallow ``.pop()`` on arrays containing nested mappings. |
|---|
| * Disallow ``delete`` on types that contain nested mappings. |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this in alphabetic order :-D? I'm honestly not sure :-D. [no need to do anything as far as I'm concerned]
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is. . before d, also originally done by vim using :sort (that's why we had that rebase artefact, it reordered another change, too)
| Type const* t = type(_operation.subExpression())->unaryOperatorResult(op); |
|---|
| if (!t) |
| TypeResult result = subExprType->unaryOperatorResult(op); |
| Type const* t = result; |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't you just remove this and replace every t below with result? If not: also fine.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I remember I actually tried that and that there was a problem with that. Something with TypeResult vs Type..
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yes. We're assigning to t in line 1615 and that's why. Assigning to a TypeResult is at the very least confusing here
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my mind assigning to TypeResult is not that big of a deal (although not sure if it just works and jumping through hoops for being able to would definitely be too much). I'm fine with keeping the t.
Marenz added a commit that referenced this pull request
Manual Resolved Conflicts: Changelog.md * Updated changelog test/externalTests/ens.sh * Merged fixes for upstream from both develop and breaking test/libsolidity/semanticTests/inlineAssembly/external_identifier_access_shadowing.sol * Removed in #11735 (breaking) test/libsolidity/semanticTests/inlineAssembly/function_name_clash.sol * Removed in #12209 (breaking) test/libsolidity/semanticTests/storage/mappings_array2d_pop_delete.sol * Removed in #11843 (breaking) test/libsolidity/semanticTests/storage/mappings_array_pop_delete.sol * Removed in #11843 (breaking) test/libsolidity/syntaxTests/inlineAssembly/basefee_berlin_function.sol * Used version of file from #11842 (breaking)