Restore eval plugin build for GHC 9.2 #2669
Conversation
|
Progress report: I can now find all comments containing code to evaluate. Tests for the plugin are now: There is two cases of failures:
-- >>> :type +no 42
--- parse error on input ‘+’
+-- parse error on input `+'
|
guibou
force-pushed
the
fix_ghc92_eval_plugin
branch
2 times, most recently
from
3238e4e to
ddb3aaa
Compare
guibou
force-pushed
the
fix_ghc92_eval_plugin
branch
from
ddb3aaa to
54e352e
Compare
|
MR is ready for review. I do have one point to discuss, see inline comments. |
Using `HasCallStack`, `testCase` can no pinpoint the call location instead of pointing inside the utility function.
It restores the eval plugin. Now annotations with comments are found by walking the AST and locating specific annotations. In order to fix unit test, I implemented a new golden test function which accepts a different naming scheme depending on the GHC version.
Eval plugin does not report progress, I don't understand why.
guibou
force-pushed
the
fix_ghc92_eval_plugin
branch
from
b5d4e65 to
4dcca04
Compare
|
All tests are fixed! |
| @@ -295,8 +285,7 @@ runEvalCmd plId st EvalParams{..} = | |||
| dbg "LOAD RESULT" $ asS loadResult | |||
| case loadResult of | |||
| Failed -> liftIO $ do | |||
| hClose logHandle | |||
| err <- readFile logFilename | |||
| let err = "" | |||
There was a problem hiding this comment.
Yes it is. Previously the err was filled by the logging architecture (see discussion here: #2669 (comment)) and @pepeiborra agreed that it can be removed. We can pull the thread even more by removing this err value, changing the return type from Either to Maybe. I tried to reduce the amount of changes in order to restore this plugin.
There was a problem hiding this comment.
If it's something to do in a follow-up PR, please leave a comment. Otherwise future readers (possibly future you :) ) will be mystified.
There was a problem hiding this comment.
I think the simplest thing to do is fill the err with asS loadResult, or something even simpler like "failed to load module". I feel like it will be hard to change the Either to a Maybe because type CommandFunction idestate a = ideState a -> LspM Conflg (Either ResponseError Value), unless we're changing CommandFunction which would change lots of things should just put some reason in there instead.
There was a problem hiding this comment.
Thank you, I'll address that in a followup commit.
| @@ -53,22 +56,44 @@ queueForEvaluation ide nfp = do | |||
| EvaluatingVar var <- getIdeGlobalState ide | |||
| modifyIORef var (Set.insert nfp) | |||
|
|
|||
| #if MIN_VERSION_ghc(9,0,0) | |||
| #if MIN_VERSION_ghc(9,2,0) | |||
| getAnnotations :: Development.IDE.GHC.Compat.Located HsModule -> [LEpaComment] | |||
There was a problem hiding this comment.
maybe nest these two in a where clause of apiAnnComments' so that the same names are defined in all versions?
There was a problem hiding this comment.
I'll address that in a followup commit.
|
Thank you for the merge. I was in the middle of addressing the comments of @michaelpj, I'll do that in a followup MR ;) |
The eval plugin is building again and working partially. The codelens
(and evaluation) only happen for code in module comment, code appearing
in the top level comments / haddock are ignored. I need to walk the AST
to locate them.
This is work in progress for #2179.