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

Confusing error message when deriving PartialOrd for structs containing mutable slices #45888

Closed
eira-fransham opened this issue Nov 9, 2017 · 6 comments
Labels
A-diagnostics Area: Messages for errors, warnings, and lints C-enhancement Category: An issue proposing an enhancement or a PR with one. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@eira-fransham
Copy link

Minimal reproduction:

#[derive(Eq, PartialEq, PartialOrd)]
struct Foo<'a, T: 'a>(&'a mut [T]);

This produces the following error message repeated precisely 8 times. I cannot tell you why this is, I have no idea. I assume it's a failure in the PartialOrd derivation code that's not being reported correctly.

error[E0389]: cannot borrow data mutably in a `&` reference
 --> src/main.rs:2:23
  |
2 | struct Foo<'a, T: 'a>(&'a mut [T]);
  |                       ^^^^^^^^^^^^
  |                       |
  |                       assignment into an immutable reference
  |                       consider changing this to `&'a mut [T])`
@kennytm kennytm added the A-diagnostics Area: Messages for errors, warnings, and lints label Nov 9, 2017
@durka
Copy link
Contributor

durka commented Nov 9, 2017

At least on nightly it has been fixed to deduplicate the errors. However the real problem is code like this (adapted from the derive expansion):

struct Foo<'a, T: 'a>(&'a mut [T]);

impl<'a, T: PartialOrd + 'a> Foo<'a, T> {
    fn bar(&self) {
        match *self {
            Foo(ref slice) => { *slice < *slice; } //~ERROR cannot borrow data mutably in a `&` reference
        }
    }
}

For some reason the same issue does not occur with PartialEq, only PartialOrd. Here are the impls: ==, <. I can't see why there would be any difference.

@eira-fransham
Copy link
Author

eira-fransham commented Nov 10, 2017

It's really weird, manually writing an implementation that calls partial_cmp works fine https://play.rust-lang.org/?gist=786449ac2addf11a7b32b3a250337114&version=stable

use std::cmp;

#[derive(Eq, PartialEq)]
struct Foo<'a, T: 'a>(&'a mut [T]);

impl<'a, T: PartialOrd + 'a> PartialOrd for Foo<'a, T> {
    fn partial_cmp(&self, other: &Self) -> Option<cmp::Ordering> {
        self.0.partial_cmp(&other.0)
    }
}

@cuviper
Copy link
Member

cuviper commented Nov 10, 2017

If you try to build from --pretty=expanded, the errors are not coming from partial_cmp, but from lt etc. as @durka said. For example, on the playground:

 65     #[inline]
 66     fn lt(&self, __arg_0: &Foo<'a, T>) -> bool {
 67         match *__arg_0 {
 68             Foo(ref __self_1_0) =>
 69             match *self {
 70                 Foo(ref __self_0_0) =>
 71                 (*__self_0_0) < (*__self_1_0) ||
 72                     !((*__self_1_0) < (*__self_0_0)) && false,
 73             },
 74         }
 75     }
error[E0389]: cannot borrow data mutably in a `&` reference
  --> foo-expanded.rs:71:33
   |
68 |             Foo(ref __self_1_0) =>
   |                 -------------- consider changing this to `ref mut __self_1_0`
...
71 |                 (*__self_0_0) < (*__self_1_0) ||
   |                                 ^^^^^^^^^^^^^ assignment into an immutable reference

error[E0389]: cannot borrow data mutably in a `&` reference
  --> foo-expanded.rs:72:39
   |
70 |                 Foo(ref __self_0_0) =>
   |                     -------------- consider changing this to `ref mut __self_0_0`
71 |                 (*__self_0_0) < (*__self_1_0) ||
72 |                     !((*__self_1_0) < (*__self_0_0)) && false,
   |                                       ^^^^^^^^^^^^^ assignment into an immutable reference

I think the actual implementation in question is PartialOrd<&'b mut B> for &'a mut A. We have __self_0_0 as & &mut [T], but you can't just dereference that to get the &mut [T]. But then I also don't know why that's not a problem for PartialEq, or why it's calling this an assignment.

@cuviper
Copy link
Member

cuviper commented Nov 10, 2017

It's happy if you change to an explicit call, like this playground:

                //(*__self_0_0) < (*__self_1_0) ||
                //    !((*__self_1_0) < (*__self_0_0)) && false,
                ::std::cmp::PartialOrd::lt(__self_0_0, __self_1_0) ||
                    !::std::cmp::PartialOrd::lt(__self_1_0, __self_0_0) && false

@eira-fransham
Copy link
Author

It's very strange that the operators require ownership but don't actually take ownership. Maybe that could be improved in the typechecker/codegen/wherever, but it could lead to confusing behaviour where &T has a different PartialOrd implementation than T.

@TimNN TimNN added the C-enhancement Category: An issue proposing an enhancement or a PR with one. label Nov 14, 2017
@crlf0710 crlf0710 added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jun 11, 2020
@rylev
Copy link
Member

rylev commented Jan 14, 2021

This no longer produces an error and compiles fine on 1.49. Closing.

@rylev rylev closed this as completed Jan 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-diagnostics Area: Messages for errors, warnings, and lints C-enhancement Category: An issue proposing an enhancement or a PR with one. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

7 participants