Enable parser to accept transient as data location or identifier

[matheusaaguiar]

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

depends on #15121 (Merged).
depends on #15168.

[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]

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.

[matheusaaguiar]

This is addressed by de95825

[cameel]

You should add my example as a SMTChecker test so that we can see that it works.

[cameel]

Since unimplemented errors are not ICEs, we should provide a clear message for the user in each one.

[cameel]

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.

[cameel]

The solUnimplementedAssert()s used in CompilerStack are missing error messages as well.

[cameel]

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.

[cameel]

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.

[cameel]

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.