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

modify ArraySize to allow for arbitrary size support by third parties #81

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

Conversation

conradludgate
Copy link

I had this idea on discord to allow for third party crates to provide their own sizes. It means that ArrayN<T, N> no longer is guaranteed to always work, but it still unblocks users with odd sized arrays such that they can unsafe impl ArraySize themselves.

This also provides a best-effort invariant check that can catch wrong impls at compile time.

I appreciate if this design is undesirable, but I feel like #66 & #79 will be a never ending issue otherwise.

@conradludgate
Copy link
Author

The invariant checker produces errors like

error[E0080]: evaluation of `<Size54321 as hybrid_array::ArraySize>::__CHECK_INVARIANT` failed
  --> /Users/conrad/Documents/code/rustcrypto/hybrid-array/src/traits.rs:23:9
   |
23 |         assert!(a == b, "ArraySize invariant violated");
   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the evaluated program panicked at 'ArraySize invariant violated', /Users/conrad/Documents/code/rustcrypto/hybrid-array/src/traits.rs:23:9
   |
   = note: this error originates in the macro `$crate::panic::panic_2021` which comes from the expansion of the macro `assert` (in Nightly builds, run with -Z macro-backtrace for more info)

note: erroneous constant encountered
  --> /Users/conrad/Documents/code/rustcrypto/hybrid-array/src/from_fn.rs:27:33
   |
27 |         core::convert::identity(U::__CHECK_INVARIANT);
   |                                 ^^^^^^^^^^^^^^^^^^^^

note: the above error was encountered while instantiating `fn hybrid_array::from_fn::<impl hybrid_array::Array<u8, Size54321>>::try_from_fn::<std::convert::Infallible, {closure@hybrid_array::from_fn::<impl hybrid_array::Array<u8, Size54321>>::from_fn<{closure@tests/custom_size.rs:21:49: 21:52}>::{closure#0}}>`
  --> /Users/conrad/Documents/code/rustcrypto/hybrid-array/src/from_fn.rs:17:9
   |
17 |         Self::try_from_fn::<Infallible>(|n| Ok(f(n))).expect("should never fail")
   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Which is a bit of soup - but the text "ArraySize invariant violated" does appear pretty early, also listing <Size54321 as hybrid_array::ArraySize> so you know which ArraySize impl is incorrectly implemented.

@conradludgate
Copy link
Author

For convenience, it might make sense to introduce type USIZE: usize = <Self::Size as Unsigned>::USIZE;. Doing this locally, I managed to get hashes to build with only minimal changes needed to the traits/utils crates.

Comment on lines -16 to -20
/// [`Unsigned::USIZE`]. Breaking this requirement will cause undefined behavior.
///
/// NOTE: This trait is effectively sealed and can not be implemented by third-party crates.
/// It is implemented only for a number of types defined in [`typenum::consts`].
pub unsafe trait ArraySize: Unsigned {
Copy link
Member

Choose a reason for hiding this comment

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

We use Unsigned::USIZE quite a bit, FWIW. I guess all of these usages would hypothetically have to change to ArraySize::Size::USIZE

Copy link
Author

Choose a reason for hiding this comment

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

Yeah, that goes for my follow up suggestion of adding the convenience const. Although you do run into ambiguities then

@tarcieri
Copy link
Member

So, I think something like this is interesting and something we should probably explore, but I worry about including it in the initial generic-array -> hybrid-array migration, specifically because it removes the supertrait bound on Unsigned for ArraySize which makes a complicated migration that much harder.

I'd like to keep this PR open, but I think we should skip it for hybrid-array v0.2

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.

2 participants