aceab7af

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.

Total

11s

Created 1 month ago Started 1 month ago Finished 1 month ago

Timeline