Track dep_finsts in exports hash, as it affects downstream deps.

Summary:
I also added some more comments about the orphan and family instance
hashing business.

Fixes #12723.

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

Test Plan: validate

Reviewers: bgamari, austin

Subscribers: thomie

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

GHC Trac Issues: #12723

13 files changed, 137 insertions, 3 deletions

diff --git a/compiler/iface/MkIface.hs b/compiler/iface/MkIface.hs
index 1a191dbb45..219d905f46 100644
--- a/compiler/iface/MkIface.hs
+++ b/compiler/iface/MkIface.hs
@@ -569,6 +569,7 @@ addFingerprints hsc_env mb_old_fingerprint iface0 new_decls
    -- tracked by the usage on the ABI hash of package modules that we import.
    let orph_mods
         = filter (/= this_mod) -- Note [Do not update EPS with your own hi-boot]
+        -- TODO: the line below is not correct, see #12733
         . filter ((== this_pkg) . moduleUnitId)
         $ dep_orphs sorted_deps
    dep_orphan_hashes <- getOrphanHashes hsc_env orph_mods
@@ -592,11 +593,41 @@ addFingerprints hsc_env mb_old_fingerprint iface0 new_decls
                        orphan_hash,
                        dep_orphan_hashes,
                        dep_pkgs (mi_deps iface0),
+                       -- See Note [Export hash depends on non-orphan family instances]
+                       dep_finsts (mi_deps iface0),
                         -- dep_pkgs: see "Package Version Changes" on
                         -- wiki/Commentary/Compiler/RecompilationAvoidance
                        mi_trust iface0)
                         -- Make sure change of Safe Haskell mode causes recomp.
 
+   -- Note [Export hash depends on non-orphan family instances]
+   -- ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+   --
+   -- Suppose we have:
+   --
+   --   module A where
+   --       type instance F Int = Bool
+   --
+   --   module B where
+   --       import A
+   --
+   --   module C where
+   --       import B
+   --
+   -- The family instance consistency check for C depends on the dep_finsts of
+   -- B.  If we rename module A to A2, when the dep_finsts of B changes, we need
+   -- to make sure that C gets rebuilt. Effectively, the dep_finsts are part of
+   -- the exports of B, because C always considers them when checking
+   -- consistency.
+   --
+   -- A full discussion is in #12723.
+   --
+   -- We do NOT need to hash dep_orphs, because this is implied by
+   -- dep_orphan_hashes, and we do not need to hash ordinary class instances,
+   -- because there is no eager consistency check as there is with type families
+   -- (also we didn't store it anywhere!)
+   --
+
    -- put the declarations in a canonical order, sorted by OccName
    let sorted_decls = Map.elems $ Map.fromList $
                           [(getOccName d, e) | e@(_, d) <- decls_w_hashes]
@@ -664,6 +695,36 @@ addFingerprints hsc_env mb_old_fingerprint iface0 new_decls
     fix_fn = mi_fix_fn iface0
     ann_fn = mkIfaceAnnCache (mi_anns iface0)
 
+-- | Retrieve the orphan hashes 'mi_orphan_hash' for a list of modules
+-- (in particular, the orphan modules which are transitively imported by the
+-- current module).
+--
+-- Q: Why do we need the hash at all, doesn't the list of transitively
+-- imported orphan modules suffice?
+--
+-- A: If one of our transitive imports adds a new orphan instance, our
+-- export hash must change so that modules which import us rebuild.  If we just
+-- hashed the [Module], the hash would not change even when a new instance was
+-- added to a module that already had an orphan instance.
+--
+-- Q: Why don't we just hash the orphan hashes of our direct dependencies?
+-- Why the full transitive closure?
+--
+-- A: Suppose we have these modules:
+--
+--      module A where
+--          instance Show (a -> b) where
+--      module B where
+--          import A -- **
+--      module C where
+--          import A
+--          import B
+--
+-- Whether or not we add or remove the import to A in B affects the
+-- orphan hash of B.  But it shouldn't really affect the orphan hash
+-- of C.  If we hashed only direct dependencies, there would be no
+-- way to tell that the net effect was a wash, and we'd be forced
+-- to recompile C and everything else.
 getOrphanHashes :: HscEnv -> [Module] -> IO [Fingerprint]
 getOrphanHashes hsc_env mods = do
   eps <- hscEPS hsc_env
diff --git a/compiler/main/HscTypes.hs b/compiler/main/HscTypes.hs
index b5f86db4e6..9b2584dd0a 100644
--- a/compiler/main/HscTypes.hs
+++ b/compiler/main/HscTypes.hs
@@ -2258,10 +2258,13 @@ data Dependencies
                         -- instance orphans as they are anyway included in
                         -- 'dep_finsts'.  But then be careful about code
                         -- which relies on dep_orphs having the complete list!)
+                        -- This does NOT include us, unlike 'imp_orphs'.
 
          , dep_finsts :: [Module]
-                        -- ^ Modules that contain family instances (whether the
-                        -- instances are from the home or an external package)
+                        -- ^ Transitive closure of depended upon modules which
+                        -- contain family instances (whether home or external).
+                        -- This is used by 'checkFamInstConsistency'.  This
+                        -- does NOT include us, unlike 'imp_finsts'.
          }
   deriving( Eq )
         -- Equality used only for old/new comparison in MkIface.addFingerprints
diff --git a/compiler/rename/RnNames.hs b/compiler/rename/RnNames.hs
index 3b37b18cc2..a57c9952fc 100644
--- a/compiler/rename/RnNames.hs
+++ b/compiler/rename/RnNames.hs
@@ -350,6 +350,11 @@ calculateAvails dflags iface mod_safe' want_boot =
 
 
       -- Compute new transitive dependencies
+      --
+      -- 'dep_orphs' and 'dep_finsts' do NOT include the imported module
+      -- itself, but we DO need to include this module in 'imp_orphs' and
+      -- 'imp_finsts' if it defines an orphan or instance family; thus the
+      -- orph_iface/has_iface tests.
 
       orphans | orph_iface = ASSERT( not (imp_mod `elem` dep_orphs deps) )
                              imp_mod : dep_orphs deps
diff --git a/compiler/typecheck/FamInst.hs b/compiler/typecheck/FamInst.hs
index d8bc8a71df..09417a7f25 100644
--- a/compiler/typecheck/FamInst.hs
+++ b/compiler/typecheck/FamInst.hs
@@ -169,8 +169,19 @@ See Note [Unique Determinism] in Unique
 listToSet :: [ModulePair] -> ModulePairSet
 listToSet l = Set.fromList l
 
+-- | Check family instance consistency, given:
+--
+-- 1. The list of all modules transitively imported by us
+--    which define a family instance (these are the ones
+--    we have to check for consistency), and
+--
+-- 2. The list of modules which we directly imported
+--    (these specify the sets of family instance defining
+--    modules which are already known to be consistent).
+--
+-- See Note [Checking family instance consistency] for more
+-- details.
 checkFamInstConsistency :: [Module] -> [Module] -> TcM ()
--- See Note [Checking family instance consistency]
 checkFamInstConsistency famInstMods directlyImpMods
   = do { dflags     <- getDynFlags
        ; (eps, hpt) <- getEpsAndHpt
diff --git a/testsuite/driver/extra_files.py b/testsuite/driver/extra_files.py
index 5918523a57..c360537c3a 100644
--- a/testsuite/driver/extra_files.py
+++ b/testsuite/driver/extra_files.py
@@ -480,6 +480,7 @@ extra_src_files = {
   'recomp010': ['Main.hs', 'X1.hs', 'X2.hs'],
   'recomp011': ['Main.hs'],
   'recomp015': ['Generate.hs'],
+  'recomp016': ['A.hs', 'A2.hs', 'C.hs', 'D.hs', 'E.hs'],
   'record_upd': ['Main.hs'],
   'rename.prog001': ['Rn037Help.hs', 'rn037.hs'],
   'rename.prog002': ['Rn037Help.hs', 'rnfail037.hs'],
diff --git a/testsuite/tests/driver/recomp016/A.hs b/testsuite/tests/driver/recomp016/A.hs
new file mode 100644
index 0000000000..17a9dc0cf5
--- /dev/null
+++ b/testsuite/tests/driver/recomp016/A.hs
@@ -0,0 +1,4 @@
+{-# LANGUAGE TypeFamilies #-}
+module A where
+type family F a
+type instance F Int = Bool
diff --git a/testsuite/tests/driver/recomp016/A2.hs b/testsuite/tests/driver/recomp016/A2.hs
new file mode 100644
index 0000000000..bb0409ee1e
--- /dev/null
+++ b/testsuite/tests/driver/recomp016/A2.hs
@@ -0,0 +1,4 @@
+{-# LANGUAGE TypeFamilies #-}
+module A2 where
+type family F a
+type instance F Int = Bool
diff --git a/testsuite/tests/driver/recomp016/C.hs b/testsuite/tests/driver/recomp016/C.hs
new file mode 100644
index 0000000000..dc3b65802f
--- /dev/null
+++ b/testsuite/tests/driver/recomp016/C.hs
@@ -0,0 +1,2 @@
+module C where
+import B
diff --git a/testsuite/tests/driver/recomp016/D.hs b/testsuite/tests/driver/recomp016/D.hs
new file mode 100644
index 0000000000..604f04522c
--- /dev/null
+++ b/testsuite/tests/driver/recomp016/D.hs
@@ -0,0 +1,2 @@
+module D where
+import C
diff --git a/testsuite/tests/driver/recomp016/E.hs b/testsuite/tests/driver/recomp016/E.hs
new file mode 100644
index 0000000000..5adb6a831d
--- /dev/null
+++ b/testsuite/tests/driver/recomp016/E.hs
@@ -0,0 +1,3 @@
+module E where
+import D
+import B
diff --git a/testsuite/tests/driver/recomp016/Makefile b/testsuite/tests/driver/recomp016/Makefile
new file mode 100644
index 0000000000..821b126c45
--- /dev/null
+++ b/testsuite/tests/driver/recomp016/Makefile
@@ -0,0 +1,19 @@
+TOP=../../..
+include $(TOP)/mk/boilerplate.mk
+include $(TOP)/mk/test.mk
+
+# Recompilation tests
+
+clean:
+	rm -f *.o *.hi
+
+# bug #12723
+
+recomp016: clean
+	echo 'module B (module A) where import A' > B.hs
+	echo 'first run'
+	'$(TEST_HC)' $(TEST_HC_OPTS) --make E.hs
+	sleep 1
+	echo 'module B (module A2) where import A2' > B.hs
+	echo 'second run'
+	'$(TEST_HC)' $(TEST_HC_OPTS) --make E.hs
diff --git a/testsuite/tests/driver/recomp016/all.T b/testsuite/tests/driver/recomp016/all.T
new file mode 100644
index 0000000000..a1a2ebd55a
--- /dev/null
+++ b/testsuite/tests/driver/recomp016/all.T
@@ -0,0 +1,7 @@
+# Test for #12723, a recompilation bug
+
+test('recomp016',
+     [ clean_cmd('$MAKE -s clean') ],
+     run_command,
+     ['$MAKE -s --no-print-directory recomp016'])
+
diff --git a/testsuite/tests/driver/recomp016/recomp016.stdout b/testsuite/tests/driver/recomp016/recomp016.stdout
new file mode 100644
index 0000000000..eb6c6fce42
--- /dev/null
+++ b/testsuite/tests/driver/recomp016/recomp016.stdout
@@ -0,0 +1,12 @@
+first run
+[1 of 5] Compiling A                ( A.hs, A.o )
+[2 of 5] Compiling B                ( B.hs, B.o )
+[3 of 5] Compiling C                ( C.hs, C.o )
+[4 of 5] Compiling D                ( D.hs, D.o )
+[5 of 5] Compiling E                ( E.hs, E.o )
+second run
+[1 of 5] Compiling A2               ( A2.hs, A2.o )
+[2 of 5] Compiling B                ( B.hs, B.o )
+[3 of 5] Compiling C                ( C.hs, C.o ) [B changed]
+[4 of 5] Compiling D                ( D.hs, D.o ) [C changed]
+[5 of 5] Compiling E                ( E.hs, E.o ) [B changed]