Always create diagnostic messages with proper Severity, do not reclassify after birth
Another stepping stone for #18516.
Currently this MR was opened against my other branch wip/adinapoli-message-class-new-design
, for ease of review. It's marked as WIP
because there is still that annoying reclassify
sitting in maybeReportError
inside GHC.Tc.Errors
, but the rest is gone.
Technically, after we fix that, we should be able to always have diagnostic messages in GHC be created with the right Severity
and DiagnosticReason
computed at birth based on the current value of the DynFlags
, without potentially buggy re-classifications a posteriori.
This patch implements what in the Wiki is called "Eventual Design". In particular:
-
defaultReasonSeverity
is gone, replaced bydiagReasonSeverity
that takesDynFlags
as input in order to compute the rightSeverity
at "birth" of a new message; - Drastically simplifies
printOrThrowWarnings
, and also removes the (re)classification in there; - Removes the need for
reclassify
insideGHC.Tc.Errors
. This required a bit of refactoring and to generalise a bunch of functions to be parametric in theDiagnosticReason
, so that we could pass either the "original" reason or the one computed inside thecec_defer_type_errors
.
This patch is essentially ready to go, but there is a disputable design question. In order for the testsuite to fully pass, I had to give isErrorMessage
the following definition:
isErrorMessage :: Diagnostic e => MsgEnvelope e -> Bool
isErrorMessage = (==) ErrorWithoutFlag . diagnosticReason . errMsgDiagnostic
While this is reasonable, it's making the decision based on the DiagnosticReason
rather than on the Severity
: the difference is subtle, but it's there. In particular, if I change this definition to use the Severity
, I get a few dozens of tests failures. I think I know why this happens: this function is used via the errorsFound
function in a bunch of places, namely in the typechecker and in the desugarer. In those two components, we want to check "is there any real error, should I just bail out or continue?" and therefore we do want to check for the DiagnosticReason
, because otherwise we might consider errors even (promoted) warnings, but that's not the right time to bail out: rather we want still do collect all the possible warnings and only later fail.
In fact, truth to be told, that's also why printOrThrowWarnings
makes the decision "is this an error or a warning?" based on the Severity
, as at that point that's the right time to do so.
I think this part of the design is still a bit iffy: not terrible, but not clean as it should be. I think a better solution would be to have two different flavours of isErrorMessage
: one that does equality checking on the DiagnosticReason
we keep using in errorsFound
, and another we use if we really need to partition warnings and messages for displaying, say, from the driver upwards.
Another option would be to implement isErrorMessage
"the right way" but give a definition of errorsFound
that uses the DiagnosticReason
.