Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Enable parser to accept transient as data location or identifier #15001

Open
wants to merge 18 commits into
base: catchSolUnimplementedFeatureErrors
Choose a base branch
from
Open

Enable parser to accept transient as data location or identifier #15001

wants to merge 18 commits into from

Conversation

matheusaaguiar
Copy link
Collaborator

@matheusaaguiar matheusaaguiar commented Apr 8, 2024 •
edited

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

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

@matheusaaguiar matheusaaguiar self-assigned this Apr 8, 2024
@matheusaaguiar matheusaaguiar marked this pull request as draft April 8, 2024 13:35
@matheusaaguiar matheusaaguiar force-pushed the transientStorageParserHack branch 3 times, most recently from 0293833 to 5edd9de Compare April 23, 2024 17:22
@matheusaaguiar matheusaaguiar marked this pull request as ready for review April 24, 2024 14:44
@cameel
Copy link
Member

cameel commented Apr 26, 2024

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

@matheusaaguiar
Copy link
Collaborator Author

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
Copy link
Member

cameel commented Apr 26, 2024 •
edited

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 matheusaaguiar force-pushed the transientStorageParserHack branch 4 times, most recently from 29e04e2 to 9064a16 Compare May 1, 2024 01:18
Base automatically changed from addStopAfterSettingToSyntaxTests to develop May 20, 2024 07:34
@cameel cameel mentioned this pull request May 23, 2024
Open
@matheusaaguiar matheusaaguiar mentioned this pull request May 31, 2024
Open
@matheusaaguiar matheusaaguiar added the has dependencies The PR depends on other PRs that must be merged first label May 31, 2024
@matheusaaguiar matheusaaguiar changed the base branch from develop to catchSolUnimplementedFeatureErrors May 31, 2024 15:49
@matheusaaguiar matheusaaguiar force-pushed the catchSolUnimplementedFeatureErrors branch from 345b55e to 2e99c16 Compare June 4, 2024 17:39
cameel
cameel reviewed Jun 7, 2024
View reviewed changes
libsolidity/formal/ModelChecker.cpp
struct TransientDataLocationChecker: ASTConstVisitor
{
TransientDataLocationChecker(SourceUnit const& _sourceUnit) { _sourceUnit.accept(*this); }
void endVisit(VariableDeclaration const& _var) { solUnimplementedAssert(_var.referenceLocation() != VariableDeclaration::Location::Transient); }
Copy link
Member

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.

libsolidity/formal/ModelChecker.cpp
Comment on lines +65 to +67
if (m_settings.engine.any())
{
struct TransientDataLocationChecker: ASTConstVisitor
Copy link
Member

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.

libsolidity/interface/CompilerStack.cpp
Comment on lines +1500 to +1504
solUnimplementedAssert(ranges::all_of(
_contract.stateVariables(),
[](auto const* _stateVar) { return _stateVar->referenceLocation() != VariableDeclaration::Location::Transient; }
));

Copy link
Member

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.

Copy link
Member

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.

test/cmdlineTests/storage_layout_transient_value_types/output.json
"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.",
Copy link
Member

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.

test/cmdlineTests/storage_layout_transient_value_types/input.json
Comment on lines +9 to +10
"A": ["storageLayout"],
"": ["storageLayout"]
Copy link
Member

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.

@matheusaaguiar matheusaaguiar force-pushed the catchSolUnimplementedFeatureErrors branch from 559e027 to ac7c9e2 Compare June 11, 2024 01:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cameel cameel cameel left review comments

@ekpyron ekpyron ekpyron left review comments

@nikola-matic nikola-matic nikola-matic left review comments

@r0qs r0qs Awaiting requested review from r0qs

@aarlt aarlt Awaiting requested review from aarlt

Assignees

@matheusaaguiar matheusaaguiar

Labels
has dependencies The PR depends on other PRs that must be merged first
Projects
Solidity Focus Board
Status: In Progress
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Transient storage parser support: accept transient as a data location or as an identifier
4 participants
@matheusaaguiar @cameel @ekpyron @nikola-matic