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

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

[cameel]

Just a heads-up that this will have to be moved over to 0.8.27 before we merge since it won't go into 0.8.26.

[matheusaaguiar]

Right. Should I already move it to a 0.8.27 section? I guess I will get a conflict anyway when rebasing after the release.

[cameel]

It's fine to wait. Just leave this thread unresolved so it's clear we still have to do it.

[cameel]

We don't normally test ICEs, one of the reason being that this output is not necessarily portable (as you can see from failing CI).

The other reason is that they should be fixed instead. Though the issue here is of course that UnimplementedFeatureError is not really an ICE and should not be treated as such. We have some limited mechanism for catching it, but for some odd reason it was implemented to only catch ones with source locations attached.

So if you want to test it, I think you'd have to replace the call to solUnimplementedAssert() with manual throwing of this error with a location attached.

So unless you find a way to get this to show up as a proper error, we're probably better of not testing this output until the feature is properly implemented.

[matheusaaguiar]

I decide to just remove it. StorageLayout does not have a errorReporter member, so I think it is simpler to just not test this for now.
However, this made me question about other outputs from codegen which are being generated basically ignoring transient and assuming storage. Should we also throw UnimplementedFeatureError for those cases too?

[matheusaaguiar]

I was thinking that we could also throw a fatalTypeError during type checking, I guess. Or just throw UnimplementedFeatureError in type checking but then most of the tests will have to be removed (and saved for later) and we could add tests using the stopAfter flag set to parsing...

[cameel]

Depends on the output. If there are any others that are wrong with transient storage, they should fail. But I don't think we need to do that with the whole type checking. The variables are getting transient location in the AST so it's clear what's happening.

It's the cases like storage layout that are bad - the compiler was giving outright wrong information - telling you that transient variables live in storage, and using wrong slots for the actual storage variables.

[matheusaaguiar]

I was thinking about codegen output, like, for example, opcodes, asm and bin which will just generate code considering the variables to be located on storage.
Compiling this with solc --opcodes:

contract C {
    uint transient x = 42;
}

generates output which uses SSTORE for the x var:

Opcodes:
PUSH1 0x80 PUSH1 0x40 MSTORE PUSH1 0x2A PUSH0 SSTORE ...

[cameel]

Ah, great point. I somehow assumed that this would already be happening here, but you're totally right, we only added unimplemented asserts for stuff like reference types. Value types are allowed and will proceed to the codegen and just ignore the location. We definitely should block that, because otherwise anyone who mistakenly assumes that transient is fully implemented and starts using it will get a bad surprise. Declaring a variable as transient and getting code using storage variable instead is something we'd normally consider a serious codegen error.

The main complication though is that if we add asserts, syntax tests will start failing because they are hard-coded to run compilation:

solidity/test/libsolidity/SyntaxTest.cpp

Line 75 in 028cf33

runFramework(withPreamble(m_sources.sources), PipelineStage::Compilation);

We'd indeed need support for stopAfter: parsing in syntax tests.

So the choices here are to add this as a setting you can enable in certain syntax tests and then use it in this PR or to just leave this PR unmerged until the codegen part is implemented. Fortunately it did not make it into 0.8.26, but there's still no 100% certainty we'll have the rest ready by 0.8.27 so merging it otherwise is risky.

[matheusaaguiar]

Yes, I was thinking about how this would be confusing for someone using the compiler and assuming that since transient was parsed and code generated, then it is working with transient storage.
I will look into implementing stopAfter: parsing for syntax tests. Seems like should not be complicated or time consuming.

[cameel]

Yeah, should be quite simple. It's just a matter of parsing the new setting in SyntaxTest constructor and later passing the selected target stage to AnalysisFramework in SyntaxTest::parseAndAnalyze().

I'd recommend doing that in a separate PR though.

And I think it'll generally be useful for future cases where we'll want to implement only parsing or only analysis. It would already have been useful for experimental - we had the same problem back then and we solved it by hacking in a condition for experimental, but this would have been a more general solution.