Do not remove custom errors in IR codegen when strip revert strings is requested by matheusaaguiar · Pull Request #16466 · argotorg/solidity

matheusaaguiar · 2026-02-11T04:19:38Z

cameel · 2026-02-11T20:45:05Z

We need to also adjust documentation as a part of this issue.

And consider changing the option or value names to make it more intuitive.

nikola-matic

The asm output test is definitely necessary, but please add more semantic test coverage; there's the // revertStrings: strip option for this.

nikola-matic · 2026-02-11T20:27:32Z

test/cmdlineTests/revert_strings_strip_legacy/input.sol

+pragma solidity >=0.0;
+// SPDX-License-Identifier: GPL-3.0
+contract C {
+    error MyError(string errorMsg);


I'd give this an argument or two more, e.g. error MyError(string errorMsg, uint256 errorId);

nikola-matic · 2026-02-11T20:43:35Z

test/cmdlineTests/revert_strings_strip_legacy/input.sol

+contract C {
+    error MyError(string errorMsg);
+    function f() pure external {
+        require(false, MyError("error"));


For the callsite, declare variables, assign them values, and pass them as arguments to the error constructor call.

string errorMsg = "error"; uint256 errorId = 111; require(false, MyError(errorMsg, errorId));

I wanna see some pushes to the stack.

nikola-matic · 2026-02-11T20:47:28Z

libsolidity/codegen/ExpressionCompiler.cpp

-					// Here, the argument is consumed, but in the other branch, it is still there.
-					m_context.adjustStackOffset(static_cast<int>(sizeOfErrorArguments));
+					if (m_context.revertStrings() == RevertStrings::Strip)
+						m_context.appendRevert();


My suspicion is that you'll leave the stack dirty here (i.e. when we iterate and handle the constructor call arguments (loop at 1279), we'll also push any non literal arguments to the stack, which will not be popped anywhere.

Take a look at the revert with error(string) implementation.

rodiazet

LGTM in general. I found a couple of small issues mainly with test coverage. There is also one question I have regarding the way how the old version worked.

rodiazet · 2026-03-16T16:08:03Z

test/libsolidity/semanticTests/errors/require_custom_error_revert_strings_strip.sol

Add:

pragma solidity >=0.0; // SPDX-License-Identifier: GPL-3.0

It does not hurt, but usually we don't add that to tests... not sure if there is a real reason, but I guess we don't care.

Ah, I realize why you suggested that. I copied these tests from the cli tests and there we actually care about that to silence warnings in the output file.
For syntax and semantic tests, that's not the case, i.e, they are filtered out.

rodiazet · 2026-03-16T16:12:03Z

libsolidity/codegen/ir/IRGeneratorForStatements.cpp

 		}
 		else
 		{
+			if (m_context.revertStrings() == RevertStrings::Strip)


I would add here a comment explaining that revert string behaves differently for custom error and for a raw message.

libsolidity/codegen/ir/IRGeneratorForStatements.cpp

Changelog.md

rodiazet · 2026-03-16T16:27:09Z

test/libsolidity/semanticTests/errors/require_custom_error_revert_strings_strip.sol

+    function f() pure external {
+        uint code = 8;
+        string memory eMsg = "error";
+        require(false, MyError(code, eMsg, flag()));


How about tests where require's first arguments cannot be evaluated at compile time? Do you think it makes sense to add them?

Hum, I don't think that matters or affects the codegen but I will double-check.
Can at least add that in some tests to cover for it, but I don't think a specific test is necessary.

rodiazet · 2026-03-16T16:29:25Z

...y/semanticTests/errors/require_custom_error_constructor_side_effect_revert_strings_strip.sol

+// revertStrings: strip
+// ----
+// f() -> FAILURE, hex"0b9344e2", hex"0000000000000000000000000000000000000000000000000000000000000001", hex"0000000000000000000000000000000000000000000000000000000000000040", hex"0000000000000000000000000000000000000000000000000000000000000005", hex"6572726f72000000000000000000000000000000000000000000000000000000"
+// counter() -> 0


Here and in other tests I would add an explanation that the counter returns 0 because the transaction failed and the most important check is that the error argument is properly evaluated to 1.

Added the explanation.

rodiazet · 2026-03-17T11:38:28Z

libsolidity/codegen/ir/IRGeneratorForStatements.cpp

+		Type const* messageArgumentType = arguments.size() > 1 ? arguments[1]->annotation().type : nullptr;

 		auto const* magicType = dynamic_cast<MagicType const*>(messageArgumentType);
 		if (magicType && magicType->kind() == MagicType::Kind::Error)


In the new version this fragment of the code also generates code for path when the require conditions evaluates to true, but I cannot find tests for this case except require_error_stack_check.sol. Not sure if it tests revertStrings = Strip. IMO we should also add test which does not revert and check that the stack is properly cleaned and not revert is called.
BTW in previous version with Strip, custom Error was handled by the code in the else branch. Was it correct?

In the new version this fragment of the code also generates code for path when the require conditions evaluates to true, but I cannot find tests for this case except require_error_stack_check.sol. Not sure if it tests revertStrings = Strip.

The new version now follows the same path of previous version when revertStrings != Strip, so it should have the same result. I will add a variation of that test with revert-strings=string anyway.

BTW in previous version with Strip, custom Error was handled by the code in the else branch. Was it correct?

The handling was the same of a require with no message, i.e., YulUtilFunctions::requireOrAssertFunction will check if the message type is null and then generate code with revert(0, 0) for the false branch of the require condition.
The arguments of require are already visited and their code generated by this point (side-effects guaranteed to happen).
So I think it was correct.

rodiazet · 2026-03-17T11:40:55Z

libsolidity/codegen/ir/IRGeneratorForStatements.cpp

 		{
 			auto const& errorConstructorCall = dynamic_cast<FunctionCall const&>(*arguments[1]);
 			appendCode() << m_utils.requireWithErrorFunction(errorConstructorCall) << "(" <<IRVariable(*arguments[0]).name();
 			for (auto argument: errorConstructorCall.arguments())


Does it make sense to add test case where there is zero custom error arguments to make sure that this corner case (zero for loop iterations) also works properly?
error MyError(); require(false, MyError()) and probably error MyError(); require(true, MyError())

rodiazet · 2026-03-17T11:47:20Z

libsolidity/codegen/ir/IRGeneratorForStatements.cpp

Would also add a test case, in the same contract, with stripped plain string message and un-stripped custom error message,

require_error_evaluation_order_2.sol tests proper order of require arguments evaluation. Does it also test this in case of revertStrings: Strip? Probably not.

Similar require_inherited_error.sol also is tested only for non-strip version.

Same for require_error_condition_evaluated_only_once.sol it's only tested for non-strip version, and it should be tested because it old code had different control flow.

…is requested

argotorg deleted a comment from stackenbotten3000 Feb 11, 2026

matheusaaguiar changed the title ~~Make legacy codege remove custom errors when compiler option revert-strings strip is requested~~ Make legacy codegen remove custom errors when compiler option revert-strings strip is requested Feb 11, 2026

matheusaaguiar changed the title ~~Make legacy codegen remove custom errors when compiler option revert-strings strip is requested~~ Make legacy codegen remove custom errors when compiler option revert-strings=strip is requested Feb 11, 2026

nikola-matic reviewed Feb 11, 2026

View reviewed changes

matheusaaguiar force-pushed the fixRevertStringFlagInconsistentHandling branch 3 times, most recently from 4913b7d to 8450ab7 Compare February 26, 2026 05:43

matheusaaguiar changed the title ~~Make legacy codegen remove custom errors when compiler option revert-strings=strip is requested~~ Do not remove custom errors in IR codegen when strip revert strings is requested Feb 26, 2026

cameel requested a review from rodiazet March 16, 2026 13:25

matheusaaguiar force-pushed the fixRevertStringFlagInconsistentHandling branch 3 times, most recently from f3491de to 1de843a Compare March 17, 2026 01:38

rodiazet requested changes Mar 17, 2026

View reviewed changes

matheusaaguiar added 2 commits March 18, 2026 02:01

Add tests

9386d87

Do not remove custom errors in IR pipeline when strip revert strings …

f761ded

…is requested

matheusaaguiar force-pushed the fixRevertStringFlagInconsistentHandling branch from 1de843a to f761ded Compare March 18, 2026 05:02

fixup! Add and update tests

5efedf8

Conversation

matheusaaguiar commented Feb 11, 2026

Uh oh!

cameel commented Feb 11, 2026

Uh oh!

nikola-matic left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rodiazet left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

matheusaaguiar Mar 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

rodiazet Mar 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

matheusaaguiar Mar 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

matheusaaguiar Mar 18, 2026 •

edited

Loading

rodiazet Mar 16, 2026 •

edited

Loading

matheusaaguiar Mar 18, 2026 •

edited

Loading

rodiazet Mar 17, 2026 •

edited

Loading

rodiazet Mar 17, 2026 •

edited

Loading