Improvements/bugfixes to signature reexport handling.

Summary:
A number of changes:

- Keep the TcGblEnv from typechecking the local signature
  around when we do merging.  In particular, we setup
  tcg_imports and tcg_rdr_env according to the local
  signature.  This improves our error output (for example,
  see bkpfail04) and also fixes a bug with reexporting
  modules in signatures (see bkpreex07)

- Fix a bug in thinning, where if we had signature A(module A),
  this previously would have *thinned out* all of the inherited
  signatures.  Now we treat every inherited signature as having
  come from an import like "import A", so a module A reexport
  will pick them up.

- Recompilation checking now keeps track of dependent source files
  of the source signature; previously we forgot to retain this
  info.

There's a manual update too.

Signed-off-by: Edward Z. Yang <ezyang@cs.stanford.edu>

Test Plan: validate

Reviewers: bgamari, austin

Subscribers: thomie

Differential Revision: https://phabricator.haskell.org/D3133

15 files changed, 156 insertions, 47 deletions

diff --git a/compiler/main/HscMain.hs b/compiler/main/HscMain.hs
index c8aa0ab390..6e6ac04e5e 100644
--- a/compiler/main/HscMain.hs
+++ b/compiler/main/HscMain.hs
@@ -420,7 +420,7 @@ hscTypecheck keep_rn mod_summary mb_rdr_module = do
             if hsc_src == HsigFile
                 then do (iface, _, _) <- liftIO $ hscSimpleIface hsc_env tc_result0 Nothing
                         ioMsgMaybe $
-                            tcRnMergeSignatures hsc_env (tcg_top_loc tc_result0) hpm iface
+                            tcRnMergeSignatures hsc_env hpm tc_result0 iface
                 else return tc_result0
 
 -- wrapper around tcRnModule to handle safe haskell extras
diff --git a/compiler/typecheck/TcBackpack.hs b/compiler/typecheck/TcBackpack.hs
index 086dee178b..a2e5abef9e 100644
--- a/compiler/typecheck/TcBackpack.hs
+++ b/compiler/typecheck/TcBackpack.hs
@@ -343,17 +343,18 @@ tcRnCheckUnitId hsc_env uid =
 
 -- | Top-level driver for signature merging (run after typechecking
 -- an @hsig@ file).
-tcRnMergeSignatures :: HscEnv -> RealSrcSpan -> HsParsedModule -> ModIface
+tcRnMergeSignatures :: HscEnv -> HsParsedModule -> TcGblEnv {- from local sig -} -> ModIface
                     -> IO (Messages, Maybe TcGblEnv)
-tcRnMergeSignatures hsc_env real_loc hsmod iface =
+tcRnMergeSignatures hsc_env hpm orig_tcg_env iface =
   withTiming (pure dflags)
              (text "Signature merging" <+> brackets (ppr this_mod))
              (const ()) $
   initTc hsc_env HsigFile False this_mod real_loc $
-    mergeSignatures hsmod iface
+    mergeSignatures hpm orig_tcg_env iface
  where
   dflags   = hsc_dflags hsc_env
   this_mod = mi_module iface
+  real_loc = tcg_top_loc orig_tcg_env
 
 thinModIface :: [AvailInfo] -> ModIface -> ModIface
 thinModIface avails iface =
@@ -484,8 +485,11 @@ merge_msg mod_name reqs =
 -- from 'requirementMerges' into this signature, producing
 -- a final 'TcGblEnv' that matches the local signature and
 -- all required signatures.
-mergeSignatures :: HsParsedModule -> ModIface -> TcRn TcGblEnv
-mergeSignatures hsmod lcl_iface0 = do
+mergeSignatures :: HsParsedModule -> TcGblEnv -> ModIface -> TcRn TcGblEnv
+mergeSignatures
+  (HsParsedModule { hpm_module = L loc (HsModule { hsmodExports = mb_exports }),
+                    hpm_src_files = src_files })
+  orig_tcg_env lcl_iface0 = setSrcSpan loc $ do
     -- The lcl_iface0 is the ModIface for the local hsig
     -- file, which is guaranteed to exist, see
     -- Note [Blank hsigs for all requirements]
@@ -494,7 +498,6 @@ mergeSignatures hsmod lcl_iface0 = do
     tcg_env <- getGblEnv
     let outer_mod = tcg_mod tcg_env
         inner_mod = tcg_semantic_mod tcg_env
-        mb_exports = hsmodExports (unLoc (hpm_module hsmod))
         mod_name = moduleName (tcg_mod tcg_env)
 
     -- STEP 1: Figure out all of the external signature interfaces
@@ -529,7 +532,16 @@ mergeSignatures hsmod lcl_iface0 = do
             as1 <- tcRnModExports insts ireq_iface
             let inst_uid = fst (splitUnitIdInsts (IndefiniteUnitId iuid))
                 pkg = getInstalledPackageDetails dflags inst_uid
-                rdr_env = mkGlobalRdrEnv (gresFromAvails Nothing as1)
+                -- Setup the import spec correctly, so that when we apply
+                -- IEModuleContents we pick up EVERYTHING
+                ispec = ImpSpec
+                            ImpDeclSpec{
+                                is_mod  = mod_name,
+                                is_as   = mod_name,
+                                is_qual = False,
+                                is_dloc = loc
+                            } ImpAll
+                rdr_env = mkGlobalRdrEnv (gresFromAvails (Just ispec) as1)
             (thinned_iface, as2) <- case mb_exports of
                     Just (L loc _)
                       | null (exposedModules pkg) -> setSrcSpan loc $ do
@@ -572,7 +584,19 @@ mergeSignatures hsmod lcl_iface0 = do
               | otherwise = WarnSome $ map (\o -> (o, inheritedSigPvpWarning)) warn_occs
         -}
     setGblEnv tcg_env {
-        tcg_rdr_env = rdr_env,
+        -- The top-level GlobalRdrEnv is quite interesting.  It consists
+        -- of two components:
+        --  1. First, we reuse the GlobalRdrEnv of the local signature.
+        --     This is very useful, because it means that if we have
+        --     to print a message involving some entity that the local
+        --     signature imported, we'll qualify it accordingly.
+        --  2. Second, we need to add all of the declarations we are
+        --     going to merge in (as they need to be in scope for the
+        --     final test of the export list.)
+        tcg_rdr_env = rdr_env `plusGlobalRdrEnv` tcg_rdr_env orig_tcg_env,
+        -- Inherit imports from the local signature, so that module
+        -- rexports are picked up correctly
+        tcg_imports = tcg_imports orig_tcg_env,
         tcg_exports = exports,
         tcg_dus     = usesOnly (availsToNameSetWithSelectors exports),
         tcg_warns   = warns
@@ -580,6 +604,7 @@ mergeSignatures hsmod lcl_iface0 = do
     tcg_env <- getGblEnv
 
     -- Make sure we didn't refer to anything that doesn't actually exist
+    -- pprTrace "mergeSignatures: exports_from_avail" (ppr exports) $ return ()
     (mb_lies, _) <- exports_from_avail mb_exports rdr_env
                         (tcg_imports tcg_env) (tcg_semantic_mod tcg_env)
 
@@ -746,6 +771,8 @@ mergeSignatures hsmod lcl_iface0 = do
             tcg_type_env = extendTypeEnvWithIds (tcg_type_env tcg_env) (map fst dfun_insts)
         }
 
+    addDependentFiles src_files
+
     return tcg_env
 
 -- | Top-level driver for signature instantiation (run when compiling
diff --git a/docs/users_guide/separate_compilation.rst b/docs/users_guide/separate_compilation.rst
index f6c710f155..280631a7df 100644
--- a/docs/users_guide/separate_compilation.rst
+++ b/docs/users_guide/separate_compilation.rst
@@ -729,7 +729,7 @@ to ``hs-boot`` files, but with some slight changes:
 - Import statements and scoping rules are exactly as in Haskell.
   To mention a non-Prelude type or class, you must import it.
 
-- Unlike regular modules, the exports and defined entities of
+- Unlike regular modules, the defined entities of
   a signature include not only those written in the local
   ``hsig`` file, but also those from inherited signatures
   (as inferred from the :ghc-flag:`-package-id` flags).
@@ -763,49 +763,67 @@ to ``hs-boot`` files, but with some slight changes:
         f :: T -> T
         g :: T
 
-- The export list of a signature applies the final export list
-  of a signature after merging inherited signatures; in particular, it
-  may refer to entities which are not declared in the body of the
-  local ``hsig`` file.  The set of entities that are required by a
-  signature is defined exclusively by its exports; if an entity
-  is not mentioned in the export list, it is not required.  This means
-  that a library author can provide an omnibus signature containing the
-  type of every function someone might want to use, while a client thins
-  down the exports to the ones they actually require.  For example,
-  supposing that you have inherited a signature for strings, you might
-  write a local signature of this form, listing only the entities
-  that you need::
+- If no export list is provided for a signature, the exports of
+  a signature are all of its defined entities merged with the
+  exports of all inherited signatures.
 
-    signature Str (Str, empty, append, concat) where
-        -- empty
+  If you want to reexport an entity from a signature, you must
+  also include a ``module SigName`` export, so that all of the
+  entities defined in the signature are exported.  For example,
+  the following module exports both ``f`` and ``Int`` from
+  ``Prelude``::
 
-  A few caveats apply here.  First, it is illegal to export an entity
-  which refers to a locally defined type which itself is not exported
-  (GHC will report an error in this case).  Second, signatures which
-  come from dependencies which expose modules cannot be thinned in this
-  way (after all, the dependency itself may need the entity); these
-  requirements are unconditionally exported, but are associated with
-  a warning discouraging their use by a module.  To use an entity
-  defined by such a signature, add its declaration to your local
-  ``hsig`` file.
+    signature A(module A, Int) where
+        import Prelude (Int)
+        f :: Int
 
-- A signature can reexport an entity brought into scope by an import.
-  In this case, we indicate that any implementation of the module
-  must export this very same entity.  For example, this signature
-  must be implemented by a module which itself reexports ``Int``::
+  Reexports merge with local declarations; thus, the signature above
+  would successfully merge with::
 
-    signature A (Int) where
-        import Prelude (Int)
+    signature A where
+        data Int
+
+  The only permissible implementation of such a signature is a module
+  which reexports precisely the same entity::
 
-    -- can be implemented by:
-    module A (Int) where
+    module A (f, Int) where
         import Prelude (Int)
+        f = 2 :: Int
 
   Conversely, any entity requested by a signature can be provided
   by a reexport from the implementing module.  This is different from
   ``hs-boot`` files, which require every entity to be defined
   locally in the implementing module.
 
+- GHC has experimental support for *signature thinning*, which is used
+  when a signature has an explicit export list without a module export of the
+  signature itself.  In this case, the export list applies to the final export
+  list *after* merging, in particular, you may refer to entities which are not
+  declared in the body of the local ``hsig`` file.
+
+  The semantics in this case is that the set of required entities is defined
+  exclusively by its exports; if an entity is not mentioned in the export list,
+  it is not required.  The motivation behind this feature is to allow a library
+  author to provide an omnibus signature containing the type of every function
+  someone might want to use, while a client thins down the exports to the ones
+  they actually require.  For example, supposing that you have inherited a
+  signature for strings, you might write a local signature of this form, listing
+  only the entities that you need::
+
+    signature Str (Str, empty, append, concat) where
+        -- empty
+
+  A few caveats apply here.  First, it is illegal to export an entity
+  which refers to a locally defined type which itself is not exported
+  (GHC will report an error in this case).  Second, signatures which
+  come from dependencies which expose modules cannot be thinned in this
+  way (after all, the dependency itself may need the entity); these
+  requirements are unconditionally exported.  Finally, any module
+  reexports must refer to modules imported by the local signature
+  (even if an inherited signature exported the module).
+
+  We may change the syntax and semantics of this feature in the future.
+
 - The declarations and types from signatures of dependencies
   that will be merged in are not in scope when type checking
   an ``hsig`` file.  To refer to any such type, you must
diff --git a/testsuite/tests/backpack/cabal/bkpcabal02/bkpcabal02.stderr b/testsuite/tests/backpack/cabal/bkpcabal02/bkpcabal02.stderr
index c1aa54d95e..681c541e21 100644
--- a/testsuite/tests/backpack/cabal/bkpcabal02/bkpcabal02.stderr
+++ b/testsuite/tests/backpack/cabal/bkpcabal02/bkpcabal02.stderr
@@ -2,8 +2,8 @@
 q/H.hsig:2:1: error:
     • Identifier ‘x’ has conflicting definitions in the module
       and its hsig file
-      Main module: x :: ghc-prim-0.5.0.0:GHC.Types.Int
-      Hsig file:  x :: ghc-prim-0.5.0.0:GHC.Types.Bool
+      Main module: x :: Int
+      Hsig file:  x :: Bool
       The two types are different
     • while merging the signatures from:
         • z-bkpcabal01-z-p-0.1.0.0[H=<H>]:H
diff --git a/testsuite/tests/backpack/reexport/all.T b/testsuite/tests/backpack/reexport/all.T
index 55a5004571..5619707e5d 100644
--- a/testsuite/tests/backpack/reexport/all.T
+++ b/testsuite/tests/backpack/reexport/all.T
@@ -5,3 +5,7 @@ test('bkpreex04', normal, backpack_typecheck, [''])
 # These signatures are behaving badly and the renamer gets confused
 test('bkpreex05', expect_broken(0), backpack_typecheck, [''])
 test('bkpreex06', normal, backpack_typecheck, [''])
+test('bkpreex07', normal, backpack_typecheck, [''])
+test('bkpreex08', normal, backpack_typecheck, [''])
+test('bkpreex09', normal, backpack_typecheck, [''])
+test('bkpreex10', normal, backpack_typecheck, [''])
diff --git a/testsuite/tests/backpack/reexport/bkpreex07.bkp b/testsuite/tests/backpack/reexport/bkpreex07.bkp
new file mode 100644
index 0000000000..9cfb539152
--- /dev/null
+++ b/testsuite/tests/backpack/reexport/bkpreex07.bkp
@@ -0,0 +1,3 @@
+unit p where
+    signature A(module Prelude) where
+        import Prelude
diff --git a/testsuite/tests/backpack/reexport/bkpreex07.stderr b/testsuite/tests/backpack/reexport/bkpreex07.stderr
new file mode 100644
index 0000000000..4852528959
--- /dev/null
+++ b/testsuite/tests/backpack/reexport/bkpreex07.stderr
@@ -0,0 +1,2 @@
+[1 of 1] Processing p
+  [1 of 1] Compiling A[sig]           ( p/A.hsig, nothing )
diff --git a/testsuite/tests/backpack/reexport/bkpreex08.bkp b/testsuite/tests/backpack/reexport/bkpreex08.bkp
new file mode 100644
index 0000000000..596e5ea034
--- /dev/null
+++ b/testsuite/tests/backpack/reexport/bkpreex08.bkp
@@ -0,0 +1,17 @@
+unit q where
+    module B where
+        f = 2 :: Int
+unit p2 where
+    dependency q
+    signature A(f) where
+        import B
+unit p where
+    dependency q
+    dependency p2[A=<A>]
+    signature A(module A, module Prelude) where
+        import Prelude
+        f :: Int
+    module M where
+        import B
+        import A
+        g = f
diff --git a/testsuite/tests/backpack/reexport/bkpreex08.stderr b/testsuite/tests/backpack/reexport/bkpreex08.stderr
new file mode 100644
index 0000000000..41983efaed
--- /dev/null
+++ b/testsuite/tests/backpack/reexport/bkpreex08.stderr
@@ -0,0 +1,8 @@
+[1 of 3] Processing q
+  Instantiating q
+  [1 of 1] Compiling B                ( q/B.hs, nothing )
+[2 of 3] Processing p2
+  [1 of 1] Compiling A[sig]           ( p2/A.hsig, nothing )
+[3 of 3] Processing p
+  [1 of 2] Compiling A[sig]           ( p/A.hsig, nothing )
+  [2 of 2] Compiling M                ( p/M.hs, nothing )
diff --git a/testsuite/tests/backpack/reexport/bkpreex09.bkp b/testsuite/tests/backpack/reexport/bkpreex09.bkp
new file mode 100644
index 0000000000..5b16c44875
--- /dev/null
+++ b/testsuite/tests/backpack/reexport/bkpreex09.bkp
@@ -0,0 +1,10 @@
+unit p where
+    signature A where
+        f :: Bool
+unit q where
+    dependency p[A=<A>]
+    signature A(module A) where
+        h :: Bool
+    module M where
+        import A
+        g = f && h
diff --git a/testsuite/tests/backpack/reexport/bkpreex09.stderr b/testsuite/tests/backpack/reexport/bkpreex09.stderr
new file mode 100644
index 0000000000..d4bedc39f5
--- /dev/null
+++ b/testsuite/tests/backpack/reexport/bkpreex09.stderr
@@ -0,0 +1,5 @@
+[1 of 2] Processing p
+  [1 of 1] Compiling A[sig]           ( p/A.hsig, nothing )
+[2 of 2] Processing q
+  [1 of 2] Compiling A[sig]           ( q/A.hsig, nothing )
+  [2 of 2] Compiling M                ( q/M.hs, nothing )
diff --git a/testsuite/tests/backpack/reexport/bkpreex10.bkp b/testsuite/tests/backpack/reexport/bkpreex10.bkp
new file mode 100644
index 0000000000..4b00e57b5e
--- /dev/null
+++ b/testsuite/tests/backpack/reexport/bkpreex10.bkp
@@ -0,0 +1,10 @@
+{-# LANGUAGE ConstraintKinds #-}
+unit p where
+    signature A(module Data.Typeable) where
+        import Data.Typeable
+unit q where
+    dependency p[A=<A>]
+    signature A(module A) where
+    module M where
+        import A
+        type X = Typeable
diff --git a/testsuite/tests/backpack/reexport/bkpreex10.stderr b/testsuite/tests/backpack/reexport/bkpreex10.stderr
new file mode 100644
index 0000000000..d4bedc39f5
--- /dev/null
+++ b/testsuite/tests/backpack/reexport/bkpreex10.stderr
@@ -0,0 +1,5 @@
+[1 of 2] Processing p
+  [1 of 1] Compiling A[sig]           ( p/A.hsig, nothing )
+[2 of 2] Processing q
+  [1 of 2] Compiling A[sig]           ( q/A.hsig, nothing )
+  [2 of 2] Compiling M                ( q/M.hs, nothing )
diff --git a/testsuite/tests/backpack/should_fail/bkpfail04.stderr b/testsuite/tests/backpack/should_fail/bkpfail04.stderr
index f445c57f8e..07159cf277 100644
--- a/testsuite/tests/backpack/should_fail/bkpfail04.stderr
+++ b/testsuite/tests/backpack/should_fail/bkpfail04.stderr
@@ -8,8 +8,8 @@
 bkpfail04.bkp:7:9: error:
     • Type constructor ‘A’ has conflicting definitions in the module
       and its hsig file
-      Main module: data A = A {foo :: GHC.Types.Int}
-      Hsig file:  data A = A {bar :: GHC.Types.Bool}
+      Main module: data A = A {foo :: Int}
+      Hsig file:  data A = A {bar :: Bool}
       The constructors do not match:
         The record label lists for ‘A’ differ
         The types for ‘A’ differ
diff --git a/testsuite/tests/backpack/should_fail/bkpfail42.stderr b/testsuite/tests/backpack/should_fail/bkpfail42.stderr
index 30b43d829e..5a9e1aa9c3 100644
--- a/testsuite/tests/backpack/should_fail/bkpfail42.stderr
+++ b/testsuite/tests/backpack/should_fail/bkpfail42.stderr
@@ -7,9 +7,9 @@ bkpfail42.bkp:9:9: error:
     • Type constructor ‘F’ has conflicting definitions in the module
       and its hsig file
       Main module: type family F a :: *
-                     where [a] F a = GHC.Types.Int
+                     where [a] F a = Int
       Hsig file:  type family F a :: *
-                    where [a] F a = GHC.Types.Bool
+                    where [a] F a = Bool
     • while merging the signatures from:
         • p[A=<A>]:A
         • ...and the local signature for A