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

Fix GNFData instance for V1 #19

Closed
treeowl opened this issue Jul 17, 2016 · 7 comments

Comments

@treeowl
Copy link
Member

commented Jul 17, 2016

Currently, we have

instance GNFData V1 where
  grnf = error "Control.DeepSeq.rnf: uninhabited type"

For old GHC, we should have

    grnf x = x `pseq` error "Control.DeepSeq.rnf: uninhabited type"

For newer GHC, we should have

    grnf x = case x of {}
@RyanGlScott

This comment has been minimized.

Copy link
Member

commented Jul 17, 2016

I agree, but is there any reason you chose pseq? Wouldn't seq suffice (especially since pseq is only available from base if you use a GHC-specific internal module)?

@treeowl

This comment has been minimized.

Copy link
Member Author

commented Jul 17, 2016

I thought I remembered some trouble getting the right error message, but I
could be confused.

On Jul 17, 2016 9:29 AM, "Ryan Scott" notifications@github.com wrote:

I agree, but is there any reason you chose pseq? Wouldn't seq suffice
(especially since pseq is only available from base if you use a GHC-specific
internal module
http://hackage.haskell.org/package/base-4.9.0.0/docs/GHC-Conc.html#v:pseq
)?


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#19 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/ABzi_faSLj0hVe92x8bGLDC-O68FHQvbks5qWi45gaJpZM4JOI2C
.

@hvr hvr added the enhancement label Jul 17, 2016

@treeowl

This comment has been minimized.

Copy link
Member Author

commented Jul 18, 2016

Indeed, I was confused. The problem I remembered with seq showed up with the old definition of Void as

newtype Void = Void Void

where the error behavior of void under 7.6 is extremely sensitive to details of its definition. In any case, the current definition (which I initially copied incorrectly—I've now edited the description to reflect reality) is really quite awful.

@rwbarton

This comment has been minimized.

Copy link

commented Jul 18, 2016

Why can't it just be grnf x = x seq ()?

treeowl added a commit to treeowl/deepseq that referenced this issue Jul 18, 2016
Fix GNFData instance for V1
Make `grnf` for `V1` a well-defined function that forces its
(bottom) argument rather than making the function itself bottom.

Fixes haskell#19
@treeowl

This comment has been minimized.

Copy link
Member Author

commented Jul 18, 2016

@rwbarton, it can be, but I don't think that's nearly as clear, since the () is unreachable. Hypothetically, that could even trigger a dead code warning in the future, I would think.

RyanGlScott added a commit that referenced this issue Aug 16, 2016
Fix GNFData instance for V1 (#20)
Make `grnf` for `V1` a well-defined function that forces its
(bottom) argument rather than making the function itself bottom.

Fixes #19
@hvr

This comment has been minimized.

Copy link
Member

commented Apr 16, 2017

@RyanGlScott @treeowl just double-checking, can you think of any (reasonable) program that could break or change its semantics due to this change when upgrading from deepseq-1.4.2 to the unreleased deepseq-1.4.3?

@RyanGlScott

This comment has been minimized.

Copy link
Member

commented Apr 16, 2017

As far as a change in semantics goes, it depends on whether you're using a very recent GHC or not.

If you're using any GHC up to 8.2, then I don't think you'd notice much of a difference at all. The reason being is that the old GNFData V1 instance always threw error "Control.DeepSeq.rnf: uninhabited type", regardless of whether the V1 value was actually nonterminating or something which threw an exception itself. But even after this most recent deepseq change, if you're using GHC 8.2 or earlier, you'll still always get an error in such a scenario. Why? It's because DeriveGeneric always derives this implementation of from for empty data types:

instance Generic Empty where
  from x = M1 $ case x of
                  _ -> error "No generic representation for empty datatype Empty"
  ...

So you'll still get an exception, just with a different error message.

In GHC 8.4/HEAD, however, DeriveGeneric's behavior with respect to empty datatypes has changes. Now this would be the derived implementation of from:

instance Generic Empty where
  from x = M1 $ case x of {}
  ...

So in GHC 8.4, you can actually observe the difference between, say, let x = x in x :: V1 () and error "Boom" :: V1 (). Here is a program that witnesses this:

{-# LANGUAGE DeriveGeneric #-}
{-# LANGUAGE ScopedTypeVariables #-}
module Foo where

import Control.DeepSeq
import Control.Exception
import GHC.Generics

data Empty deriving Generic
instance NFData Empty

empty :: Empty
empty = let x = x in x

main :: IO ()
main = evaluate (rnf empty) `catch` \(_ :: SomeException) -> putStrLn "Caught error"

With GHC 8.4 and deepseq-1.4.2.0, this will print Caught error. With GHC 8.4 and deepseq-1.4.3.0, this will loop forever.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.