inconsistency in BoundedVec type

[thiolliere]

I feel like the bounded vec API serve 2 very purpose.

first purpose is to have a vec which is ensure not to go beyond some length, second purpose is to have a vec which logs some error when it goes beyond some length.

Mixing both makes the API quite inconsistent to me. Furthermore I think the doc is ambiguous:

/// As the name suggests, the length of the queue is always bounded. All internal operations ensure
/// this bound is respected.

This is the doc, but the implementation of Decode for bounded doesn't return an error when the vec is more than the expected length. (I understand why we accept longer vec, the reason is the second purpose: to have a vec which logs some error when it goes beyond some length. (also the decode implementation should ideally log such error)).

Another inconsistency: the API has: force_from which seems to be misused here #8793 (comment) ?
For the first-purpose bounded vec we can have a force_from_or_truncate instead which logs some error and ensure the bound is respected. for the second-purpose bounded vec we can have a force_from as it is currently

Also force_from and unchecked_from are memory safe. I think is it a wrong of unsafe property.

Proposition:

we introduce 2 types: BoundedVec and WeakBoundedVec (or LightBoundedVec, ..)

BoundedVec: return err when decoding a vec bigger than its bound, no method which allow to go beyond the bond.
WeakBoundedVec: log err when decoding a vec bigger than its bound, method to allow to go beyond the bond, but they log errors.

cc @shawntabrizi @coriolinus @kianenigma

[coriolinus]

Fundamentally I agree with you; there are definitely two use cases here, and we could have a simpler API by specializing our types to a single use case.

Also force_from and unchecked_from are memory safe. I think is it a wrong of unsafe property.

Disagree on this point: while those constructors are memory-safe, they require users to uphold, and potentially violate, a constraint which is otherwise guaranteed by the type system. To me this is the definition of unsafe code. It flags an area where the normal constraints aren't enforced.

we introduce 2 types: BoundedVec and WeakBoundedVec

BoundedT and WeakBoundedT naming scheme sounds good to me. We'd probably also implement WeakBoundedBTreeMap, WeakBoundedBTreeSet to be comprehensive.

However, do we have definite use cases for both strong and weak variants? If we never use the strong variant, or we never use the weak one, then we could save some trouble by just rewriting the existing types to use the appropriate property.

[thiolliere]

Disagree on this point: while those constructors are memory-safe, they require users to uphold, and potentially violate, a constraint which is otherwise guaranteed by the type system. To me this is the definition of unsafe code. It flags an area where the normal constraints aren't enforced.

Rust safe/unsafe is about memory safety and undefined behavior, not about difficult API. https://doc.rust-lang.org/book/ch19-01-unsafe-rust.html#when-to-use-unsafe-code
Here we don't use unsafe code, I don't see why it is marked as unsafe

If you want to warn about some function, then you should but this warning in the name of the function itself.

I'm very convince by this, and I think Sergei will back me up.

[kianenigma]

I agree with the overall point.

I recall that initially the type was supposed to be strictly bounded in my head, where you could only consume it to get the inner 'vec' and then use 'try_from'. Then I added the 'force_xxx' functions due to need which causes the confusion.

I agree that these can be two separate types, albeit personally wouldn't mind using the current one either. Also, I think in most use cases we actually want the weakly bounded version. Maybe we should focus on that first.

Regardless, I totally agree that the use of 'unsafe' here is wrong, as it is not about memory safety, thanks for pointing it out!

[shawntabrizi]

The solution here is to migrate all the logic which required force_xxx and then remove it from the code. But that is a larger item which we cannot have block the PoV work.

For now, we need it for backwards compatibility. Nothing here is worse than before, and generally situations where we use force_xxx we should be conscious of how the limits may be violated and adjusted accordingly.

[thiolliere]

ok, then I will for now consider that BoundedVec is not enforcing the vec length to respect the limit.
I made this PR to make the documetation more correct then #8842

[thiolliere]

some note: with a strictly enforced BoundedVec we could use them as call arguments directly, and we wouldn't even need to check the lenght.
Because the decoding would fail if the length is too big.