Parser support for explicit storage locations

[matheusaaguiar]

First step for #597/#15727.

[matheusaaguiar]

I guess it should be something more specific, because expression will accept anything. Or is it fine?

[r0qs]

Yes, I also believe that we should have something more specific. If I recall correctly, it must be a constant expression that can be evaluate at compile-time. So maybe we should have a new parse function that is restricted to TokenTraits::isArithmeticOp, Literals and the erc7201 helper only.

[matheusaaguiar]

I am assuming that we will check those kind of things later (constant, compile-time eval, etc) at type checking phase.
I am worried about parsing something weird (like a function declaration or a nested contract declaration), but I think I should look more closely to the behaviour of parseExpression to get a better feeling for that.

[ekpyron]

Parsing arbitrary expressions here and then rejecting complex expressions later (in type checking or even already in syntax checking) seems fine to me.
Might be valuable to quickly confirm that there's no really pathological weird cases that may arise because of it, but I don't anticipate any out of my head.

[nikola-matic]

First step for #597.

Do we have another issue detailing the actual steps? I.e. syntax/parsing (this), type checking/analysis, code gen, docs, etc.?

Otherwise, this particular PR looks good, you seem to have covered all of the edge cases I could think of.

[matheusaaguiar]

Do we have another issue detailing the actual steps? I.e. syntax/parsing (this), type checking/analysis, code gen, docs, etc.?

I just created #15727 in order to keep track of the tasks.

[cameel]

I'm done with the review. The biggest problem I found is that the parser will set the location wrong for multi-token expressions.

Other than that only minor stuff: missing sanity checks, naming (especially in AST export), smart vs naked ptrs, etc.

And of course could use a few more tests. Especially one showing the AST structure.

[cameel]

The location of the layout specifier produced by the parser still seems a bit off (it now overlaps the inheritance specifier).

Also some test corrections are still needed (we don't want to test ICE output; unimplemented feature errors need coverage too; use ASTJSON tests).

[cameel]

Ouch, so we're not even handling those AST asserts in Standard JSON mode? Let's replace it with a test that uses the CLI interface, the ICE output is not meant to be tested. It's not portable between platforms. This is why t_osx_cli is failing.

Also please mention in #15854 that currently only the CommandLineInterface catches the exceptions in any way:

solidity/solc/CommandLineInterface.cpp

Lines 981 to 996 in 868154c

	try
	{
	m_compiler->importASTs(parseAstFromInput());
	if (!m_compiler->analyze())
	{
	formatter.printErrorInformation(m_compiler->errors());
	astAssert(false, "Analysis of the AST failed");
	}
	}
	catch (Exception const& _exc)
	{
	// FIXME: AST import is missing proper validations. This hack catches failing
	// assertions and presents them as if they were compiler errors.
	solThrow(CommandLineExecutionError, "Failed to import AST: "s + _exc.what());
	}

StandardCompiler does not, which makes it non-testable.

[matheusaaguiar]

Done.

[cameel]

Please remember to add it to the issue though.

[matheusaaguiar]

Added already.

[matheusaaguiar]

Unrelated to the PR, but it was inconsistent with the compiler parser.
I added test/libsolidity/syntaxTests/functionCalls/empty_call_options.sol which would be accepted by the grammar before this change.

[matheusaaguiar]

@cameel , I think I addressed all of your comments.

[cameel]

Looks fine in terms of functionality so approving. You may want to clean up the things pointed out by @nikola-matic though.

Also, would be nice to replace the Standard JSON tests with ones using CLI. The current ones work, but are much more verbose than they really need to be. Especially when testing secondary source locations, you really care only about the error and not all the noisy JSON details.

[nikola-matic]

I'd say merging this (after squashing) would be best; if there are any small issues, we'll likely notice them in the analysis and codegen PRs, so no need to delay this one.