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

Generic instance does not work for single-constructor-single-argument types #24

Open
edsko opened this issue Dec 13, 2022 · 4 comments · May be fixed by #25
Open

Generic instance does not work for single-constructor-single-argument types #24

edsko opened this issue Dec 13, 2022 · 4 comments · May be fixed by #25

Comments

@edsko
Copy link
Contributor

edsko commented Dec 13, 2022

The default definition of wNoThunks currently is

  wNoThunks ctxt x = gwNoThunks (Proxy @'[]) ctxt fp
    where
      -- Force the result of @from@ to WHNF: we are not interested in thunks
      -- that arise from the translation to the generic representation.
      fp :: Rep a x
      !fp = from x

The problem is with the bang on the fp, comment notwithstanding. First, let's consider the case of a datatype with a single constructor with two fields:

data T = MkT Int Int

Evaluation would proceed something like this:

   gwNoThunks .. $! from (MkT x y)
== gwNoThunks .. $! case MkT x y of MkT x y -> M1 (K x :*: K y)
== gwNoThunks .. $! M1 (K x :*: K y)
== gwNoThunks .. $  (K x :*: K y)
== allNoThunks [gwNoThunks .. $ K x, gwNoThunks .. $ K y]
== allNoThunks [noThunks .. $ x, noThunks .. $ y]

In this case the $! actually is not making any difference: the instance for products will pattern match on the :*: constructor, which will force the evaluation of the case MkT .. anyway.

The case for a datatype with multiple constructors is similar:

data T = MkA Int | MkB Int

Now evaluation proceeds something like this:

   gwNoThunks .. $! from (MkA x)
== gwNoThunks .. $! case MkA x of MkA x -> M1 (L1 (K x))
                                  MkB x -> ..
== gwNoThunks .. $! M1 (L1 (K x))
== gwNoThunks .. $  L1 (K x)
== gwNoThunks .. $ K x
== noThunks   .. $ x

Again, the $! makes no difference here: the pattern match on L1 in the instance for sums will force the case MkA x .. anyway.

But now consider the case for a datatype with a single constructor and a single field:

data T = MkT Int

Now we are doomed if we do and doomed if we don't. In the library as it stands, with the $!, we get

   gwNoThunks $! from (MkT x)
== gwNoThunks $! case MkT x of MkT x -> M1 (K x)
== gwNoThunks $! M1 (K x)
== ..

Notice what is happening here: since the x is not protected by any constructor (both M1 and K are newtypes), this will end up forcing x. This is bad, because nothunks should not result in any evaluation happening. But we cannot remove the ! either; if we do, we get something like

   gwNoThunks $ from (MkT x)
== gwNoThunks $ case MkT x of MkT x -> M1 (K x)
== ..
== noThunks (case MkT x of MkT x -> ..)

In other words, we will report thunks when there aren't any.

I'm not entirely sure what the right solution is here, or if there even is one. Ideally the K constructor from GHC generics would not be a newtype; that would solve the problem, but is of course hardly a practical solution.

I think for now we should probably just document that the generic instance does not work for single-constructer-single-argument types. Whether or not that means changing the code (removing the bang in the definition of fp) I'm not totally sure about. As it stands, nothunks may force some evaluation to happen for such types, but will not result in confusing error messages. If we remove it, then using the generic instance for such types will result in thunks being found where there aren't really any, but at least you would be aware that something is amiss. Would appreciate an independent opinion here.

@edsko
Copy link
Contributor Author

edsko commented Dec 13, 2022

@kosmikus suggested a possible way out here; I will try and if it works, will submit a PR.

@michaelpj
Copy link

I think possibly something has got lazier and this is now biting on single-constructor multi-field types? We are upgrading our codebase to GHC 9.6 and we got an unexpected nothunks error in a single-constructor two-field record type with StrictData. I changed the instance from the Generic one to an explicit one with

wNoThunks ctx (T f1 f2) = allNoThunks [ noThunks ctx f1, noThunks ctx f2 ]

and the error went away. I don't have a small reproducer yet, sorry, but it does seem like it has something to do with the generic conversion.

@effectfully
Copy link
Member

effectfully commented May 16, 2023

@michaelpj I doubt it's that. We have other places where NoThunks is derived for a data type with two strict fields just fine.

What looks suspicious is that in the offending case f2 is a single-constructor data type having a custom (i.e. not derived) NoThunks instance. Although that should evaluate exactly like specified in the description of the ticket? I.e. like this:

<...>
== gwNoThunks .. $  (K x :*: K y)
== allNoThunks [gwNoThunks .. $ K x, gwNoThunks .. $ K y]
== allNoThunks [noThunks .. $ x, noThunks .. $ y]

Not sure.

@michaelpj
Copy link

If that was it then I don't know why changing the instance definition for T helps 🤷

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants