... | ... | @@ -75,12 +75,12 @@ As this strand of work touches a lot of modules, doing everything as a single ga |
|
|
</details>
|
|
|
|
|
|
|
|
|
# Short summary / Intro
|
|
|
# 1. Short summary / Intro
|
|
|
|
|
|
Following the high-level plan posted in [ghc proposal#306](https://github.com/ghc-proposals/ghc-proposals/pull/306), we have been planning on switching our error representation away from mere documents. The end goal is to have subsystem-specific error data types, where each constructor of such a data type will represent one error that this subsystem can throw. A global `GhcMessage` sum type would also be provided, at the driver level, so as to be able to represent any error thrown by any subsystem and provide a trivial mechanism for API users to deal with error values (namely: just hand them a bag of `GhcMessage` values).
|
|
|
The relevant ticket is #18516.
|
|
|
|
|
|
# Background: the current error infrastructure
|
|
|
# 2. Background: the current error infrastructure
|
|
|
|
|
|
<details><summary>Current error infrastructure (click the arrow to expand!)</summary>
|
|
|
|
... | ... | @@ -144,7 +144,7 @@ data WarnReason |
|
|
|
|
|
</details>
|
|
|
|
|
|
# New API
|
|
|
# 3. New API
|
|
|
|
|
|
Let's start by making `Messages` and `MsgEnvelope` polymorphic over `e`, which is the particular message they now carry:
|
|
|
|
... | ... | @@ -171,7 +171,7 @@ class Diagnostic a |
|
|
This allows us to move away from `SDoc` and simply instantiate `e` with a structured message, specific a particular compilation phase (parsing, tcRn, ...)
|
|
|
|
|
|
|
|
|
# The envelope contents (i.e. the diagnostics)
|
|
|
# 4. The envelope contents (i.e. the diagnostics)
|
|
|
|
|
|
We can get each subsystem to define their own message (diagnostic) types. At the beginning, to smooth out the integration, they can even be very simple wrappers around `DecoratedSDoc`s:
|
|
|
|
... | ... | @@ -213,7 +213,7 @@ This might involve systematically retaining a bit more information (context line |
|
|
|
|
|
At the "top level", in the driver, where we call the different subsystems to process Haskell modules, we would end up accumulating and reporting `GhcMessage` values. The goal is to have the GHC program emit the exact same diagnostics as it does today, but affect the API in such a way that GHC API users would at this point get a chance to work with the algebraic error descriptions, making inspection and custom treatment a whole lot easier to implement. We could perhaps even demonstrate it at this point by implementing a little "demo" of sorts for this new way to consume errors.
|
|
|
|
|
|
# New API key points
|
|
|
# 5. New API key points
|
|
|
|
|
|
API design explanation/considerations:
|
|
|
|
... | ... | @@ -392,7 +392,7 @@ A few details to highlight: |
|
|
* Right now, all code that generates warnings first checks to see whether the warning flag is enabled. With this proposed design, it seems straightforward to extend `Severity` with `SevIgnore` that instructs the logger to simply do nothing on a message.
|
|
|
* This design points out that `defaultReasonSeverity` is naughty, as it bypasses `DynFlags`. I bet there are real bugs in GHC around this, but they are hard to trigger: the warnings that bypass the normal mechanism arise in Core Lint, Stg Lint, the optimizer, and in two situations dealing with dynamic linking. (I searched for uses of `MCDiagnostic` with a hard-coded warning reason.) The warning text does not appear in the testsuite, so these code paths are completely untested. This proposed design does not fix these bugs, but it makes them more obvious by forcing the use of a function that is labeled as wrong.
|
|
|
|
|
|
# Eventual design
|
|
|
# 6. Eventual design
|
|
|
|
|
|
The design described above is a temporary waypoint. In particular, it includes `defaultReasonSeverity :: DiagReason -> Severity`, which cannot be implemented correctly, because determining the `Severity` of a `DiagReason` requires consulting the `DynFlags`. A more correct design would look like this:
|
|
|
|
... | ... | @@ -419,7 +419,7 @@ This eventual design classifies diagnostics as error or warnings *at birth*, usi |
|
|
|
|
|
Once this is done, we should hopefully be able to rid ourselves of `defaultReasonSeverity`. Exception: `defaultReasonSeverity` is correct for errors. So we might keep around some `errReasonSeverity :: DiagReason -> Severity` which always returns `SevError` after confirming that its argument is `ErrorWithoutFlag`, panicking otherwise. It's not clear whether this is a good design or a bad design.
|
|
|
|
|
|
# Suppressed warnings
|
|
|
# 7. Suppressed warnings
|
|
|
|
|
|
This section contains thoughts on improvements to the design to support more uniform suppression of warnings that the
|
|
|
user does not want. Note that this *builds on* the design above. It does *not* suggest a new alternative to the design above.
|
... | ... | @@ -442,9 +442,9 @@ Today, all code that creates a warning should check whether that warning is supp |
|
|
|
|
|
This code produces a warning that the monomorphism restriction has affect types. Note that it first checks whether `Opt_WarnMonomorphism` is enabled. Then, (maybe) it creates a warning with `WarningWithFlag Opt_WarnMonomorphism`. This is strage, because the code must interact with `Opt_WarnMonomorphism` *twice*: once to see whether the warnings should be generated at all, and again to mark the flag for the warning, which other code uses to determine whether the warning should be fatal (i.e. because of `-Werror`). This is error prone (every code that generates a warning must remember to make an extra check) and duplicative.
|
|
|
|
|
|
## Proposed Design A
|
|
|
## The new disign
|
|
|
|
|
|
One possibility is to introduce a new `Severity`:
|
|
|
We introduce a new `Severity` field:
|
|
|
|
|
|
```hs
|
|
|
data Severity = SevIgnore | SevWarning | SevError
|
... | ... | @@ -460,7 +460,13 @@ diagReasonSeverity _ ErrorWithoutFlag |
|
|
|
|
|
`SevIgnore`d diagnostics are never printed. This is nice and uniform with the other severities and would likely be very easy to implement. However, it's a bit disappointing in that we would continue to carry around lots of `SevIgnore`d warning messages, just to let them be garbage-collected at the end of execution.
|
|
|
|
|
|
## Proposed Design B
|
|
|
Now that we have the `Severity` field, we can remove the many places throughout GHC that check for a warning flag before emitting a warning. We must be slightly careful, though: it might be computationally intensive even to know whether to produce a warning, and it's foolish to work hard only to ignore the result. (Maybe sometimes laziness would save us, but not if any monadic work is involved, of course.) This computationally-intensive case is likely the exception, and so most places that might give rise to a warning can just skip the check entirely, leaving it to the error-message infrastructure above to toss out unwanted warnings.
|
|
|
|
|
|
A good way to approach this is simply to search for any occurrence of `Opt_Warn` in the code and remove extra conditionals.
|
|
|
|
|
|
## An alterantive design (not adopted)
|
|
|
|
|
|
<details><summary> An atternative design </summary>
|
|
|
|
|
|
Another possibility is to keep `data Severity = SevWarning | SevError` (as described above) but make the creation of `MsgEnvelope`s partial. To wit:
|
|
|
|
... | ... | @@ -485,17 +491,9 @@ mkMsgEnvelope dflags locn print_unqual err |
|
|
```
|
|
|
|
|
|
This appears to be more efficient (we more eagerly drop any ignored messages) but it requires all callers of `mkMsgEnvelope` to deal with the possibility of failure. A quick review of usage sites suggests this won't be hard (the result is often just put into a bag, and if `mkMsgEnvelope` returns `Nothing`, it's very easy to put nothing into a bag), but this design is definitely more invasive than Design A.
|
|
|
</details>
|
|
|
|
|
|
## Knock-on simplifications
|
|
|
|
|
|
One we execute either Design A or Design B, we can then remove the many places throughout GHC that check for a warning flag before emitting a warning. We must be slightly careful, though: it might be computationally intensive even to know whether to produce a warning, and it's foolish to work hard only to ignore the result. (Maybe sometimes laziness would save us, but not if any monadic work is involved, of course.) This computationally-intensive case is likely the exception, and so most places that might give rise to a warning can just skip the check entirely, leaving it to the error-message infrastructure above to toss out unwanted warnings.
|
|
|
|
|
|
A good way to approach this is simply to search for any occurrence of `Opt_Warn` in the code and remove extra conditionals.
|
|
|
|
|
|
## Conclusion
|
|
|
|
|
|
I (Richard) recommend design B, because I'm worried about the performance implications of Design A. Though, it might be worthwhile to implement A first (should be very very easy), then remove the conditionals, and then measure performance before going with the more-involved B.
|
|
|
</details>
|
|
|
|
|
|
# Integrating Parser diagnostics into the new infrastructure
|
|
|
|
... | ... | |