Enhancements to top-level signatures #232
Conversation
| @@ -104,7 +104,8 @@ typecheckModule (IdeDefer defer) packageState deps pm = | |||
| let modSummary = pm_mod_summary pm | |||
| modSummary' <- initPlugins modSummary | |||
| (warnings, tcm) <- withWarnings "typecheck" $ \tweak -> | |||
| GHC.typecheckModule $ demoteIfDefer pm{pm_mod_summary = tweak modSummary'} | |||
| GHC.typecheckModule $ enableTopLevelWarnings | |||
There was a problem hiding this comment.
I’ve brought this up a couple of times before but I really dislike the idea of enabling warnings by default. If users care about these warnings, they should enable them in their cabal file. I don’t want to see a bunch of warnings in my IDE that I haven’t enabled. If we manage to make this work such that we only show the codelenses by default but not the warnings, I could be convinced that this is fine but given that the codelenses are based on diagnostics this doesn’t seem trivial and I think the complexity of trying to make this work is probably not worth it.
There was a problem hiding this comment.
I am happy both ways. I can attempt to write something which remembers whether these warning were already in effect at the beginning, so that we can filter out diagnostics if needed. But I agree: it might be quite complicated.
@ndmitchell you seemed to have an idea of how to achieve this in #231. Any suggestions?
There was a problem hiding this comment.
Code which doesn't have a warning is more likely to have top-level signatures omitted, and more likely to benefit from having them as code lenses, since I see them as helpful docs, rather than a mechanism to easily add them. I separately think that turning on some warnings in the IDE vs compiler is reasonable, but see that as unrelated to this patch.
I'm not convinced the code is that complex. You turn on the warning always, but demote the warning to an info if errMsgReason is missing top level signatures. At some point (if we want) we ditch info level diagnostics before sending them to the LSP client. A few lines here, a few lines (that will be generally useful) in the diagnostic sending code.
There was a problem hiding this comment.
I am admittedly a bit biased here, I don’t recall the last time I worked on a project that didn’t have these warnings enabled and that seems to be quite common in the ecosystem in general, so I’m somewhat hesitant to add any complexity (even if it’s probably not too much) to cater to that usecase.
That said, the argument that we’ll need this for other warnings eventually that are less popular is compelling so I’m fine with adding it.
There was a problem hiding this comment.
None of my projects have it turned on :)
There was a problem hiding this comment.
My loss is not getting to work with you awesome guys every day anymore!
There was a problem hiding this comment.
It took a while, but finally I managed to "degrade" warnings about missing signatures to info level if they were not explicitly enabled by the user. They look great in VSCode, since they are shown with a blue underline.
| @@ -53,14 +55,16 @@ codeLens | |||
| -> CodeLensParams | |||
| -> IO (List CodeLens) | |||
| codeLens _lsp ideState CodeLensParams{_textDocument=TextDocumentIdentifier uri} = do | |||
| diag <- getDiagnostics ideState | |||
| -- diag <- getDiagnostics ideState | |||
There was a problem hiding this comment.
It looks like this line can be removed?
I’ve tried out the new behavior added in #232 and it is extremely noisy, in particular, the warnings enabled by Opt_WarnMissingLocalSignatures which I don’t want to see. I think for now it makes more sense to disable them completely. We still show the codelenses which is somewhat on the edge but at least they don’t clutter the problems tab/CLI output so I’m fine with keeping those even if they are not enabled.
* Try adding a dependency on TypeCheck * Show warning regardless of the status of -Wall * Try diagnostics after type checking, again * Use `useE` instead of `use_` to not get a `BadDependency` error * Degrade information about signatures if not present in user options * Fix tests * Better suggested signatures for polymorphic bindings * Remove old comment
* Try adding a dependency on TypeCheck * Show warning regardless of the status of -Wall * Try diagnostics after type checking, again * Use `useE` instead of `use_` to not get a `BadDependency` error * Degrade information about signatures if not present in user options * Fix tests * Better suggested signatures for polymorphic bindings * Remove old comment
* Try adding a dependency on TypeCheck * Show warning regardless of the status of -Wall * Try diagnostics after type checking, again * Use `useE` instead of `use_` to not get a `BadDependency` error * Degrade information about signatures if not present in user options * Fix tests * Better suggested signatures for polymorphic bindings * Remove old comment
I am trying to implement #231. However, I think I have done something very wrong, because I get a
BadDependencyerror message.@cocreature @ndmitchell could you provide some help? Thanks in advance :)