Testing OwnedStorage trait, growable AllocVec

[andrewwhitehead]

For my needs it's pretty important to support a heap Vec that can reallocate, as well as being able to support either inline or allocated Vecs depending on what features are enabled. I put together this set of changes to test a few updates. I see you've created an issue recently which suggests adding a new GrowableVec type; that could also be supported under this general scheme, leaving AllocVec as constant capacity.

Here, the OwnedStorage trait allows for new, with_capacity, Clone, Default, and From to be supported for multiple storage types, including custom storage types that might be implemented externally. An interesting example would be a zeroizing buffer, which would have better characteristics than a Zeroizing<Vec<T>> (from the zeroize crate) because that type is not able to account for copies made during reallocations.

In order to support new for AllocVec it needs to either define a minimum capacity or behave like the standard Vec and start with no allocation, I chose the second option here. The realloc code would probably need significant work in order to optimize it and handle corner cases.

[teryror]

Hi! Thanks for tackling this, that'll be a huge help.

Some remarks:

• try_grow probably shouldn't mutate self, but return a Result<Self> instead. This way, it's the client data structures' responsibility to copy data over. There, we know how much storage is currently being used, so we don't have to potentially copy uninitialized memory, and we may also want to not copy over some data depending on the context that triggered the reallocation, e.g. in Vec::replace_range.
• It's important to me that even on AllocStorage, reallocation remain optional. Please, either introduce a new type, or add a generic parameter const GROWABLE: bool. Thinking about it now, I would actually favor the latter, as I think that could later be expanded to be a more configurable reallocation policy (e.g. multiply capacity by a different factor on each reallocation, set an optional maximum capacity...), which would be cool.
• You can't rely on realloc to do the copying for you. It may work for Vec, but it'll break the invariants of most other types.
• The OwnedStorage trait is the right idea in general, but I don't think it needs the new method when Default exists, which InlineStorage can also implement.

To be clear, I don't expect you to do the method-specific copy optimizations, the copying strategy in data structures other than Vec (that's the one you're interested in, right?), or the reallocation policy thing. I'll gladly do those myself (eventually...), if you don't want to do it. I'm happy you want to contribute at all, and I just want you to know where I'm going with this.

Also, it's fine you used rustfmt, but the formatting changes make it kind of annoying to find the semantic changes to review. It's on me for not doing this sooner, but I'll quickly run it myself on the whole code base. If you don't mind starting over from that commit, that would be very nice of you, but I fully understand if you don't want to do that.

[andrewwhitehead]

Awesome, I'll rebase to fix the conflicts due to the formatting. Rust analyzer is also having a hissy fit about some of the syntax, I think it might be the <T, const N: usize, O=Default> pattern which seems to be 1.59+.

For the try_grow implementation I agree that it would be better to return a new Storage instance, it would also be more conducive to things like a generic zeroizing wrapper around a storage type because it only needs to add a Drop implementation. And that doesn't break the invariants outlined in the comments. One thing I'm not sure about is whether the size of the Result would cause issues when called for a large Inline storage type, although I suppose an extra const trait property could be used to avoid that call if it is an issue.

Replacing owned_new with Default makes sense. It seems like AllocVec would only implement Default when it is Growable.

[andrewwhitehead]

Actually, it doesn't look like Default can be supported for InlineStorage without making it a newtype, because it's not supported for MaybeUninit. But it might make sense to move that functionality to a DefaultStorage trait for example.

[teryror]

Ah, true. I think it's fine to just make it a newtype though. Seems better than introducing another new trait.

[andrewwhitehead]

I have it implemented using an additional trait at the moment. The nice part about that is that the initializer can be const, although due to other limitations Vec::new cannot yet be const for supported Storage types.

[andrewwhitehead]

I think I'm done with potential updates for now, so I can mark this as ready for review or you can merge in what you like manually. It looks like as of Rust 1.61 the Vec::new() method could be made const, as trait bounds other than Sized were stabilized for const methods (as long as you aren't calling any trait methods). buffer_too_large_for_index_type being const would depend on the const_type_name feature being stabilized, or the Capacity trait could have a const name property. It's also possible to drop the DefaultStorage trait and implement Default for the InlineStorage but I think that would make a const initializer impossible.

I've added a try_with_capacity to avoid panics in Vec-owning structs that don't necessarily know what storage backend they will be using and want to provide a fallible constructor. I think in addition to the From implementations for slices and arrays, it might be useful to have matching TryFrom implementations?

[andrewwhitehead]

I've also added an initial implementation of the zeroizing storage wrapper I was talking about: https://github.com/andrewwhitehead/coca-zero

[andrewwhitehead]

Hiya, any thoughts on this PR?