Skip to content

Add DivRemGeneric trait #7722

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

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

0xpantera
Copy link

Add DivRemGeneric<T, U> (heterogeneous div-mod) & keep DivRem<T> working

This PR implements the generic, asymmetric division-with-remainder trait that had
been marked TODO in core::traits.
It also supplies a zero-cost compatibility bridge so all existing code that uses the
legacy DivRem<T> continues to compile unchanged
.


Why introduce DivRemGeneric?

DivRem<T> (before) DivRemGeneric<T, U> (now)
Operand types symmetric (T / T) fully generic (T / U)
Return types always (T, T) associated: (Quotient, Remainder)
Expressiveness cannot describe u256 / u128 describes any integral mix that has an algorithm
Implementations hand-written per integer keep old impls; add only the mixed-width ones you need

Benefits over legacy DivRem<T>

  1. Mixed-width math without ad-hoc helpers
    DivRemGeneric<u256, u128> lets us express u256 / u128 directly, instead of exposing u256_safe_div_rem and friends.

  2. Accurate associated types
    Call-sites get the exact Quotient / Remainder types from the trait, enabling
    generic algorithms (e.g. wide_div_round) to compile for any (T,U) pair.

  3. No breaking changes today
    The bridge keeps DivRem<T> working, so downstream crates can migrate at their
    own pace while getting deprecation nudges.


What’s in the patch?

File Key additions
corelib/src/traits.cairo • Definition of trait DivRemGeneric<T, U>
#[deprecated] trait DivRem<T> (unchanged API)
• Internal module _divrem_bridge that automatically provides DivRemGeneric<T,T> for every pre-existing DivRem<T> impl (u8, u16, u32, u64, u128, u256).
corelib/src/integer.cairo No behaviour change – existing 29 symmetric impls left intact.
corelib/src/test/divrem_generic_test.cairo New test-suite exercising:
• legacy symmetric path (u32 / u32)
• generic asymmetric path (u256 / u128).
corelib/src/lib.cairo #[feature("generic-divrem")] gate added.

Migration / Back-compat

  • No user-visible breaking change.
    Code that calls DivRem::<T>::div_rem compiles exactly as before (now routed
    through the bridge).
  • New code can opt-in to the generic form by importing core::traits::DivRemGeneric.
  • DivRem<T> is soft-deprecated with the attribute:
#[deprecated(feature: "generic-divrem", note: "Use `DivRemGeneric`.")]

The warning only appears when a crate doesn’t enable the new feature flag,
so corelib itself builds cleanly.

@reviewable-StarkWare
Copy link

This change is Reviewable

Copy link
Collaborator

@orizi orizi left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 7 files reviewed, 1 unresolved discussion


corelib/src/traits.cairo line 367 at r1 (raw file):

}

/// Performs truncated division **and** remainder.

new traits are not added here - adding traits here is deprecated.

this should probably go to num/traits/ops and should probably additionally lose the Generic in the name.

@0xpantera
Copy link
Author

Thanks for the feedback! Will move to 'num/traits/ops'. For future reference, where could I have found that adding traits in 'corelib/src/traits' was deprecated?

Copy link
Collaborator

@orizi orizi left a comment

Choose a reason for hiding this comment

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

it was just an internal decision - you can just see all new traits were added elsewhere.

Reviewable status: 0 of 7 files reviewed, 1 unresolved discussion (waiting on @0xpantera)

@0xpantera 0xpantera requested a review from orizi May 7, 2025 14:26
Copy link
Collaborator

@orizi orizi left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 7 files at r2, all commit messages.
Reviewable status: 1 of 11 files reviewed, 3 unresolved discussions (waiting on @0xpantera)


corelib/src/traits.cairo line 367 at r2 (raw file):

}

/// Legacy trait for division with remainder.

revert the doc changes - this trait is by no means deprecated.
we must make sure the change fully work before such a thing.


corelib/src/num/traits/ops/divrem.cairo line 74 at r2 (raw file):

    pub impl BridgeU256 = Bridge<u256>;
}
use _divrem_bridge::*;
  1. starting with _ means unused - this is just private.
  2. Either impl for all - and therefore no inner module is required, or impl specifically - and then the impl aliases should be out of the module - the use * did both.

Suggestion:

mod by_divrem_legacy {
    /// Generic adapter: if the old symmetric `DivRem<T>` exists,
    /// provide the corresponding `DivRemGeneric<T,T>` implementation.
    impl Impl<T, +crate::traits::DivRem<T>> of super::DivRem<T, T> {
        type Quotient = T;
        type Remainder = T;
        fn div_rem(lhs: T, rhs: NonZero<T>) -> (T, T) {
            crate::traits::DivRem::<T>::div_rem(lhs, rhs)
        }
    }
}
// Instantiate the generic adapter for every concrete integer type
// that already has a symmetric `DivRem` implementation.
pub impl DivRemU8 = by_divrem_legacy::Impl<u8>;
pub impl BridgeU16 = by_divrem_legacy::Impl<u16>;
pub impl BridgeU32 = by_divrem_legacy::Impl<u32>;
pub impl BridgeU64 = by_divrem_legacy::Impl<u64>;
pub impl BridgeU128 = by_divrem_legacy::Impl<u128>;
pub impl BridgeU256 = by_divrem_legacy::Impl<u256>;

@0xpantera 0xpantera force-pushed the feat/divrem-associated-types branch from 0e81f17 to 960bacc Compare May 8, 2025 11:04
0xpantera added a commit to 0xpantera/cairo that referenced this pull request May 8, 2025
* New file `corelib/src/num/traits/ops/divrem.cairo`
  – defines the generic `DivRem<T,U>` trait (was in core traits.cairo).
  – adds the `by_divrem_legacy::Bridge` adapter that maps every existing
    symmetric `crate::traits::DivRem<T>` implementation to
    `DivRem<T,T>` so old code keeps compiling.
  – publishes concrete aliases (`DivRemU8 … DivRemU256`) outside the
    module, per review guidelines.

* `corelib/src/num/traits/ops.cairo`
  – re-exports `DivRem`, making it part of the public numeric-ops API.

* `corelib/src/num/traits.cairo`
  – bubbles the new trait up alongside the other `num::traits` re-exports.

* `corelib/src/traits.cairo`
  – removes the now-redundant generic-DivRem definition and restores the
    legacy `DivRem<T>` docs (no longer marked deprecated).

These edits address reviewer feedback on PR starkware-libs#7722:
  • trait must live under `num/traits/ops`
  • drop the “Generic” suffix
  • inner adapter module should stay private; concrete `pub impl`s live
    at top level.

All existing tests pass, and the new `divrem` tests in `num_test.cairo`
verify both generic and legacy paths.
@0xpantera
Copy link
Author

@orizi Appreciate the feedback. I have reverted the doc changes and fixed the legacy bridge implementation.

@0xpantera 0xpantera requested a review from orizi May 8, 2025 11:06
Copy link
Collaborator

@orizi orizi left a comment

Choose a reason for hiding this comment

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

Reviewable status: 1 of 25 files reviewed, 4 unresolved discussions (waiting on @0xpantera)


-- commits line 3 at r3:
fix your rebase - it now contains random stuff.

0xpantera added 5 commits May 8, 2025 15:29
* New file `corelib/src/num/traits/ops/divrem.cairo`
  – defines the generic `DivRem<T,U>` trait (was in core traits.cairo).
  – adds the `by_divrem_legacy::Bridge` adapter that maps every existing
    symmetric `crate::traits::DivRem<T>` implementation to
    `DivRem<T,T>` so old code keeps compiling.
  – publishes concrete aliases (`DivRemU8 … DivRemU256`) outside the
    module, per review guidelines.

* `corelib/src/num/traits/ops.cairo`
  – re-exports `DivRem`, making it part of the public numeric-ops API.

* `corelib/src/num/traits.cairo`
  – bubbles the new trait up alongside the other `num::traits` re-exports.

* `corelib/src/traits.cairo`
  – removes the now-redundant generic-DivRem definition and restores the
    legacy `DivRem<T>` docs (no longer marked deprecated).

These edits address reviewer feedback on PR starkware-libs#7722:
  • trait must live under `num/traits/ops`
  • drop the “Generic” suffix
  • inner adapter module should stay private; concrete `pub impl`s live
    at top level.

All existing tests pass, and the new `divrem` tests in `num_test.cairo`
verify both generic and legacy paths.
@0xpantera 0xpantera force-pushed the feat/divrem-associated-types branch from 960bacc to a0292ef Compare May 8, 2025 13:48
@0xpantera
Copy link
Author

Rebased onto current main, resolved the lowering conflicts by keeping
upstream changes, and rewrote the bridge as requested.
All tests green again. Thanks for your patience.

@0xpantera 0xpantera requested a review from orizi May 8, 2025 13:49
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.

3 participants