Skip to content

needless_pass_by_ref_mut false positive in nested async #11299

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

Closed
hatoo opened this issue Aug 6, 2023 · 10 comments · Fixed by #11314
Closed

needless_pass_by_ref_mut false positive in nested async #11299

hatoo opened this issue Aug 6, 2023 · 10 comments · Fixed by #11314
Labels
C-bug Category: Clippy is not doing the correct thing I-false-positive Issue: The lint was triggered on code it shouldn't have

Comments

@hatoo
Copy link

hatoo commented Aug 6, 2023

Summary

needless_pass_by_ref_mut false positive

I think it's another variant of #11179, sorry if this issue is a duplicate.

Lint Name

needless_pass_by_ref_mut

Reproducer

I tried this code:

async fn f(x: &mut i32) {
    async {
        *x += 1;
    }
    .await;
}

I saw this happen:

❯ cargo +nightly clippy 
warning: this argument is a mutable reference, but not used mutably
 --> src/main.rs:8:15
  |
8 | async fn f(x: &mut i32) {
  |               ^^^^^^^^ help: consider changing to: `&i32`
  |
  = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#needless_pass_by_ref_mut
  = note: `#[warn(clippy::needless_pass_by_ref_mut)]` on by default

I expected to see this happen:

No warning

Version

rustc 1.73.0-nightly (eb088b8b9 2023-08-05)
binary: rustc
commit-hash: eb088b8b9d98f1af1b0e61bbdcd8686e1b0db7b6
commit-date: 2023-08-05
host: x86_64-unknown-linux-gnu
release: 1.73.0-nightly
LLVM version: 16.0.5

Additional Labels

No response

@hatoo hatoo added C-bug Category: Clippy is not doing the correct thing I-false-positive Issue: The lint was triggered on code it shouldn't have labels Aug 6, 2023
@Centri3
Copy link
Member

Centri3 commented Aug 6, 2023

cc @GuillaumeGomez

Doesn't look like the same issue.

@GuillaumeGomez
Copy link
Member

I see the problem: need recursion for closures. Gonna check tomorrow.

@GuillaumeGomez
Copy link
Member

I opened #11314 to fix this issue.

@MingweiSamuel
Copy link
Contributor

MingweiSamuel commented Aug 16, 2023

Following example is also a false positive: (and looks about the same?)
https://play.rust-lang.org/?version=nightly&mode=debug&edition=2021&gist=2f930223459b2cb29f6c9e4a06a87546

pub async fn foo(n: &mut usize) -> impl '_ + FnMut() {
    || {
        *n += 1;
    }
}

@GuillaumeGomez could you add [something like] this to the test cases in #11314 ?

Edit: #11380

@SpeckyYT
Copy link

SpeckyYT commented Aug 17, 2023

I'm also experiencing this same false positive.

warning: this argument is a mutable reference, but not used mutably
 --> src\input.rs:8:19
  |
8 |     inputs_queue: &mut VecDeque<Inputs<bool>>,
  |                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider changing to: `&VecDeque<Inputs<bool>>`
  |
  = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#needless_pass_by_ref_mut
  = note: `#[warn(clippy::needless_pass_by_ref_mut)]` on by default

despite doing inputs_queue.append(&mut inputs_vector) in code.
I also using async/await.

@GuillaumeGomez
Copy link
Member

@MingweiSamuel Thanks! I'll add it.

@SpeckyYT Would have been better to come up with a small example showing the issue. ^^'

@GuillaumeGomez
Copy link
Member

@MingweiSamuel Just took a look and your case is a different one. Do you mind opening a new issue and tagging me on it please?

@SpeckyYT
Copy link

Sorry for not providing an example.
Here is a simplified version of what my code is doing (the original code contains .awaits and other stuff, but they seem to not affect this bug).

async fn a(b: &mut Vec<bool>) {
    b.append(&mut vec![])
}
warning: this argument is a mutable reference, but not used mutably
 --> src\test.rs:1:15
  |
1 | async fn a(b: &mut Vec<bool>) {
  |               ^^^^^^^^^^^^^^ help: consider changing to: `&Vec<bool>`
  |

@GuillaumeGomez
Copy link
Member

Thanks! I don't know if #11314 is the one who fixed it but in my case I don't have any warning with your code. I tested with push as well just in case. I'll add your code as a regression test in the PR.

@bors bors closed this as completed in d5298be Aug 17, 2023
flip1995 pushed a commit to flip1995/rust that referenced this issue Aug 17, 2023
…_block, r=Centri3

Correctly handle async blocks for NEEDLESS_PASS_BY_REF_MUT

Fixes rust-lang/rust-clippy#11299.

The problem was that the `async block`s are popping a closure which we didn't go into, making it miss the mutable access to the variables.

cc `@Centri3`

changelog: none
flip1995 pushed a commit to flip1995/rust-clippy that referenced this issue Aug 24, 2023
…_block, r=Centri3

Correctly handle async blocks for NEEDLESS_PASS_BY_REF_MUT

Fixes rust-lang#11299.

The problem was that the `async block`s are popping a closure which we didn't go into, making it miss the mutable access to the variables.

cc `@Centri3`

changelog: none
@MingweiSamuel
Copy link
Contributor

MingweiSamuel commented Aug 29, 2023

@GuillaumeGomez

@MingweiSamuel Just took a look and your case is a different one. Do you mind opening a new issue and tagging me on it please?

Looks like someone opened #11380 for this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: Clippy is not doing the correct thing I-false-positive Issue: The lint was triggered on code it shouldn't have
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants