Constant evaluator support for member constants #16056
Conversation
matheusaaguiar
force-pushed
the
constantEvaluatorSupportForMemberAccess
branch
2 times, most recently
from
93d7f5a to
78e1d3e
Compare
matheusaaguiar
force-pushed
the
constantEvaluatorSupportForMemberAccess
branch
from
1e7955b to
985b03f
Compare
test/libsolidity/syntaxTests/constantEvaluator/member_access.sol
Outdated
Show resolved
Hide resolved
matheusaaguiar
force-pushed
the
constantEvaluatorSupportForMemberAccess
branch
from
ad3f23a to
7fa4363
Compare
matheusaaguiar
force-pushed
the
constantEvaluatorSupportForMemberAccess
branch
2 times, most recently
from
e596763 to
d5beb9e
Compare
matheusaaguiar
force-pushed
the
constantEvaluatorSupportForMemberAccess
branch
from
d5beb9e to
9d5f93f
Compare
|
This pull request is stale because it has been open for 14 days with no activity. |
github-actions
bot
added
the
stale
matheusaaguiar
removed
the
stale
|
This pull request is stale because it has been open for 14 days with no activity. |
github-actions
bot
added
the
stale
matheusaaguiar
removed
the
stale
matheusaaguiar
force-pushed
the
constantEvaluatorSupportForMemberAccess
branch
3 times, most recently
from
66a8367 to
a1c8007
Compare
matheusaaguiar
force-pushed
the
constantEvaluatorSupportForMemberAccess
branch
from
a1c8007 to
1145d25
Compare
| if (auto const* parentIdentifier = dynamic_cast<Identifier const*>(&_memberAccess.expression())) | ||
| { | ||
| if (auto const* contract = dynamic_cast<ContractDefinition const*>(parentIdentifier->annotation().referencedDeclaration)) |
There was a problem hiding this comment.
This does not cover all relevant cases. I think it will break at least on these:
m.CONST(expression is a module)(C).CONST(expression is not an identifier)m.C.CONST(member access on a member access)
Especially the first and the third one are not far fetched at all.
These are probably not all cases either. I can also think about a few more that are now disallowed due to our previous changes in implicit conversions:
[C][0].CONST(true ? C : C).CONST
There was a problem hiding this comment.
This should really be possible to implement in a more general way, without handling all those cases one by one. Have you tried getting the constant definition via _memberAccess.annotation().referencedDeclaration?
There was a problem hiding this comment.
The problem is that constant evaluation is done "on demand" whenever it is needed, so in some (most?) cases information like referencedDeclaration may not be present and will only be calculated later.
For example, in the case of array sizes, the expression will be evaluated during the DeclarationTypeChecker step and referencedDeclaration is resolved later when TypeChecker step runs.
On the other hand, in the case of the custom storage layout of the contract, the evaluation would occur during the same step and information like referencedDeclaration would be available.
I ended up focusing only in the simplest use of contract constants, and then later was considering handling modules in a different PR to not hold this one back.
I am not sure about expressions like (true ? C : C).CONST, I think maybe it kinda falls out of scope, because currently the constant evaluator can't handle them, so it would not be able to yield C as the "result" there...
There was a problem hiding this comment.
We need more tests. At least:
- Access to a non-constant member should produce an appropriate error.
- Member access that is not a top-level expression in array size, e.g.
a[1 + C.CONST + 2]. - Nesting: member access to a constant whose value contains member access as well.
- Cycles: Two member access constants using each other as values (should be an error).
Especially the last one looks worrying. I just found #12928 (comment), which may be the reason we had this limitation to begin with. If that's the case, then properly solving this may require deeper changes to analysis phases.
Fix #16055.