haskell-infra / hackage-trustees

Issue tracker for Hackage maintainance and trustee operations
https://hackage.haskell.org/packages/trustees/
42 stars 7 forks source link

transformers-0.6.0.3 and 0.6.0.4 violates PVP #352

Closed phadej closed 1 year ago

phadej commented 1 year ago

From the changelog:

Restrict MonadTrans constraint to GHC >= 8.8

I suggest making a revision so transformers-0.6.0.3 and transformers-0.6.0.4 are not buildable with GHCs prior to GHC-8.8.

I cannot open an upstream issue as https://hub.darcs.net/ross/transformers/issues returns 502 bad gateway at the moment

EDIT: as issue tracker is unavailable I also don't know why the change was made. Maybe proper solution is to restrict transformers >=0.6.0.0 && <0.6.0.3 to use GHC >=8.8 instead.

andreasabel commented 1 year ago

I also don't know why the change was made.

The change was made because the MonadTrans constraint was incorrect for GHC-8.6 where Monad still had fail. See:

phadej commented 1 year ago

Ok, then it seems the proper solution is to restrict the lower bound on base on the beginning of transformers-0.6 series.

andreasabel commented 1 year ago

Concretely, this would mean base >= 4.13 for transformers-0.6.0.0/1/2, I suppose?

Slightly off-topic: Quite a few releases of transformers have base >= 2 as lower bound. How do you even test this nowadays? Or is this to be interpreted as "no known problem with any of the GHC versions"? [Sadly, the hackage matrix is gone which provided at least evidence for GHC >= 7.0.]

phadej commented 1 year ago

Concretely, this would mean base >= 4.13 for transformers-0.6.0.0/1/2, I suppose?

Yes.

Slightly off-topic: Quite a few releases of transformers have base >= 2 as lower bound. How do you even test this nowadays?

You don't. I most likely will drop testing anything older than GHC-8.0 / base-4.9 soon, as building (a local) matrix for over a dozen of GHC version takes too much time, so if there are base==4.* bounds, they will just bitrot on the lower end. OTOH I doubt it will badly affect many people.

Atm you can use trustee to build a local matrix for GHC >=7.0, you just need to get a machine with all these GHCs. In practice it means using ubuntu bionic, which will be EOL in less than a year. There aren't GHC-7 bindists for newer Ubuntu versions. That means that probably around may 2023 the haskell-ci will also drop GHC-7 stuff (and probably drop hvr-ppa installation method).

andreasabel commented 1 year ago

Concretely, this would mean base >= 4.13 for transformers-0.6.0.0/1/2, I suppose?

Yes.

This looks like a safe path of action, as far as I can see.

Thanks for the advice / update on testing older GHC versions. As soon as haskell-ci drops GHC 7.x, I will do so as well for coming releases of packages I maintain.

andreasabel commented 1 year ago

@RossPaterson: If you are listening, any chance to get transformers hosted on github.com? This would simplify the communication...

Bodigrim commented 1 year ago

Concretely, this would mean base >= 4.13 for transformers-0.6.0.0/1/2, I suppose?

I suggest we revise entire 0.6 series with base >= 4.13. Even leaving aside MonadFail issue, having different superclasses for MonadTrans, depending on GHC version, is asking for trouble.

RossPaterson commented 1 year ago

Concretely, this would mean base >= 4.13 for transformers-0.6.0.0/1/2, I suppose?

I suggest we revise entire 0.6 series with base >= 4.13. Even leaving aside MonadFail issue, having different superclasses for MonadTrans, depending on GHC version, is asking for trouble.

Having different superclasses for different versions is a problem, but keeping transformers working for older GHCs is also an aim. What about releasing a transformers-0.7 and marking 0.6.0.3/4 as broken?

Bodigrim commented 1 year ago

@RossPaterson practically speaking, no one uses transformers-0.6 yet, because even GHC 9.4 ships with transformers-0.5.6.2. I don't believe there is an appetite amongst remaining GHC 8.6 users (if there are any) to use transformers-0.6 - one must be pretty insistent even to try, essentially rebuilding the world.

Releasing a new major version of transformers-0.7 would only exacerbate this, as all version bumps to allow transformers-0.6 will be in vain. And that's been no small deal.

andreasabel commented 1 year ago

Well, mtl-2.3.1 is available from GHC-8.6, so maybe transformers-0.6 should naturally also be.
@phadej correctly suggested that the MonadTrans superclass constraint would have worked for me even with GHC 8.6, yet I was not aware of the MonadFailDesugaring trick:

@Bodigrim worte:

having different superclasses for MonadTrans, depending on GHC version, is asking for trouble.

Could you detail on what could go wrong here?

Bodigrim commented 1 year ago

Could you detail on what could go wrong here?

A package API is defined by its own version and must remain the same whatever dependencies (or compilers) are used. Seeing one superclass in documentation generated with a new GHC, but actually asking for none in an old GHC could be puzzling at least.

#if __GLASGOW_HASKELL__ >= 808
class (forall m. Monad m => Monad (t m)) => MonadTrans t where
#else
-- prior to GHC 8.8 (base-4.13), the Monad class included fail
class MonadTrans t where
#endif
andreasabel commented 1 year ago

Seeing one superclass in documentation generated with a new GHC, but actually asking for none in an old GHC could be puzzling at least.

transformers-0.6 had such conditional from the start (only with 806 instead of 808), so that is orthogonal to the OP.

I think we are looking for a more severe problem. Can you can construct something where type inference fails because the superclass constraint is absent?

Bodigrim commented 1 year ago

transformers-0.6 had such conditional from the start (only with 806 instead of 808), so that is orthogonal to the OP.

Yes and in my opinion this has been flawed from the start. Whatever, I don't feel too strongly about it.

andreasabel commented 1 year ago

@phadej: When opening this issue

Restrict MonadTrans constraint to GHC >= 8.8

did you have an example where build fails because of this relaxation?

Would be good to have a MWE so we more concretely see the problem and its severity.

phadej commented 1 year ago

The first example from the Quantified Class Constraints paper

{-# LANGUAGE PolyKinds #-}
import Control.Monad (liftM, ap)
import Control.Monad.Trans.Class

newtype (ComposeT t1 t2) m a = C { runC :: t1 (t2 m) a }

instance (MonadTrans t1, MonadTrans t2) => MonadTrans (ComposeT t1 t2) where
  lift = C . lift . lift

instance (MonadTrans t1, MonadTrans t2, Monad m) => Functor (ComposeT t1 t2 m) where
  fmap = liftM

instance (MonadTrans t1, MonadTrans t2, Monad m) => Applicative (ComposeT t1 t2 m) where
  pure = return
  (<*>) = ap

-- this could have `Monad (t1 (t2 m))`  context as well, that's what `base` moving back to with `Compose`.
instance (MonadTrans t1, MonadTrans t2, Monad m) => Monad (ComposeT t1 t2 m) where
  return = lift . return
  C m >>= k = C (m >>= runC . k)
C.hs:8:14: error:
    • Could not deduce (Monad (t2 m)) arising from a use of ‘lift’
      from the context: (MonadTrans t1, MonadTrans t2)
        bound by the instance declaration at C.hs:7:10-70
      or from: Monad m
        bound by the type signature for:
                   lift :: forall (m :: * -> *) a.
                           Monad m =>
                           m a -> ComposeT t1 t2 m a
        at C.hs:8:3-6
    • In the first argument of ‘(.)’, namely ‘lift’
      In the second argument of ‘(.)’, namely ‘lift . lift’
      In the expression: C . lift . lift
  |
8 |   lift = C . lift . lift
  |              ^^^^

FWIW, that type would be great to exist in transformers ifself.

Bodigrim commented 1 year ago
#!/usr/bin/env cabal
{- cabal:
build-depends: base, transformers >= 0.6
-}

import Control.Monad.Trans.Class
import Control.Monad.Trans.Reader

foo :: (Monad m, MonadTrans t) => t m ()
foo = return ()

main :: IO ()
main = runReaderT foo ()

This compiles with GHC >= 8.8, but fails with earlier ones.

Bodigrim commented 1 year ago

What is worse is that if in the example above you write

foo :: (Monad m, Monad (t m), MonadTrans t) => t m ()

then -Wredundant-constraints will push you to remove Monad (t m) as redundant. This makes it even worse, actively pushing users away from keeping backward compatibility.

andreasabel commented 1 year ago

Thanks for the MWEs! So the issue is severe. Summarizing:

  1. Some GHC-8.6 Haskell programs might break when going from <= 0.6.0.2 to >= 0.6.0.3.
  2. Having a GHC-version dependent monad superclass constraint might nudge users to remove class constraints that are only redundant for later GHC versions.

I still think that 2. isn't so severe. We have comparable situations e.g. with -Wunused-imports, which will prompt you to remove imports that are needed in connection with older version of base. If you want to support multiple GHC versions, you need to secure them via CI; everything else is naive. But I admit it could be annoying.

Point 1. could be fixed by either switching the superclass constraint back to GHC >= 8.6, deprecating 0.6.0.3 and 0.6.0.4, or by keeping the superclass constraint restricted to GHC >= 8.8, and deprecate 0.6.0.0/1/2.

Bodigrim commented 1 year ago

CI would not really help to test GHC 8.6 with transformers-0.6, unless someone asks for a build with constraints. If I were to take any action, I'd rather fix both points in one go.

phadej commented 1 year ago

@bodigrim

CI would not really help to test GHC 8.6 with transformers-0.6, unless someone asks for a build with constraints. If I were to take any action, I'd rather fix both points in one go.

But with transformers-0.5 your example

foo :: (Monad m, MonadTrans t) => t m ()
foo = return ()

won't work. I don't see a andreas' second point as a problem.


@andreasabel

I'm not sure what you mean by deprecating in

Point 1. could be fixed by either switching the superclass constraint back to GHC >= 8.6, deprecating 0.6.0.3 and 0.6.0.4, or by keeping the superclass constraint restricted to GHC >= 8.8, and deprecate 0.6.0.0/1/2.

I suggested, either revisions:

So for GHC-8.8+ the API of transformers-0.6 would be the same, and the only difference between the two is at which point the quantified constraints superclass is added.


Hackage deprecations are virtually useless as they are not strict. Solver can still pick up the deprecated version. I don't call the addition of unsolvable bounds like base <0 deprecation, to not confuse with deprecation on Hackage, but that is not required here.


Making revisions to all transformers-0.6 versions, so that transformers-0.6 becomes only GHC-8.8+ package is not in Hackage Trustees power. If maintainer decides to do so later, its their right. (EDIT: Ross could made the transformers-0.6 GHC-8.6+ package from beginning, which I indeed would preferred, but he didn't, and we have to respect that choice). Yet the problem can be fixed with less, and I'll hope trustees don't overuse the power granted to them.

andreasabel commented 1 year ago

@phadej suggests:

either revisions:

* Add `base >=4.13` (GHC-8.8) to `transformers-0.6.0.1..2` or

* Add `base >=4.13` to `transformers-0.6.0.3..4`

@RossPaterson which of these would you prefer?

phadej commented 1 year ago

Another way to put in: Will the next transformers version have Monad m => Monad (t m) also for GHC-8.6 or not. I haven't followed your discussions elsewhere, whether you decided on that.

(I'd prefer it to be so, as whole 8.8 thing was a fluke as far as see it).

RossPaterson commented 1 year ago

It seems that it should have that constraint for 8.6 also. 8.8 worked, but was too conservative.

phadej commented 1 year ago

@andreasabel Ross answered, could you do the required revisions (and maybe even a patch to transformers to undo the change as it's on the way to be bundled with GHC-9.6, not that important but annoying nevertheless as GHC-8.8 bound is mentioned in the docs) to close this issue.

andreasabel commented 1 year ago

@phadej : Not sure, @Ross answered to you but not to me. I gave a binary choice which could be easily answered by "first alternative" or "second alternative", but for some reason he didn't want to commit yet.

@Ross: Does your answer to @phadej imply that you want .3 and .4 revised to base >= 4.13?

phadej commented 1 year ago

@andreasabel @RossPaterson

I went ahead and made base >=4.13 (GHC-8.8+) revisions to all transformers-0.6 releases.

When you figure out what's the future should be you may either revert/relax revisions, or better make clean transformers-0.6.0.5 release.

For now this issue is fixed, now transformers-0.6.0.3 and transformers-0.6.0.4 don't violate PVP anymore,as the change cannot be observed.