ref_binding_to_reference false negative in expressions misclassified as postfix

[dtolnay]

Summary

I noticed this surprising code because I am working on removing PREC_POSTFIX from rustc_ast.

rust-clippy/clippy_lints/src/dereference.rs

Lines 1157 to 1164 in 32374a1

	if parent.precedence().order() == PREC_POSTFIX {
	// Parentheses would be needed here, don't lint.
	*outer_pat = None;
	} else {
	pat.always_deref = false;
	let snip = snippet_with_context(cx, e.span, parent.span.ctxt(), "..", &mut pat.app).0;
	pat.replacements.push((e.span, format!("&{snip}")));
	}

The point of the above condition is to distinguish between 2 cases:

#![warn(clippy::ref_binding_to_reference)]

struct Struct { field: i32 }

impl std::ops::Index<bool> for &&Struct {
    type Output = i32;
    fn index(&self, _: bool) -> &Self::Output {
        &self.field
    }
}

fn main() {
    if let Some(ref x) = Some(&Struct { field: 1 }) {
        debug(x as *const _);
    }

    if let Some(ref x) = Some(&Struct { field: 1 }) {
        debug(x[false]);
    }
}

fn debug<T: std::fmt::Debug>(thing: T) {
    println!("{:?}", thing);
}

In the first if let/debug, Clippy wants to suggest that we remove the ref and change the next line to replace x with &x to preserve the meaning of the program.

warning: this pattern creates a reference to a reference
  --> src/main.rs:13:17
   |
13 |     if let Some(ref x) = Some(&Struct { field: 1 }) {
   |                 ^^^^^
   |
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#ref_binding_to_reference
note: the lint level is defined here
  --> src/main.rs:1:9
   |
1  | #![warn(clippy::ref_binding_to_reference)]
   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
help: try
   |
13 ~     if let Some(x) = Some(&Struct { field: 1 }) {
14 ~         debug(&x as *const _);
   |

In the second if let/debug, Clippy does not want to suggest removing the ref because changing debug(x[false]) to debug((&x)[false]) would require one more level of parentheses to preserve the meaning of the program than was needed before. Without the ref, debug(&x[false]) and debug(x[false]) both mean something different than what the program did originally, and neither one compiles.

error[E0608]: cannot index into a value of type `&Struct`
  --> src/main.rs:18:16
   |
18 |         debug(x[false]);
   |                ^^^^^^^

Everything described so far is behaving correctly.

The bug is that Clippy is trying to draw the above distinction based on the precedence of get_parent_expr(cx, e) being postfix, which doesn't make sense.

When evaluating whether to suggest removing the ref on a ref x, the x in the following expressions all have a parent expression kind with postfix precedence:

parent expr of x	ExprKind
x.await	Await
x()	Call
f(x)	Call
x[i]	Index
other[x]	Index
x.f()	MethodCall
other.f(x)	MethodCall
x?	Try

However, the parent expression kind being a postfix expression is not the right condition for the purpose of the ref_binding_to_reference lint. The relevant thing is whether x's parent expression is a postfix of x specifically. x() and f(x) are both Call expressions, but f(&x) is okay to suggest while (&x)() is not.

Of the expressions in the table, Clippy has a false negative on f(x) and other[x] and other.f(x).

Lint Name

ref_binding_to_reference

Reproducer

#![warn(clippy::ref_binding_to_reference)]

fn main() {
    if let Some(ref x) = Some(&false) {
        std::convert::identity(x);
    }
}

Version

rustc 1.81.0-nightly (bcf94dec5 2024-06-23)
binary: rustc
commit-hash: bcf94dec5ba6838e435902120c0384c360126a26
commit-date: 2024-06-23
host: x86_64-unknown-linux-gnu
release: 1.81.0-nightly
LLVM version: 18.1.7