Opened 11 days ago

Last modified 32 hours ago

#15149 new bug

Identical distinct type family fields miscompiled

Reported by: NeilMitchell Owned by:
Priority: normal Milestone: 8.6.1
Component: Compiler Version: 8.4.2
Keywords: ORF Cc: ndmitchell@…, adamgundry
Operating System: Unknown/Multiple Architecture: Unknown/Multiple
Type of failure: GHC rejects valid program Test Case:
Blocked By: Blocking:
Related Tickets: Differential Rev(s):
Wiki Page:

Description

Given the code:

-- An.hs
{-# LANGUAGE TypeFamilies #-}
module An where
data family An c :: *

-- AnInt.hs
{-# LANGUAGE TypeFamilies #-}
module AnInt where
import An
data instance An Int = AnInt {an :: Int} deriving Show

-- AnDouble.hs
{-# LANGUAGE TypeFamilies #-}
module AnDouble where
import An
data instance An Double = AnDouble {an :: Double} deriving Show

-- Main.hs
{-# LANGUAGE DisambiguateRecordFields #-}
module Main where
import AnInt
import AnDouble
main = print (AnDouble{an=1}, AnInt{an=1})

I would expect this code to work. In reality it fails at runtime with GHC 8.2.2:

Main.hs:4:15-28: warning: [-Wmissing-fields]
    * Fields of `AnDouble' not initialised: an
    * In the expression: AnDouble {an = 1}
      In the first argument of `print', namely
        `(AnDouble {an = 1}, AnInt {an = 1})'
      In the expression: print (AnDouble {an = 1}, AnInt {an = 1})
  |
6 | main = print (AnDouble{an=1}, AnInt{an=1})
  |               ^^^^^^^^^^^^^^

*** Exception: Main.hs:4:15-28: Missing field in record construction an

And fails at compile time in GHC 8.4.2:

Main.hs:4:31-41: error:
    * Constructor `AnInt' does not have field `an'
    * In the expression: AnInt {an = 1}
      In the first argument of `print', namely
        `(AnDouble {an = 1}, AnInt {an = 1})'
      In the expression: print (AnDouble {an = 1}, AnInt {an = 1})
  |
6 | main = print (AnDouble{an=1}, AnInt{an=1})
  |                               ^^^^^^^^^^^

This code was extracted from a real example, where this bug is pretty fatal, as I haven't been able to find any workarounds (without just avoiding clashing record fields).

Change History (3)

comment:1 Changed 11 days ago by RyanGlScott

Keywords: ORF added

comment:2 Changed 35 hours ago by simonpj

Cc: adamgundry added

I investigated. Here is what is going on:

  • When compiling module An, there are two an's in scope
  • You might resaonably think that with DisamgiguateRecordFields, the an in AnInt { an = 1 } is obviously the an from module AnInt.
  • But in fact GHC's renamer uses the type constructor, not the data constructor to disambiguate which an you mean. And both an's have the same "parent", namely the type constructor An.

This was fine before data families, because a single type constructor could not have data constructors with distinct an record fields. But now it can.

A short term fix is to complain about ambiguity rather than arbitrarily picking one (as is done now). This happens here in TcExpr:

tcRecordField con_like flds_w_tys (L loc (FieldOcc sel_name lbl)) rhs
  | Just field_ty <- assocMaybe flds_w_tys sel_name
  = ...

That assocMaybe just picks the first. So we could fix that.

But how can we fix it properly? After all, all the information is there, staring us in the face.

  • One way forward might be to say that the data constructor AnInt is the "parent" of AnInt.an, and the type constructor An is the parent of the data constructor AnInt. But that change would mean we had a multi-level parent structure, with consequences that are hard to foresee.
  • But I think a better way is to stop trying to make the choice in the renamer. Instead in a HsRecordFields structure, keep the field names as not-yet-looked-up OccNames in the renamer, and look them up in the typechecker. At that point, we have the data constructor available in its full glory and don't need to bother with the tricky parent structures in the renamer.

Adam, do you think that would work?

comment:3 Changed 32 hours ago by adamgundry

I think this would work, and would amount to eliminating the distinction between HsRecField (containing FieldOcc) and HsRecUpdField (containing AmbiguousFieldOcc). At the moment, the former is used for record construction and pattern matching (fields always resolved by the renamer), while the latter is used for selection and update (ambiguous fields resolved by the type-checker). Here "ambiguous" really means "ambiguous from the renamer's perspective, but not necessarily the type-checker".

It would probably be simpler to defer all field resolution to the type-checker, though we've also considered going in the opposite direction, and moving all field resolution back to the renamer (see https://github.com/ghc-proposals/ghc-proposals/pull/84). I'm not really sure which is better!

There's an awkward corner if we were to defer all field resolution to the type-checker, which is that Ambiguous fields are not currently supported by DsMeta. That is, using them inside a TH bracket will fail with a "not supported" error. The basic problem is that DsMeta works on renamed syntax, even though it runs after type-checking. Would that be difficult to change?

Note: See TracTickets for help on using tickets.