Support constants in custom storage layout expression #15944
Conversation
matheusaaguiar
force-pushed
the
allowConstantsCustomStorageLayout
branch
2 times, most recently
from
944dc30 to
059349f
Compare
matheusaaguiar
force-pushed
the
allowConstantsCustomStorageLayout
branch
from
4aae044 to
00ee32e
Compare
| "The base slot of the storage layout must evaluate to an integer." | ||
| ); | ||
| return; | ||
| if (integerType->isSigned()) |
There was a problem hiding this comment.
This could also be removed from here and then replace the assert in line 173 as suggested in a previous PR #15528 (comment)
There was a problem hiding this comment.
Depends. The range check should catch it anyway and it provides a message that will be clearer to the user so I'd rather not check it. Removing this and adding an assert would be a better choice.
But I'm not sure if we will actually reach that check with constants. What happens in this case if you remove the isSigned(). Will it fail in constant evaluator instead?
int constant X = -1;
contract C layout at X {}
|
thank you for supporting constants! this is a huge composability unlock! |
|
Would this also apply to immutables? |
No, this only targets constants. Immutables are a little bit different and demand a different handling. |
|
We can't really support immutables here, because the layout must already be defined at creation time (constructor can initialize state variables), but their values can still change at that point. In case where they do not, the immutable can be replaced by a constant anyway. @matheusaaguiar Speaking of immutables, please make sure you have a test case against #15989. I.e. one showing that immutables are rejected even if they end up being treated as pure by the compiler. |
matheusaaguiar
force-pushed
the
allowConstantsCustomStorageLayout
branch
from
ec9785d to
d1dbd2e
Compare
| { | ||
| m_errorReporter.typeError( | ||
| 6396_error, | ||
| baseSlotExpression.location(), | ||
| "The base slot of the storage layout must evaluate to a rational number." | ||
| "The base slot of the storage layout must evaluate to a rational integer number." |
There was a problem hiding this comment.
| "The base slot of the storage layout must evaluate to a rational integer number." | |
| "The base slot of the storage layout must evaluate to an integer." |
We could also say a rational number or an integer, but not sure if the user will get the distinction. Unfortunately, out terminology here is a bit ambiguous.
| @@ -4,4 +4,4 @@ contract A { | |||
|
|
|||
| contract C is A layout at A.x { } | |||
| // ---- | |||
| // TypeError 6396: (68-71): The base slot of the storage layout must evaluate to a rational number. | |||
| // TypeError 1505: (68-71): The base slot expression cannot be evaluated during compilation. | |||
There was a problem hiding this comment.
This one is weird. Why are we getting this and not something like:
Error: Member "x" not found or not visible after argument-dependent lookup in type(contract A).
Is ConstantEvaluator silencing this error and just returning nullopt?
| import "A" as M; | ||
| contract C layout at M.x{ } | ||
| // ---- | ||
| // TypeError 1505: (B:38-41): The base slot expression cannot be evaluated during compilation. |
There was a problem hiding this comment.
This one should work without errors.
There was a problem hiding this comment.
Same for constant_initialized_from_cast.sol
| address immutable a = 0x0000000000000000000000000000000000000001; | ||
| uint immutable x = 1; |
There was a problem hiding this comment.
Please also add an immutable that's not pure (i.e. one that's not initialized with a literal) since that's a separate case. Also a comment explaining how they differ.
| @@ -1,3 +1,3 @@ | |||
| contract C layout at ~uint(0xFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF) {} | |||
| // ---- | |||
| // TypeError 6396: (21-94): The base slot of the storage layout must evaluate to a rational number. | |||
| // TypeError 1505: (21-94): The base slot expression cannot be evaluated during compilation. | |||
There was a problem hiding this comment.
I think that users will be confused about why we can't evaluate this, because fundamentally it should be possible. It's documented, but maybe it would not hurt to just say that the constant evaluator is just too limited for this?
| // TypeError 1505: (21-94): The base slot expression cannot be evaluated during compilation. | |
| // TypeError 1505: (21-94): The base slot expression contains elements that are not yet supported by the internal constant evaluator and therefore cannot be evaluated at compilation time. |
| contract at layout at type(uint).max { } | ||
| // ---- | ||
| // TypeError 6396: (22-36): The base slot of the storage layout must evaluate to a rational number. | ||
| // TypeError 1505: (22-36): The base slot expression cannot be evaluated during compilation. |
There was a problem hiding this comment.
I was hoping this would work already in this PR, but apparently it's another thing that requires proper compile-time evaluation... #11183 (comment)
part of #15727.