"X has fields" error doesn't tell me what the problem is (`-XOverloadedRecordDot`)

[ocharles]

I have the following:

instance HasField x a b => HasField (x :: Symbol) (Behavior a) (Behavior b) where
  getField behavior = behavior <&> getField @x

Yet when I compile this I get

lib/Reactive/Banana/Orphans.hs:36:28: error:
    • Illegal instance declaration for
        ‘HasField x (Behavior a) (Behavior b)’
        Behavior has fields
    • In the instance declaration for
        ‘HasField (x :: Symbol) (Behavior a) (Behavior b)’
   |
36 | instance HasField x a b => HasField (x :: Symbol) (Behavior a) (Behavior b) where
   |                            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

which just leaves me confused.

Why is the instance illegal? "Behavior has fields" - yes, and? Is that a problem? Also Behavior doesn't have even fields - it's an opaque type!

[goldfirere]

I'm not sure whether your questions are hypothetical ("someone might wonder ...") or real ("@ocharles doesn't know whether ..."). And so I will answer the questions and then muse about a better message.

Why is the instance illegal? Assuming Behavior has fields: Because the instance might overlap with existing instances created for those fields. As it turns out, Behavior does have fields, namely one called unB. Given that there's nothing stopping the T in a Behavior T from having an unB field, the declared instance might overlap with the existing one, and so GHC rejects it.

It's tempting to say that the opaqueness of Behavior matters, but I think GHC is correct in saying that it doesn't. There might be a downstream module that sees both your instance and has access to Behaviors constructor and field. Maybe GHC could plausibly track whether a constructor is even exported from its birth module and then use that to determine opaqueness, but that actually wouldn't work here: the declaring module for Behavior does export the constructor. So instead GHC could see whether a package exports a constructor from any of its exposed modules and consider the type opaque outside that package. But even that more complex analysis wouldn't be enough, because the constructor is exported from the package in Reactive.Banana.Model. So the possibility of overlap is quite real. I really don't know how to do better here.

---

The error message could be improved. Here is an attempt:

lib/Reactive/Banana/Orphans.hs:36:28: error:
    • Illegal instance declaration for
        ‘HasField x (Behavior a) (Behavior b)’
        Behavior was declared with fields, and this instance might overlap
        NB: GHC synthesizes HasField instances for all record fields
    • In the instance declaration for
        ‘HasField (x :: Symbol) (Behavior a) (Behavior b)’
   |
36 | instance HasField x a b => HasField (x :: Symbol) (Behavior a) (Behavior b) where
   |       

Note "was declared with", sidestepping the opaqueness issue. The NB gives just a little more background that is hopefully enough to digest the error.

You might argue that this HasField mechanism is now a glaring abstraction-leak: a library author who wants a type to be abstract is now leaking their implementation details. Downstream users can detect (by trying to write a HasField instance) which abstract types are actually records under the hood. Yes, it is a leak! Maybe an alternative design would be better here, but it is what we have for the moment.

[ocharles]

Thanks for your reply @goldfirere. I was genuinely confused (because to me, Behavior has no fields, and infact despite being a maintainer of the library, I didn't even know that newtype had unB until I looked!). This "has no fields" restriction is a different kettle of fish, but I'm chatting more about that in https://gitlab.haskell.org/ghc/ghc/-/issues/21324. I'll leave my reply to your points out for now, and if anything we can continue in the GHC issue.

I wanted this issue to be more about the error message itself. What you propose is just what I'd like to see. An error message should tell me what is wrong, briefly tell me why it's wrong, and if possible, what I can do to resolve it. In this case, there's not much I can do to resolve it, but at least explaining what the restriction is would help me a bit more. I would also go further and say it could tell at least a few of the field names, though that makes a leaky abstraction yet more leaky - I'm not sure if we care about that. But I do think being told "Behavior has fields (unB) but HasField` instances can only be defined on types with no fields" would have lead me to immediately understand the problem.

[goldfirere]

OK. In light of the last comment, let me revise the error message a bit:

lib/Reactive/Banana/Orphans.hs:36:28: error:
    • Illegal instance declaration for
        ‘HasField x (Behavior a) (Behavior b)’
        Behavior was declared with fields; manual HasField instances are
        not allowed for types with fields so as to avoid overlap with GHC's
        synthesized instances.
    • In the instance declaration for
        ‘HasField (x :: Symbol) (Behavior a) (Behavior b)’
   |
36 | instance HasField x a b => HasField (x :: Symbol) (Behavior a) (Behavior b) where
   |       

Is that better? Worse? Too wordy?

[vanceism7]

OK. In light of the last comment, let me revise the error message a bit:

lib/Reactive/Banana/Orphans.hs:36:28: error:
    • Illegal instance declaration for
        ‘HasField x (Behavior a) (Behavior b)’
        Behavior was declared with fields; manual HasField instances are
        not allowed for types with fields so as to avoid overlap with GHC's
        synthesized instances.
    • In the instance declaration for
        ‘HasField (x :: Symbol) (Behavior a) (Behavior b)’
   |
36 | instance HasField x a b => HasField (x :: Symbol) (Behavior a) (Behavior b) where
   |       

Is that better? Worse? Too wordy?

For what it's worth, I think this revision of the error message is great. It clearly explains the issue 👍