-
-
Notifications
You must be signed in to change notification settings - Fork 367
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
GHC 9.4 compatibility + Multiple Home Units #2994
Conversation
Perhaps we should move all the code actions that depend on |
I've added one commit which adds the required files for GHC 9.4 for nix. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM for wingman
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Although I haven't tested in my environment, it seems ok as for the Splice Plugin.
Just curious: is this change to make the splice plugin dependent on hls-refactor-plugin
also fixes the Splice Plugin also on GHC 9.2, or do we need an extra effort?
f425458
to
4232bd3
Compare
3308000
to
6a6ffa1
Compare
a888817
to
88fd42f
Compare
I've just added naively the different nix lines. The configuration file comes from a copy of the one for 9.2. With that, we can open a shell with `nix develop .\#haskell-language-server-941-dev` and type `cabal build`. (cherry picked from commit 48084ab)
@guibou could you help with the nix failure? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor comments only, as the slice of HLS that I understand shrinks with every GHC update.
This is a massive amount of work, thank you @wz1000 !!!
then uses_ GetModSummary fs | ||
else uses_ GetModSummaryWithoutTimestamps fs | ||
return (map (NodeKey_Module . msKey) dep_mss) | ||
ms <- msrModSummary <$> use_ GetModSummary file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can use GetModSummaryWithoutTimestamps
when fullModSummary
is false here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, its for the new ModuleGraph
which includes all ModSummary
s paired with their direct dependencies (Earlier GHC versions only had the ModSummary
s). It shouldn't be used for much other than mgTransDeps
gives us a reasonably fast and cached way of querying for transitive dependencies (which is used in hscCompileCoreExprHook to get all the Linkable
s needed for a splice). I don't think omitting timestamps will matter here, especially since we omitted timestamps in the ModSummary
s we put into the ModuleGraph
on older GHCs.
session' <- liftIO $ mergeEnvs hsc mss inLoadOrder depSessions | ||
let inLoadOrder = map (\HiFileResult{..} -> HomeModInfo hirModIface hirModDetails Nothing) ifaces | ||
#if MIN_VERSION_ghc(9,3,0) | ||
mss_imports <- uses_ GetLocatedImports (file : deps) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A comment explaining why collecting the mod summaries of the direct dependencies here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You were right, there was something fishy here. We were including the ModSummary
for the current module in GHC 9.4 but not before. I've changed this to always include the current ModSummary
, which allows us to not query imports of direct dependencies. We still need to get the ModSummary
s of direct dependencies in order to know their Module
s, which we need to reference in the ModuleGraph
.
I've written a short note documenting all the assumptions we now make.
1269371
to
bb49204
Compare
Yes, for sure, I'll have a look at it tomorrow. |
TODO