Enable parser to accept transient as data location or identifier

[matheusaaguiar]

First step on providing Solidity high-level support for transient storage.
closes #15006.

[cameel]

Just realized that it's also missing a changelog entry. Is it because it's considered experimental at this point?

[matheusaaguiar]

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 😅

[cameel]

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...

[matheusaaguiar]

@cameel , I have restricted the parser to only accept transient for state variables now. There were some tests from before which are kinda useless right now, but I guess they will come in handy later when we allow transient for other kinds of variable.

[cameel]

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.

[cameel]

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...

[matheusaaguiar]

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.

[cameel]

Is this really needed? It seems redundant. identifier already includes Transient.

[cameel]

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.

[cameel]

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.

[cameel]

I'd avoid adding tests at the top level. This one would fit e.g. in parsing/.

[cameel]

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).

Suggested change

	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) {}
	}
	// ----

[cameel]

Would be good to cover this too (should fail):

    uint storage transient;

[cameel]

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;