Convert from newtype to lens

[byorgey]

We were using the newtype package in several places; since it's now entirely superseded by lens this patch converts from the former to the latter. This will be a breaking change since in particular the type of pathTrails is now different (it was a field accessor and is now an Iso). Maybe that's not a good idea and we should leave pathTrails for backwards compatibility, and provide something like pathTrailsIso instead? What do people think?

There are probably still many ways the code could be cleaned up, we could make better use of what lens offers, etc.

[bergey]

I'm excited to have the Isos available. Do you expect more compatibility-breaking changes as we add lenses? If so, should we try to get as many as possible into one version bump?

[byorgey]

I suppose the real issue is not about breaking vs non-breaking changes (I pretty much always come down on the side of the "right" change, whether it is breaking or not), but the consistency of the user interface. If we want to switch to having some record accessors be lenses, then we ought to be consistent and change all record accessors to lenses. I don't want to be in the strange situation where users can never remember which record accessors are just normal projections functions and which ones are lenses. So, to summarize, the choices (as I see it) are:

1. bite the bullet, break All The Things, and change all record accessors everywhere to lenses
2. provide lenses for some things, but give them new names

The upside of (1) is that lenses are awesome and useful. The downside is that now diagrams users have to know a bit about lenses too. Though I would hope that with good documentation we can easily provide users with enough to be able to use them in a rudimentary way.

Thoughts?

[bergey]

Changing everything to lenses is pretty tempting. (2) might be frustrating, if we can't compose the new lenses with preexisting accessors. If we add a lens copy of every record accessor, we have lots of underscores, two ways to write every access, and more functions for new users to learn.

But I haven't used lenses a whole lot. I don't have a good sense yet how hard to understand the type errors are. Type errors were my biggest frustration learning Haskell, and keeping Diagrams accessible to novice Haskellers seems worthwhile.

[byorgey]

I, on the other hand, do have a good sense of how hard to understand the type errors are, which I can now share: lens type errors are astoundingly incomprehensible. However, existing diagrams type errors are not exactly a walk in the park either. Getting good type error messages for embedded DSLs is an open research question.

In any case, I typically allow diagrams design decisions to be driven by choosing power and flexibility over simplicity. If we can find good ways to make diagrams accessible to novice Haskellers, so much the better, but that is a secondary goal. So I really think we ought to go with (1).

[bergey]

So I really think we ought to go with (1).

Exciting. Let me know how I can help.

[jeffreyrosenbluth]

yes, me too. Helping out will force me to learn more about lenses!

[byorgey]

It's actually fairly straightforward. We can start, of course, by merging this pull request. Then look for any and all places (in -core, -lib, -cairo, -svg, -postscript, -contrib...) where we define a record and export its accessors. Change all the field labels to begin with underscores, and add a call to makeLenses for that type. Then find all uses of those fields (which will be easy since they won't type check) and change them to use the new lenses. For the most part that means using (^.) (or view) or (.~). We will also need to update documentation, examples, etc., since now instead of writing things like

foo with { blah = baz, bar = quux }

we will write things like

foo (with & blah .~ baz & bar .~ quux)

Perhaps we should even stop using with and go back to just using def, since it is no longer quite so cute this way.

I will let the two of you decide how to split up the work if you both want to do it.

[jeffreyrosenbluth]

@bergey It doesn't really matter to me how we split it up. Should we just go at it one file at a time and let each other know what we are working on via trello or do you want to pick -core, -lib, ... and i'll choose a different package?

[bergey]

Trello sounds good to me. I made a trello card, with a long checklist of modules. I figure it's probably easier to do -core and -lib, then the things which depend on them.

[jeffreyrosenbluth]

After implementing several of the -lib modules using lenses for the def options, I'm having second thoughts regarding whether or not Lenses make sense in this case. I'm don't think using normal projections for faking optional parameters will be confusing to the user - since this subject needs explanation in any event. It seems to me there are several disadvantages to using Lenses here.

The code is uglier (IMHO) with more boilerplate. For example in Arrows where we now need only to export ArrowOpts(..) we will need to export ArrowOpts(ArrowOpts) and all 10 lenses we create for the record fields. We need to define all the field names with underscores, invoke Template Haskell with makeLenses. I like

foo with { blah = baz, bar = quux }

better than

foo (with & blah .~ baz & bar .~ quux)

This syntax can really be quite cumbersome for creating for example a highly customized arrow. Additionally, almost every existing diagrams program will break. I know that's not our primary concern, nevertheless it will be a nuisance.

Is it worth giving some thought to changing all record accessors to iso or lenses except for the faked optional parameters. I'll go along with whatever you guys decide. But I think we should pause to make sure it's a good idea.

[byorgey]

Hmm, you have a point. So the idea would be to distinguish between optional-argument records (with a Default instance), for which we would continue to just use normal field labels, and more "structural" sorts of things, for which we will switch to using lenses? I suppose that is a pretty clear-cut distinction so users would not necessarily be confused by the "inconsistency".

The only advantage of using lenses for optional argument records is if you wanted to "compute" with them, e.g. take an existing record of arguments and do something like multiplying one of the argument values by six. With lenses you could just say otherArgs & baz %~ (*6) whereas now you would have to say otherArgs { baz = baz otherArgs * 6 }. However, admittedly that is rare (I don't think I have ever done it!).

@bergey, what do you think? Can you see any other advantages to using lenses for optional argument records?

[bergey]

I'm not bothered by the syntax change. I don't find it ugly, and it only adds one non-whitespace character per field set.

@jeffreyrosenbluth makes a good point about the extra exports.

I agree with @byorgey that the mane point is whether we'd like to say things like args & bas *~ 6. With ArrowOpts, I wouldn't be that surprised to want that. ArrowOpts also has a Trail inside, blurring the distinction between optional-argument records and geometric types.

I'm OK either way. @byorgey, will you make the call?

[byorgey]

OK, in the spirit of "let's make this as powerful and flexible as possible", and seeing as it is impossible to predict what crazy/interesting things users will want to do (e.g. compute over options records), let's continue plowing ahead and converting everything to lenses, even optional argument records. And for the record, I don't mind the lens syntax either (at least, I find it no uglier than the record syntax).

One more nuisance we haven't talked about, though, is the need to move all the Haddock documentation from the record fields themselves to the lenses. Attaching Haddock documentation to TH-generated lenses is a bit fiddly but it can be done. Here's how:

1. Determine the types of the generated lenses/isos (e.g. by running Haddock on the module and looking at the generated documentation).

2. Replace makeLenses ''Foo by makeLensesWith (lensRules & generateSignatures .~ False) ''Foo. The idea is to make TH stop generating the type signature, so we can give one ourselves and hence attach Haddock documentation to it.

3. Below the splice, add things like

   -- | The Furble of a Foo.
   furble :: Lens' Foo Furble

   using the type(s) determined in part 1. It is a little-known feature of Haskell that top-level type declarations and definitions do not have to be together, in particular the type declaration can come after the definition. It should be enough to just move the documentation from the record field to the corresponding lens. In particular, we don't need to modify the documentation to say anything about lenses; we will just say that in a few central places in the user manual/tutorial.

This is a bit annoying but (1) it's the only way to retain decent Haddocks and (2) it makes the code easier to read, too, since one can actually see what the TH is generating.

[jeffreyrosenbluth]

OK, Let's finish the conversion first (since we have already begun) and
then go back and fix the haddock docs.

On Mon, Oct 7, 2013 at 3:06 PM, Brent Yorgey notifications@github.comwrote:

OK, in the spirit of "let's make this as powerful and flexible as
possible", and seeing as it is impossible to predict what crazy/interesting
things users will want to do (e.g. compute over options records), let's
continue plowing ahead and converting everything to lenses, even
optional argument records. And for the record, I don't mind the lens syntax
either (at least, I find it no uglier than the record syntax).

One more nuisance we haven't talked about, though, is the need to move all
the Haddock documentation from the record fields themselves to the lenses.
Attaching Haddock documentation to TH-generated lenses is a bit fiddly but
it can be done. Here's how:

1. Determine the types of the generated lenses/isos (e.g. by running
   Haddock on the module and looking at the generated documentation).

2. Replace makeLenses ''Foo by makeLensesWith (lensRules &
   generateSignatures .~ False) ''Foo. The idea is to make TH stop
   generating the type signature, so we can give one ourselves and hence
   attach Haddock documentation to it.
   3.

   Below the splice, add things like

   -- | The Furble of a Foo.furble :: Lens' Foo Furble

   using the type(s) determined in part 1. It is a little-known feature
   of Haskell that top-level type declarations and definitions do not have to
   be together, in particular the type declaration can come after the
   definition. It should be enough to just move the documentation from the
   record field to the corresponding lens. In particular, we don't need to
   modify the documentation to say anything about lenses; we will just say
   that in a few central places in the user manual/tutorial.

This is a bit annoying but (1) it's the only way to retain decent Haddocks
and (2) it makes the code easier to read, too, since one can actually see
what the TH is generating.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/124#issuecomment-25835899
.

[byorgey]

Agreed.

[jeffreyrosenbluth]

Argh!!! I can't get the type checker to cooperate when trying to build lenses for ArrowOpts. ghc says:
Could not deduce (c ~ c5) from the context (HasStyle c) bound by the type signature for setHeadStyle :: HasStyle c => ArrowOpts -> (c -> c) -> ArrowOpts

data ArrowOpts
  = ArrowOpts
    { _arrowHead  :: ArrowHT   -- ^ A shape to place at the head of the arrow.
    , _arrowTail  :: ArrowHT   -- ^ A shape to place at the tail of the arrow.
    , _arrowShaft :: Trail R2  -- ^ The trail to use for the arrow shaft.
    , _headSize   :: Double    -- ^ Radius of a circumcircle around the head.
    , _tailSize   :: Double    -- ^ Radius of a circumcircle around the tail.
    , _headGap    :: Double    -- ^ Distance to leave between
                              --   the head and the target point.
    , _tailGap    :: Double    -- ^ Distance to leave between the
                              --   starting point and the tail.
    , _headStyle  :: HasStyle c => c -> c  -- ^ Style to apply to the head.
    , _tailStyle  :: HasStyle c => c -> c  -- ^ Style to apply to the tail.
    , _shaftStyle :: HasStyle c => c -> c  -- ^ Style to apply to the shaft.
    }

makeLensesFor [ ("_arrowHead", "arrowHead")
              , ("_arrowTail", "arrowTail")
              , ("_arrowShaft", "arrowShaft")
              , ("_headSize", "headSize")
              , ("_tailSize", "tailSize")
              , ("_headGap", "headGap")
              , ("_tailGap", "tailGap") ] ''ArrowOpts

getHeadStyle :: HasStyle c => ArrowOpts -> (c -> c)
getHeadStyle (ArrowOpts {_headStyle = hs}) = hs

setHeadStyle :: HasStyle c => ArrowOpts -> (c -> c) -> ArrowOpts
setHeadStyle ao hs = ao {_headStyle = hs}

getTailStyle :: HasStyle c => ArrowOpts -> (c -> c)
getTailStyle (ArrowOpts {_tailStyle = ts}) = ts

setTailStyle :: HasStyle c => ArrowOpts -> (c -> c) -> ArrowOpts
setTailStyle ao ts = ao {_tailStyle = ts}

getShaftStyle :: HasStyle c => ArrowOpts -> (c -> c)
getShaftStyle (ArrowOpts {_shaftStyle = ss}) = ss

setShaftStyle :: HasStyle c => ArrowOpts -> (c -> c) -> ArrowOpts
setShaftStyle ao ss = ao {_shaftStyle = ss}

headStyle = lens getHeadStyle setHeadStyle
tailStyle = lens getTailStyle setTailStyle
shaftStyle = lens getShaftStyle setShaftStyle

[bergey]

If you don't provide a type signature, what does it infer?

Jeffrey Rosenbluth notifications@github.com wrote:

Argh!!! I can't get the type checker to cooperate when trying to build
lenses for ArrowOpts. ghc says:
Could not deduce (c ~ c5) from the context (HasStyle c) bound by the
type signature for setHeadStyle :: HasStyle c => ArrowOpts -> (c -> c) -> ArrowOpts

data ArrowOpts
 = ArrowOpts
{ _arrowHead  :: ArrowHT   -- ^ A shape to place at the head of the
arrow.
, _arrowTail  :: ArrowHT   -- ^ A shape to place at the tail of the
arrow.
 , _arrowShaft :: Trail R2  -- ^ The trail to use for the arrow shaft.
, _headSize   :: Double    -- ^ Radius of a circumcircle around the
head.
, _tailSize   :: Double    -- ^ Radius of a circumcircle around the
tail.
   , _headGap    :: Double    -- ^ Distance to leave between
                             --   the head and the target point.
   , _tailGap    :: Double    -- ^ Distance to leave between the
                             --   starting point and the tail.
, _headStyle  :: HasStyle c => c -> c  -- ^ Style to apply to the head.
, _tailStyle  :: HasStyle c => c -> c  -- ^ Style to apply to the tail.
, _shaftStyle :: HasStyle c => c -> c  -- ^ Style to apply to the
shaft.
   }

makeLensesFor [ ("_arrowHead", "arrowHead")
             , ("_arrowTail", "arrowTail")
             , ("_arrowShaft", "arrowShaft")
             , ("_headSize", "headSize")
             , ("_tailSize", "tailSize")
             , ("_headGap", "headGap")
             , ("_tailGap", "tailGap") ] ''ArrowOpts

getHeadStyle :: HasStyle c => ArrowOpts -> (c -> c)
getHeadStyle (ArrowOpts {_headStyle = hs}) = hs

setHeadStyle :: HasStyle c => ArrowOpts -> (c -> c) -> ArrowOpts
setHeadStyle ao hs = ao {_headStyle = hs}

getTailStyle :: HasStyle c => ArrowOpts -> (c -> c)
getTailStyle (ArrowOpts {_tailStyle = ts}) = ts

setTailStyle :: HasStyle c => ArrowOpts -> (c -> c) -> ArrowOpts
setTailStyle ao ts = ao {_tailStyle = ts}

getShaftStyle :: HasStyle c => ArrowOpts -> (c -> c)
getShaftStyle (ArrowOpts {_shaftStyle = ss}) = ss

setShaftStyle :: HasStyle c => ArrowOpts -> (c -> c) -> ArrowOpts
setShaftStyle ao ss = ao {_shaftStyle = ss}

headStyle = lens getHeadStyle setHeadStyle
tailStyle = lens getTailStyle setTailStyle
shaftStyle = lens getShaftStyle setShaftStyle

---

Reply to this email directly or view it on GitHub:
#124 (comment)

Sent from my Android phone with K-9 Mail. Please excuse my brevity.

[jeffreyrosenbluth]

It can't infer the type.

Could not deduce (t ~ (c -> c)) from the context (HasStyle c) bound by a type expected by the context: HasStyle c => c -> c

[bergey]

setHeadStyle :: ArrowOpts -> (forall c. HasStyle c => c -> c) -> ArrowOpts
setHeadStyle ao hs = ao {_headStyle = hs}

setTailStyle :: ArrowOpts -> (forall c. HasStyle c => c -> c) -> ArrowOpts
setTailStyle ao ts = ao {_tailStyle = ts}

[jeffreyrosenbluth]

OK, I guess that works but If I try to use (^.) to access the ArrowOpt fields I still get all kinds of type check errors. I'm not very comfortable that this model works yet.

[jeffreyrosenbluth]

!! Name Clash: & in both Data.Coordinates and Control.Lens.
This is a bit of a problem as they will both occur in the same module quite often.
Suggestions? For now I'm importing & from Control.Lens qualified as L.&, but now we are really talking ugly.

Also, we probably want to export Control.Lens ((&), (.~), (^.)) from diagrams-prelude so that all users don't have to import it. So I'm guessing we are going to need to rename &

[jeffreyrosenbluth]

So it looks like @bergey and I have finished a first pass at converting -core and -lib to use lenses. After writing a descent amount of code to convert -lib to use Control.Lens I want to say one last time that I think a wholesale replacement of projections with Lenses as record accessors and mutators is a mistake. I'm just not convinced we are getting much extra power and flexibility. I agree that the lens are awesome but I don't think they are the best solution all the time. I have come across few if any places in Opts records where I need to compose lenses or access deeply nested fields in records. I don't like several aspects of the new code which I'll list here. Some I have mentioned above:

1. TH forces us to think about the order of definitions in a module. I.e. what comes before and after a splice. I find this annoying an very un-haskell. I did it quickly because I was tired, but take a look at the order I had to use in TwoD.Offset to get everything in scope. -- I improved this by moving the TH records to the top of the module

2. Pattern matching is a fact of life and I don't like having to use the underscore field names. Additionally, setting up default records also uses the underscores.

3. & clashes with & from Data.Coordinates. We will also want to export several other combinators form the prelude which will clutter up the namespace.

4. I don't know what's up with the Arrows module yet, but the types are already a bit more complicated than previously and we still can't use things like (^.). -- I was able to fix this as well.

5. More boilerplate. Look at how much code we had to write for Arrow just to create the lenses for ArrowOpts -- bergey seems to have fixed this, but he did have to change the semantics.

6. Haddock documentation seems like it will be a real pain.

7. It feels like we are using very heavy machinery for a very lightweight task.

I don't often recommend throwing out lots of code I have worked hard to write. But in this case I suggest we rethink this one more time.
@byorgey

[byorgey]

You certainly make some valid points. Some responses below.

1. TH forces us to think about the order of definitions in a module. I.e. what comes before and after a splice. I > find this annoying an very un-haskell.

Yes, this is a bit annoying, but it's mostly a one-time cost for us, and will not in any way affect users of diagrams.

2. Pattern matching is a fact of life and I don't like having to use the underscore field names. Additionally, setting up default records also uses the underscores.

Another valid annoyance, but again, it doesn't affect users much if at all. (If users want to pattern match, I think that means we haven't given them enough high-level combinators.)

3. & clashes with & from Data.Coordinates. We will also want to export several other combinators form the prelude which will clutter up the namespace.

Very true. We will simply have to rename Data.Coordinates.&. (As for cluttering the namespace, that ship sailed a long time ago---Diagrams.Prelude already exports over 600 different names. =)

4. I don't know what's up with the Arrows module yet, but the types are already a bit more complicated than previously and we still can't use things like (^.). -- I was able to fix this as well.
5. More boilerplate. Look at how much code we had to write for Arrow just to create the lenses for ArrowOpts -- bergey seems to have fixed this, but he did have to change the semantics.

The problem with ArrowOpts, as I understand it, was the existential types. The style fields had types like HasStyle c => c -> c where the c did not show up in the type of ArrowOpts itself. You cannot make lenses for fields like this (indeed, what would be the type of a getter for this field? It would have to be something like ArrowOpts -> (exists c. HasStyle c => c -> c) but that is not a valid Haskell type). In the end I think this actually ended up being a win for lenses. The reason we had HasStyle c => c -> c instead of Style R2 is so that users could provide functions like fc red . lw 0.1 instead of having to apply them to mempty which is annoying. But now, with lenses, we can have our cake (use the simpler type Style R2) and eat it too (users don't have to write annoying boilerplate): def & headStyle %~ (fc red . lw 0.1). (Of course, we'll need some good documentation: many users will probably get tripped up by trying to write e.g. def & headStyle .~ fc red.)

6. Haddock documentation seems like it will be a real pain.

Very true.

7. It feels like we are using very heavy machinery for a very lightweight task.

What exactly is "the task"? Part of the point is that previously, we were using lightweight, ugly, inflexible machinery to accomplish certain things (constructing optional arguments records) which could not be easily extended to other things (computing over them). Now we are using heavy machinery indeed, but it is accomplishing a different task: what we end up with is more flexible and composable. In that sense the task is not so lightweight after all. I agree that we have not yet seen many interesting occurrences of computing over options records---but when all you have is a hammer, sometimes using a screwdriver doesn't even occur to you. If we do end up going with lensy options records, I'm willing to bet some enterprising users will eventually come up with some interesting uses for them.

In the end I am still sold on the consistency of using lenses for all record accessors, and providing users with more flexibility. There's no denying that it certainly does come at a cost---but most of that cost consists of one-time pain for us (not users). (Where by "us" I mean, of course, "you"---thanks for all your hard work on this!!) The rest of the pain, being amortized over future development, I don't see as being a huge problem. (e.g. when writing a new module with an optional argument record, you have to be a little careful about the order of things because of TH, and you have to pull the strange trick for putting Haddock comments on the lenses, which will take slightly longer than just writing the Haddock comments directly on the fields.)

Please don't interpret the above as shutting down the conversation; indeed, I'm really thankful to you for forcing us to think through this carefully, and I'm happy to continue the conversation as long as necessary. I am willing to serve as a "benevolent dictator" when absolutely necessary but I'd much rather see us all come to a consensus.

Also, the one thing I haven't yet done is actually look carefully at the code changes, and it's always possible that might change my mind! We should also try converting some example code (e.g. in the user manual and so on) to see what it's like and whether there are any unexpected surprises.

[jeffreyrosenbluth]

Thanks for your thoughtful response addressing all of my points. The Control.Lens is indeed an awesome package and using it for all of our record accessors gave me a great start in learning to use it. I agree that in the end most of the pain is for us, and that it is actually an improvement to the Arrow module. So I'm happy to join the consensus and endorse the move to Lenses. It will be interesting to see what you think of the code and how the rest of the community likes using the new syntax. We definitely need to convert some sample code, perhaps starting with something smaller than the user manual, like the gallery.