Convert diagnostics in Tc Utils, Solver,Module & co to proper `TcRnMessage`

See also #19905 and #18516.

Short Summary: Get rid of the usages of TcRnUnknownMessage in various modules. Do so by creating new type constructors for the TcRnMessage type, specific to the diagnostics you are converting.

Long summary

There are various occurrences of TcRnUnknownMessage in the following modules:

• GHC.Tc.Deriv (9)
• GHC.Tc.Errors (4)
• GHC.Tc.Instance.Family (2)
• GHC.Tc.Module (14)
• GHC.Tc.Solver.Interact (1)
• GHC.Tc.Solver (1)
• GHC.Tc.Utils.Backpack (5)
• GHC.Tc.Utils.Env (7)
• GHC.Tc.Utils.Instantiate (2)
• GHC.Tc.Utils.TcMType (3)

These occurrences takes a SDoc as input and produces a TcRnMessage, but they carry very little information and structure with them, which means that GHC API users can't meaningfully derive much from the SDoc unless it's parsed or analysed. We would like to replace these with new type constructors to be added to the TcRnMessage type, which would give us a precise (and structured) semantic of the diagnostics in question.

Example

To give you a concrete example of a successfully ported message, we want to go from something like this:

localPatternSynonymErr :: TcRnMessage
localPatternSynonymErr
  = TcRnUnknownMessage $ mkPlainError noHints $
    hang (text "Illegal pattern synonym declaration for" <+> quotes (ppr rdrname))
       2 (text "Pattern synonym declarations are only valid at top level")

to something like this:

localPatternSynonymErr = TcRnIllegalPatSynDecl rdrname

Where TcRnIllegalPatSynDecl is defined as:

  data TcRnMessage =
    ...
    | TcRnIllegalPatSynDecl :: !(LIdP GhcPs) -> TcRnMessage
    ...

Possible plan of action

What follows is a possible plan of action, to guide the developer working on this:

• Add as many type constructors as needed to GHC.Tc.Errors.Types.TcRnMessage. If possible, try to document these following the schema used for the existing ones. It might be possible there is already a type constructor readily available to use, so check for its presence.

• This is a good time to grep the codebase for the text of the diagnostics you want to port, which should hopefully yield a number of test cases. You can later run the testsuite on those tests locally (e.g. ./hadrian/build test --only=...), to verify the final output didn't change. Occasionally the final output might change, for example if new hints are added. Don't be afraid to amend the tests in case that happens, and use your best judgement. The goal is to port existing messages to a new infrastructure, not necessarily improve GHC's diagnostic English prose. That can happen at a later stage.

• Occasionally some instances of TcRnUnknownMessage might be wrapping SDocs which are generated from other SDocs (perhaps even passed as input in a function that produces yet another SDoc). All those little SDoc fragments should be converted into proper Haskell types so that we would be passing to new type constructors not one or more SDocs but richer Haskell types (random examples: Name, OccName, HsBind GhcPs, etc).

• In rare circumstances it might be not feasible to pass a proper Haskell type, maybe due to some nasty cyclic import. If that happens, it's passable (but not ideal) to do something like this:

    ...
    TcRnNewConstructor :: Outputable a => ComplexTypeThatTakesAn a -> ... -> TcRnMessage

• GHC will now complain that the instance Diagnostic TcRnMessage inside GHC.Tc.Errors.Ppr is not complete, if new type constructors were added. The diagnosticMessage, diagnosticReason and diagnosticHints will need to handle the new type constructors.

• Implement diagnosticMessage by copying the original textual representation of the diagnostic message from the ported module. If the textual representation is using external data (i.e. local binds in that module) add the necessary type arguments to the constructor to "carry the data over". Again, follow the existing patterns.

• Implement diagnosticReason by copying the original DiagnosticReason(s) from the original module, where the message was produced originally.

• Implement diagnosticHints by copying, once again, what the original diagnostic in ported module is doing. If there are no hints, return noHints. Be aware that hints might be tucked and hidden into the English prose of the diagnostic you are porting, in which case you might have to create a brand new GhcHint type constructor if none of the existing ones captures the hint.

added error messages label

added Pnormal Ttask newcomer labels

marked this issue as related to #20118 (closed)

marked this issue as related to #20117 (closed)

marked this issue as related to #20116 (closed)

marked this issue as related to #20115 (closed)

marked this issue as related to #20114 (closed)

mentioned in issue #19905

I will temporarily pick this one as it has a big surface area and I have a bit of spare cycles. I can pass the baton or break it down further in case folks would like to take a crack at it.

assigned to @adinapoli

changed the description

marked the checklist item GHC.Tc.Solver (1) as completed

marked the checklist item GHC.Tc.Solver.Interact (1) as completed

mentioned in merge request !6414 (closed)

marked the checklist item GHC.Tc.Utils.Instantiate (2) as completed

marked the checklist item GHC.Tc.Instance.Family (2) as completed

Part one of this ticket was addressed by !6414 (closed), I can press ahead with the rest.

mentioned in merge request !6561 (closed)

Something I have noticed while working on porting the diagnostics in GHC.Tc.Deriv is that we have a function called doDerivInstErrorChecks2 which should report a TcRnPartialTypeSignatures, pretty-printed this way:

    TcRnPartialTypeSignatures _ theta
      -> mkSimpleDecorated $
           text "Found type wildcard" <+> quotes (char '_')
                       <+> text "standing for" <+> quotes (pprTheta theta)

This text crops up in a lot of tests in GHC, but surprisingly enough none of the tests in the testsuite tests this function. In fact, they are all hitting the mkHoleError inside GHC.Tc.Errors, which throws a diagnostic with the same text (i.e. those should just be the same diagnostic type):

...
    hole_msg = case sort of
      ExprHole _ -> vcat [ hang (text "Found hole:")
                            2 (pp_occ_with_type occ hole_ty)
                         , tyvars_msg, expr_hole_hint ]
      TypeHole -> vcat [ hang (text "Found type wildcard" <+> quotes (ppr occ))
                            2 (text "standing for" <+> quotes pp_hole_type_with_kind)
                       , tyvars_msg, type_hole_hint ]
      ConstraintHole -> vcat [ hang (text "Found extra-constraints wildcard standing for")
                                  2 (quotes $ pprType hole_ty)  -- always kind constraint
                             , tyvars_msg, type_hole_hint ]
...

This is a bit unsatisfactory, so we should:

1. Make sure that eventually GHC.Tc.Errors reuses TcRnPartialTypeSignatures;
2. Make sure that doDerivInstErrorChecks2 is actually tested.

I'm responsible for the code in GHC.Tc.Deriv. At the time I was writing it (which was before the current error message diagnostics), I remember trying to consolidate the two error messages' strings, but I must have abandoned the idea for one reason or the other. My hope is that the current diagnostics will make this consolidation easier.

If you need a test case, it would suffice to run the code in T13324_compile with -Wpartial-type-signatures enabled.

I'm responsible for the code in GHC.Tc.Deriv. At the time I was writing it (which was before the current error message diagnostics), I remember trying to consolidate the two error messages' strings, but I must have abandoned the idea for one reason or the other. My hope is that the current diagnostics will make this consolidation easier.

Hey Ryan, thanks for chiming in and I sympathise completely -- the GHC.Tc.Errors module is a bit of a weird beast (due to the deferred errors) so I can totally see why it wasn't easy or feasible at the time to unify the two. Hopefully now we are in a better shape and such unification will be possible, eventually

If you need a test case, it would suffice to run the code in T13324_compile with -Wpartial-type-signatures enabled

Ah, thank you, that's a useful pointer! I was already running T13324_compile in my batch of tests, but it didn't occur to me that in the test driver we were running it with -Wno-partial-type-signatures. Perhaps it's worth somehow having a slightly different flavour of it that runs with -Wpartial-type-signatures, as you suggest? That seems universally useful to me, as it tests exactly the logic where we recommend "Use PartialTypeSignatures" vs the "no hints" case.

Perhaps it's worth somehow having a slightly different flavour of it that runs with -Wpartial-type-signatures, as you suggest?

If the GHC test suite is amenable to that, that sounds fine to me. But it's a pretty small test case, so copy-pasting the code would work just as well if need be.

marked the checklist item GHC.Tc.Deriv (9) as completed