Add PolyKinds as default extension to clash-prelude

[lmbollen]

PolyKinds is now a default-extension of clash-prelude. NoPolyKinds prevented the derivation of
typeclass instances of types that contained a Kind, for example BitPack (Proxy 52).

• This PR also corrects a mistake in the BitPack instance definition, which was not intended to have any constraints.
• This should also be backported to 1.8 We can't backport this to 1.8, as it is a change that makes some (shoddy) code not compile anymore.

Still TODO:

• Write a changelog entry (see changelog/README.md)
• Check copyright notices are up to date in edited files

[lmbollen]

Why did you create a PR on a PR instead of just including it into PR #2993? This feels confusing. If you first merge this into #2993 and then merge that into master, what will the history even look like?

I took the liberty of copy-editing the title.

Because I intend this to be a separate PR and now the commit history in the GUI reflects that.
The reason being I just did not want this change to be held back because of review etc on the last PR because my bittide work is blocked on it.

If you agree with the last PR. shall we just close this one and squash + merge that one?

[DigitalBrains1]

Ah, I never considered merging PR #2933 as-is, adding PolyKinds to a module for a really really brief time and then reverting it again.

My preference would be to close the other PR and include all the changes here. But if you don't want to do that, I am not against merging first #2933 and then this. However, there's one last thing to address in #2933. Again, I'm sorry I missed that one on initial review, I mentally corrected it to what I expected it to say.

[edit]

If you agree with the last PR. shall we just close this one and squash + merge that one?

I don't really understand what you're going for here, though. I don't think #2933 should be squashed, I thought the three commits were deliberate. And if you close this one, you still have to make a new PR that does what this one does? I feel like we're probably miscommunicating :-D I'm probably interpreting it completely wrong.
[/edit]

[DigitalBrains1]

I removed a superfluous change to clash-prelude/src/Clash/Class/BitPack/Internal.hs in the first commit, and added the removal of PolyKinds in clash-prelude/src/Clash/Annotations/SynthesisAttributes.hs as I think you simply overlooked that one.

[DigitalBrains1]

Corrected a typo in the first commit message (and formatted the changelog/ file to be a single line, which makes creating the CHANGELOG.md easier).

[DigitalBrains1]

Ah, it would appear that CI hasn't been passing all along. From a single glance, I think some tests define their own BitPack instances which now collide with the newly provided BitPack Proxy instance. But this was a real quick single glance.

[edit]
They were still passing with

instance BitPack (Proxy a) => BitPack (Proxy a)

but stopped working when this was changed to

instance BitPack (Proxy a)

maybe?
[/edit]

[DigitalBrains1]

[edit] They were still passing with

instance BitPack (Proxy a) => BitPack (Proxy a)

but stopped working when this was changed to

instance BitPack (Proxy a)

maybe? [/edit]

Nah, that was a red herring. I'm thinking: by enabling instance BitPack (Proxy a) to match for a being a Kind, it now also matches t 'WireRep in T1242.hs. In other words, the type Proxy 'Wirerep now has two family instances: BitSize (Proxy a) from this PR and BitSize (t 'WireRep) from T1242.hs:

clash-compiler/tests/shouldwork/Basic/T1242.hs

Line 30 in 1267e21

instance (BitPack (t 'AppRep), WireApp (t 'WireRep) (t 'AppRep)) => BitPack (t 'WireRep) where

[martijnbastiaan]

@lmbollen: AFAICT your changes to T1242.hs make it not test for the regression anymore. I discussed it with Peter the other day and we should:

• Revert the patch to 1.8 given that this might break code as described in Synthesising BitPack instances for type with phantom parameter fails #1242.
• Add a local type class definition similar to BitPack in T1242.hs so we continue testing the type family resolution behavior of Clash.
• Add a changelog outlining that code like the one in T1242 needs to change, because we added the BitPack (Proxy a) instance.

@DigitalBrains1: By now I'm pretty convinced code like T1242 is "wrong". You shouldn't make instances of the form "any type that takes X as an argument", as this removes the "agency" of specific types defining what their instance behavior should look like. I can make a more precise argument, but I hope this vague description already convinces you.

[DigitalBrains1]

I can make a more precise argument, but I hope this vague description already convinces you.

It does :-)

[DigitalBrains1]

Why the heck does the test suite succeed on older GHC's even though you use OPAQUE? Is "unknown pragma" perhaps just a warning?

Also, I doubt this is actually still testing the regression. With some mucking about, I managed to run the original and new tests on the parent of the commit that introduced the fix (5477a2f) and the new test does not error.

[DigitalBrains1]

Yep, just a warning. We might want to make this an error, it might seriously affect the integrity of the regression tests on older GHC's:

tests/shouldwork/Basic/T1242.hs:16:1: warning: [-Wunrecognised-pragmas]
    Unrecognised pragma
   |
16 | {-# OPAQUE myPack# #-}
   | ^^^

[martijnbastiaan]

Alright, what a ride. I've:

• Made a much smaller reproducer for Synthesising BitPack instances for type with phantom parameter fails #1242. This eliminates the need to have a local BitPack definition, a strategy that didn't actually test for the regression..
• Made sure that writing {-# OPAQUE f #-} actually fails on CI. Similarly, {-# CLASH_OPAQUE f #-} without {-# LANGUAGE CPP #-} should now fail too.

I think this is good, but I'll wait a few days for it to attract more comments.

[DigitalBrains1]

Here, have another aproval

Nice work untangling everything!