Fix #15594 (--abi-hash with Backpack sometimes fails)
authorEdward Z. Yang <ezyang@fb.com>
Mon, 12 Nov 2018 03:39:29 +0000 (22:39 -0500)
committerEdward Z. Yang <ezyang@cs.stanford.edu>
Mon, 12 Nov 2018 03:39:36 +0000 (22:39 -0500)
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

compiler/backpack/RnModIface.hs
testsuite/tests/backpack/cabal/T15594/Makefile [new file with mode: 0644]
testsuite/tests/backpack/cabal/T15594/Setup.hs [new file with mode: 0644]
testsuite/tests/backpack/cabal/T15594/Sig.hsig [new file with mode: 0644]
testsuite/tests/backpack/cabal/T15594/Stuff.hs [new file with mode: 0644]
testsuite/tests/backpack/cabal/T15594/all.T [new file with mode: 0644]
testsuite/tests/backpack/cabal/T15594/pkg.cabal [new file with mode: 0644]
testsuite/tests/backpack/cabal/T15594/src/Lib.hs [new file with mode: 0644]

index 3ae01d7..896303b 100644 (file)
@@ -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 (file)
index 0000000..57ef67f
--- /dev/null
@@ -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 (file)
index 0000000..9a994af
--- /dev/null
@@ -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 (file)
index 0000000..1342a7b
--- /dev/null
@@ -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 (file)
index 0000000..053949b
--- /dev/null
@@ -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 (file)
index 0000000..1978865
--- /dev/null
@@ -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 (file)
index 0000000..cf6fdda
--- /dev/null
@@ -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 (file)
index 0000000..73cc27c
--- /dev/null
@@ -0,0 +1,7 @@
+module Lib where
+
+import Stuff
+
+doSomeStuff :: String
+doSomeStuff =
+  test