Opened 22 months ago

Last modified 10 months ago

#14848 new bug

-XDuplicateRecordFields breaks record expression splices

Reported by: dailectic Owned by: adamgundry
Priority: normal Milestone:
Component: Compiler Version: 8.2.2
Keywords: ORF Cc: adamgundry
Operating System: Linux Architecture: x86_64 (amd64)
Type of failure: GHC rejects valid program Test Case:
Blocked By: Blocking:
Related Tickets: #11103 Differential Rev(s):
Wiki Page:

Description

{-# language TemplateHaskell #-}
{-# language DuplicateRecordFields #-}
module Lib where
import Language.Haskell.TH
import Language.Haskell.TH.Syntax

data A = A {x :: Int, y :: String}
a = A 3 "test"
test = $([e|case a of A {x = b} -> b|])

Without DuplicateRecordFields it compiles correctly so test = 3 but with DuplicateRecordFields enabled it gives:

     • Illegal variable name: ‘$sel:x:A’
      When splicing a TH expression:
        case Lib.a of
    (Lib.A {Lib.$sel:x:A = b_0}) -> b_0
    • In the untyped splice: $([| case a of { A {x = b} -> b } |])

Additionally, there doesn't seem to be a workaround for munging the name manually, since the $sel:x:A name is the one actually in scope, there is no A.x like there would be normally, even when the label is not a duplicate.

Is there a way to get around this? Ex: by changing the binding name somehow manually?

Change History (5)

comment:1 Changed 22 months ago by RyanGlScott

Keywords: ORF added

comment:2 Changed 22 months ago by adamgundry

Cc: adamgundry added

comment:3 Changed 21 months ago by adamgundry

I think a possible fix here is to apply the approach described in Note [Reifying field labels] from TcSplice in the HsRecFld case of DsMeta.repE. That is, rather than representing the field as a NameG containing the mangled selector name (which isn't a legal variable name, let alone in scope), we could represent it as a NameQ containing the field label.

comment:4 Changed 10 months ago by adamgundry

I've just rediscovered this issue, and the interaction between DuplicateRecordFields and TemplateHaskell definitely needs to be addressed more carefully.

The problem here is the case for HsRecFld in DsMeta.repE, which simply replaces the field occurrence with the selector function (which has an internal name that cannot be spliced in, namely Name "$sel:x:A" (NameG VarName pkg "Lib")). There are other related problems in DsMeta for representing fields in record definition, construction and pattern matching.

The way #11103 dealt with a similar problem in reification was to represent field names using a specially-crafted NameQ, rather than a NameG. In this example that approach would give Name "x" (NameQ "Lib") instead. Note that NameG cannot be used with the field label in place of the selector, because that leads to looking up an Orig name that doesn't exist.

However, while using NameQ should fix this example, it doesn't work in general: it means the name is dynamically bound, but of course the field might not be in scope when the splice is run. For a concrete example of this, consider a tiny variant of the test case from #12993:

{-# LANGUAGE DuplicateRecordFields #-}
{-# LANGUAGE TemplateHaskell #-}
module T12993_Lib (q) where
data X = X { x :: Int }
q = [|x|]
{-# LANGUAGE TemplateHaskell #-}
module T12993 where
import T12993_Lib
f = $(q)

This currently fails with an "Illegal variable name" error similar to the original report from this ticket. With the NameQ approach, it would still fail because x is not in scope in T12993.

It seems fairly clear to me that dealing with this properly requires extending the TH AST in some way. The least invasive option might be to extend NameFlavour with a new constructor for field names, which would be rather like NameG but would carry the selector name. I'm not sure if there are better designs, however. What is the process for proposing/discussing such changes to TH?

comment:5 Changed 10 months ago by adamgundry

Owner: set to adamgundry

The least invasive option might be to extend NameFlavour with a new constructor for field names, which would be rather like NameG but would carry the selector name.

I've tried this, and it looks like a relatively straightforward change (although we may need an equivalent to NameL as well, I'm not sure yet). Will propose a patch.

Note: See TracTickets for help on using tickets.