-
Notifications
You must be signed in to change notification settings - Fork 5.6k
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
Fix ICE caused by an array of mappings #9702
Conversation
I don't think we should disallow them - sorry :) |
Should we move #7410 back from the implementation backlog then? Or is there a consensus how to proceed? |
One way to proceed is to disallow |
cb49b91
to
d575941
Compare
libsolidity/analysis/TypeChecker.cpp
Outdated
if ( | ||
((funType->kind() == FunctionType::Kind::ArrayPush && arguments.value().numArguments() != 0) || funType->kind() == FunctionType::Kind::ArrayPop) && | ||
exprType->containsNestedMapping() | ||
) |
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.
if (
auto arrayType = dynamic_cast<ArrayType const*>(exprType);
arrayType &&
arrayType->location() == DataLocation::Storage &&
arrayType->containsNestedMapping() &&
((memberName == "push" && arguments.value().numArguments() != 0) || memberName == "pop")
)
Would this be better?
Several tests fail because of |
Disallowing Just out of curiosity: |
d575941
to
e18652f
Compare
e18652f
to
3c876fc
Compare
In the sense that it does not crash the compiler, yes. |
Fixes #7410 by disallowing
.push(<arg>)
for storage arrays with mappings.@chriseth @leonardoalt @hrkrshnn
From the #7410 discussion, I somewhat concluded that the idea is to forbid arrays with (nested) mappings.I added a simple check and updated several test cases (more needs to be updated). Is it the intended direction? The impact (in terms of the number of failing tests) is rather big.Disallows
.push(<arg>)
andfor storage arrays with nested mappings..pop()