fix: allow keccak256() and uint() in compile-time context for layout base expressions#16439
fix: allow keccak256() and uint() in compile-time context for layout base expressions#16439JPier34 wants to merge 5 commits intoargotorg:developfrom
Conversation
|
Thank you for your contribution to the Solidity compiler! A team member will follow up shortly. If you haven't read our contributing guidelines and our review checklist before, please do it now, this makes the reviewing process and accepting your contribution smoother. If you have any questions or need our help, feel free to post them in the PR or talk to us directly on the #solidity-dev channel on Matrix. |
There was a problem hiding this comment.
Pull request overview
This PR implements compile-time evaluation of keccak256() and explicit uint() type conversions, enabling storage layout base expressions like contract C layout at uint(keccak256("my.contract.id")) {}. This addresses issue #16421 and implements Stage 1 of issue #16420.
Changes:
- Adds compile-time evaluation support for
uint256type conversions from rational values - Adds compile-time evaluation support for
keccak256()with string and hex string literals - Updates test expectations to reflect the new compile-time capabilities
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| libsolidity/analysis/ConstantEvaluator.h | Adds endVisit method declaration for FunctionCall nodes |
| libsolidity/analysis/ConstantEvaluator.cpp | Implements compile-time evaluation logic for uint() conversions and keccak256() calls, with proper range checking and error handling |
| test/libsolidity/syntaxTests/storageLayoutSpecifier/literal_cast.sol | Removes error expectation since uint(42) is now supported in compile-time context |
| test/libsolidity/syntaxTests/storageLayoutSpecifier/layout_keccak256_string_literal.sol | Adds new test case demonstrating the main use case: uint(keccak256("...")) in layout expressions |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
test/libsolidity/syntaxTests/storageLayoutSpecifier/layout_keccak256_string_literal.sol
Show resolved
Hide resolved
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
matheusaaguiar
left a comment
There was a problem hiding this comment.
Hi @JPier34, Thank you for the initiative and the work you’ve put into this.
While I think it’s a solid starting point and it is in the right direction, I feel that the implementation currently aligns more with a proof of concept/prototype.
It is missing several tests and I encountered some issues like the conversion as the length of an array. I still need to check to make sure if that is a problem with the implementation or something to do with the order the AST nodes are type checked.
Additionally, I started wondering about support for byte conversions/string constants for keccak. Constant strings can now be evaluated as part of the introduction of builtin erc7201 in the last pre-release. This requires some design decisions by the team.
Considering that, we have decided that it’s best for us to complete this work internally, so I will be closing this PR.
We truly appreciate your effort and willingness to contribute, and we hope you’ll consider collaborating again in the future.
You can participate in the design discussions and get more involved (and hopefully more straightforward first tasks/issues) by attending our Solidity Language Design Calls. Check the agenda for day/time and topics.
| { | ||
| // Stage 1 (#16420): explicit uint() in comptime (enables layout at uint(keccak256(...)) #16421). | ||
| if (_functionCall.annotation().kind.set() && *_functionCall.annotation().kind == FunctionCallKind::TypeConversion && | ||
| _functionCall.arguments().size() == 1) |
There was a problem hiding this comment.
I think that conversions always have exactly one argument, so this would be better as an assertion inside the if block.
| if (_functionCall.annotation().kind.set() && *_functionCall.annotation().kind == FunctionCallKind::TypeConversion && | ||
| _functionCall.arguments().size() == 1) | ||
| { | ||
| Type const* resultType = _functionCall.annotation().type; |
There was a problem hiding this comment.
Not necessarily wrong, but I am wondering whether functionType::returnParametersTypes() is more precise.
| ); | ||
| return; | ||
| } | ||
| bigint intValue = value.numerator() / value.denominator(); |
There was a problem hiding this comment.
| bigint intValue = value.numerator() / value.denominator(); | |
| bigint intValue = value.numerator(); |
|
|
||
| // #16421: keccak256() in comptime (other common layout base beside erc7201 from PR 15968). | ||
| auto const* builtin = dynamic_cast<MagicVariableDeclaration const*>(ASTNode::referencedDeclaration(_functionCall.expression())); | ||
| if (!builtin || _functionCall.arguments().size() != 1) |
There was a problem hiding this comment.
| if (!builtin || _functionCall.arguments().size() != 1) | |
| if (!builtin) |
Surely keccak256 gets exactly one argument, but I guess that at least for readability/clarity of purpose, best to check the kind of the function as done in the next lines.
| if (!builtin || _functionCall.arguments().size() != 1) | ||
| return; | ||
| auto const* functionType = builtin->functionType(true); | ||
| if (!functionType || functionType->kind() != FunctionType::Kind::KECCAK256) |
There was a problem hiding this comment.
| if (!functionType || functionType->kind() != FunctionType::Kind::KECCAK256) | |
| if (functionType->kind() != FunctionType::Kind::KECCAK256) |
Should probably just assert functionType before.
|
|
||
| solAssert(functionType->returnParameterTypes().size() == 1, ""); | ||
| Type const* bytes32Type = functionType->returnParameterTypes().front(); | ||
| u256 const val = u256(util::h256::Arith(hash)); |
There was a problem hiding this comment.
I think it would be better to keep the value as our internal h256.
There was a problem hiding this comment.
Also this would probably require extending further TypedRational (which already was refactored into TypedValue in the last version).
test/libsolidity/syntaxTests/storageLayoutSpecifier/layout_keccak256_string_literal.sol
Show resolved
Hide resolved
|
I see. Thank you for commenting and appreciating my effort! Look forward to cooperate with you again in the future! |
Summary
Implements evaluation of
keccak256()and explicituint()conversion in a compile-time context. This allows usingkeccak256()in storage layout base expressions:Fixes #16421. Implements Stage 1 of #16420.
Motivation
Layout base expressions commonly use either ERC-7201 (handled in PR #15968) or plain hashing with
keccak256(). This change supports the latter. Sincekeccak256()returnsbytes32while the layout base must be an integer, we also evaluate explicituint()conversions at compile time so thatuint(keccak256("..."))is valid.Changes
ConstantEvaluator.cpp/h
uint(...)type conversion at compile time when the target type isuint256.keccak256(...)at compile time when the argument is a string or hex literal.Tests
literal_cast.solnow expects success foruint(42).layout_keccak256_string_literal.solforlayout at uint(keccak256("...")).Backwards compatibility
Fully backwards-compatible; no change to runtime behaviour.