Skip to content
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

Support hls-hlint-plugin and hls-stylish-plugin for ghc9.0 and ghc9.2 #2854

Merged
merged 17 commits into from
Apr 28, 2022

Conversation

July541
Copy link
Collaborator

@July541 July541 commented Apr 25, 2022

No description provided.

@pepeiborra
Copy link
Collaborator

What about 9.0? Should be able to play the same trick

@July541
Copy link
Collaborator Author

July541 commented Apr 26, 2022

What about 9.0? Should be able to play the same trick

Haha, sorry for my unclear words, I just want to express that this pr will support hlint-plugin and stylish-haskell-plugin to 9.2(include any version less than 9.2)

@July541 July541 changed the title Support hls-hlint-plugin and hls-stylish-plugin to ghc9.2 Support hls-hlint-plugin and hls-stylish-plugin for ghc9.0 and ghc9.2 Apr 26, 2022
Copy link
Collaborator

@michaelpj michaelpj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you also update the plugin support table in the docs when this is ready? (#2840 brings it up to date with the current state, but this PR makes it better!)

hlint +ghc-lib,
stylish-haskell +ghc-lib

source-repository-package
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need this? when can we remove it?

Copy link
Collaborator Author

@July541 July541 Apr 26, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Due to the limitation of hls-hlint-plugin code structure, we must force ghc-lib-parser uses ghc-lib instead of ghc to make compile, this repo adds a flag to allow us to force use ghc-lib in stylish-haskell(or is there another way to do this with out edit the package?)


Why do we have to use ghc-lib?

  1. We have two code path in hls-hlint-plugin, includes using ghc and using ghc-lib. the ghc codebase relies on our compiled module by getParsedModuleWithComments which has some ann operation that ghc-9.2 has totally different views, we can make ghc-9.2 also use this after some revise(Not much work, but will introduce more complex compat). The ghc-lib codebase parses the source code directly and we can use this for any ghc-lib easily.
  2. We use apply-refact to apply hlint, and for ghc before 9.2, it requires apply-refact-0.9 and in ghc-9.2, it needs apply-refact-0.10. Since in ghc codebase we pass parsed module to apply-refact, it works in 0.9 but not in 0.10. apply-refact-0.10 uses an internal IORef(https://github.com/mpickering/apply-refact/blob/cb8464af74ce9c1cee26fa4862c713bb5ff96114/src/Refact/Internal.hs#L751-L755) and the only entrance is parse the source by apply-refact. (https://github.com/mpickering/apply-refact/blob/cb8464af74ce9c1cee26fa4862c713bb5ff96114/src/Refact/Internal.hs#L709). If we still pass a parsed module to apply-refact-0.10, it crashed with undefined(https://github.com/mpickering/apply-refact/blob/cb8464af74ce9c1cee26fa4862c713bb5ff96114/src/Refact/Internal.hs#L557)

I think we can remove this after the new stylish-haskell with ghc-lib flag is released.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks we can turn to ghc-lib and abandon ghc in hls-hlint-plugin, can we?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@@ -23,7 +23,7 @@ library
build-depends:
, base >=4.12 && <5
, filepath
, fourmolu ^>=0.3 || ^>=0.4 || ^>= 0.5
, fourmolu ^>=0.3 || ^>=0.4 || ^>= 0.6
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to disallow 0.5 or should we just add 0.6?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

0.5 and 0.6 have the same requirements but 0.6 has bug fix, I don't think we have a reason to allow 0.5. https://github.com/fourmolu/fourmolu/blob/master/CHANGELOG.md

plugins/hls-hlint-plugin/test/Main.hs Outdated Show resolved Hide resolved
@@ -28,6 +28,7 @@ library
, base >=4.12 && <5
, containers
, ghc
-- Require ghc-exactprint >= 1.5 on ghc9.2
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

did you mean to also actually change the bound?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oops, I think we can only add a constraint in .project or stack-x.yaml since ghc-exactprint-0.6 is required from ghc8 to ghc9.0. I'll add a comment here.

Unfuntunately, just a comment.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could have a cabal conditional based on the GHC version? Maybe it's not worth it.

@July541
Copy link
Collaborator Author

July541 commented Apr 27, 2022

I think this pr is ready to be merged. Everything is ok except the nix config, we have a working pr to upgrade it, #2853, we can add nix contents after nix scripts upgrade.

@July541 July541 marked this pull request as ready for review April 27, 2022 13:19
Copy link
Collaborator

@eddiemundo eddiemundo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Can't comment on the ghc-lib stuff because I never really understood it.

@michaelpj michaelpj added the merge me Label to trigger pull request merge label Apr 28, 2022
@mergify mergify bot merged commit c14cbdb into haskell:master Apr 28, 2022
sloorush pushed a commit to sloorush/haskell-language-server that referenced this pull request May 21, 2022
…haskell#2854)

* hls-hlint-plugin & hls-stylish-haskell-plugin rush to ghc92

* Restore allow-newer

* Add BufSpan for lower version ghc

* Add Cabal constraint for stack-9.0.1.yaml

* Try ghc-lib-parser for BufSpan

* Loose tactic compiling constraint

* Rename hlint34

* Rerun test

* Update doc

* Comment ghc-exactprint requirements

* Add test for apply-refact-0.10

* Use hackage stylish-haskell

* ghc-exactprint constraints for hls-class-plugin

* Remove uncompiled if

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
hololeap pushed a commit to hololeap/haskell-language-server that referenced this pull request Aug 26, 2022
…haskell#2854)

* hls-hlint-plugin & hls-stylish-haskell-plugin rush to ghc92

* Restore allow-newer

* Add BufSpan for lower version ghc

* Add Cabal constraint for stack-9.0.1.yaml

* Try ghc-lib-parser for BufSpan

* Loose tactic compiling constraint

* Rename hlint34

* Rerun test

* Update doc

* Comment ghc-exactprint requirements

* Add test for apply-refact-0.10

* Use hackage stylish-haskell

* ghc-exactprint constraints for hls-class-plugin

* Remove uncompiled if

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
hololeap added a commit to hololeap/haskell-language-server that referenced this pull request Aug 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge me Label to trigger pull request merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants