Generalize GConName

[edsko]

@stevana , first of all, let me apologize for submitting another PR so soon after your release. As I am continuing to work on my example and getting very detailed about shrinking, I realized that there is another problem with the lib that makes it impossible for me to apply some of the techniques I've been using. The good news is that this is a minor change only; the bad news is that technically speaking it is an API breaking change :/

Many top-level functions had constraints such as

Generic1 cmd, GConName1 (Rep1 cmd)

This is however problematic when using commands for which Generic1 instances are difficult are impossible to derive or define. It is also somewhat unidiomatic Haskell: typically we don't require Generic1 (or Generic), and then some function on the generic representation; instead we introduce a special type class for what we want, and then provide default implementations that use generics. In this patch I do precisely this. It actually doesn't change very much: the GConName1 class changes to

class GConName1 f where
  gconName1  :: f a -> String
  gconNames1 :: Proxy (f a) -> [String]

  default gconName1 :: (Generic1 f, GConName1 (Rep1 f)) => f a -> String
  gconName1 = gconName1 . from1

  default gconNames1 :: forall a. GConName1 (Rep1 f) => Proxy (f a) -> [String]
  gconNames1 _ = gconNames1 (Proxy @(Rep1 f a))

Notice how the translation to the generic representation is now done in the class definition rather than in call sites; those call sites therefore change to have constraints

GConName1 cmd

instead, and don't need to worry about the translation to the generic representation anymore. Example code does not become any more difficult, we just need to define one more class, any DeriveAnyClass does the job; just as one example, here is the one from the ticket dispenser example:

data Action (r :: * -> *)
  = TakeTicket
  | Reset
  deriving (Show, Generic1, Rank2.Functor, Rank2.Foldable, Rank2.Traversable, GConName1)

[edsko]

Rebased with --signoff.

[stevana]

Thanks for making things more idiomatic!

According to Travis your change doesn't build with GHC 8.2.2 (lts-11) however. Is there a simple fix for this? (I don't care about supporting old versions of GHC unless somebody explicitly asks for it, but we shouldn't break things if there's a simple fix).

[edsko]

Let me take a look.

[edsko]

@stevana I wanted to keep the changes to a minimum, so I didn't rename anything. However, it might be worth renaming it CommandNames with methods commandName and commandNames, and add some documentation? Might make it a bit more obvious what it's supposed to do. If you think that would be good, I'll include that in my fix for 8.2. But I can also just leave it as is, whichever you prefer.

[stevana]

Feel free to document and make things more clear (and sorry for not having done a better job in the first place)!

One possible alternative which I've considered is to use Data.Data.toConstr and scrap GConName, but I don't know if you can get all constructors of a datatype given its Data instance? Perhaps you have some thoughts on this?

[stevana]

Just found Data.Data.dataTypeConstrs!

[edsko]

So, concretely, I was suggesting to rename GConName1 to

-- | The names of all possible commands
--
-- This is used for things like tagging, coverage checking, etc.
class CommandNames cmd where
  -- | Name of this particular command
  cmdName  :: cmd r -> String

  -- | Name of all possible commands
  cmdNames :: Proxy (cmd r) -> [String]

Then GConName (without the "1") can go altogether, since it has exactly one instance, and is used for Command only.

[stevana]

Looks good to me.

[edsko]

As for Data.Data.dataTypeConstrs, my vote is still not to depend on any specific way to get these names. My specific use case being a good case in point. My actual command type looks something like

data Cmd fp h = ....

where fp and h are parameters of kind *, nothing to do with the r parameter that quickcheck-state-machine expects. This is a more abstract type, which has some important benefits: in particular, this can be interpreted also over the model (I'll explain this in detail in my blog post). Then I wrap this at the end in a form that suits quickcheck-state-machine:

newtype At t r = At (t (PathRef r) (HandleRef r))
  deriving (Generic)

type (:@) t r = At t r

so for instance we might deal with Cmd :@ Symbolic. But, of course, I don't want to have the constructor names of At, but rather the constructor names of Cmd! Having the CommandNames class lets me do such things, and then pick my own way how I construct my names; having the generic default means that it's not really more difficult for people who don't need that additional flexibility, but it's there when needed.

[edsko]

(I personally use generics-sop to get the names of Cmd, where it's a one-liner.. but of course, being one of the authors of that lib, I may be a tad biased :grin: )

[stevana]

Ah ok, yeah that makes sense.

[stevana]

If you feel things can be cleaned up with generics-sop or other libraries, feel free to do so.

(In PR #242 we introduce a dependency on generic-data (because we need a generic bounded and enum instance). I've still not had a look at if generic-data could simplify things...)

[edsko]

@stevana I fixed 8.2 (thanks to Thomas for finding the solution there) and I did the renaming.

[edsko]

If you feel things can be cleaned up with generics-sop or other libraries, feel free to do so.

Well, it's up to you. The generic implementation in ConstructorName.hs (should I rename that module too? :thinking: ) would be much cleaner with generics-sop; in particular, the nasty edge case

instance Constructor c => CommandNames (M1 C c f) where
  cmdName                            = conName
  cmdNames (_ :: Proxy (M1 C c f p)) = [ conName @c undefined ] -- Can we do
                                                                  -- better
                                                                  -- here?

would not be needed. However, it introduces another dependency and requires people to derive the SOP generics instance for their datatypes.

On the other hand, generic-sop also can be used to define generic bounded and enum instances, so it might unify some stuff. Up to you.

[stevana]

However, it introduces another dependency and requires people to derive the SOP generics instance for their datatypes.

On the other hand, generic-sop also can be used to define generic bounded and enum instances, so it might unify some stuff. Up to you.

Hmmm, alright lets get this in first and then think about simplifying/unifying things with generic-sop later (the Rank2 stuff could also possibly be simplified).

[edsko]

Ok. Mind that it would technically be yet another API breaking chance, since people would have to derive SOP.Generics (even if that's just another DeriveAnyClass thing). I don't mind either way, I can make the change if you wish.

[stevana]

I'll have to think about it, it's not clear to me at the moment how much it would simplify the library (if at all considering the overhead of learning generic-sop) vs how annoying it would be for the users of the library (yet another dependency, api change, extension and deriving instance, etc)...

I'll create an issue for it so we don't forget about it.