Show instance import provenance on overlapping instances
Needs RevisionPublic

Authored by dminuoso on Nov 25 2018, 9:03 AM.


Trac Issues

Implements a feature request from Trac Trac #15814

dminuoso created this revision.Nov 25 2018, 9:03 AM
dminuoso updated the Trac tickets for this revision.Nov 25 2018, 9:04 AM
dminuoso updated this revision to Diff 18884.Nov 26 2018, 12:47 AM
  • Break cyclic dependency
RyanGlScott requested changes to this revision.Nov 26 2018, 5:13 AM
RyanGlScott added a subscriber: RyanGlScott.

Excellent work, @dminuoso!

Have you ran the full test suite after making these changes? I'd be surprised if there weren't any test cases whose expected stderr didn't move around as a result of the changes to this error message.

I've left some suggestions inline.


It might be helpful to have a small Note explaining what all of the moving parts here do.

Also, since you mentioned in that this currently only works for instances defined in orphan modules or the same module as they were imported from, it would be good to mention this restriction in the Note, and perhaps sketch out would need to be done if someone wanted to lift this restriction.


Since you went through the effort of implementing this code path where an instance can be imported through multiple modules, it would be good to have a corresponding test case that exercises this code path.

This revision now requires changes to proceed.Nov 26 2018, 5:13 AM
dminuoso updated this revision to Diff 18999.Dec 4 2018, 3:43 AM
  • Add documentation
dminuoso updated this revision to Diff 19000.Dec 4 2018, 4:20 AM
  • Fix newline typo
simonpj added a subscriber: simonpj.Dec 4 2018, 4:45 AM
simonpj added inline comments.

Explain the goal! We want to find the *import declaration(s)* in this module that are responsible for this $df being in scope.

Also, could you give a concrete example of each case?


But obviously those three cases are no exhaustive! What if $df comes from a module that is not any of these three. Please say.


What does it mean "that M compiles"?? Perhaps "that M directly imports"? But if so its interface is already loaded!


But that's not true:

data Dependencies
  = Deps { dep_mods   :: [(ModuleName, IsBootInterface)]
                        -- ^ All home-package modules transitively below this one
                        -- I.e. modules that this one imports, or that are in the
                        --      dep_mods of those directly-imported modules

That is, we certainly have the transitive closure of of all modules *in the home package" readily to hand. Let's use them.


I still don't understand why you need to do any loadIface stuff

dminuoso added inline comments.Dec 4 2018, 6:55 AM


Examples would take up a lot of space. Would it be fine to insert about 2 pages of comments for this minor addition?


Oh interesting, I was not aware of this. This is my first time touching GHC so all of this is really new to me, it seems as if getEpsAndHpt and lookupIfaceByModule give me access to those ifaces. :)


Ah yeah, when I originally looked at that type it I ignored dep_mods because it did not seem useful to know the import provenance when one of the instances was in the home-package in case of overlapping instances. At the time Joachim agreed, but I realize now that for consistency alone I shouldn't arbitrarily leave out information.

I'll adapt the code to also use dep_mods

Examples would take up a lot of space. Would it be fine to insert about 2 pages of comments for this minor addition?

Generally speaking clarity is more important than compactness. It's easy to skip over a Note! Put yourself in the position of reading this code, from scratch, in five years time. What is it trying to do? How does it do it? Examples are usually a huge help. Thanks.

But don't go overobard!

How is this going, @dminuoso?

bgamari requested changes to this revision.Jan 20 2019, 6:42 PM
This revision now requires changes to proceed.Jan 20 2019, 6:42 PM