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

Wrong syntax suggested by "consider changing this to be a mutable reference" with mut parameter in closure when iterating a vector #115259

Closed
chylex opened this issue Aug 26, 2023 · 3 comments · Fixed by #115308
Labels
A-closures Area: Closures (`|…| { … }`) A-diagnostics Area: Messages for errors, warnings, and lints A-suggestion-diagnostics Area: Suggestions generated by the compiler applied by `cargo fix` 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

@chylex
Copy link

chylex commented Aug 26, 2023

Code

fn main() {}

pub trait Layer {
	fn process(&mut self) -> u32;
}

pub struct State {
	layers: Vec<Box<dyn Layer>>,
}

impl State {
	pub fn process(&mut self) -> u32 {
		self.layers.iter().fold(0, |result, mut layer| result + layer.process())
	}
}

Current output

error[E0596]: cannot borrow `**layer` as mutable, as it is behind a `&` reference
  --> src/main.rs:13:59
   |
13 |         self.layers.iter().fold(0, |result, mut layer| result + layer.process())
   |                                                                 ^^^^^^^^^^^^^^^ `layer` is a `&` reference, so the data it refers to cannot be borrowed as mutable
   |
help: consider changing this to be a mutable reference
   |
13 |         self.layers.iter().fold(0, |result, mut lmut ayer| result + layer.process())
   |                                                  +++

Desired output

No response

Rationale and extra context

Initially, my State.process function looked like this:

self.layers.iter().fold(0, |result, layer| result + layer.process())

This led to a diagnostic that already has an open issue #62387 (it should suggest iter -> iter_mut).

I didn't notice I was not using iter_mut and tried changing layer -> mut layer, which led to the compiler somehow adding a second mut in the middle of the closure parameter.

obrazek

I don't know what the desired output should be in this case; I don't think mut parameter is correct here either way, but it might be worth investigating why the compiler is making a syntactically wrong suggestion.

Other cases

No response

Anything else?

Tested on 1.71.0 and 1.72.0 (stable-x86_64-unknown-linux-gnu).

@chylex chylex 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 Aug 26, 2023
@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Aug 26, 2023
@asquared31415
Copy link
Contributor

Minimized, it does not need the trait or dyn, just needs to try to use a & arg as a &mut and have the arg be mut binding:

fn take_f(_: impl FnMut(&String)) {}

fn main() {
    #[allow(unused_mut)] // `mut layer` needed to reproduce the diagnostic
    take_f(|mut layer| {
        layer.push('\n');
    });
}

@rustbot label -needs-triage +A-closures +A-suggestion-diagnostics +D-invalid-suggestion

@rustbot rustbot added A-closures Area: Closures (`|…| { … }`) A-suggestion-diagnostics Area: Suggestions generated by the compiler applied by `cargo fix` D-invalid-suggestion Diagnostics: A structured suggestion resulting in incorrect code. and removed needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. labels Aug 27, 2023
@chenyukang
Copy link
Member

chenyukang commented Aug 27, 2023

The syntactically wrong suggestion is removed in nightly(in both test cases).

https://play.rust-lang.org/?version=nightly&mode=debug&edition=2021&gist=8e4ce576c014f50452c9234bbd01acca

but the current suggestion also seems not so good,
it's best if we could suggest iter_mut from the beginning.

@chylex
Copy link
Author

chylex commented Aug 27, 2023

Suggesting iter_mut is already covered by #62387, so perhaps this should be closed unless there's a possible improvement to the compile error in asquared31415's minimized case that doesn't use vectors? I don't know enough about Rust to understand what the compiler wants, and there is no suggestion:

obrazek

Dylan-DPC added a commit to Dylan-DPC/rust that referenced this issue Sep 11, 2023
…ut, r=davidtwco

suggest iter_mut() where trying to modify elements from .iter()

Fixes rust-lang#115259
Fixes rust-lang#62387
@bors bors closed this as completed in 68c2f5b Sep 11, 2023
github-actions bot pushed a commit to rust-lang/miri that referenced this issue Sep 12, 2023
…twco

suggest iter_mut() where trying to modify elements from .iter()

Fixes rust-lang/rust#115259
Fixes rust-lang/rust#62387
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-closures Area: Closures (`|…| { … }`) A-diagnostics Area: Messages for errors, warnings, and lints A-suggestion-diagnostics Area: Suggestions generated by the compiler applied by `cargo fix` 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