Replace assertions with proper validations in AST import

[cameel]

Abstract

The AST import code in CommandLineInterface is currently wrapped in an exception handler that catches all exceptions and prints an error message. This is most likely because the JSON input does not get validated before it's passed to ASTJsonImporter and the importer just asserts things that it expects not to happen.

These assertions should be replaced with proper validations.

The --import-asm-json option that will be introduced in #11807 will likely suffer from the same problem.

Example

This is how you can trigger a validation error now:

echo "{}" | solc - --import-ast

Failed to import AST: Invalid Format for import-JSON: Must have 'sources'-object

Motivation

The current handler suppresses all the extra debug information we include in internal compiler errors. This is what we want if these errors are actually validation errors but not for normal assertions. It means that our usual assertions result in a message looking like this:

Failed to import AST: 

The message is meant to be followed by error description but our assertions often have no description. This makes debugging harder.

Specification

We should define a new exception type representing a validation error and use it in ASTJsonImporter. Then only catch this exception in CommandLineInterface and let other exceptions through.

Since the exception handler also covers CompilerStack->analyze(), it's likely that the assertions in ASTJsonImporter are not enough to satisfy its assumptions. As a part of this task we need to check what exactly these assumptions are and add additional validations against them in ASTJsonImporter.

Backwards Compatibility

This is just a refactor and should not cause changes visible to the user other than in internal compiler errors - and we do not provide backwards-compatibility guarantees for these.

[ryzheboka]

Hi, I would like to work on this issue. I'm new to contributing, can you please check my understanding of the needed changes? So I would need to add a new Exception to Exceptions.h. Then, I should throw this exception in ASTJsonImporter. ASRJsonImporter::jsonToSourceUnit is then used in CompilerStack::importASTs. This function would pass the Exception to CommandLineInterface. In CommandLineInterface, I need to catch Exceptions of the new type, instead of the type "Exception".

[cameel]

Generally yes, but this is only a part of the task. The other part is to ensure that the current validations are sufficient so that switching to a more specialized exception handler in CommandLineInterface does not result in crashes on invalid input. I suspect that they might not be sufficient and it's probably the reason why the handler is so broad.

Checking that will require some experimentation with invalid input and adding tests covering all cases that should be validated. Looks like we currently have nearly zero coverage for AST import. ast_json_import_wrong_evmVersion is the only test I can find.

As for the exception, looks like we already have one in liblangutil/Exceptions.h - InvalidAstError so you can reuse it. This is actually the exception that is thrown by the astAssert() macro. I originally thought that the importer is mixing actual assertions with ones used for input validation but after looking through the file I see that they're all used for validation. This means that leaving these macro invocations as is would be fine and you just need to rename the macro to something like astCheck() or astValidate() to make it clear that it's not an assertion.

Overall this is not going to be a small task if we want to be sure the change does not break anything. It's not hard but might be a bit tedious to get the full coverage. If you'd rather take something smaller, I'd recommend #9627 or #10308.

[ryzheboka]

Thanks for the explanations! #10308 sounds less extensive and more manageable than #12017. It might end up too overwhelming for me to search for possible other unknown reasons for exceptions in this issue.

[chriseth]

Sounds good!

[github-actions]

This issue has been marked as stale due to inactivity for the last 90 days.
It will be automatically closed in 7 days.

[github-actions]

Hi everyone! This issue has been automatically closed due to inactivity.
If you think this issue is still relevant in the latest Solidity version and you have something to contribute, feel free to reopen.
However, unless the issue is a concrete proposal that can be implemented, we recommend starting a language discussion on the forum instead.