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

needless_borrow false positive, with broken suggestion #8380

Closed
ijackson opened this issue Jan 31, 2022 · 5 comments · Fixed by #8355 or #8441
Closed

needless_borrow false positive, with broken suggestion #8380

ijackson opened this issue Jan 31, 2022 · 5 comments · Fixed by #8355 or #8441

Comments

@ijackson
Copy link

To reproduce

Run cargo clippy with this in src/main.rs:

struct A { }

#[derive(Debug)]
struct B { }

impl From<&A> for B {
    fn from(_a: &A) -> B { B { } }
}

fn main() {
    let a = A { };
    
    // warning: this expression borrows a value the compiler would automatically borrow
    // 12 | let b: B = (&a).into();
    //                 ^^^^ help: change this to: `a`
    let b: B = (&a).into();
    
    // error[E0277]: the trait bound `B: std::convert::From<A>` is not satisfied
    // let b: B = a.into();
    
    println!("{:?}", &b)
}

Output with current nightly (bad)

    Checking foo v0.1.0 (/home/rustcargo/d/foo)
warning: this expression borrows a value the compiler would automatically borrow
  --> src/main.rs:16:16
   |
16 |     let b: B = (&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

warning: `foo` (bin "foo") generated 1 warning
    Finished dev [unoptimized + debuginfo] target(s) in 0.18s

Discussion

The disagnostic is not correct. Changing the program as clippy suggests does not work (see comment in my example). Both nightly and stable reject the modified program.

Stable clippy IMO-rightly accepts the original program without complaint.

Meta

rustcargo@zealot:~/d/foo$ rustc +stable --version
rustc 1.58.1 (db9d1b20b 2022-01-20)
rustcargo@zealot:~/d/foo$ rustc +nightly --version
rustc 1.60.0-nightly (6abb6385b 2022-01-26)
rustcargo@zealot:~/d/foo$ 
@ijackson
Copy link
Author

@rustbot modify labels +A-clippy +regression-from-stable-to-nightly

@ijackson
Copy link
Author

FTR, my test case was with a fresh project made with cargo new foo, so here is the Cargo.toml:

[package]
name = "foo"
version = "0.1.0"
edition = "2021"

# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html

[dependencies]

@ijackson
Copy link
Author

For my reference, this affects arti: https://gitlab.torproject.org/tpo/core/arti/-/issues/310

@matthiaskrgr matthiaskrgr transferred this issue from rust-lang/rust Jan 31, 2022
@camelid
Copy link
Member

camelid commented Jan 31, 2022

@rustbot label: -I-prioritize

@Jarcho
Copy link
Contributor

Jarcho commented Jan 31, 2022

Duplicate of #8367 with a fix in #8355.

@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
None yet
Projects
None yet
4 participants