haskell / deepseq

Deep evaluation of data structures
http://hackage.haskell.org/package/deepseq
Other
40 stars 29 forks source link

remove instance NFData (a -> b) #16

Open amigalemming opened 8 years ago

amigalemming commented 8 years ago

I suggested to remove the instance NFData (a -> b) because it cannot be implemented properly. I think the proposal got broad support: https://mail.haskell.org/libraries/2016-May/026961.html We have still to decide whether to drop the instance or replace it by an unimplementable one. The latter one would have the advantage that people see that the instance is omitted by intention. We would need a major version bump. I can setup a pull request if you like.

hvr commented 8 years ago

tbh, I'd like the @haskell/core-libraries-committee to sign off on this.

Personally, I'd prefer an unimplementable instance, as otherwise all it takes is one orphan instance hidden somewhere in a popular package to thwart this change. And even worse, if more than one orphan instance appears, we would risk ending up in an even worse situation than we're now in :-/

(/cc'ing @RyanGlScott as he wasn't yet part of the @haskell/core-libraries-committee github team at time of writing)

ekmett commented 8 years ago

I have no objection to removing the instance, but I'm not a fan of the idea of making it unimplementable.

Yes, one popular package could bring it back from the dead, or bring into scope a variant that enumerates the entire domain of a function and memoizes all results, but if it can weather the storm of popular opinion then perhaps it deserves to come back. This scenario is unlikely given the complete absence of interest in the current instance.

We have an open world. Orphan instances are deeply unpopular and anti-modular. If someone needs the instance, let them have it, it won't go far enough upstream to matter.

-Edward

On Mon, May 23, 2016 at 6:29 AM, Herbert Valerio Riedel < notifications@github.com> wrote:

tbh, I'd like the @haskell/core-libraries-committee https://github.com/orgs/haskell/teams/core-libraries-committee to sign off on this.

Personally, I'd prefer an unimplementable instance, as otherwise all it takes is one orphan instance hidden somewhere in a popular package to thwart this change. And even worse, if more than one orphan instance appears, we would risk ending up in an even worse situation than we're now in :-/

(/cc'ing @RyanGlScott https://github.com/RyanGlScott as he wasn't yet part of @haskell/core-libraries-committee https://github.com/orgs/haskell/teams/core-libraries-committee at time of writing)

— You are receiving this because you are on a team that was mentioned. Reply to this email directly or view it on GitHub https://github.com/haskell/deepseq/issues/16#issuecomment-220944194

amigalemming commented 8 years ago

On Mon, 23 May 2016, Edward Kmett wrote:

We have an open world. Orphan instances are deeply unpopular and anti-modular. If someone needs the instance, let them have it, it won't go far enough upstream to matter.

There needs to be only one package in any of hundred packages I import defining this instance as an orphan and then this disables my type safety check. I prefer the non-implementable instance and if someone needs the enumerating instance he can implement it for a newtype wrapper of the function type.

hvr commented 8 years ago

@amigalemming afaik you suggested to implement some warning facility to annotate undesirable/questionable instances; can you think of a variant which would help here?

ekmett commented 8 years ago

And you can just as easily implement an orphan instance module that provides the impossible instance with the same head.

class Don'tExportMe where boom :: a instance Don'tExportMe => NFData (a -> b) where rnf = boom

and import that to know that the other isn't being used in your code, because now you'll get a overlapping instance head.

I'd, however, treat whatever package exported this instance as just as questionable as one that provided the other.

NFData provides no guarantees that it "really" does anything. There are lots of such "mildly dangerous instances" that may not be as fully strict as you'd like for NFData. You'll never rule them all out.

For the scenario you fear to come to pass you have to make two lapses in judgment. First you have to depend on a package that provides the instance, and then you have to write code that depends on the instance existing.

That seems well balanced against the fact that someone using criterion might very well want to define the instance for NFData (a -> b) locally in their benchmark suite, regardless of whether or not you believe the instance should exist.

-Edward

On Mon, May 23, 2016 at 7:00 AM, amigalemming notifications@github.com wrote:

On Mon, 23 May 2016, Edward Kmett wrote:

We have an open world. Orphan instances are deeply unpopular and anti-modular. If someone needs the instance, let them have it, it won't go far enough upstream to matter.

There needs to be only one package in any of hundred packages I import defining this instance as an orphan and then this disables my type safety check. I prefer the non-implementable instance and if someone needs the enumerating instance he can implement it for a newtype wrapper of the function type.

— You are receiving this because you are on a team that was mentioned. Reply to this email directly or view it on GitHub https://github.com/haskell/deepseq/issues/16#issuecomment-220949948

snoyberg commented 8 years ago

I agree with Edward here, my vote goes with his stance.

On Mon, May 23, 2016, 3:33 PM Edward Kmett notifications@github.com wrote:

And you can just as easily implement an orphan instance module that provides the impossible instance with the same head.

class Don'tExportMe where boom :: a instance Don'tExportMe => NFData (a -> b) where rnf = boom

and import that to know that the other isn't being used in your code, because now you'll get a overlapping instance head.

I'd, however, treat whatever package exported this instance as just as questionable as one that provided the other.

NFData provides no guarantees that it "really" does anything. There are lots of such "mildly dangerous instances" that may not be as fully strict as you'd like for NFData. You'll never rule them all out.

For the scenario you fear to come to pass you have to make two lapses in judgment. First you have to depend on a package that provides the instance, and then you have to write code that depends on the instance existing.

That seems well balanced against the fact that someone using criterion might very well want to define the instance for NFData (a -> b) locally in their benchmark suite, regardless of whether or not you believe the instance should exist.

-Edward

On Mon, May 23, 2016 at 7:00 AM, amigalemming notifications@github.com wrote:

On Mon, 23 May 2016, Edward Kmett wrote:

We have an open world. Orphan instances are deeply unpopular and anti-modular. If someone needs the instance, let them have it, it won't go far enough upstream to matter.

There needs to be only one package in any of hundred packages I import defining this instance as an orphan and then this disables my type safety check. I prefer the non-implementable instance and if someone needs the enumerating instance he can implement it for a newtype wrapper of the function type.

— You are receiving this because you are on a team that was mentioned. Reply to this email directly or view it on GitHub https://github.com/haskell/deepseq/issues/16#issuecomment-220949948

— You are receiving this because you are on a team that was mentioned.

Reply to this email directly or view it on GitHub https://github.com/haskell/deepseq/issues/16#issuecomment-220967419

dolio commented 8 years ago

I'm also for no instance at this point. If you think the existing instance is bad, it's better for no instance to exist, so that you get an error if you accidentally try to use it. The other instances I can think of have roughly the same negatives as the existing one. You don't get a compile-time complaint, you just have to figure out what went wrong if your program ever exercises the instance (and either blows up or loops uselessly for a very long time). And the instance that doesn't just blow up requires introducing a new class to implement ideally, and then getting people to implement it.

amigalemming commented 8 years ago

On Mon, 23 May 2016, dolio wrote:

I'm also for no instance at this point. If you think the existing instance is bad, it's better for no instance to exist, so that you get an error if you accidentally try to use it. The other instances I can think of have roughly the same negatives as the existing one. You don't get a compile-time complaint,

I definitely want a compile-time complaint, and I think this can be better done with an explicitly forbidden instance. I think of something similar to Edward, only in Haskell 98, e.g.

class NotImplementable a where -- not exported instance NotImplementable a => NFData (a -> b) where

This instance tells any programmer that the instance cannot be defined anymore and that this is by intent.

amigalemming commented 8 years ago

On Mon, 23 May 2016, Herbert Valerio Riedel wrote:

@amigalemming afaik you suggested to implement some warning facility to annotate undesirable/questionable instances; can you think of a variant which would help here?

I proposed this one: https://ghc.haskell.org/trac/ghc/ticket/11796

It would solve the issue here, too, but it is not implemented in GHC, it did not even start a discussion, so far.

ekmett commented 8 years ago

My point about that instance is that it is something that you are just as free to implement as someone who wants to implement the instance you don't like.

Just to be clear, I was not advocating that the instance I mentioned be placed in base.

I was mentioning that if you wanted to rule out the instance in question, you could write your own orphan instance in your own package somewhere, and it'd infect the global context and conflict with any other attempted instance.

So you can either, simply check to make sure that your code still compiles with any version of your dependencies you know doesn't have the instance around, or you can jump through hoops and create one of the orphan 'impossible' instances described so far.

Only in the latter case do you break users who just want some instance like instance NFData (a -> b) locally in their criterion benchmarks, and they asked for it by depending on your code.

-Edward

On Mon, May 23, 2016 at 9:44 AM, amigalemming notifications@github.com wrote:

On Mon, 23 May 2016, dolio wrote:

I'm also for no instance at this point. If you think the existing instance is bad, it's better for no instance to exist, so that you get an error if you accidentally try to use it. The other instances I can think of have roughly the same negatives as the existing one. You don't get a compile-time complaint,

I definitely want a compile-time complaint, and I think this can be better done with an explicitly forbidden instance. I think of something similar to Edward, only in Haskell 98, e.g.

class NotImplementable a where -- not exported instance NotImplementable a => NFData (a -> b) where

This instance tells any programmer that the instance cannot be defined anymore and that this is by intent.

— You are receiving this because you are on a team that was mentioned. Reply to this email directly or view it on GitHub https://github.com/haskell/deepseq/issues/16#issuecomment-220984242

amigalemming commented 8 years ago

On Mon, 23 May 2016, Edward Kmett wrote:

Just to be clear, I was not advocating that the instance I mentioned be placed in base.

I understood it this way.

Only in the latter case do you break users who just want some instance like instance NFData (a -> b) locally in their criterion benchmarks, and they asked for it by depending on your code.

Is this a frequent usecase? How about using a newtype wrapper there? The deepseq package could provide the wrapper even by itself.

ekmett commented 8 years ago

The criterion example was one usecase. One that enumerates all of the inputs and memoizes a function's results is another that has the same head shape that also conflicts. Sure, we could hide it behind a newtype. We could hide every instance for (->) behind a newtype, but we don't.

But if you can build with any version of your dependencies that doesn't have the instance, then you can't be relying on it.

You'll only run into this problem when you explicitly call rnf on a function and when someone you don't like that you depend upon brings an instance into scope. That frankly strikes me as pretty far removed from an active concern, and more a theoretical exercise.

-Edward

On Mon, May 23, 2016 at 10:38 AM, amigalemming notifications@github.com wrote:

On Mon, 23 May 2016, Edward Kmett wrote:

Just to be clear, I was not advocating that the instance I mentioned be placed in base.

I understood it this way.

Only in the latter case do you break users who just want some instance like instance NFData (a -> b) locally in their criterion benchmarks, and they asked for it by depending on your code.

Is this a frequent usecase? How about using a newtype wrapper there? The deepseq package could provide the wrapper even by itself.

— You are receiving this because you are on a team that was mentioned. Reply to this email directly or view it on GitHub https://github.com/haskell/deepseq/issues/16#issuecomment-220999093

amigalemming commented 8 years ago

On Mon, 23 May 2016, Edward Kmett wrote:

The criterion example was one usecase. One that enumerates all of the inputs and memoizes a function's results is another that has the same head shape that also conflicts. Sure, we could hide it behind a newtype. We could hide every instance for (->) behind a newtype, but we don't.

The existence of different possible implementations is an argument pro dedicated newtype wrappers for me.

You'll only run into this problem when you explicitly call rnf on a function and when someone you don't like that you depend upon brings an instance into scope. That frankly strikes me as pretty far removed from an active concern, and more a theoretical exercise.

I want to be sure that I do not accidentally call 'rnf' or that a library function calls 'rnf' for me on any structure, where a function is contained somewhere deep in it, which can happen if only one of hundred transitively imported packages defines an orphan NFData (a -> b) instance. Even if I check the absense of such an orphan instance today, it may change easily if one of the transitively imported packages extends its imports.

ekmett commented 8 years ago

They can do that anyway if they just define their instance for any composite structure in the middle by hand. Even your newtype solution is evidence of this. You get no such transitive guarantee.

Sent from my iPhone

On May 23, 2016, at 12:03 PM, amigalemming notifications@github.com wrote:

On Mon, 23 May 2016, Edward Kmett wrote:

The criterion example was one usecase. One that enumerates all of the inputs and memoizes a function's results is another that has the same head shape that also conflicts. Sure, we could hide it behind a newtype. We could hide every instance for (->) behind a newtype, but we don't.

The existence of different possible implementations is an argument pro dedicated newtype wrappers for me.

You'll only run into this problem when you explicitly call rnf on a function and when someone you don't like that you depend upon brings an instance into scope. That frankly strikes me as pretty far removed from an active concern, and more a theoretical exercise.

I want to be sure that I do not accidentally call 'rnf' or that a library function calls 'rnf' for me on any structure, where a function is contained somewhere deep in it, which can happen if only one of hundred transitively imported packages defines an orphan NFData (a -> b) instance. Even if I check the absense of such an orphan instance today, it may change easily if one of the transitively imported packages extends its imports. — You are receiving this because you are on a team that was mentioned. Reply to this email directly or view it on GitHub

chessai commented 5 years ago

I think the main issue is the removal of the instance. It seems that this instance is not widely useful, and can be implemented as an orphan (or perhaps a newtype) for those who need it. Making it unimplementable seems to be a more controversial stretch, and tacking it on to the issue of the instance's removal makes it more likely that nothing will get done at all.

I think we should just remove the instance, make a changelog entry, and majour version bump.

amigalemming commented 5 years ago

On Mon, 24 Jun 2019, chessai wrote:

I think we should just remove the instance, make a changelog entry, and majour version bump.

Yes, please.

andrewthad commented 5 years ago

I agree that this instance just needs to be removed.

chessai commented 5 years ago

see #47

Zemyla commented 4 years ago

What it actually should do is use seq to turn it to WHNF, then use unpackClosure# to make sure everything inside it is evaluated as well.

amigalemming commented 4 years ago

On Sat, 2 Nov 2019, Zemyla wrote:

What it actually should do is use seq to turn it to WHNF, then use unpackClosure# to make sure everything inside it is evaluated as well.

The function can still be partial and then rnf should diverge, too, shouldn't it?

Zemyla commented 4 years ago

unpackClosure# doesn't actually evaluate the function. It simply takes it apart into the component pieces where any data needed for actually calling the function are stored.

Basically, imagine a subset of Haskell where only named functions exist and can be passed to things. To represent a partially evaluated function of some sort, you'd need an existential datatype, and functions to manipulate it.

data Closure a = forall u. Closure u (u -> a) -- Here, the (u -> a) function has to be able to be named at compile-time.

closeFmap :: (u -> a, a -> b, u) -> b
closeFmap (g, f, u) = g (f u)

instance Functor Closure where
  fmap f (Closure u g) = Closure (g, f, u) closeFmap

closeLiftA2 :: (u -> a, v -> b, a -> b -> c, u, v) -> c
closeLiftA2 (gu, gv, f, u, v) = f (gu u) (gv v)

closeAp :: (u -> a -> b, v -> a, u, v) -> b
closeAp (gu, gv, u, v) = gu u (gv v)

instance Applicative Closure where
  pure a = Closure a id

  liftA2 f (Closure u gu) (Closure v gv) = Closure (gu, gv, f, u, v) closeLiftA2

  Closure u gu <*> Closure v gv = Closure (gu, gv, u, v) closeAp

closeBind :: (u -> a, u, a -> Closure b) -> b
closeBind (gu, u, f) = case f (gu u) of
  Closure v gv -> gv v

instance Monad Closure where
  Closure u gu >>= f = Closure (gu, u, f) closeBind

This may seem silly at first, but it's pretty much what the stackless, tagless G-machine behind GHC does when you write code with closed variables, and it's also what you need to do when you work with some forms of distributed Haskell.

amigalemming commented 4 years ago

On Sat, 2 Nov 2019, Zemyla wrote:

unpackClosure# doesn't actually evaluate the function. It simply takes it apart into the component pieces where any data needed for actually calling the function are stored.

I think I understand what you are suggesting, but I doubt that it is a useful definition.

int-index commented 4 years ago

I agree with @Zemyla that rnf for functions should force everything inside the function's closure.

I think I understand what you are suggesting, but I doubt that it is a useful definition.

I disagree @amigalemming. This definition would be very much in spirit of the goals of deepseq. The documentation on Hackage presents lazy IO as a use case for rnf:

import System.IO
import Control.DeepSeq
import Control.Exception (evaluate)

readFile' :: FilePath -> IO String
readFile' fn = do
    h <- openFile fn ReadMode
    s <- hGetContents h
    evaluate (rnf s)
    hClose h
    return s

Here, it is safe because rnf s forces the string fully. However, what if we applied some function to it first?

    s <- hGetContents h
    let x = f s
    evaluate (rnf x)
    hClose h

If, say, x :: [Bool], then this is still safe. For example, we can imagine f = map isDigit. However, if we had f = map (==), then we'd end up with x :: [Char -> Bool]. Then to make it safe we should force the closures of these functions.

Thus the definition offered by @Zemyla would make the pattern above safe in the general case.

int-index commented 4 years ago

Although I'm not sure how @Zemyla imagines running rnf for the data captured in the function closure. Where would the NFData instances for the captured data come from? This suggests that rnf should be a primitive implemented in the RTS.

cartazio commented 4 years ago

yeah, as @int-index says, forcing the captured thunks in a closure requires RTS hooks to work, and closures are existentials, and thus the RTS would have to know about how to NFData everything underneath the function itself. Thats a pretty hard ask

cartazio commented 4 years ago

or maybe i'm missunderstanding this thread :)

int-index commented 4 years ago

and thus the RTS would have to know about how to NFData everything underneath the function itself. Thats a pretty hard ask

Maybe it's not hard – I believe NFData can be implemented in a generic manner, without having instances at all. Just like seq does not require any instances.

int-index commented 4 years ago

I'd like to offer the following specification:

Currently, this holds for some choices of f and cont, such as:

f = id
cont a = a `seq` return ()

However, it fails with other choices of f and cont due to the NFData function instance (the removal of which is proposed in this ticket):

f = const
cont g = g () `seq` return ()

Removing the instance for functions would turn the failing case into a compile-time error. Implementing NFData in the RTS to traverse the function's closure would allow both cases to succeed.

Zemyla commented 4 years ago

Exactly. The Haskell RTS knows what is inside each ADT and function, or else the garbage collector wouldn't work. And actually performing the RNF isn't trivial, but it is mechanical; evaluate thunks, walk into ADTs and closures and lifted immutable pointers (Array#, ArrayArray#, Weak#, etc), and RNF everything inside. Keep everything that's been fully evaluated in a hash table or something so you don't get stuck in a loop, which would be an advantage this would have over naive, user-defined NFData instances. There could also be a global table for CAFs, so they wouldn't be rnfed more than once either. Best of all, most of this machinery is already in place in the RTS, for creating compact regions.

At that point, I'm not sure you'd even need the NFData class. The only use I could think of would be custom instances like Once by Edward Kmett, but even that would be unnecessary if we used pointer tagging for RNF as well as WHNF. ADTs could be tagged automatically in the constructor (if all the arguments are RNF, then the constructor is as well), and closures and lifted immutable pointers could be tagged during garbage collection. At that point, not only would the typeclass be entirely unnecessary, but you could have a !! annotation for data declarations that shows the value in the constructor is in RNF already.

cartazio commented 4 years ago

One gotcha is making sure it plays nice with garbage collection. One languishing huge issue implicit with current rts primops is some of them dont yield to the scheduler tool completion.

On Wed, Jan 22, 2020 at 3:56 PM Zemyla notifications@github.com wrote:

Exactly. The Haskell RTS knows what is inside each ADT and function, or else the garbage collector wouldn't work. And actually performing the RNF isn't trivial, but it is mechanical; evaluate thunks, walk into ADTs and closures and lifted immutable pointers (Array#, ArrayArray#, Weak#, etc), and RNF everything inside. Keep everything that's been fully evaluated in a hash table or something so you don't get stuck in a loop, which would be an advantage this would have over naive, user-defined NFData instances. There could also be a global table for CAFs, so they wouldn't be rnfed more than once either. Best of all, most of this machinery is already in place in the RTS, for creating compact regions.

At that point, I'm not sure you'd even need the NFData class. The only use I could think of would be custom instances like Once by Edward Kmett, but even that would be unnecessary if we used pointer tagging for RNF as well as WHNF. ADTs could be tagged automatically in the constructor (if all the arguments are RNF, then the constructor is as well), and closures and lifted immutable pointers could be tagged during garbage collection. At that point, not only would the typeclass be entirely unnecessary, but you could have a !! annotation for data declarations that shows the value in the constructor is in RNF already.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/haskell/deepseq/issues/16?email_source=notifications&email_token=AAABBQWOU4VYZJRNELETYJ3Q7CXIXA5CNFSM4CERLO7KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEJVCJ6I#issuecomment-577381625, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAABBQQV2VGWSAUGBY5AO5TQ7CXIXANCNFSM4CERLO7A .

phadej commented 4 years ago

The in-RTS "deepseq" can be implemented without touching deepseq library, so that's out of scope of this library, and would require GHC proposal.

mixphix commented 1 year ago

This is mentioned as part of the milestone for deepseq-1.5.0.0. Given that I am in the process of updating certain instances to use QuantifiedConstraints, and considering dropping support for old versions of GHC (< 8.6), I think it's right to bump the major version soon. I'm inclined to remove the instance based on the discussion above, unless someone can make a good argument to leave it in.

phadej commented 1 year ago

@mixphix you can also do

-instance NFData (a -> b) where rnf = rwhnf
+instance GHC.Exts.Any => NFData (a -> b) where rnf = error "foo"

to forbid the use of instance

*Control.DeepSeq> force (id :: Int -> Int)

<interactive>:5:1: error:
    • Could not deduce: GHC.Exts.Any arising from a use of ‘force’

(using Any is better behaved than TypeError, until Unsatisfiable https://github.com/ghc-proposals/ghc-proposals/blob/f8ef93dbb0273bf26c28aba5008544bdf0d4edd8/proposals/0433-unsatisfiable.rst is implemented. The proposal discussses why TypeError as constraint is not great).

mixphix commented 1 year ago

I hesitate to use such "hacks" in favour of simply removing the instance. While this approach may let users define their own instance of NFData (a -> b), and doing so is generally agreed to be a bad idea, I don't think this library should explicitly forbid them from doing so if a user deems it absolutely necessary.

arybczak commented 1 year ago

Does this mean that it will no longer be possible for data types that have functions as fields to derive NFData generically? If so, the ensuing breakage throughout the ecosystem will most likely be unacceptable.

Isn't deepseq a core library? If so, this requires CLC proposal.

phadej commented 1 year ago

I hesitate to use such "hacks" i

It's a hack only because Unsatisfiable is not implemented.

and doing so is generally agreed to be a bad idea

I don't agree. What people do not agree on is what that instance should do.

If you think, for example, that NFData (a -> b) should traverse the function's closure then you should forbid it now, so when the GHC/RTS adds such support, you can just add it.

As herbert said in the first reply:

Personally, I'd prefer an unimplementable instance, as otherwise all it takes is one orphan instance hidden somewhere in a popular package to thwart this change.


@arybczak

Isn't deepseq a core library? If so, this requires CLC proposal.

Yes, but No. CLC proposals are only needed for changes in base. https://github.com/haskell/core-libraries-committee says

Maintainers of Core Libraries may at their own accord seek CLC approval for controversial changes, but are not required to do so

mixphix commented 1 year ago

I disagree that forbidding the instance from such a deep place in the hierarchy is the path forward, especially if in some later update it will become unforbidden. The lack of the instance provides its own type error:

- instance NFData (a -> b) where rnf = rwhnf
+ -- instance NFData (a -> b) where rnf = rwhnf
ghci> rnf (id :: Int -> Int)

<interactive>:1:1: error:
    • No instance for (NFData (Int -> Int)) arising from a use of ‘rnf’
        (maybe you haven't applied a function to enough arguments?)
    • In the expression: rnf (id :: Int -> Int)
      In an equation for ‘it’: it = rnf (id :: Int -> Int)
phadej commented 1 year ago

@mixphix I repeat

Personally, I'd prefer an unimplementable instance, as otherwise all it takes is one orphan instance hidden somewhere in a popular package to thwart this change.

mixphix commented 1 year ago

That's a bug for that library, then, not this one.

phadej commented 1 year ago

Sure. At least if you decide to add unimplementable instance, that won't be a breaking change.

mixphix commented 1 year ago

@phadej I repeat

Given that I am in the process of updating certain instances to use QuantifiedConstraints, and considering dropping support for old versions of GHC (< 8.6), I think it's right to bump the major version soon.

phadej commented 1 year ago

@mixphix Yes.

Removing instance now altogether is a breaking change, but if later you reconsider to add unimplementable one (e.g. when Unsatisfiable is implemented, for these GHCs), that change won't be breaking one,

cartazio commented 1 year ago

Question: what’s stopping us from writing the c minus minus today to walk the closures?

On Tue, Jan 24, 2023 at 10:15 AM Oleg Grenrus @.***> wrote:

@mixphix https://github.com/mixphix Yes.

Removing instance now altogether is a breaking change, but if later you reconsider to add unimplementable one (e.g. when Unsatisfiable is implemented, for these GHCs), that change won't be breaking one,

— Reply to this email directly, view it on GitHub https://github.com/haskell/deepseq/issues/16#issuecomment-1402112019, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAABBQX6BK2DQBOW2LYFZULWT7W2ZANCNFSM4CERLO7A . You are receiving this because you commented.Message ID: @.***>

phadej commented 1 year ago

Question: what’s stopping us from writing the c minus minus today to walk the closures?

That might be a wrong thing to do. Consider a Vector which is a Array underneath with possibly a bunch of undefined values at the tail. (That's why compacting Vectors can sometimes diverge).

And for traversing closures you don't need a type-class to begin with.

cartazio commented 1 year ago

Oh yeah those are fun bugs. Good point

On Tue, Jan 24, 2023 at 10:49 AM Oleg Grenrus @.***> wrote:

Question: what’s stopping us from writing the c minus minus today to walk the closures?

That might be a wrong thing to do. Consider a Vector which is a SmallArray underneath with possibly a bunch of undefined values at the tail. (That's why compacting Vectors can sometimes diverge).

And for traversing closures you don't need a type-class to begin with.

— Reply to this email directly, view it on GitHub https://github.com/haskell/deepseq/issues/16#issuecomment-1402171344, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAABBQWG62IVP5IFHMT2I4LWT72WXANCNFSM4CERLO7A . You are receiving this because you commented.Message ID: @.***>

tomjaguarpaw commented 1 year ago

Is there a law for NFData which demonstrates that a -> b instance is wrong? The documentation for rnf says

rnf should reduce its argument to normal form (that is, fully evaluate all sub-components), and then return '()'.

But that seems to beg the question. What are "all sub-components"?

phadej commented 1 year ago

@tomjaguarpaw

\x -> fromMaybe (lookup idx bigArray) x

is only a weak head normal form. seq only does as much to evaluate functions until it's a lambda.

The normal form would be

\x -> fromMaybe lookedUpValue x

where lookedUpValue is a result of evaluating lookup idx bigArray


This also another example why walking up closures is hard. The closure might look like

idx bigArray \x ->  fromMaybe (lookup idx bigArray) x

where idx and bigArray are closed over (and lookup is a global name).

Then naively rnfing idx and bigArray will evaluate too much. We'd want to lookup the element evaluate only it, but we cannot as that's hidden in the (static) code of the closure.

TL;DR closure can contain reference to huge structures (when forced) but only need a tiny bit of them.

tomjaguarpaw commented 1 year ago

Sure, I understand what people informally assume NFData (a -> b) to imply, I'm just asking whether that has been stated as a property that instances must obey, with a formal definition? I haven't seen either.

evincarofautumn commented 1 year ago

2¢: I’m for just removing the instance, and I strongly agree with the idea of moving the functionality into the compiler/RTS, making NFData a derived-only or auto-derived class.

Lysxia commented 1 year ago

Is there a law for NFData which demonstrates that a -> b instance is wrong?

"Reduce its argument to normal form" is plenty formal already if interpreted in terms of an operational semantics.

For an denotational specification, maybe this:

if rnf x = () = rnf y
then x = y

for a "=" that is aware of bottoms. This forces x and y to be maximal elements (and thus "not containing bottom"), because if there were another x' > x, then rnf x' = () by monotonicity, so x' = y = x, contradicting x' > x.