fizruk / http-api-data

Converting to/from HTTP API data like URL pieces, headers and query parameters.
http://hackage.haskell.org/package/http-api-data
Other
52 stars 42 forks source link

Preventing the anyclass deriving of ToHttpApiData and FromHttpApiData #124

Open Kleidukos opened 1 year ago

Kleidukos commented 1 year ago

Hi @fizruk! I'm a long-time Servant user (and thus of http-api-data), and I'd like to suggest a solution to prevent the anyclass deriving strategy of ToHttpApiData and FromHttpData via a solution that @mangoIV came up with:

{-# LANGUAGE DataKinds #-}
{-# LANGUAGE DefaultSignatures #-}
{-# LANGUAGE DeriveAnyClass #-}
{-# LANGUAGE DerivingStrategies #-}
{-# LANGUAGE StandaloneKindSignatures #-}
{-# LANGUAGE TypeFamilies #-}
{-# LANGUAGE TypeOperators #-}
{-# LANGUAGE UndecidableInstances #-}
{-# OPTIONS_GHC -Wno-unrecognised-pragmas, -Wall #-}

module PreventAnyClassDeriving (A (..), C (..), C' (..), C'' (..), MkTypeError) where

import Data.Kind (Type)
import GHC.TypeLits (ErrorMessage (ShowType, Text, (:<>:)), TypeError)

type MkTypeError :: k -> Type
type family MkTypeError c where
  MkTypeError c =
    TypeError ('Text "Don't use anyclass deriving with class " ':<>: 'ShowType c)

type PreventAnyClassDeriving c = MkTypeError c ~ ()

class C a where
  f :: a -> Int
  default f :: PreventAnyClassDeriving C => a -> Int
  f = error "someone removed the PreventAnyClassDeriving constraint"

class C' where
  f' :: Int
  default f' :: PreventAnyClassDeriving C' => Int
  f' = error "someone removed the PreventAnyClassDeriving constraint"

class C'' a b where
  f'' :: a -> b
  default f'' :: PreventAnyClassDeriving C'' => a -> b
  f'' = error "someone removed the PreventAnyClassDeriving constraint"

data A = MkA
-- deriving anyclass C
-- deriving anyclass instance C'' Int Int
-- will throw compile time error "Don't use anyclass deriving with class C"

What do you think of this?

phadej commented 1 year ago

Why do things in so complicated way when we can (at least advocate for) using -Werror=missing-methods https://github.com/ghc-proposals/ghc-proposals/issues/544

And as far as I see, the strategy you propose won't work for http-api-data which has circular definitions of members (and that's the reason anyclass deriving works). i.e. there are defaults in place.

Kleidukos commented 1 year ago

Why do things in so complicated way when we can (at least advocate for) using -Werror=missing-methods ghc-proposals/ghc-proposals#544

  1. Because I didn't know it existed.
  2. Because the mechanism allows for customising the error message (but this is not relevant in this particular context because as you pointed out, circular definition)
MangoIV commented 1 year ago

I’m very much for advocating your ghc-proposal but how is it relevant as a solution for present problems; the proposal is neither accepted nor implemented?

This is very much about improving the API not about being able to give a recommendation in the documentation.

phadej commented 1 year ago

This is very much about improving the API

As said, the approach proposed won't work (prove me wrong) as all methods of From/ToHttpApiData already have default definitions, and changing that is a big change.

I assume the bad thing about DeriveAnyClass is that it succeeds for FromHttpApiData as the class is circular, so is "trivially" implemented for any type. But that is exactly what I discuss in the ghc-proposals issue. That's bad.

{-# LANGUAGE DeriveAnyClass #-}

class FooBar a where
    foo :: a -> a
    foo = bar

    bar :: a -> a
    bar = foo

    {-# MINIMAL foo | bar #-}

data U = U deriving FooBar

should error. Luckily it at least warns today (even without -Wall):

Circular.hs:15:21: warning: [-Wmissing-methods]
    • No explicit implementation for
        either ‘foo’ or ‘bar’
    • In the instance declaration for ‘FooBar U’
   |
15 | data U = U deriving FooBar
   |                     ^^^^^^

I like to fix the cause of issues, not to band-aid them all over the place. Making -Wmissing-methods being an error in GHC is one potential fix to this (and other) issues. And in fact, that train is slowly moving, but could move faster if people weight in.


Because the mechanism allows for customising the error message (

I don't see that to be valuable at all. GHC is definitely capable of saying that "you forgot to define methods". And that is the best you can do, as mentioning DeriveAnyClass is incorrect.

Modifying the example in issue description, note no DerivingAnyClass:

{-# LANGUAGE DataKinds #-}
{-# LANGUAGE DefaultSignatures #-}
{-# LANGUAGE StandaloneKindSignatures #-}
{-# LANGUAGE TypeFamilies #-}
{-# LANGUAGE TypeOperators #-}
{-# LANGUAGE UndecidableInstances #-}
{-# OPTIONS_GHC -Wno-unrecognised-pragmas, -Wall #-}

module PreventAnyClassDeriving (A (..), C (..), C' (..), C'' (..), MkTypeError) where

import Data.Kind (Type)
import GHC.TypeLits (ErrorMessage (ShowType, Text, (:<>:)), TypeError)

type MkTypeError :: k -> Type
type family MkTypeError c where
  MkTypeError c =
    TypeError ('Text "Don't use anyclass deriving with class " ':<>: 'ShowType c)

type PreventAnyClassDeriving c = MkTypeError c ~ ()

class C a where
  f :: a -> Int
  default f :: PreventAnyClassDeriving C => a -> Int
  f = error "someone removed the PreventAnyClassDeriving constraint"

class C' where
  f' :: Int
  default f' :: PreventAnyClassDeriving C' => Int
  f' = error "someone removed the PreventAnyClassDeriving constraint"

class C'' a b where
  f'' :: a -> b
  default f'' :: PreventAnyClassDeriving C'' => a -> b
  f'' = error "someone removed the PreventAnyClassDeriving constraint"

data A = MkA

-- "ordinary" empty instance declaration
instance C A

still errors with Don't use anyclass deriving with class C which is incorrect.