[#518] Build ghcide with GHC 8.10.1 #519

chshersh · 2020-04-02T13:42:57Z

Resolves #518

I was able to implement a small patch that allows ghcide to build with GHC 8.10. A couple of tests are failing, though. Any feedback is appreciated!

digitalasset-cla · 2020-04-02T13:43:00Z

All committers have signed the CLA.

src/Development/IDE/Core/Rules.hs

mpickering · 2020-04-02T14:01:42Z

ghcide.cabal

@@ -144,7 +144,7 @@ library
        Development.IDE.Plugin.CodeAction.RuleTypes
        Development.IDE.Plugin.Completions.Logic
        Development.IDE.Plugin.Completions.Types
-    if impl(ghc > 8.7) || flag(ghc-lib)
+    if (impl(ghc > 8.7) && impl(ghc < 8.10)) || flag(ghc-lib)


If this is correct, then why do we need the file in tree for 8.8 as well?

This file contains the implementation of the mkHieFile function. This function is present in GHC 8.10. And the implementation for GHC-8.8 is based on HIE files from GHC, where the implementation of this function for GHC 8.6 is based on ghcide own reimplementation of HIE files. So there are 3 cases for this function.

The version of mkHieFile in GHC 8.8 was unsuitable for ghcide, so we inlined the implementation locally to fix it. After that, @mpickering yourself propagated the fix upstream, which is the reason it happily does not need to be inlined when building against GHC 8.10

EDIT: oh no, it looks like the fix didn't make it into 8.10, and we still need to use the local version. I guess that complicates things a bit ...

chshersh · 2020-04-02T15:23:43Z

src/Development/IDE/GHC/HieAst.hs

  let tc_binds = tcg_binds ts
  (asts', arr) <- getCompressedAsts tc_binds rs
  let Just src_file = ml_hs_file $ ms_location ms
+  src <- liftIO $ BS.readFile src_file


It is how the mkHieFile function is implemented in GHC 8.10. This function is called only once in the whole codebase and empty string "" is passed as an argument to it instead of src. Maybe this was done for some performance/memory reasons, but I made the implementation consistent with GHC 8.10 to have fewer surprises.

This implementation is unsuitable for ghcide where files opened in the editor live in a virtual file system, and was the reason that we had to inline this module and change the implementation of mkHieFile. Please revert :)

The point of copying this file is to remove this part of the implementation if I remember correctly.

@pepeiborra Thanks for the detailed explanation. This change is reverted. The HieAst module for ghc-8.8 has no changes now.

src/Development/IDE/Core/Rules.hs

chshersh · 2020-04-04T20:46:08Z

Thanks, @pepeiborra for the patch! I guess now the build is failing due to HLint warnings in the new file.

chshersh · 2020-04-07T13:59:27Z

CI for GHC 8.10 fails due to the test failure. The following test fails, don't know how to fix it:

  plugins:                                                       FAIL (1.49s)
    test/src/Development/IDE/Test.hs:39:
    Could not find (DsError,(8,14),"Variable not in scope: c") in List [Diagnostic {_range = Range {_start = Position {_line = 0, _character = 0}, _end = Position {_line = 0, _character = 0}}, _severity = Just DsError, _code = Nothing, _source = Just "typecheck", _message = "/tmp/extra-dir-78866602889079/Testing.hs:1:1: error:\n    Bad interface file: /home/vsts/.stack/snapshots/x86_64-linux/0d09385e35771f64a4c4bf9b92b8b30fbc8ce487126f21b5edfae99a26a1d108/8.10.1/lib/x86_64-linux-ghc-8.10.1/ghc-typelits-knownnat-0.7.2-DCI9V85uoFeJnRWeFtLtpR/GHC/TypeLits/KnownNat.hi\n        Something is amiss; requested module  ghc-typelits-knownnat-0.7.2:GHC.TypeLits.KnownNat differs from name found in the interface file array-0.5.4.0:GHC.TypeLits.KnownNat (if these names look the same, try again with -dppr-debug)", _tags = Nothing, _relatedInformation = Nothing}]

pepeiborra

CI for GHC 8.10 fails due to the test failure. The following test fails, don't know how to fix it:

  plugins:                                                       FAIL (1.49s)
    test/src/Development/IDE/Test.hs:39:
    Could not find (DsError,(8,14),"Variable not in scope: c") in List [Diagnostic {_range = Range {_start = Position {_line = 0, _character = 0}, _end = Position {_line = 0, _character = 0}}, _severity = Just DsError, _code = Nothing, _source = Just "typecheck", _message = "/tmp/extra-dir-78866602889079/Testing.hs:1:1: error:\n    Bad interface file: /home/vsts/.stack/snapshots/x86_64-linux/0d09385e35771f64a4c4bf9b92b8b30fbc8ce487126f21b5edfae99a26a1d108/8.10.1/lib/x86_64-linux-ghc-8.10.1/ghc-typelits-knownnat-0.7.2-DCI9V85uoFeJnRWeFtLtpR/GHC/TypeLits/KnownNat.hi\n        Something is amiss; requested module  ghc-typelits-knownnat-0.7.2:GHC.TypeLits.KnownNat differs from name found in the interface file array-0.5.4.0:GHC.TypeLits.KnownNat (if these names look the same, try again with -dppr-debug)", _tags = Nothing, _relatedInformation = Nothing}]

I can repro this error locally with a ghc 8.10.1 build via Nix, which removes stack from the equation.
And reproducing what the test does in ghci doesn't show the error, so it's almost certainly not an issue in the related plugin and libraries.

pepeiborra · 2020-04-02T17:47:28Z

src/Development/IDE/GHC/HieAst.hs

  let tc_binds = tcg_binds ts
  (asts', arr) <- getCompressedAsts tc_binds rs
  let Just src_file = ml_hs_file $ ms_location ms
+  src <- liftIO $ BS.readFile src_file


This implementation is unsuitable for ghcide where files opened in the editor live in a virtual file system, and was the reason that we had to inline this module and change the implementation of mkHieFile. Please revert :)

src-ghc810/Development/IDE/GHC/HieAst.hs

pepeiborra · 2020-04-08T18:13:28Z

CI for GHC 8.10 fails due to the test failure. The following test fails, don't know how to fix it:

  plugins:                                                       FAIL (1.49s)
    test/src/Development/IDE/Test.hs:39:
    Could not find (DsError,(8,14),"Variable not in scope: c") in List [Diagnostic {_range = Range {_start = Position {_line = 0, _character = 0}, _end = Position {_line = 0, _character = 0}}, _severity = Just DsError, _code = Nothing, _source = Just "typecheck", _message = "/tmp/extra-dir-78866602889079/Testing.hs:1:1: error:\n    Bad interface file: /home/vsts/.stack/snapshots/x86_64-linux/0d09385e35771f64a4c4bf9b92b8b30fbc8ce487126f21b5edfae99a26a1d108/8.10.1/lib/x86_64-linux-ghc-8.10.1/ghc-typelits-knownnat-0.7.2-DCI9V85uoFeJnRWeFtLtpR/GHC/TypeLits/KnownNat.hi\n        Something is amiss; requested module  ghc-typelits-knownnat-0.7.2:GHC.TypeLits.KnownNat differs from name found in the interface file array-0.5.4.0:GHC.TypeLits.KnownNat (if these names look the same, try again with -dppr-debug)", _tags = Nothing, _relatedInformation = Nothing}]

I can repro this error locally with a ghc 8.10.1 build via Nix, which removes stack from the equation.
And reproducing what the test does in ghci doesn't show the error, so it's almost certainly not an issue in the related plugin and libraries.

Following GHC's suggestion to use -dppr-debug, I can confirm that the two names are really the same. Quite confusing:

  /home/pepe/scratch/ghcide/Testing.hs:1:1: error:
  Bad interface file:
  /nix/store/y47mcwxg3xjzm1vncv8vm3km103l1kzn-ghc-typelits-knownnat-0.7.2/lib/ghc-8.10.1/x86_64-linux-ghc-8.10.1/ghc-typelits-knownnat-0.7.2-EOol1HudK6SImK1KLBAdq1/GHC/TypeLits/KnownNat.hi
  Something is amiss; requested module
  ghc-typelits-knownnat-0.7.2-EOol1HudK6SImK1KLBAdq1:GHC.TypeLits.KnownNat differs from name found
  in the interface file ghc-typelits-knownnat-0.7.2-EOol1HudK6SImK1KLBAdq1:GHC.TypeLits.KnownNat (if
  these names look the same, try again with -dppr-debug)

cocreature

Thank you very much! The inlined hie implementation makes me a bit sad but I don’t see a better solution.

It looks like this conflicted with some other PRs merged recently so it needs rebasing. If you run into issues let me know and I’m happy to help!

Resolves #518

The fix for mkHieFile didn't make it into 8.10.1, so the override is still needed

chshersh · 2020-05-03T16:24:58Z

@cocreature Thanks for the heads up! I've rebased on the latest master and resolved all conflicts.

cocreature · 2020-05-03T17:31:02Z

/azp run

azure-pipelines · 2020-05-03T17:31:12Z

Azure Pipelines successfully started running 1 pipeline(s).

cocreature · 2020-05-03T17:33:25Z

src/Development/IDE/GHC/Orphans.hs

@@ -1,8 +1,10 @@
 -- Copyright (c) 2019 The DAML Authors. All rights reserved.
 -- SPDX-License-Identifier: Apache-2.0

+{-# LANGUAGE CPP               #-}


Hlint is failing because we explicitly whitelist modules that are allowed to use CPP. You can just add this module to the list: https://github.com/digital-asset/ghcide/blob/9adb11125ed8c671e802405a272c5c34ec1df0d3/.hlint.yaml#L77

Thanks! Should be fixed now.

cocreature · 2020-05-04T07:16:38Z

Hm, the test failure still persists. I have to admit that I have no idea what is going wrong here 🤔

pepeiborra · 2020-05-07T17:41:59Z

Only the Linux build of 8.10 fails. The windows build succeeds. Platform specific GHC bug?

cocreature · 2020-05-08T11:29:14Z

We don’t actually run tests on Windows at all atm. We just check that they compile.

In the interest in moving this forward, how about we open an issue for the failing test and just disable it for 8.10 for now? This still seems like a clear improvement over the status quo so it would be a shame to block it on this.

Resolves #518

The fix for mkHieFile didn't make it into 8.10.1, so the override is still needed

Rebase and disable broken test

stack810.yaml

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

chshersh · 2020-05-11T08:17:57Z

@pepeiborra Thanks for the fix! The build seems passing now 👍
@cocreature @mpickering Is there something left to do in this PR?

cocreature

Looks great, thanks!

* [haskell/ghcide#518] Build ghcide with GHC 8.10.1 Resolves haskell/ghcide#518 * Move CPP logic to the Compat module * Revert changes to mkHieFile * Add local fork of HieAst for 8.10.1 The fix for mkHieFile didn't make it into 8.10.1, so the override is still needed * Ignore hlint in src-ghc810/HieAst.hs * Whitelist CPP for Development.IDE.GHC.Orphans * [haskell/ghcide#518] Build ghcide with GHC 8.10.1 Resolves haskell/ghcide#518 * Move CPP logic to the Compat module * Revert changes to mkHieFile * Add local fork of HieAst for 8.10.1 The fix for mkHieFile didn't make it into 8.10.1, so the override is still needed * Ignore hlint in src-ghc810/HieAst.hs * Whitelist CPP for Development.IDE.GHC.Orphans * Plugin tests known broken in 8.10.1 (haskell/ghcide#556) * Bump up ghc-check version Co-authored-by: Pepe Iborra <pepeiborra@gmail.com> Co-authored-by: pepe iborra <pepeiborra@gmail.com>

mpickering reviewed Apr 2, 2020

View reviewed changes

src/Development/IDE/Core/Rules.hs Outdated Show resolved Hide resolved

mpickering reviewed Apr 2, 2020

View reviewed changes

chshersh commented Apr 2, 2020

View reviewed changes

mpickering reviewed Apr 3, 2020

View reviewed changes

src/Development/IDE/Core/Rules.hs Outdated Show resolved Hide resolved

mpickering reviewed Apr 3, 2020

View reviewed changes

src/Development/IDE/Core/Rules.hs Outdated Show resolved Hide resolved

jneira mentioned this pull request Apr 6, 2020

Support for ghc-8.10.1 haskell/haskell-ide-engine#1712

Open

pepeiborra reviewed Apr 7, 2020

View reviewed changes

cocreature approved these changes May 3, 2020

View reviewed changes

chshersh and others added 5 commits May 3, 2020 17:16

[#518] Build ghcide with GHC 8.10.1

195e29b

Resolves #518

Move CPP logic to the Compat module

d9b790b

Revert changes to mkHieFile

ff18d93

Add local fork of HieAst for 8.10.1

c6969b0

The fix for mkHieFile didn't make it into 8.10.1, so the override is still needed

Ignore hlint in src-ghc810/HieAst.hs

01e49b4

cocreature reviewed May 3, 2020

View reviewed changes

Whitelist CPP for Development.IDE.GHC.Orphans

bbe7650

chshersh and others added 5 commits May 9, 2020 12:34

[#518] Build ghcide with GHC 8.10.1

66f494f

Resolves #518

Move CPP logic to the Compat module

9511441

Revert changes to mkHieFile

ac88a9c

Add local fork of HieAst for 8.10.1

b89d6b3

The fix for mkHieFile didn't make it into 8.10.1, so the override is still needed

Ignore hlint in src-ghc810/HieAst.hs

915ded0

chshersh and others added 3 commits May 9, 2020 12:34

Whitelist CPP for Development.IDE.GHC.Orphans

636dcea

Plugin tests known broken in 8.10.1 (#556)

1679c72

Merge pull request #2 from pepeiborra/chshersh-ghc-8.10.1-pr

8821e18

Rebase and disable broken test

pepeiborra reviewed May 10, 2020

View reviewed changes

stack810.yaml Outdated Show resolved Hide resolved

Bump up ghc-check version

4c71e83

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

cocreature approved these changes May 11, 2020

View reviewed changes

cocreature merged commit 07cf929 into haskell:master May 11, 2020

pepeiborra mentioned this pull request Jan 1, 2021

Plugins broken with GHC 8.10.1 haskell/haskell-language-server#917

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[#518] Build ghcide with GHC 8.10.1 #519

[#518] Build ghcide with GHC 8.10.1 #519

chshersh commented Apr 2, 2020

digitalasset-cla commented Apr 2, 2020 •

edited

Loading

mpickering Apr 2, 2020

chshersh Apr 2, 2020

pepeiborra Apr 2, 2020 •

edited

Loading

chshersh Apr 2, 2020

pepeiborra Apr 2, 2020

mpickering Apr 3, 2020

chshersh Apr 8, 2020

chshersh commented Apr 4, 2020

chshersh commented Apr 7, 2020

pepeiborra left a comment

pepeiborra Apr 2, 2020

pepeiborra commented Apr 8, 2020

cocreature left a comment

chshersh commented May 3, 2020

cocreature commented May 3, 2020

azure-pipelines bot commented May 3, 2020

cocreature May 3, 2020

chshersh May 3, 2020

cocreature commented May 4, 2020

pepeiborra commented May 7, 2020

cocreature commented May 8, 2020

chshersh commented May 11, 2020

cocreature left a comment

[#518] Build ghcide with GHC 8.10.1 #519

[#518] Build ghcide with GHC 8.10.1 #519

Conversation

chshersh commented Apr 2, 2020

digitalasset-cla commented Apr 2, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pepeiborra Apr 2, 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

chshersh commented Apr 4, 2020

chshersh commented Apr 7, 2020

pepeiborra left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pepeiborra commented Apr 8, 2020

cocreature left a comment

Choose a reason for hiding this comment

chshersh commented May 3, 2020

cocreature commented May 3, 2020

azure-pipelines bot commented May 3, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cocreature commented May 4, 2020

pepeiborra commented May 7, 2020

cocreature commented May 8, 2020

chshersh commented May 11, 2020

cocreature left a comment

Choose a reason for hiding this comment

digitalasset-cla commented Apr 2, 2020 •

edited

Loading

pepeiborra Apr 2, 2020 •

edited

Loading