LSP window message log recorder #2750
Conversation
pepeiborra
requested review from
drsooch,
eddiemundo and
michaelpj
and removed request for
drsooch
There was a problem hiding this comment.
So this is what you meant by using a plugin to get the env... nice. Additionally, the issue of how to choose which specific messages to send to window is sidestepped by sending all the ones with Error.
The ghcide/exe version doesn't have the issue tracker url message but that's fine since most people will be using hls rather than ghcide.
There are a few other places that use SWindowMessage to add to the logging TODO list:
splice plugin
tactics plugin
fourmolu plugin
retrie plugin
extendImportHandler plugin in Completions.hs
displayTHWarning in Rules.hs
Sorry for not being too active, been distracted recently.
Thanks. I'll take a look at those and see if they can be changed to log errors meaningfully. |
Yes - this is just a coarse simple way of selecting what messages get shown. The finer way to do this (and what we do in Sigma IDE where we have lots of telemetry) is to do a case analysis on the entire log type to filter exactly what things we want to record (and how). |
* failing to set the unsafe dynflags is an error * makeLspRecorder * include link to the issue tracker * avoid double popup * catch another ghc lib dir error
There was a problem hiding this comment.
I also have a branch that implements this, here's the commit: michaelpj@57ac4a1
The main difference is the way we get the LanguageContextEnv to the Recorder. I'm really not a fan of the plugin trick, I think we should just pass it in. I don't mind the IORef way, but I think we could just pass it down as an ordinary parameter instead of smuggling it in like this.
I also want to include a recorder that sends window/logMessage notifications, but I'll do that separately.
|
|
||
| renderDoc :: Doc a -> Text | ||
| renderDoc d = renderStrict $ layoutPretty defaultLayoutOptions $ vcat | ||
| ["Unhandled exception, please check your setup and/or the [issue tracker](" <> issueTrackerUrl <> "): " |
There was a problem hiding this comment.
It's not necessarily an exception, right? It could be any other error.
There was a problem hiding this comment.
True, I copied this message over from an earlier exception site - will change to error
There was a problem hiding this comment.
And indeed, since we send it as an error message, we don't need to tell the user again that it's an error. Including the issue link is maybe still helpful.
| case mbenv of | ||
| Nothing -> liftIO $ atomicModifyIORef'_ backLogRef (it :) | ||
| Just env -> sendMsg env it | ||
| -- the plugin captures the language context, so it can be used to send messages |
There was a problem hiding this comment.
I don't really like this approach. It's extremely indirect. My branch just has a function LanguageContextEnv config -> Recorder ... and then we have to pass it along until we reach initialize.
There was a problem hiding this comment.
I think we could relatively easily adapt this to just pass the IORef to be filled in rather than going via a plugin...
There was a problem hiding this comment.
Please send a PR! I suspect that the plugin solution is more modular, as it abstracts away the passing of the mutable state around.
There was a problem hiding this comment.
Yes, but it does so by using a mechanism that really isn't designed for that. Now the recorder mechanism is intertwined with the plugin mechanism, which it should have nothing to do with that. We shouldn't have to fix the loggers if we change how plugins work; that seems anti-modular to me.
| IO (Recorder (WithPriority Text), PluginDescriptor c) | ||
| makeLspShowMessageRecorder = do | ||
| envRef <- newIORef Nothing | ||
| -- messages logged before the LSP stream is initialized will be sent when it is |
There was a problem hiding this comment.
This is neat, my version just didn't record until it gets setup when the LSP server is ready.
| } | ||
| return (recorder, plugin) | ||
|
|
||
| sendMsg :: MonadUnliftIO m => LanguageContextEnv config -> WithPriority Text -> m () |
There was a problem hiding this comment.
You don't actually need the MonadUnliftIO constraint: just declare this function to run in IO, and call liftIO at the use site. Then you can get rid of all the other stuff.
There was a problem hiding this comment.
Good catch - I'll drop the instance before @wz1000 gets a chance to abuse it :)
This PR adds a new
Recorderthat emits LSPWindowShowMessagenotifications, and then uses it to surface errors to the user.