Add module export reification + add lookup of Module imports #10391
Needs RevisionPublic

Authored by mgsloan on Jul 2 2018, 7:59 PM.

Details

Reviewers
goldfire
bgamari
Summary

This change adds support for getting a list of names of all the top level exports of a reified module - see
https://ghc.haskell.org/trac/ghc/ticket/10391

This change also clarifies that the first field of ModuleInfo reports module usage info rather than module imports - see https://ghc.haskell.org/trac/ghc/ticket/8489#comment:13 . I think this makes more sense than attempting to provide info about declared imports, since I don't see how that information would be useful.

I noticed that there was no convenient way to reify a module given just its name - you also needed to have a package name and construct a 'Module' value. This adds a 'lookupImportedModule' function which can make a 'Module' value for any imported module. The restriction to just imported modules is to make this more deterministic, since they are guaranteed to be present in the environment. This may also improve recompilation detection, though I suspect that in general compilation dependencies due to reification are not yet properly tracked. It makes sense to me to not address that in this patch.

Test Plan

testsuite/tests/th/T10391.hs

mgsloan created this revision.Jul 2 2018, 7:59 PM
goldfire requested changes to this revision.Jul 3 2018, 7:11 AM

I'm unclear on the design here. Why do we need lookupImportedModule? It looks like, currently, the TH client is expected to build a Module directly by using the Module constructor. Why does this change?

libraries/template-haskell/Language/Haskell/TH/Syntax.hs
1404

That sounds like transitive dependencies. But I think you just mean the module's imports.

1407

What if an exporting module lists constructors (using the pattern keyword allowed in export lists to export constructors without their types) or associated types? Does that change this behavior?

testsuite/tests/th/T10391.hs
8

It would be helpful also to have a test (maybe this same test) that deals with a module with an explicit export list (that omits some declarations).

This revision now requires changes to proceed.Jul 3 2018, 7:11 AM
mgsloan updated this revision to Diff 17183.Jul 4 2018, 1:39 AM
  • Removed implementation of lookupImportedModule
mgsloan updated this revision to Diff 17184.Jul 4 2018, 1:44 AM
mgsloan marked an inline comment as done.
  • Add explicit import list and tycon hiding to test for T10391
mgsloan updated this revision to Diff 17185.Jul 4 2018, 2:04 AM
  • Add explicit import list and tycon hiding to test for T10391

I'm unclear on the design here. Why do we need lookupImportedModule? It looks like, currently, the TH client is expected to build a Module directly by using the Module constructor. Why does this change?

Yes, indeed, it's a convenience function. I typed up an explanation of why the current API is quite inconvenient. However, I've decided that for ease of review and discussion that indeed this change should be done in a separate diff, if at all. Thanks for the feedback!

Based on your other comments, I'm thinking all that's left here is more documentation of the ModuleInfo type to include some of the discussed details. Perhaps the API should account for the pattern details, though? I'm not sure how much it matters, as long as it is documented.

libraries/template-haskell/Language/Haskell/TH/Syntax.hs
1404

It really is often more like usage info - see https://ghc.haskell.org/trac/ghc/ticket/8489#comment:5 . Only transitive in terms of re-exports. This is because interface files do not have actual import information. On revisiting this, though, I've realized that running reifyModule =<< thisModule really does report the imported modules rather than usages. What a mess! I honestly have no idea what this would even be useful for..

I'm beginning to think that instead this should just look like adding reifyModuleExports :: Module -> Q [Name], and leaving reifyModule alone, possibly deprecating it. Or perhaps, alternatively, calling it moduleUsages but having the comment clarify the full, strange, semantics here.

What do you think? Thanks for your time in considering these details.

1407

That is an interesting case. I didn't know that pattern could be used that way. The behavior for this case is that the type appears in the list of exported names, even though it is not in scope.

I've looked around the code for about an hour, added traces, etc, and haven't been able to figure out how the type is appearing in mi_exports but it is not in scope. For example, ghci :browse knows to qualify the name of the un-exported type when showing the info. It seems like it knows that due to the GlobalRdrEnv, but the calculation of that in filterImports for an unqualified import is done directly from AvailInfo. My guess is that I'm just not seeing how this info is present in AvailInfo.

Do you think it is sufficient to just document this quirk? It might be a bit strange for the API to include the constructor name in this list for this case. I did have a much more complicated version of this change that I still have in a stash, which yields something closer to the AvailInfo structure to TH. However, I reasoned that exporting top level names and relying on reify is better.

testsuite/tests/th/T10391.hs
8

Good idea! I've pushed test changes for this.

I've realized that we'll also want a way to query a list of names for top level definitions from before the current splice. This is quite different from reifying exports, but kinda similar. The current situation with reifyModule having different semantics for thisModule vs others seems quite analogous. I propose:

  1. Make reifyModule fail when used on thisModule. Leave it otherwise unchanged. Possibly deprecate or even remove it.
  2. Add reifyCurrentImports to get imports from the current module.
  3. Add reifyCurrentDecls to get the top level declarations that are currently known about (stuff from above the splice).
  4. Add reifyModuleExports, which gets the export info implemented here, when the module interface is available.

Are there better names? I considered reifyCurrentModuleImports etc, but that seems long winded. Could even just go with reifyImports and reifyDecls, I kinda like the look of that.

Perhaps discussions like this belong on trac instead? Not sure what the preferred process is.

Perhaps discussions like this belong on trac instead? Not sure what the preferred process is.

Yes. The general guideline is that Trac comments are intended to stay around a long time, while Phab comments might not necessarily. So any design conversations belong on Trac.

As for your comment above: it's hard for me to understand without the types of the functions you propose. And what does "above this splice" mean for non-declaration splices? Answer on Trac, please. Thanks!

bgamari requested changes to this revision.Aug 21 2018, 11:07 AM

Requesting changes as there appears to still be active discussion here.

This revision now requires changes to proceed.Aug 21 2018, 11:07 AM

Yep, this is on my TODOs - sorry for the delay but got busy with other
things. Hopefully will have a spare cycle soon