Add 'imported-from' command.

[DanielG]

[carlohamalainen]

So I'm right to clone the main repo and push directly to this branch?

[DanielG]

Yeah, you can just add this repo as a remote to your existing repo though, no need to re-clone.

[lierdakil]

WRT tests, I should probably tweak test code so it fails early, but here's an actual problem:

>>>> Cabal errors begin
Warning: /home/travis/build/DanielG/ghc-mod/ghc-mod.cabal: Ignoring unknown
section type: custom-setup
cabal: Could not resolve dependencies:
trying: ghc-mod-5.6.0.0 (user goal)
trying: base-4.8.2.0/installed-0d6... (dependency of ghc-mod-5.6.0.0)
next goal: haddock-api (dependency of ghc-mod-5.6.0.0)
rejecting: haddock-api-2.17.3, 2.17.2, 2.16.1, 2.16.0 (conflict: ghc-mod =>
haddock-api<2.16)
rejecting: haddock-api-2.15.0.2, 2.15.0.1, 2.15.0 (conflict:
base==4.8.2.0/installed-0d6..., haddock-api => base>=4.3 && <4.8)
Dependency tree exhaustively searched.
<<<< Cabal errors end

[lierdakil]

Okay, 7.10 fixed. 7.8 is still failing, in part due to test files not compiling under 7.8 (f.ex. ImportedFrom03.hs has some instance-related fails), and in part I honestly don't know why, but it throws GMENoVisibleExports.

[carlohamalainen]

The failures on 7.8.4 are my fault, have reproduced them on my laptop. I'll look into it.

[carlohamalainen]

@DanielG I've gone through all your comments, happy for final review now.

[DanielG]

I'm doing a cleanup pass right now so don't bother fixing any of the things I'm about to comment on. Those are just FYI.

[DanielG]

@carlohamalainen I think using ModIfaces directly might make this a lot more reliable. Have you looked into that already by any chance?

[DanielG]

Hmm nah that's a bad idea too.

I do wonder why you need to work with haddock's interfaces though? Doesn't GHC already have a superset of the information in there or am I missing something?

[carlohamalainen]

Haddock provides instVisibleExports so I figured it best not to reimplement the wheel.

[DanielG]

What I really don't understand is why you have so many different ways to add/filter stuff out of the result set. Seems to me like the code could be much simpler. The thing that irks me most is how you use the pretty printer on things that will return syntax elements and not just identifiers (qualifiedNameAt) and then try parse the identifiers out of that soup again (refineRemoveHiding).

I mean maybe I'm just not understanding it but it does seem quite messy.

[carlohamalainen]

When I first wrote ghc-imported-from I didn't foresee how complex it would become, parsing and filtering the soup of strings.

Is there a function in the GHC API to go from a file, line, col to a Name? Or from LHsBind Id?

[DanielG]

No, most likely not but the names surely is in the AST so it is just a matter of traversing that (possibly using syb like we do for the type command).

[lierdakil]

Um... type Id = Var, instance NamedThing Var, so you can basically hsBindToName (LHsBind id) = getName id, right?

[DanielG]

Oh right, I missed the LHsBind Id bit.

[lierdakil]

So... let me get this straight. Sorry if I'm off the mark entirely.

Most code here is just dealing with getting module and package name of a named thing, yes? So couldn't we just getName on said named thing, which will get us a Name. Then we can apply nameModule_maybe to Name and get Module.

Now Module is a product type of UnitId (or PackageKey it's called in GHC-7.10) and ModuleName. Latter is self-explanatory, and we can get String representation with moduleNameString, and former we could use in lookupPackage to get PackageConfig, which will get us haddockHTMLs (which we can then happily filter by comparing to ModuleName with dots replaced by dashes), and package version, among other things.

Last step is a little more complicated with GHC-7.8, but it's essentially the same (just we'd have to dissect DynFlags a bit to get lookupPackage to work).

Am I missing something here?

[carlohamalainen]

What does nameModule_maybe give for something like String? Will it tell me GHC.Base.String, which has no Haddock page, or Prelude, which is the sensible place to send the user since that is where the name is ultimately exported from?

Currently imported-from will say this:

base-4.8.2.0:GHC.Base.String Prelude https://hackage.haskell.org/package/base-4.8.2.0/docs/Prelude.html

[lierdakil]

You're right, nameModule will get an originating module, not an immediate module from which symbol was imported. Can't think of a way to rectify that from the top of my head, apart from stopping on Parser stage and working from there, but there might be one.

[lierdakil]

Okay, here's another idea.

We can limit ourselves to working with RenamedSource, which has list of used (located) Names and list of imports (almost verbatim from ParsedModule) separately. Then we can iterate over imports: for each ModuleName, we can get Module with findImportedModule (since source is renamed/typechecked, all imports should be in current HscEnv), and we can get renamed module exports with getModuleInfo and modInfoExports (or possibly we could use modInfoIsExportedName, since we don't actually need all the exports). Then just filter until we find module that imports symbol we're interested in. Hiding etc can be used as a tiebreaker.

Compared to current approach, working with RenamedSource allows to work with fully-qualified Names directly, so most of the awkwardness should be gone.

This is a bit more work then my previous idea, but could probably be condensed to something like 100-200 LOC instead of 900.

[lierdakil]

P.S. findModule instead of findImportedModule would probably be better in this case, since we're pretty sure that it wouldn't throw, and result doesn't need unwrapping.

[lierdakil]

Here's an extremely rough sketch of what I was thinking: 3f282a8

Of course, it's not good code, there are a few irrefutable patterns that might be refuted, it doesn't do exactly the same, findSpanName could be more specific, and performance is probably horrible due to use of list operations like nub and \\. But should be enough to get my point across.

[lierdakil]

I think at this point this is "good enough". I could probably refine this ad infinum given the chance, but I think I have to stop at some point, and this particular point seems as good as any.

@DanielG, if you could review this, that would be cool.

[DanielG]

Looks good, though we had a mode to just print the package+module name instead of the full haddock path (see comments following https://github.com/DanielG/ghc-mod/pull/810#issuecomment-236202896) . @lierdakil did you remove that?

Also we seem to have reverted to using error to report user facing errors which we fixed before, see fa1417af7706b4cc05f12e3a8f125f5ecb56d98f for example.

[lierdakil]

Looks good, though we had a mode to just print the package+module name instead of the full haddock path (see comments following #810 (comment)) . @lierdakil did you remove that?

Yep, missed that bit. I basically reimplemented the whole deal from scratch while stealing bits and pieces from @carlohamalainen's implementation when appropriate, so I was bound to miss something.

Although I should point out that while hackage url can be, in general, inferred from package id, module and symbol, local haddock path can not, and current implementation prefers the latter if it's available. So there's that. I think we can solve this problem in another way though, see below.

Also we seem to have reverted to using error to report user facing errors which we fixed before, see fa1417a for example.

Reason being, those errors never leave the scope of the function, so my reasoning was that why define exceptions that are local to the module. Now that I think about it, I should probably make it throw GME exceptions and handle those downstream, since it might be more useful to library users. Sorry, didn't think about that -- was OD'ing on caffeine at the time, so I kinda rushed a bit.

Also while on the topic of library users. I've been bitching about library interface for a while (#696), and now seems as good a time as any to start imposing at least some structure on the returned values. So what do you think about returning [ImportedFromResult] instead of String? We can make an instance of TextOutput ImportedFromResult (and also TextOutput a => TextOutput [a]) to convert that to String in ghc-mod binary. Also we can dump the whole batch of information out, including package name and hackage url here, without having to pass a ton of flags to the function itself (we could instead add those into TextOutput instances -- e.g. by defining an instance-specific output config) Thoughts?

[DanielG]

Returning proper datatypes instead of strings sounds lovely actually I don't see why we shouldn't.

[carlohamalainen]

I updated this PR against the master branch but I'm not seeing any status from travis - is there something else that I have to do?

[DanielG]

We've stopped using travis a while ago. The builds are now on Gitlab(CI) but it looks like the repo mirroring has broken, I'm looking into that. If you have a gitlab account I can add you there so you can push this branch to the other repo as well, which will trigger a build.

FYI you can actually configure git to push to multiple repos at once when you push to a remote, see https://gist.github.com/bjmiller121/f93cd974ff709d2b968f

[carlohamalainen]

Just made an account: https://gitlab.com/carlohamalainen

[DanielG]

Done

[DanielG]

@carlohamalainen are you still working on this? Do you need help with the new CI setup or something?