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

hash_map::IterMut changed Send bounds with hashbrown #61357

Closed
cuviper opened this issue May 30, 2019 · 6 comments · Fixed by #61388
Closed

hash_map::IterMut changed Send bounds with hashbrown #61357

cuviper opened this issue May 30, 2019 · 6 comments · Fixed by #61388
Labels
A-collections Area: `std::collection` C-bug Category: This is a bug. regression-from-stable-to-beta Performance or correctness regression from stable to beta. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@cuviper
Copy link
Member

cuviper commented May 30, 2019

In 1.35, IterMut's Send requires K: Send, but in 1.36-beta it requires K: Sync. Both are auto-derived, so it must be a change in their internals. Obviously, hashbrown is quite different than the old implementation.

https://doc.rust-lang.org/1.35.0/std/collections/hash_map/struct.IterMut.html#impl-Send
image

https://doc.rust-lang.org/beta/std/collections/hash_map/struct.IterMut.html#impl-Send
image

Ditto for ValuesMut. I'm not sure if there's any real impact in this, but it should be considered, and perhaps audit for other similar changes.

cc @Amanieu, as discussed on IRC.

@cuviper cuviper added A-collections Area: `std::collection` C-bug Category: This is a bug. regression-from-stable-to-beta Performance or correctness regression from stable to beta. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels May 30, 2019
bors added a commit to rust-lang/hashbrown that referenced this issue May 30, 2019
Fix the bounds on K for the Send impl of IterMut

This makes the bounds match the ones of the previous `HashMap` implementation in libstd.

Fixes rust-lang/rust#61357
bors added a commit to rust-lang/hashbrown that referenced this issue May 30, 2019
Fix the bounds on K for the Send impl of IterMut

This makes the bounds match the ones of the previous `HashMap` implementation in libstd.

cc rust-lang/rust#61357
@Amanieu
Copy link
Member

Amanieu commented May 30, 2019

This is fixed in hashbrown 0.4.0.

@aclonegeek
Copy link
Contributor

@Amanieu Shouldn't ValuesMut be fixed as well?

@cuviper
Copy link
Member Author

cuviper commented Jun 1, 2019

Since ValuesMut is actually based on IterMut, it derives the same constraints.
https://docs.rs/hashbrown/0.4.0/hashbrown/hash_map/struct.ValuesMut.html#synthetic-implementations

@aclonegeek
Copy link
Contributor

@cuviper Ah, gotcha. Thanks! Sorry for the false alarm :).

Amanieu added a commit to Amanieu/rust that referenced this issue Jun 1, 2019
bors added a commit that referenced this issue Jun 1, 2019
@Aaron1011
Copy link
Member

I think it would be a good idea to add a run-pass to check that these auto-trait impls don't change again.

@gnzlbg
Copy link
Contributor

gnzlbg commented Jun 2, 2019

@Aaron1011 might make sense to add them as tests to most collections. Vec, VecDeque, etc. don't have tests for these either. I've made this error in some of my collections (changing some internal implementation detail and breaking users that rely on auto traits working "like before").

pietroalbini pushed a commit to pietroalbini/rust that referenced this issue Jun 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-collections Area: `std::collection` C-bug Category: This is a bug. regression-from-stable-to-beta Performance or correctness regression from stable to beta. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants