Closed zerbina closed 2 months ago
@saem: Here are the observations about error reporting / handling in NimSkull that shaped the current design. Feel free to voice your disagreement with any of them.
wrapper nkError
errors are cumbersome. They tend to force tracking whether there was an error somewhere (e.g., hasError
), which adds a lot of noise to the analysis procedures. The wrapper errors also complicate error correction, as they hide the wrapped expression's type
the facilities introduced to simplify error propagation, CheckedAst
and ElaborateAst
, haven't seen any broader usage so far
except for fatal errors, reporting something (whether it's a warning, hint, or error) is generally fire-and-forget. That is, the analysis reporting something doesn't (nor needs to) care what exactly happens with the report
analysis doesn't abort on an error early enough. If errorMax
is 1, or analysis happens as part of a "does it sem-check" (i.e., compiles
) query, the compiler could either terminate right away, or hand back control to whatever invoked analysis, but sem
currently carries on until the first localReport
is reached (which can be quite far away)
the error reporting/handling system is very complex, taking into account a lot of state in order to decide what to do (e.g., whether to raise, whether to quit, whether to output, etc.)
error handling configuration is misplaced in ConfigRef
I've also tried to take into account the previous discussion that took place regarding error handling and reporting.
The system I came up with is this:
ReportContext
typeReportContext
implementation to restore some safe context (or terminate the process -- whatever it sees fit)This abstracts away the actual handling and reporting of a message, leaving the logic wanting to report something with a simple interface, while allowing one to swap the implementation at run-time.
For example, errorMax
handling, promoting hints and warnings to errors, and stopping on errors would all be the responsibility of the ReportContext
implementation.
The used nomenclature is a bit fuzzy, I think, and it probably needs some adjustment.
Error correction is (or at least should) be the sole responsibility of an analysis pass, and it's thus not relevant to the ReportContext
system.
I believe this is good to merge, doing so now.
Summary
ReportContext
), intended for use by the compilerReportContext
intosource2il
expr
test runner now report a text message for each errorDetails
reporting
anddefault_reporting
, containing theReportContext
type and a simple implementation thereof, respectivelyReportContext
instance when creating aModuleCtx
source2il
, report an error message whenever detecting an errorThe error messages are currently formatted in-place, but the idea is to eventually send dedicated message objects (which store all necessary information for rendering the message to text) to the
ReportContext
.