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.

Sign up for GitHub

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

Jump to bottom

Document pitfall with impl PartialEq<B> for A #66566

Merged
merged 1 commit into from
Nov 23, 2019
Merged

Document pitfall with impl PartialEq<B> for A #66566

merged 1 commit into from
Nov 23, 2019

Conversation

robamler
Copy link
Contributor

Fixes #66476 by turning the violating example into an explicit
counterexample.

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @rkruppe (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 20, 2019
@robamler
Copy link
Contributor Author

Sorry for the two separate commits. I forgot to declare the counterexample as should_panic. Please let me know if I should squash the two commits into one and then force-push.

hanna-kruppe
hanna-kruppe approved these changes Nov 20, 2019
View reviewed changes
Copy link
Contributor

@hanna-kruppe hanna-kruppe left a comment

Choose a reason for hiding this comment

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

LGTM (except for a typo), thanks!

Separate commits are generally good for review, we just squash them after review / before merging. Which in this case would be after fixing the typo, then I'll happily merge this.

src/libcore/cmp.rs Outdated
/// ```
/// A comparison like the one above, which ignores some fields of the struct,
/// can be dangerous. It can easily lead to an unintended violation of the
/// requirements for a partial equivalence relation. Fore example, if we kept
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo

Suggested change
/// requirements for a partial equivalence relation. Fore example, if we kept
/// requirements for a partial equivalence relation. For example, if we kept

@robamler
Copy link
Contributor Author

Thank you for fixing the typo, Dylan!

@rkruppe Do you need me to squash or do you use the Github "Squash & Merge" feature? (Sorry, couldn't find this in the the contribution instructions.)

@hanna-kruppe
Copy link
Contributor

We're not using any variant of Github's merge button, it all goes through our integration bot @bors. They can't squash, so it'll have to happen manually. Sorry for the confusion.

@robamler
Document pitfall with impl PartialEq<B> for A
5028fd8 
Fixes rust-lang#66476 by turning the violating example into an explicit
counterexample.
@robamler
Copy link
Contributor Author

Squashed it into a single commit, should be ready to merge.

@hanna-kruppe
Copy link
Contributor

Thanks!

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Nov 22, 2019

📌 Commit 5028fd8 has been approved by rkruppe

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 22, 2019
pietroalbini added a commit to pietroalbini/rust that referenced this pull request Nov 22, 2019
@pietroalbini
Rollup merge of rust-lang#66566 - robamler:issue-66476, r=rkruppe
203c906 
Document pitfall with `impl PartialEq<B> for A`

Fixes rust-lang#66476 by turning the violating example into an explicit
counterexample.
@pietroalbini pietroalbini mentioned this pull request Nov 22, 2019
Closed
Centril added a commit to Centril/rust that referenced this pull request Nov 22, 2019
@Centril
Rollup merge of rust-lang#66566 - robamler:issue-66476, r=rkruppe
afc78e1 
Document pitfall with `impl PartialEq<B> for A`

Fixes rust-lang#66476 by turning the violating example into an explicit
counterexample.
@Centril Centril mentioned this pull request Nov 22, 2019
Merged
bors added a commit that referenced this pull request Nov 22, 2019
@bors
Auto merge of #66640 - Centril:rollup-862009l, r=Centril
a449535 
Rollup of 8 pull requests

Successful merges:

 - #66183 (*Syntactically* permit visibilities on trait items & enum variants)
 - #66566 (Document pitfall with `impl PartialEq<B> for A`)
 - #66575 (Remove pretty printing of specific nodes in AST)
 - #66587 (Handle statics in MIR as const pointers)
 - #66619 (follow the convention in this file to use third-person singular verbs)
 - #66633 (Error code's long explanation cleanup)
 - #66637 (fix reoccuring typo: dereferencable -> dereferenceable)
 - #66639 (resolve: more declarative `fresh_binding`)

Failed merges:

r? @ghost
@bors bors merged commit 5028fd8 into rust-lang:master Nov 23, 2019
@robamler robamler deleted the issue-66476 branch November 23, 2019 04:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hanna-kruppe hanna-kruppe hanna-kruppe approved these changes

Assignees

@hanna-kruppe hanna-kruppe

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Documentation of PartialEq has example that violates transitivity
4 participants
@robamler @rust-highfive @hanna-kruppe @bors