Decouple operator supertraits from arithmetic traits (0.2.0) - #14
Conversation
…utput (0.2.0) The checked/wrapping/saturating/overflowing/strict/euclid/rounding/carrying traits routed their return type through a core::ops operator supertrait (WrappingAdd: Add<Self>, returning <Self as Add<Self>>::Output). That leaked an Add<Output=Self> requirement onto every consumer and forced backends to impl the operator just to impl the wrapping trait — even though wrapping_add is a distinct operation from +. Each such trait now carries its own fresh 'type Output' and drops the operator supertrait, so consumers bound on WrappingAdd<Output=Self> (the trait they use) rather than the operator they don't. Non-Copy backends keep the impl-for-&T owned-return path (the reason for an associated Output rather than a bare -> Self). Also, dropping pure-leak supertraits surfaced by the same audit: WideningMul's dead Mul<Self> (returns via type Wide); the Rem<Self> on MultipleOf/DivExact/ NextMultipleOf that never sourced a return; and relaxing Signed (Num -> Zero + PartialOrd + Neg + Signum) and Unsigned (bare marker) so sign queries don't drag in Div/Rem/One. Euclid gains a single type Output (div and rem share it); its Checked/Wrapping/Overflowing/Strict variants re-spell through it. CarryingMul gains a second assoc type for the high word. Consumers: replace 'T: WrappingAdd + Add<Output=T>' style bounds with 'T: WrappingAdd<Output=T>'; drop operator bounds only present for the projection. Impls are unchanged (primitive Output is Self). Breaking; targets 0.2.0.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @kaidokert, you have reached your weekly rate limit of 1500000 diff characters.
Please try again later or upgrade to continue using Sourcery
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 37 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. 📝 WalkthroughWalkthroughThis PR refactors numeric operation traits (bits, carrying, checked, euclid, overflowing, rounding, saturating, strict, wrapping) to use explicit associated ChangesAssociated Output type refactor
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request decouples various mathematical and bitwise operation traits from standard library operator traits (such as Add, Sub, Mul, Div, Rem, Shl, Shr, and Neg) by removing them as supertraits and introducing an associated Output type. This change allows for more flexible implementations, particularly for non-Copy types, and simplifies blanket implementations for Wrapping. Additionally, the Signed and Unsigned traits have had their bounds relaxed (e.g., replacing Num with Zero + PartialOrd or Sized). There are no review comments, so I have no further feedback to provide.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a20b1a3f82
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Cargo.toml (1)
11-11: 📐 Maintainability & Code Quality | 🔵 TrivialKeep the published version and README examples in sync.
README.mdstill tells consumers to depend on0.1, so shipping0.2.0without updating those examples will point users at the wrong upgrade path. Please update the dependency snippets alongside this bump.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Cargo.toml` at line 11, The published version in Cargo.toml has been bumped to 0.2.0, but the README dependency examples still reference 0.1, so update the README snippets to match the new release. Keep the version string in sync across the package metadata and the example dependency declarations so consumers are pointed to the correct upgrade path.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@Cargo.toml`:
- Line 11: The published version in Cargo.toml has been bumped to 0.2.0, but the
README dependency examples still reference 0.1, so update the README snippets to
match the new release. Keep the version string in sync across the package
metadata and the example dependency declarations so consumers are pointed to the
correct upgrade path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9a15dd14-d45d-464d-afd5-08309197fee8
📒 Files selected for processing (12)
Cargo.tomlsrc/ops/bits.rssrc/ops/carrying.rssrc/ops/checked.rssrc/ops/euclid.rssrc/ops/overflowing.rssrc/ops/rounding.rssrc/ops/saturating.rssrc/ops/strict.rssrc/ops/wrapping.rssrc/pow.rssrc/sign.rs
PrimInt inherits Saturating unpinned, so after Saturating gained a type Output, generic PrimInt code saw saturating_add return an opaque <T as Saturating>::Output instead of Self. Pin it like the CheckedAdd/Sub/Mul/Div supertraits already are. Also sync the README dependency snippets to 0.2.
1 participant
Breaking (0.2.0). Removes the
core::opsoperator supertrait that the arithmetic traits carried solely to source their return type, replacing the projection with a fresh trait-localtype Output.Why
WrappingAdd: Add<Self>returning<Self as Add<Self>>::Outputleaked anAdd<Output = Self>requirement onto every consumer, and forced a backend to implement the operator just to implement the wrapping trait — even thoughwrapping_addis a distinct operation from+(a backend whose+panics still has a well-definedwrapping_add). Each such trait now carries its owntype Outputand drops the operator supertrait.WrappingAdd<Output = Self>(the trait they use), notWrappingAdd + Add<Output = Self>(the operator they don't).Copybackends keep theimpl Trait for &Towned-return path — that is the reason for an associatedOutputrather than a bare-> Self. This was the hard requirement driving the design.OutputisSelf);PrimInt'sCheckedAdd<Output = Self>bounds and theWrapping<T>blanket impls keep resolving.Scope (from a 4-way audit of the whole trait surface)
DivCeil/DivFloor)/carrying(CarryingAdd/BorrowingSub)/bits(UnboundedShl/Shr,ShlExact/ShrExact) → freshtype Output.Euclid→ singletype Output(div and rem share it);Checked/Wrapping/Overflowing/StrictEuclidre-spell through<Self as Euclid>::Output.CarryingMul→ second assoc type for the high word (alongsidetype Unsigned).WideningMul's deadMul(returns viatype Wide); theRemonMultipleOf/DivExact/NextMultipleOfthat never sourced a return.Signed:Num→Zero + PartialOrd + Neg + Signum;Unsigned: bare marker — so sign queries don't drag inDiv/Rem/One(a CT/non-divisible signed type can now answeris_positive/abs).Migration
Replace
T: WrappingAdd + Add<Output = T>-style bounds withT: WrappingAdd<Output = T>, and drop operator bounds that were only present to pin the projection. Impl sites are unchanged.Verified: stable (lib + integration + 259 doctests), nightly
--features nightly,ct(const surface + 261 doctests), clippy clean on default +ct, and the no_std /ct/libmbuild configs.Summary by CodeRabbit
New Features
Bug Fixes
Chores