Skip to content

needless_borrow suggestion doesn't compile #8367

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
smoelius opened this issue Jan 29, 2022 · 4 comments · Fixed by #8355 or #8441
Closed

needless_borrow suggestion doesn't compile #8367

smoelius opened this issue Jan 29, 2022 · 4 comments · Fixed by #8355 or #8441
Labels
C-bug Category: Clippy is not doing the correct thing

Comments

@smoelius
Copy link
Contributor

Summary

On the most recent nightly, the needless_borrow lint produces a suggestion that doesn't compile.

cargo +nightly-2022-01-29 clippy triggers this bug, but cargo +nightly-2022-01-28 clippy does not.

cc: @Jarcho (maybe?)

Reproducer

I tried this code:

fn main() {
    let s0 = String::new();
    let mut chars = s0.chars();
    let s1 = (&mut chars).take(0).collect::<String>();
    println!("{}", s1);
    drop(chars);
}

I expected to see this happen:

Nothing.

Instead, this happened:

warning: this expression borrows a value the compiler would automatically borrow
 --> src/main.rs:4:14
  |
4 |     let s1 = (&mut chars).take(0).collect::<String>();
  |              ^^^^^^^^^^^^ help: change this to: `chars`
  |
  = note: `#[warn(clippy::needless_borrow)]` on by default
  = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#needless_borrow

If you make the suggested change, you get:

error[E0382]: use of moved value: `chars`
    --> src/main.rs:6:10
     |
3    |     let mut chars = s0.chars();
     |         --------- move occurs because `chars` has type `Chars<'_>`, which does not implement the `Copy` trait
4    |     let s1 = chars.take(0).collect::<String>();
     |                    ------- `chars` moved due to this method call
5    |     println!("{}", s1);
6    |     drop(chars);
     |          ^^^^^ value used here after move
     |
note: this function takes ownership of the receiver `self`, which moves `chars`
    --> /home/smoelius/.rustup/toolchains/nightly-x86_64-unknown-linux-gnu/lib/rustlib/src/rust/library/core/src/iter/traits/iterator.rs:1317:13
     |
1317 |     fn take(self, n: usize) -> Take<Self>
     |             ^^^^

Version

rustc 1.60.0-nightly (e0a55f449 2022-01-28)
binary: rustc
commit-hash: e0a55f4491a729bffc63b402ba903d90858c806b
commit-date: 2022-01-28
host: x86_64-unknown-linux-gnu
release: 1.60.0-nightly
LLVM version: 13.0.0

Additional Labels

No response

@smoelius smoelius added the C-bug Category: Clippy is not doing the correct thing label Jan 29, 2022
@ivan
Copy link

ivan commented Jan 29, 2022

needless_borrow also started suggesting something that will compile but make the program segfault with a stack overflow :)

struct ThingA;
struct ThingB;

impl From<&ThingA> for ThingB {
    fn from(_: &ThingA) -> ThingB {
        ThingB
    }
}

impl From<ThingA> for ThingB {
    fn from(a: ThingA) -> ThingB {
        (&a).into()
    }
}

fn main() {
    let _: ThingB = ThingA.into();
}
warning: this expression borrows a value the compiler would automatically borrow
  --> src/main.rs:12:9
   |
12 |         (&a).into()
   |         ^^^^ help: change this to: `a`
   |
   = note: `#[warn(clippy::needless_borrow)]` on by default
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#needless_borrow

@crumblingstatue
Copy link

Here is another false positive:

use rand::distributions::Alphanumeric;
use rand::Rng;

#[derive(Debug)]
pub struct GameEntity {
    pub name: String,
    pub hp: i32,
    pub godmode: bool,
}

impl GameEntity {
    pub fn rand() -> Self {
        let mut rng = rand::thread_rng();
        let name_len = rng.gen_range(3..24);
        Self {
            name: (&mut rng)
                .sample_iter(&Alphanumeric)
                .take(name_len)
                .map(char::from)
                .collect(),
            hp: rng.gen_range(0..100),
            godmode: rng.gen(),
        }
    }
}
warning: this expression borrows a value the compiler would automatically borrow
  --> src/lib.rs:16:19
   |
16 |             name: (&mut rng)
   |                   ^^^^^^^^^^ help: change this to: `rng`

@Jarcho
Copy link
Contributor

Jarcho commented Jan 29, 2022

This will be fixed by #8355 whenever that gets merged.

@chris-morgan
Copy link
Member

This issue is spawning quite a few duplicates: #8380, #8391, #8407, #8408, with multiple confirmations in several. #8355 (the patch that is supposed to fix this) doesn’t seem to be moving; it may need bumping, or else reverting whatever caused the regression, if possible.

@bors bors closed this as completed in 7ee2081 Feb 17, 2022
bors added a commit that referenced this issue Jun 28, 2022
Add lint `explicit_auto_deref` take 2

fixes: #234
fixes: #8367
fixes: #8380

Still things to do:

* ~~This currently only lints `&*<expr>` when it doesn't trigger `needless_borrow`.~~
* ~~This requires a borrow after a deref to trigger. So `*<expr>` changing `&&T` to `&T` won't be caught.~~
* The `deref` and `deref_mut` trait methods aren't linted.
* Neither ~~field accesses~~, nor method receivers are linted.
* ~~This probably shouldn't lint reborrowing.~~
* Full slicing to deref should probably be handled here as well. e.g. `&vec[..]` when just `&vec` would do

changelog: new lint `explicit_auto_deref`
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
Projects
None yet
5 participants