ethereum / solidity

Solidity, the Smart Contract Programming Language
https://soliditylang.org
GNU General Public License v3.0
23.12k stars 5.73k forks source link

Graceful handling of non-ICE exceptions #15139

Open cameel opened 4 months ago

cameel commented 4 months ago

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 https://github.com/ethereum/solidity/pull/15001#discussion_r1603214954, this limitation leads to dropping otherwise useful tests.

Specification

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

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 commented 3 months ago

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 https://github.com/ethereum/solidity/pull/15174.