Implement storage::vec::Slice

[KaiserKarel]

Initial possible design for slices in storage::vec::Vec. If this design is safe and agreed upon, I'll add useful traits such as Iterator and co.

Due to the index trait returning a reference, we cannot slice like so:

let v = StorageVec::new()
let x = v[1..10];

as the implementation creates owned slices. I've not found a way around this yet.

[KaiserKarel]

@Robbepop do you have some preliminary feedback?

[HCastano]

So far I think it's looking alright.

I know it's early, but I still want to make sure it's said: make sure errors are correctly instead of unwrap()-ing 😄

[KaiserKarel]

So far I think it's looking alright.

I know it's early, but I still want to make sure it's said: make sure errors are correctly instead of unwrap()-ing

Totally agree :)

[KaiserKarel]

Still need to write the unit tests and provide examples in the doc comments, but I think the design is good now. Please take a look @Robbepop and @HCastano.

[HCastano]

I haven't been able to look at everything just yet, mainly went through the Slice/SlideMut impls today

[HCastano]

Switching reviewers from Robin to Michi since Robin is on vacation

[KaiserKarel]

Most of the tests have been ported from core. I still need to add tests to ensure that split_at_xxx methods are correct.

Aside from that, spell-check seems to trip over some symbols. These doc comments mirror standard, so do I add those to the config?

[HCastano]

Aside from that, spell-check seems to trip over some symbols. These doc comments mirror standard, so do I add those to the config?

Yeah, just add the words which you think are OK to the .config/cargo_spellcheck.dic file. Otherwise you can wrap words in backticks. cargo-spellcheck doesn't complain about words formatted like that.

[KaiserKarel]

I need to implement some tests which verify that compilation fails. Is there a policy on including (test) dependencies? I was thinking of using https://github.com/Manishearth/compiletest-rs.

[HCastano]

I need to implement some tests which verify that compilation fails. Is there a policy on including (test) dependencies? I was thinking of using https://github.com/Manishearth/compiletest-rs.

@KaiserKarel nope, no policies here. However, if you try and introduce a left-pad, or a fishy looking crate we'll definitely have something to say about it.

[KaiserKarel]

I need to implement some tests which verify that compilation fails. Is there a policy on including (test) dependencies? I was thinking of using https://github.com/Manishearth/compiletest-rs.

@KaiserKarel nope, no policies here. However, if you try and introduce a left-pad, or a fishy looking crate we'll definitely have something to say about it.

But then how will we know if a 'bool-is-true' 😄

[Robbepop]

Sorry for the late reply.

I have trouble with this PR since it looks to me as if the author simply copy'n'pasted a huge amount of code from the Rust standard library and fiddled around with it until it compiled again without making proper ink! specific design considerations whether the Rust standard library design is actually applicable to ink! itself.
One example for this is the ContiguousStorage trait which totally makes sense in Rust standard library means but I do not currently fully understand what the needs and reasoning for it is with respect to ink! storage model.

I do see the need for storage slices for storage vectors although I believe we need a more thorough approach that respects ink!'s very special storage model in all details.

[KaiserKarel]

simply copy'n'pasted a huge amount of code from the Rust standard library and fiddled around with it until it compiled again without making proper ink! specific design considerations whether the Rust standard library design is actually applicable to ink! itself.

I find this comment a bit attacking, as I went through quite some iterations, attempting to deduplicate a lot of code to abstract over mutable iterators etc, before then copying code from the stdlib to provide an almost identical interface. Spent a few evenings looking through this, and I believe that ContiguousStorage actually fits well, as ink! has at least two storage types which are semantically contiguous, but cannot be proven to be so by the compiler, such as proving that the mutable references obtained from the backing BtreeMap of StorageVec do not overlap.

This PR is of course a proposal implementation (although I have been a bit too busy lately to finish it up).

I'd love some feedback on why it does not work for Ink's storage models. I feel like the trait is a bit verbose and a lot of code gets added to the codebase, but couldn't find a simpler implementation.

[Robbepop]

Sorry, I did not want to sound attacking towards you since you obviously put a lot of effort into the PR.

My critique is more seen from 10k feet above point of view since the whole code in the PR looks a lot just like copy'n'pasted (which it is) and there is a natural aversion against copy'n'pasted code for me since there are often just way too many things you can oversee while doing so, even if done very cautiously.
The ContiguousMemory properties are often used in languages such as C++ or Rust to indicate that it is possible to interact with elements of the data structure using plain old pointer arithmetics to some limited extend.
In our storage model pointer arithmetic does not really make sense since we are actually operating on storage cells instead which are not at all contiguous on hardware level but contiguous with the same respect with which languages such as C++ and Rust are using the Index operators for. So they are just contiguous with respect to their abstraction.

However, if you view this on a more high-level point of view where you view storage cells as memory cells you could definitely apply contiguous storage notation to them and instead of pointer arithmetic you could use storage key arithmetic. So you actually got a point there!

[KaiserKarel]

Sorry, I did not want to sound attacking towards you since you obviously put a lot of effort into the PR.

No problem :).

With the copying, I attempted to find a balance with mimicking the stdlib as much as possible, as it seems to me that ink attempts to make the storage types mimick the regular stdlib collections with a drop in replacement. I can of course remove some/all of the helper functions if that is desirable.

In this case, ContiguousStorage might be half-appropriately named, if your first indication is pointer-arithmetic. Naming is of course the hardest problem. Perhaps renaming it to SliceLike would be more appropriate.

However there is a class of collections which can safely provide multiple mutable references/slices, in the case of ink those are StorageVec and StorageArray. Rust is a bit of a hassle with abstracting over mutability, that is why the trait is needed. Alternatively we can use a macro to generate concrete impl of iterators for vec and array.

[Robbepop]

In this case, ContiguousStorage might be half-appropriately named, if your first indication is pointer-arithmetic. Naming is of course the hardest problem. Perhaps renaming it to SliceLike would be more appropriate.

I think ContiguousStorage is the better name if documentation makes very clear what is meant by it.

However there is a class of collections which can safely provide multiple mutable references/slices, in the case of ink those are StorageVec and StorageArray. Rust is a bit of a hassle with abstracting over mutability, that is why the trait is needed. Alternatively we can use a macro to generate concrete impl of iterators for vec and array.

Yeah that bridge between StorageVec and StorageArray is pretty neat.

[KaiserKarel]

Aside from the feedback needing to be added, I feel uncomfortable merging this without #853. Not intending to be the one to introduce UB in ink!. 😨

[HCastano]

@KaiserKarel We've decided to close this PR. The reasoning is that we're looking to migrate all our data structures away from caching based to something more lightweight (see #945).

While it would be nice to have a Mapping based Slice, we don't think it's a high priority (which is why nobody's circled back to this). If you disagree with our decision let us know and we can talk through the best way to move forward.

Thanks for spending time working on this, we appreciate it!

[Robbepop]

While it would be nice to have a Mapping based Slice, we don't think it's a high priority (which is why nobody's circled back to this). If you disagree with our decision let us know and we can talk through the best way to move forward.

I cannot follow how a Mapping based Slice is anything we actually would ever want. Can you elaborate on this please? Currently we are using LazyIndexMap which works entirely different from Mapping. As soon as we have a single-caching (aka unchached) LazyIndexMap replacement where our ink_storage::collections::Vec can be based upon we are good to go further with this experiement again. Until then I would like to keep this closed since it adds tons of very performance critical code that requires very in-depth review.

[HCastano]

I cannot follow how a Mapping based Slice is anything we actually would ever want. Can you elaborate on this please? Currently we are using LazyIndexMap which works entirely different from Mapping.

Sure, maybe I didn't think through all the technical nuances here, but point is we are looking to move to a more lightweight data-structure based world. What specific building blocks we use is kinda irrelevant, but either way I think we'd need to have big changes to this PR (or other PRs, like to introduce a non-caching LazyIndexMap) to support this and it's not high priority.