-
-
Notifications
You must be signed in to change notification settings - Fork 367
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
Update refactor/splice/hlint/fourmolu/retrie/gadt plugin for GHC 9.4 #3317
Conversation
3123648
to
b2d735b
Compare
Let me know if you need any help with ls-refactor-plugin! |
4ade99c
to
39188b4
Compare
2a2df37
to
3459f48
Compare
flake.nix
Outdated
# The GHC 9.4 pull request builds upon these unpublished changes, | ||
# which is why we include them. | ||
# | ||
# Also, there is no PR for these changes. The fourmolu maintainers |
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.
this isn't true, for example the first commit is this pr: fourmolu/fourmolu#229
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.
OK, but this is just for testing. We'll have to release a new Fourmolu before this PR can build outside of Nix.
-- the error messages are significantly different from the ones without that, and | ||
-- thus the code action triggers (such as in hls-refactor-plugin) are not fired properly. | ||
-- So we disable the error demotion. | ||
demoteIfDefer = id |
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 actually couldn't figure out yet what would be the best action here. What I observed is that due to the error message difference between the case with and without defer, several hls-refactor-plugin suggestions are not fired (which uses matchRegex on error messages). With defer turned on, on GHC 9.4, there is no way to obtain source location (which relied on a fragile mechanism of parsing error message), so if we do not want to have this brute-force disabling, we need to make those suggestions and tests disabled.
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.
cc: @santiweight
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.
Disabling deferred type errors will have an impact on many other IDE features, including diagnostics and code navigation. For this reason, I think the only alternatives here are:
- update the regexes to account for the changes in 9.4 (incrementally, in other PRs if necessary)
- migrate the suggestion providers to use typed diagnostics
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.
Thanks for the feedback!
I will revert this disabling and mark the failed tests due to this with comments. The fix for the failed tests will not be small as previously the necessary error source location info was obtained from the error message which is gone with the deferred type error flag, so let us address that in a separate PR. After all, typed diagnostics is the best direction, so we had better move on in that direction.
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.
reverted the change and mark the affected test as broken.
a20ee04
to
1b027bf
Compare
retrie 1.2.1 is now available in Hackage |
- Build against GHC 9.4 API - Increase upper bound on `hlint`
Sorry I don't know how the GitHub reviewers mechanic really works I guess |
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.
Awesome, thank you very much!
I skimmed the changes for sanity but not correctness, so take this approval with a grain of salt 🙈
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.
LGTM again, as the CI has passed. Thanks!
I've added 9.4 support for the retrie and gadt plugins to this PR. |
3787c9e
to
410d0d0
Compare
WIP tracking work on GHC 9.4 support for HLS plugins, particularly interested in
hls-refactor-plugin
.We will need
fourmolu
to cut a new release before CI can start building this PR successfully. fourmolu/fourmolu#242 (comment)