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

Prepare for GHC 9.2.* upgrade #3304

Merged
merged 24 commits into from
Aug 15, 2022
Merged

Prepare for GHC 9.2.* upgrade #3304

merged 24 commits into from
Aug 15, 2022

Conversation

ChrisPenner
Copy link
Contributor

@ChrisPenner ChrisPenner commented Aug 9, 2022

Overview

Instead of upgrading actual GHC, this is now a patch containing all the changes which will be necessary for the upgrade. It looks like HLS support will be coming soon, and we can complete the upgrade at that time by reverting this commit

Upgrades to GHC 9.2.3

We'll want to upgrade Enlil at the same time: https://github.com/unisoncomputing/enlil/pull/74

Built on #3117

Implementation notes

  • MOST changes were just adding type signatures to pattern synonyms
  • in one or two cases GHC got smarter about redundant pattern matches (which we had commented or just filled in with an error, so now I could delete them 👌🏼 )
  • A LOT of uni-patterns, which are an error under new GHC with our current flags, so I swapped most of them to explicit error's
  • GHC is now picky about canonical instance for Semigroup and Applicative, so I just moved some of those definitions around. (e.g. it wants <> to be defined instead of mappend)

Loose Ends

When we're ready to upgrade we can simply revert this commit to complete the process.


This change is Reviewable

@ChrisPenner ChrisPenner changed the title Ghc 9.2.3 cleanup Upgrade to GHC 9.2.3 Aug 9, 2022
-- Call False n as
-- | False -- known unsaturated call
-- =
-- Ins (Name (Env n 0) as) $ Yield (BArg1 0)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dolio Were these "always failing" case matches here for a reason? They're triggering 'redundant match' in GHC 9.2.3

Copy link
Contributor

Choose a reason for hiding this comment

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

They were just there to remind me to some day implement that feature. I don't think it's a great loss to just remove them, or comment them.

@ChrisPenner ChrisPenner marked this pull request as ready for review August 9, 2022 19:43
@ChrisPenner ChrisPenner self-assigned this Aug 9, 2022
@ChrisPenner
Copy link
Contributor Author

Looks like HLS isn't fully supported on 9.2.3 yet, I think we should wait until we have case-splitting at least since that's handy; I tried upgrading to 9.0.2, but that's a no-go due to a QuantifiedConstraints bug (I linked the issue in the PR description).

I'll probably just remove the actual GHC upgrade and we can merge all the busy-work, that way we're ready to upgrade whenever HLS catches up 👍🏼

@ChrisPenner ChrisPenner changed the title Upgrade to GHC 9.2.3 Prepare for GHC 9.2.* upgrade Aug 10, 2022
These are all the changes which are incompatible between 8.10.7 and GHC 9.2.3
@@ -261,14 +261,7 @@ pretty0
| otherwise ->
paren (p >= 11 || isBlock x && p >= 3) $
fmt S.DelayForceChar (l "'")
<> ( case x of
Lets' _ _ -> id
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This case is impossible because of the earlier pattern match on Lets' against x.

GHC 9.2.3 is smart enough to detect that and issues a warning here.

Copy link
Member

Choose a reason for hiding this comment

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

I think we should keep that deleted comment

@@ -997,7 +999,9 @@ wantRequest ::
Term v loc ->
Type v loc ->
(Type v loc, Wanted v loc)
wantRequest loc ~(Type.Effect'' es t) = (t, fmap (Just loc,) es)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

GHC got confused and even though this pattern is irrefutable it wouldn't believe that the patterns were exhaustive, even if I added the COMPLETE pragma for Effect'', so I just used unEffect0 instead, which is all that the Effect'' pattern does.

@@ -141,7 +141,7 @@ instance Show Stanza where
API apiRequests ->
"```api\n"
<> ( apiRequests
& fmap (\(GetRequest txt) -> Text.unpack txt)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This uni-pattern was actually a bug in the transcript parser before, 9.2.3 caught it 🎉

@@ -32,4 +32,6 @@ reference_ =

toShortHash :: ConstructorReference -> ShortHash
toShortHash (ConstructorReference r i) =
(Reference.toShortHash r) {ShortHash.cid = Just (tShow i)}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was a partial record update before.

@@ -0,0 +1,71 @@
{-# LANGUAGE ExistentialQuantification #-}
Copy link
Member

Choose a reason for hiding this comment

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

This file was deleted on trunk; accidentally added back in a merge?

@ChrisPenner ChrisPenner added ready-to-merge Apply this to a PR and it will get merged automatically once CI passes and 1 reviewer has approved and removed ready-to-merge Apply this to a PR and it will get merged automatically once CI passes and 1 reviewer has approved labels Aug 15, 2022
@ChrisPenner ChrisPenner merged commit 55781fe into trunk Aug 15, 2022
@ChrisPenner ChrisPenner deleted the ghc-9.2.3-cleanup branch August 15, 2022 18:42
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.

3 participants