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

Add traits for BITS #271

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

JonathanWoollett-Light
Copy link

Adds a trait for getting the BITS constants e.g. u32::BITS.

@cuviper
Copy link
Member

cuviper commented May 24, 2023

What is your use case? Preferably not a hypothetical, or "because we can," but what is a real example where you would like to use generic T: Bits?

See also #246 and #247, but those did not provide justification either...

This is also different than BigUint::bits(&self), for example, which returns the number of significant bits as u64. If we don't expect such types to implement this trait, we should make that clear.

Also, {integer}::BITS have only been stable since Rust 1.53, and this crate maintains an older MSRV, so it would need some conditional handling here. A fallback of 8 * size_of is probably fine.

@JonathanWoollett-Light
Copy link
Author

JonathanWoollett-Light commented May 24, 2023

Serializing to and deserializing from bitfields.

We didn't use this crate in this PR firecracker-microvm/firecracker#3722 because we needed to get the BITS field and this wasn't supported. We instead defined our own trait for it firecracker-microvm/firecracker@0b215a5#diff-67fb01ab595aeec3769d00a0a18cc6c71eade58ff873f137fcbaa3802e26d137R114 which is awkward.

@tarcieri
Copy link
Contributor

tarcieri commented May 25, 2023

When using big integers as backing storage for cryptographic keys, there are many times it can be helpful to know the bit size of the key.

Here are some real-world code examples where we use our own trait for this in @RustCrypto projects.

@cuviper
Copy link
Member

cuviper commented May 25, 2023

Is there a reason you made this a function? This PR proposes fn bits() -> u32, but:

We instead defined our own trait for it

... with const BITS: u32, which also looks like what RustCrypto is using.

@JonathanWoollett-Light
Copy link
Author

JonathanWoollett-Light commented May 25, 2023

Is there a reason you made this a function? This PR proposes fn bits() -> u32, but:

We instead defined our own trait for it

... with const BITS: u32, which also looks like what RustCrypto is using.

Bounded uses functions (min_value and max_value) to define the max and min values, I used a function to be consistent with this.

I do not have a strong opinion about using a constant or function.

@tarcieri
Copy link
Contributor

A function might be a bit more helpful, since the size can be determined at runtime: https://github.com/search?q=repo%3ARustCrypto%2FRSA+bits%28%29&type=code

FWIW we’re looking at adding a runtime-selected fixed-width integer type to crypto-bigint for those purposes: RustCrypto/crypto-bigint#221

@cuviper
Copy link
Member

cuviper commented May 25, 2023

Bounded uses functions (min_value and max_value) to define the max and min values,

That trait predates both const fn and associated const features. Plus those need to return Self, which might not have a const constructor at all for some types.

A function might be a bit more helpful, since the size can be determined at runtime

Then that would need to be fn bits(&self) -> u32 (or larger?), and this is again similar to BigUint::bits mentioned earlier, although that case does not represent a fixed capacity. I suppose we could split ConstBits and runtime Bits.

@tarcieri
Copy link
Contributor

@cuviper yeah, the use cases are similar to BigUint::bits, although it would be nice to be able to compute the value in a data-independent manner.

In the case of e.g. RSA the standard works around the significant bits vs bit capacity issue by ensuring the two are equal at key generation time, so either definition would work there.

use core::{u128, u16, u32, u64, u8, usize};

/// Numbers which have a specified number of bits.
pub trait Bits {
Copy link
Contributor

Choose a reason for hiding this comment

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

What if you called this trait Precision instead? Then it could also have bytes()

@tarcieri
Copy link
Contributor

I assume many of the traits in this crate predate associated constants, but is there any thought to having both trait methods and associated constants? Perhaps in separate traits? (with a blanket impl linking them)

Associated constants would be nice for stack-allocated types, and methods for heap-allocated types.

@JonathanWoollett-Light
Copy link
Author

JonathanWoollett-Light commented May 30, 2023

I assume many of the traits in this crate predate associated constants, but is there any thought to having both trait methods and associated constants? Perhaps in separate traits? (with a blanket impl linking them)

Associated constants would be nice for stack-allocated types, and methods for heap-allocated types.

I think this makes sense (although will likely not be very useful until generic_const_exprs is stabilized, see below).

In this case, these implementations may look like:

trait Bits {
    fn bits() -> u32;
}
trait ConstBits: Bits {
    const BITS: u32;
}
impl Bits for u32 {
    fn bits() -> u32 { u32::BITS }
}
impl ConstBits for u32 {
    const BITS: u32 = u32::BITS;
}

In the future when const-trait-impl is stabilized it may look like:

#[const_trait]
trait Bits {
    fn bits() -> u32;
}
impl const Bits for u32 {
    fn bits() -> u32 { u32::BITS }
}

Presently const items are not particularly useful due to the error:

generic parameters may not be used in const operations type parameters may not be used in const expressions use #![feature(generic_const_exprs)] to allow generic const expressions

e.g.

struct Y;
trait MyTrait {
    const MY_CONST: usize;
}
impl MyTrait for Y {
    const MY_CONST: usize = 4;
}
fn main() {

}
fn blah<T: MyTrait>(x:T) where [();T::MY_CONST]: {
    let a = [00u8;T::MY_CONST];
}

but adding #![feature(generic_const_exprs)] fixes this.

A lot of potential right now is restricted by:

#![feature(generic_const_exprs)]
#![feature(const_trait_impl)]
#![feature(adt_const_params)]

@tarcieri
Copy link
Contributor

The nice thing about associated constants is they can be used in a const fn context on stable today, which as you noted won't be possible with trait methods until const impl is available.

@dzmitry-lahoda
Copy link

Was rounding num-rational Ratio to Ratio. This would be nice trait to use in this case.

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.

4 participants