Opened 2 weeks ago

Last modified 10 days ago

#14630 new bug

name shadowing warnings by record pattern synonyms + RecordWildCards or NamedFieldPuns

Reported by: mizunashi_mana Owned by:
Priority: normal Milestone:
Component: Compiler Version: 8.2.2
Keywords: PatternSynonyms Cc: adamgundry, mpickering
Operating System: Unknown/Multiple Architecture: Unknown/Multiple
Type of failure: Incorrect error/warning at compile-time Test Case:
Blocked By: Blocking:
Related Tickets: #11228 Differential Rev(s):
Wiki Page:

Description

When I use PatternSynonyms + RecordWildCards/NamedFieldPuns, I get name shadowing warnings. I am hoping that these warnings don't trigger in the below case.

{-# LANGUAGE PatternSynonyms #-}
{-# LANGUAGE RecordWildCards #-}
{-# LANGUAGE NamedFieldPuns  #-}

module TestPatternSynonyms where

pattern Tuple :: a -> b -> (a, b)
pattern Tuple{x, y} = (x, y)

{-# COMPLETE Tuple #-}

f :: (a, b) -> a
f Tuple{x} = x
{- warning: [-Wname-shadowing]
    This binding for ‘x’ shadows the existing binding
-}

g :: (Int, Int) -> Int
g Tuple{..} = x + y
{- warning: [-Wname-shadowing]
    This binding for ‘x’ shadows the existing binding 
    This binding for ‘y’ shadows the existing binding
-}

Change History (5)

comment:1 Changed 2 weeks ago by mizunashi_mana

Type of failure: None/UnknownIncorrect error/warning at compile-time

comment:2 Changed 2 weeks ago by RyanGlScott

Hrm, I'm not sure how to fix this. The issues lies with the is_shadowed_gre function in RnUtils, defined here:

is_shadowed_gre :: GlobalRdrElt -> RnM Bool
    -- Returns False for record selectors that are shadowed, when
    -- punning or wild-cards are on (cf Trac #2723)
is_shadowed_gre gre | isRecFldGRE gre
    = do { dflags <- getDynFlags
         ; return $ not (xopt LangExt.RecordPuns dflags
                         || xopt LangExt.RecordWildCards dflags) }
is_shadowed_gre _other = return True

This uses the isRecFldGRE function to detect record selectors, which is in turn defined as follows:

isRecFldGRE :: GlobalRdrElt -> Bool
isRecFldGRE (GRE {gre_par = FldParent{}}) = True
isRecFldGRE _                             = False

The problem is that pattern synonym record selectors don't use FldParent as their Parent, but rather NoParent. At first, I thought this might have been an oversight, but it turns out there's a reason for this, as explained in this comment:

Record pattern synonym selectors are treated differently. Their parent information is NoParent in the module in which they are defined. This is because a pattern synonym P has no parent constructor either.

So it seems that we need to adjust isRecFldGRE to be aware of this fact somehow. But I doubt that having isRecFldGRE return True whenever it sees any occurrence of NoParent is the right thing to do... any ideas?

comment:3 Changed 12 days ago by RyanGlScott

This is likely related in nature to #11228 (Interaction between ORF and record pattern synonyms needs to be resolved.), since FldParent is used to disambiguate duplicate record fields in the presence of DuplicateRecordFields. This means that this program will not compile:

{-# LANGUAGE DuplicateRecordFields #-}
{-# LANGUAGE PatternSynonyms #-}
module Bug where

pattern Foo :: Int -> Int -> (Int, Int)
pattern Foo {x, y} = (x, y)

pattern Bar :: Int -> Int -> (Int, Int)
pattern Bar {x, y} = (x, y)

Whereas if x and y were normal record selectors from a data type constructor, GHC would accept them.

comment:4 Changed 11 days ago by simonpj

Cc: adamgundry added

Adam Gundry may want to comment.

comment:5 Changed 10 days ago by adamgundry

Cc: mpickering added

I agree with Ryan's analysis. Now that we have pattern synoyms with record fields, I think we need to disentangle two aspects of Parent that are now orthogonal:

  • whether the Name has a parent at all (this can change due to pattern-synonym bundling)
  • whether the Name is a record field, and if so, its label (this shouldn't change)

For example, we could add a constructor to Parent that has a Maybe FieldLabelString but not a parent Name. This ought to be enough to make isRecFldGRE accurate and hence calculate the name shadowing warnings correctly (fixing this ticket).

However, this isn't quite enough for #11228, for which there are other complications (see discussion on https://github.com/ghc-proposals/ghc-proposals/pull/84).

Note: See TracTickets for help on using tickets.