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

Implement PartialOrd<&B> for &mut A #87328

Closed

Conversation

inquisitivecrystal
Copy link
Contributor

@inquisitivecrystal inquisitivecrystal commented Jul 21, 2021

Due to an apparent oversight, the impl PartialOrd<&B> for &mut A did not previously exist. This leads to some very weird asymmetry. To quote #65589, "you can't compare &mut i32 with &i32, but you can compare &i32 with &mut i32". This PR adds the missing impl.

This is essentially copied from Duddino's work in #65609.

Fixes #65589.

@rust-highfive
Copy link
Collaborator

r? @scottmcm

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 21, 2021
@scottmcm scottmcm added needs-fcp This change is insta-stable, so needs a completed FCP to proceed. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Jul 21, 2021
@scottmcm
Copy link
Member

scottmcm commented Jul 21, 2021

you can't compare &mut i32 with &i32, but you can compare &i32 with &mut i32

Hmm, but there's no impl PartialOrd<&mut B> for &A either.

If this were going to be added, then I'd expect both impls to be added. But I'm not sure it's legal to add this, because references are fundamental. (See how impl Extend for &mut E couldn't be added #48597 (comment).)

But this is an insta-stable impl, so I'll delegate this over to libs-api
r? @yaahc

@scottmcm scottmcm assigned yaahc and unassigned scottmcm Jul 21, 2021
@inquisitivecrystal
Copy link
Contributor Author

Hmm, but there's no impl PartialOrd<&mut B> for &A either.

I tried to add it, but it wouldn't build. I presumed, without much thinking about it, that that was broken in some way this wasn't. Silly of me, I guess. ;)

These continual trait problems are really frustrating from a stability without stagnation perspective. Not like I know how to fix them though.

@scottmcm
Copy link
Member

I guess one way works because of coercions but the other doesn't.

Hypothesis: if the LHS is a & and the RHS a &mut, the solver picks the & impl, and then the RHS reborrows. But if the LHS is a &mut then it can't fix the RHS.

(AKA the classic "inference and coercions don't mix" kind of problem.)

@inquisitivecrystal
Copy link
Contributor Author

inquisitivecrystal commented Jul 21, 2021

I guess one way works because of coercions but the other doesn't.

Hypothesis: if the LHS is a & and the RHS a &mut, the solver picks the & impl, and then the RHS reborrows. But if the LHS is a &mut then it can't fix the RHS.

(AKA the classic "inference and coercions don't mix" kind of problem.)

Hmmm. If that's the problem, maybe the real solution is to tweak the coercion rules somehow. I'll confess that I have no clue how they work or whether that's viable though.

@inquisitivecrystal
Copy link
Contributor Author

I'm going to close this. I think we need to do something about this, but this PR is probably too backwards incompatible to actually be worth merging.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-fcp This change is insta-stable, so needs a completed FCP to proceed. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

You can't compare &mut i32 with &i32, but you can compare &i32 with &mut i32
4 participants