Fix #15594 (--abi-hash with Backpack sometimes fails)

Summary:
For holes, its necessary to "see through" the instantiation
of the hole to get accurate family instance dependencies.
For example, if B imports <A>, and <A> is instantiated with
F, we must grab and include all of the dep_finsts from
F to have an accurate transitive dep_finsts list.

However, we MUST NOT do this for regular modules.
First, for efficiency reasons, doing this
bloats the the dep_finsts list, because we *already* had
those modules in the list (it wasn't a hole module, after
all). But there's a second, more important correctness
consideration: we perform module renaming when running
--abi-hash.  In this case, GHC's contract to the user is that
it will NOT go and read out interfaces of any dependencies
(https://github.com/haskell/cabal/issues/3633); the point of
--abi-hash is just to get a hash of the on-disk interfaces
for this *specific* package.  If we go off and tug on the
interface for /everything/ in dep_finsts, we're gonna have a
bad time.  (It's safe to do do this for hole modules, though,
because the hmap for --abi-hash is always trivial, so the
interface we request is local.  Though, maybe we ought
not to do it in this case either...)

Signed-off-by: Edward Z. Yang <ezyang@fb.com>

Test Plan: validate

Reviewers: alexbiehl, goldfire, bgamari

Subscribers: ppk, shlevy, rwbarton, carter

GHC Trac Issues: #15594

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

8 files changed, 99 insertions, 3 deletions

diff --git a/compiler/backpack/RnModIface.hs b/compiler/backpack/RnModIface.hs
index 3ae01d72b8..896303b55a 100644
--- a/compiler/backpack/RnModIface.hs
+++ b/compiler/backpack/RnModIface.hs
@@ -138,10 +138,36 @@ rnDepModules sel deps = do
     -- in these dependencies.
     fmap (nubSort . concat) . T.forM (sel deps) $ \mod -> do
         dflags <- getDynFlags
+        -- For holes, its necessary to "see through" the instantiation
+        -- of the hole to get accurate family instance dependencies.
+        -- For example, if B imports <A>, and <A> is instantiated with
+        -- F, we must grab and include all of the dep_finsts from
+        -- F to have an accurate transitive dep_finsts list.
+        --
+        -- However, we MUST NOT do this for regular modules.
+        -- First, for efficiency reasons, doing this
+        -- bloats the the dep_finsts list, because we *already* had
+        -- those modules in the list (it wasn't a hole module, after
+        -- all). But there's a second, more important correctness
+        -- consideration: we perform module renaming when running
+        -- --abi-hash.  In this case, GHC's contract to the user is that
+        -- it will NOT go and read out interfaces of any dependencies
+        -- (https://github.com/haskell/cabal/issues/3633); the point of
+        -- --abi-hash is just to get a hash of the on-disk interfaces
+        -- for this *specific* package.  If we go off and tug on the
+        -- interface for /everything/ in dep_finsts, we're gonna have a
+        -- bad time.  (It's safe to do do this for hole modules, though,
+        -- because the hmap for --abi-hash is always trivial, so the
+        -- interface we request is local.  Though, maybe we ought
+        -- not to do it in this case either...)
+        --
+        -- This mistake was bug #15594.
         let mod' = renameHoleModule dflags hmap mod
-        iface <- liftIO . initIfaceCheck (text "rnDepModule") hsc_env
-                        $ loadSysInterface (text "rnDepModule") mod'
-        return (mod' : sel (mi_deps iface))
+        if isHoleModule mod
+          then do iface <- liftIO . initIfaceCheck (text "rnDepModule") hsc_env
+                                  $ loadSysInterface (text "rnDepModule") mod'
+                  return (mod' : sel (mi_deps iface))
+          else return [mod']
 
 {-
 ************************************************************************
diff --git a/testsuite/tests/backpack/cabal/T15594/Makefile b/testsuite/tests/backpack/cabal/T15594/Makefile
new file mode 100644
index 0000000000..57ef67f977
--- /dev/null
+++ b/testsuite/tests/backpack/cabal/T15594/Makefile
@@ -0,0 +1,20 @@
+TOP=/home/ezyang/Dev/ghc-known-nat/testsuite
+include $(TOP)/mk/boilerplate.mk
+include $(TOP)/mk/test.mk
+
+SETUP='$(PWD)/Setup' -v0
+CONFIGURE=$(SETUP) configure $(CABAL_MINIMAL_BUILD) --with-ghc='$(TEST_HC)' --ghc-options='$(TEST_HC_OPTS)' --package-db='$(PWD)/tmp.d' --prefix='$(PWD)/inst'
+
+T15594: clean
+	'$(GHC_PKG)' init tmp.d
+	'$(TEST_HC)' $(TEST_HC_OPTS) -v0 --make Setup
+	$(CONFIGURE)
+	$(SETUP) build
+	$(SETUP) copy
+	$(SETUP) register
+ifneq "$(CLEANUP)" ""
+	$(MAKE) -s --no-print-directory clean
+endif
+
+clean :
+	$(RM) -rf tmp.d inst dist Setup$(exeext)
diff --git a/testsuite/tests/backpack/cabal/T15594/Setup.hs b/testsuite/tests/backpack/cabal/T15594/Setup.hs
new file mode 100644
index 0000000000..9a994af677
--- /dev/null
+++ b/testsuite/tests/backpack/cabal/T15594/Setup.hs
@@ -0,0 +1,2 @@
+import Distribution.Simple
+main = defaultMain
diff --git a/testsuite/tests/backpack/cabal/T15594/Sig.hsig b/testsuite/tests/backpack/cabal/T15594/Sig.hsig
new file mode 100644
index 0000000000..1342a7b4db
--- /dev/null
+++ b/testsuite/tests/backpack/cabal/T15594/Sig.hsig
@@ -0,0 +1,3 @@
+signature Sig where
+
+foo :: String
diff --git a/testsuite/tests/backpack/cabal/T15594/Stuff.hs b/testsuite/tests/backpack/cabal/T15594/Stuff.hs
new file mode 100644
index 0000000000..053949ba22
--- /dev/null
+++ b/testsuite/tests/backpack/cabal/T15594/Stuff.hs
@@ -0,0 +1,10 @@
+{-# LANGUAGE TypeFamilies #-}
+module Stuff where
+
+data family T a
+
+data instance T Int = T Int
+
+test :: String
+test =
+  "test"
diff --git a/testsuite/tests/backpack/cabal/T15594/all.T b/testsuite/tests/backpack/cabal/T15594/all.T
new file mode 100644
index 0000000000..1978865665
--- /dev/null
+++ b/testsuite/tests/backpack/cabal/T15594/all.T
@@ -0,0 +1,9 @@
+if config.cleanup:
+   cleanup = 'CLEANUP=1'
+else:
+   cleanup = 'CLEANUP=0'
+
+test('T15594',
+     extra_files(['Setup.hs', 'Stuff.hs', 'Sig.hsig', 'pkg.cabal', 'src']),
+     run_command,
+     ['$MAKE -s --no-print-directory T15594 ' + cleanup])
diff --git a/testsuite/tests/backpack/cabal/T15594/pkg.cabal b/testsuite/tests/backpack/cabal/T15594/pkg.cabal
new file mode 100644
index 0000000000..cf6fddadd6
--- /dev/null
+++ b/testsuite/tests/backpack/cabal/T15594/pkg.cabal
@@ -0,0 +1,19 @@
+cabal-version:       2.0
+name:                backpack-trans
+version:             0.1.0.0
+license:             BSD3
+author:              Alex Biehl
+maintainer:          alex.biehl@target.com
+build-type:          Simple
+
+library indef
+  signatures:          Sig
+  exposed-modules:     Stuff
+  build-depends:       base
+  default-language:    Haskell2010
+
+library
+  exposed-modules:     Lib
+  build-depends:       base, indef
+  default-language:    Haskell2010
+  hs-source-dirs:      src
diff --git a/testsuite/tests/backpack/cabal/T15594/src/Lib.hs b/testsuite/tests/backpack/cabal/T15594/src/Lib.hs
new file mode 100644
index 0000000000..73cc27caa0
--- /dev/null
+++ b/testsuite/tests/backpack/cabal/T15594/src/Lib.hs
@@ -0,0 +1,7 @@
+module Lib where
+
+import Stuff
+
+doSomeStuff :: String
+doSomeStuff =
+  test