ekmett / lens

Lenses, Folds, and Traversals - Join us on web.libera.chat #haskell-lens
http://lens.github.io/
Other
2.03k stars 272 forks source link

Have prisms with ad-hoc classes for every field (equivalent to how `makeFields` generates lenses) #934

Open eyeinsky opened 4 years ago

eyeinsky commented 4 years ago

Perhaps this is not possible for some reason (or I'm blind), but would it make sense to have prisms generated with ad-hoc classes for every field -- similar to how makeFields does it?

I.e right now if two data types (with multiple data constructors for prisms to make sense) have identically named data constructors then the prisms need to be generated in different namespaces and also qualified at use sites. (The makeClassyPrisms makes a class per set of prisms, but that doesn't fit for when only some data constructor names match.)

RyanGlScott commented 4 years ago

I'm not quite sure what you're asking for. Can you provide a concrete example of the code you'd like to declare, along with the code that you would want to see generated by Template Haskell?

eyeinsky commented 4 years ago

Something like this:

module Other where

data Type2
  = One Double Int String
  | More Int

module Main where

import qualified Other

data Type1
  = One Int String
  | Two String

-- The following is meant to be TH generated:

class Has_One a b | a -> b where
  _One :: Prism' a b

instance Has_One Type1 (Int, String) where
  _One = prism
    (\(a, b) -> One a b)
    (\self -> case self of
      One a b -> Right (a, b)
      _ -> Left self)

instance Has_One Other.Type2 (Double, Int, String) where
  _One = prism
    (\(a, b, c) -> Other.One a b c)
    (\self -> case self of
      Other.One a b c -> Right (a, b, c)
      _ -> Left self)

Both Type1 and Type2 have a data constructor named One, so the prism named _One needs to be a class method. But instead of having a class per data type (makeClassyPrisms would generate classes AsType1 and AsType1 with both having a class-method _One) we would have a class per prism name (Has_One) -- so every data type with a data constructor One would get an instance of this class.

RyanGlScott commented 4 years ago

Thanks, that's a little bit clearer now. I suppose, then, you want something like makeConstructors :: Name -> DecsQ? I'm not 100% sold on that particular name, but just as makeFields generates overloaded field accessors, makeConstructors is intended to generate overloaded constructors. Then if you wrote $(makeConstructors ''Type1), you would get:

_Type1One :: Prism' Type1 (Int, String)
class HasOne s a | s -> a where
  _One :: Prism' s a
instance HasOne Type1 (Int, String) where
  _One = _Type1One
_Type1Two :: Prism' Type1 String
class HasTwo s a | s -> a where
  _Two :: Prism' s a
instance HasTwo Type1 String where
  _Two = _Type1Two

This is a bit different than what you were asking for, as it wouldn't generate an instance of HasOne for Type2. But it would provide better symmetry with how makeFields works.

eyeinsky commented 4 years ago

This looks like pretty much what I meant -- how do you mean it is different? I.e if I were to write something like this:

$(makeConstructors ''Type1)
$(makeConstructors ''Type2)

then it wouldn't generate HasOne for Type2?

Also, is this makeConstructors function in an upcoming release, I can't find it in the master?

RyanGlScott commented 4 years ago

This looks like pretty much what I meant -- how do you mean it is different? I.e if I were to write something like this:

$(makeConstructors ''Type1)
$(makeConstructors ''Type2)

then it wouldn't generate HasOne for Type2?

No, you're exactly right—that's how I would imagine my proposed design would work. I only said that it was different from your proposal in https://github.com/ekmett/lens/issues/934#issuecomment-663488542 because your proposal would have generated the instances for Type1 and Type2 in a single invocation (IIUC), whereas my proposal would require two separate invocations of makeConstructors in order to generate the two instances.

Also, is this makeConstructors function in an upcoming release, I can't find it in the master?

To be clear, makeConstructors doesn't exist anywhere yet. It's just a hypothetical name that I put forth to describe a possible way that this could be implemented. Again, I'm not super-attached to the name makeConstructors. If you have a better naming suggestion, I'd be open to alternatives.

eyeinsky commented 4 years ago

I think we are in complete agreement! :) And I don't really mind what the name of the TH function is nor what the class names would be, just that the prisms would be called as they usually are (but that is pretty much fixed anyway).

Why I was thinking about Has_One (with the underscore) is that then HasOne is still available for lenses where the method would be called one. Because although the class head (types and the fundep) is the same for both then HasOne for prisms would require the class method _One, but HasOne (as made by makeFields) as lens would require class method one. And I think these would conflict if both a lens and a prism with the same name were to be declared in the same module.

But what the exact convention for the class name for the prism field accessor is, I don't mind. And perhaps for the class name other people could chime in to for ideas.

phadej commented 4 years ago

Class names could begin with As, that is the prefix makePrisms uses (c.f. makeClassyLenses and makeFields both using Has*).

_Type1One :: Prism' Type1 (Int, String)
class AsOne s a | s -> a where
  _One :: Prism' s a
instance AsOne Type1 (Int, String) where
  _One = _Type1One
_Type1Two :: Prism' Type1 String
class AsTwo s a | s -> a where
  _Two :: Prism' s a
instance AsTwo Type1 String where
  _Two = _Type1Two

so the usage would be

val :: AsTwo s String => s
val = _Two # "foo"

We kind of shorten HasField "foo" into HasFoo, and HasConstructor "Bar" into AsBar.

I think makeConstructors is reasonable name.

RyanGlScott commented 4 years ago

I don't have a strong opinion on As vs. Has. As far as whether the generated class name should use an underscore or not, it is worth noting that lens' existing TH functions already have the potential to generate conflicting names, e.g.,

{-# LANGUAGE TemplateHaskell #-}
{-# OPTIONS_GHC -ddump-splices #-}
module Foo where

import Control.Lens

data X = MkX { _xField :: Int }
$(makeClassy ''X)

data Foo = MkFoo { _fooX :: Int }
$(makeFields ''Foo)

This will attempt to generate code involving a class named "HasX", but in two completely different contexts, resulting in a type error. As a result, I'd be inclined to just stick to the existing lens naming conventions (i.e., no extra underscore). We could always expose a variant of makeConstructors that allows users to override the default naming conventions (à la makeLensesWith) if they wish.

eyeinsky commented 4 years ago

For makeClassy and makeFields conflicting I think the library author would choose one or the other, so it won't break if the author is consistent. And if As is already used as prefix for classy prisms then this looks like the better solution.

@phadej You mentioned HasConstructor -- is this something that is becoming part of GHC for multi-constructor data types (like HasField is for records)?

phadej commented 4 years ago

HasConstructor doesn't exist and isn't proposed either, it could exist though.