search

ekmett / bytes

Serialization primitives that work with both cereal and binary.
http://hackage.haskell.org/package/bytes
Other
22 stars 13 forks source link

Question: how to adapt bytes to GHC 8.8? #45

Closed RyanGlScott closed 5 years ago

RyanGlScott commented 5 years ago

bytes fails to build with GHC 8.8.1-alpha1:

``` [5 of 5] Compiling Data.Bytes.Serial ( src/Data/Bytes/Serial.hs, /home/rgscott/Documents/Hacking/Haskell/staging/dist-newstyle/build/x86_64-linux/ghc-8.8.0.20190424/bytes-0.15.5/build/Data/Bytes/Serial.o ) src/Data/Bytes/Serial.hs:282:28: error: • Could not deduce (MonadFail m) arising from a use of ‘fail’ from the context: (MonadGet m, Storable a) bound by the type signature for: restore :: forall (m :: * -> *) a. (MonadGet m, Storable a) => m a at src/Data/Bytes/Serial.hs:278:1-54 Possible fix: add (MonadFail m) to the context of the type signature for: restore :: forall (m :: * -> *) a. (MonadGet m, Storable a) => m a • In the second argument of ‘($)’, namely ‘fail "restore: Required more bytes"’ In a stmt of a 'do' block: unless (n >= required) $ fail "restore: Required more bytes" In the expression: do let required = sizeOf (undefined :: a) PS fp o n <- getByteString required unless (n >= required) $ fail "restore: Required more bytes" return $ unsafePerformIO $ withForeignPtr fp $ \ p -> peekByteOff p o | 282 | unless (n >= required) $ fail "restore: Required more bytes" | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ src/Data/Bytes/Serial.hs:345:17: error: • Could not deduce (MonadFail m) arising from a use of ‘fail’ from the context: MonadGet m bound by the type signature for: deserialize :: forall (m :: * -> *). MonadGet m => m Void at src/Data/Bytes/Serial.hs:345:3-13 Possible fix: add (MonadFail m) to the context of the type signature for: deserialize :: forall (m :: * -> *). MonadGet m => m Void • In the expression: fail "I looked into the void." In an equation for ‘deserialize’: deserialize = fail "I looked into the void." In the instance declaration for ‘Serial Void’ | 345 | deserialize = fail "I looked into the void." | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ src/Data/Bytes/Serial.hs:608:18: error: • Could not deduce (MonadFail m) arising from a use of ‘fail’ from the context: MonadGet m bound by the type signature for: gdeserialize :: forall (m :: * -> *) a. MonadGet m => m (V1 a) at src/Data/Bytes/Serial.hs:608:3-14 Possible fix: add (MonadFail m) to the context of the type signature for: gdeserialize :: forall (m :: * -> *) a. MonadGet m => m (V1 a) • In the expression: fail "I looked into the void." In an equation for ‘gdeserialize’: gdeserialize = fail "I looked into the void." In the instance declaration for ‘GSerial V1’ | 608 | gdeserialize = fail "I looked into the void." | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ src/Data/Bytes/Serial.hs:622:10: error: • Could not deduce (MonadFail m) arising from a use of ‘fail’ from the context: (GSerial f, GSerial g) bound by the instance declaration at src/Data/Bytes/Serial.hs:616:10-52 or from: MonadGet m bound by the type signature for: gdeserialize :: forall (m :: * -> *) a. MonadGet m => m ((:+:) f g a) at src/Data/Bytes/Serial.hs:619:3-14 Possible fix: add (MonadFail m) to the context of the type signature for: gdeserialize :: forall (m :: * -> *) a. MonadGet m => m ((:+:) f g a) • In the expression: fail "Missing case" In a case alternative: _ -> fail "Missing case" In the expression: case a of 0 -> liftM L1 gdeserialize 1 -> liftM R1 gdeserialize _ -> fail "Missing case" | 622 | _ -> fail "Missing case" | ^^^^^^^^^^^^^^^^^^^ src/Data/Bytes/Serial.hs:791:24: error: • Could not deduce (MonadFail m) arising from a use of ‘fail’ from the context: MonadGet m bound by the type signature for: gdeserializeWith :: forall (m :: * -> *) a. MonadGet m => m a -> m (V1 a) at src/Data/Bytes/Serial.hs:791:3-18 Possible fix: add (MonadFail m) to the context of the type signature for: gdeserializeWith :: forall (m :: * -> *) a. MonadGet m => m a -> m (V1 a) • In the expression: fail "I looked into the void." In an equation for ‘gdeserializeWith’: gdeserializeWith _ = fail "I looked into the void." In the instance declaration for ‘GSerial1 V1’ | 791 | gdeserializeWith _ = fail "I looked into the void." | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ src/Data/Bytes/Serial.hs:803:10: error: • Could not deduce (MonadFail m) arising from a use of ‘fail’ from the context: (GSerial1 f, GSerial1 g) bound by the instance declaration at src/Data/Bytes/Serial.hs:797:10-55 or from: MonadGet m bound by the type signature for: gdeserializeWith :: forall (m :: * -> *) a. MonadGet m => m a -> m ((:+:) f g a) at src/Data/Bytes/Serial.hs:800:3-18 Possible fix: add (MonadFail m) to the context of the type signature for: gdeserializeWith :: forall (m :: * -> *) a. MonadGet m => m a -> m ((:+:) f g a) • In the expression: fail "Missing case" In a case alternative: _ -> fail "Missing case" In the expression: case a of 0 -> liftM L1 (gdeserializeWith m) 1 -> liftM R1 (gdeserializeWith m) _ -> fail "Missing case" | 803 | _ -> fail "Missing case" | ^^^^^^^^^^^^^^^^^^^ src/Data/Bytes/Serial.hs:839:10: error: • Could not deduce (MonadFail m) arising from a use of ‘fail’ from the context: MonadGet m bound by the type signature for: deserializeWith2 :: forall (m :: * -> *) a b. MonadGet m => m a -> m b -> m (Either a b) at src/Data/Bytes/Serial.hs:836:3-18 Possible fix: add (MonadFail m) to the context of the type signature for: deserializeWith2 :: forall (m :: * -> *) a b. MonadGet m => m a -> m b -> m (Either a b) • In the expression: fail "Missing case" In a case alternative: _ -> fail "Missing case" In the expression: case a of 0 -> liftM Left m 1 -> liftM Right n _ -> fail "Missing case" | 839 | _ -> fail "Missing case" | ^^^^^^^^^^^^^^^^^^^ ```

All of these errors occur within a MonadGet constraint, so one tempting solution to this problem is to simply make MonadFail a superclass of MonadGet:

diff --git a/bytes.cabal b/bytes.cabal
index 401777b..540c928 100644
--- a/bytes.cabal
+++ b/bytes.cabal
@@ -74,7 +74,8 @@ library
     build-depends: ghc-prim

   if impl(ghc < 8.0)
-    build-depends: semigroups >= 0.5      && < 1
+    build-depends: fail       == 4.9.*,
+                   semigroups >= 0.5      && < 1

   exposed-modules:
     Data.Bytes.Get
diff --git a/src/Data/Bytes/Get.hs b/src/Data/Bytes/Get.hs
index 6ab1a04..fd5e26b 100644
--- a/src/Data/Bytes/Get.hs
+++ b/src/Data/Bytes/Get.hs
@@ -28,6 +28,7 @@ module Data.Bytes.Get
 #if __GLASGOW_HASKELL__ < 710
 import Control.Applicative
 #endif
+import qualified Control.Monad.Fail as Fail
 import Control.Monad.Reader
 import Control.Monad.Trans.Except as Except
 import Control.Monad.RWS.Lazy as Lazy
@@ -43,7 +44,7 @@ import Data.Int
 import qualified Data.Serialize.Get as S
 import Data.Word

-class (Integral (Remaining m), Monad m, Applicative m) => MonadGet m where
+class (Integral (Remaining m), Fail.MonadFail m, Applicative m) => MonadGet m where
   -- | An 'Integral' number type used for unchecked skips and counting.
   type Remaining m :: *

This makes bytes build on recent GHCs. For older (pre-8.0) GHCs, however, this presents a problem. Here's what happens if you build with GHC 7.10.3, for instance:

[4 of 5] Compiling Data.Bytes.Get   ( src/Data/Bytes/Get.hs, /home/rgscott/Documents/Hacking/Haskell/staging/bytes/dist-newstyle/build/x86_64-linux/ghc-7.10.3/bytes-0.15.5/build/Data/Bytes/Get.o )

src/Data/Bytes/Get.hs:202:10:
    No instance for (Fail.MonadFail B.Get)
      arising from the superclasses of an instance declaration
    In the instance declaration for ‘MonadGet B.Get’

Eek! Get from the binary package has only been an instance of MonadFail since 0.8.2.0, which is more recent than the version that is shipped with GHC 7.10.3.

One alternative woud be to switch all uses of fail with error. But this feels a bit wrong, since most parser data types have dedicated fail implementations that do something besides throw an exception.

phadej commented 5 years ago

One option is to add instances for old boot library Monad(Fail)s into fail package. That’s some work, not sure if GHC-7.10 is worth it. Neither whether Herbert would agree with that.

I’m ok for dropping old binary support. fail plan can be implemented later, if there’s need for it.

On 27 Apr 2019, at 16.26, Ryan Scott notifications@github.com wrote:

bytes fails to build with GHC 8.8.1-alpha1:

All of these errors occur within a MonadGet constraint, so one tempting solution to this problem is to simply make MonadFail a superclass of MonadGet:

diff --git a/bytes.cabal b/bytes.cabal index 401777b..540c928 100644 --- a/bytes.cabal +++ b/bytes.cabal @@ -74,7 +74,8 @@ library build-depends: ghc-prim

if impl(ghc < 8.0)

  • build-depends: semigroups >= 0.5 && < 1
  • build-depends: fail == 4.9.*,

  • semigroups >= 0.5 && < 1

    exposed-modules: Data.Bytes.Get diff --git a/src/Data/Bytes/Get.hs b/src/Data/Bytes/Get.hs index 6ab1a04..fd5e26b 100644 --- a/src/Data/Bytes/Get.hs +++ b/src/Data/Bytes/Get.hs @@ -28,6 +28,7 @@ module Data.Bytes.Get

    if __GLASGOW_HASKELL__ < 710

    import Control.Applicative

    endif

    +import qualified Control.Monad.Fail as Fail import Control.Monad.Reader import Control.Monad.Trans.Except as Except import Control.Monad.RWS.Lazy as Lazy @@ -43,7 +44,7 @@ import Data.Int import qualified Data.Serialize.Get as S import Data.Word

-class (Integral (Remaining m), Monad m, Applicative m) => MonadGet m where +class (Integral (Remaining m), Fail.MonadFail m, Applicative m) => MonadGet m where -- | An 'Integral' number type used for unchecked skips and counting. type Remaining m :: * This makes bytes build on recent GHCs. For older (pre-8.0) GHCs, however, this presents a problem. Here's what happens if you build with GHC 7.10.3, for instance:

[4 of 5] Compiling Data.Bytes.Get ( src/Data/Bytes/Get.hs, /home/rgscott/Documents/Hacking/Haskell/staging/bytes/dist-newstyle/build/x86_64-linux/ghc-7.10.3/bytes-0.15.5/build/Data/Bytes/Get.o )

src/Data/Bytes/Get.hs:202:10: No instance for (Fail.MonadFail B.Get) arising from the superclasses of an instance declaration In the instance declaration for ‘MonadGet B.Get’ Eek! Get from the binary package has only been an instance of MonadFail since 0.8.2.0, which is more recent than the version that is shipped with GHC 7.10.3.

One alternative woud be to switch all uses of fail with error. But this feels a bit wrong, since most parser data types have dedicated fail implementations that do something besides throw an exception.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or mute the thread.

RyanGlScott commented 5 years ago

I think adding new instances to the fail package is a no-go. I've proposed this idea to @hvr once and he (somewhat understandably) wasn't enthusiastic about maintaining out the necessary CPP to make this work. This is why I ended up backporting MonadFail instances for monad transformer types separately in transformers-compat. As @hvr predicted, the CPP required ended up being a huge mess.

I suppose we could guard the MonadGet Get instance behind a Cabal flag, much like the parsers package does. Of course, this would necessarily require dropping support for certain pre–GHC 8.0 build plans. I wonder how many people would actually be affected by this?

fumieval commented 5 years ago

Seems like it's worth having a survey how many GHC releases do people want the ecosystem to support ? Supporting GHCs older than 8.0 is getting quite painful.

RyanGlScott commented 5 years ago

To be clear, it would be quite easy to continue supporting the main skeleton of bytes on old GHCs. The real question is how far back we wish to provide instances for data types in the binary package.

RyanGlScott commented 5 years ago

One thing worth noting is that conditionally exposing the MonadGet Data.Binary.Get instance would affect some of bytes' dependencies. I've confirmed that the bound and linear libraries both rely on the existence of a MonadGet Data.Binary.Get instance in order to compile, and it's possible that there are other, non-Kmett libraries that do the same.

phadej commented 5 years ago

I could add MonadFail Get to binary-orphans. It’s on the lightest dependency, but if it doesn’t form cycle it would work.

Users of old GHC would have to pay, but they would get new bytes :)

On 3 May 2019, at 15.52, Ryan Scott notifications@github.com wrote:

One thing worth noting is that conditionally exposing the MonadGet Data.Binary.Get instance would affect some of bytes' dependencies. I've confirmed that the bound and linear libraries both rely on the existence of a MonadGet Data.Binary.Get instance in order to compile, and it's possible that there are other, non-Kmett libraries that do the same.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub, or mute the thread.

RyanGlScott commented 5 years ago

If we do go down the orphan-instance package route, it almost feels like this instance deserves its own library. binary-orphans, AFAICT, exists to provide orphan Binary instances for data types which do not otherwise have them, whereas we wish to simply backport an existing instance from the binary package to make it available on old versions of binary. That would also have the added bonus of not having to incur dependencies on huge packages like aeson when building with old versions of GHC.

Unfortunately, the name binary-orphans has already been taken, so it's unclear to me what to call this hypothetical compatibility package.

phadej commented 5 years ago

binary-fail :)

Sent from my iPhone

On 3 May 2019, at 16.07, Ryan Scott notifications@github.com wrote:

If we do go down the orphan-instance package route, it almost feels like this instance deserves its own library. binary-orphans, AFAICT, exists to provide orphan Binary instances for data types which do not otherwise have them, whereas we wish to simply backport an existing instance from the binary package to make it available on old versions of binary. That would also have the added bonus of not having to incur dependencies on huge packages like aeson when building with old versions of GHC.

Unfortunately, the name binary-orphans has already been taken, so it's unclear to me what to call this hypothetical compatibility package.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub, or mute the thread.

RyanGlScott commented 5 years ago

Resolved in #47.

ekmett commented 5 years ago

I'm probably not saying anything new to anyone here, but, for the record, I'm okay with pulling the plug on GHC < 8.0 or on comparably old binary versions for new releases of bytes. Legacy code can be supported by legacy code. What is required is that some version of bytes can satisfy the dependency, when existing code depends on bytes, not that the current version builds back to the stone age.

That policy might allow us a more sane upgrade and maintenance path, at the cost of occasionally having to backport the odd mission critical patch here or there.