Skip to content

Adds missing Data.Num.Linear.* instances for Word, Integer, Natural, Float, Word8/16/32/64 Int8/16/32/64#467

Merged
aspiwack merged 4 commits intotweag:masterfrom
Qqwy:more_num_instances
Nov 6, 2023
Merged

Adds missing Data.Num.Linear.* instances for Word, Integer, Natural, Float, Word8/16/32/64 Int8/16/32/64#467
aspiwack merged 4 commits intotweag:masterfrom
Qqwy:more_num_instances

Conversation

@Qqwy
Copy link
Copy Markdown
Contributor

@Qqwy Qqwy commented Oct 20, 2023

This PR fixes two (related) issues (#403 and #466):

  • Movable (and derived Consumable/Dupable) instances are added for Integer and Natural. This is safe since these types are represented as strict sum types whose elements are unlifed unboxed primitives so there is no way laziness might break linearity in there. (See also the last part of the discussion in #403)
  • Instances for the typeclasses in Data.Num.linear (Additive/AddIdentity/AdditiveGroup/Multiplicative/MultIdentity/Semiring/Ring/Num) for the general-purpose number types that exist in the Prelude/base which were still missing them:
    • Word
    • Float
    • Integer
    • Natural
    • Word8/Word16/Word32/Word64
    • Int8/Int16/Int32/Int64

Still missing are:

  • Instances for Ratio a and Complex a.
  • Instances for Foreign.C.Types like CInt/CFloat etc.
  • Instances for Foreign.Ptr types like IntPtr, WordPtr.

Those could be added just as easily but because I was not 100% sure whether adding linear instances for them would be (morally) correct or have unintended consequences that might break linearity in ineffable ways, I left them alone for now.

Qqwy added 3 commits October 20, 2023 16:53
- Integer
- Natural*
- Word
- Float
- Int8/Int16/Int32/Int64
- Word8/Word16/Word32/Word64

(The Additive, Multiplicative, AddIdentity, MultIdentity, AdditiveGroup,
Semiring, Ring, FromInteger and Num typeclasses)

*: Natural does not have a Ring (nor Num) instance because it cannot
satisfy the 'additive inverse' law.

Fixes tweag#466
@Qqwy
Copy link
Copy Markdown
Contributor Author

Qqwy commented Oct 20, 2023

The Movable instances introduces in this PR pattern matches down to the internals of Integer and Natural just like was done for Word/Int etc. before. Doing so is only possible by pulling in the ghc-bignum package. As far as I can tell base itself depends on it so I believe adding this 'extra' dependency does not bloat the library in any way.

@Qqwy Qqwy force-pushed the more_num_instances branch from 6f55879 to 650930a Compare October 20, 2023 15:43
instance (AddIdentity a) => Monoid (Sum a) where
mempty = Sum zero

{- ORMOLU_DISABLE -}
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I took the liberty of re-ordering these by instance rather than by class because I believe it's easier on the eyes. Ormolu is disabled in this part of the code because it would add a newline between each deriving line. I hope you agree. If not, we can always change it back.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Disabling Ormolu is a little unorthodox, but I must admit that in this case, it looks like it's worth it.

Copy link
Copy Markdown
Member

@aspiwack aspiwack left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This all looks good. Thank you very much. I noticed that the CI hadn't ran yet. So I triggered it, and I'll (auto)merge when green.

instance (AddIdentity a) => Monoid (Sum a) where
mempty = Sum zero

{- ORMOLU_DISABLE -}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Disabling Ormolu is a little unorthodox, but I must admit that in this case, it looks like it's worth it.

@aspiwack
Copy link
Copy Markdown
Member

aspiwack commented Nov 6, 2023

Actually, can I, instead, ask you to add the newline to the appropriate commit to avoid a silly commit? Sorry about the rather trivial request.

@Qqwy Qqwy force-pushed the more_num_instances branch from 650930a to c0438dd Compare November 6, 2023 13:19
@Qqwy
Copy link
Copy Markdown
Contributor Author

Qqwy commented Nov 6, 2023

Ormolu/git should be happy now; of course the CI only runs after being approved once more.

@Qqwy Qqwy requested a review from aspiwack November 6, 2023 17:25
@aspiwack
Copy link
Copy Markdown
Member

aspiwack commented Nov 6, 2023

I'm guessing Github will keep requiring an approval for CI until you have at least one commit on master. I relaunched it.

@aspiwack aspiwack enabled auto-merge November 6, 2023 18:04
@aspiwack aspiwack merged commit 3e80229 into tweag:master Nov 6, 2023
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