Skip to content

Re-introduce covariance for containers #502

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

Merged
merged 1 commit into from
Mar 18, 2025

Conversation

sosthene-nitrokey
Copy link
Contributor

MpMc and spsc::Queue are not migrated to the new pattern because they were already invariant due to the UnsafeCell.

Fix #501

@sosthene-nitrokey sosthene-nitrokey force-pushed the vec-covariance branch 2 times, most recently from b17927e to aff1351 Compare July 4, 2024 15:48
@sosthene-nitrokey sosthene-nitrokey force-pushed the vec-covariance branch 2 times, most recently from 1a65947 to b862172 Compare October 8, 2024 09:26
@sosthene-nitrokey
Copy link
Contributor Author

Rebased. CI is failing only because of CHANGELOG.

@sosthene-nitrokey sosthene-nitrokey mentioned this pull request Feb 25, 2025
@sosthene-nitrokey sosthene-nitrokey force-pushed the vec-covariance branch 2 times, most recently from 40a7fd3 to 21dd498 Compare March 3, 2025 20:19
@newAM newAM modified the milestones: v0.9.0, v1.0.0 Mar 9, 2025
newAM
newAM previously approved these changes Mar 9, 2025
Copy link
Member

@newAM newAM left a comment

Choose a reason for hiding this comment

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

Thanks!

This looks logical, but I'm not confident enough to merge this on my review alone, will wait for another review from @rust-embedded/libs to merge.

@newAM newAM requested a review from a team March 9, 2025 18:59
@jannic
Copy link
Member

jannic commented Mar 12, 2025

It doesn't mean much because I'm not a member of the libs team, and also don't know much about the internals of heapless.

But I was curious and spent some time to understand both the issue and the proposed solution. The key information not obvious from other parts of the documentation was https://rustc-dev-guide.rust-lang.org/variance.html#variance-and-associated-types. After I found that, I'm convinced your proposal is a good solution. It's unfortunate that it complicates the code a bit, but it's probably worth it.

I tried to measure the effect on binary sizes and compiled the embassy uart_r503 example with heapless 0.8.0 and with this branch, and the version from this branch was actually 20 bytes smaller than with 0.8.0.
Notably, I was unable to compare it with the current main branch, because that didn't compile: It failed with some entirely unhelpful error message about lifetimes. :-) I guess this shows that the fix is indeed useful.

Copy link
Contributor

@therealprof therealprof left a comment

Choose a reason for hiding this comment

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

Looking great to me.

@therealprof therealprof added this pull request to the merge queue Mar 18, 2025
Merged via the queue into rust-embedded:main with commit 3c97d2f Mar 18, 2025
22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The deduped versions of the *View types break variance:
4 participants