deepseq icon indicating copy to clipboard operation
deepseq copied to clipboard

Generic 'NFData' instance breaks with embedded function space (regression in GHC 9.10.3)

Open andreasabel opened this issue 9 months ago • 14 comments

Deriving NFData from Generic does not work any longer if a constructor embeds function spaces. https://github.com/agda/agda/blob/9fa8910e340cf5b860f96b817516914f87054364/src/full/Agda/Utils/Benchmark.hs#L47-L48 https://github.com/agda/agda/blob/9fa8910e340cf5b860f96b817516914f87054364/src/full/Agda/Utils/Benchmark.hs#L247

data BenchmarkOn a = BenchmarkOff | BenchmarkOn | BenchmarkSome (Account a -> Bool)
  deriving Generic
instance NFData a => NFData (BenchmarkOn a)
src/full/Agda/Utils/Benchmark.hs:247:10: error: [GHC-68441] [-Wdeprecations, -Werror=deprecations]
    In the use of
      instance NFData (a -> b) -- Defined in ‘Control.DeepSeq’
    arising from a use of ‘Control.DeepSeq.$dmrnf’
    Deprecated: "NFData is not well-defined on function types. https://github.com/haskell/deepseq/issues/16"
    |
247 | instance NFData a => NFData (BenchmarkOn a)
    |          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Could you export instead an instance NFData (a -> b) that does nothing?

What would one anyway expect from normalizing a function? It is the same function again, because we cannot look into the black box.

I think keeping instance NFData (a -> b) would be a better migration path, in the light that most of NFData instances are probably derived generically.

Ref:

  • #16
  • #109 (@L0neGamer @mixphix)

andreasabel avatar Jul 31 '25 07:07 andreasabel

Churn caused on Agda:

  • https://github.com/agda/agda/pull/8041

andreasabel avatar Jul 31 '25 09:07 andreasabel

I can see one would want a warning when manually defining a NFData instance and calling rnf on a function. But deriving the instance from Generic should do the sensible thing: skip functions in the normalization.

andreasabel avatar Jul 31 '25 09:07 andreasabel

For what it's worth I think that automatically no-opping when we get a function to deepseq is the wrong move. We want to avoid thunks in data structures, and generic instances that permit functions allow people to accidentally do allow thunks. If we allow them in such cases, we end up like cases like these: https://github.com/haskell/deepseq/issues/16#issuecomment-1405321124.

Up to the maintainer what to do, of course.

Maybe we add a specific "deriving via" new type that allows end users to derive instances and then ignore functions? It wouldn't help existing breakage but allows easy derivations for users who are willing to take that risk going into the future.

L0neGamer avatar Jul 31 '25 09:07 L0neGamer

We want to avoid thunks in data structures, and generic instances that permit functions allow people to accidentally do allow thunks.

Why don't you want to leave this choice to the developers whether they want thunks in their data structures or not?

If we allow them in such cases, we end up like cases like these: #16 (comment). @int-index wrote:

Has anyone ever actually had a real bug or problem due to NFData (a -> b)? I had. I assumed that deepseq would bring my data to normal form, and I relied on this to guarantee timely release of resources (with lazy IO). Before you start bashing lazy IO, I want to point out that my design would have worked if not for this instance and would allow me to make large amount of business logic in the application non-monadic.

This is a single anecdote in an issue that was discussed very controversially.

Sorry, if you have functions embedded in your data and you expect some magic that will normalize the functions, then you have not understood much about programming languages and their semantics. Functions are infinite objects that must be treated as black boxes. Everything else can only be a terrible hack. So normalization of functions does naturally nothing, and that is implemented in the (now deprecated) NFData (a -> b) instance, and this is the sensible implementation.

andreasabel avatar Jul 31 '25 10:07 andreasabel

It sounds sensible to have a deriving via that allows this.

I think having the default behavior be that we assume functions are whnf = nf is a footgun exactly because most users aren't PL experts and will expect calling nf will get rid of all thunks.

At the very least we should have a warning to say that this is very likely not what you intended.

TeofilC avatar Jul 31 '25 10:07 TeofilC

Out of interest, why does BenchmarkOn even have an NFData instance?

instance NFData a => NFData (BenchmarkOn a)

Having NFData a in the antecedent here seems suspect considering it's not used in the instance.

TeofilC avatar Jul 31 '25 10:07 TeofilC

Sorry, if you have functions embedded in your data and you expect some magic that will normalize the functions, then you have not understood much about programming languages and their semantics.

So it's not possible to normalize the functions? Fine, they don't get an NFData instance then.

int-index avatar Jul 31 '25 14:07 int-index

So it's not possible to normalize the functions? Fine, they don't get an NFData instance then.

They are already in normal form, so this is why the identity is sufficient and always works.

andreasabel avatar Aug 02 '25 13:08 andreasabel

So it's not possible to normalize the functions? Fine, they don't get an NFData instance then.

They are already in normal form, so this is why the identity is sufficient and always works.

Of course. Any function is already in normal form, so in this sense rnf fn = fn `seq` () is correct. The problem is it is incorrect for the use cases that the library is advertised for.

Exhibit 1. The package description says:

Deep evaluation is often used for adding strictness to a program, e.g. in order to force pending exceptions, remove space leaks, or force lazy I/O to happen.

Exhibit 2. The very first example from the documentation is this:

A typical use is to prevent resource leaks in lazy IO programs, by forcing all characters from a file to be read. For example:

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

Note: The example above should rather be written in terms of bracket to ensure releasing file-descriptors in a timely matter


Please consider the implications of this. Now the user expects deepseq to be useful/usable in conjunction with lazy I/O. And why is lazy I/O used? Because the file is too large to load it into memory all at once.

So in practice, instead of immediately forcing the contents with evaluate (rnf s), one would do something closer to

readFile' :: FilePath -> IO (Maybe Int)
readFile' fn =
  withFile fn ReadMode $ \h -> do
    s <- hGetContents h
    let r = f s
    evaluate (rnf r)
    return r

where f is some function that processes the contents of the file. And that works great, as advertised! So the useful pattern:

  1. Read a large file with lazy IO
  2. Process it with some function f to produce a result that fits in memory
  3. Force the result with evaluate and rnf to ensure no lingering references to the file handle
  4. Close the file

The problem is that it falls apart if the result of f contains functions, which means they may capture a reference to the file handle h in their closure, which in turn results in an exception

Main.hs: test.txt: hGetContents: illegal operation (delayed read on closed handle)

It is quite unhaskelly to use a library API as described in documentation, get your program past the type checker, only to see a runtime exception. I don't use Haskell because I'm smart, I use it because I'm dumb and I want the compiler to hold my hand and avoid silly mistakes.

There are three ways I see out of this situation:

  1. Remove the NFData (a -> b) instance.
  2. Use "magic" in the NFData (a -> b) to traverse its closure. Note that the "magic" is fully consistent with the literature definition of a normal form. Indeed, any function is and remains in normal form, it's just that our implementation of rnf could give a stronger guarantee.
  3. Be very loud in the documentation about the NFData (a -> b) footgun.

int-index avatar Aug 02 '25 19:08 int-index

Thanks for the detailed explanation, @int-index !

I guess it boils down on whether you expect that rnf distributes over function application, i.e., whether rnf (f s) = rnf f $ rnf s. One might be tricked to believing this by "textual substitution", but if one thinks about the nature of functions and applications one may realize that this law is not to be expected. The reason is that an application f s, even though I see two distinct syntactic things f and s there, can semantically not be split into two values f and s. Application is irreversible.
Shouldn't one expect the user to know this? Shifting away from deepseq to another function, say show, one would not expect to be able to write a Show instance such that show (f s) = show f ++ " applied to " ++ show s. Once you throw an argument to function it is unreachable and it is at the mercy of the function what happens with the argument. So, if you pass some lazy data to a function, it is not reasonable to expect that you can "strictify" the data after the fact.

rnf forces lazy ground data only, removing thunks in ground data, that is, types of order 0. Higher-order data (functions) cannot be forced as they need arguments to run.

For my use case, I want generically derived NFData instances for all my data structures that do the job of forcing all the ground data in it, even if the structure also contains some higher-order data. Without a NFData (a -> b) instance, the generic derivation breaks. This is bad for my case.

If you want instance NFData D as an oracle of whether D contains higher-order data, then you do not want NFData (a -> b). But the question is whether NFData promises to be such an oracle.

There are three ways I see out of this situation:

  1. Remove the NFData (a -> b) instance.

Users can work around this by adding an orphan instance NFData (a -> b) back. However, orphan instances are a can of worms themselves. It is likely that some package will export it if they export NFData instances for types with embedded function spaces. And then we are worse off than before. The problem is that you cannot control the export of instances.
You could maybe create a package deepseq-fun with just that orphan instance and hope everyone will use this package rather than defining it on their own.
But even then you might unknowingly import this instance via a dependency that uses deepseq-fun, and again you cannot use derived NFData as an oracle of whether you have embedded function spaces.

Removing the instance is basically saying "this instance is bad for everyone", which is not true.

  1. Use "magic" in the NFData (a -> b) to traverse its closure. Note that the "magic" is fully consistent with the literature definition of a normal form. Indeed, any function is and remains in normal form, it's just that our implementation of rnf could give a stronger guarantee.

I would not be opposed to this, but I could not implement this myself. It would still be a bit brittle, I suppose, because it would depend on how the evaluation of application is implemented, and other compilers could make different decisions, not allowing the magic that GHC offers.

  1. Be very loud in the documentation about the NFData (a -> b) footgun.

+1.

As a compromise, one could also attach a custom (x-...) warning (rather than the generic deprecations one) that users could deactivate without silencing all deprecation warnings. The warning, showing up in generically derived instance NFData as well, could explain the problem with the NFData (a -> b) instance rather than just saying it is deprecated. This would be a compromise: you explain to the user that there are problems with the instance, but do not make the judgement that it is bad for everyone.

andreasabel avatar Aug 09 '25 07:08 andreasabel

Such a custom warning would still break your flow and require changes if you have -Wall -Werror. And it would also mean that if someone does want a fully ground-data strict'd type they would have to make sure that their dependencies don't just ignore the warnings, as is common. If we were now introducing this instance, it would be obvious that this is bad behaviour and thus a warning should dissuade all but the knowers from using it, but we're dealing with the instance already existing and being used throughout the ecosystem.

The dependency issue matters less than I outline above, since a determined dependency can just write completely incorrect instances if they feel like it. But the "derivation does what is expected by default" way forward means not having this instance.

Would a suitable alternative for you be a generic deriving newtype that skips rnf'ing functions?

L0neGamer avatar Aug 09 '25 07:08 L0neGamer

Would a suitable alternative for you be a generic deriving newtype that skips rnf'ing functions?

What is exactly meant by "generic deriving newtype"?

andreasabel avatar Aug 10 '25 12:08 andreasabel

rnf forces lazy ground data only, removing thunks in ground data, that is, types of order 0. Higher-order data (functions) cannot be forced as they need arguments to run.

@andreasabel That's a nice way to put it. Suppose we had

class NFData a where
  type GroundData a :: Bool
  rnf :: a -> ()

rnfGroundData :: (NFData a, GroundData a ~ True) => a -> ()
rnfGroundData = rnf

Then the documentation could explain that only rnfGroundData provides the guarantee that some users might expect.

Of course, the problem is how to derive GroundData without breaking existing code. I don't know if generic deriving can handle it. Maybe it can?

  • For functions, define GroundData (a -> b) = False
  • For ADTs, GroundData T = And (FieldsOf T), where And is promoted and :: [Bool] -> Bool, and FieldsOf traverses the generic Rep to collect all field types.

int-index avatar Aug 10 '25 13:08 int-index

@andreasabel like so: https://github.com/haskell/deepseq/pull/112

L0neGamer avatar Aug 10 '25 17:08 L0neGamer