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

Refactor CanHold(Un)Signed Typeclass Instances #23

Merged
merged 8 commits into from
Jan 17, 2024

Conversation

NathanielB123
Copy link
Contributor

@NathanielB123 NathanielB123 commented Jan 16, 2024

  • Avoided redundant transitive instances by doing type-level natural number comparisons
  • Use custom type errors to explain why the instance is not available
  • The downside is that the constraints are not extensible - i.e. users cannot create their own instances for their own numeric types. I think the only way to solve this while keeping the nice type errors would be to rely on overlapping instances (but there might be a trick that I am unaware of)

Note that GHC now (as of 9.8.1) supports a slightly more principled way of adding custom type errors via Unsatisfiable constraints. The main trouble with trying to use it here is BitWidth (which doesn't return Constraint). We could create another constraint family with an appropriate error message, or perhaps have it take a b :: Bits standing for the return value and return equality constraints (b ~ B8/b ~ B16, etc...) in the successful cases, hoping Haskell's constraint solver makes everything work out nicely.

Avoided redundant transitive instances by doing type-level natural
number comparisons
Use custom type errors to explain why the instance is not available
The downside is that the constraints are not extensible - i.e: users
cannot create their own instances for their own numeric types.
I think the only way to solve this while keeping the nice type errors
would be to rely on overlapping instances (but there might be a trick
that I am unaware of)
TypeError moved from GHC.TypeLits to GHC.TypeError
@NathanielB123
Copy link
Contributor Author

NathanielB123 commented Jan 16, 2024

I have literally no idea why the CI isn't running anymore...

Edit: Or where all the random merge commits are coming from... I hate Git

NathanielB123 and others added 4 commits January 16, 2024 19:18
I tried defining Assert manually, but it seems that the behaviour for
when type errors are reported is different for older GHC versions -
specifically `SatisfiesBound` was reporting a type error because
`BitsNat b <=? BitsNat (BitWidth t)` could not be solved.

An easy solution here is to (on older GHC versions) just use `(<=)`.
This will lead to lower-quality error messages, but will at least work.
I got signed and unsigned mixed up in the errors emitted by `IsSigned`
@j-mie6
Copy link
Owner

j-mie6 commented Jan 16, 2024

I have literally no idea why the CI isn't running anymore...

Edit: Or where all the random merge commits are coming from... I hate Git

Ah, I have to approve all runs for a non-merged contributor

Oof, so close.

On GHC < 9.2.1 term-level `Natural` could not be promoted and `Nat`
referred to a distinct kind of type-level naturals.
Since then, `Nat` is just an alias for `Natural`, so using `Nat`
consistently should work on all versions
@NathanielB123
Copy link
Contributor Author

NathanielB123 commented Jan 16, 2024

Ah, I have to approve all runs for a non-merged contributor

Ahh ok that makes sense

Turning `SatisfiesBound` from a type alias into a type family
seemingly prevents the error in the RHS being reported so eagerly
(i.e. GHC now properly waits for the arguments to be instantiated)
@j-mie6 j-mie6 self-requested a review January 17, 2024 00:11
@j-mie6
Copy link
Owner

j-mie6 commented Jan 17, 2024

I love how the PR to reduce the duplication in the instances is +78-41 😂

@j-mie6 j-mie6 merged commit 3099e8c into j-mie6:feat/token Jan 17, 2024
7 checks passed
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.

2 participants