Hlint plugin using ghc-lib #166

jneira · 2020-06-19T12:33:10Z

Based in Hlint plugin #43
It includes the new private lib to configure ghc-lib usage and match the hlint one and the double code path to use ghc or ghc-lib to parse the module, as discussed in hlint plugin #32
~~It only has diagnostics for now, and it is not testes, only compiles. The plan is add the apply-refact part as well~~ added!
But reviewing the ghcide submodule code i've found this comment about the getParsedModule action:
https://github.com/alanz/ghcide/blob/3ee692a4cdb98792c371765c9f8adb5237d0a515/src/Development/IDE/Core/Rules.hs#L239-L246
- So it seems the Shake.hs actions should be updated to return two versions of the parsed module to get the raw one, without the haddock part
- It likely will increase the memory used (as @wz1000 noted in irc)
So maybe we should get ride of the parsed module rule and reparsing in hlint in all cases, no only for ghc <= 8.10.1
Closes hlint plugin #32

Pending:

Separate the plugin in its own package, it needs Project architecture to support plugins #164
Make the plugin honour the lsp client configuration to activate it
Show some progress while the refactor is run (it could take some seconds)
Activate/create tests
Merge ghcide pr with client settings: Store the lsp client settings in shakeExtras and create a Rule to get them ghcide#731
Fix tests in ci
Change label of code action to remove not sensible info

After merging the pr (we will create issues)

Continue investigating bad refactors and create issues upstream if needed
Investigate how ghc flags (in cabal file, commnand line or in the hs file) affecting the parse of the module by hlint are handled

ndmitchell · 2020-06-19T14:52:31Z

Yes, the fact that you can have Haddock XOR tokens is super frustrating. Would be great to get that fixed upstream. I think using reparsing always is quite reasonable, although it means that things like CPP are likely to be worse. Passing the right extensions onward would be great though.

pepeiborra · 2020-06-20T19:01:06Z

Note that ghcide HEAD currently returns the raw AST instead of the Haddock one, unlike the hls fork. This means that hovers and completions of identifiers in the same module don't have docs, which is oddly inconsistent and should probably be addressed by returning the Haddock AST instead.

Ailrun · 2020-06-22T08:54:09Z

haskell-language-server.cabal

+  manual: True
+  description: Force dependency on ghc-lib-parser even if GHC API in the ghc package is supported
+
+library hls-ghc-lib


Does stack cradle work with private library now? I thought it hasn't been fixed.

No, it won't, thanks for pointing it out, i forgot to warn about that in pr description. Unfortunately afaik we cant convert it in a common stanza cause the main reason is to be able to have its own flags and dependencies.
There are plans to allow plugins live in its own package, i think this one could be one of them.

Thank you for the clarification. Could you direct me to the description of the plan? I may take and/or tackle some parts of the plan.

@Ailrun sure: #164

shayne-fletcher · 2020-06-22T21:41:50Z

Yes, the fact that you can have Haddock XOR tokens is super frustrating. Would be great to get that fixed upstream.

I'm pretty sure this will fall out of Vlad's haddock work in progress right now.

jneira · 2020-07-10T06:16:22Z

Status
- Diagnostics works, they are shown like in hie
- Refactor applying hints works for simple cases but generate bad ws edits for the HLint modult itself for example. Investigating

jneira · 2020-07-10T06:23:33Z

haskell-language-server.cabal

+  manual: True
+  description: Force dependency on ghc-lib-parser even if GHC API in the ghc package is supported
+
+library hlint-plugin


As noted above the private lib does not work for stack based cradle.
Moreover i have to periodically delete the haskell-language-server library from the cabal store due to

exe\Main.hs:147:9: error: • Couldn't match expected type ‘Ide.Types.PluginDescriptor’ with actual type ‘haskell-language-server-0.2.0.0:Ide.Types.PluginDescriptor’ NB: ‘haskell-language-server-0.2.0.0:Ide.Types.PluginDescriptor’ is defined in ‘Ide.Types’ in package ‘haskell-language-server-0.2.0.0’ ‘Ide.Types.PluginDescriptor’ is defined in ‘Ide.Types’ in package ‘haskell-language-server-0.2.0.0’ • In the expression: Hlint.descriptor "hlint" In the expression: [GhcIde.descriptor "ghcide", Pragmas.descriptor "pragmas", Floskell.descriptor "floskell", Ormolu.descriptor "ormolu", ....] In an equation for ‘basePlugins’: basePlugins = [GhcIde.descriptor "ghcide", Pragmas.descriptor "pragmas", Floskell.descriptor "floskell", ....] | 147 | , Hlint.descriptor "hlint" | ^^^^^^^^^^^^^^^^^^^^^^^^

So i think this plugin must be separated in its own package, after the restructuration of the lib (see #164)

jneira · 2020-07-10T06:25:42Z

plugins/hlint-hls-plugin/src/Ide/Plugin/Hlint.hs

+          let dflags = hsc_dflags hsc
+          let hscExts = EnumSet.toList (extensionFlags dflags)
+          logm $ "getIdeas:setExtensions:hscExtensions:" ++ show hscExts
+          let hlintExts = mapMaybe (GhclibParserEx.readExtension . show) hscExts


Having to cast the types from the "real" ghc used by ghcide and the ghc-lib used by hlint is not good at all.
Not sure if there is an alternative.

jneira · 2020-07-10T07:58:29Z

plugins/hlint-hls-plugin/src/Ide/Plugin/Hlint.hs

+type instance RuleResult GetHlintDiagnostics = ()
+
+rules :: Rules ()
+rules = do


I would need Config here to enable or disable hlint usage. I guess i could get it using getClientConfig :: LSP.LspFuncs Config -> IO Config but i did not find a way to get LSP.LspFuncs Config in Rules aor Action a.
Or maybe should it be disabled in other higher level place?
//cc @alanz

After further investigation:

It seems Config it is not being stored in the shake graph (or at least i have been not able to find it). ghcide seems to ignore it.

So it cant be used in Rules, the actual way to provide diagnostics in the hls plugin api

The rest of plugin types (completion, renaming, formatting) are not using Rules but PartialHandlers, via custom providers so they have access to LspFuncs Config

There is a DiagnosticProvider defined (from hie) that could be used to get the config but it is not implemented in hls

So, there would be two alternatives:

Store the lsp config in the shake graph, creating a Rule for it. We should have in account that it can change at any time by the user, so it have to be changed

Implement the DiagnosticProvider and dont use Rules for provide hlint diagnostics

//cc @alanz

I like the idea of a Shake Rule for the config, that way if the user does change the config the diagnostics should then get updated again right?

jneira · 2020-07-13T22:30:19Z

The output of hlint file.hs --refactor is giving me weird results, calling it directly from the cli over several files of hls itself:

hints are not applied
spaces are inserted
in some regions tokens are being repeated:

importimportqualifiedqualified Language.Haskell.LSP.TypesLanguage.Haskell.LSP.Typesasas LSPLSP
importimportqualifiedqualified Language.Haskell.LSP.Types.LensLanguage.Haskell.LSP.Types.Lensasas LSPLSP

I've posted a bad refactored file example: https://gist.github.com/jneira/70478dee9e9a01d3d10d68218f8fa514#file-hlint-refactored-hs
Refactors works for simpler files so if it is related with windows there should be another factor involved.
I've got same results calling refactors programatically using the plugin.

Pending:

Separate the plugin in its own package, it needs Project architecture to support plugins #164
Make the plugin honour the lsp client configuration to activate it
Show some progress while the refactor is run (it could take some seconds)
Continue investigating bad refactors and create issues upstream if needed

ndmitchell · 2020-07-15T13:08:46Z

Note that the latest version of the refactor tool has a lot of improvements. I'd check you are on the latest version first.

I'd also check line endings on Windows. Perhaps it is broken on Windows style newlines?

jneira · 2020-07-15T20:29:06Z

Note that the latest version of the refactor tool has a lot of improvements. I'd check you are on the latest version first.

I am afraid i was already using the last released apply-refact version 0.8.1.0, as a lib and the cli executable (they both generate the same wrong results)

I'd also check line endings on Windows. Perhaps it is broken on Windows style newlines?

I have CRLF in the simple file that is refactored correctly and in the files that fail, but it still could be a factor involved, i will test it

Thanks for the help!

jneira · 2020-07-15T21:06:45Z

I've tried the refactoring after converting new lines to LF and the result remains the same. The refactoring even has converted the new lines to CRLF again 😞

jneira · 2020-08-16T22:29:07Z

I've opened Store the lsp client settings in shakeExtras and create a Rule to get them ghcide#731 to upstream the needed changes in ghcide for access client settings in rules. The commit was cherry-picked from the ghcide fork used in hls (that one is the used used in this pr)

jneira · 2020-08-25T05:31:09Z

Pending:

Separate the plugin in its own package, it needs Project architecture to support plugins #164
Make the plugin honour the lsp client configuration to activate it
Show some progress while the refactor is run (it could take some seconds)
Continue investigating bad refactors and create issues upstream if needed
Activate/create tests

expipiplus1 · 2020-08-26T17:54:28Z

Gave this a spin and it seems to be working well!

I resurrected the old hlint commands for vscode-haskell (expipiplus1/vscode-hie-server@6c1d055) applyAll seems to work but applyOne does not. It's almost certainly on my end though!

expipiplus1 · 2020-08-27T03:36:35Z

Not 100% sure where the issue lies (in hls or coc.nvim) but the diagnostic messages seem to include the filepath and line number which seems a little redundant:

Co-authored-by: Pepe Iborra <pepeiborra@me.com>

to check if a hlint hint should trigger a code action

jneira · 2020-10-29T06:37:56Z

Ok, i gave up on flaky tests and copied the way ghcide run the test suite:

cabal test --test-options="-j1 --rerun-update" || cabal test --test-options="-j1 --rerun" || LSP_TEST_LOG_COLOR=0 LSP_TEST_LOG_MESSAGES=true LSP_TEST_LOG_STDERR=true cabal test --test-options="-j1 --rerun"

The test suite is retried three times with --rerun and the las one outputs stderr and lsp server messages to help debug them.
I suspect we are using the first cabal test to warm the ide specific cache: the shake database and hie files and it makes the tests pass in the two last executions. Imo that should be taken in account in lsp-test, cause tests are failing even if they does not reach the session timeout so it seems there is another timeout involved (???)

@ndmitchell when you have some time, could you take one last look? i would prefer to merge with your approval 🙂

ndmitchell

Looks great to me! Keen to try it out in practice 😄

jneira mentioned this pull request Jun 19, 2020

hlint plugin #32

Closed

Ailrun reviewed Jun 22, 2020

View reviewed changes

jneira force-pushed the hlint-plugin-ghc-lib branch from 4d29002 to 7108e61 Compare June 28, 2020 21:21

jneira force-pushed the hlint-plugin-ghc-lib branch from d071ebb to f32cde1 Compare July 7, 2020 07:07

jneira commented Jul 10, 2020

View reviewed changes

jneira force-pushed the hlint-plugin-ghc-lib branch 4 times, most recently from 1916bdf to 1d3d0d0 Compare July 15, 2020 07:23

jneira mentioned this pull request Jul 16, 2020

Show documentation on hover for symbols defined in the same module haskell/ghcide#691

Merged

fendor mentioned this pull request Jul 28, 2020

Document what is required to get HLint working #249

Closed

jneira force-pushed the hlint-plugin-ghc-lib branch from 1d3d0d0 to bd1fd28 Compare August 2, 2020 13:03

jneira force-pushed the hlint-plugin-ghc-lib branch 2 times, most recently from 1708ffb to 8c3fd42 Compare August 11, 2020 22:25

jneira mentioned this pull request Aug 16, 2020

Store the lsp client settings in shakeExtras and create a Rule to get them haskell/ghcide#731

Merged

jneira force-pushed the hlint-plugin-ghc-lib branch 2 times, most recently from 79210d3 to 1ddacf7 Compare August 26, 2020 08:02

jneira and others added 21 commits October 28, 2020 07:36

Use GHC.Compat from ghcide

814ad46

Co-authored-by: Pepe Iborra <pepeiborra@me.com>

Restore imports for missing defs

95dde7c

Comment about the need to use tmp file

fc4ea30

Remove hlintOn logging

73f669d

Remove hlint parser error from diagnostics

21b47f2

Comment about GetHlintDiagnostics RuleResult

d558e9a

Comment how hlint diags are recomputed

1245439

Correct identation

e26b23f

Use diagnostic code prefix "refact:"

7f5a4ef

to check if a hlint hint should trigger a code action

Return hlint diags if no ghc parse errors

eeb62e0

Combine hlint and hscEnv extensions

929aaea

Correct replace params

f5d4311

Correct component name

1c1f7f3

Fix apply refactoring

8b8e568

Update tests to match new diags and ca's

c35cdda

Add GHC810 explicit version

0e72151

Set hlint code by ghc version

5d4d2e2

Use hlint-3.2

f1751b7

Remove ghc specific diag title

b549506

Add and update comments

ff0e4c7

Provide evidence edits changes hlint diags

01f8fd8

jneira force-pushed the hlint-plugin-ghc-lib branch from 61df7a6 to 01f8fd8 Compare October 28, 2020 06:51

Rerun test suite to avoid flaky tests

94b7a3e

ndmitchell approved these changes Oct 29, 2020

View reviewed changes

jneira merged commit 5673556 into haskell:master Oct 29, 2020

andys8 mentioned this pull request Nov 11, 2020

README: hlint is supported #587

Merged

jneira deleted the hlint-plugin-ghc-lib branch December 3, 2020 20:35

pepeiborra pushed a commit that referenced this pull request Dec 27, 2020

More things are in the latest stack snapshot (#166)

97e5731

jneira mentioned this pull request Mar 25, 2022

Cabal: Reexport all of Cabal-syntax haskell/cabal#8064

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Hlint plugin using ghc-lib #166

Hlint plugin using ghc-lib #166

jneira commented Jun 19, 2020 •

edited

Loading

ndmitchell commented Jun 19, 2020

pepeiborra commented Jun 20, 2020

Ailrun Jun 22, 2020

jneira Jun 22, 2020

Ailrun Jun 22, 2020

jneira Jun 22, 2020

shayne-fletcher commented Jun 22, 2020

jneira commented Jul 10, 2020 •

edited

Loading

jneira Jul 10, 2020 •

edited

Loading

jneira Jul 10, 2020

jneira Jul 10, 2020

jneira Jul 15, 2020

lukel97 Jul 30, 2020

jneira commented Jul 13, 2020 •

edited

Loading

ndmitchell commented Jul 15, 2020

jneira commented Jul 15, 2020

jneira commented Jul 15, 2020

jneira commented Aug 16, 2020

jneira commented Aug 25, 2020

expipiplus1 commented Aug 26, 2020

expipiplus1 commented Aug 27, 2020

jneira commented Oct 29, 2020 •

edited

Loading

ndmitchell left a comment

Hlint plugin using ghc-lib #166

Hlint plugin using ghc-lib #166

Conversation

jneira commented Jun 19, 2020 • edited Loading

ndmitchell commented Jun 19, 2020

pepeiborra commented Jun 20, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shayne-fletcher commented Jun 22, 2020

jneira commented Jul 10, 2020 • edited Loading

jneira Jul 10, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jneira commented Jul 13, 2020 • edited Loading

ndmitchell commented Jul 15, 2020

jneira commented Jul 15, 2020

jneira commented Jul 15, 2020

jneira commented Aug 16, 2020

jneira commented Aug 25, 2020

expipiplus1 commented Aug 26, 2020

expipiplus1 commented Aug 27, 2020

jneira commented Oct 29, 2020 • edited Loading

ndmitchell left a comment

Choose a reason for hiding this comment

jneira commented Jun 19, 2020 •

edited

Loading

jneira commented Jul 10, 2020 •

edited

Loading

jneira Jul 10, 2020 •

edited

Loading

jneira commented Jul 13, 2020 •

edited

Loading

jneira commented Oct 29, 2020 •

edited

Loading