Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Typed errors / exceptions #7877

Closed
1 of 3 tasks
chriseth opened this issue Dec 3, 2019 · 42 comments
Closed
1 of 3 tasks

Typed errors / exceptions #7877

chriseth opened this issue Dec 3, 2019 · 42 comments
Assignees
Labels
closed due inactivity The issue/PR was automatically closed due to inactivity. language design :rage4: Any changes to the language, e.g. new features stale The issue/PR was marked as stale because it has been open for too long.

Comments

@chriseth
Copy link
Contributor

chriseth commented Dec 3, 2019

Add the error keyword to specify error / exception types in the same way as events. These errors can then be thrown using throw and they can be used inside revert and require causing the 4-byte selector of its type being used for the revert data. The matching catch clause destructures them.

Note: For now, we implicitly use the error type Error(string memory) and encode its 4-byte selector in the revert data.

TODO:

  • disallow the following selectors: all zeros, all ones, selector of Panic and of Error
  • add .selector and .signature member for errors?
  • catch errors try/catch for custom errors #11278
@christianparpart
Copy link
Member

As previously suggested, I've moved the textual changes to a Gist page, that is now to be treated as technical document (not as a blog post), so I can leave in syntax/sema/codegen paragraphs.

https://gist.github.com/christianparpart/8dbfafca45306d102eb115485aa48fe3

/cc @chriseth

@axic axic added the language design :rage4: Any changes to the language, e.g. new features label May 8, 2020
@chriseth
Copy link
Contributor Author

@franzihei can you create a blog post out of the gist for the category "feature proposals"?

@franzihei
Copy link
Member

We don't have such a category on the blog. We only have:

  • Announcements
  • Explainers
  • Releases
  • Security Alerts

Maybe "Explainers"?
And yes, I can make a blog post out of this. : )

@franzihei
Copy link
Member

Or do you mean we should make it a series, just like with the Solidity 0.6.x feaures? We could embed it in the title like Solidity feature proposals: Error types?

@chriseth
Copy link
Contributor Author

Let's discuss in the meeting if we actually want a blog post pre-release about this.

@chriseth
Copy link
Contributor Author

Made another writeup: https://hackmd.io/b0vr4MFrSkCLlBEacnds0Q

@cameel cameel added the feature label Sep 26, 2020
@haltman-at
Copy link
Contributor

This is an interesting proposal! I have a few questions on reading this:

  1. Will errors be able to be free-standing, or will they require to be defined in contracts? (Also I'm assuming they'll be heritable?) If they're free-standing it's harder to say what ABI they should go in. OTOH, unlike events, errors are necessarily an anything-can-come-from-anywhere scenario, so maybe there's nothing wrong with taking the "effective ABI" approach for errors in the ABI, unlike with events? But it could still complicate looking up extra information in the AST for those who want to decode enums and such. Blech. This will need to be figured out.
  2. What will happen to messageless revert() and require(bool)? Will these continue to produce no return data at all, or will they have some new error type attached to them? (Like Error(), as opposed to Error(string)?)
  3. If you go with the more traditional ABI approach instead of the "effective ABI" approach for putting these in the ABI, I'm guessing Error(string) and Panic(uint) (and possibly Error(), if that's the answer to question 2) will just be built-in error types that won't go in the ABI as they're well-known builtins that don't need to be specified? Or how will this be handled?
  4. To what extent will selectors be checked for collisions? Like, since you're only using the first four bytes of the hash, collisions will definitely be possible. Now currently when compiling a contract, function selectors are checked for collisions, but that comparison only needs to be carried out between functions within each contract separately. However errors are an "anything can come from anywhere" sort of thing... is there going to have to be cross-contract selector collision checking? Or is this just not worth handling?

Here's the sort of nasty scenario I'm imagining... imagine two contracts that both define an error with signature MyError(uint8), except that those uint8s in the ABI are actually enums in the original contracts, and they're different enums, with different numbers of possibilities. Again -- maybe this sort of thing is not worth handling, and we just have to accept that errors can be ambiguous, much as we already have to accept this for other things and it isn't a big problem. Honestly I think that's probably fine from my angle as a decoder writer, but it might be less OK from more of a Solidity language user angle. (Imagine that you try to catch an error, but the catch block panics when it notices that the enum it has received is out of bounds.) Or maybe that's not a big problem either, I dunno. So I wanted to point out the issue at least.

Also, I'm guessing revert(string) and require(bool,string) will be kept for compatibility?

But yeah while it maybe has some minor kinks to work out on the whole this seems pretty nice! :)

@chriseth
Copy link
Contributor Author

1. Will errors be able to be free-standing, ...

They can be defined both in contracts and on the file level. The plan is, as with events, to include in the ABI all errors that can possibly be thrown by the contract, regardless of where they are defined.

2. What will happen to messageless `revert()` and `require(bool)`? 

I did not plan to change this, so still no error message. This also means that no error message is a "regular" error, not a panic.

3. [put built-in errors in the ABI?]

That's not clear yet. It might be better to just always put them in the ABI, so that it is also compatible with other languages that choose to have different built-in errors.

4. [selector collisions]

Indeed, collisions are a problem. We can warn if two errors with the same signature occur in the same "translation unit" (or even try/catch block), but we cannot detect if an externally-called contract (to which we do not have the source) uses an error with the same signature as one that we have defined.

My main question is if there is a big use-case for distinguishing between different errors at the code-level at all, or if this will be mainly used as a user-interface convenience that is much cheaper than error strings.

@haltman-at
Copy link
Contributor

The plan is, as with events, to include in the ABI all errors that can possibly be thrown by the contract, regardless of where they are defined.

Hm, is that the plan for events, or is the plan for events to have a separate effectiveABI or whatever...?

@chriseth
Copy link
Contributor Author

One still open problem is how to deal with side-effects of the construction of an error:

require(condition, MyError(f()));

This should be syntactically valid and I would assume that side-effects of f take effect regardless of the value of condition since require is "just another function" and this is explicitly not equivalent to if (condition) { revert(MyError(f())); }.

This was referenced Feb 11, 2021
@axic
Copy link
Member

axic commented Feb 11, 2021

Moved from #10922:

These are the require statements in the ERC777 implementation of OZ (removed duplicates messages):

require(_msgSender() != operator, "ERC777: authorizing self as operator");
require(operator != _msgSender(), "ERC777: revoking self as operator");
require(isOperatorFor(_msgSender(), sender), "ERC777: caller is not an operator for holder");
require(recipient != address(0), "ERC777: transfer to the zero address");
require(holder != address(0), "ERC777: transfer from the zero address");
require(account != address(0), "ERC777: mint to the zero address");
require(from != address(0), "ERC777: burn from the zero address");
require(from != address(0), "ERC777: send from the zero address");
require(to != address(0), "ERC777: send to the zero address");
require(spender != address(0), "ERC777: approve to the zero address");
require(!to.isContract(), "ERC777: token recipient contract has no implementer for ERC777TokensRecipient");

With the current syntax, one could create the following errors:

error AuthorizingSelfAsOperator();
error RevokingSelfAsOperator();
error CallerNotAnOperator();
error TransferToZero();
error TransferFromZero();
error MintFromZero();
error BurnFromZero();
error SendFromZero();
error SendToZero();
error ApproveToZero();
error NoImplementForRecipient();

Is there any other option they could do with the current syntax?

If we consider having enum datatypes, then we could replace these with:

enum ERC777ErrorEnum {
    AuthorizingSelfAsOperator,
    …
}

error ERC777Error(ERC777ErrorEnum);

We could go further and say that errors are actually enum datatypes:

error ERC777Error {
    AuthorizingSelfAsOperator,
    …
}

This would generate a function signature of ERC777Error(enum) and then following the encoding rules of the enum.

@hrkrshnn
Copy link
Member

In these proposals like require(c) throws Error(), will something like if (c) { throws Error() } be also valid? Or is it only relevant in the context of require?

@axic
Copy link
Member

axic commented Feb 17, 2021

If we go with something like throw Error() or revert Error() that should be valid syntax on its own.

@axic
Copy link
Member

axic commented Feb 17, 2021

I came around to think that the unless (c) revert E(a, b) construct is the best, as long as we consider unless as an inverted if, and we consider unless (c) and revert E(a, b) valid constructs in their own. That means we do not introduce some specific syntax here, rather make use of two valid constructs.

Basically unless (c) is an alias to if (!c) and can be used interchangeably in the language.

@hrkrshnn
Copy link
Member

hrkrshnn commented Feb 17, 2021

Basically unless (c) is an alias to if (!c) and can be used interchangeably in the language.

I feel that having both if and unless in the same code is slightly confusing. I would prefer if the unless statement was always followed by a revert or an error.

Also, solidity doesn't normally have syntactic sugars, like this unless proposal. So not sure if this is the right direction.

@axic
Copy link
Member

axic commented Feb 17, 2021

Since we weren't fully convinced with unless as a keyword, thesaurus offers two alternatives: except and without.

without (signature.length == 65) revert Error("invalid length");

@chriseth
Copy link
Contributor Author

We more or less came to the conclusion that revert CustomError(arg1, arg2); is a good syntax for using revert with a custom error because of its alignment to the way events are emitted (emit EventName(arg1, arg2);).

What is still open is how to properly deal with require. Of course, require is optional, since if (!c) revert CustomError(arg1, arg2); can be used instead of it. On the other hand, its declarative way to express preconditions is very handy and intuitive.

The first idea was to use require(c, CustomError(arg1, arg2)). This has the big downside that it is not clear if side-effects of the construction of the error take effect if c is true.

One way to make this clearer and also have the syntax for require more closely follow the one of revert is to use something from the following list:

  • require(c) else CustomError(arg1, arg2);
  • require(c): CustomError(arg1, arg2);
  • require(c) or CustomError(arg1, arg2);
  • require(c) or revert CustomError(arg1, arg2);
  • require(c) else revert CustomError(arg1, arg2);

An different approach is to avoid the use of the require keyword, but instead remove the negation in if (!c) revert CustomError(arg1, arg2);: This is by introducing an unless statement:

  • unless (c) revert CustomError(arg1, arg2)

unless (c) ... is equivalent to if (!c) ... and it can also be used without revert.

@daejunpark
Copy link

[Thoughts from someone who prefers restrictions over convenience for security.]

I vote for require(c) or revert CustomError(arg1, arg2);. It is clear and least confusing. The only downside I can think of is that it is slightly verbose, but considering the importance of input validation for security, I think such inconvenience(?) is worth taking.

BTW, in general, I'm concerned about introducing unless (or without etc.) that is the negation of if. It seems quite confusing and easy to make a mistake especially when both if and unless are used together at the same place.

@cameel
Copy link
Member

cameel commented Feb 19, 2021

I just found #9454 ("Add require(bool,bytes) global function") which seems very relevant to the discussion about require() syntax.

@axic
Copy link
Member

axic commented Feb 19, 2021

Isn't that already possible?

Shouldn't this already work:

(bool result, bytes memory returndata) = someAddress.call{ value: msg.value }('');
require(result, string(returndata));

@cameel
Copy link
Member

cameel commented Feb 19, 2021

Good point. I just checked and it does compile.

@chriseth
Copy link
Contributor Author

We will proceed with revert MyError() and defer the decision on require.

@ekpyron
Copy link
Member

ekpyron commented Mar 1, 2021

Not sure if I'm a bit late with a suggestion like that... but have we ever considered just making errors proper types? I.e.

contract C {
  error SomeError(uint);
  error SomeOtherError(bytes);
  function f(uint v, bytes memory b) internal pure returns(error x, uint y) {
    if (v == 0) x = SomeError(42);
   else if (v == 10) x = SomeOtherError(b);
   else y = b.length;
  }
  function g(bytes memory b) public {
    (error err, uint y) = f(123, b);
    if (!err)
    {
        require(y > 0, err ? err : SomeError(1));
    }
  }
}

It would just need to be an alias of bytes memory. Converting to boolean would just check whether the size is empty. The default value would be an empty bytes memory...
This way (1) the error definitons would actually be called and actually return something and we could think of require as being overloaded with the error type on the second argument. So in this scenario we don't need emit-like syntax, because function call syntax makes sense. (2) It allows for passing them around conveniently. (3) it clarifies the evaluation order (clearly the arguments and the error itself will be created before the actual require - although the optimizer could still move it of course, if admissible). (4) We could in the future even allow to deconstruct them:

error err = ...;
try err { /* !err case */ } catch SomeError(uint x) { ... }

@chriseth
Copy link
Contributor Author

chriseth commented Mar 1, 2021

Yes, allowing errors to have their own type is a logical extension, but we did not want to go there yet to simplify the implementation.

I think evaluation order is still not clarified, because the syntax in the current PR is still possible: require(c, SomeError(f())).
Also I don't think most people will care about what SomeError(f()) returns internally...

@ekpyron
Copy link
Member

ekpyron commented Mar 1, 2021

Yes, require(c, SomeError(f())) is still possible - but if I know that that's equivalent to error x = SomeError(f()); require(c, x); i.e. it really is just a function call like any other, then that seems quite clear to me. (Not if it's a syntactic restriction to only be able to use errors as second argument of require or first argument of return, though).
I'd also consider still making it revert(SomeError(f()); after all, if errors really become special functions returning an error type...

@tfalencar
Copy link

tfalencar commented Oct 7, 2021

using require in the following way:

require(c) else E(a, b);

gets complicated because require already does other things, as pointed out. How about instead go for a new keyword?

guard(c) else revert E(a,b); 

This seems to be simpler to implement (less upfront changes needed), and seems to provide a cleaner upgrade path for solidity users.

Anyway, I think at this point any decision would be better than stalling progress.

@chriseth
Copy link
Contributor Author

chriseth commented Oct 7, 2021

@tfalencar we settled with if (!c) revert E(a, b);

@fulldecent
Copy link
Contributor

We can probably say that using Errors is best practice and is or will be preferred over revert strings.

Also I look forward to a future where Errors can be localized (we can do this entirely in NatSpec).


In many (most) use cases, E(a, b); will be simple. And there is no risk of the expression reverting when instantiating it. The if (!c) revert E(a, b); syntax causes two issues:

  1. It causes people to think in double negative, whereas some people prefer to think in assertions as positive prose.
  2. It requires to add 5+ lines of source code per unique error on top of the old way, if following Style Guide best practices.

Here is an example.

// SPDX-License-Identifier: GPL-3.0
pragma solidity >=0.8.0;
// This error condition is documented, but uses an error string which is not localizable
contract Good {
    // @notice This is a really BIG thing
    function doThing() external view {
        require(tx.origin == address(0), "Only the Devine can execute");
    }
}
// This error condition is documented and uses localizable errors
contract Better {
    // @dev The transaction origin is not devine
    error NotGod();

    // @notice This is a really BIG thing
    function doThing() external view {
        if (tx.origin != address(0)) {
            revert(NotGod());
        }
    }
}
// This error condition is documented and uses localizable errors
contract Best {
    // @dev The transaction origin is not devine
    error NotGod();

    // @notice This is a really BIG thing
    function doThing() external view {
        require(tx.origin == address(0), NotGod());
    }
}

Here is a proposal for a syntactic sugar which solves the two issues above while avoiding the possibility that evaluating the second parameter of require() will itself result in a separate revert.

I would like to see a differentiation between "simple" error invocations and "complex" invocations.

Any error invocation that uses only compile time constant values is "simple". Others are "complex".

Now the require function can be overloaded with require(bool expression, SimpleError error).

Test case:

require(false, SomeError(tx.origin.call()))

will result in "The require function with an error parameter only accepts simple errors. Try using if/revert instead."

@github-actions
Copy link

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 github-actions bot added the stale The issue/PR was marked as stale because it has been open for too long. label Feb 25, 2023
@github-actions
Copy link

github-actions bot commented Mar 5, 2023

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.

@github-actions github-actions bot added the closed due inactivity The issue/PR was automatically closed due to inactivity. label Mar 5, 2023
@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Mar 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
closed due inactivity The issue/PR was automatically closed due to inactivity. language design :rage4: Any changes to the language, e.g. new features stale The issue/PR was marked as stale because it has been open for too long.
Projects
None yet
Development

No branches or pull requests