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

hybrid-array: hybrid typenum/const-generic arrays #707

Merged
merged 7 commits into from
May 7, 2022
Merged

Conversation

tarcieri
Copy link
Member

@tarcieri tarcieri commented Jan 8, 2022

This crate is a generic-array-inspired array type which uses some of the ideas from crypto-bigint to provide type-level conversions between const generics and typenum-based arrays.

I originally looked at trying to add this sort of thing as an afterthought to generic-array, but felt that it would really be
helpful if it were the core abstraction everything else is built on. Also generic-array is full of huge swaths of complicated unsafe code which is completely unnecessary in the setup used in this crate, which merely uses the type system to map to Rust's underlying array types. It manages to have #[forbid(unsafe_code)] enabled, at least for now.

It provides a FlexArray type similar to GenericArray aloing with an Array<T, const N: usize> trait which can be used to map const generic parameters to underlying array types.

Macros are used to write the Array/ArrayLength trait impls along with other traits which can't be implemented generically because of unconstrained generic parameters: Index/IndexMut, IntoIterator, and TryFrom<&[T]>. These are impl'd for the following sizes.

  • 0-64
  • 96
  • 128
  • 192
  • 256
  • 384
  • 448
  • 512
  • 768
  • 896
  • 1024
  • 2048
  • 8192

This is a smaller range of sizes than what generic-array supports due to typenum type alias definitions. It is necessary to make the const generic linkage work. (Maybe flex-array is a bad choice of name in this regard)

Compile times are currently similar to generic-array with the current range of supported types. In debug mode I get 4s for flex-array and 2s for generic-array, and in release mode I get the opposite with 2s for flex-array and 4s for generic-array.

More sizes could be added at the cost of more compile time. Likewise there are some self-referential relationships between the traits that could be removed to improve compile times, but seem handy.

The overall API is much closer to core::array than GenericArray is. For example, GenericArray has a panicking From providing TryFrom-like behavior, and confusingly receives the blanket impl of TryFrom, so people can mistakenly use TryFrom expecting a fallible API only to get the blanket impl of From and a panic in the event of an error.

Everywhere possible, it uses const generic implementations of traits. This includes:

  • AsRef<[T; N]>
  • AsMut<[T; N]>
  • Borrow<[T; N]>
  • BorrowMut<T; N]>
  • From<[T; N]>

The Array<T, const N: usize> trait is likewise bounded on all of the impls above and can be used to express any combination of them.

@tarcieri
Copy link
Member Author

tarcieri commented Jan 8, 2022

Just a little experiment I whipped up to see how well it works. I know a lot of people have been frustrated with the ergonomics of generic-array, which seems to be rooted in a much earlier dialect of Rust and hasn't really evolved to keep up.

The plan there has been to migrate from generic-array to const generics, but the features we actually need to do that seem to be a reasonable amount of time off from now, particularly features like generic_const_exprs (rust-lang/rust#76560).

A hybrid approach means we can start using const generics in user-facing APIs, but then convert those arrays to FlexArray (using the IntoFlexArray trait) and then take advantage of the richer bounds that typenum presently supports. This should permit a much more incremental approach to introducing const generics into our codebases without losing the benefits of typenum where we need it.

Obviously migrating away from generic-array would be a monumental effort, but I think a lot of the complexity is actually in typenum and in many ways this could function as something close to a drop-in replacement. However, it's something that touches pretty much every crate, so it's no small task.

Copy link
Member

@newpavlov newpavlov left a comment

Choose a reason for hiding this comment

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

Looks interesting! I also had some minor irritations with generic-array around interoperability with built-in arrays. But we will need to try a full-scale migration to see whether functionality in the crate is sufficient and if not, to see what has to be added.

Off the top of my head, we use sequence::Concat in the IGE implementation (i.e. ideally we need a way to express "double/half of this block size") and it would be nice to have a stable variant of slice::as_chunks.

Another concern is MSRV of this crate, 1.56 could be a bit too steep for some of our widely used crates (e.g. sha2).

$(
impl<T> Array<T, $len> for FlexArray<T, typenum::$ty> {
type Length = typenum::$ty;

Copy link
Member

@newpavlov newpavlov Jan 8, 2022

Choose a reason for hiding this comment

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

Maybe it's worth to add an associated constant const LENGTH: usize = $len;?

Copy link
Member Author

Choose a reason for hiding this comment

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

Could do that.

Speaking of, I borrowed the ArrayLength name from generic-array, but within @RustCrypto it seems like we generally use *Size instead (which I also prefer for being shorter and easier to type, if nothing else).

What about using ArraySize instead? (and SIZE associated constants)

Copy link
Member

@newpavlov newpavlov Jan 8, 2022

Choose a reason for hiding this comment

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

Yeah, ArraySize sounds good. Maybe we also could use Array instead of FlexArray and use a different name for the trait? I think the trait will be used less often than the type (though a lot of explicit GenericArays have been replaced with type aliases based on the *User traits). And maybe the trait is not needed in the first place, i.e. be generic over ArraySize and have all methods be inherent on the wrapper type? UPD: Ah, scratch the last question, it probably will not work without more advanced const generics...

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, Array for the struct type sounds good. I played around with a more minimal Array trait before but it didn't permit the current generic impls of traits like AsRef/Borrow/From, and defining all of the traits with a macro really slowed down compile times

@tarcieri tarcieri changed the title [WIP] flex-array: hybrid typenum/const-generic arrays flex-array: hybrid typenum/const-generic arrays Jan 23, 2022
@tarcieri tarcieri marked this pull request as ready for review January 23, 2022 16:02
@tarcieri
Copy link
Member Author

Renamed the FlexArray type to Array, and the previous Array trait to ArrayOps.

I'm still going to keep poking around at attempts to simplify it.

Also still not wild about the name flex-array. Maybe const-array would be more descriptive, but still unclear it's arrays that provide const generic/typenum linkage, rather than const fn

@dconnolly
Copy link

Renamed the FlexArray type to Array, and the previous Array trait to ArrayOps.

I'm still going to keep poking around at attempts to simplify it.

Also still not wild about the name flex-array. Maybe const-array would be more descriptive, but still unclear it's arrays that provide const generic/typenum linkage, rather than const fn

I would love to use this, can it get merged? Otherwise I can point to this branch as a dependency

/// Flexible generic array type.
#[derive(Copy, Clone, Debug, Eq, Hash, PartialEq, PartialOrd, Ord)]
#[repr(transparent)]
pub struct Array<T, U: ArrayLength<T>>(pub U::ArrayType);

Choose a reason for hiding this comment

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

ArrayOps is not implemented for all Arrays, only for specific <T, length>s because of the macro; is it possible to have as_slice or similar methods on the struct, not just the ArrayOps trait? This is like what generic-array does.

Since the compiler doesn't know what type has ArrayOps implemented for it yet, it says it's not found. And all the other traits impl'd on Array like AsRef place a bound that requires ArrayOps to be impl'd for Array.

To get around this I impl'd an ArrayExt, but maybe we can add it to the Array struct?

Suggested change
pub struct Array<T, U: ArrayLength<T>>(pub U::ArrayType);
pub struct Array<T, U: ArrayLength<T>>(pub U::ArrayType);
impl<T, U> Array<T, U>
where
U: ArrayLength<T>,
{
pub fn as_slice(&self) -> &[T] {
self.0.as_ref()
}
}

This crate is a `generic-array`-inspired array type which uses some of
the ideas from `crypto-bigint` to provide type-level conversions between
const generics and typenum-based arrays.

I originally looked at trying to add this sort of thing as an
afterthought to `generic-array`, but felt that it would really be
helpful if it were the core abstraction everything else is built on.
Also `generic-array` is full of huge swaths of complicated `unsafe` code
which is completely unnecessary in the setup used in this crate, which
merely uses the type system to map to Rust's underlying array types.

It provides a `FlexArray` type similar to `GenericArray` aloing with an
`Array<T, const N: usize>` trait which can be used to map const generic
parameters to underlying array types.

Macros are used to write the `Array`/`ArrayLength` trait impls along
with other traits which can't be implemented generically because of
unconstrained generic parameters: `Index`/`IndexMut`, `IntoIterator`,
and `TryFrom<&[T]>`. These are impl'd for the following sizes.

- 0-64
- 96
- 128
- 192
- 256
- 384
- 448
- 512
- 768
- 896
- 1024
- 2048
- 8192

This is a smaller range of sizes than what `generic-array` supports due
to `typenum` type alias definitions. It is necessary to make the
const generic linkage work.

Compile times are similar to `generic-array` with the current range of
supported types. In `debug` mode I get 4s for `flex-array` and 2s for
`generic-array`, and in `release` mode I get the opposite with 2s for
`flex-array` and 4s for `generic-array`.

More sizes could be added at the cost of more compile time. Likewise
there are some self-referential relationships between the traits that
could be removed to improve compile times, but seem handy.

The overall API is *much* closer to `core::array` than `GenericArray`
is. For example, `GenericArray` has a panicking `From` providing
`TryFrom`-like behavior, and confusingly receives the blanket impl of
`TryFrom`, so people can mistakenly use `TryFrom` expecting a fallible
API only to get the blanket impl of `From` and a panic in the event of
an error.

Everywhere possible, it uses const generic implementations of traits.
This includes:

- `AsRef<[T; N]>`
- `AsMut<[T; N]>`
- `Borrow<[T; N]>`
- `BorrowMut<T; N]>`
- `From<[T; N]>`
@tarcieri tarcieri changed the title flex-array: hybrid typenum/const-generic arrays hybrid-array: hybrid typenum/const-generic arrays May 7, 2022
@tarcieri tarcieri merged commit 4601ef2 into master May 7, 2022
@tarcieri tarcieri deleted the flex-array branch May 7, 2022 17:43
@dconnolly
Copy link

🎉🎉🎉

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