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

Improve compile error for borrowing mut while missing Sized trait bound #93078

Closed
wheird-lee opened this issue Jan 19, 2022 · 5 comments · Fixed by #106410
Closed

Improve compile error for borrowing mut while missing Sized trait bound #93078

wheird-lee opened this issue Jan 19, 2022 · 5 comments · Fixed by #106410
Labels
A-diagnostics Area: Messages for errors, warnings, and lints D-confusing Diagnostics: Confusing error or lint that should be reworked. D-newcomer-roadblock Diagnostics: Confusing error or lint; hard to understand for new users. D-terse Diagnostics: An error or lint that doesn't give enough information about the problem at hand. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@wheird-lee
Copy link

Given the following code:

trait Modify {
    fn modify(&mut self) ;
}

impl<T> Modify for T  {
    fn modify(&mut self)  {
        // ...
    }
}

trait Foo {
    fn mute(&mut self) {
        self.modify();
    }
}

The current output is:

self.modify();
^^^^^^^^^^^^^ cannot borrow as mutable

Ideally the output should look like:

the size for values of type `Self` cannot be known at compilation time

add Sized trait bound and the code compiles successfully:

trait Modify {
    fn modify(&mut self) ;
}

impl<T> Modify for T  {
    fn modify(&mut self)  {
        // ...
    }
}

trait Foo: Sized {
    fn mute(&mut self) {
        self.modify();
    }
}

rust version: rustc 1.58.0 (02072b4 2022-01-11)

@wheird-lee wheird-lee added A-diagnostics Area: Messages for errors, warnings, and lints T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jan 19, 2022
@wheird-lee
Copy link
Author

okay..l realized the real reason is that I implies Modify for T: Sized, so maybe "trait not implied for Self" is more suitable?

@compiler-errors
Copy link
Member

Oh lol, I think this is because of autoref.

It's trying to take &mut of self (i.e. grab a &mut &mut Self pointer) since that means T = &mut Self which is Sized, but the &mut self argument (i.e. self: &mut Self) is not mutable.

Wonder how much work it is to make autoref during method dispatch to skip taking &mut of a self method receiver in cases where it's known to fail (e.g. &mut self parameter shorthand syntax).

@wheird-lee
Copy link
Author

well, I think you are right. It's my thoughtlessness.

@compiler-errors
Copy link
Member

@Niiiiiil I think this is still worthwhile to keep open as an issue because of the confusing error message!

@wheird-lee wheird-lee reopened this Jan 20, 2022
@QuineDot
Copy link

This came up on URLO in the context of an extension trait, and I'd like to point out how unhelpful the error is:

  • Given that the signature is fn mute(&mut self) {, it's confusing to say
    error[E0596]: cannot borrow `self` as mutable, as it is not declared as mutable
      --> src/lib.rs:13:9
       |
    13 |         self.modify();
       |         ^^^^^^^^^^^^^ cannot borrow as mutable
  • This is the only thing that is said, there is no suggestion
  • You have to figure out it means the &mut Self is not mutable, vs. the referenced Self
  • You have to know about method resolution to have a chance of understanding it's trying to create a &mut &mut self to call the implementation of &mut Self
  • You have to know about implicit Sized bounds and where they aren't to have a chance of understanding why it's not calling an implementation of Self

It is a bit tricky though, as there are at least three possible suggestions:

  • Change the signature to fn mute(mut self: &mut Self) {
  • Add a Sized bound to trait Foo
  • Add a ?Sized bound on the blanket implementation:
    impl<T: ?Sized> Modify for T  {

The last solution is the most correct in my opinion (but can only be applied if that implementation is local).

Finally, this variation:

    fn mute(self: &mut Self) {
        self.modify();
    }

Does give a suggestion:

help: consider making the binding mutable
   |
11 |     fn mute(mut self: &mut Self) {
   |             +++

Even though it's not the ideal suggestion. Let me know if I should file a separate bug for &mut self sugar disabling diagnostic suggestions. (If it exists I didn't find it.)

@rustbot label +D-confusing +D-terse +D-newcomer-roadblock

@rustbot rustbot added D-confusing Diagnostics: Confusing error or lint that should be reworked. D-newcomer-roadblock Diagnostics: Confusing error or lint; hard to understand for new users. D-terse Diagnostics: An error or lint that doesn't give enough information about the problem at hand. labels Oct 27, 2022
JohnTitor pushed a commit to JohnTitor/rust that referenced this issue Jan 8, 2023
…-diag, r=compiler-errors

Suggest `mut self: &mut Self` for `?Sized` impls

Closes rust-lang#106325
Closes rust-lang#93078

The suggestion is _probably_ not what the user wants (hence `MaybeIncorrect`) but at least makes the problem in the above issues clearer. It might be better to add a note explaining why this is the case, but I'm not sure how best to word that so this is a start.

`@rustbot` label +A-diagnostics
@bors bors closed this as completed in fe07531 Jan 8, 2023
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 D-confusing Diagnostics: Confusing error or lint that should be reworked. D-newcomer-roadblock Diagnostics: Confusing error or lint; hard to understand for new users. D-terse Diagnostics: An error or lint that doesn't give enough information about the problem at hand. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants