Simplify and deduplicate ModSummary logic #884
Conversation
| let preproc_warnings = diagFromStrings "parser" DsWarning preproc_warns | ||
| ms <- getModSummaryFromBuffer filename modTime dflags parsed' contents |
There was a problem hiding this comment.
@mpickering need your help here. Does removePackageImports need to be called before we compute the ModSummary? Because we don't recompute the ModSummary here anymore, we just re-use the one we computed before.
There was a problem hiding this comment.
My take on this: since we don't allow GHC to resolve imports anymore, the only place where import resolution happens is in our code, namely in GetLocatedImportsRule. So the removal of package imports can be inlined there in order to preserve the AST and original imports as much as possible. Otherwise plugins like the HLS import lens or code actions which use the AST might end up "losing" package imports accidentally.
There was a problem hiding this comment.
OK, but I'm confused. GetLocatedImportsRule uses GetModSummaryWithoutTimestamps, which doesn't call removePackageImports, so it should never have been affected by it. The only things which are affected by removePackageImports are things which use the modsummary from the parsed module.
There was a problem hiding this comment.
Back in the day when removePackageImports was added, GetModSummary didn't exist and GetLocatedImportsRule still used the parsed AST to extract the imports.
There was a problem hiding this comment.
Sounds like we need a testcase. How do I invoke this code path? Will a multi-cradle with a file using PackageImports to import a conflicting module from two different packages do it?
There was a problem hiding this comment.
Looks like we can just remove it, since the logic is already present in findImports: https://github.com/haskell/ghcide/blob/master/src/Development/IDE/Import/FindImports.hs#L126
There was a problem hiding this comment.
Test case is the generic-lens repo. I think the situation was as you described (conflicting module from two different packages)
wz1000
changed the title
|
Allocation seems to consistently be slightly decreased: @pepeiborra I often observe that results for |
|
Note that the document symbol benchmarking weirdness shows up in the final results, but reversed this time: |
* Simplify and dedup parsing logic * delete removePackageImports * add dependencies on included files * hlint
* Simplify and dedup parsing logic * delete removePackageImports * add dependencies on included files * hlint
* Simplify and dedup parsing logic * delete removePackageImports * add dependencies on included files * hlint
We can re-use the modsummary when we parse, don't need to generate a new one.