haskell / network-uri

URI manipulation facilities
Other
24 stars 33 forks source link

Add a quasiquoter for safely expressing URI literals at compile time #40

Closed neongreen closed 3 years ago

neongreen commented 5 years ago

https://hackage.haskell.org/package/network-uri-static-0.1.0.0/docs/Network-URI-Static.html

hvr commented 5 years ago

If network-uri does this, please make sure to not require a GHC with interpreter support; otherwise this will exclude some platform GHC supports

neongreen commented 5 years ago

And we should also provide instance Lift URI, which is currently defined as an orphan instance in network-uri-static (luckily, it seems to be nowhere else on Hackage).

ezrakilty commented 5 years ago

The module itself seems appealing; I don't know TH well enough to know how onerous the dependencies might be. @hvr how would I avoid requiring GHC interpreter support?

Does it impose any other requirements, to use TH in the Network.URI module? I presume that client modules don't have to start parsing in TH, right?

hvr commented 5 years ago

@ezrakilty the important bit is to avoid the use of {-# LANGUAGE TemplateHaskell #-} or {-# LANGUAGE QuasiQuotes #-} in network-uri.

However, some time ago (since GHC 8.0 iirc) the pragma {-# LANGUAGE TemplateHaskellQuotes #-} was introduced which is both Safe-inferred under SafeHaskell and does not require to run the TH interpreter (thus is also faster to compile).

ezrakilty commented 5 years ago

OK, Thanks for the explanation; very helpful.

Now, I was hoping to limit the Template Haskell involvement to the .Static submodule, but in order to avoid the "orphan instance" for Lift, I think we'll have to pull in (to Network.URI itself) the TemplateHaskellQuotes pragma and also import Language.Haskell.TH.Syntax. And in any case, the whole package begins to depend on the template-haskell package.

Are any of these dependencies worrisome? The alternative is of course just to leave network-uri-static as its own package. Neither course seems to objectionable overall. But I will pull it into the main package if we all think the new dependencies won't cause any headaches.

hvr commented 5 years ago

And in any case, the whole package begins to depend on the template-haskell package.

depending on the template-haskell package is not a problem at all; the ability to depend upon/link against template-haskell doesn't imply requiring a bytecode interpreter

PS: I'm starting to think that this deserves a "How to provide TH/QQ support in packages w/o requiring a compiler capable of running TH" article :-)

ezrakilty commented 5 years ago

@neongreen Could you please test the latest version in this repo, along with this version of network-uri-static? https://github.com/snakamura/network-uri-static/pull/6

To @hvr's points, we wound up having to rely on TemplateHaskellQuotes in both the Network.URI module (because the Lift instance moved to its "home") and in Network.URI.Static (because it's all about TH quoting).

We made the versioning work out fairly well such that I think no one will have trouble using the two packages (network-uri and network-uri-static) together. As far as I have been able to test the two packages together, everything seems to look smoothly.

I'd like to hear from some independent testers before releasing it, though.

neongreen commented 5 years ago

I tried the following commits:

- git: https://github.com/haskell/network-uri
  commit: e0db19dbe3e83e75567ca5396feb48dd8ec7336e
- git: https://github.com/snakamura/network-uri-static
  commit: 6dd454416e2f67003d51bd0ef93fb2fb429fd7a6

Unfortunately, it doesn't quite work for me:

    /Users/yom/wire/server/.stack-work/downloaded/vOKJFYRHo6vk/src/Network/URI/Static.hs:105:10: error:
        Duplicate instance declarations:
          instance Lift URI -- Defined at src/Network/URI/Static.hs:105:10
          instance [safe] Lift URI -- Defined in ‘Network.URI’
        |
    105 | instance Lift URI where
        |          ^^^^^^^^

    /Users/yom/wire/server/.stack-work/downloaded/vOKJFYRHo6vk/src/Network/URI/Static.hs:108:10: error:
        Duplicate instance declarations:
          instance Lift URIAuth
            -- Defined at src/Network/URI/Static.hs:108:10
          instance [safe] Lift URIAuth -- Defined in ‘Network.URI’
        |
    108 | instance Lift URIAuth where
        |          ^^^^^^^^^^^^

It looks like it tries to compile the module anyway, even though it's not in exposed-modules. I'm not sure why.

ezrakilty commented 5 years ago

Interesting. How did you invoke the compilation? The automated build succeeds: https://travis-ci.org/haskell/network-uri (except on ghc "head", 8.7, which I think is because of an unrelated known bug in the core libraries).

Please share your command line?

I suppose we can fix this by putting #ifs around the body of Network.URI.Static, but I'd like to make sure I understand what you're seeing.

neongreen commented 5 years ago

It's in a huge Stack project. I'll try to prepare a minimal test case.

On Nov 19 2018, at 1:18 am, Ezra elias kilty Cooper notifications@github.com wrote:

Interesting. How did you invoke the compilation? The automated build succeeds: https://travis-ci.org/haskell/network-uri (except on ghc "head", 8.7, which I think is because of an unrelated known bug in the core libraries). Please share your command line? I suppose we can fix this by putting #ifs around the body of Network.URI.Static, but I'd like to make sure I understand what you're seeing. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub (https://github.com/haskell/network-uri/issues/40#issuecomment-439739460), or mute the thread (https://github.com/notifications/unsubscribe-auth/ABc-amRxuMkXBNM5Sq4m_nxOmZ0fHrp2ks5uwfi6gaJpZM4WXP8Q).

snakamura commented 5 years ago

I think we need https://github.com/snakamura/network-uri-static/pull/7. Both network-uri and network-uri-static will have these instances without this #if.

ezrakilty commented 5 years ago

I think you're right, @snakamura.

I noticed another mistake on my part: the CPP code that "turns off" Network.URI.Static within network-uri 2.7 is keyed on __GLASGOW_HASKELL__ < 800. I think that was the wrong key to use, since it should be possible to use GHC >= 8 with network-uri-static < 0.1.2 and then you'd get duplicates. But I'm not sure whether I can switch on the version of network-uri-static without actually depending on it. I'll see if I can make something work.

arianvp commented 4 years ago

The problem here is that 2.7 was deprecated on Hackage and instead 2.6.3 was released. We will have to change network-uri-static to conditionally work on 2.6.3 instead of 2.7

ezrakilty commented 4 years ago

Yes, I made a mistake when I released 2.7 which had a lot of new material, but nothing that was actually breaking. So we're on network-uri 2.6.3, and the network-uri-static code will have to be updated accordingly. I'll see if I can make that happen.