2750b75f

By: Tom Sydney Kerckhove <syd@cs-syd.eu>

Ask for an error type rather than a String

HsNoStringError reports a failure written as prose, in the two places prose
gets written. The first is a type that carries one: the first argument of
Either, Except, ExceptT, MonadError or Validation, when that argument is
String, Text, [Char], or a list or NonEmpty of those. The second is a
declaration that names itself an error and is prose underneath, because naming
the prose is the obvious way to answer the first half without changing
anything. The finding underlines the prose rather than the signature or the
declaration around it, since that is the part to change.

A caller can branch on a constructor and cannot branch on prose without taking
the message apart again, so a String error asks every handler to treat every
failure the same way. Writing the words at the point of failure also fixes them
there, which is the place that knows least about who will read them.

Read off the type syntax rather than off the names in it, because the question
is which argument of which type a name is written as: the String in
Either String a is a failure and the one in Either a String is a result. A type
node arrives more than once, since a strict field is a bang around parentheses
around the application, so applications are peeled and deduplicated by
location, and the function half of an application is dropped: the Either String
inside Either String Int is the same String. peelType and peelApp move to
Hopinion.Extract.Ghc, which is where the reading no rule owns lives.

Two of the readings are narrower than they look, and both narrowings are what
make the rule worth having rather than worth turning off.

base, mtl and transformers fix the argument order of four of the carriers, so
matching those names is matching the types we think they are. Nothing fixes
Validation: several packages define it, and one that put its success first
would read identically here. Only a container of prose is read as prose in that
argument, which is a shape a success-first type would not have, and the recall
it costs is recall over code nobody writes, since a Validation that does not
accumulate is an Either with more syllables.

A wrapper counts only when its constructor is the type's name over again, with
or without Mk. That is the shape a reader learns nothing from, and its renderer
is id: renderNarInfoParseError (NarInfoParseError s) = s. A constructor that
names the failure holds what was not understood rather than words about it, and
the words come from the renderer that matches on the name. Reporting every
one-constructor wrapper found seventeen across the repositories below, and
eleven of them were that second shape, written deliberately. Reporting only the
echo finds three.

Tried out over nix-ci8, smos, salsa, kronor, sydtest, validity and
graphql-engine: 1534 findings, no false positives, every distinct flagged line
read apart from the generated clients, and every one of the seventeen candidate
wrappers opened to see what its renderer does. All five carriers fire on real
code, so none of them is carried by the resources alone.

Two things it does not see, both held in good.hs so that they stay decisions.
An Either String that is never written down, where a combinator supplies it and
the lambda handed to bimapCodec, eitherReader or fromPersistValue only writes
Left "...". And a wrapper named better than it is, where the constructor names
the failure and the renderer hands its payload straight back: what decides that
is a module away from the declaration and would be a larger rule than this one.

The one place here is fromPersistValueViaCodec, which is persistent's
fromPersistValue under another name. Its reason moves from the module header
into the suppression, so it is said once.