RustCrypto / crypto-bigint

Cryptography-oriented big integer library with constant-time, stack-allocated (no_std-friendly) implementations of modern formulas
Apache License 2.0
167 stars 45 forks source link

Add a const_new method to NonZero #602

Open dvdplm opened 4 weeks ago

dvdplm commented 4 weeks ago

Makes it so users can have NonZero constants.

dvdplm commented 4 weeks ago

There used to be a const_new for this purpose but it relied on the custom CtChoice machinery that is gone now (for good reason!). Maybe this can be a stop-gap solution until we get to a better place with const support?

tarcieri commented 4 weeks ago

The constructor is also what's checking the T: Zero bound. This approach with absolutely no checks allows you to make NonZero<String> and all sorts of other nonsense type combinations.

Perhaps instead until const impl is stable it could be defined directly on concrete types like e.g. NonZero<Uint<LIMBS>> which would also enable checking for zero and panicking, rather than allowing for construction of a numerically invalid value.

dvdplm commented 4 weeks ago

The constructor is also what's checking the T: Zero bound. This approach with absolutely no checks allows you to make NonZero and all sorts of other nonsense type combinations.

I can add the Zero bound of course, but that's a bit of a "false safety" no?

dvdplm commented 4 weeks ago

Perhaps instead until const impl is stable it could be defined directly on concrete types like e.g. NonZero<Uint<LIMBS>> which would also enable checking for zero and panicking, rather than allowing for construction of a numerically invalid value.

You mean bring back a variant of what 0.5.5 had?

impl<const LIMBS: usize> NonZero<Uint<LIMBS>> {
    /// Creates a new non-zero integer in a const context.
    /// The second return value is `FALSE` if `n` is zero, `TRUE` otherwise.
    pub const fn const_new(n: Uint<LIMBS>) -> (Self, CtChoice) {
        (Self(n), n.ct_is_nonzero())
    }
}

…but without CtChoice?

tarcieri commented 4 weeks ago

Yes, I forget offhand why we got rid of that, but for defining constants a const constructor that panics is probably preferable until e.g. .unwrap()/.expect() can work in const contexts

dvdplm commented 4 weeks ago

I found a workaround for my concrete issue using to_nz() and expect (both const):

trait Beef {
    const MOO: NonZero<Uint<16>>
}

struct MyBeef;
impl Beef for MyBeef {
    const MOO: U512::from_be_hex("FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFEBAAEDCE6AF48A03BBFD25E8CD0364141FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFEBAAEDCE6AF48A03BBFD25E8CD0364141")
        .to_nz()
        .expect("Valid by construction");
}

I had a stab at making a panicking const_new, but ran into issues. I'll have another go later.

dvdplm commented 4 weeks ago

@tarcieri PTAL at https://github.com/RustCrypto/crypto-bigint/pull/602/commits/cf6486715602000b50de25e3a99e696dc7496194 – is that what you had in mind here? :)

tarcieri commented 4 weeks ago

Sure, that works. You could also implement it with .to_nz().expect as per above.