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

Incorrect suggestion when trying to write to an immutable field in an async function #93093

Closed
jyn514 opened this issue Jan 19, 2022 · 5 comments · Fixed by #93221
Closed

Incorrect suggestion when trying to write to an immutable field in an async function #93093

jyn514 opened this issue Jan 19, 2022 · 5 comments · Fixed by #93221
Labels
A-async-await Area: Async & Await A-borrow-checker Area: The borrow checker A-diagnostics Area: Messages for errors, warnings, and lints A-suggestion-diagnostics Area: Suggestions generated by the compiler applied by `cargo fix` AsyncAwait-Polish Async-await issues that are part of the "polish" area AsyncAwait-Triaged Async-await issues that have been triaged during a working group meeting. 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

@jyn514
Copy link
Member

jyn514 commented Jan 19, 2022

Given the following code: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=df6e36b97b73bd3e3ea0fdfe9325b1c9

struct S { foo: usize }
impl S {
    async fn bar(&self) {
        self.foo += 1;
    }
}

The current output is:

error[E0594]: cannot assign to `self.foo`, which is behind a `&` reference
 --> src/lib.rs:4:9
  |
3 |     async fn bar(&self) {
  |                  ----- help: consider changing this to be a mutable reference: `&mut S`
4 |         self.foo += 1;
  |         ^^^^^^^^^^^^^ `self` is a `&` reference, so the data it refers to cannot be written

Ideally the output should look like:

error[E0594]: cannot assign to `self.foo`, which is behind a `&` reference
 --> src/lib.rs:4:9
  |
3 |     async fn bar(&self) {
  |                  ----- help: consider changing this to be a mutable reference: `&mut self`
4 |         self.foo += 1;
  |         ^^^^^^^^^^^^^ `self` is a `&` reference, so the data it refers to cannot be written

&mut S is not a valid argument; it needs to be either &mut self or some_name: &mut S. For some reason this problem only happens when the function is async.

@jyn514 jyn514 added A-diagnostics Area: Messages for errors, warnings, and lints A-borrow-checker Area: The borrow checker T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. A-async-await Area: Async & Await A-suggestion-diagnostics Area: Suggestions generated by the compiler applied by `cargo fix` D-invalid-suggestion Diagnostics: A structured suggestion resulting in incorrect code. labels Jan 19, 2022
@compiler-errors
Copy link
Member

This is the same diagnostic you get when mutably dereferencing let-bindings that are &T: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=b71c99d23b4e58a67afd52a0870bed65
So it seems like this has to do with the fact that ast_lowering emits a let self = self; binding when desugaring.

I wonder if we can get the name of the variable when emitting this diagnostic, and special-case when it == "self".

@jyn514
Copy link
Member Author

jyn514 commented Jan 19, 2022

Another (possibly related?) bug: self.foo += 1 doesn't have the help suggestion if it's in an async block: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=23ea8e56639ab035349c69050bd74148

struct S { foo: usize }

impl S {
    fn bar(&self) {
        async move {
            self.foo += 1;
        };
    }
}

@compiler-errors
Copy link
Member

@jyn514 Same with closures. I would probably classify that as a separate bug, since the original bug has to do specifically with async fn desugaring, and the latter has to do with closure/generator captures.

@eholk
Copy link
Contributor

eholk commented Jan 24, 2022

Thanks for the report and getting a PR up so quickly!

@rustbot label +AsyncAwait-Triaged +AsyncAwait-Polish

@rustbot rustbot added AsyncAwait-Polish Async-await issues that are part of the "polish" area AsyncAwait-Triaged Async-await issues that have been triaged during a working group meeting. labels Jan 24, 2022
@jyn514
Copy link
Member Author

jyn514 commented Jan 25, 2022

Another similar error, not related to async this time:

error[E0596]: cannot borrow `*callback` as mutable, as it is behind a `&` reference
   --> boring/src/ssl/callbacks.rs:278:5
    |
273 |     let callback = ctx
    |         -------- help: consider changing this to be a mutable reference: `&mut F`
...
278 |     callback(ctx, session)
    |     ^^^^^^^^ `callback` is a `&` reference, so the data it refers to cannot be borrowed as mutable

let &mut F = ... isn't valid Rust.

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Feb 2, 2022
[borrowck] Fix help on mutating &self in async fns

Previously, when rustc was provided an async function that tried to
mutate through a shared reference to an implicit self (as shown in the
ui test), rustc would suggest modifying the parameter signature
to `&mut` + the fully qualified name of the ty (in the case of the repro
`S`). If a user modified their code to match the suggestion, the
compiler would not accept it.

This commit modifies the suggestion so that when rustc is provided the
ui test that is also attached in this commit, it suggests (correctly)
`&mut self`. We try to be careful about distinguishing between implicit
and explicit self annotations, since the latter seem to be handled
correctly already.

This is my first PR here so I'm pretty sure I probably missed something/could use better terminology. I also didn't try to make the match exhaustive since implicit self is the only real special case that I need to handle (that I'm aware of), and I'm pretty sure there's a cleaner way to do this so any advice would be greatly appreciated! (I'm also not terribly confident about how I wrote the ui tests)

here is your cc as requested `@compiler-errors`

This is an attempt to fix rust-lang#93093
@bors bors closed this as completed in b885700 Feb 2, 2022
@tmandry tmandry moved this to Done in wg-async work Dec 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-async-await Area: Async & Await A-borrow-checker Area: The borrow checker A-diagnostics Area: Messages for errors, warnings, and lints A-suggestion-diagnostics Area: Suggestions generated by the compiler applied by `cargo fix` AsyncAwait-Polish Async-await issues that are part of the "polish" area AsyncAwait-Triaged Async-await issues that have been triaged during a working group meeting. 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
Status: Done
Development

Successfully merging a pull request may close this issue.

4 participants