Remove auto-derivable Typeable instances

GetShopTV / swagger2

Swagger 2.0 data model.

http://hackage.haskell.org/package/swagger2

BSD 3-Clause "New" or "Revised" License

74 stars 59 forks source link

Remove auto-derivable Typeable instances #233

Closed ysangkok closed 2 years ago

ysangkok commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

@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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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

ysangkok commented 2 years ago

@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 commented 2 years ago

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