Replace unnecessary validations with assertions #15185
Conversation
cameel
force-pushed
the
replace-unnecessary-validations-with-assertions
branch
from
9cd0682
to
57ba060
Compare
cameel
force-pushed
the
replace-unnecessary-validations-with-assertions
branch
from
57ba060
to
9068112
Compare
cameel
force-pushed
the
replace-unnecessary-validations-with-assertions
branch
3 times, most recently
from
7ab08c3
to
00995d4
Compare
cameel
force-pushed
the
replace-unnecessary-validations-with-assertions
branch
from
00995d4
to
2694190
Compare
| // This FatalError con occur if the errorReporter has too many errors. | ||
| yulAssert(!watcher.ok(), "Fatal error detected, but no error is reported."); | ||
| // NOTE: There's a cap on the number of reported errors, but watcher.ok() will work fine even if | ||
| // we exceed it because the reporter keeps counting (it just stops adding errors to the list). | ||
| // Note also that fact of exceeding the cap triggers a FatalError so one can get thrown even | ||
| // if we don't make any of our errors fatal. | ||
| yulAssert(!watcher.ok(), "Unreported fatal error: "s + error.what()); |
There was a problem hiding this comment.
I initially thought this was a bug and that the assert would fail with more than 256 errors. Turns out it doesn't because the error reporter keeps increasing the count. I added a test against this in any case - turns out we didn't have one yet.
| @@ -63,7 +63,7 @@ bool NameAndTypeResolver::registerDeclarations(SourceUnit& _sourceUnit, ASTNode | |||
| } | |||
| catch (langutil::FatalError const& error) | |||
| { | |||
| solAssert(!m_errorReporter.errors().empty(), "Unreported fatal error: "s + error.what()); | |||
| solAssert(m_errorReporter.hasErrors(), "Unreported fatal error: "s + error.what()); | |||
There was a problem hiding this comment.
This one is a bug though. hasErrors() is not the same as !errors.empty(). The latter will still count warnings and infos. This means we'll silence a buggy FatalError if some unrelated warning or info happened to be reported before it.
I don't think the results of this are visible to the user, since it only makes a difference when combined with another bug (throwing FatalError without reporting it), but nevertheless, it looks unintended.
CompilerStackhas a lot of checks for things that are never meant to happen and do not need a user-facing validation. We normally enforce such expectations with assertions, because violating them is a bug, not a compilation error.This has been bothering me for a long time, but became especially annoying now when I was reviewing #15168 and needed to figure out what kinds of runtime exceptions we throw and when.
I also fixed two somewhat related things:
InternalCompilerErrorbeing thrown directly rather than as a result of a failed assert.FatalErrorbeing rethrown rather than immediately handled as a violated assumption. Now it's an ICE and the assert makes it clear that it's unexpected, not requiring extra comments.