-
-
Notifications
You must be signed in to change notification settings - Fork 414
Capture output of eval plugin and some girls scout changes
#4726
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
base: master
Are you sure you want to change the base?
Conversation
199c13f to
6d59fcd
Compare
fabcd5e to
11a0f3f
Compare
eval plugin and some girls scout changes
fendor
left a comment
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.
Needs a proper test in hls-eval-plugin, but otherwise looks good to me, thank you for the clean up! Feel free to merge once you've added a test :)
| Variable not in scope: cls :: t0 -> t | ||
| Data constructor not in scope: C | ||
| WAS Variable not in scope: cls :: t0 -> t | ||
| WAS Data constructor not in scope: C | ||
| NOW Illegal term-level use of the class `C' | ||
| NOW defined at <interactive>:1:2 | ||
| NOW In the first argument of `cls', namely `C' | ||
| NOW In the expression: cls C | ||
| NOW In an equation for `it_a5Zks': it_a5Zks = cls C | ||
| NOW Variable not in scope: cls :: t0_a5ZlS[tau:1] -> t1_a5ZlU[tau:1] |
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.
Slightly weird, probably needs to be fixed one way or another?
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.
I didn't realize the eval plugin changed some other results. Thanks for picking that up. I will investigate.
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.
I fixed those WAS NOW errors, one question though:
Does the plugin also evaluate all actions in the same comment block in your case?
If I evaluate one action in this comment block, all of them get evaluated and a lot of WAS NOW messages get added. Some of them are wrong. I am not sure if this PR added this discrepancy, or if the problem was already there before. I will check.
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.
After locally running, I can confirm I also get these errors, independent of your changes.
This is how this file looks like after running eval:
diff --git a/plugins/hls-eval-plugin/src/Ide/Plugin/Eval/Handlers.hs b/plugins/hls-eval-plugin/src/Ide/Plugin/Eval/Handlers.hs
index 1f19b5b47..0ae407bcb 100644
--- a/plugins/hls-eval-plugin/src/Ide/Plugin/Eval/Handlers.hs
+++ b/plugins/hls-eval-plugin/src/Ide/Plugin/Eval/Handlers.hs
@@ -426,8 +426,14 @@ A, possibly multi line, error is returned for a wrong declaration, directive or
Some flags have not been recognized: -XNonExistent
>>> cls C
-Variable not in scope: cls :: t0 -> t
-Data constructor not in scope: C
+WAS Variable not in scope: cls :: t0 -> t
+WAS Data constructor not in scope: C
+NOW Illegal term-level use of the class `C'
+NOW defined at <interactive>:1:2
+NOW In the first argument of `cls', namely `C'
+NOW In the expression: cls C
+NOW In an equation for `it_a2KTO': it_a2KTO = cls C
+NOW Variable not in scope: cls :: t0_a2KVe[tau:1] -> t1_a2KVg[tau:1]
>>> "A
lexical error in string/character literal at end of input
@@ -437,16 +443,20 @@ in GHCi or doctest. This allows it to be used as a hack to simulate print until
get proper IO support. See #1977
>>> 3 `div` 0
-divide by zero
+WAS divide by zero
+NOW *** Exception: divide by zero
>>> error "Something went wrong\nbad times" :: E.SomeException
-Something went wrong
+WAS Something went wrong
+NOW *** Exception: Something went wrong
bad times
Or for a value that does not have a Show instance and can therefore not be displayed:
>>> data V = V
>>> V
-No instance for (Show V) arising from a use of ‘evalPrint’
+WAS No instance for (Show V) arising from a use of ‘evalPrint’
+NOW No instance for `Show V' arising from a use of `evalPrint'
+NOW In a stmt of an interactive GHCi command: evalPrint it_a2LvA
-}
evals :: Recorder (WithPriority Log) -> Bool -> TEnv -> DynFlags -> [Statement] -> Ghc [Text]
evals recorder mark_exception fp df stmts = doThere 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.
I think, we probably need to ignore these.
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.
Alright! Thanks for confirming. I will make some room this week ;-).
Did we conclude on whether we should limit the maximum output? I think we decided not to have a limit, did we?
5ade953 to
5ef2000
Compare
|
This could be an interesting surprise for some users. I often eval expressions that produce > 1000 lines of IO. In my case detailed pretty printed logs. If this was to suddenly be rendered in the code editor I'd need to change my workflow. If i wasn't expecting it, it would be quite confusing. |
|
Yes, if this is your workflow. In theory, we could limit the amount of output. The underlying library redirects output to a temporary file and then reads this temporary file. We could amend the function so that it only takes the first N lines of this file (or bytes, or whatever). |
This PR contains some girl-scout changes as well as changes to how the
evalplugin treatsstdoutandstderr.Eval plugin
We now capture output to
stdoutandstderrinduced as a possible side effect by the statement being evaluated. This is fragile because the output may be scrambled in a concurrent setting when HLS is writing to one of these file handles from a different thread.Fixes #1977, most likely also #4390 (although this is debatable, we could do something more sophisticated).
Girl scout changes
evalplugin in-source documentation