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

Invalid suggestion to change &self to &mut self in trait implementation #68049

Closed
phil-opp opened this issue Jan 9, 2020 · 3 comments · Fixed by #85100
Closed

Invalid suggestion to change &self to &mut self in trait implementation #68049

phil-opp opened this issue Jan 9, 2020 · 3 comments · Fixed by #85100
Labels
A-diagnostics Area: Messages for errors, warnings, and lints A-suggestion-diagnostics Area: Suggestions generated by the compiler applied by `cargo fix` A-trait-system Area: Trait system C-bug Category: This is a bug. D-invalid-suggestion Diagnostics: A structured suggestion resulting in incorrect code. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@phil-opp
Copy link
Contributor

phil-opp commented Jan 9, 2020

Summary: The compiler suggests changing &self to &mut self when trying to modify self in a method. For trait implemantions this does not work because it would require changing the trait definition too (which is often not possible).

Example:

I was trying to implement the GlobalAllocator trait from the standard library for a custom type:

use std::alloc::{GlobalAlloc, Layout};

struct Test(u32);

unsafe impl GlobalAlloc for Test {
    unsafe fn alloc(&self, _layout: Layout) -> *mut u8 {
        self.0 += 1;
        0 as *mut u8
    }

    unsafe fn dealloc(&self, _ptr: *mut u8, _layout: Layout) {
        unimplemented!();
    }
}

(playground)

This of course doesn't work since you can't modify data behind a &self reference:

error[E0594]: cannot assign to `self.0` which is behind a `&` reference
 --> src/main.rs:7:9
  |
6 |     unsafe fn alloc(&self, _layout: Layout) -> *mut u8 {
  |                     ----- help: consider changing this to be a mutable reference: `&mut self`
7 |         self.0 += 1;
  |         ^^^^^^^^^^^ `self` is a `&` reference, so the data it refers to cannot be written

However, I noticed that the help message suggests changing the &self to &mut self, which is incorrect since that would require changing the trait definition in the standard library too.

Suggestion: I don't know if this is possible, but maybe this help message should only be shown for "normal" methods and hidden when implementing trait methods?

@jonas-schievink jonas-schievink added A-diagnostics Area: Messages for errors, warnings, and lints A-suggestion-diagnostics Area: Suggestions generated by the compiler applied by `cargo fix` A-trait-system Area: Trait system C-bug Category: This is a bug. D-invalid-suggestion Diagnostics: A structured suggestion resulting in incorrect code. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jan 9, 2020
@Kixunil
Copy link
Contributor

Kixunil commented Jan 12, 2020

Maybe only hide the suggestion if the trait comes from a foreign crate?

@HKalbasi
Copy link
Member

Or suggest to change the trait (if it is in this crate) not change the function in the impl part.

@Kixunil
Copy link
Contributor

Kixunil commented Mar 25, 2021

One more idea: suggest Cell/RefCell/Mutex/RwLock

Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue May 10, 2021
Fix invalid suggestion of changing impl trait signature

Fix rust-lang#68049
@bors bors closed this as completed in 382f748 May 11, 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 A-suggestion-diagnostics Area: Suggestions generated by the compiler applied by `cargo fix` A-trait-system Area: Trait system C-bug Category: This is a bug. D-invalid-suggestion Diagnostics: A structured suggestion resulting in incorrect code. 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