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 in case when compiler does not automatically borrow #9111

Closed
leighmcculloch opened this issue Jul 3, 2022 · 12 comments · Fixed by ruma/ruma#1579
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 I-suggestion-causes-error Issue: The suggestions provided by this Lint cause an ICE/error when applied

Comments

@leighmcculloch
Copy link

leighmcculloch commented Jul 3, 2022

Summary

The needless_borrow lint has a false positive where the suggested fix results in code that won't compile, because the compiler doesn't automatically borrow, even though the lint says it does.

Lint Name

needless_borrow

Reproducer

I tried this code:

#[derive(Debug, Clone, Hash, PartialEq, Eq, PartialOrd, Ord)]
pub struct VecM<T, const MAX: u32 = { u32::MAX }>(Vec<T>);

impl<T: Clone, const N: usize, const MAX: u32> TryFrom<&[T; N]> for VecM<T, MAX> {
    type Error = ();

    fn try_from(v: &[T; N]) -> Result<Self, ()> {
        todo!()
    }
}

#[cfg(test)]
mod test {
    use crate::VecM;

    #[test]
    fn test_try_from() {
        let _vm: VecM<u8, 32> = (&[]).try_into().unwrap();
        //let _vm: VecM<u8, 32> = [].try_into().unwrap();
    }
}
❯ cargo clippy --all-targets
    Checking clippy-needless-borrow v0.1.0 (/Users/leighmcculloch/Code/clippy-needless-borrow)
warning: this expression borrows a value the compiler would automatically borrow
  --> src/lib.rs:23:33
   |
18 |         let _vm: VecM<u8, 32> = (&[]).try_into().unwrap();
   |                                 ^^^^^ help: change this to: `[]`
   |
   = 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: `clippy-needless-borrow` (lib test) generated 1 warning
    Finished dev [unoptimized + debuginfo] target(s) in 0.12s

Making the change that clippy suggests, results in the following and the following compiler error:

#[derive(Debug, Clone, Hash, PartialEq, Eq, PartialOrd, Ord)]
pub struct VecM<T, const MAX: u32 = { u32::MAX }>(Vec<T>);

impl<T: Clone, const N: usize, const MAX: u32> TryFrom<&[T; N]> for VecM<T, MAX> {
    type Error = ();

    fn try_from(v: &[T; N]) -> Result<Self, ()> {
        todo!()
    }
}

#[cfg(test)]
mod test {
    use crate::VecM;

    #[test]
    fn test_try_from() {
        //let _vm: VecM<u8, 32> = (&[]).try_into().unwrap();
        let _vm: VecM<u8, 32> = [].try_into().unwrap();
    }
}
❯ cargo clippy --all-targets
    Checking clippy-needless-borrow v0.1.0 (/Users/leighmcculloch/Code/clippy-needless-borrow)
error[E0277]: the trait bound `VecM<u8, 32_u32>: std::convert::From<[_; 0]>` is not satisfied
  --> src/lib.rs:24:36
   |
19 |         let _vm: VecM<u8, 32> = [].try_into().unwrap();
   |                                    ^^^^^^^^ the trait `std::convert::From<[_; 0]>` is not implemented for `VecM<u8, 32_u32>`
   |
   = note: required because of the requirements on the impl of `std::convert::Into<VecM<u8, 32_u32>>` for `[_; 0]`
   = note: required because of the requirements on the impl of `std::convert::TryFrom<[_; 0]>` for `VecM<u8, 32_u32>`
   = note: required because of the requirements on the impl of `std::convert::TryInto<VecM<u8, 32_u32>>` for `[_; 0]`

For more information about this error, try `rustc --explain E0277`.
error: could not compile `clippy-needless-borrow` due to previous error

Version

❯ rustc -Vv
rustc 1.64.0-nightly (f2d93935f 2022-07-02)
binary: rustc
commit-hash: f2d93935ffba3ab9d7ccb5300771a2d29b4c8bf3
commit-date: 2022-07-02
host: x86_64-apple-darwin
release: 1.64.0-nightly
LLVM version: 14.0.6

Additional Labels

@rustbot label +I-suggestion-causes-error

@leighmcculloch leighmcculloch 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 Jul 3, 2022
@rustbot rustbot added the I-suggestion-causes-error Issue: The suggestions provided by this Lint cause an ICE/error when applied label Jul 3, 2022
@Jarcho
Copy link
Contributor

Jarcho commented Jul 3, 2022

Looks to be the same as #9095. Checking if a generic trait is implemented by a reference type isn't working correctly.

@droogmic
Copy link

Looks to be the same as #9095. Checking if a generic trait is implemented by a reference type isn't working correctly.

#9095 is fixed, but I still have this issue in 2022-07-17.

@Jarcho
Copy link
Contributor

Jarcho commented Jul 17, 2022

The fix hasn't been merged into the main rustc repo yet.

@droogmic
Copy link

ah true, I thought I had seen a release go out.

@yshui
Copy link

yshui commented Sep 9, 2022

Has this been released? I am still seeing this with rustup 2022-09-08 toolchain.

@kraktus
Copy link
Contributor

kraktus commented Sep 17, 2022

The issue is fixed on the 2022-09-16 toolchain (tested on playground)

@poliorcetics
Copy link

Do you know in which PR/commit ?

@kraktus
Copy link
Contributor

kraktus commented Sep 17, 2022

I'd assume #9096

@BusyJay
Copy link

BusyJay commented Oct 13, 2022

I'm still getting similar false positive on 2022-10-13.

struct A;

impl Extend<u8> for A {
    fn extend<T: IntoIterator<Item = u8>>(&mut self, _: T) { unimplemented!() }
}

impl<'a> Extend<&'a u8> for A {
    fn extend<T: IntoIterator<Item = &'a u8>>(&mut self, _: T) { unimplemented!() }
}

fn main() {
    let mut a = A;
    a.extend(&[]); // vs a.extend([]);
}

@smoelius
Copy link
Contributor

@BusyJay With your example, I see:

warning: the borrowed expression implements the required traits
  --> src/main.rs:13:14
   |
13 |     a.extend(&[]); // vs a.extend([]);
   |              ^^^ help: change this to: `[]`
   |
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#needless_borrow

I think this is a different bug.

bors added a commit that referenced this issue Oct 27, 2022
Fix `needless_borrow` false positive

The PR fixes the false positive exposed by `@BusyJay's` example in: #9111 (comment)

The current approach is described in #9674 (comment) and #9674 (comment).

The original approach appears below.

---

The proposed fix is to flag only "simple" trait implementations involving references, a concept
that I introduce next.

Intuitively, a trait implementation is "simple" if all it does is dereference and apply the trait
implementation of a type named by a type parameter. `AsRef` provides a good example of a simple
implementation: https://doc.rust-lang.org/std/convert/trait.AsRef.html#impl-AsRef%3CU%3E-for-%26T

We can make this idea more precise as follows. Given a trait implementation, first determine
whether the implementation is "used defined." If so, then examine its nested obligations.
Consider the implementation simple if-and-only-if:
- there is at least one nested obligation for the same trait
- for each type `X` in the nested obligation's substitution, either `X` is the same as that of
  the original obligation's substitution, or the original type is `&X`

For example, the following implementation from `@BusyJay's` example is "complex" (i.e., not simple)
because it produces no nested obligations:

```rust
impl<'a> Extend<&'a u8> for A { ... }
```

On the other hand, the following slightly modified implementation is simple, because it produces
a nested obligation for `Extend<X>`:

```rust
impl<'a, X> Extend<&'a X> for A where A: Extend<X> { ... }
```

How does flagging only simple implementations help? One way of interpreting the false positive in
`@BusyJay's` example is that it separates a reference from a concrete type. Doing so turns a
successful type inference into a failing one. By flagging only simple implementations, we
separate references from type variables only, thereby eliminating this class of false positives.

Note that `Deref` is a special case, as the obligations generated for it already involve the
underlying type.

r? `@Jarcho` (Sorry to keep pinging you with `needless_borrow` stuff. But my impression is no one knows this code better than you.)

changelog: fix `needless_borrow` false positive
@y21
Copy link
Member

y21 commented Jun 16, 2023

This issue seems to be fixed. None of the reproducers mentioned here get linted anymore it looks like: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=3c4b7032d2e1c46d488b42a1a5cc4627

@Jarcho
Copy link
Contributor

Jarcho commented Jun 16, 2023

Thank you for triaging.

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 I-suggestion-causes-error Issue: The suggestions provided by this Lint cause an ICE/error when applied
Projects
None yet
Development

Successfully merging a pull request may close this issue.

10 participants