Require with custom error

[nikola-matic]

Fixes: #14442

• changelog
• docs
• more tests
• forbid in legacy
• ast id in function signature hash

[ekpyron]

I'm not sure how many of these we may already have, but since we're changing the return type of the error constructor calls, it'd be good to have a few more tests like calsuchling as a standalone statement CustomError(); or using in abi.encode and as a type in abi.decode, passing one to a regular function and so on - maybe also even for uncalled error constructors (CustomError; and passing that around in those cases - we don't need to be too strict there, but it'd be good to see the behaviour in tests anyways, although those we may also already have, I didn't check). The post type checker thing should catch all those calls to error constructors, but better to make sure.

[ekpyron]

Also maybe a small AST test with just a require-with error call, s.t. we can see if and how the new type shows up there. EDIT: I just saw that there's already one at least where it shows up.

[ekpyron]

We might also instead reuse m_currentStatement here, i.e. set that here to the outer call? Well, not sure which is better. If you flip m_currentStatement we might be able to assert that it's nullptr when we hit this - since revert and emit statements should only allow errors and events after earlier analysis I guess? But yeah, whatever is easiest to make sure we don't get weird combinations of things slipping through.
I haven't fully thought this through in nested cases yet - might be worthwhile to check for pathological things like revert SomeError(require(false,AnotherError())); or so - none of this should be valid (that example should e.g. be rejected at the very least as an invalid argument type to SomeError), but worthwhile to think about whether there can be a case that slips through.

[nikola-matic]

Sure; the reason I did it this way is precisely because I couldn't think of a situation where a require would be within a nested call.

[ekpyron]

For the docs by the way, it's important to be clear about the fact that the arguments to the error are evaluated unconditionally (like for any regular function call).

We should also have a pathological test case for that like:

contract C {
  error E(uint);
  function r() internal returns (uint) {
    assembly { mstore(0, 7) return(0,32) }
    return 42;
  }
  function f() public returns (uint) {
    require(false, E(r());
  }
  function g() public returns (uint) {
    require(true, E(r());
    revert E(42);
  }
}

Both calling f and g should return 7 - no reverts.

We can also point out that that's consistent with a require we already have that takes a string.

We can also further point out that the allocation of the memory for the error-based revert reason will only happen in the reverting pass and that constants and string literals will become optimized, so the "expected" use should remain cheap.

[cameel]

Suggested change

	// @returns function that has equivalent logic of a require statement, but with a custom
	// error constructor parameter.
	// @returns function that has equivalent logic of a require function, but with a custom
	// error constructor parameter.

[matheusaaguiar]

Just a bunch of nitpicks. I am still looking at the code, but can't see anything wrong.

[matheusaaguiar]

Suggested change

	* Introduce a new overload ``require(bool, Error)`` that allows usage of ``require`` functions with custom errors. This feature is available in the ``via-ir`` pipeline only.
	* Introduce a new usage of ``require`` functions with custom errors. This feature is available in the ``via-ir`` pipeline only.

Maybe omitting this detail makes it simpler to the general user?

[cameel]

I have a few small corrections and final nitpicks, but nothing serious. None of this should really take more than a few minutes to fix and when it's dealt with I'm going to approve and we can merge.

[cameel]

One thing that I only realized now is that you generate names for the require function's parameters based on the expressions used in the call rather than on the error definition (which has no names for parameters, so fair enough). In other cases, e.g. abi.encode(), we don't do this and we instead generate a sequence of numbered names with suffixedVariableNameList(). In your case we get parameters named like expr_29, which looks a bit odd to me because that's how I expect locals to be named.

I see that it was already in your initial implementation, but back then it was up to you how you name them. Now revertWithError() actually expects this naming pattern so I guess it's fine and maybe better than the alternative of having to explicitly pass those parameter names to it. But I think it's at least worth a comment here pointing out that this is happening. The fact that the variable here is named errorArgumentVars and not errorParameterVars is a hint but it's too subtle. Also, I'd rename the errorArguments template var to something like functionParameterNames, because that's more accurate.

I also started wondering it this won't cause some naming conflicts with locals, but I guess it shouldn't because the name counter is global. Old names won't appear here and the new ones will get higher numbers...

[cameel]

I see that #14913 (comment) was still not addressed.

I'm going to tentatively approve the PR because it's the last thing to do there, but please apply my suggestion below before merging it.

[0xbok]

should be require(amount <= balances[msg.sender]?

[nikola-matic]

Ha, you're totally right; I'll open a PR to fix it. Thanks!

[0xbok]

Opened a pr actually #15055