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

Graceful handling of non-ICE exceptions #15139

Open
cameel opened this issue May 23, 2024 · 1 comment
Open

Graceful handling of non-ICE exceptions #15139

cameel opened this issue May 23, 2024 · 1 comment
Labels
annoys users 😢 medium effort Default level of effort medium impact Default level of impact must have Something we consider an essential part of Solidity 1.0.

Comments

@cameel
Copy link
Member

cameel commented May 23, 2024

Abstract

Currently exceptions such as UnimplementedFeatureError, CodeGenerationError, StackTooDeepError and CompilerError are reported as internal compiler errors, i.e. by completely interrupting what the compiler is doing and printing out diagnostic information that's meant to help with tracking down a bug in the compiler. This behavior is appropriate for development assertions, which generally won't fail unless there is a bug, but not for errors that users will normally encounter when compiling their code.

Motivation

The current behavior is very confusing to users as evidenced by the discussion thread in #12783.

There have been some limited attempts at improving this, for example we do catch UnimplementedFeatureError if it carries a source location. Unfortunately this is not nearly enough. The solUnimplementedAssert() macro for raising such errors does not even allow attaching a location, so it is uncommon to have one, even when it is easy to obtain. E.g. in #14913 the obvious thing to do would be to point at the problematic require() call. Instead the user gets diagnostic info pointing at a location in the compiler's source, which is useless in that context. In other cases, like #15001 (comment), this limitation leads to dropping otherwise useful tests.

Specification

There are several things we should do to address this situation:

  • UnimplementedFeatureError, CodeGenerationError, StackTooDeepError and CompilerError should be caught and reported as proper compilation errors even if they do not have a location.
  • The errors should include location where possible:
    • solUnimplemented() should allow including location.
    • solUnimplementedAssert() should be removed to avoid suggesting that it's an assertion like solAssert(). It's actually a validation.
      • Its message parameter should have been mandatory. A message for the user should always be provided with such errors.
    • Add locations to the current uses of UnimplementedFeatureError where available. There aren't that many of them and it will greatly improve user experience.
    • For cases where location is not known we could try iterating on Daniel's attempt at obtaining the location automatically.
  • Review existing handlers for these exceptions. After doing the above most of them should not be necessary. All these errors inherit from util::Exception and if not caught where we expect them, they should generally be treated as any other unexpected exception by the top-level handler.

Backwards Compatibility

I don't think there are any backwards-compatibility concerns associated with this change. Currently these exceptions produce diagnostic output that is platform-dependent and we don't guarantee its stability in any way.

@nikola-matic
Copy link
Collaborator

When StackTooDeepError is caught and handled gracefully, add the following test to libsolidity/semanticTests/errors/:

error E(
    uint, uint, uint, uint,
    uint, uint, uint, uint,
    uint, uint, uint, uint,
    uint, uint, uint, uint
);

contract C {
    function f() public {
        require(false, E(
            0, 0, 0, 0,
            0, 0, 0, 0,
            0, 0, 0, 0,
            0, 0, 0, 0
        ));
    }
}

// ----
// f() ->

in order to catch said error introduced in #15174.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
annoys users 😢 medium effort Default level of effort medium impact Default level of impact must have Something we consider an essential part of Solidity 1.0.
Projects
None yet
Development

No branches or pull requests

2 participants