-ddump-minimal-imports does not dump some pattern imports correctly

added Tbug needs triage labels

Some further observations. https://hackage.haskell.org/package/ghc-8.10.1/docs/src/GHC.Hs.ImpExp.html#pprImpExp contains the code for outputting imports and exports. Which looks like it should do the right thing. The pprImpExp function doesn't have a pattern case but it no longer seems to be used.

Earlier versions of ghc (eg, https://downloads.haskell.org/~ghc/7.6.1/docs/html/libraries/ghc-7.6.1/src/HsImpExp.html#pprImpExp) used pprImpExp in the IEThingAbs, IEThingAll and IEThingWith cases (but presumably they didn't have patterns at that time).

I realise that I am clutching at straws, but perhaps someone with closer knowledge of the code base will be able to spot the issue.

instance OutputableBndrId p => Outputable (IE (GhcPass p)) where
    ppr (IEVar       _     var) = ppr (unLoc var)
    ppr (IEThingAbs  _   thing) = ppr (unLoc thing)
    ppr (IEThingAll  _   thing) = hcat [ppr (unLoc thing), text "(..)"]
    ppr (IEThingWith _ thing wc withs flds)
        = ppr (unLoc thing) <> parens (fsep (punctuate comma
                                              (ppWiths ++
                                              map (ppr . flLabel . unLoc) flds)))
      where
        ppWiths =
          case wc of
              NoIEWildcard ->
                map (ppr . unLoc) withs
              IEWildcard pos ->
                let (bs, as) = splitAt pos (map (ppr . unLoc) withs)
                in bs ++ [text ".."] ++ as
    ppr (IEModuleContents _ mod')
        = text "module" <+> ppr mod'
    ppr (IEGroup _ n _)           = text ("<IEGroup: " ++ show n ++ ">")
    ppr (IEDoc _ doc)             = ppr doc
    ppr (IEDocNamed _ string)     = text ("<IEDocNamed: " ++ string ++ ">")
    ppr (XIE x) = ppr x

instance (HasOccName name) => HasOccName (IEWrappedName name) where
  occName w = occName (ieWrappedName w)

instance (OutputableBndr name) => OutputableBndr (IEWrappedName name) where
  pprBndr bs   w = pprBndr bs   (ieWrappedName w)
  pprPrefixOcc w = pprPrefixOcc (ieWrappedName w)
  pprInfixOcc  w = pprInfixOcc  (ieWrappedName w)

instance (OutputableBndr name) => Outputable (IEWrappedName name) where
  ppr (IEName    n) = pprPrefixOcc (unLoc n)
  ppr (IEPattern n) = text "pattern" <+> pprPrefixOcc (unLoc n)
  ppr (IEType    n) = text "type"    <+> pprPrefixOcc (unLoc n)

pprImpExp :: (HasOccName name, OutputableBndr name) => name -> SDoc
pprImpExp name = type_pref <+> pprPrefixOcc name
    where
    occ = occName name
    type_pref | isTcOcc occ && isSymOcc occ = text "type"
              | otherwise                   = empty

This revision seems to be where the pretty-printer was updated: 0d1cb157

@alanz, could you take a peek, please? Thanks!

assigned to @alanz

Here are a couple of test files to play with and demonstrate the issue. Just ghc -ddump-minimal-imports ImportPattern.hs to run.

ExportPattern.hs

ImportPattern.hs

Ok, taking a look

I'm most grateful. I tried -dppr-debug but it didn't generate anything that I could interpret.

@jrp2014 what version of Agda do you get your first example from? Can you give me a link to the "pattern synonym for regular def" bit.

Just straight from the github repository. https://github.com/agda/agda/blob/master/src/full/Agda/Syntax/Abstract.hs (line 115).

To get agda to build, change the make file https://github.com/agda/agda/blob/master/Makefile GHC settings to

GHC_OPTS           = "-ddump-minimal-imports +RTS -M1.7G -RTS"
else
GHC_OPTS           = "-ddump-minimal-imports +RTS -M3G -RTS"

and you can make quicker-install-bin for a 5m build.

Personally, I would try to work with the example files that I attached above at #18247 (comment 278739) which show the same behaviour without as much ceremony.

Thanks. I am building a test case based on your examples, and see how it manifests. I was just confused as I initially looked at the hackage Agda version, and did not see those, they are a recent addition.

Great. I have also investigated whether the issue might be with the dumpMinimalImports function because it prints things unqualified, which is what you need for import/export lists, of course, but printing things out with the flag that prints module names too didn't help. Still no pattern. Very peculiar.

I have also checked that the imported things really are recognised as PatSyns and they seem to be.

Making the exports explicit in the example doesn't seem to help either.

module ExportPattern (
  T(..),
  Nat(..),
  Showable(..),
  Type(..),
  pattern ExNumPat,
  pattern Head,
  pattern Single,
  pattern Ess,
  pattern Pair,
  pattern One,
  pattern Sh,
  pattern Succ,
  pattern (:>),
  pattern (:<),
  pattern Empty,
  pattern Maybe,
  pattern Int,
  pattern Arrow,
  pattern P ) where

@jrp2014 The problem is actually that the imports get serialized an deserialized via the .hi file.

I am running ghc -ddump-minimal-imports -fforce-recomp -ddump-to-file -ddump-parsed-ast -ddump-hi ImportPattern.hs to get a view into those.

But the ExportPattern.dump-hi file has

41b259d7b66794d1859561d256537af1
  pattern ExportPattern.Empty :: Data.Sequence.Internal.Seq a
f2ea83eea252471551ca28412f75fce0

which seems to have what we need.

Yes, otherwise presumably it wouldn't compile. Perplexing.

So I think the ModIFace mod_exports field just lists the name in an instance of

data AvailInfo

  -- | An ordinary identifier in scope
  = Avail Name

  -- | A type or class in scope
  --
  -- The __AvailTC Invariant__: If the type or class is itself to be in scope,
  -- it must be /first/ in this list.  Thus, typically:
  --
  -- > AvailTC Eq [Eq, ==, \/=] []
  | AvailTC
       Name         -- ^ The name of the type or class
       [Name]       -- ^ The available pieces of type or class,
                    -- excluding field selectors.
       [FieldLabel] -- ^ The record fields of the type
                    -- (see Note [Representing fields in AvailInfo]).

but the actual declaration is stored more fully later.

And this is processed for the minimal imports via the to_ie function in GHC.Rename.Names.getMinimalImports.

This calls to_ie_post_rn, which does not care about adding the correct IEWrappedName because after parsing it should not matter. For reference, we have

to_ie_post_rn_var :: (HasOccName name) => Located name -> LIEWrappedName name
to_ie_post_rn_var (L l n)
  | isDataOcc $ occName n = L l (IEPattern (L l n))
  | otherwise             = L l (IEName    (L l n))


to_ie_post_rn :: (HasOccName name) => Located name -> LIEWrappedName name
to_ie_post_rn (L l n)
  | isTcOcc occ && isSymOcc occ = L l (IEType (L l n))
  | otherwise                   = L l (IEName (L l n))
  where occ = occName n

so we probably need to modify the calls in to_ie to use to_ie_post_rn_var at the appropriate places.

The function https://hackage.haskell.org/package/ghc-8.10.1/docs/src/TcRnExports.html#exports_from_avail seems to manage to do the right thing in the opposite direction. Don't know whether it'd help here to reuse some of that logic.

But I am puzzled because I thought that the minimal imports that I looked through at one point had IEPatterns in them, but may be mistaken.

I think it might be as simple as merging to_ie_post_rn and to_ie_post_rn_var into a single function. They do not conflict in the cases they manage. The only worry is if we can have a isDataOcc name that is not a pattern.

exports_from_avail already has the necessary IEWrappedName.

I just tried a run using

to_ie_post_rn :: (HasOccName name) => Located name -> LIEWrappedName name
to_ie_post_rn (L l n)
  | isTcOcc occ && isSymOcc occ = L l (IEType (L l n))
  | isDataOcc occ               = L l (IEPattern (L l n))
  | otherwise                   = L l (IEName (L l n))
  where occ = occName n

and it generates the right values, as

import Control.Monad ()
import qualified Data.Sequence as Seq ()
import ExportPattern
    ( T,
      Nat(pattern Z, pattern S),
      Showable(..),
      Type,
      pattern ExNumPat,
      pattern Head,
      pattern Single,
      pattern Pair,
      pattern One,
      pattern Succ,
      pattern (:>),
      pattern (:<),
      pattern Empty,
      pattern Int,
      pattern Arrow,
      pattern P )

and

import Control.Monad ( guard )
import qualified Data.Sequence as Seq
    ( viewl,
      viewr,
      ViewL(pattern (:<), pattern EmptyL),
      ViewR(pattern (:>)) )

The thing is to convince ourselves that this will not break anything else.

Looks v promising! I guess the there might be some merit in finding whoever wrote the code originally and seeing why they wrote it as they did. I'll also plug this into my test harness and see if anything breaks. Agda makes a good fuzz test!

This comment may help think about the menagerie of possibilities:

-- Renaming exports lists is a minefield. Five different things can appear in
-- children export lists ( T(A, B, C) ).
-- 1. Record selectors
-- 2. Type constructors
-- 3. Data constructors
-- 4. Pattern Synonyms
-- 5. Pattern Synonym Selectors
--
-- However, things get put into weird name spaces.
-- 1. Some type constructors are parsed as variables (-.->) for example.
-- 2. All data constructors are parsed as type constructors
-- 3. When there is ambiguity, we default type constructors to data
-- constructors and require the explicit `type` keyword for type
-- constructors.
--
-- This function first establishes the possible namespaces that an
-- identifier might be in (`choosePossibleNameSpaces`).
--
-- Then for each namespace in turn, tries to find the correct identifier
-- there returning the first positive result or the first terminating
-- error.

... and then are always seem to be exceptions for data families.

Note to self: the original to_ie_post_rn_var (which produces IEPattern or IEName) handles the case where the imported thing has field selectors, none of which are overloaded (DuplicateRecordFields) so we can import just the selectors, without the enclosing datatype.

to_ie_post_rn (IEType or IEName) handled all other cases.

Since patterns don't always have field selectors, there is a problem.

For ease of reference, this is where the two functions that we are thinking about merging were introduced 0d1cb157

mentioned in commit 66aa1a64

Thanks, when you say "I think the correct solution is to dereference the Avails to the actual declarations" what do you have in mind more specifically? I can see that that the Avails are trying to preserve only essential info for Rn and beyond and so discard the interesting stuff in LIEs tells you how to print them, etc, which is what you need for plugins, etc. So you end up with Avail Names that you need to track back to the originals, which seems odd.

While there is tidying up going on, a couple of hlint suggestions for getMinimalImports:

Warning: Redundant map
Found:
  all (`elem` fld_lbls) (map flLabel avail_flds)
Perhaps:
  all ((`elem` fld_lbls) . flLabel) avail_flds

Warning: Hoist not
Found:
  all (not . flIsOverloaded)
Perhaps:
  not . any flIsOverloaded

to_ie_post_rn :: (HasOccName name) => Located name -> LIEWrappedName name
to_ie_post_rn (L l n)
  | isTcOcc occ && isSymOcc occ = L l (IEType (L l n))
  | isDataOcc occ               = L l (IEPattern (L l n))
  | otherwise                   = L l (IEName (L l n))
  where occ = occName n

results in

   parse error on input ‘pattern’
  |
5 |       ViewL(pattern (:<), pattern EmptyL),
  |             ^^^^^

(from the second of the fragments that you generated above)

Looking at 9.7.3 in the 8.10.1 user guide suggests that the pattern keyword should only be used when the pattern is exported on its own, to avoid confusion with type constructors.

Pattern synonyms can be imported and exported through association with a type constructor (including with wildcard). In that case there should be no pattern.

So I can see why we have two to_ie_post_rn functions.

 80     to_ie :: ModIface -> AvailInfo -> [IE GhcRn]
   81     -- The main trick here is that if we're importing all the constructors
   82     -- we want to say "T(..)", but if we're importing only a subset we want
   83     -- to say "T(A,B,C)".  So we have to find out what the module exports.
   84     to_ie _ (Avail n)
   85        = [IEVar noExtField (to_ie_post_rn_var $ noLoc n)]
   86 --       = [IEVar noExtField (to_ie_post_rn $ noLoc n)]
   87     to_ie _ (AvailTC n [m] [])
   88        | n==m = [IEThingAbs noExtField (to_ie_post_rn $ noLoc n)]
   89     to_ie iface (AvailTC n ns fs)
   90       = case [(xs,gs) |  AvailTC x xs gs <- mi_exports iface
   91                  , x == n
   92                  , x `elem` xs    -- Note [Partial export]
   93                  ] of
   94            [xs] | all_used xs -> [IEThingAll noExtField (to_ie_post_rn $ noLoc n)]
   95                 | otherwise   ->
   96                    [IEThingWith noExtField (to_ie_post_rn $ noLoc n) NoIEWildcard
   97                                 (map (to_ie_post_rn . noLoc) (filter (/= n) ns))
   98                                 (map noLoc fs)]
   99                                           -- Note [Overloaded field import]
  100            _other | all_non_overloaded fs
  101                            -> map (IEVar noExtField . to_ie_post_rn . noLoc) $ ns
  102 --                           -> map (IEVar noExtField . to_ie_post_rn_var . noLoc) $ ns
  103                                  ++ map flSelector fs
  104                   | otherwise ->
  105                       [IEThingWith noExtField (to_ie_post_rn $ noLoc n) NoIEWildcard
  106                                 (map (to_ie_post_rn . noLoc) (filter (/= n) ns))
  107                                 (map noLoc fs)]
  108         where
  109 
  110           fld_lbls = map flLabel fs
  111 
  112           all_used (avail_occs, avail_flds)
  113               = all (`elem` ns) avail_occs
  114                     && all ((`elem` fld_lbls) . flLabel) avail_flds
  115 
  116           all_non_overloaded = not . any flIsOverloaded
  117 
  118 to_ie_post_rn_var :: (HasOccName name) => Located name -> LIEWrappedName name
  119 to_ie_post_rn_var (L l n)
  120   | isDataOcc $ occName n = L l (IEPattern (L l n))
  121   | otherwise             = L l (IEName    (L l n))
  122 
  123 
  124 to_ie_post_rn :: (HasOccName name) => Located name -> LIEWrappedName name
  125 to_ie_post_rn (L l n)
  126   | isTcOcc occ && isSymOcc occ = L l (IEType (L l n))
  127   | otherwise                   = L l (IEName (L l n))
  128   where occ = occName n

produces the first of your right examples above, without modding the to_ie_post_rns which is great.

I will have to study whether the IETypes are correct. For example, is the isSymOcc at line 126 right?

Another issue is that you get Patterns not matched: GHC.Hs.ImpExp.XImportDecl _ in relation to the decl in the body of mk_minimal.

Well, I've tweaked mk_minimal and it works better than the original, but isn't completely perfect. For example,

if I have a module List1 that imports Data.List.NonEmpty as List1 and import it into another module, where I use the :| operator, than that import is reduced to and import of (:|), rather than NonEmpty(:|) or pattern (:|). It works fine, if I import from Data.List.NonEmpty directly, rather than via List1
if I import a record type qualified and use it, and its fields, the fields don't all feature in the minimal imports.

I'll test this come more, but I'd be grateful if you could examine the logic, at least. Many thanks.

PS: (And while I remember, is there an easy way of telling whether an available export generated by exports_from_avail needs to be qualified, to distinguish it from an imported name?)

[See below for later, clearer attempt]

added PatternSynonyms label

@alanz Alan, Well, I've kicked the tyres and not found any regressions in the approach above and it certainly removes cases where there were missing pattern / type annotations. There are, no doubt, still limitations, but they are no worse than they were before.

I have not fixed the warning

Pattern match(es) are non-exhaustive
    In a record-update construct:
        Patterns not matched: GHC.Hs.ImpExp.XImportDecl

for example and, of course, the serialization issue that I don't profess to understand remains.

Unless you have any better ideas, we could either leave it at that, or at least merge this version as an improvement, if not the final destination. What do you think?

Here is the latest (expanded) iteration:

   74 getMinimalImports :: [ImportDeclUsage] -> RnM [LImportDecl GhcRn]
   75 getMinimalImports = mapM mk_minimal
   76   where
   77     mk_minimal (L l decl, used_gres, unused)
   78       | null unused
   79       , Just (False, _) <- ideclHiding decl
   80       = return (L l decl)
   81       | otherwise
   82       = do { let ImportDecl { ideclName    = L _ mod_name
   83                             , ideclSource  = is_boot
   84                             , ideclPkgQual = mb_pkg } = decl
   85            ; iface <- loadSrcInterface doc mod_name is_boot (fmap sl_fs mb_pkg)
   86 #if MIN_VERSION_GLASGOW_HASKELL(8,8,0,0)
   87            -- TODO not sure when this was introduced
   88            ; let used_avails = gresToAvailInfo used_gres
   89                  lies = map (L l) (concatMap (to_ie iface) used_avails)
   90 #else
   91            -- used_gres are actually already AvailInfo in earlier versions of
   92            -- GHC
   93            ; let lies = map (L l) (concatMap (to_ie iface) used_gres)
   94 #endif
   95            ; return (L l (decl { ideclHiding = Just (False, L l lies) })) }
   96       where
   97         doc = text "Compute minimal imports for" <+> ppr decl
   98 
   99     to_ie :: ModIface -> AvailInfo -> [IE GhcRn]
  100     -- The main trick here is that if we're importing all the constructors
  101     -- we want to say "T(..)", but if we're importing only a subset we want
  102     -- to say "T(A,B,C)".  So we have to find out what the module exports.
  103     to_ie _ (Avail n) -- An ordinary identifier (eg, var, data constructor)
  104        = [IEVar noExt (to_ie_post_rn_var $ noLoc n)]
  105     to_ie _ (AvailTC n [m] []) -- type or class with absent () list
  106        | n==m = [IEThingAbs noExt (to_ie_post_rn_name $ noLoc n)]
  107     to_ie iface (AvailTC n ns fs)
  108       = case [(xs,gs) |  AvailTC x xs gs <- mi_exports iface
  109                  , x == n
  110                  , x `elem` xs    -- Note [Partial export]
  111                  ] of
  112            -- class / type with methods / constructors s
  113            [xs] | all_used xs -> [IEThingAll noExt (to_ie_post_rn_name $ noLoc n)] -- (..)
  114 
  115                 | isTcOcc (occName n) -> -- typeclass -- @class Functor ...
  116                    [IEThingWith noExt (to_ie_post_rn_name $ noLoc n) NoIEWildcard
  117                                 (map (to_ie_post_rn_varn . noLoc) (filter (/= n) ns))
  118                                 (map noLoc fs)]
  119                                           -- Note [Overloaded field import]
  120 
  121                 | otherwise   -> -- type constructor (ie, @data X =@)
  122                    [IEThingWith noExt (to_ie_post_rn_name $ noLoc n) NoIEWildcard
  123                                 (map (to_ie_post_rn_cname . noLoc) (filter (/= n) ns))
  124                                 (map noLoc fs)]
  125 
  126            -- record type
  127            _other | all_non_overloaded fs
  128                            -> map (IEVar noExt . to_ie_post_rn_name . noLoc) $ ns
  129                                  ++ map flSelector fs
  130                   | otherwise -> -- DuplicateRecordFields is applicable
  131                       [IEThingWith noExt (to_ie_post_rn_name $ noLoc n) NoIEWildcard
  132                                 (map (to_ie_post_rn_cname . noLoc) (filter (/= n) ns))
  133                                 (map noLoc fs)]
  134         where
  135 
  136           fld_lbls = map flLabel fs
  137 
  138           all_used (avail_occs, avail_flds)
  139               = all (`elem` ns) avail_occs
  140                     && all ((`elem` fld_lbls) . flLabel) avail_flds
  141 
  142           all_non_overloaded = not . any flIsOverloaded
  143 
  144 
  145 -- The following were originally one function.  Splitting it out this way allows
  146 -- different variants to be used in different contexts above.  This seems to
  147 -- work, but needs review.  Eg, do we need to use an isSymOcc in some cases?
  148 to_ie_post_rn_name :: Located name -> LIEWrappedName name
  149 to_ie_post_rn_name (L l n) = L l (IEName (L l n))
  150 
  151 to_ie_post_rn_var :: (HasOccName name) => Located name -> LIEWrappedName name
  152 to_ie_post_rn_var (L l n)
  153   | isDataOcc $ occName n = L l (IEPattern (L l n))
  154   | otherwise = L l (IEName (L l n))
  155 
  156 to_ie_post_rn_varn :: (HasOccName name) => Located name -> LIEWrappedName name
  157 to_ie_post_rn_varn (L l n)
  158   | isTcOcc $ occName n = L l (IEType (L l n))
  159   | otherwise = L l (IEName (L l n))
  160 
  161 to_ie_post_rn_cname :: (HasOccName name) => Located name -> LIEWrappedName name
  162 to_ie_post_rn_cname (L l n)
  163   | isTcOcc $ occName n = L l (IEType (L l n))
  164   | otherwise = L l (IEName (L l n))
  165 
  166 -- Notes
  167 --
  168 -- https://gitlab.haskell.org/ghc/ghc/-/wikis/pattern-synonyms/associating-synonyms
  169 
  170 -- An import is
  171 -- a var
  172 -- a tycls -> [(..) | ( var1 , … , varn )]          -- class, etc
  173 -- a tycon -> [ (..) | ( cname1 , … , cnamen )]     -- data
  174 
  175 -- cname -> var | con
  176 -- var -> varid | ( varsym )  -- (does not start with :)
  177 -- con -> conid | ( consym )  -- (starts with :)
  178 
  179 -- GHC User Guide 8.7.3
  180 -- The name of the pattern synonym is in the same namespace as proper data constructors.
  181 -- Like normal data constructors, pattern synonyms can be imported through associations
  182 -- with a type constructor or independently.
  183 -- To export them *on their own*, in an export or import specification,
  184 -- you must prefix pattern names with the pattern keyword
  185 --
  186 -- GHC User Guide 9.9.5
  187 --The form C(.., mi, .., type Tj, ..), where C is a class, names the class C, and the
  188 --specified methods mi and associated types Tj. The types need a keyword “type” to distinguish
  189 --them from data constructors.
  190 --
  191 --Whenever there is no export list and a data instance is defined, the corresponding
  192 --data family type constructor is exported along with the new data constructors, regardless of
  193 --whether the data family is defined locally or in another module.
  194 
  195 -- isVarOcc -> variable name
  196 -- isTvOcc -> is type variable
  197 -- isTcOcc -> is type class name
  198 -- isValOcc -- either in the variable or data constructor namespaces
  199 -- isDataOcc -- Data constructor
  200 -- isDataSymOcc -> Data contructuctor starting with a symbol
  201 -- isSymOcc -> operator(data constructor, variable, etc)
  202 --
  203 -- So there are
  204 -- var -> IEName
  205 -- tycon -> can have IEPattern
  206 -- tyclas -> can have IEType type (:+:)

changed milestone to %8.10.2

mentioned in issue #18264 (closed)

added macOS label and removed needs triage label

removed macOS label

mentioned in issue #18785

added dump flags label

-ddump-minimal-imports does not dump some pattern imports correctly

Summary

Environment

Child items ...

Activity