Remove overly specific handlers for unexpected exceptions #15186
Conversation
| catch (Json::parse_error const& _exception) | ||
| { | ||
| return formatFatalError(Error::Type::InternalCompilerError, std::string("JSON parse_error exception: ") + util::removeNlohmannInternalErrorIdentifier(_exception.what())); | ||
| } | ||
| catch (Json::invalid_iterator const& _exception) | ||
| { | ||
| return formatFatalError(Error::Type::InternalCompilerError, std::string("JSON invalid_iterator exception: ") + util::removeNlohmannInternalErrorIdentifier(_exception.what())); | ||
| } | ||
| catch (Json::type_error const& _exception) | ||
| { | ||
| return formatFatalError(Error::Type::InternalCompilerError, std::string("JSON type_error exception: ") + util::removeNlohmannInternalErrorIdentifier(_exception.what())); | ||
| } | ||
| catch (Json::out_of_range const& _exception) | ||
| { | ||
| return formatFatalError(Error::Type::InternalCompilerError, std::string("JSON out_of_range exception: ") + util::removeNlohmannInternalErrorIdentifier(_exception.what())); | ||
| } | ||
| catch (Json::other_error const& _exception) | ||
| { | ||
| return formatFatalError(Error::Type::InternalCompilerError, std::string("JSON other_error exception: ") + util::removeNlohmannInternalErrorIdentifier(_exception.what())); | ||
| } | ||
| catch (Json::exception const& _exception) | ||
| { | ||
| return formatFatalError(Error::Type::InternalCompilerError, std::string("JSON runtime exception: ") + util::removeNlohmannInternalErrorIdentifier(_exception.what())); | ||
| } |
There was a problem hiding this comment.
Note that we do want this kind of error handling during deserialization of JSON input and serialization of output to JSON. It's covered by separate handlers in StandardCompiler::compile(string) below and is not being removed here.
I'm only removing the handlers for the exceptions that are basically equivalent to failed assertions. This is the case if they occur during processing of already deserialized input, compilation or preparing the output for serialization. This should not normally happen because we're expected to validate the input before using it and generate a proper JSONError if it's invalid. If they do happen it represents a bug - a missing validation or some kind of misuse of the JSON library.
There was a problem hiding this comment.
Note also that by removing those we're not losing any details. In the generic exception handler we'll still get the full message and also diagnostics saying where it was thrown from. For an ICE the message does not need to be sanitized with removeNlohmannInternalErrorIdentifier(), because it can be considered diagnostic info too. In fact, the more internal details the better.
This sanitization is still good for proper JSONErrors though, because those can appear in test expectations and are considered a part of our interface.
| catch (Error const& _error) | ||
| { | ||
| if (_error.type() == Error::Type::DocstringParsingError) | ||
| { | ||
| report(Error::Severity::Error, *boost::get_error_info<errinfo_comment>(_error)); | ||
| solThrow(CommandLineExecutionError, "Documentation parsing failed."); | ||
| } | ||
| else | ||
| { | ||
| m_hasOutput = true; | ||
| formatter.printErrorInformation(_error); | ||
| solThrow(CommandLineExecutionError, ""); | ||
| } | ||
| } |
There was a problem hiding this comment.
I always thought that we do this sometimes with analysis or parsing errors but turns out we don't. Removing this handler does not result in any failures in the test suite so no properly reported errors need it.
In particular DocstringParsingErrors are not fatal so ErrorReporter::docstringParsingError() does not throw them. And even if they were, ErrorReporter throws FatalError, not util::Error and that would be caught in CompilerStack::analyze().
I think that we only ever throw util::Error at the codegen and later stages, so handling it only in CompilerStack::compile() seems enough. It should never get to this handler or the one in StandardCompiler.
| if (_error.type() != Error::Type::CodeGenerationError) | ||
| throw; | ||
| // Since codegen has no access to the error reporter, the only way for it to | ||
| // report an error is to throw. In most cases it uses dedicated exceptions, | ||
| // but CodeGenerationError is one case where someone decided to just throw Error. | ||
| solAssert(_error.type() == Error::Type::CodeGenerationError); | ||
| m_errorReporter.error(_error.errorId(), _error.type(), SourceLocation(), _error.what()); |
There was a problem hiding this comment.
We should define a separate exception type for CodeGenerationError. It seems to be the only weird case that's thrown inside a util::Error. Other failures during codegen have dedicated exception classes: StackTooDeep, OptimizerError, etc.
We should also be handling those existing exceptions here along with CodeGenerationError, but that's a different story (and the subject of #15139).
cameel
force-pushed
the
remove-overly-specific-exception-handlers
branch
2 times, most recently
from
833fb69
to
f5bb300
Compare
cameel
force-pushed
the
remove-overly-specific-exception-handlers
branch
from
f5bb300
to
8a854a1
Compare
|
I found some more unnecessary handlers in other places. Their removal is based on the same principle so I thought I'd add them ere instead of creating a new PR. Please take another look at it. |
cameel
force-pushed
the
remove-overly-specific-exception-handlers
branch
2 times, most recently
from
114d706
to
1e9e83e
Compare
- In analysis we use an error reporter and never just throw util::Error. We do it in one case in the codegen (CodeGenerationError) but outside of that case this should not be treated as a proper way to report an error. - Now such errors will be treated as unexpected. They're bugs that should be fixed.
matheusaaguiar
force-pushed
the
remove-overly-specific-exception-handlers
branch
from
1e9e83e
to
c2c8c28
Compare
|
Thanks! |
In some places we have handlers for some exceptions that are unexpected and can reach those handlers only as a result of a bug. This is unnecessary and just complicates error handling. We should be explicitly handling expected exceptions and let everything else bubble up as far as possible and deal with it in the most generic handler possible.