-
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
Enable parser to accept transient
as data location or identifier
#15001
base: catchSolUnimplementedFeatureErrors
Are you sure you want to change the base?
Enable parser to accept transient
as data location or identifier
#15001
Conversation
7bea970
to
c0ec00e
Compare
...ns/externalFunction/function_argument_location_specifier_test_external_transient_storage.sol
Outdated
Show resolved
Hide resolved
0293833
to
5edd9de
Compare
6e15e6c
to
b0e31af
Compare
test/libsolidity/syntaxTests/immutable/state_var_transient_data_location.sol
Outdated
Show resolved
Hide resolved
test/libsolidity/syntaxTests/dataLocations/transient_function_type.sol
Outdated
Show resolved
Hide resolved
test/libsolidity/syntaxTests/constants/constant_transient_state_variable.sol
Outdated
Show resolved
Hide resolved
Just realized that it's also missing a changelog entry. Is it because it's considered experimental at this point? |
I guess I always forget about the changelog entry 😅 |
Well, it's actually a good question whether it should have an entry. The feature is not really usable to users in the current state so not much point advertising it. On the other hand we do include an entry when an experimental feature is introduced (just not when we change it), so maybe we should still have it. Also, it's not so much experimental as just incomplete... |
29e04e2
to
9064a16
Compare
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.
I did a final pass through the PR and found only one more serious thing - we should not be generating storage layouts that include transient variables. These layouts are not correct and should result in an unimplemented error instead.
Other than that, there's a bunch of small corrections that should all be straightforward to apply.
test/cmdlineTests/storage_layout_transient_value_types/input.json
Outdated
Show resolved
Hide resolved
"language": "Solidity", | ||
"sources": { | ||
"fileA": { | ||
"content": "//SPDX-License-Identifier: GPL-3.0\npragma solidity >=0.0;\ncontract A { uint transient x; bytes32 transient b; address transient addr; }" |
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.
Can you add some non-transient variables here? Best if they're interleaved with the transient ones. Also, you should put the code in a standalone .sol file so that it can be normally formatted.
And yeah, looks like we have a problem here because the returned storage layout will probably be wrong in presence of transient variables. It looks like they'll be assigned slots like normal storage variables. It's not the end of the world, since no one will be able to actually generate bytecode to go with such a layout, but I still think it's wrong to output something like that without producing an error. I think we should use solUnimplementedAssert()
when generating the stack layout and only allow if if there are no transient variables.
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.
Oh, also, I think we should check what the SMTChecker will do in presence of transient
storage variables, because I expect it will not simply fail, but just confidently tell you something that's wrong and this is dangerous.
Take this contract for example:
contract C {
uint /* transient */ x = 42;
function test() public view { assert(x == 42); }
}
solc test.sol --model-checker-engine all
Info: CHC: 1 verification condition(s) proved safe! Enable the model checker option "show proved safe" to see all of them.
If you get the same answer after uncommenting transient
, we need to stick an unimplemented assert somewhere in it to make sure that it will just fail when you try to use it with transient variables.
On the other hand if you get an ICE, it's fine to leave it as is.
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.
This is addressed by de95825
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.
You should add my example as a SMTChecker test so that we can see that it works.
...lidity/syntaxTests/dataLocations/libraries/library_function_with_data_location_transient.sol
Outdated
Show resolved
Hide resolved
test/libsolidity/syntaxTests/dataLocations/state_variable_storage_transient.sol
Show resolved
Hide resolved
test/cmdlineTests/storage_layout_transient_value_types/output.json
Outdated
Show resolved
Hide resolved
f7a7cf4
to
2892ee4
Compare
2892ee4
to
511b702
Compare
511b702
to
41abbe5
Compare
41abbe5
to
ba5dbc9
Compare
345b55e
to
2e99c16
Compare
6b6de54
to
9b325bb
Compare
struct TransientDataLocationChecker: ASTConstVisitor | ||
{ | ||
TransientDataLocationChecker(SourceUnit const& _sourceUnit) { _sourceUnit.accept(*this); } | ||
void endVisit(VariableDeclaration const& _var) { solUnimplementedAssert(_var.referenceLocation() != VariableDeclaration::Location::Transient); } |
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.
Since unimplemented errors are not ICEs, we should provide a clear message for the user in each one.
if (m_settings.engine.any()) | ||
{ | ||
struct TransientDataLocationChecker: ASTConstVisitor |
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.
I think that this check would fit better in analyze()
(somewhere after the if (m_settings.engine.none())
check there).
checkRequestedSourcesAndContracts()
looks more like it was meant for validating settings, not sources. It does look at the AST but only to validate the contract names supplied in settings. Rejecting unsupported transient variables on the other hand feels more like it should be a part of the analysis process.
If you do it here, you're always checking it for all the sources. So you're making it impossible for the user to still use SMTChecker in presence of transient
by just telling it to run on contracts that do not contain it. In fact, maybe it would even be better to move this check as far as CHC::visit(ContractDefinition)
and BMC::visit(ContractDefinition)
because otherwise we're still ignoring the contracts
setting.
solUnimplementedAssert(ranges::all_of( | ||
_contract.stateVariables(), | ||
[](auto const* _stateVar) { return _stateVar->referenceLocation() != VariableDeclaration::Location::Transient; } | ||
)); | ||
|
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.
The solUnimplementedAssert()
s used in CompilerStack
are missing error messages as well.
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.
Also, they'd be better off placed simply in each codegen, in the visit(VariableDeclaration)
that would generate code for the variable since that's the problematic spot. Placing them in CompilerStack
should be is equivalent (I think), but gives me a bit less certainty that all paths that may lead to the codegen are really blocked.
"formattedMessage": "UnimplementedFeatureError: Transient storage layout is not supported yet. | ||
|
||
", | ||
"message": "Unimplemented feature (/solidity/libsolidity/interface/StorageLayout.cpp:49):Transient storage layout is not supported yet.", |
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.
Like I mentioned in #15168 (comment), your exception handler prints information about the C++ source locations. This is diagnostic info that should not ever be included in non-ICE messages that we test.
"A": ["storageLayout"], | ||
"": ["storageLayout"] |
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.
We don't have a file-level storageLayout
output, do we? Note that we don't validate Standard JSON outputs (#12153) so you won't get an error for outputs that do not exist.
44750e3
to
a68961d
Compare
This reverts commit f5024f9.
f5076cc
to
453c309
Compare
First step on providing Solidity high-level support for transient storage.
closes #15006.
depends on #15121 (Merged).
depends on #15168.