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

Warn GHC 9 Compatibility to LSP Client #1992

Merged
merged 18 commits into from
Jul 7, 2021
Merged

Warn GHC 9 Compatibility to LSP Client #1992

merged 18 commits into from
Jul 7, 2021

Conversation

konn
Copy link
Collaborator

@konn konn commented Jul 1, 2021

As I proposed in #1990, this PR makes ghcide to send Warning to LSP clients if GHC >= 9.0.1.
It sends the message right before the initialisation; if it's inappropriate, please fix me.

It also adds README to the notes on GHC 9 compatibility.

When invoked on GHC 9 projects, ghcide warns like this:

スクリーンショット 2021-07-01 10 47 27

expanded:

スクリーンショット 2021-07-01 10 47 31

@@ -280,3 +281,10 @@ ioe_dupHandlesNotCompatible :: Handle -> IO a
ioe_dupHandlesNotCompatible h =
ioException (IOError (Just h) IllegalOperation "hDuplicateTo"
"handles are incompatible" Nothing Nothing)

isOverGhc9 :: Bool
Copy link
Member

@jneira jneira Jul 1, 2021

Choose a reason for hiding this comment

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

We have something similar in hls-test-utils but more general, maybe it worths move the ghc version check code to Development.IDE.GHC.Compat and make it available for the entire codebase (hls-test-utils depends on ghcide):

data GhcVersion
= GHC810
| GHC88
| GHC86
| GHC84
deriving (Eq,Show)
ghcVersion :: GhcVersion
#if (defined(MIN_VERSION_GLASGOW_HASKELL) && (MIN_VERSION_GLASGOW_HASKELL(8,10,0,0)))
ghcVersion = GHC810
#elif (defined(MIN_VERSION_GLASGOW_HASKELL) && (MIN_VERSION_GLASGOW_HASKELL(8,8,0,0)))
ghcVersion = GHC88
#elif (defined(MIN_VERSION_GLASGOW_HASKELL) && (MIN_VERSION_GLASGOW_HASKELL(8,6,0,0)))
ghcVersion = GHC86
#elif (defined(MIN_VERSION_GLASGOW_HASKELL) && (MIN_VERSION_GLASGOW_HASKELL(8,4,0,0)))
ghcVersion = GHC84
#endif

@wz1000
Copy link
Collaborator

wz1000 commented Jul 1, 2021

I think this is not worth the annoyance on every start up. I would consider it acceptable if it was limited to the first start.

@jneira
Copy link
Member

jneira commented Jul 1, 2021

I think this is not worth the annoyance on every start up. I would consider it acceptable if it was limited to the first start.

Is it possible using only the lsp spec? If it is not maybe the simpler thing is add the warning in the vscode editor, when it downlods hls for ghc-9.0.1. It should be done for each editor though.

@anka-213
Copy link
Contributor

anka-213 commented Jul 1, 2021

vscode is the only one that downloads the language server automatically, so it's probably not as important for the other editors.

@jneira
Copy link
Member

jneira commented Jul 1, 2021

maybe we could convert the alert in a log output, like the warning about the implicit cradle?

@konn
Copy link
Collaborator Author

konn commented Jul 2, 2021

maybe we could convert the alert in a log output, like the warning about the implicit cradle?

Hmm, log output cannot be seen unless a user actively opens and navigate it so it makes less sense, I think...
Well, if there is not much consensus on adding this warning, I would close this.

@jneira
Copy link
Member

jneira commented Jul 2, 2021

Hmm, log output cannot be seen unless a user actively opens and navigate it so it makes less sense, I think...
Well, if there is not much consensus on adding this warning, I would close this.

Well i can imagine a situation where users not using the automatic download of vscode get an error or detect a plugin is not enabled (and they don't know they are disabled for ghc 9) and examine and/or post the logs in an issue. The message will help them or us to detect quickly the cause.

@jneira
Copy link
Member

jneira commented Jul 6, 2021

@konn do you mind if i take over the pr to convert the alert in a log message? fine with closing it too if you think it is better

@konn
Copy link
Collaborator Author

konn commented Jul 6, 2021

Sorry for leaving this PR idle (I've been a bit busy these days...).

@konn do you mind i take over the pr to convert the alert in a los message?

Ah, please take this over! And thanks for promoting GhcVer things to project-level.

@jneira jneira force-pushed the warn-ghc9-compat branch from ec29ef4 to 56bd451 Compare July 6, 2021 20:43
@jneira jneira force-pushed the warn-ghc9-compat branch from 56bd451 to ee65c27 Compare July 6, 2021 20:45
Copy link
Member

@jneira jneira left a comment

Choose a reason for hiding this comment

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

lgtm 😉, thanks for adding the warning

@jneira jneira added the merge me Label to trigger pull request merge label Jul 7, 2021
@mergify mergify bot merged commit b8bb06e into master Jul 7, 2021
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