[ghc-steering-committee] [Proposal] #175: Lift.liftTyped (recommendation: accept)
Simon Peyton Jones
simonpj at microsoft.com
Wed Dec 19 09:28:10 UTC 2018
Is there any reason for not adopting the second alternative in section 5: make liftTyped the sole method of Lift?
I have elaborated on the tracker
https://github.com/ghc-proposals/ghc-proposals/pull/175#issuecomment-448527596
Simon
| -----Original Message-----
| From: ghc-steering-committee <ghc-steering-committee-bounces at haskell.org>
| On Behalf Of Eric Seidel
| Sent: 19 December 2018 03:37
| To: ghc-steering-committee <ghc-steering-committee at haskell.org>
| Subject: Re: [ghc-steering-committee] [Proposal] #175: Lift.liftTyped
| (recommendation: accept)
|
| To recap the discussion so far (thanks Joachim!):
|
| - Ben recommended accepance.
| - Richard pointed out that the proposed change will break instances that
| rely on the default method in a very bad way: they will compile but loop at
| runtime due to the mutually recursive defaults.
| - Iavor replied that it seems unlikely that much code would be affected, as
| DeriveLift has been the recommended strategy for a long time.
|
| I'm inclined to agree with Iavor that the actual impact is likely to be
| minimal, but this is something we could investigate with a scan of Hackage.
| (I believe people have done this in the past, so there may even be some
| scripts we can repurpose?)
|
| We could also mitigate the issue in a couple ways:
|
| 1. We could make violations of MINIMAL pragmas errors instead of warnings.
| This would need its own proposal, but it seems like a good idea to me
| regardless.
| 2. We could add a special warning/error for empty Lift instances. This is
| ad-hoc and kludgy, but easily doable since template-haskell is bundled with
| ghc.
| 3. We could provide a ghc-fix tool that automatically fixes this issue (and
| maybe others!). A number of other languages do this and it seems to work
| well for them. It also sounds like a very nice application of the ghc-
| exactprint work that Alan has done.
|
| On Wed, Nov 7, 2018, at 12:45, Eric Seidel wrote:
| > This is a wonderfully clever idea, but it has the unfortunate
| > consequence of making the new default lift (written in terms of
| > liftTyped) unusable even if you provide a custom liftTyped. I’ve had
| > to write my own lift on occasion (eg for types that contain Text
| > fields), so this is something we should consider. That being said, the
| > consequence of the TypeError fix is that custom instances will need an
| > extra one-line definition of lift, which is not that severe.
| >
| > This whole issue does make me wonder though, why does a violation of
| > the MINIMAL pragma produce a warning rather than an error? It seems
| > like the standard use case is to describe how one can break a chain of
| > mutually- recursive default definitions. Failure to do so will result
| > in a non- terminating program, which really ought to be an error.
| >
| > Sent from my iPhone
| >
| > > On Nov 7, 2018, at 10:40, Richard Eisenberg <rae at cs.brynmawr.edu>
| wrote:
| > >
| > > If we really want to encourage users to use `DeriveLift`, we could
| > > do something like
| > >
| > > class Lift t where
| > > ...
| > > default lift :: TypeError "Use DeriveLift" => ...
| > >
| > > Then, anyone relying on the old behavior would get a beautiful error
| message telling them exactly how to migrate.
| > >
| > > Richard
| > >
| > >> On Nov 5, 2018, at 6:31 PM, Iavor Diatchki <iavor.diatchki at gmail.com>
| wrote:
| > >>
| > >> Hello,
| > >>
| > >> I'd be surprised if there is a lot of code affected by that. Either
| way, I guess this wouldn't be an issue, if we just changed the proposal to
| make the default instance for `typedLift` looks like the old default for
| `lift` (i.e., use the `Data` constraint).
| > >>
| > >> The proposed design does have two benefits though:
| > >> * The new design requires fewer GHC extensions (no need for
| `DefaultSignatures`)
| > >> * It encourages programmers to use `DeriveLift`, which is shorter,
| safer, and more direct.
| > >>
| > >> -Iavor
| > >>
| > >>
| > >>
| > >>> On Mon, Nov 5, 2018 at 10:50 AM Richard Eisenberg
| <rae at cs.brynmawr.edu> wrote:
| > >>> Will this break existing code? The proposal suggests that liftTyped
| will have a suitable default implementation.
| > >>>
| > >>> Actually, this will break code, but not in the way one might think
| (with an undefined liftTyped). Instead, the proposal removes the default
| implementation of lift to use liftTyped. This means that any code relying
| on the default implementation of lift will now have an instance with
| mutually recursive, non-terminating methods. And this will all be silent.
| And, it's something that might be discovered only in clients of a library,
| rather than in the library itself. So I think that's pretty problematic.
| Even with our recommendation to derive Lift instances, this change in
| behavior is so terrible that it makes me lean against the proposal. Is
| there a design / migration path that avoids this problem?
| > >>>
| > >>> (Yes, yes, I know that I could voiced this opinion earlier, but it
| > >>> didn't occur to me until just now.)
| > >>>
| > >>> Richard
| > >>>
| > >>> > On Nov 5, 2018, at 1:37 PM, Ben Gamari <ben at well-typed.com> wrote:
| > >>> >
| > >>> > Hi everyone,
| > >>> >
| > >>> > I have been asked to Shepard #175, which proposes to add a
| > >>> > `liftTyped` method to the `Lift` typeclass used to lift Haskell
| > >>> > values into Template Haskell splices. Currently the `Lift`
| > >>> > typeclass provides no guarantee of type-safety, even when used in a
| Typed Template Haskell context.
| > >>> >
| > >>> > The proposal resolves this limitation by adding a typed lifting
| > >>> > operation to the `Lift` class:
| > >>> >
| > >>> > class Lift t where
| > >>> > lift :: t -> Q Exp
| > >>> > liftTyped :: t -> Q (TExp t)
| > >>> >
| > >>> > While this addition will break manually-written `Lift`
| > >>> > instances, we recommended that users derive such instances for
| > >>> > quite a while now, so it is not expected that the breakage will
| > >>> > be wide-spread. In light of this, I recommend that we accept this
| proposal.
| > >>> >
| > >>> > Given that we will likely want to get this in to 8.8, I suggest
| > >>> > that we limit the discussion to a week unless there is dissent.
| > >>> >
| > >>> > Cheers,
| > >>> >
| > >>> > - Ben
| > >>> > _______________________________________________
| > >>> > ghc-steering-committee mailing list
| > >>> > ghc-steering-committee at haskell.org
| > >>> > https://mail.haskell.org/cgi-bin/mailman/listinfo/ghc-steering-c
| > >>> > ommittee
| > >>>
| > >>> _______________________________________________
| > >>> ghc-steering-committee mailing list
| > >>> ghc-steering-committee at haskell.org
| > >>> https://mail.haskell.org/cgi-bin/mailman/listinfo/ghc-steering-com
| > >>> mittee
| > >
| > > _______________________________________________
| > > ghc-steering-committee mailing list
| > > ghc-steering-committee at haskell.org
| > > https://mail.haskell.org/cgi-bin/mailman/listinfo/ghc-steering-commi
| > > ttee
| _______________________________________________
| ghc-steering-committee mailing list
| ghc-steering-committee at haskell.org
| https://mail.haskell.org/cgi-bin/mailman/listinfo/ghc-steering-committee
More information about the ghc-steering-committee
mailing list