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

Remove auto-derivable Typeable instances #233

Merged
merged 1 commit into from
Feb 3, 2022
Merged

Remove auto-derivable Typeable instances #233

merged 1 commit into from
Feb 3, 2022

Conversation

ysangkok
Copy link
Contributor

@ysangkok ysangkok commented Feb 3, 2022

This fixes compilation on GHC 9.2. Typeable has been auto-derivable for many years now, so even the oldest GHC version in CI doesn't need these statements.

The fact that it breaks compilation on GHC 9.2 may be a bug, I will investigate this. But meanwhile, we can just remove this.

Passes the test suite on GHC 8.6 and GHC 9.2.

The test failure on MacOS seems unrelated.

Thanks to awpr on Libera.Chat.

@swamp-agr
Copy link
Collaborator

swamp-agr commented Feb 3, 2022

See #232. I fixed it in another way via making kinds explicit and explicitly enable PolyKinds as suggested in GHC 9.2 Migration Guide: https://gitlab.haskell.org/ghc/ghc/-/wikis/migration/9.2#polykinds

@swamp-agr
Copy link
Collaborator

The bug is old and very known for swagger2: https://gitlab.haskell.org/ghc/ghc/-/issues/19460. It was fixed in 8.10.7, 9.0.2 releases and GHC~HEAD. Therefore, we consider 9.0.2 as the latest stable version where all tests finally become to pass.

@ysangkok
Copy link
Contributor Author

ysangkok commented Feb 3, 2022

@swamp-agr How can that bug be relevant to the Typeable problem on master where 8.10.7 compiles but 9.2.1 doesn't? If it were that bug, those two versions should behave the same, no?

What is the value of the Typeable instances if all compiler versions on CI are able to derive them on-demand? Why not remove them?

@swamp-agr
Copy link
Collaborator

swamp-agr commented Feb 3, 2022

  • The thing is: swagger2 compiles fine on 8.10, 9.0.1, 9.0.2, even on 9.2.1 with latest patch.
  • But tests were starting to fail due to that GHC bug introduced somewhere around GHC 8.10 release.
  • To be specific they're doctests.
  • cabal or cabal-doctest performed build of doctests executable with all flags and so on.
  • If you run cabal repl with 9.0.1 or 9.2.1 GHC enabled, you'll see the same GHC panic as in tests output.
>>> instance ToSchema Person
  • In 9.2.1 PolyKinds was enabled by default in GHC2021 extension.
  • I think (I haven't check but I pretty sure) that this extension is silently enabled by default in doctest.
  • While cabal build could be executed successfully, cabal test doctests and doctests will load Data.Swagger.Internal with PolyKinds enabled by default. And it will produce different kinds and different errors, i.e. mismatch between build and tests.

Please let me know if you have more questions.

@swamp-agr
Copy link
Collaborator

src/Data/Swagger.hs:276: failure in expression `instance ToSchema Person'
expected:
 but got: ghc-9.2.1: panic! (the 'impossible' happened)
          ^
            (GHC version 9.2.1:
          \tlinkSomeBCOs: no break array

          Please report this as a GHC bug:  https://www.haskell.org/ghc/reportabug

In other words, this is expected behaviour on 9.2.1 during tests.

But this one was not expected:

telegram-cloud-photo-size-2-5469803399244527648-y

And it comes with 9.2 + doctests.

@swamp-agr
Copy link
Collaborator

swamp-agr commented Feb 3, 2022

Could you please revert NoPolyKinds? :) #232 is already merged. Both enabled NoPolyKinds and PolyKinds will contradict each other.

@ysangkok
Copy link
Contributor Author

ysangkok commented Feb 3, 2022

@swamp-agr Ok, done. Thanks for the explanation. I guess we'll just have to wait for that GHC bug to get fixed.

@swamp-agr swamp-agr merged commit 965fa21 into GetShopTV:master Feb 3, 2022
@swamp-agr
Copy link
Collaborator

Thanks for helping me. Going to release all that stuff.

@ysangkok ysangkok deleted the janus/fix-ghc92 branch February 3, 2022 23:34
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

2 participants