GHC 9.8: Warnings about partial head and tail (-Wx-partial)

[andreasabel]

GHC 9.8 (alpha 1-3) reports new warnings with -Wall:

src/Lens/Micro/TH/Internal.hs:88:43: warning: [GHC-63394] [-Wx-partial]
    In the use of ‘head’
    (imported from Prelude, but defined in GHC.List):
    "This is a partial function, it throws an error on empty lists. Use pattern matching or Data.List.uncons instead. Consider refactoring to use Data.List.NonEmpty."
   |
88 |     (\n -> GadtC [n] argTys retTy) <$> f (head ns)
   |                                           ^^^^

src/Lens/Micro/TH/Internal.hs:90:46: warning: [GHC-63394] [-Wx-partial]
    In the use of ‘head’
    (imported from Prelude, but defined in GHC.List):
    "This is a partial function, it throws an error on empty lists. Use pattern matching or Data.List.uncons instead. Consider refactoring to use Data.List.NonEmpty."
   |
90 |     (\n -> RecGadtC [n] argTys retTy) <$> f (head ns)
   |                                              ^^^^

These are not fixable because template-haskell does not use the non-empty list type in the definition of [Rec]GadtC.

There are two more warnings which looks that they could be fixed easily:

src/Lens/Micro/TH.hs:1142:47: warning: [GHC-63394] [-Wx-partial]
    In the use of ‘head’
    (imported from Prelude, but defined in GHC.List):
    "This is a partial function, it throws an error on empty lists. Use pattern matching or Data.List.uncons instead. Consider refactoring to use Data.List.NonEmpty."
     |
1142 |                        (appE (varE 'phantom) (head fxs))
     |                                               ^^^^

src/Lens/Micro/TH.hs:1143:25: warning: [GHC-63394] [-Wx-partial]
    In the use of ‘tail’
    (imported from Prelude, but defined in GHC.List):
    "This is a partial function, it throws an error on empty lists. Replace it with drop 1, or use pattern matching or Data.List.uncons instead. Consider refactoring to use Data.List.NonEmpty."
     |
1143 |                        (tail fxs)
     |                         ^^^^

[nomeata]

Can you use {-# OPTIONS_GHC -Wno-x-partial #-} in these files to assert “I acknowledge these warnings, no need to show them to my users?”

[andreasabel]

Sure. But I'd like standard warnings (-Wall) to be those which are fixable rather than those that I have to switch off. (Pertaining to the discussion at https://gitlab.haskell.org/ghc/ghc/-/issues/23934.)

If I chose to opt into -Wx-partial, I would have to switch it off for Internal.hs.

It would be better if I could switch a warning off on a finer level, e.g. just for one definition rather than for the whole file. (In this case, though, I wouldn't need that fine control.)

[nomeata]

Its a warning, not an error, so I think it's expected that in some cases the “fix” is telling the compiler they you know what you are doing (possibly by turning off the warning).

Finer grained control would be nice indeed, for now we have it at the module level.

[parsonsmatt]

Sure. But I'd like standard warnings (-Wall) to be those which are fixable rather than those that I have to switch off.

Looking at the code in question:

-- | On @template-haskell-2.11.0.0@ or later, if a 'GadtC' or 'RecGadtC' has
-- multiple 'Name's, the leftmost 'Name' will be chosen.
instance HasName Con where
  name f (NormalC n tys)       = (`NormalC` tys) <$> f n
  name f (RecC n tys)          = (`RecC` tys) <$> f n
  name f (InfixC l n r)        = (\n' -> InfixC l n' r) <$> f n
  name f (ForallC bds ctx con) = ForallC bds ctx <$> name f con
#if MIN_VERSION_template_haskell(2,11,0)
  name f (GadtC ns argTys retTy) =
    (\n -> GadtC [n] argTys retTy) <$> f (head ns)
  name f (RecGadtC ns argTys retTy) =
    (\n -> RecGadtC [n] argTys retTy) <$> f (head ns)
#endif

I suspect that this is a bug - the form of having multiple names would be:

data GadtType where
  GadtCon1, GadtCon2 :: Int -> CHar -> GadtType

But this would collapse that into a single constructor.

I do think you're guaranteed to have at least one Name, so head ns is safe. You can nevertheless provide a better error message to end users with:

case ns of
  [] -> error "Provided an empty list of names in a GadtC, should never happen"
  (n:_) -> n

-- or,
fromMaybe (error "Provided an empty list of names in a GadtC") $ listToMaybe ns

These are fixes, since error doesn't have the partiality warning.

[andreasabel]

@parsonsmatt wrote:

I suspect that this is a bug - the form of having multiple names would be:

data GadtType where
  GadtCon1, GadtCon2 :: Int -> CHar -> GadtType

But this would collapse that into a single constructor.

Thanks @parsonsmatt, for this alert!, this should be investigated. Ping @stevenfontanella.

I do think you're guaranteed to have at least one Name, so head ns is safe.

Yes it is safe here.

You can nevertheless provide a better error message to end users with:
...
These are fixes, since error doesn't have the partiality warning.

One should only use head where it is safe to use it.
Of course, it is better if we can prove this safety by using non-empty lists.
The x-partial warnings can be understood as a nudge to refactor code to use non-empty lists where possible.
However, here it is not possible because it would be upstream's job to do this first.
I am not sure that replacing head by error gains us much (except for maybe getting a more precise information where the invariant failed, but I thought head can now deliver this through the call stack, can't it?).
Isn't it even going in the wrong direction, by removing this nudge?

[parsonsmatt]

I think the larger issue is that HasName Con => Lens' Con Name is just wrong. I think you need a separate class HasNames which offers a Traversal instead of a Lens.

(except for maybe getting a more precise information where the invariant failed, but I thought head can now deliver this through the call stack, can't it?).

It looks like head in GHC 9.2 and prior did not include callstack information. But GHC 9.4 and above will render a callstack.

The x-partial warnings can be understood as a nudge to refactor code to use non-empty lists where possible.

That is one understanding. There are many ways to avoid using partial functions, and I gave a few other alternatives.

In virtually all cases, I think writing fromMaybe (error "my specific and cool error message") . listToMaybe is better than writing head. *** Exception: Prelude.head: empty list is almost totally useless, and knowing where head is called is only slightly better - you'd be pointed to this definition in microlens, and since name lacks a HasCallStack constraint, you wouldn't know what actually called the function that caused the problem without doing some digging.

But that's getting into the weeds on this specific case, where the real bug fix is to delete the instance, or warn on the instance, or introduce class HasNames con where names :: Traversal' con Name.