AsmJsonImporter: Replace validation asserts with proper AstImportError reported as JSONError by cavdarahmet · Pull Request #16874 · argotorg/solidity

cavdarahmet · 2026-07-17T17:30:56Z

Description

Reworked per the review feedback below: from a pure test addition into the validation fix it actually needed.

AsmJsonImporter parses untrusted JSON (the inline assembly subtree of ASTs supplied via the experimental SolidityAST standard-json input), but validated it with yulAssert/solAssert. Malformed input therefore surfaced as "type": "Exception" — documented as an internal failure that "should be reported as an issue" — and some unchecked accesses escaped as raw nlohmann type_errors.

Changes:

Introduce langutil::AstImportError and throw it via solRequire()/solThrow() for every malformed-input condition in AsmJsonImporter (all 16 former asserts, including the message-less ones).
Add a requiredString() helper for JSON string fields that were previously read unchecked (name, kind, hexValue, value, nodeType), so missing or mistyped fields produce a proper message instead of a raw type_error.
Catch the new type in StandardCompiler and report it as "type": "JSONError" ("JSON input doesn't conform to the required format"), keeping the established Failed to import AST: message prefix. The CLI path is unaffected — its broad catch already yields the same message.
Replace the Boost test with four standard-json cmdline tests (test/cmdlineTests/standard_import_ast_*), one per validation path, including the createStatement nodeType prefix check linked in the review.

Example — importing an AST whose YulAssignment node has "nodeType": "XulAssignment":

Before: "type": "Exception", message Unknown exception during compilation: ... Failed to import AST: Invalid nodeType prefix
After: "type": "JSONError", message Failed to import AST: Invalid nodeType prefix

Partially addresses #15854 (the AsmJsonImporter half). Out of scope, left for follow-ups: ASTJsonImporter's astAsserts (the other half of #15854, including the related FIXME in CommandLineInterface), and non-structural value errors (invalid hex digits via fromHex, oversized number literals), which still surface as Exception.

Checklist

I have read the contributing guidelines and the review checklist
I have personally reviewed, understood, and tested every change in this PR

AI Disclosure

No AI tools were used

cavdarahmet · 2026-07-21T18:36:18Z

While working on this, I noticed the generic catch (...) fallback in StandardCompiler::compileSolidity() uses boost::current_exception_diagnostic_information(), which leaks internal details (file/line, C++ type names) into the error message shown to users, e.g.:

Before:

Unknown exception during compilation: /solidity/libsolidity/interface/StandardCompiler.cpp(1487): Throw in function ... Dynamic exception type: boost::wrapexcept<solidity::util::Exception> std::exception::what: Failed to import AST: Invalid nodeType prefix

After (using .what() when it's a util::Exception):

Unknown exception during compilation: Failed to import AST: Invalid nodeType prefix

Happy to open a small separate PR for this if useful, once this one is merged.

cameel · 2026-07-24T15:34:47Z

which leaks internal details (file/line, C++ type names) into the error message shown to users, e.g.:

This is intentional and should stay as is. What the handler prints is not a typical error message meant to be shown in the course of normal usage of the compiler. It's more of a crash dump that helps us track down the bug that caused it. If anything, I'd make it more, not less detailed.

cameel · 2026-07-24T15:40:07Z

 	BOOST_CHECK(!containsError(result, "FatalError", ""));
 }

+BOOST_AUTO_TEST_CASE(import_ast_malformed_inline_assembly)


Can these tests be replaced with ASTJSON or astPropertyTests tests? We're trying to reduce the number of Boost-based tests in favor of custom isoltest test cases, which are easier to maintain. I would not add more tests here if we can avoided and even the existing tests should be rewritten with isoltest whenever possible (though this is very low priority).

Though they do not accommodate error cases currently. We should extend them though so that they can show errors.

Done — the Boost test is gone entirely. The scenarios now live under test/cmdlineTests/ as standard JSON tests (standard_import_ast_*). ASTJSON/astPropertyTests couldn't host them: they exercise the export direction and take Solidity sources as input, while these need malformed JSON as the input itself.

cameel · 2026-07-24T15:51:42Z

+	// AsmJsonImporter::createStatement/createExpression: yulAssert(nodeType prefix == "Yul")
+	{
+		Json result = compileWithMutation(
+			baseAstJson,
+			[](Json& _ast)
+			{
+				Json* node = findFirstNodeByType(_ast, "YulIdentifier");
+				BOOST_REQUIRE(node);
+				(*node)["nodeType"] = "XulIdentifier";
+			});
+		BOOST_REQUIRE(result["errors"][0]["message"].is_string());
+		BOOST_CHECK(
+			result["errors"][0]["message"].get<std::string>().find("Failed to import AST: Invalid nodeType prefix")
+			!= std::string::npos);
+		BOOST_CHECK(result["errors"][0]["type"] == "Exception");
+	}


We can't do it like this. This basically amounts to testing a failing assertion, i.e. compiler's undefined behavior resulting from a bug:

solidity/libyul/AsmJsonImporter.cpp

Line 92 in 8a07979

yulAssert(nodeType.substr(0, 3) == "Yul", "Invalid nodeType prefix");

Before this code can be tested, it needs to have proper validations. The fact is that AST import is still an experimental feature and is unfinished. It wrongly uses asserts for things that should be proper validations.

If you want to add a test for this, we need to change that. We need to define a proper exception type to represent these failure conditions (they should not come out as just type: Exception). Then change the exporter to throw that exception (or use solRequire() when the validation is a simple one-liner).

Implemented as suggested: langutil::AstImportError, thrown via solRequire()/solThrow() across AsmJsonImporter, caught by type in StandardCompiler, and reported as JSONError — per the docs that's the type for JSON input not conforming to the required format, while Exception/InternalCompilerError are documented as "report as an issue". Unchecked nlohmann accesses on required string fields are validated the same way now, so they no longer escape as raw type_errors. The new cmdline tests pin the exact check you linked (createStatement's prefix validation). Out of scope here: ASTJsonImporter's astAsserts (the other half of #15854) and non-structural value errors (fromHex digits, oversized number literals).

cavdarahmet · 2026-07-24T17:27:36Z

Thanks for the review and the explanation.

I see the issue now. It makes sense that these conditions should be handled through proper validation rather than testing failing assertions. I'll look into the suggested approaches and rework the implementation accordingly before updating the tests.

The importer runs on untrusted JSON input but used yulAssert/solAssert for input validation. Failures surfaced as `type: "Exception"`, which the documentation defines as an internal bug to be reported. - Introduce langutil::AstImportError and throw it via solRequire()/ solThrow() for all malformed-input conditions in AsmJsonImporter. - Validate string fields accessed through nlohmann (name, kind, hexValue, value) instead of letting raw type_errors escape. - Catch the new type in StandardCompiler and report it as `type: "JSONError"`, keeping the "Failed to import AST: " prefix. - Cover the failure paths with standard-json cmdline tests. Partially addresses argotorg#15854 (the AsmJsonImporter half).

cavdarahmet · 2026-08-06T03:35:23Z

This is intentional and should stay as is. What the handler prints is not a typical error message meant to be shown in the course of normal usage of the compiler. It's more of a crash dump that helps us track down the bug that caused it.

You're right — withdrawing that suggestion. It makes sense as a crash dump for actual bugs. With this rework the malformed-input cases no longer reach that handler at all.

github-actions Bot added the external contribution ⭐ label Jul 17, 2026

nikola-matic self-assigned this Jul 20, 2026

cameel reviewed Jul 24, 2026

View reviewed changes

cameel added the testing 🔨 label Jul 24, 2026

cavdarahmet requested a review from cameel July 24, 2026 17:08

cavdarahmet force-pushed the add-asmjsonimporter-malformed-ast-test branch from b7d7097 to 12dbf44 Compare August 6, 2026 03:24

cavdarahmet changed the title ~~test: add regression test for malformed AST import in AsmJsonImporter~~ AsmJsonImporter: Replace validation asserts with proper AstImportError reported as JSONError Aug 6, 2026

Navigation Menu

Uh oh!

AsmJsonImporter: Replace validation asserts with proper AstImportError reported as JSONError - #16874

AsmJsonImporter: Replace validation asserts with proper AstImportError reported as JSONError#16874
cavdarahmet wants to merge 1 commit into
argotorg:developfrom
cavdarahmet:add-asmjsonimporter-malformed-ast-test

cavdarahmet commented Jul 17, 2026 •

edited

Loading

Uh oh!

cavdarahmet commented Jul 21, 2026

Uh oh!

cameel commented Jul 24, 2026

Uh oh!

cameel Jul 24, 2026

Uh oh!

cameel Jul 24, 2026

Uh oh!

cavdarahmet Aug 6, 2026

Uh oh!

cameel Jul 24, 2026

Uh oh!

cavdarahmet Aug 6, 2026

Uh oh!

cavdarahmet commented Jul 24, 2026

Uh oh!

cavdarahmet commented Aug 6, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

cavdarahmet commented Jul 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checklist

AI Disclosure

Uh oh!

cavdarahmet commented Jul 21, 2026

Uh oh!

cameel commented Jul 24, 2026

Uh oh!

cameel Jul 24, 2026

Choose a reason for hiding this comment

Uh oh!

cameel Jul 24, 2026

Choose a reason for hiding this comment

Uh oh!

cavdarahmet Aug 6, 2026

Choose a reason for hiding this comment

Uh oh!

cameel Jul 24, 2026

Choose a reason for hiding this comment

Uh oh!

cavdarahmet Aug 6, 2026

Choose a reason for hiding this comment

Uh oh!

cavdarahmet commented Jul 24, 2026

Uh oh!

cavdarahmet commented Aug 6, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

cavdarahmet commented Jul 17, 2026 •

edited

Loading