Name an annotation pragma after what it annotates
## What was wrong
GHC parses `{-# ANN foo ... #-}` as a declaration rather than handing it over as
a comment, so it is the next declaration a comment above it can attach to. That
declaration fell through `declFactsOf`'s catch-all and was named
`"declaration"`, so a Haddock above an annotation pragma came out attached to a
name nothing else in the module answers to:
$ hopinion module nix-ci-cache/src/NixCI/Cache/NarInfo.hs --dump-comments
...NarInfo.hs:203:1: StyleHaddockNext
AttachedToDecl (DeclName {declNameText = "declaration"})
"Interpret lexed narinfo fields into a 'NarInfo'. ..."
The source there being:
-- | Interpret lexed narinfo fields into a 'NarInfo'. ...
{-# ANN narInfoFromFields ("DisableMutations: ..." :: String) #-}
narInfoFromFields :: Map ByteString (NonEmpty ByteString) -> Either NarInfoParseError NarInfo
A comment's scope is what a suppression is matched against, so this is not
cosmetic. In that position:
- an `[allow:...]` matches no finding, because the finding it means to suppress
is scoped to the real declaration;
- a `[tag:]` or `[ref:]` connects nothing, for the same reason;
- any rule asking what a comment is about gets an answer with no names in it.
## What the fix does
`annotationDeclName` in `Hopinion.Extract` names the pragma's fact after
whatever the pragma annotates, which is what it is about and what a comment
written above it is about:
AnnD _ (HsAnnotation _ provenance _) -> one (annotationDeclName provenance) DeclOther
A value or type annotation takes the annotated name. A module annotation has no
name to take and is the one spelling left describing itself.
The alternative was to teach the attachment walk in `Hopinion.Comment` to step
over pragma declarations the way it steps over comment-only lines. That needs
`CommentContext` to carry which lines are pragmas, and it would still leave the
fact claiming a declaration called `declaration` for everything else that reads
`moduleContextDecls`. Correcting the fact fixes attachment as a consequence
rather than working around it.
The kind stays `DeclOther` deliberately: an annotation is not a value, a
signature or a type, and any rule counting those should keep not seeing it.
## Tests
Two resources in `test_resources/Comment/`, which is one file and one golden
each and no test code:
- `27-above-an-ann-pragma.hs` — the reported case. Its golden was written by
hand with the right answer first, and it failed against `master`
(`declaration` where `resolveTarget` was expected) before the fix went in.
- `28-above-a-module-ann-pragma.hs` — pins the `ModuleAnnProvenance` branch,
which is the one that keeps a description of itself.
No existing golden changed, in `test_resources/Comment/` or anywhere else, so
nothing else was relying on the old answer.
## Worth checking by hand
- **`"module annotation"` is still a name nothing answers to.** A comment above
`{-# ANN module ... #-}` remains in a scope holding no names, so the same
three consequences apply to it. It is arguably `AttachedToFile`, but that
needs the attachment walk to treat a module annotation differently from a
declaration, which is the bigger change this deliberately is not. Rare enough
that I left it, and pinned it in a golden so the next person sees it is a
decision rather than an oversight.
- **The other catch-all names are unchanged**: `"signature"`, `"instance"`,
`"foreign"`, `"pattern binding"`, `"variable binding"`, `"data family
instance"`, `"type family instance"` and the remaining `"declaration"`. Each
has the same shape of problem if a comment can sit above it. I did not go
through them, because I have a reported case for the annotation pragma and
none for the rest, and guessing at the right name for a standalone deriving
clause without one seemed worse than leaving it.
- The two real sites, if you want to see it on live code:
`nix-ci-cache/src/NixCI/Cache/NarInfo.hs:203` and
`nix-ci-common/src/NixCI/Common/PostgresConnectInfo.hs:95` in nix-ci8, both of
which now attach to the function below the pragma.
## Notes
- Branched off `master`, since there is no `development` branch here.
- `hopinion check` run in-process still reports one `UNUSED_SUPPRESSION` for
`[allow:TestJsonSpecPerJsonType]` in `Hopinion/Check/Obligation.hs`. It does
the same on `master`, and `nix flake check` is green, so it is an artefact of
running without `.hie` files rather than anything this branch did.