solidity
solidity copied to clipboard
Graceful handling of non-ICE exceptions
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:
-
UnimplementedFeatureError
,CodeGenerationError
,StackTooDeepError
andCompilerError
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 likesolAssert()
. 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.