Skip to content
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

Vec: add a "View" that can mutate the vec and erase the const generic. #425

Merged
merged 19 commits into from
Feb 7, 2024

Conversation

sosthene-nitrokey
Copy link
Contributor

@sosthene-nitrokey sosthene-nitrokey commented Dec 15, 2023

Alternative approach to #424, with even better ergonomics and features, but slightly more confusing documentation.

use heapless::{Vec, VecView};

let mut vec: Vec<u8, 10> = Vec::from_slice(&[1, 2, 3, 4]).unwrap();
// No const-generic on the view, and `&Vec` coerces to `&VecView` through unsize coercion 
let view: &VecView<u8> = &vec;
assert_eq!(view, &[1, 2, 3, 4]);

// No const-generic on the view, and `&mut Vec` coerces to `&mut VecView` through unsize coercion 
let mut_view: &mut VecView<u8> = &mut vec;
mut_view.push(5);
assert_eq!(vec, [1, 2, 3, 4, 5]);

Pros

This approach is more "scalable" in the sense that using it means that other structures that use a Vec internally can also have a View type that is simply the !Sized version of the same structure. This would be useful for the implementations of String, LinearMap, BinaryHeap, and heapless-bytes.

This approach also enables implicit conversion between the owned type and the view without needing to make the owned type Deref to the view.

Cons

The Vec and VecView types being aliases to a private type could be confusing.

Not totally sure about the semver compatibility of migrating to type alias for something that used to be a struct. Cargo semver-checks seems to complain.


Closes #424.

@sosthene-nitrokey sosthene-nitrokey changed the title Implement #424 but with the additional impl Unsize<VecView> for Vec Alternative approach to #424 with impl Unsize<VecView> for Vec Dec 15, 2023
@sosthene-nitrokey
Copy link
Contributor Author

sosthene-nitrokey commented Dec 15, 2023

I'm surprised by the miri failures, especially that hey don't happen in #424 where all the logic is the same, except that #424 relies on pointer casts where this PR relies just on Unsized coercion.

@sosthene-nitrokey
Copy link
Contributor Author

sosthene-nitrokey commented Dec 16, 2023

I think I fixed the documentation issues. By having a

#[cfg(doc)]
#[doc(hidden)]
pub use vec::VecInner as __private_api_do_not_use;

The documentation for the aliases is generated with all the methods. See rust-lang/rust#119015

The miri test fails were because not all methods were forwarded to the View implementation, but it seems to raise some very subtle behaviour with the as_mut_ptr method. It looks like there is some subtlety with stacked borrows when mix-matching methods on the view and methods on the owned type. I'm not really sure what this is about.

@burrbull
Copy link
Member

The documentation for the aliases is generated with all the methods.

Yeah this is painful.

@sosthene-nitrokey
Copy link
Contributor Author

Yeah this is painful.

What do you mean by that?

What I meant was that without the #[doc(hidden)] line the aliases where not documented at all by rustdoc. With the line, the aliases now properly all the traits they implement as well as the method that are available. Now the docs look like that:

image

Which I think is almost as good as it can get. The only "confusing" part is the second line showing it being an alias

@burrbull
Copy link
Member

What do you mean by that?

Sorry. It was just a comment.

svd2rust has the same issue and I also have not found better solution.
Type aliases make code much clearer and faster to compile, but documentation is not good due to this issue.

@sosthene-nitrokey sosthene-nitrokey force-pushed the vec-dyn-alias branch 3 times, most recently from 8e22a81 to 7cb0e80 Compare December 21, 2023 08:41
@sosthene-nitrokey sosthene-nitrokey changed the title Alternative approach to #424 with impl Unsize<VecView> for Vec Vec: add an "View" that can mutate the view and erase the const generic. Jan 2, 2024
@sosthene-nitrokey sosthene-nitrokey changed the title Vec: add an "View" that can mutate the view and erase the const generic. Vec: add an "View" that can mutate the vec and erase the const generic. Jan 2, 2024
src/vec.rs Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
src/vec.rs Outdated Show resolved Hide resolved
src/vec.rs Outdated Show resolved Hide resolved
src/vec.rs Outdated Show resolved Hide resolved
src/vec.rs Outdated Show resolved Hide resolved
src/vec.rs Outdated Show resolved Hide resolved
src/vec.rs Outdated Show resolved Hide resolved
src/vec.rs Outdated Show resolved Hide resolved
src/vec.rs Outdated Show resolved Hide resolved
@sosthene-nitrokey
Copy link
Contributor Author

I made a detailed blog post explaining the motivations and possible approaches to this PR: https://sgued.fr/blog/heapless-howto/

@reitermarkus
Copy link
Member

Firstly this would be a significant breaking change requiring a lot of downstream churn when released.

Yes, which is why I am asking this now so we don't immediately need a breaking change for VecView as well, which would make it more complex than just replacing Vec with Vector/VecBuf. It really comes down to how much we care about following the std API (#442).

What would the equivalent for StringView be? Naming it str feels like asking for trouble.

I guess StringBuf -> &String -> &str?

@reitermarkus reitermarkus changed the title Vec: add an "View" that can mutate the vec and erase the const generic. Vec: add a "View" that can mutate the vec and erase the const generic. Feb 7, 2024
@sosthene-nitrokey
Copy link
Contributor Author

Given #442 it indeed makes sense to do the renaming you want, but I disagree with the main point of #442 and therefore I disagree with this renaming too.

@reitermarkus reitermarkus mentioned this pull request Feb 7, 2024
Copy link
Member

@reitermarkus reitermarkus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is working as intended. I don't think the naming is a blocker, but this should be discussed further in #442.

@sosthene-nitrokey
Copy link
Contributor Author

Once this is merged I'll make a similar PR for String and the other types in heapless that could benefit from this pattern.

@reitermarkus reitermarkus added this pull request to the merge queue Feb 7, 2024
Merged via the queue into rust-embedded:main with commit c593fa5 Feb 7, 2024
22 checks passed
@reitermarkus
Copy link
Member

Thanks, @sosthene-nitrokey!

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

Successfully merging this pull request may close these issues.

3 participants