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

Add GHC 9.2 support for hie-compat #2003

Merged
merged 2 commits into from Jul 26, 2021
Merged

Conversation

fendor
Copy link
Collaborator

@fendor fendor commented Jul 7, 2021

Just adds the same code as for 9.0 and fixes an import statement.

@jneira
Copy link
Member

jneira commented Jul 9, 2021

umm could both versions 9.0 and 9.2 use the same src dir? or would it need too much CPP?

Comment on lines 1 to 3
{-
Forked from GHC v9.2.1 to work around the readFile side effect in mkHiefile

Copy link
Collaborator

Choose a reason for hiding this comment

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

I may have asked this question before, but do we still need this fork?

GHC 9.0 and later include mkHieFileFromSource which no longer calls readFile, so maybe we can just drop the 9.0 fork and this one.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think I looked and the api was still slightly different but will check just to be sure

Copy link
Collaborator

Choose a reason for hiding this comment

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

are you still planning to check?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yep, sorry, will do so tomorrow!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok, so both GHC 9.0 and 9.2 expose the function mkHieFileWithSource which we wrap in mkHieFile. The function mkHieFile itself from GHC 9.0 and 9.2 still perform side-effects.

However, I could not find a single consumer of the function mkHieFile (from hie-compat), so we can remove it completely?

We should be able to drop the source for GHC 9.2 and 9.0 and just rename the modules in the cabal file.
However, we should not do this, as when a module gets renamed in GHC, and we have for example this structure in the cabal file:

   if impl(ghc >= 9.0) && impl(ghc < 9.4)
      reexported-modules:
           GHC.Iface.Ext.Types                   as Compat.HieTypes
   if impl(ghc >= 9.4)
      reexported-modules:
           GHC.Iface.Hie.Types                   as Compat.HieTypes

cabal check will refute to upload such a .cabal description to Hackage as we re-export Compat.HieTypes twice, which is not allowed due to this cabal bug: haskell/cabal#4629.

To reduce maintainer burden, I think it is the best course of action to keep the source files but re-export immediately without custom logic.

  • Rename mkHieFile to mkHieFileWithSource (change API appropriately)
  • Keep source module such as Compat.HieAst but re-export functions without custom logic.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Where do we use mkHieFile?

Copy link
Collaborator Author

@fendor fendor Jul 15, 2021

Choose a reason for hiding this comment

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

Nowhere, not hls, not ghcide, not hiedb. Hackage reverse dependency lookup yielded only ghcide and hiedb as a result.

We are re-exporting in ghcide/src/Development/IDE/GHC/Compat.hs but as far as I can tell, only use the variant mkHieFile'

Copy link
Collaborator

Choose a reason for hiding this comment

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

So why don't we drop it and then it should become clear that we can hopefully drop hie-compat for GHC 9.x ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Dropped it and updated old versions to also drop it.

@fendor
Copy link
Collaborator Author

fendor commented Jul 9, 2021

@jneira it would need some cpp, which we avoided so far in hie-compat. I thought it looked weird to share the sources and the code duplication is minimal, imo.

@fendor fendor force-pushed the ghc92-hie-compat branch 2 times, most recently from 77dbb73 to f77e9e5 Compare July 26, 2021 10:09
Then we can share the module re-exports for GHC >= 9
Copy link
Collaborator

@pepeiborra pepeiborra left a comment

Choose a reason for hiding this comment

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

Thanks!!

@pepeiborra pepeiborra merged commit 5ddc93a into haskell:master Jul 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants