GitLab

Marge Bot pushed to branch master at Glasgow Haskell Compiler / GHC

Commits:

cef8938f

by sheaf at 2025-09-17T19:34:09-04:00

Bad record update msg: allow out-of-scope datacons

This commit ensures that, when we encounter an invalid record update
(because no constructor exists which contains all of the record fields
mentioned in the record update), we graciously handle the situation in
which the constructors themselves are not in scope. In that case,
instead of looking up the constructors in the GlobalRdrEnv, directly
look up their GREInfo using the lookupGREInfo function.

Fixes #26391

a2d9d7c2

by sheaf at 2025-09-17T19:34:09-04:00

Improve Notes about disambiguating record updates

This commit updates the notes [Disambiguating record updates] and
[Type-directed record disambiguation], in particular adding more
information about the deprecation status of type-directed disambiguation
of record updates.

6 changed files:

compiler/GHC/Rename/Env.hs
compiler/GHC/Rename/Pat.hs
compiler/GHC/Tc/Gen/Expr.hs
+ testsuite/tests/overloadedrecflds/should_fail/T26391.hs
+ testsuite/tests/overloadedrecflds/should_fail/T26391.stderr
testsuite/tests/overloadedrecflds/should_fail/all.T

Changes:

compiler/GHC/Rename/Env.hs

@@ -112,7 +112,7 @@ import Control.Arrow    ( first )
  import Control.Monad
  import Data.Either      ( partitionEithers )
  import Data.Function    ( on )
 -import Data.List        ( find, partition, groupBy, sortBy )
 +import Data.List        ( find, partition, sortBy )
  import qualified Data.List.NonEmpty as NE
  import qualified Data.Semigroup as Semi
  import System.IO.Unsafe ( unsafePerformIO )
@@ -1455,6 +1455,12 @@ lookupFieldGREs env (L loc rdr)
             do { let (env_fld_gres, env_var_gres) =
                        partition isRecFldGRE $
                        lookupGRE env (LookupRdrName rdr (RelevantGREsFOS WantBoth))
 +                -- Make sure to use 'LookupRdrName': if a record update contains
 +                -- a qualified field name, only look up GREs which are in scope
 +                -- with that same qualification.
 +                --
 +                -- See Wrinkle [Qualified names in record updates]
 +                -- in Note [Disambiguating record updates] in GHC.Rename.Pat.
                -- Handle implicit qualified imports in GHCi. See T10439.
                ; ghci_gres <- lookupQualifiedNameGHCi WantBoth rdr
@@ -1532,10 +1538,10 @@ lookupRecUpdFields :: NE.NonEmpty (LHsRecUpdField GhcPs GhcPs)
                     -> RnM (NE.NonEmpty (HsRecUpdParent GhcRn))
  lookupRecUpdFields flds
  -- See Note [Disambiguating record updates] in GHC.Rename.Pat.
 -  = do { -- Retrieve the possible GlobalRdrElts that each field could refer to.
 +  = do { -- (1) Retrieve the possible GlobalRdrElts that each field could refer to.
         ; gre_env <- getGlobalRdrEnv
         ; fld1_gres NE.:| other_flds_gres <- mapM (lookupFieldGREs gre_env . getFieldUpdLbl) flds
 -         -- Take an intersection: we are only interested in constructors
 +         -- (2) Take an intersection: we are only interested in constructors
           -- which have all of the fields.
         ; let possible_GREs = intersect_by_cons fld1_gres other_flds_gres
@@ -1546,15 +1552,16 @@ lookupRecUpdFields flds
         ; case possible_GREs of
 -          -- There is at least one parent: we can proceed.
 +          -- (3) (a) There is at least one parent: we can proceed.
            -- The typechecker might be able to finish disambiguating.
            -- See Note [Type-directed record disambiguation] in GHC.Rename.Pat.
         { p1:ps -> return (p1 NE.:| ps)
 -          -- There are no possible parents for the record update: compute
 -          -- a minimum set of fields which does not belong to any data constructor,
 -          -- to report an informative error to the user.
 -       ; _ ->
 +          -- (3) (b) There are no possible parents for the record update:
 +          -- compute a minimal set of fields which does not belong to any
 +          -- data constructor, to report an informative error to the user.
 +       ; _ -> do
 +          hsc_env <- getTopEnv
            let
              -- The constructors which have the first field.
              fld1_cons :: UniqSet ConLikeName
@@ -1564,9 +1571,9 @@ lookupRecUpdFields flds
              -- The field labels of the constructors which have the first field.
              fld1_cons_fields :: UniqFM ConLikeName [FieldLabel]
              fld1_cons_fields
 -              = fmap (lkp_con_fields gre_env)
 +              = fmap (lkp_con_fields hsc_env gre_env)
                $ getUniqSet fld1_cons
 -          in failWithTc $ badFieldsUpd (NE.toList flds) fld1_cons_fields } }
 +          failWithTc $ badFieldsUpd (NE.toList flds) fld1_cons_fields } }
    where
      intersect_by_cons :: NE.NonEmpty FieldGlobalRdrElt
@@ -1585,13 +1592,22 @@ lookupRecUpdFields flds
        , not $ isEmptyUniqSet both_cons
+       ]
 -    lkp_con_fields :: GlobalRdrEnv -> ConLikeName -> [FieldLabel]
 -    lkp_con_fields gre_env con =
 +    -- Look up all in-scope fields of a 'ConLike'.
 +    lkp_con_fields :: HscEnv -> GlobalRdrEnv -> ConLikeName -> [FieldLabel]
 +    lkp_con_fields hsc_env gre_env con =
        [ fl
 -      | let nm = conLikeName_Name con
 -      , gre      <- maybeToList $ lookupGRE_Name gre_env nm
 -      , con_info <- maybeToList $ recFieldConLike_maybe gre
 -      , fl       <- conInfoFields con_info ]
 +      | let con_nm = conLikeName_Name con
 +            gre_info =
 +              (greInfo <$> lookupGRE_Name gre_env con_nm)
 +                `orElse`
 +              lookupGREInfo hsc_env con_nm
 +              -- See Wrinkle [Out of scope constructors]
 +              -- in Note [Disambiguating record updates] in GHC.Rename.Pat.
 +      , IAmConLike con_info <- [ gre_info ]
 +      , fl <- conInfoFields con_info
 +      , isJust $ lookupGRE_FieldLabel gre_env fl
 +             -- Ensure the fields are in scope.
 +      ]
  {-**********************************************************************
  *                                                                      *
@@ -1615,8 +1631,9 @@ getUpdFieldLbls
  -- aren't really relevant to the problem.
  --
  -- NB: this error message should only be triggered when all the field names
 --- are in scope (i.e. each individual field name does belong to some
 +-- are in scope. It's OK if the constructors themselves are not in scope
 +-- (see Wrinkle [Out of scope constructors] in Note [Disambiguating record updates]
 +-- in GHC.Rename.Pat).
  badFieldsUpd
    :: (OutputableBndrId p)
    => [LHsRecUpdField (GhcPass p) q]
@@ -1649,7 +1666,7 @@ badFieldsUpd rbinds fld1_cons_fields
              in
              -- Fields that don't change the membership status of the set
              -- are redundant and can be dropped.
 -            map (fst . head) $ groupBy ((==) `on` snd) growingSets
 +            map (fst . NE.head) $ NE.groupBy ((==) `on` snd) growingSets
      aMember = assert (not (null members) ) fst (head members)
      (members, nonMembers) = partition (or . snd) membership

compiler/GHC/Rename/Pat.hs

@@ -1047,25 +1047,90 @@ In a record update, the `lookupRecUpdFields` function tries to determine
  the parent datatype by computing the parents (TyCon/PatSyn) which have
  at least one constructor (DataCon/PatSyn) with all of the fields.
 -For example, in the (non-overloaded) record update
 +To do this, given the (non-empty) set of fields in the record update,
 +lookupRecUpdFields proceeds as follows:
 -    r { fld1 = 3, fld2 = 'x' }
 +  (1) For each field, retrieve all the in-scope GREs that it could possibly
 +      refer to.
 -only the TyCon R contains at least one DataCon which has both of the fields
 -being updated: in this case, MkR1 and MkR2 have both of the updated fields.
 -The TyCon S also has both fields fld1 and fld2, but no single constructor
 -has both of those fields, so S is not a valid parent for this record update.
 +  (2) Take an intersection to compute the possible parent data constructors.
 +      For example, for an update
 -Note that this check is namespace-aware, so that a record update such as
 +        r { fld1 = 3, fld2 = 'x' }
++
 +      the possible parents for each field are:
++
 +        fld1: [MkR1 |-> R.fld1, MkR2 |-> R.fld1, MkS1 |> S.fld1]
 +        fld2: [MkR1 |-> R.fld2, MkR2 |-> R.fld2, MkS2 |> S.fld2]
++
 +      after intersecting by constructor, we get:
++
 +        fld1: [MkR1 |-> R.fld1, MkR2 |-> R.fld1]
 +        fld2: [MkR1 |-> R.fld2, MkR2 |-> R.fld2]
++
 +      This reflects the fact that only the TyCon R contains at least one DataCon
 +      which has both of the fields being updated: MkR1 and MkR2.
 +      The TyCon S also has both fields fld1 and fld2, but no single constructor
 +      has both of those fields, so S is not a valid parent for this record update.
++
 +  (3)
 +    (a)
 +      If there is at least one possible parent TyCon, succeed. The typechecker
 +      might still be able to disambiguate if there remains more than one
 +      candidate parent TyCon (see Note [Type-directed record disambiguation]).
 +    (b)
 +      Otherwise, report an error saying "No constructor has all these fields".
 +      This is the job of GHC.Rename.Env.badFieldsUpd. This function tries
 +      to report a minimal set of fields, so that in a record update like
++
 +        r { fld1 = x1, fld2 = x2, [...], fld99 = x99 }
++
 +      we don't report a massive error message saying "No constructor has all
 +      the fields fld1, ..., fld99" and instead report e.g. "No constructor
 +      has all the fields { fld3, fld17 }".
++
 +Wrinkle [Qualified names in record updates]
++
 +  Note that we must take into account qualified names in (1), so that a record
 +  update such as
      import qualified M ( R (fld1, fld2) )
      f r = r { M.fld1 = 3 }
 -is unambiguous, as only R contains the field fld1 in the M namespace.
 -(See however #22122 for issues relating to the usage of exact Names in
 -record fields.)
 +  is unambiguous: only R contains the field fld1 with the M qualifier.
++
 +  The function that looks up the GREs for the record update is 'lookupFieldGREs',
 +  which uses 'lookupGRE env (LookupRdrName ...)', ensuring that we correctly
 +  filter the GREs with the correct module qualification (with 'pickGREs').
++
 +  (See however #22122 for issues relating to the usage of exact Names in
 +  record fields.)
++
 +Wrinkle [Out of scope constructors]
++
 +  For (3)(b), we have an invalid record update because no constructor has
 +  all of the fields of the record update. The 'badFieldsUpd' then tries to
 +  compute a minimal set of fields which are not children of any single
 +  constructor. The way this is done is explained in
 +  Note [Finding the conflicting fields] in GHC.Rename.Env, but in short that
 +  function needs a mapping from ConLike to all of its fields to do its business.
 +  (You may remark that we did not need such a mapping for step (2).)
++
 +  This means we need to look up each constructor and find its fields; this
 +  information is stored in the GREInfo field of a constructor GRE.
 +  We need this information even if the constructor itself is not in scope, so
 +  we proceed as follows:
++
 +    1. First look up the constructor in the GlobalRdrEnv, using lookupGRE_Name.
 +       This handles constructors defined in the current module being renamed,
 +       as well as in-scope imported constructors.
 +    2. If that fails (e.g. the field is imported but the constructor is not),
 +       then look up the GREInfo of the constructor in the TypeEnv, using
 +       lookupGREInfo. This makes sure we give the right error message even when
 +       the constructors are not in scope (#26391).
 -See also Note [Type-directed record disambiguation] in GHC.Tc.Gen.Expr.
 +    Note that we do need (1), as (2) does not handle constructors defined in the
 +    current module being renamed (as those have not yet been added to the TypeEnv).
  Note [Using PatSyn FreeVars]
  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~

compiler/GHC/Tc/Gen/Expr.hs

@@ -1200,13 +1200,34 @@ Wrinkle [Using IdSig]
  Note [Type-directed record disambiguation]
  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 -GHC currently supports an additional type-directed disambiguation
 -mechanism, which is deprecated and scheduled for removal as part of
 -GHC proposal #366 https://github.com/ghc-proposals/ghc-proposals/blob/master/proposals/0366-no-ambiguous-field-access.rst.
+-
 -To perform this disambiguation, when there are multiple possible parents for
 -a record update, the renamer defers to the typechecker.
 -See GHC.Tc.Gen.Expr.disambiguateRecordBinds, and in particular the auxiliary
 +Deprecation notice:
 +  The type-directed disambiguation mechanism for record updates described in
 +  this Note is deprecated, as per GHC proposal #366 (https://github.com/ghc-proposals/ghc-proposals/blob/master/proposals/0366-no-ambiguous-field-access.rst).
 +  The removal of type-directed disambiguation for record updates is tracked
 +  in GHC ticket #19461, but progress towards this goal has stalled.
++
 +  Why? There are several suggested replacement mechanisms, such as:
 +    1. using module qualification to disambiguate,
 +    2. using OverloadedRecordUpdate for type-directed disambiguation
 +      (as described in Note [Overview of record dot syntax] in GHC.Hs.Expr).
 +  However, these solutions do not work in all situations:
 +    1. Module qualification doesn't work for fields defined in the current module,
 +       nor to disambiguate between constructors of different data family instances
 +       of a given parent data family TyCon.
 +    2. OverloadedRecordUpdate does not allow for type-changing record update,
 +       nor can it deal with fields with existentials or polytypes.
 +  There are also some avenues to improve the renamer's ability to disambiguate:
 +    - GHC ticket #23032 suggests using as-patterns to disambiguate in the renamer.
 +    - GHC proposal https://github.com/ghc-proposals/ghc-proposals/pull/537
 +      suggests a syntactic form of type-directed disambiguation that could be
 +      carried out in the renamer.
 +  Neither of these have been accepted/implemented at the time of writing (Sept 2025).
 +  This means that removal of type-directed disambiguation is currently stalled.
++
 +GHC tries to disambiguate record updates in the renamer, as described in
 +Note [Disambiguating record updates] in GHC.Rename.Pat. However, if the renamer
 +is unable to disambiguate, the renamer will defer to the typechecker: see
 +GHC.Tc.Gen.Expr.disambiguateRecordBinds, and in particular the auxiliary
  function identifyParentLabels, which picks a parent for the record update
  using the following additional mechanisms:

testsuite/tests/overloadedrecflds/should_fail/T26391.hs

 +module T26391 where
++
 +import Data.Semigroup (getSum, getProduct)
++
 +-- This record update is invalid (no constructor has both 'getSum' and 'getProduct').
 +--
 +-- This test makes sure that GHC can handle reporting a good error even when
 +-- the parent constructors (here, Sum and Product) are out of scope.
 +a = undefined { getSum = undefined, getProduct = undefined }

testsuite/tests/overloadedrecflds/should_fail/T26391.stderr

 +T26391.hs:9:5: error: [GHC-14392]
 +    Invalid record update.
 +    No constructor in scope has all of the following fields:
 +      ‘getSum’, ‘getProduct’
++

testsuite/tests/overloadedrecflds/should_fail/all.T

@@ -39,6 +39,7 @@ test('T17420', [extra_files(['T17420A.hs'])], multimod_compile_fail,
  test('T17469', [extra_files(['T17469A.hs'])], multimod_compile_fail,
       ['T17469', ''])
  test('T17965', normal, compile_fail, [''])
 +test('T26391', normal, compile_fail, [''])
  test('DRFHoleFits', extra_files(['DRFHoleFits_A.hs']), multimod_compile_fail, ['DRFHoleFits', '-v0'])
  test('DRFPartialFields', normal, compile_fail, [''])
  test('T16745', extra_files(['T16745C.hs', 'T16745B.hs']), multimod_compile_fail, ['T16745A', ''])