Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add instance Lift (Expr s a) #1119

Merged
merged 22 commits into from
Jul 21, 2019
Merged

Conversation

ocharles
Copy link
Member

@ocharles ocharles commented Jul 16, 2019

This allows Exprs to be lifted in template-haskell. This is useful to build a [dhall| ... |] quasiquoter

This allows Exprs to be lifted in template-haskell. This is useful to
build a [dhall||] quasiquoter
{-# LANGUAGE OverloadedStrings #-}
{-# LANGUAGE TemplateHaskell #-}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that @vmchale at one point requested that Dhall not use TemplateHaskell or QuasiQuotes due to issues with cross-compilation. See: #89

@vmchale: Is this still a concern for you?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, even if it were, we could guard this behind a cabal configure flag so that users can compile out Template Haskell support if necessary. But let's still confirm whether or not we even need to do so

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It wouldn't be hard to remove it from this file.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, if it's easy to do, then go ahead. The real issue here is that I just don't really understand what are the exact requirements for Dhall to be easy-to-cross-compile. For example, it's not clear to me if depending on th-lift-instances matters or not for the original use case @vmchale had in mind.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can actually get away with just TemplateHaskellQuotes, which is cross-compilation friendly.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've changed this to TemplateHaskellQuotes, I hadn't even heard of that extension!

@sjakobi
Copy link
Collaborator

sjakobi commented Jul 17, 2019

Sorry for the merge conflicts, @ocharles! :/

@ocharles
Copy link
Member Author

No problem! I have a little more work to do anyway, should be easy to fix up.

@ocharles
Copy link
Member Author

Anyone got any ideas on the build failure? Is this Cabal messing up? It seems to be in the "Preprocessing" stage.

@AndrasKovacs
Copy link
Collaborator

FWIW, when I built your branch with stack just now, I got the same error, and added th-lift-instances-0.1.13@sha256:2852e468511805cb25d9e3923c9e91647d008ab4a764ec0921e5e40ff8a8e874 to stack.yaml. Then I got one more similar error about some th package, and added that to stack.yaml as well, and then it worked.

@ocharles
Copy link
Member Author

Oh, that's a slightly different error but I do probably need to fix the stack.yml to use the newer versions of th-lift-instances and th-lift.

@ocharles
Copy link
Member Author

I have a feeling that even though DeriveLift is supported on 7.10, this doesn't build on 7.10 because Cabal wasn't updated in time. haskell/cabal#2831 is the PR that teaches Cabal about the extension. @Gabriel439 what do you want to do about this?

@Gabriella439
Copy link
Collaborator

@ocharles: I think the only option is to guard all of the Lift-related changes behind a Cabal macro. We currently have to support GHC 7.10 for Eta

@quasicomputational
Copy link
Collaborator

Cabal actually doesn't care - that list is only relevant for other-extensions / default-extensions, and even then it does the sensible thing of passing unknown values through to GHC, which is what really counts.

@ocharles
Copy link
Member Author

@quasicomputational are you sure? Because this extension has existed since 7.4, yet the build is failing on 7.10

@quasicomputational
Copy link
Collaborator

GHC's manual says new in 8.0, so actually, yeah, this won't work without either conditionally exposing the Lift instance (which IMO is a bad idea) or defining it manually.

@ocharles
Copy link
Member Author

Huh. This says since 7.2.1

@ocharles
Copy link
Member Author

I guess this was a bug and the documentation has been fixed in HEAD?

@sjakobi
Copy link
Collaborator

sjakobi commented Jul 21, 2019

I guess this was a bug and the documentation has been fixed in HEAD?

Right: https://gitlab.haskell.org/ghc/ghc/commit/1ad3c8245cc114c3f862633d027361700fba3e50

@ocharles
Copy link
Member Author

Ok, I've taken a different approach using the default instance for Lift for any types that have a Data instance. All our types do, so this should work just as well.

@Gabriella439
Copy link
Collaborator

@ocharles: It looks like we might need to replace TemplateHaskellQuotes with another extension for older GHC versions

@ocharles
Copy link
Member Author

ocharles commented Jul 21, 2019 via email

@Gabriella439 Gabriella439 merged commit d55bf8f into dhall-lang:master Jul 21, 2019
@Gabriella439
Copy link
Collaborator

Thank you! 🙂

@ocharles
Copy link
Member Author

Oh good, I finally got all CI systems happy!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants