Better type errors for NamedRoutes without Generic instance

[amesgen]

If you forget to derive Generic for your API types in the context of named routes, you get rather unhelpful error messages. For example, consider

data FooAPI mode = FooAPI
  { foo :: mode :- "foo" :> Post '[JSON] String
  }
  -- deriving stock (Generic)

fooClient :: Client ClientM (NamedRoutes FooAPI)
fooClient = client (Proxy @(NamedRoutes FooAPI))

which yields

Test.hs:20:13: error:
    • Overlapping instances for HasClient
                                  ClientM
                                  (Servant.API.Generic.GToServant (GHC.Generics.Rep (FooAPI AsApi)))
        arising from a use of ‘client’
      Matching instances:
        instance [overlappable] (Servant.Client.Core.RunClient.RunClient m,
                                 (TypeError ...)) =>
                                HasClient m api
          -- Defined in ‘Servant.Client.Core.HasClient’
        instance (HasClient m a, HasClient m b) => HasClient m (a :<|> b)
          -- Defined in ‘Servant.Client.Core.HasClient’
        instance Servant.Client.Core.RunClient.RunClient m =>
                 HasClient m EmptyAPI
          -- Defined in ‘Servant.Client.Core.HasClient’
        ...plus 13 others
        ...plus 19 instances involving out-of-scope types
        (use -fprint-potential-instances to see them all)
      (The choice depends on the result of evaluating ‘GHC.Generics.Rep,
                                                       Servant.API.Generic.GToServant’
       To pick the first instance above, use IncoherentInstances
       when compiling the other instance declarations)
    • In the expression: client (Proxy @(NamedRoutes FooAPI))
      In an equation for ‘fooClient’:
          fooClient = client (Proxy @(NamedRoutes FooAPI))
   |
20 | fooClient = client (Proxy @(NamedRoutes FooAPI))
   |             ^^^^^^

Test.hs:20:13: error:
    • Couldn't match type: Client
                             n
                             (Servant.API.Generic.GToServant (GHC.Generics.Rep (FooAPI AsApi)))
                     with: Servant.API.Generic.GToServant
                             (GHC.Generics.Rep (FooAPI (AsClientT n)))
        arising from a use of ‘client’
    • In the expression: client (Proxy @(NamedRoutes FooAPI))
      In an equation for ‘fooClient’:
          fooClient = client (Proxy @(NamedRoutes FooAPI))
   |
20 | fooClient = client (Proxy @(NamedRoutes FooAPI))
   |             ^^^^^^

for me on GHC 9.2.4. One might spot the Rep type family here from GHC.Generics, but it is certainly not obvious at all, the first message in particular. This can be particularly bad when you have many nested APIS, residing in completely different modules.

In this PR, we use a trick from this blog post: https://blog.csongor.co.uk/report-stuck-families/

With it, the first error message gets much simpler:

Test.hs:20:13: error:
    • Named routes require a Generic instance for FooAPI
    • In the expression: client (Proxy @(NamedRoutes FooAPI))
      In an equation for ‘fooClient’:
          fooClient = client (Proxy @(NamedRoutes FooAPI))
   |
20 | fooClient = client (Proxy @(NamedRoutes FooAPI))
   |             ^^^^^^

Test.hs:20:13: error:
    • Couldn't match type: Client
                             n
                             (Servant.API.Generic.GToServant (GHC.Generics.Rep (FooAPI AsApi)))
                     with: Servant.API.Generic.GToServant
                             (GHC.Generics.Rep (FooAPI (AsClientT n)))
        arising from a use of ‘client’
    • In the expression: client (Proxy @(NamedRoutes FooAPI))
      In an equation for ‘fooClient’:
          fooClient = client (Proxy @(NamedRoutes FooAPI))
   |
20 | fooClient = client (Proxy @(NamedRoutes FooAPI))
   |             ^^^^^^

The error messages for other snippets such as

fooApp :: Application
fooApp = serve (Proxy @(NamedRoutes FooAPI)) FooAPI {foo = pure "foo"}

also improve in the same manner.

I added the error-reporting constraint as a superclass constraint to the various instances of NamedRoutes. There might be further places to apply this pattern, or even a way to also make the second error message better (naively adding ErrorIfNoGeneric to e.g. GenericServant does not work, as then certain call sites of toServant/fromServant then no longer compile).

---

As this approach relies non-trivially on type family evaluation semantics, I can't outrule that this might yield type errors in contexts where one tries to use NamedRoutes with a not-yet-concrete api, but playing around, I couldn't find anything, and a relatively large internal code base using servant and various add-on packages also compiled fine.

[tchoutri]

@amesgen It's a fantastic addition, thank you very much! Tell me if you need help with the doctest CI :)

[amesgen]

Tell me if you need help with the doctest CI :)

Yeah, I think I could use some help there; I can reproduce the CI failure locally, but I am a bit mystified as I don't modify Servant.API.TypeLevel and it does not use named routes 🤔 I will try to tinker with it a bit more, but feel free to e.g. directly push to this branch if you see what is going wrong.

[amesgen]

Maybe it is due to something like sol/doctest#327? Personally, I've been using cabal-docspec ever since I became aware of it as it was much more robust for me.

[tchoutri]

Can't blame you, cabal-docspec really is the best one out there.

[amesgen]

Moved the custom error message to the TypeErrors module; which seems to somehow have fixed the doctest failures.

[gdeest]

@amesgen Thanks for your contribution ! I find it very helpful.

As this approach relies non-trivially on type family evaluation semantics, I can't outrule that this might yield type errors in contexts where one tries to use NamedRoutes with a not-yet-concrete api, but playing around, I couldn't find anything, and a relatively large internal code base using servant and various add-on packages also compiled fine.

So I am guessing this will only be a problem if one tries to call serveWithContext on NamedRoutes routes where routes itself isn't concrete. I don't think that's a common use-case, and adding constraints on Rep (routes ()) into context might be enough to unstuck the type family application and solve the issue. So I think it is safe to merge this PR.