Update to GHC 9.2.2 by hdgarrood · Pull Request #4326 · purescript/purescript

hdgarrood · 2022-05-15T21:44:04Z

This PR updates us to GHC 9.2.2. Most of the updates are fairly straightforward; I will explain why I've made them as inline review comments.

The main reasons I think we might want to do this are:

to benefit from numerous performance enhancements in GHC in the 9.2 series (and the upcoming 9.4 series as well); see the release notes. I think these performance enhancements will mean that GHC can compile the PureScript compiler more quickly, but I don't think we would expect to see a speedup in the performance of the PureScript compiler itself.
to be able to provide ARM binaries for systems like M1 macs: as of version 9.2.1, GHC has a native ARM code generator.

Reasons not to merge this yet:

We depend on protolude-0.3.1 which is tagged but not yet uploaded to Hackage so for now I'm using a git reference in stack.yaml; this means that people would probably struggle to compile the PureScript compiler with cabal if we made a release with these changes. We should probably either drop the protolude dependency or wait for 0.3.1 to be uploaded to Hackage.
~~Weeder does not yet compile on GHC 9.2~~ Weeder 2.4.0, which targets GHC 9.2, has just been released

I should probably figure out what the deal with this warning is. I'm fairly sure it's a false positive as we aren't actually using the GHC API anywhere (AFAIK???)

Warning:
    This package indirectly depends on multiple versions of the same package. This is very likely to cause a compile failure.
      package typed-process (typed-process-0.2.8.0-qJ7FQLjAUsDoh5UIGjmGT) requires process-1.6.13.1-1ayluMYLS1yTV7wv3m4fy
      package streaming-commons (streaming-commons-0.2.2.4-8nDHmhtmpx83qtrdduE76B) requires process-1.6.13.1-1ayluMYLS1yTV7wv3m4fy
      package sourcemap (sourcemap-0.1.7-9a6OzM8XbZDSeQz7MTa1o) requires process-1.6.13.1-1ayluMYLS1yTV7wv3m4fy
      package purescript (purescript-0.15.1) requires process-1.6.13.1-1ayluMYLS1yTV7wv3m4fy
      package purescript (purescript-0.15.1) requires process-1.6.13.1-1ayluMYLS1yTV7wv3m4fy
      package purescript (purescript-0.15.1) requires process-1.6.13.1-1ayluMYLS1yTV7wv3m4fy
      package optparse-applicative (optparse-applicative-0.17.0.0-G8nAkP3cjJUHOGraopmgpx) requires process-1.6.13.1-1ayluMYLS1yTV7wv3m4fy
      package haskeline (haskeline-0.8.2-27iD1wLo6nE3EGXBbzNrZN) requires process-1.6.13.1-1ayluMYLS1yTV7wv3m4fy
      package gitrev (gitrev-1.3.1-5EZyLHF9iyU5HR4ImcQtFa) requires process-1.6.13.1-1ayluMYLS1yTV7wv3m4fy
      package conduit-extra (conduit-extra-1.3.6-IWwDkCT2NsHEPOAoybwv54) requires process-1.6.13.1-1ayluMYLS1yTV7wv3m4fy
      package Cabal (Cabal-3.6.3.0-FskfNehIkKSJXUcrlACH2g) requires process-1.6.13.1-1ayluMYLS1yTV7wv3m4fy
      package ghc (ghc-9.2.2) requires process-1.6.13.2

hdgarrood · 2022-05-15T22:00:55Z

.github/workflows/ci.yml

          - # If upgrading the Haskell image, also upgrade it in the lint job below
            os: "ubuntu-latest"
-            image: "haskell:8.10.7-stretch@sha256:100f8fb7d7d8d64adb5e106fe8136b8d4cbdc03aeb2cbd145a7597d74b69bafb"
+            image: "haskell:9.2.2-buster@sha256:c2f489a95be8dd0b253da6980420a6e9c02e81c2d14838ca774f86f5b5db91f4"


buster (Debian 10) is the next Debian release after stretch. There is no stretch image which includes GHC 9.2.2, probably because stretch will stop receiving security updates next month (June 2022).

Looks like this would mean bumping our glibc dependency from 2.24 to 2.28.

Should we make a Discourse post asking if people would object to that? Seems like we consistently stir up trouble whenever this changes.

Yes, good point. This certainly isn’t ideal. Maybe we should just take a basic Debian stretch image and install the Haskell tools ourselves? My inclination is to avoid bumping this if we can manage it.

hdgarrood · 2022-05-15T22:01:28Z

app/Version.hs

-    | $(GitRev.gitDirty) = " DIRTY"
-    | otherwise = ""
+  dirty =
+    if $(GitRev.gitDirty)


Before this change, GHC would complain that the otherwise case was redundant if GitRev.gitDirty evaluated to True.

hdgarrood · 2022-05-15T22:03:11Z

src/Language/PureScript/AST/Declarations.hs

  deriving (Show, Eq, Ord, Foldable, Functor, Traversable)

 $(deriveJSON (defaultOptions { sumEncoding = ObjectWithSingleField }) ''NameSource)
+$(deriveJSON (defaultOptions { sumEncoding = ObjectWithSingleField }) ''ExportSource)


This (and a few other JSON instance rearrangements) were necessary because any instances defined below a TH deriveJSON call like this one won't be visible to that call, so if a data type A references another data type B in its definition, then the deriveJSON for B has to come before the one for A.

hdgarrood · 2022-05-15T22:04:21Z

src/Language/PureScript/CST/Utils.hs

 properName n = ProperName (N.coerceProperName <$> n)

+getProperName :: forall a. ProperName -> Name (N.ProperName a)
+getProperName pn = _getProperName pn -- eta expansion needed here due to simplified subsumption


Without this change, you get an error like:

src\Language\PureScript\CST\Utils.hs:47:17: error: * Couldn't match type: forall (a1 :: N.ProperNameType). Name (N.ProperName a1) with: Name (N.ProperName a) Expected: ProperName -> Name (N.ProperName a) Actual: ProperName -> forall (a :: N.ProperNameType). Name (N.ProperName a) * In the expression: _getProperName In an equation for `getProperName': getProperName = _getProperName * Relevant bindings include getProperName :: ProperName -> Name (N.ProperName a) (bound at src\Language\PureScript\CST\Utils.hs:47:1) | 47 | getProperName = _getProperName | ^^^^^^^^^^^^^^

hdgarrood · 2022-05-15T22:05:40Z

src/Language/PureScript/CoreFn/ToJSON.hs

 constructorTypeToJSON SumType = toJSON "SumType"

+infixr 8 .=
+(.=) :: ToJSON a => String -> a -> Pair


Almost everything in this module is due to aeson-2 changes, specifically the left hand side of .= must now be a Key rather than a Text. I thought it might be easier to define our own version of .= so that we don't have to convert the key to a different type each time we call it.

hdgarrood · 2022-05-15T22:07:13Z

src/Language/PureScript/CoreImp/Optimizer/Unused.hs

-  go jss | not (any isReturn jss) = jss
-         | otherwise = let (body, ret : _) = break isReturn jss in body ++ [ret]
+  go jss =
+    case break isReturn jss of


This change was necessitated by the fact that -Wincomplete-uni-patterns was added to -Wall at some point between 8.10.7 and 9.2.2; the let (body, ret : _) = ... is an incomplete pattern because it does not account for what happens if the second element of the tuple is an empty list. Thankfully we can avoid it (as well as an unnecessary traversal of the list) by rewriting the function to use break instead.

hdgarrood · 2022-05-15T22:08:12Z

src/Language/PureScript/Ide/Imports.hs

-      let (x, a : y) = splitAt i l
-      in x ++ [t a] ++ y
+updateAtFirstOrPrepend predicate update def xs =
+  case break predicate xs of


Another case where using break solves both a -Wincomplete-uni-patterns warning and also removes an unnecessary traversal of the list at the same time. I thought I'd take the opportunity to use clearer argument names here too.

hdgarrood · 2022-05-15T22:09:08Z

src/Language/PureScript/Sugar/Names/Env.hs

-      let ImportRecord (Qualified (Just mnNew) _) mnOrig _ _ = head byOrig
-      in return (mnNew, mnOrig)
+      case head byOrig of
+        ImportRecord (Qualified (Just mnNew) _) mnOrig _ _ ->


Another -Wincomplete-uni-patterns warning

hdgarrood · 2022-05-15T22:09:37Z

src/Language/PureScript/TypeChecker.hs

+-- | Check that a newtype has just one data constructor with just one field, or
+-- throw an error. If the newtype is valid, this function returns the single
+-- data constructor declaration and the single field, as a 'proof' that the
+-- newtype was indeed a valid newtype.


Returning the decl and the field here means that we avoid an unsafe pattern match in Sugar/TypeClasses/Deriving.

src/Language/PureScript/TypeChecker/Entailment.hs

- Allow -Wincomplete-uni-patterns warnings in tests - Compare the graph fixture as a JSON Value so that the ordering doesn't matter - define and use `tails1` in TypeChecker.Entailment to make the validity of the non-emptiness assumption more obvious - Bump to stack 2.7.5 in CI (to match the 9.2.2 Docker image)

hdgarrood · 2022-05-16T00:08:13Z

src/Language/PureScript/TypeChecker/Entailment.hs

                           dicts
                  -- process instances in a chain in index order
-                  let found = for (init $ tails chain) $ \(tcd:tl) ->
+                  let found = for (tails1 chain) $ \(tcd :| tl) ->


Another -Wincomplete-uni-patterns warning. This was safe before, but now the safety is tracked in the types.

hdgarrood · 2022-05-16T00:09:53Z

tests/TestGraph.hs

      Right res ->
-        let textRes = Text.decodeUtf8 $ ByteString.toStrict $ Json.encode res
-        in graphFixture `shouldBe` textRes
+        res `shouldBe` graphFixture


This change means that we don't have to update the fixture if the order of fields changes, and we don't get errors if the fields end up in different orders due to different build configurations. Updating aeson seems to have caused the fields in the result to end up in a different order.

Also, you're supposed to put the expected value second. The error message describes the 'expected' vs 'actual' value, so the error can be misleading if you put the expected value first; this is why I've flipped them around.

hdgarrood · 2022-05-16T13:51:22Z

Seems like CI time for this branch is hovering around 14-15 mins, which might be slightly faster than it is currently (usually 15-18 mins from the looks of things), but the variance seems quite high so it's hard to tell.

hdgarrood · 2022-05-22T13:19:15Z

I've created #4336, which covers a part of this (the new warnings) that can be merged now. As for the rest, I am inclined to wait until the upstream dependencies (weeder and protolude) are ready, so I think I'll close this for now.

JordanMartinez · 2022-05-26T13:20:02Z

Looks like Weeder got updated to 9.2: https://github.com/ocharles/weeder/releases/tag/2.4.0

hdgarrood · 2022-06-05T14:54:06Z

So now that protolude has been updated I think the only remaining task is to figure out why cabal thinks PureScript depends on the GHC API. If you try building PureScript with cabal instead of stack, it tries to compile the GHC API and fails. I think I might write something up and ask on the Cabal issue tracker or something, because this is quite strange to me.

arrowd · 2022-06-07T08:49:50Z

If you try building PureScript with cabal instead of stack, it tries to compile the GHC API and fails

What package, exactly?

hdgarrood · 2022-06-07T08:57:22Z

ghc

arrowd · 2022-06-07T09:14:11Z

Can't reproduce this on my side. I just successfully compiled the code from this PR.

hdgarrood · 2022-06-07T12:30:31Z

Interesting 🤔

hdgarrood added 4 commits May 15, 2022 20:56

Update to GHC 9.2.2

0dd6c89

Empty commit for CI

ad2ac8c

Disable hlint eta reduce

40ea88f

Update Ubuntu CI image to haskell-9.2.2 buster

ddea220

hdgarrood commented May 15, 2022

View reviewed changes

hdgarrood added 3 commits May 16, 2022 01:02

Merge branch 'master' into update-ghc-9

f364696

Reduce diff size

31fd3df

hdgarrood commented May 16, 2022

View reviewed changes

hdgarrood added 4 commits May 16, 2022 01:16

Bump lens version

0562d8c

Empty commit to test CI speed with warm cache

16f2fc5

Another empty commit to test CI with warm cache

df3b063

Empty commit to test CI speed

99edb5c

hdgarrood closed this May 22, 2022

arrowd mentioned this pull request May 22, 2022

lang/purescript: Fails to build with GHC 9.2.2 freebsd/freebsd-ports-haskell#66

Closed

JordanMartinez mentioned this pull request Jun 4, 2022

Update codebase to GHC to 9.x series #4350

Closed

JordanMartinez mentioned this pull request Jun 8, 2022

Update codebase to GHC 9.2.3 #4351

Merged

5 tasks

hdgarrood deleted the update-ghc-9 branch September 23, 2022 14:48

Conversation

hdgarrood commented May 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hdgarrood commented May 16, 2022

Uh oh!

hdgarrood commented May 22, 2022

Uh oh!

JordanMartinez commented May 26, 2022

Uh oh!

hdgarrood commented Jun 5, 2022

Uh oh!

arrowd commented Jun 7, 2022

Uh oh!

hdgarrood commented Jun 7, 2022

Uh oh!

arrowd commented Jun 7, 2022

Uh oh!

hdgarrood commented Jun 7, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

hdgarrood commented May 15, 2022 •

edited

Loading