Skip to content

LenType causes regression regarding supported Vec sizes and usage in libraries. #568

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

Closed
sosthene-nitrokey opened this issue Apr 29, 2025 · 7 comments
Labels

Comments

@sosthene-nitrokey
Copy link
Contributor

sosthene-nitrokey commented Apr 29, 2025

The addition of LenType to vecs and the DefaultLenType means that arbitrary length of Vec are much less usable.

Not all lengths "just work" with Vec like they did previously

For example, the following: Vec<u8, 587> does not compile anymore, and needs to be worked around with Vec<u8, u16, 587>.

This itself is I think a significant downgrade in readability (the benefits of LenType are in most cases I think not worth making code less readable, since the overhead of the Vec itself will often be negligible compared to the buffer of the Vec).

In a library setting it forces the library writer to be explicit of the LenType:

fn some_func<const N: usize>() -> Vec<u8, N> {
    todo!()
}

Fails to compile with

error[E0277]: Length `N` does not have a default `LenType` mapping
   --> src/lib.rs:261:35
    |
261 | fn some_func<const N: usize>() -> Vec<u8, N> {
    |                                   ^^^^^^^^^^ the trait `SmallestLenType` is not implemented for `Const<N>`
    |
    = note: Provide the `LenType` explicitly, such as `usize`
    = help: the following other types implement trait `SmallestLenType`:
              Const<0>
              Const<1000>
              Const<100>
              Const<101>
              Const<1024>
              Const<102>
              Const<103>
              Const<1048576>
            and 287 others

It makes compatibility with View types much harder.

Because of the LenType the following fails to compile:

let vec: &mut VecView<u8> = Vec::<u8, 8>::new();
error[E0308]: mismatched types
   --> src/lib.rs:262:33
    |
262 |     let vec: &mut VecView<u8> = Vec::<u8, 8>::new();
    |              ----------------   ^^^^^^^^^^^^^^^^^^^ expected `&mut VecInner<u8, usize, ...>`, found `VecInner<u8, u8, ...>`
    |              |
    |              expected due to this
    |
    = note: expected mutable reference `&mut VecInner<_, usize, VecStorageInner<[MaybeUninit<u8>]>>`
                          found struct `VecInner<_, u8, VecStorageInner<[MaybeUninit<u8>; 8]>>

It can be worked around by specifying the LenType, but this will mean that the VecView will not coerce from most Vecs since most will not use usize as the LenType.

@sosthene-nitrokey
Copy link
Contributor Author

While the LenType additions are welcome, I think the mechanism to auto-select the LenType is causing too much trouble, and should be opt-in. That could work just by having all LenTypes default to usize to keep the previous behaviour.
There could be another alias with the automatic LenType for those that need the memory savings and are fine with opting-into the issues that come with the automatic type selection.

I think this could warrant yanking the 0.9 release.

sosthene-nitrokey added a commit to Nitrokey/heapless that referenced this issue Apr 29, 2025
This is a breaking change, 0.9.0 should be yanked. See
rust-embedded#568
@sosthene-nitrokey
Copy link
Contributor Author

sosthene-nitrokey commented Apr 29, 2025

And actually I thought the next release would have been 0.8.1, not a breaking release. In my PR I made efforts to make sure that there were no breaking changes, and documented the breaking changes that were pending, but they were not implemented once it was decided to go with a 0.9.0 instead of 0.8.1 release: #494

I saw the PR for the 0.9 release I should have remembered this sorry.

sosthene-nitrokey added a commit to Nitrokey/heapless that referenced this issue Apr 29, 2025
This is a breaking change, 0.9.0 should be yanked. See
rust-embedded#568
@newAM newAM added the bug label Apr 29, 2025
@newAM
Copy link
Member

newAM commented Apr 29, 2025

This is a fairly serious issue that is going to break a lot of code.

I have yanked 0.9.0 from crates.io for now to save people the effort of attempting to update their code.

newAM added a commit to newAM/heapless that referenced this issue Apr 29, 2025
There is an unexpected regression in 0.9.0 with generics that is going
to break a lot of code.
rust-embedded#568
sosthene-nitrokey added a commit to Nitrokey/heapless that referenced this issue Apr 29, 2025
This is a breaking change, 0.9.0 should be yanked. See
rust-embedded#568
@reitermarkus
Copy link
Member

I wouldn't really call this a regression since LenType is working as designed, and is a breaking change and a bit less ergonomic, as was to be expected.

Vec<u8, 587> does not compile anymore, and needs to be worked around with Vec<u8, u16, 587>.

We can add more sizes, e.g. all up to 1024 to avoid needing the explicit type in most common cases.

In a library setting it forces the library writer to be explicit of the LenType:

Is that a bad thing? A library author should know whether usize is needed or e.g. u8 is sufficient for a particular function.

It makes compatibility with View types much harder.

Not that much harder:

let vec: &mut VecView<_, _> = &mut Vec::<u8, 8>::new();

Note this isn't an argument for or against anything, just a clarification that it isn't as dramatically broken as this issue suggests.

@sosthene-nitrokey
Copy link
Contributor Author

sosthene-nitrokey commented Apr 29, 2025

I don't think it's "broken". But it will cause a lot of code churn, for a feature I don't think many users need. At least we don't, we much more concerned about compile times, binary size, and reducing the use of generics in our application code than the small memory overhead this might save (when it even saves anything given the alignment requirements of T).

I suggested yanking because I don't think it was understood that the disruption would be this great.
Without the DefaultLenType mechanism, most crates could have upgraded by just bumping the number. Now it means almost every downstream will have to adapt their code, adding generics, that add noise to the code and increase monomorphization.

sosthene-nitrokey added a commit to Nitrokey/heapless that referenced this issue Apr 29, 2025
This is a breaking change, 0.9.0 should be yanked. See
rust-embedded#568
sosthene-nitrokey added a commit to Nitrokey/heapless that referenced this issue Apr 29, 2025
This is a breaking change, 0.9.0 should be yanked. See
rust-embedded#568
@tomkris
Copy link

tomkris commented May 5, 2025

There is a way to auto implement this kind of len for any capacity using unstable generic_const_exprs

https://play.rust-lang.org/?version=nightly&mode=debug&edition=2024&gist=1fd5196d7b7774192aab4aea17b13a11

Unfortunately I cannot make compiler to believe that implementation exists for all possible C: usize, so constraint on capacity has to be added on every struct where Vec<T, C> is used without explicitly specifying C as constant; this makes use of this approach even harder.

If we cannot figure out how to make len auto-size on all possible inputs, I thin kit should default to usize to keep backwards compatibility with existing code and maintain ergonomics.

Personally I'm very interested to have auto-size len though. I have highly memory constrained workload, some of my dependencies use heapless::Vec extensively.

sosthene-nitrokey added a commit to Nitrokey/heapless that referenced this issue May 19, 2025
This is a breaking change, 0.9.0 should be yanked. See
rust-embedded#568
@sosthene-nitrokey
Copy link
Contributor Author

Closed by #569

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants