Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
82 changes: 75 additions & 7 deletions clippy_lints/src/dereference.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ use clippy_utils::ty::{
use clippy_utils::{
DefinedTy, ExprUseNode, get_expr_use_site, get_parent_expr, is_block_like, is_from_proc_macro, is_lint_allowed, sym,
};
use rustc_ast::BinOpKind;
use rustc_ast::util::parser::ExprPrecedence;
use rustc_data_structures::fx::FxIndexMap;
use rustc_errors::Applicability;
Expand Down Expand Up @@ -1025,6 +1026,61 @@ fn ty_contains_field(ty: Ty<'_>, name: Symbol) -> bool {
}
}

// Returns true if the expression is in a position where replacing it with &expr would create a ref
// to a pattern binding that doesn't live long enough
fn is_in_pattern_branch_tail(tcx: TyCtxt<'_>, mut id: HirId) -> bool {
loop {
match tcx.parent_hir_node(id) {
Node::Arm(arm) => return arm.body.hir_id == id,
Node::Block(block) if block.expr.is_some_and(|e| e.hir_id == id) => {
id = block.hir_id;
},
Node::Expr(expr) => match expr.kind {
ExprKind::Block(..) | ExprKind::DropTemps(_) => {
id = expr.hir_id;
},
ExprKind::If(cond, then_expr, else_expr) => {
if then_expr.hir_id == id {
if contains_let_expr(cond) {
return true;
}
// Bubble up for normal if branch
id = expr.hir_id;
} else if else_expr.is_some_and(|e| e.hir_id == id) {
// Bubble up for normal else branch
id = expr.hir_id;
} else {
return false;
}
},

// break x or break { x }
ExprKind::Break(_, Some(break_expr)) if break_expr.hir_id == id => {
return true;
},

_ => return false,
},

_ => return false,
}
}
}

fn contains_let_expr(cond: &Expr<'_>) -> bool {
match cond.kind {
ExprKind::Let(_) => true,
// let _ = if cond && Some(ref x) = opt {
// x
// }
ExprKind::Binary(op, lhs, rhs) if matches!(op.node, BinOpKind::And) => {
contains_let_expr(lhs) || contains_let_expr(rhs)
},
ExprKind::DropTemps(expr) => contains_let_expr(expr),
_ => false,
}
}

impl<'tcx> Dereferencing<'tcx> {
fn in_deref_impl(&self) -> bool {
self.outermost_deref_impl.is_some()
Expand Down Expand Up @@ -1065,21 +1121,33 @@ impl<'tcx> Dereferencing<'tcx> {
pat.replacements.push((span, snip.into()));
},
Some(parent) if !parent.span.from_expansion() => {
// Double reference might be needed at this point.
if cx.precedence(parent) == ExprPrecedence::Unambiguous {
// Parentheses would be needed here, don't lint.
// If the expression is in the tail position of a match arm, suggesting
// `&x` would create a reference to a local binding that doesn't live
// long enough.
if is_in_pattern_branch_tail(cx.tcx, e.hir_id)
// If parentheses would be needed here, don't lint.
|| cx.precedence(parent) == ExprPrecedence::Unambiguous
{
*outer_pat = None;
} else {
// Double reference might be needed at this point.
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}")));
}
},
_ if !e.span.from_expansion() => {
// Double reference might be needed at this point.
pat.always_deref = false;
let snip = snippet_with_applicability(cx, e.span, "..", &mut pat.app);
pat.replacements.push((e.span, format!("&{snip}")));
// If the expression is in the tail position of a match arm, suggesting
// `&x` would create a reference to a local binding that doesn't live
// long enough.
if is_in_pattern_branch_tail(cx.tcx, e.hir_id) {
*outer_pat = None;
} else {
// Double reference might be needed at this point.
pat.always_deref = false;
let snip = snippet_with_applicability(cx, e.span, "..", &mut pat.app);
pat.replacements.push((e.span, format!("&{snip}")));
}
},
// Edge case for macros. The span of the identifier will usually match the context of the
// binding, but not if the identifier was created in a macro. e.g. `concat_idents` and proc
Expand Down
159 changes: 141 additions & 18 deletions tests/ui/ref_binding_to_reference.fixed
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
#![warn(clippy::ref_binding_to_reference)]
#![expect(clippy::explicit_auto_deref)]
#![expect(clippy::collapsible_match, clippy::explicit_auto_deref, clippy::never_loop)]
#![allow(clippy::needless_borrowed_reference)]

fn f1(_: &str) {}
Expand All @@ -23,21 +23,11 @@ fn main() {
None => return,
};

// Err, reference to a &String
#[expect(
clippy::ref_binding_to_reference,
reason = "The suggestion doesn't compile, see https://github.com/rust-lang/rust-clippy/issues/17370"
)]
let _: &&String = match Some(&x) {
Some(ref x) => x,
None => return,
};

// Err, reference to a &String
#[expect(
clippy::ref_binding_to_reference,
reason = "The suggestion doesn't compile, see https://github.com/rust-lang/rust-clippy/issues/17370"
)]
let _: &&String = match Some(&x) {
Some(ref x) => {
f1(x);
Expand Down Expand Up @@ -89,11 +79,144 @@ impl T1 for S {
}
}

fn check_expect_suppression() {
let x = String::new();
#[expect(clippy::ref_binding_to_reference)]
let _: &&String = match Some(&x) {
Some(ref x) => x,
None => return,
};
mod issue17370 {
fn f1(_: &str) {}

fn match_ref_some() {
let x = String::new();
let _: &&String = match Some(&x) {
Some(ref x) => x,
None => return,
};
}

fn match_ref(x: String) {
let _: &&String = match &x {
ref x => x,
_ => return,
};
}

fn if_let(x: String) {
let opt = Some(&x);

let _: &&String = if let Some(ref x) = opt {
x
} else {
return;
};
}

fn if_let_block_tail(x: String) {
let opt = Some(&x);

let _: &&String = if let Some(ref x) = opt {
f1(x);
f1(*x);
x
} else {
return;
};
}

fn normal_if_tail(x: String) {
let _: &&String = match Some(&x) {
Some(ref x) => {
if true {
x
} else {
return;
}
},
None => return,
};
}

fn normal_if_block_tail(x: String) {
let _: &&String = match Some(&x) {
Some(ref x) => {
if true {
f1(x);
f1(*x);
x
} else {
return;
}
},
None => return,
};
}

fn normal_else_tail(x: String) {
let _: &&String = match Some(&x) {
Some(ref x) => {
if true {
return;
} else {
x
}
},
None => return,
};
}

fn normal_else_block_tail(x: String) {
let _: &&String = match Some(&x) {
Some(ref x) => {
if true {
return;
} else {
f1(x);
f1(*x);
x
}
},
None => return,
};
}

fn break_if_let(x: String) {
let opt = Some(&x);

let _: &&String = loop {
if let Some(ref x) = opt {
break x;
} else {
return;
}
};
}

fn break_match(x: String) {
let opt = Some(&x);

let _: &&String = loop {
match opt {
Some(ref x) => break x,
None => return,
}
};
}

fn if_let_chain(x: String) {
let opt = Some(&x);

let _: &&String = if true && let Some(ref x) = opt {
x
} else {
return;
};
}

fn if_let_chain_block_tail(x: String) {
let opt = Some(&x);

let _: &&String = if true && let Some(ref x) = opt {
f1(x);
f1(*x);
x
} else {
return;
};
}
}
Loading