search

RustCrypto / utils

Utility crates used in RustCrypto
440 stars 129 forks source link

hybrid-array: seal ArraySize? #975

Closed newpavlov closed 1 year ago

newpavlov commented 1 year ago

ArraySize is already bounded by the Unsigned trait sealed in typenum. Third-party users will not be able to add their own types which implement ArraySize and they can not implement it for typenum types as well. So this trait is already effectively can not be implemented by third-party crates. So why not make this trait safe and sealed?

Also, is there a use case for Array<T, U0>? Maybe we should remove ArraySize implementation for U0?

tarcieri commented 1 year ago

So why not make this trait safe and sealed?

It needs to be part of the public API, and since its supertrait is already sealed, I'm not sure what value sealing it provides?

Maybe we should remove ArraySize implementation for U0?

It's needed for things that may potentially be nonzero but are often zero in practice, like AeadCore::CiphertextOverhead: https://github.com/RustCrypto/traits/blob/5011178/aead/src/lib.rs

newpavlov commented 1 year ago

Yes, as I wrote in the OP the trait is effectively sealed. But the documentation may cause confusion: https://github.com/RustCrypto/utils/blob/316f8648d7a0d93850f909bfaa1f4471f0c953d1/hybrid-array/src/lib.rs#L605-L613

It's written as if the trait may be implemented in third-party crates. I think it could be worth to make this trait explicitly sealed or at the very least change the docs to indicate the sealed nature of the trait.

tarcieri commented 1 year ago

I mean, the NOTE at the bottom suggests as much, but if you would like to explicitly describe the sealed nature of the supertraits in the doc comments that seems fine.

I think it would be good to at least retain the safety comments which describe the invariants of the unsafe aspects of the trait, but they don't have to be doc comments.

newpavlov commented 1 year ago

In my understanding, the note implies that the trait can be implemented in third-party crates. Also, strictly speaking, typenum could in theory remove the sealed bound on Unsigned in a v1 release since it's a backwards-compatible change.