Disallow delete on types containing nested mappings. #11843
Conversation
Marenz
force-pushed
the
no-delete-on-mapping-8535
branch
from
11485af
to
8d85d2e
Compare
Marenz
force-pushed
the
no-delete-on-mapping-8535
branch
from
8d85d2e
to
5e44195
Compare
Marenz
force-pushed
the
no-delete-on-mapping-8535
branch
from
5e44195
to
65246b6
Compare
Marenz
force-pushed
the
no-delete-on-mapping-8535
branch
from
65246b6
to
ba54358
Compare
libsolidity/analysis/TypeChecker.cpp
Outdated
| @@ -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()) | |||
There was a problem hiding this comment.
With the delete check, I feel that nameable condition can be removed. But not sure :(
There was a problem hiding this comment.
I removed it and checked isoltest locally. If the CI is happy I'd say it's safe?
There was a problem hiding this comment.
I looked a bit through the code and tested some cases and it seems this is good.
Marenz
force-pushed
the
no-delete-on-mapping-8535
branch
2 times, most recently
from
c9b6fca
to
78fc149
Compare
libsolidity/analysis/TypeChecker.cpp
Outdated
| @@ -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()) | |||
There was a problem hiding this comment.
Is it more convenient to move this check inside Type::unaryOperatorResult? We would only need it on reference types.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
That actually sounds good!
Marenz
force-pushed
the
no-delete-on-mapping-8535
branch
2 times, most recently
from
5f54d59
to
d9b18d8
Compare
Changelog.md
Outdated
| * 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. |
There was a problem hiding this comment.
That's a rebase artifact, isn't it?
There was a problem hiding this comment.
oh. Yes, I must have missed that
Marenz
force-pushed
the
no-delete-on-mapping-8535
branch
from
d9b18d8
to
d248d35
Compare
| * Disallow ``.pop()`` on arrays containing nested mappings. | ||
| * Disallow ``delete`` on types that contain nested mappings. |
There was a problem hiding this comment.
Is this in alphabetic order :-D? I'm honestly not sure :-D. [no need to do anything as far as I'm concerned]
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
Can't you just remove this and replace every t below with result? If not: also fine.
There was a problem hiding this comment.
I remember I actually tried that and that there was a problem with that. Something with TypeResult vs Type..
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
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)
fixes #8535