Opened 6 years ago

Last modified 2 years ago

#8426 new bug

one-shot compilation + TH doesn't see instances that is seen in batch mode

Reported by: errge Owned by:
Priority: normal Milestone:
Component: Template Haskell Version: 7.7
Keywords: Cc: goldfire
Operating System: Unknown/Multiple Architecture: Unknown/Multiple
Type of failure: Other Test Case:
Blocked By: Blocking:
Related Tickets: #7867 Differential Rev(s):
Wiki Page:

Description

Haskell prime and Haskell98 both states that instances are always seen if there is any chain of imports leading from the current module to the instance.

http://darcs.haskell.org/haskell-prime-report/report/haskell-report-html/modules.html#import-instances

The ghc manual contains a discussion why this specification is too performance heavy to näively implement and contains the definition of orphanness and an algorithm to reproduce the specification with better performance.

http://www.haskell.org/ghc/docs/7.6.3/html/users_guide/separate-compilation.html#orphan-modules

Unfortunately this cleverness breaks template haskell. Namely, if:

  • Class.hs contains a class,
  • NonOrphan.hs contains a data that implements the class,
  • Importer.hs just imports NonOrphan.hs,
  • Main.hs imports Importer.hs,
  • Main.hs reifies the class;

then:

  • one-shot compilation's reify doesn't see the instance,
  • batch compilation's reify sees the instance.

This ambiguity is shown in the attached tgz.

Furthermore if NonOrphan.hs is in a separate package, then of course even batch compilation mode wouldn't reify the instance correctly. In that case there is no disambiguity between one-shot and batch, simply both is missing the info.

I propose to solve this by enforcing a loading of all the interface files for every import transitively for the current module, when we meet an open type family or class reification request in template haskell's reify handler in TcSplice.hs.

So I propose to keep the optimization of orphan instances and use it 99.9% of the time. We would only ever go on the quest of reading interface files for every import transitively when the user tries to reify something where we have to return an instance list. This means that TH compilations that reify types will get a little bit slower in exchange of being correct and unambiguous. Compilation time in cases when TH reification is not used will not change.

I volunteer to prepare the patch, but would like to hear others first.

So, any opinions, alternative ideas?

Attachments (6)

th-ambiguity.tgz (872 bytes) - added by errge 6 years ago.
ghc-all-instances-ghc.patch (6.5 KB) - added by errge 6 years ago.
proposed patch, ghc part
ghc-all-instances-th.patch (4.0 KB) - added by errge 6 years ago.
proposed patch, th part
ghc-all-instances-dph.patch (690 bytes) - added by errge 6 years ago.
proposed patch, dph fix
ghc-all-instances-testsuite.2.patch (3.6 KB) - added by errge 6 years ago.
proposed patch, testsuite fixes
ghc-all-instances-testsuite.patch (3.6 KB) - added by errge 6 years ago.
proposed patch, testsuite fixes

Download all attachments as: .zip

Change History (22)

Changed 6 years ago by errge

Attachment: th-ambiguity.tgz added

comment:1 Changed 6 years ago by errge

Version: 7.6.37.7

comment:2 Changed 6 years ago by errge

For related work, see also the discussion in: http://ghc.haskell.org/trac/ghc/wiki/TemplateHaskell/Annotations

comment:3 Changed 6 years ago by simonpj

That seems plausible. But reading the transitive closure of all interface files could be thousands of files! And 90% of the time when reifying a family or class you don't actually want an exhaustive list of all possible instances in all possible modules.

A possible alternative is this: remove [InstanceDec] from ClassI and FamilyI. Instead you have to use qReifyInstances to find out about instances. Because the latter has a [Type] you can use that to know which interface files to load.

To me that sounds simpler (delete code rather than add it), without really removing useful functionality.

Simon

comment:4 Changed 6 years ago by errge

Yes, the exhaustive list can be big.

I agree with your plan of just removing [InstanceDec] from ClassI and FamilyI, fixing the ambiguity.

But actually I need the functionality of looking up instances without types.

So what if we somehow expose that functionality directly? Meaning that only those users will pay the cost who explicitly want to do that.

I see two options:

  • (clean) adding a new Q monad method for this,
  • (a bit hacky) specifying that ReifyInstances with an *empty* list parameter does that.
Last edited 6 years ago by errge (previous) (diff)

Changed 6 years ago by errge

Attachment: ghc-all-instances-ghc.patch added

proposed patch, ghc part

Changed 6 years ago by errge

Attachment: ghc-all-instances-th.patch added

proposed patch, th part

Changed 6 years ago by errge

Attachment: ghc-all-instances-dph.patch added

proposed patch, dph fix

Changed 6 years ago by errge

proposed patch, testsuite fixes

Changed 6 years ago by errge

proposed patch, testsuite fixes

comment:5 Changed 6 years ago by errge

Over the weekend I implemented option 1.

So, what the patch currently does is the following:

  • as Simon suggested, get rid of instance reification when reifying a class or type family, because they can't be made unambiguous,
  • add a new Q monad statement to get the instances from all of our dependencies: this may be slow, but never called implicitly, so doesn't add accidental slowness,
  • the patch is careful that even if you call the slow transitive instance search, we don't pollute the EPS map, so future EPS lookups won't get slower because of bigger map size.

I also attached the necessary one liner change in dph and the testsuite updates. Ran full validate (not just fast), the attached testsuite patch fixes everything what this change causes.

On top of this it's easy to implement the controversial "module listing" functionality from #8398 cleanly. We just load transitively in every interface file and then return all of our dependencies. That's well defined and not compiler implementation dependent.

Please review. Thanks.

Last edited 6 years ago by errge (previous) (diff)

comment:6 Changed 6 years ago by errge

Blocking: 8398 added
Milestone: 7.8.1
Status: newpatch

comment:7 Changed 6 years ago by errge

Blocking: 8398 removed

comment:8 Changed 6 years ago by errge

Milestone: 7.8.17.10.1

The provided patch changes the TH API and the testsuite needs to be updated.

At this point it seems more meaningful to target 7.10.1 with this change.

comment:9 Changed 5 years ago by thoughtpolice

Milestone: 7.10.17.12.1
Status: patchinfoneeded

Gergley, sorry for dropping this patch on the floor. As you can tell since last year a lot has changed! Can you please rebase these patches? I also vaguely remember you wrote a wiki page about what they all addressed. Thanks!

comment:10 Changed 4 years ago by ezyang

The patches are pretty short; I think the real reason this ticket is stalled is because simonpj never signed off on errge's suggested implementation strategy; and in the meantime, it seems this bug has been routed around. Perhaps we should just apply simonpj's original suggestion (remove [InstanceDec] from ClassI and FamilyI) and call this bug fixed.

comment:11 in reply to:  10 Changed 4 years ago by errge

Replying to ezyang:

The patches are pretty short; I think the real reason this ticket is stalled is because simonpj never signed off on errge's suggested implementation strategy; and in the meantime, it seems this bug has been routed around. Perhaps we should just apply simonpj's original suggestion (remove [InstanceDec] from ClassI and FamilyI) and call this bug fixed.

No, I would like to strongly ask you not to do that. As I mentioned before in the bug report, I depend on the functionality to look up instance declarations for classes. If this info were to be removed, then there would be no way for me to continue to support the HFlags library after 7.12.

As can be seen in the history of the ticket in comment 5 I have a patch which keeps the functionality in a very careful way (doesn't cause any accidental slowness for people who don't want to use it). It's also easy to understand from the user's point of view: looking up instances is expensive so you have to call it explicitly if you ever want it.

I'm prepared to rebase that patch and fix up the test suite, if I get some agreement from a maintainer with actual commit rights that the idea is acceptable. Of course, this is not a blank cheque, I would still have to provide quality code, but not willing to work a lot again on this just so that you guys drop this without ever replying or discussing with me for 14 months. I remember prioritizing this work over my other commitments exactly so it doesn't stall and keeps moving, but no one cared. Sorry for the harsh words, I can of course totally understand that for everyone else the contribution time is already thin and very valuable; this is why what I'm looking for is an agreement on goals and designs before anyone (you or me) has to invest serious time and effort on either side.

I'm also open to other implementation ideas, but I seriously depend on this feature so would like to ask you guys to keep it on in some form.

Thanks and sorry for the long comment!

comment:12 Changed 4 years ago by thoughtpolice

Milestone: 7.12.18.0.1

Milestone renamed

comment:13 Changed 4 years ago by thomie

Status: infoneedednew

comment:14 Changed 4 years ago by bgamari

Cc: goldfire added

errge, it seems that this was dropped yet again; I'm sorry that this process has been so messy.

Would you like to rebase your patches and put them up on Phabricator? There we can do a proper review. I'm also cc'ing goldfire, who is our Template Haskell czar.

comment:15 Changed 4 years ago by thomie

Milestone: 8.0.1

comment:16 Changed 2 years ago by spacekitteh

Does this still occur?

Note: See TracTickets for help on using tickets.