haskell / core-libraries-committee

95 stars 16 forks source link

Move `base` `Lift` instances into `base` #287

Closed TeofilC closed 1 month ago

TeofilC commented 2 months ago

Summary

Historically both base and template-haskell were non-reinstallable. This means that the end user is forced to use whatever version came with their compiler. The only way a user can upgrade these packages is by upgrading their compiler.

Recently we've been working towards making these packages reinstallable. A key part of that is moving the wired-in bits into ghc-internal.

In this MR: https://gitlab.haskell.org/ghc/ghc/-/merge_requests/12479, I moved the wired-in identifiers from template-haskell into ghc-internal. This meant that the Lift typeclass was no longer defined in template-haskell, but in ghc-internal. Lift is wired-in because the desugaring of quote syntax makes use of it.

As part of this MR, I moved all the Lift instances into either ghc-internal or base depending on where the type was defined. I mistakenly didn't make a CLC proposal at the time, so I'm making one now. If the CLC rejects this move, I'm happy to implement an alternative or revert the MR altogether.

Proposal

Move all the Lift instances for base types into base.

Lift was not exported from base and continues to not be exported from base. If users wish to access the Lift typeclass they will need to use quote syntax, import it from ghc-internal (users are discouraged from doing this), or import it from the user-facing template-haskell package. From a users perspective nothing will have changed. Perhaps Haddocks will be slightly different, but no code should break from this change.

This does increase the amount of code in base, though most of these instances are formulaic and will rarely change.

Alternative: Orphan instances

The only viable alternative I can think of is to keep the instances in template-haskell, but because the typeclass is now in ghc-internal these would become orphan instances. This would be a large breaking change, because users can make use of lift without importing template-haskell. The compiler desugars qoutes that require lift without requiring a direct import.

So, for instance, the following module would then fail to compile (x is automatically lifted):

import Data.Array.Byte
foo = let x = mempty :: ByteArray in [|x|]

Conclusion

Moving these instances is the only viable way of making template-haskell reinstallable that I am aware of. This change is relatively invisible to end-users, but it does come at the cost of increasing the amount of things in base and therefore the things under the CLC's purview.

simonpj commented 2 months ago

So long as the API of base remains unchanged, I think the CLC should be entirely content. Part of the ghc-internal vs base discussion was agreeing that

Can you say whether the changes proposed here would be visible to a client of base?

tomjaguarpaw commented 2 months ago

Can you say whether the changes proposed here would be visible to a client of base?

Technically, yes, because base now exposes an instance it didn't previously. That's a small API change, but it is one all the same.

TeofilC commented 2 months ago

Can you say whether the changes proposed here would be visible to a client of base?

I think it's a bit of an edge case. In order to see the instance you need to have both the type in question and Lift in scope. In order to do that you have to import two libraries base and template-haskell (or ghc-internal). This is the same before and after this change.

With a normal library that definition site would make a difference because you could change the version of base and get a different instance definition. But because these were both non-reinstallable that didn't apply. The Lift instance was fixed by the definition that came with ghc. Once base becomes reinstallable, there might be a difference though. Varying the version of base might change the instance for types in base. (Though the majority of the types are defined in ghc-internal for now so their instances would still be fixed for each GHC version)

I think there is a change in the Haddocks. Previously the instances would only show in template-haskell, now they also show in base, but I'm not sure if we care about that.

tomjaguarpaw commented 2 months ago

I think there is a change in the Haddocks. Previously the instances would only show in template-haskell, now they also show in base, but I'm not sure if we care about that.

In any case, documentation changes are out of scope for CLC.

phadej commented 2 months ago

I think it's a bit of an edge case. In order to see the instance you need to have both the type in question and Lift in scope.

You actually don't. (EDIT: and you actually give the very same example in the proposal).

With

cabal-version: 3.4
name: ex
version: 0

executable ex
  main-is: Ex.hs
  build-depends: base

Note, only dependency on base, and

{-# LANGUAGE TemplateHaskellQuotes #-}
module Main (main) where

import Data.Array.Byte

main :: IO ()
main = do 
  e <- (let x = mempty :: ByteArray in [| x |])
  print e

The program works:

% cabal run ex -w ghc-9.10.1
Build profile: -w ghc-9.10.1 -O1
In order, the following will be built (use -v for more details):
 - ex-0 (exe:ex) (file Ex.hs changed)
Preprocessing executable 'ex' for ex-0..
Building executable 'ex' for ex-0..
[1 of 1] Compiling Main             ( Ex.hs, /codetmp/ex/dist-newstyle/build/x86_64-linux/ghc-9.10.1/ex-0/x/ex/build/ex/ex-tmp/Main.o ) [Source file changed]
[2 of 2] Linking /codetmp/ex/dist-newstyle/build/x86_64-linux/ghc-9.10.1/ex-0/x/ex/build/ex/ex [Objects changed]
AppE (AppE (VarE Language.Haskell.TH.Syntax.addrToByteArray) (LitE (IntegerL 0))) (LitE (BytesPrimL ))

as far as I understand what GHC-9.10 does, when TemplateHaskellQuotes are involved, the (implicit) dependency on template-haskell is added by GHC, and necessary modules are loaded.

With orphans, GHC won't find them "as by-product" of loading module where Lift is defined.


TL;DR: if this proposal is not accepted, the orphan Lift ByteArray instance won't be found, and the above example will fail to work.

One could argue that implicit loading of template-haskell wasn't correct to begin with, i.e. if user asked for TemplateHaskell(Quotes) and used quotations, but template-haskell weren't passed by -package-id, GHC should refused to desugar quotations; but now as definitions are in ghc-internal it's less of an issue (though IMHO implicit - non transitive - dependencies on ghc-prim / ghc-internal are still unclean; but having no-dependency packages is very rare; but there are such: https://hackage.haskell.org/package/composition-1.0.2.2)

TeofilC commented 2 months ago

Yes good point. I should've said "explicitly import Lift from template-haskell/ghc-internal or implicitly use it through use of TemplateHaskellQoutes"

Bodigrim commented 2 months ago

Can you say whether the changes proposed here would be visible to a client of base?

Since https://gitlab.haskell.org/ghc/ghc/-/merge_requests/12479/diffs#2ca7b1d6bb24cd05ca30a428efb924268c8dbc51 had to update testsuite/tests/interface-stability/base-exports.stdout, we can be certain that the changes affected base API and are visible to clients.

TeofilC commented 2 months ago

I want to give a bit more time for folks to ask questions and for discussion, but I'd also like to hear about the CLCs decision relatively soon so I can revert the changes if need be. Would it be OK to put this to a vote around the 30th of September?

TeofilC commented 2 months ago

had to update testsuite/tests/interface-stability/base-exports.stdout

To save folks having to navigate the quite slow/buggy GitLab interface for such a large MR, here's the relevant bit of the diff:

instance GHC.Internal.TH.Lift.Lift Data.Array.Byte.ByteArray -- Defined in ‘Data.Array.Byte’
instance forall k (a :: k). GHC.Internal.TH.Lift.Lift (Data.Fixed.Fixed a) -- Defined in ‘Data.Fixed’

The rest of the instances exist in ghc-internal because that's where the types are defined nowadays

tomjaguarpaw commented 2 months ago

FWIW my vote will be +1

Bodigrim commented 2 months ago

If there are no more concerns / comments, let's trigger a vote soon, yes.

(My vote will be +1 as well)

Bodigrim commented 2 months ago

Dear CLC members, let's vote on the proposal moving instance Lift ByteArray and instance Lift (Fixed a) from template-haskell into base. The MR https://gitlab.haskell.org/ghc/ghc/-/merge_requests/12479/diffs is rather large, please scroll down to libraries/base to see diffs falling under CLC purview.

The change is caused by moving class Lift from template-haskell (which is downstream dependency of base) to ghc-internal (which is upstream dependency of base). Because of this the only other alternative is to keep these two instances as orphans in template-haskell which has many unpleasant consequences (and likely to break in certain circumstances). The proposed change is not breaking.

@tomjaguarpaw @parsonsmatt @hasufell @velveteer @mixphix @angerman


+1 from me. I'm slightly worried about TH guts leaking into base, but this proposal is probably lesser evil and works towards reinstallable template-haskell, which would be a huge win.

tomjaguarpaw commented 2 months ago

+1

mixphix commented 2 months ago

+1

parsonsmatt commented 1 month ago

+1

Bodigrim commented 1 month ago

Thanks all, that's enough votes to approve.