-
Notifications
You must be signed in to change notification settings - Fork 14.1k
In BTreeMap::eq, do not compare the elements if the sizes are different.
#149125
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
+98
−1
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,96 @@ | ||||||
| //! Regression tests which fail if some collections' `PartialEq::eq` impls compare | ||||||
| //! elements when the collections have different sizes. | ||||||
| //! This behavior is not guaranteed either way, so regressing these tests is fine | ||||||
| //! if it is done on purpose. | ||||||
| use std::cmp::Ordering; | ||||||
| use std::collections::{BTreeMap, BTreeSet, HashMap, HashSet, LinkedList}; | ||||||
|
|
||||||
| /// This intentionally has a panicking `PartialEq` impl, to test that various | ||||||
| /// collections' `PartialEq` impls don't actually compare elements if their sizes | ||||||
| /// are unequal. | ||||||
| /// | ||||||
| /// This is not advisable in normal code. | ||||||
| #[derive(Debug, Clone, Copy, Hash)] | ||||||
| struct Evil; | ||||||
|
|
||||||
| impl PartialEq for Evil { | ||||||
| fn eq(&self, _: &Self) -> bool { | ||||||
| panic!("Evil::eq is evil"); | ||||||
| } | ||||||
| } | ||||||
| impl Eq for Evil {} | ||||||
|
|
||||||
| impl PartialOrd for Evil { | ||||||
| fn partial_cmp(&self, _: &Self) -> Option<Ordering> { | ||||||
| Some(Ordering::Equal) | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| impl Ord for Evil { | ||||||
| fn cmp(&self, _: &Self) -> Ordering { | ||||||
| // Constructing a `BTreeSet`/`BTreeMap` uses `cmp` on the elements, | ||||||
| // but comparing it with with `==` uses `eq` on the elements, | ||||||
| // so Evil::cmp doesn't need to be evil. | ||||||
| Ordering::Equal | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| // check Evil works | ||||||
| #[test] | ||||||
| #[should_panic = "Evil::eq is evil"] | ||||||
| fn evil_eq_works() { | ||||||
| let v1 = vec![Evil]; | ||||||
| let v2 = vec![Evil]; | ||||||
|
|
||||||
| _ = v1 == v2; | ||||||
| } | ||||||
|
|
||||||
| // check various containers don't compare if their sizes are different | ||||||
|
|
||||||
| #[test] | ||||||
| fn vec_evil_eq() { | ||||||
| let v1 = vec![Evil]; | ||||||
| let v2 = vec![Evil; 2]; | ||||||
|
|
||||||
| assert_eq!(false, v1 == v2); | ||||||
| } | ||||||
|
|
||||||
| #[test] | ||||||
| fn hashset_evil_eq() { | ||||||
| let s1 = HashSet::from([(0, Evil)]); | ||||||
| let s2 = HashSet::from([(0, Evil), (1, Evil)]); | ||||||
|
|
||||||
| assert_eq!(false, s1 == s2); | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit:
Suggested change
(also pertinent to rest of the assertions in this file) |
||||||
| } | ||||||
|
|
||||||
| #[test] | ||||||
| fn hashmap_evil_eq() { | ||||||
| let m1 = HashMap::from([(0, Evil)]); | ||||||
| let m2 = HashMap::from([(0, Evil), (1, Evil)]); | ||||||
|
|
||||||
| assert_eq!(false, m1 == m2); | ||||||
| } | ||||||
|
|
||||||
| #[test] | ||||||
| fn btreeset_evil_eq() { | ||||||
| let s1 = BTreeSet::from([(0, Evil)]); | ||||||
| let s2 = BTreeSet::from([(0, Evil), (1, Evil)]); | ||||||
|
|
||||||
| assert_eq!(false, s1 == s2); | ||||||
| } | ||||||
|
|
||||||
| #[test] | ||||||
| fn btreemap_evil_eq() { | ||||||
| let m1 = BTreeMap::from([(0, Evil)]); | ||||||
| let m2 = BTreeMap::from([(0, Evil), (1, Evil)]); | ||||||
|
|
||||||
| assert_eq!(false, m1 == m2); | ||||||
| } | ||||||
|
|
||||||
| #[test] | ||||||
| fn linkedlist_evil_eq() { | ||||||
| let m1 = LinkedList::from([Evil]); | ||||||
| let m2 = LinkedList::from([Evil; 2]); | ||||||
|
|
||||||
| assert_eq!(false, m1 == m2); | ||||||
| } | ||||||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1 +1,2 @@ | ||
| mod binary_heap; | ||
| mod eq_diff_len; |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the various
Setcollections don't need to have(usize, Evil)tuples as elements.Actually, I'm not even sure that having these tuples would catch the failures you're testing for, since the
usizewould be compared first and found unequal, soEvil::eqwould never be called. No?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this test actually fail on beta?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
now that I think about it, I'm not completely sure the
Map-collection tests would catch this regression either, since their keys would always compare as unequal and short-circuit thePartialEqimplementation?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The suggested change makes the two sets have the same length, so would not be testing the case of comparing sets with different lengths (actually, it makes the test panic during
HashSet::from([Evil; 2]), becauseHashSetwill callEvil::eqto deduplicate during construction). The tuples are there specifically to avoid callingEvil::eqwhen constructing the sets/maps, since the first element/key makes the comparison short-circuit.As I said in #149125 (comment) ,
To expand on that: currently
BTreeMap'seqimpl goes in order, so if it did compare the elements, it would start with the two first elements which are both(0, Evil), so it would callEvil::eq. Similarly,HashMap'seqimpl (checks the lengths are equal, and then) iterates overselfand checks that all ofself's keys exist inotherwith the equal values.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, missed that paragraph from the PR description. Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A holistic test would probably need a
struct Evil(usize);where theHashimplementation is guaranteed to return a different hash forEvil(0)andEvil(1), but panic in the impl forPartialEq::eq?(just curious, not sure if it's worth the implementation, and even if it did, could definitely go in a follow-up PR)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should it? was HashSet in the affected set?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, it shouldn't!
Brain went AWOL for a bit there :D