Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Full rework of the compiler message handling pipeline. Remove old-style message generation that was
based purely on strings that were formatted in-place, and instead implement structured logging where
main compiler code only instantiates objects with required information.
Explanation of changes for this commit will be split into several sections, matching number of edit
iterations I had to make in order for this to work properly.
* File reorganization
In addition to the architectural changes this PR requires some type definition movements.
- PNode, PSym and PType definitions (and all associated types and enums) were moved to the
ast_types.nim file which is then reexported in the ast.nim
- Enum defininitions for the nilability checking were moved to nilcheck_enums.nim and then
reexported
- Enum definitions for the VM were moved to to vm_enums.nim
* New files
- Main definition of the report types is provided in the reports.nim file, together with minor
helper procedures and definition of the ReportList type. This file is imported by options, msgs
and other parts of the compiler.
- Implementation of the default error reporting is provided in the cli_reporter.nim - all
disguisting hardcoded string hacks were moved to this single file. Now you can really witness the
"error messages are good enough for me" thinking that prevailed in the compiler UX since it's
inception.
* Changed files
Most of the changes follow very simple pattern - old-style hardcoded hacks are replaced with
structural report that is constructed in-place and then converted to string /when necessary/. I'm
pretty sure this change already puts me close to getting CS PHD - it was *unimaginably hard* to
figure out that hardcoding text formatting in place is not the best architecture. Damn, I can
probably apply to the nobel prize right now after I figure that out.
** Changes in message reporting pipeline
Old error messages where reportined via random combinations of the following:
- Direct calls to `msgs.liMessage` proc - it was mostly used in the helper templates like
`lintReport`
- `message`
- `rawMessage`
- `fatal`
- `globalError` - template for reporting global errors. Misused to the point where name completely
lost it's meaning and documentation does not make any sense whatsoever. [fn:global-err]
- `localError` - template for reporting necessary error location, wrapper around `liMessage`
- `warningDeprecated` - used two times in the whole compiler in order to print out error message
about deprecated command switches.
Full pipeline of the error processing was:
- Message was generated in-place, without any colored formatting (was added in `liMessage`)
- There were several ways message could be generated - all of them were used interchangeably,
without any clear system.
1. Some file had a collection of constant strings, such as `errCannotInferStaticParam = "cannot
infer the value of the static param '$1'"` that were used in the `localReport` message
formatting immediately. Some constants were used pretty often, some were used only once.
2. Warning and hint messages were categorized to some extent, so and the enum was used to
provide message formatting via `array[TMsgKind, string]` where `string` was a `std/strutils`
formatting string.
3. In a lot of cases error message was generated using hardcoded (and repeatedly copy-pasted)
string
- It was passed down to the `liMessage` (removed now) procedure, that dispatched based on the global
configuration state (whether `ignoreMsgBecauseOfIdeTools` holds for example)
- Then message, together with necessary bits of formatting (such as `Hint` prefix with coloring) was
passed down to the `styledMsgWriteln(args: varargs[typed])` template, whcih did the final dispatch
into
- One of the two different /macros/ for writing text out - `callStyledWriteLineStderr` and
`callIgnoringStyle`.
- Dispatch could happen into stdout/stderr or message writer hook if it was non-nil
- When message was written out it went into `writeLnHook` callback (misused for
`{.explain.}` [fn:writeln-explain]) (hacked for `compiles()` [fn:writeln-compiles]) and was
written out to the stdout/stderr.
It is now replaced with:
- `Report` object is instantiated at some point of a compilation process (it might be an immediate
report via `localError` or delayed via `nkError` and written out later)
- `structuredReportHook` converts it to string internally. All decitions for formatting, coloring
etc. happen inside of the structured report hook. Where to write message, which format and so on.
- `writeLnHook` /can/ be used by the `structuredReportHook` if needed - right now it is turned into
simple convenience callback. In future this could even be replaced with simple helper proc, for
now I left it as it is now because I'm not 100% sure that I can safely remove this.
** Changes in the warning and hint filtering
Report filtering is done in the particular *implementation* of the error hook -
`options.isEnabled()` provides a default predicate to check whether specific report can be written
out, but it must still be called manually. This allows to still see all the reports if needed. For
example, `cli_reporter.reportHook()` uses additional checks to force write some reports (such as
debug trace in `compiles()` context).
Most of the hint and warning were already categorized, altough some reports had to be split into
several (such as `--hint=Performance` that actually controlled reports for `CopiesToSink` and
`CannotMakeSink`, `--hint=Name` that controlled three reports).
None of the errors were categorized (reports from the `nkError` progress was incorporated, but in
I'm mostly describing changes wrt. to old reporting system) and all were generated in-place
** Minor related changes
- List of allowed reports is now stored in the `noteSets: array[ConfNoteSet, ReportKinds]` field of
the `ConfigRef`, instead of *seven* separate feilds. Access is now done via getter/setter procs,
which allows to track changes in the current configuration state.
- Renamed list of local options to `localOptions` - added accessors to track changes in the state.
- Move all default report filter configuration to `lineinfos.computeNotesVerbosity()` - previously
it was scattered in two different places: `NotesVerbosity` for `--verbosity:N` was calculated in
`lineinfos`, foreignPackageNotesDefault was constructed in `options`
- Changed formatting of the `internalAssert` and `internalError` messages
- Removed lots of string formatting procs from the various `sem*` modules - ideally they should
*all* be moved to the `cli_reporter` and related.
-------
Additional notes
[fn:global-err] As I said earlier, `globalError` was misused - it is still not possible to fully get
rid of this sickening hack, since it is used for /control flow/ (you read this correct - it is an
error reporting template, and it is used for control flow. Wonderful design right?).
So, in short - `globalError` can raise `ERecoverableError` that can be caught in some other layer
(for example `sem.tryConstExpr` abuses that in order to bail out (pretty sure it was done as an
'optimization' of some sort, just like 'compiles') when expression fails to evaluate at
compile-time [fn:except])
[fn:except] nim-works#94 (comment)
[fn:writeln-explain] Of course you can't have a proper error reporting in the nim compiler, so this
hook was also misused to the point of complete nonsense. Most notable clusterfuck where you could
spot this little shit is implementation of `{.explain.}` pragma for concepts. It was implemented via
really 'smart' (aka welcome to hell) solution in
nim-works@74a8098 (sigmatch
~704) and then further "improved" in
nim-lang/Nim@fe48dd1 by slicing out parts
of the error message with `let msg = s.replace("Error:", errorPrefix)`
For now I had to reuse similar hack - I *do* override error reporting hook in order to collect all
the missing report information. In the future it would be unnecessary - when concept is matched it's
body will be evaluated to `nkError` with reports that are written out later.
[fn:writeln-compiles] when `compiles()` check is executed, all error output is temporarily disabled
(both stderr and stdout) via setting allowed output flags to `{}` (by default it is set to- Loading branch information
1 parent
fd14e3d
commit 3fb02e1