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

Authored by ezyang on Sep 2 2018, 5:46 PM.

Details

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

Diff Detail

Repository
rGHC Glasgow Haskell Compiler
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
ezyang created this revision.Sep 2 2018, 5:46 PM
ezyang updated this revision to Diff 17889.Sep 2 2018, 7:56 PM

syntaxfix

Thank you for the quick fix, Edward!

@shlevy let's exercise our backport machinery for this one!

alexbiehl added a comment.EditedSep 3 2018, 1:50 AM

Also, I know this is an unpopular request by now but this is biting us in production and would be nice to have on ghc-8.6 @bgamari.

JFTR we are using this patch in production for and it works great!

ezyang retitled this revision from Fix #8441 (--abi-hash with Backpack sometimes fails) to Fix #15594 (--abi-hash with Backpack sometimes fails).Sep 12 2018, 9:41 AM
ppk added a subscriber: ppk.Sep 13 2018, 1:25 AM

The summary for this phab is very sparse can there be more text there ?

compiler/backpack/RnModIface.hs
147

Really minor point here and in any case take my remark with a pinch of salt as I am not a native speaker. The double negation "MUST NOT do ... not a hole module" is a bit confusing to read. Can you replace "not a hole module" with "completed module"/"filled modules"/ (what exactly is the terminology for such a module) or may be even "modules without holes".

ppk added inline comments.Sep 13 2018, 1:26 AM
testsuite/tests/backpack/cabal/T15594/Stuff.hs
7

There is no line that can be uncommented here. I think some thing is missing here.

bgamari requested changes to this revision.Oct 15 2018, 12:29 PM
bgamari updated the Trac tickets for this revision.

Thanks for reviewing @ppk!

testsuite/tests/backpack/cabal/T15594/Stuff.hs
7

Yes, this should be fixed. @ezyang, should there be something more here or should the comment be dropped?

This revision now requires changes to proceed.Oct 15 2018, 12:30 PM
ezyang edited the summary of this revision. (Show Details)Nov 7 2018, 8:37 AM
ezyang updated this revision to Diff 18608.Nov 7 2018, 8:37 AM

Comment updates

ezyang added a comment.Nov 7 2018, 8:38 AM

Comment should be dropped, I believe; it came from Alex's repro.

ezyang marked 3 inline comments as done.Nov 7 2018, 8:38 AM

All issues resolved!

This revision was not accepted when it landed; it landed in state Needs Review.Nov 11 2018, 9:40 PM
This revision was automatically updated to reflect the committed changes.