... | ... | @@ -245,4 +245,134 @@ As this strand of work touches a lot of modules, doing everything as a single ga |
|
|
architecture;
|
|
|
|
|
|
- [ ] Convert the `Driver` error types to adhere to the new error-messages
|
|
|
architecture. |
|
|
\ No newline at end of file |
|
|
architecture.
|
|
|
|
|
|
# Alternative Proposal
|
|
|
|
|
|
The "New API" design above suffers from an infelicity: duplication. Imagine we have
|
|
|
|
|
|
```hs
|
|
|
data TcRnMessage = TcRnOutOfScope ... | TcRnBadTelescope ... | ...
|
|
|
```
|
|
|
|
|
|
and we have a `MsgEnvelope TcRnMessage`. Such a structure effectively contains two "reasons" for the error: one encoded in the choice of constructor of `TcRnMessage`, and one in the payload of the `errMsgSeverity` field. This alternative is meant to address this problem.
|
|
|
|
|
|
Key points to consider:
|
|
|
* Every warning or error arises for a *reason*.
|
|
|
* Most warnings are controlled by specific `-Wblah-blah` flags; we call these *reasons* for the warning.
|
|
|
* Some warnings are not associated with a flag. We currently say these have "no reason" for arising, but really, we mean that there is no flag.
|
|
|
* Errors are not currently controlled by flags. But this is a small lie, as a flag like `-fdefer-type-errors` really is meant to change an error into a warning. By making a *reason* for the error that connects it to a flag, we might imagine controlling errors by flags, just like we do warnings. (Some errors will always be fatal.)
|
|
|
* Separate from a reason, each diagnostic has a *severity*: will reporting the diagnostic always halt the compiler, or can we continue?
|
|
|
* Relating a reason with a severity requires the use of the `DynFlags`, because the `DynFlags` stores the presence/absence of the `-Wblah-blah` flags.
|
|
|
* Not just any `DynFlags` will do when converting a *reason* to a *severity*: we must use the `DynFlags` in action when the diagnostic was generated. This is important because, for example, `deriving` code might give rise to a warning that we wish to suppress, even with `-Werror` enabled. This is accomplished by locally changing the `DynFlags` while processing code generated by `deriving`. If we use the outer, top-level `DynFlags` to get a severity from a reason, we'll get this wrong. Conclusion: the severity must be chosen when the error message is first created or soon thereafter. (Current design: soon thereafter.)
|
|
|
* When we have informative datatypes describing errors (like `TcRnMessage`), each constructor will be associated with precisely one reason.
|
|
|
* Not all messages are diagnostics, but non-diagnostic error messages do not have associated reasons or flags.
|
|
|
* Messages sometimes have bullet points in them.
|
|
|
|
|
|
A lesser (but non-negligible) concern:
|
|
|
* If we classify severity at the birth of an error message, we get error message regressions with `-Werror`: GHC on occasion checks if any error messages have been emitted. If `-Werror` warnings have already been classified as fatal diagnostics, than the check for error messages will return `True`, prematurely aborting compilation. Really, we want functions like `checkIfErrs` to look for always-fatal messages, not just happens-to-be-fatal messages, but perhaps that would be easier in a separate patch.
|
|
|
|
|
|
When we consider a `MsgEnvelope`, then, we do not want the envelope to describe the reason for the message, as doing so would be redundant: the payload of a `MsgEnvelope` (something that might be a `TcRnMessage`) implicitly stores a reason. It also must be possible to extract a reason and severity from a `MsgEnvelope` in order to correctly format the error message; this must be done without consulting the `DynFlags` (which may be the wrong `DynFlags` for severity-classification, if they have changed).
|
|
|
|
|
|
Putting this all together, this situation suggests the following temporary design:
|
|
|
|
|
|
```hs
|
|
|
data Severity = SevWarning | SevError
|
|
|
data DiagReason = WarningWithoutFlag | WarningWithFlag !WarningFlag | ErrorWithoutFlag
|
|
|
|
|
|
-- | Computes a severity from a reason in the absence of DynFlags. This will likely
|
|
|
-- be wrong in the presence of -Werror.
|
|
|
defaultReasonSeverity :: DiagReason -> Severity
|
|
|
defaultReasonSeverity WarningWithoutFlag = SevWarning
|
|
|
defaultReasonSeverity (WarningWithFlag {}) = SevWarning
|
|
|
defaultReasonSeverity ErrorWithoutFlag = SevError
|
|
|
|
|
|
data MsgEnvelope a = MsgEnvelope -- assume `Diagnostic a` holds
|
|
|
{ errMsgSpan :: SrcSpan
|
|
|
, errMsgContext :: PrintUnqualified
|
|
|
, errMsgDiagnostic :: a
|
|
|
, errMsgSeverity :: Severity
|
|
|
} deriving Functor
|
|
|
|
|
|
data MessageClass
|
|
|
= MCOutput
|
|
|
| ... -- several other non-diagnostic message classifications
|
|
|
| MCDiagnostic Severity DiagReason
|
|
|
|
|
|
-- | A 'MessageClass' for always-fatal errors.
|
|
|
mcDiagnosticError :: MessageClass
|
|
|
mcDiagnosticError = MCDiagnostic SevError ErrorWithoutFlag
|
|
|
|
|
|
-- | Make a 'MessageClass' for a given 'DiagReason', without consulting the 'DynFlags'.
|
|
|
-- This will not respect -Werror or warning suppression and so is probably wrong
|
|
|
-- for any warning.
|
|
|
mkMCDiagnostic :: DiagReason -> MessageClass
|
|
|
mkMCDiagnostic reason = MCDiagnostic (defaultReasonSeverity reason) reason
|
|
|
|
|
|
-- Text organized into bullets.
|
|
|
newtype DecoratedSDoc = Decorated { unDecorated :: [SDoc] }
|
|
|
|
|
|
data DiagnosticMessage = DiagnosticMessage
|
|
|
{ diagMessage :: !DecoratedSDoc
|
|
|
, diagReason :: !DiagReason
|
|
|
}
|
|
|
|
|
|
type WarnMsg = MsgEnvelope DiagnosticMessage -- INVARIANT: errMsgSeverity == SevWarning
|
|
|
type ErrMsg = MsgEnvelope DiagnosticMessage -- INVARIANT: errMsgSeverity == SevError
|
|
|
|
|
|
class Diagnostic a where
|
|
|
diagnosticMessage :: a -> DecoratedSDoc
|
|
|
diagnosticReason :: a -> DiagReason
|
|
|
|
|
|
instance Diagnostic DiagnosticMessage where
|
|
|
diagnosticMessage = diagMessage
|
|
|
diagnosticReason = diagReason
|
|
|
|
|
|
mkMsgEnvelope :: Diagnostic err => SrcSpan -> PrintUnqualified -> err -> MsgEnvelope err
|
|
|
mkMsgEnvelope locn print_unqual err
|
|
|
= MsgEnvelope { errMsgSpan = locn
|
|
|
, errMsgContext = print_unqual
|
|
|
, errMsgDiagnostic = err
|
|
|
, errMsgSeverity = defaultReasonSeverity (diagnosticReason err) -- wrong, but will be fixed in printOrThrowWarnings
|
|
|
}
|
|
|
|
|
|
-- in the type-checker
|
|
|
data TcRnMessage
|
|
|
= TcRnUnknownMessage DiagnosticMessage
|
|
|
| TcRnNameShadowing ...
|
|
|
| TcRnBadTelescope ...
|
|
|
| ...
|
|
|
|
|
|
pprTcRnMessage :: TcRnMessage -> DecoratedSDoc
|
|
|
pprTcRnMessage = ...
|
|
|
|
|
|
instance Diagnostic TcRnMessage where
|
|
|
diagnosticMessage = pprTcRnMessage
|
|
|
diagnosticReason = \case
|
|
|
TcRnUnknownMessage msg -> diagReason msg
|
|
|
TcRnNameShadowing {} -> WarningWithFlag Opt_WNameShadowing
|
|
|
TcRnBadTelescope {} -> ErrorWithoutFlag
|
|
|
...
|
|
|
|
|
|
-- in GHC.Driver.Errors
|
|
|
printOrThrowWarnings = ...
|
|
|
where
|
|
|
is_warn_msg_fatal :: Diagnostic err => DynFlags -> WarnMsg -> Bool
|
|
|
is_warn_msg_fatal dflags (MsgEnvelope { errMsgSeverity = SevError }) = panic "but this should be a warning"
|
|
|
is_warn_msg_fatal dflags (MsgEnvelope { errMsgDiagnostic = d }) = case diagReason d of
|
|
|
WarningWithoutFlag -> gopt Opt_WarnIsError dflags
|
|
|
WarningWithFlag flag -> wopt_fatal flag dflags
|
|
|
ErrorWithoutFlag -> panic "error classed as warning"
|
|
|
|
|
|
promote_warning_as_error :: WarnMsg -> ErrMsg
|
|
|
promote_warning_as_error msg = msg { errMsgSeverity = SevError }
|
|
|
```
|
|
|
|
|
|
A few details to highlight:
|
|
|
* `DiagReason` and `Severity` are independent. A `DiagReason` is a fundamental property of a message, while `Severity` also takes `DynFlags` into account.
|
|
|
* While a `Severity` is chosen at birth, it can be modified by `printOrThrowWarnings`. This is the temporary part of the design; ideally, we would get the severity correct at birth. In this "at birth" design, `mkMsgEnvelope` would also take a `DynFlags`.
|
|
|
* There is a representable impossible state: `MCDiagnostic SevWarning ErrorWithoutFlag`. This one seems hard to get rid of without resorting to fancy types. (Really, we want the severity to be greater than or equal to the default severity.)
|
|
|
* There is no duplication between the `MsgEnvelope` and its payload.
|
|
|
* `MCDiagnostic` carries enough information to be used in much the same way it is used in !5024.
|
|
|
* 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. |
|
|
\ No newline at end of file |