Skip to content

Always create diagnostic messages with proper Severity, do not reclassify after birth

Alfredo Di Napoli requested to merge wip/adinapoli-diag-reason-severity into master

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 by diagReasonSeverity that takes DynFlags as input in order to compute the right Severity at "birth" of a new message;
  • Drastically simplifies printOrThrowWarnings, and also removes the (re)classification in there;
  • Removes the need for reclassify inside GHC.Tc.Errors. This required a bit of refactoring and to generalise a bunch of functions to be parametric in the DiagnosticReason, so that we could pass either the "original" reason or the one computed inside the cec_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.

Edited by Alfredo Di Napoli

Merge request reports