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: develop
Are you sure you want to change the base?
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
b8174b5
to
dbdbb33
Compare
f222839
to
aff3909
Compare
test/libsolidity/syntaxTests/constants/constant_transient_state_variable.sol
Show resolved
Hide resolved
aff3909
to
af0af77
Compare
1a02699
to
ccaec3a
Compare
@cameel , I have restricted the parser to only accept |
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.
Changelog.md
Outdated
@@ -5,6 +5,7 @@ Language Features: | |||
|
|||
|
|||
Compiler Features: | |||
* Parser: Accept declaration of state variables with ``transient`` data location. |
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.
That would be a language feature actually.
Also, still, not sure whether we should include it. I guess it's probably ok as long as we make it clear we only support the syntax and not code generation. The feature is available in the parser, just not very useful without codegen...
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.
Yeah, I created this entry just in case we actually decided that is needed. I am keeping it in a separate commit until we merge this.
docs/grammar/SolidityParser.g4
Outdated
)* | ||
name=identifier | ||
(transientName=Transient | name=identifier) |
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.
Is this really needed? It seems redundant. identifier
already includes 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.
I was thinking that it would be needed in the case we had transient
both as a data location and a variable name. But I guess I overlooked the fact that the flag transientLocationSet
already takes care of not consuming it more than once as a data location.
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.
I'd avoid adding tests at the top level. This one would fit e.g. in parsing/
.
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.
Done.
library L { | ||
function f(uint[] transient) private pure returns (uint[] transient) { } | ||
function g(uint[] transient) internal pure returns (uint[] transient) { } | ||
function h(uint[] transient) public pure returns (uint[] transient) { } | ||
function i(uint[] transient) external pure returns (uint[] transient) { } | ||
} | ||
// ---- | ||
// DeclarationError 2333: (67-83): Identifier already declared. | ||
// DeclarationError 2333: (145-161): Identifier already declared. | ||
// DeclarationError 2333: (221-237): Identifier already declared. | ||
// DeclarationError 2333: (299-315): Identifier already declared. |
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.
It would be best if the only invalid thing in the test was the thing you're testing for, i.e. the transient
location. To avoid the DeclarationError
here you could test parameters and return values separately (i.e. have a separate function for each).
library L { | |
function f(uint[] transient) private pure returns (uint[] transient) { } | |
function g(uint[] transient) internal pure returns (uint[] transient) { } | |
function h(uint[] transient) public pure returns (uint[] transient) { } | |
function i(uint[] transient) external pure returns (uint[] transient) { } | |
} | |
// ---- | |
// DeclarationError 2333: (67-83): Identifier already declared. | |
// DeclarationError 2333: (145-161): Identifier already declared. | |
// DeclarationError 2333: (221-237): Identifier already declared. | |
// DeclarationError 2333: (299-315): Identifier already declared. | |
library L { | |
function fin(uint[] transient) private pure {} | |
function gin(uint[] transient) internal pure {} | |
function hin(uint[] transient) public pure {} | |
function iin(uint[] transient) external pure {} | |
function fout() private pure returns (uint[] transient) {} | |
function gout() internal pure returns (uint[] transient) {} | |
function hout() public pure returns (uint[] transient) {} | |
function iout() external pure returns (uint[] 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.
Done.
@@ -0,0 +1,5 @@ | |||
contract C { | |||
uint storage transient x; |
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.
Would be good to cover this too (should fail):
uint storage 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.
And these (all should work):
function () transient f;
function () transient internal fti;
function () internal transient fit;
function () internal transient internal fiti;
function () internal internal transient fiit;
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.
Done.
Co-authored-by: Kamil Śliwak <kamil.sliwak@codepoets.it>
First step on providing Solidity high-level support for transient storage.
closes #15006.