op_ref false positive with deref coercions when comparing

[euclio]

Perhaps related to #2042, but it seemed different enough to open a new issue.

With the following code: https://play.rust-lang.org/?gist=35d367db95cd56394d0bb9a05247b94c&version=nightly

fn main() {
    let a: &str = "abc";
    let b: String = "abc".to_owned();
    println!("{}", a > &b);
}

Clippy complains that the reference should be removed, but removing the reference causes the code to stop compiling.

[oli-obk]

Clippy on playpen is down. If memory serves right, isn't it also suggesting to deref the other side?

[euclio]

The error message reported by clippy is:

┌ euclio@apollo ~/repos/op_ref_test (git:master)
└╌╌┄┄ ❯❯ cargo +nightly clippy
   Compiling op_ref_test v0.1.0 (file:///Users/anrussell/repos/op_ref_test)
warning: taken reference of right operand
 --> src/main.rs:6:20
  |
6 |     println!("{}", a > &b);
  |                    ^^^^--
  |                        |
  |                        help: use the right value directly: `b`
  |
  = note: #[warn(op_ref)] on by default
  = help: for further information visit https://rust-lang-nursery.github.io/rust-clippy/v0.0.191/index.html#op_ref

    Finished dev [unoptimized + debuginfo] target(s) in 1.37 secs

[oli-obk]

We need to suggest a deref on the lhs here, too.

[ghost]

From the playpen link, he is actually comparing &str and String. In this case, deref on the lhs doesn't work. I think this is a false positive since gt requires a reference.

fn main() {
    let a: &str = "abc";
    let b: String = "abc".to_owned();
    println!("{}", a > &b); // works
    println!("{}", *a > b); // doesn't work
}

   Compiling playground v0.0.1 (file:///playground)
error[E0308]: mismatched types
 --> src/main.rs:5:25
  |
5 |     println!("{}", *a > b); // doesn't work
  |                         ^ expected str, found struct `std::string::String`
  |
  = note: expected type `str`
             found type `std::string::String`

[oli-obk]

Dereferencing both sides works. We can probably figure this out by checking for autoref coercions

[ghost]

I didn't know that was possible but what benefit does *a > *b have over a > &b?
Also this seems to be a regression of #1665.

[oli-obk]

The advantage is one less temporary variable (the comparison operator internally takes a reference to its arguments).

[jsgf]

I just ran into this with https://play.rust-lang.org/?version=stable&mode=debug&edition=2015&gist=890234c7ce6d7ccde1d036ae41d782d4

pub fn false_clippy_op_ref<T: Ord + PartialOrd>(array: &[T], val: &T, idx: usize) -> bool {
    &array[idx] < val
}

[Luro02]

I think I ran into this too
https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=d57a5e67a0a9e7d50e4bf85a76c7602a

pub fn example_problematic_lint(value: &String, rhs: &str) -> bool {
    &**value < rhs
}


fn main() {
    dbg!(example_problematic_lint(&"Hello".to_string(), "Worl"));
}

Clippy output

   Checking playground v0.0.1 (/playground)
warning: needlessly taken reference of left operand
 --> src/main.rs:3:5
  |
3 |     &**value < rhs
  |     --------^^^^^^
  |     |
  |     help: use the left value directly: `**value`
  |
  = note: `#[warn(clippy::op_ref)]` on by default
  = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#op_ref

    Finished dev [unoptimized + debuginfo] target(s) in 0.57s

[flip1995]

This is probably fixed by #4878, but needs more tests, e.g. #2597 (comment).

An improvement of this lint would still be to suggest a deref on both sides, when possible.

[ssokolow]

I just ran into this with a newtype I defined:

warning: taken reference of right operand
   --> src/lib.rs:208:37
    |
208 |         || f64::from(stats.words) < f64::from(stats.urls) * &*config.min_words_per_url
    |                                     ^^^^^^^^^^^^^^^^^^^^^^^^--------------------------
    |                                                             |
    |                                                             help: use the right value directly: `*config.min_words_per_url`
    |
    = note: `#[warn(clippy::op_ref)]` implied by `#[warn(clippy::all)]`
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#op_ref

error[E0277]: cannot multiply `f64` by `config::newtypes::PositiveF64`
   --> src/lib.rs:208:59
    |
208 |         || f64::from(stats.words) < f64::from(stats.urls) * config.min_words_per_url
    |                                                           ^ no implementation for `f64 * config::newtypes::PositiveF64`
    |
    = help: the trait `std::ops::Mul<config::newtypes::PositiveF64>` is not implemented for `f64`
    = help: the following other types implement trait `std::ops::Mul<Rhs>`:
              <&'a f32 as std::ops::Mul<f32>>
              <&'a f64 as std::ops::Mul<f64>>
              <&'a i128 as std::ops::Mul<i128>>
              <&'a i16 as std::ops::Mul<i16>>
              <&'a i32 as std::ops::Mul<i32>>
              <&'a i64 as std::ops::Mul<i64>>
              <&'a i8 as std::ops::Mul<i8>>
              <&'a isize as std::ops::Mul<isize>>
            and 49 others

Granted, I'll implement Mul after posting this, but I'm sure there are situations where that's not an option.