From 383a9023b0c376df0812ff86302e8eb1f7f4aeaa Mon Sep 17 00:00:00 2001 From: Gri-ffin Date: Tue, 21 Jul 2026 23:24:58 +0100 Subject: [PATCH 1/2] fix ref_binding_to_reference advising to return a reference that doesn't live long enough --- clippy_lints/src/dereference.rs | 106 +++++++++++++++------ tests/ui/ref_binding_to_reference.fixed | 114 +++++++++++++++++++---- tests/ui/ref_binding_to_reference.rs | 114 +++++++++++++++++++---- tests/ui/ref_binding_to_reference.stderr | 10 +- 4 files changed, 272 insertions(+), 72 deletions(-) diff --git a/clippy_lints/src/dereference.rs b/clippy_lints/src/dereference.rs index cd1ad57ee828..4289f2dfce45 100644 --- a/clippy_lints/src/dereference.rs +++ b/clippy_lints/src/dereference.rs @@ -317,8 +317,8 @@ impl<'tcx> LateLintPass<'tcx> for Dereferencing<'tcx> { }, RefOp::Method { mutbl, is_ufcs } if !is_lint_allowed(cx, EXPLICIT_DEREF_METHODS, expr.hir_id) - // Allow explicit deref in method chains. e.g. `foo.deref().bar()` - && (is_ufcs || !is_in_method_chain(cx, expr)) => + // Allow explicit deref in method chains. e.g. `foo.deref().bar()` + && (is_ufcs || !is_in_method_chain(cx, expr)) => { let ty_changed_count = usize::from(!deref_method_same_type(expr_ty, typeck.expr_ty(sub_expr))); self.state = Some(( @@ -403,37 +403,37 @@ impl<'tcx> LateLintPass<'tcx> for Dereferencing<'tcx> { // If this trait impl is implemented on `&T`, then auto-borrowing won't work (impl_ty.is_ref() && implements_trait( - cx, - impl_ty, - trait_id, - &args[..cx.tcx.generics_of(trait_id).own_params.len() - 1], - )) + cx, + impl_ty, + trait_id, + &args[..cx.tcx.generics_of(trait_id).own_params.len() - 1], + )) // If there's an inherent method, or a method from another trait, // with the same name that's also implemented on this same type, // then removing the borrow might cause that method to be chosen // instead of the current one. || get_adt_inherent_method(cx, impl_ty, method_name).is_some() || cx.tcx.in_scope_traits(hir_id).is_some_and(|traits| { - traits - .iter() - .filter(|trait_| { - cx.tcx - .non_blanket_impls_for_ty(trait_.def_id, impl_ty) - .next() - .is_some() - || !cx - .tcx - .trait_impls_of(trait_.def_id) - .blanket_impls() - .is_empty() - }) - .any(|trait_| { - cx.tcx - .associated_items(trait_.def_id) - .filter_by_name_unhygienic(method_name) - .any(|item| item.tag() == AssocTag::Fn && item.def_id != fn_id) - }) - }) + traits + .iter() + .filter(|trait_| { + cx.tcx + .non_blanket_impls_for_ty(trait_.def_id, impl_ty) + .next() + .is_some() + || !cx + .tcx + .trait_impls_of(trait_.def_id) + .blanket_impls() + .is_empty() + }) + .any(|trait_| { + cx.tcx + .associated_items(trait_.def_id) + .filter_by_name_unhygienic(method_name) + .any(|item| item.tag() == AssocTag::Fn && item.def_id != fn_id) + }) + }) ) { false @@ -1025,6 +1025,43 @@ fn ty_contains_field(ty: Ty<'_>, name: Symbol) -> bool { } } +/// Returns `true` if the expression is in tail position of a branch whose pattern bindings do not +/// live past that branch. Replacing it with `&expr` would create a reference to a local 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 matches!(cond.kind, ExprKind::Let(_)) { + 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; + } + }, + + _ => return false, + }, + + _ => return false, + } + } +} + impl<'tcx> Dereferencing<'tcx> { fn in_deref_impl(&self) -> bool { self.outermost_deref_impl.is_some() @@ -1076,10 +1113,17 @@ impl<'tcx> Dereferencing<'tcx> { } }, _ 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 diff --git a/tests/ui/ref_binding_to_reference.fixed b/tests/ui/ref_binding_to_reference.fixed index 0a16bed4a43b..147079de13fa 100644 --- a/tests/ui/ref_binding_to_reference.fixed +++ b/tests/ui/ref_binding_to_reference.fixed @@ -1,6 +1,6 @@ #![warn(clippy::ref_binding_to_reference)] #![expect(clippy::explicit_auto_deref)] -#![allow(clippy::needless_borrowed_reference)] +#![allow(clippy::needless_borrowed_reference, clippy::collapsible_match)] fn f1(_: &str) {} macro_rules! m2 { @@ -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); @@ -89,11 +79,99 @@ 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, + }; + } } diff --git a/tests/ui/ref_binding_to_reference.rs b/tests/ui/ref_binding_to_reference.rs index 04d8c9e95cf3..d4bc967aeb40 100644 --- a/tests/ui/ref_binding_to_reference.rs +++ b/tests/ui/ref_binding_to_reference.rs @@ -1,6 +1,6 @@ #![warn(clippy::ref_binding_to_reference)] #![expect(clippy::explicit_auto_deref)] -#![allow(clippy::needless_borrowed_reference)] +#![allow(clippy::needless_borrowed_reference, clippy::collapsible_match)] fn f1(_: &str) {} macro_rules! m2 { @@ -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); @@ -89,11 +79,99 @@ 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, + }; + } } diff --git a/tests/ui/ref_binding_to_reference.stderr b/tests/ui/ref_binding_to_reference.stderr index b16cccf6f239..881010a7970e 100644 --- a/tests/ui/ref_binding_to_reference.stderr +++ b/tests/ui/ref_binding_to_reference.stderr @@ -1,5 +1,5 @@ error: this pattern creates a reference to a reference - --> tests/ui/ref_binding_to_reference.rs:52:14 + --> tests/ui/ref_binding_to_reference.rs:42:14 | LL | Some(ref x) => m2!(x), | ^^^^^ @@ -13,7 +13,7 @@ LL + Some(x) => m2!(&x), | error: this pattern creates a reference to a reference - --> tests/ui/ref_binding_to_reference.rs:58:15 + --> tests/ui/ref_binding_to_reference.rs:48:15 | LL | let _ = |&ref x: &&String| { | ^^^^^ @@ -27,7 +27,7 @@ LL ~ let _: &&String = &x; | error: this pattern creates a reference to a reference - --> tests/ui/ref_binding_to_reference.rs:66:12 + --> tests/ui/ref_binding_to_reference.rs:56:12 | LL | fn f2<'a>(&ref x: &&'a String) -> &'a String { | ^^^^^ @@ -42,7 +42,7 @@ LL ~ x | error: this pattern creates a reference to a reference - --> tests/ui/ref_binding_to_reference.rs:75:11 + --> tests/ui/ref_binding_to_reference.rs:65:11 | LL | fn f(&ref x: &&String) { | ^^^^^ @@ -56,7 +56,7 @@ LL ~ let _: &&String = &x; | error: this pattern creates a reference to a reference - --> tests/ui/ref_binding_to_reference.rs:85:11 + --> tests/ui/ref_binding_to_reference.rs:75:11 | LL | fn f(&ref x: &&String) { | ^^^^^ From 87a2a8b8fef728f8a22efe595b32960fcbcfa278 Mon Sep 17 00:00:00 2001 From: Gri-ffin Date: Wed, 29 Jul 2026 19:58:50 +0100 Subject: [PATCH 2/2] handle more where ref doesn't live long enoug --- clippy_lints/src/dereference.rs | 92 ++++++++++++++++--------- tests/ui/ref_binding_to_reference.fixed | 49 ++++++++++++- tests/ui/ref_binding_to_reference.rs | 49 ++++++++++++- 3 files changed, 152 insertions(+), 38 deletions(-) diff --git a/clippy_lints/src/dereference.rs b/clippy_lints/src/dereference.rs index 4289f2dfce45..a31a30036b3c 100644 --- a/clippy_lints/src/dereference.rs +++ b/clippy_lints/src/dereference.rs @@ -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; @@ -317,8 +318,8 @@ impl<'tcx> LateLintPass<'tcx> for Dereferencing<'tcx> { }, RefOp::Method { mutbl, is_ufcs } if !is_lint_allowed(cx, EXPLICIT_DEREF_METHODS, expr.hir_id) - // Allow explicit deref in method chains. e.g. `foo.deref().bar()` - && (is_ufcs || !is_in_method_chain(cx, expr)) => + // Allow explicit deref in method chains. e.g. `foo.deref().bar()` + && (is_ufcs || !is_in_method_chain(cx, expr)) => { let ty_changed_count = usize::from(!deref_method_same_type(expr_ty, typeck.expr_ty(sub_expr))); self.state = Some(( @@ -403,37 +404,37 @@ impl<'tcx> LateLintPass<'tcx> for Dereferencing<'tcx> { // If this trait impl is implemented on `&T`, then auto-borrowing won't work (impl_ty.is_ref() && implements_trait( - cx, - impl_ty, - trait_id, - &args[..cx.tcx.generics_of(trait_id).own_params.len() - 1], - )) + cx, + impl_ty, + trait_id, + &args[..cx.tcx.generics_of(trait_id).own_params.len() - 1], + )) // If there's an inherent method, or a method from another trait, // with the same name that's also implemented on this same type, // then removing the borrow might cause that method to be chosen // instead of the current one. || get_adt_inherent_method(cx, impl_ty, method_name).is_some() || cx.tcx.in_scope_traits(hir_id).is_some_and(|traits| { - traits - .iter() - .filter(|trait_| { - cx.tcx - .non_blanket_impls_for_ty(trait_.def_id, impl_ty) - .next() - .is_some() - || !cx - .tcx - .trait_impls_of(trait_.def_id) - .blanket_impls() - .is_empty() - }) - .any(|trait_| { - cx.tcx - .associated_items(trait_.def_id) - .filter_by_name_unhygienic(method_name) - .any(|item| item.tag() == AssocTag::Fn && item.def_id != fn_id) - }) - }) + traits + .iter() + .filter(|trait_| { + cx.tcx + .non_blanket_impls_for_ty(trait_.def_id, impl_ty) + .next() + .is_some() + || !cx + .tcx + .trait_impls_of(trait_.def_id) + .blanket_impls() + .is_empty() + }) + .any(|trait_| { + cx.tcx + .associated_items(trait_.def_id) + .filter_by_name_unhygienic(method_name) + .any(|item| item.tag() == AssocTag::Fn && item.def_id != fn_id) + }) + }) ) { false @@ -1025,9 +1026,8 @@ fn ty_contains_field(ty: Ty<'_>, name: Symbol) -> bool { } } -/// Returns `true` if the expression is in tail position of a branch whose pattern bindings do not -/// live past that branch. Replacing it with `&expr` would create a reference to a local that -/// doesn't live long enough. +// 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) { @@ -1041,7 +1041,7 @@ fn is_in_pattern_branch_tail(tcx: TyCtxt<'_>, mut id: HirId) -> bool { }, ExprKind::If(cond, then_expr, else_expr) => { if then_expr.hir_id == id { - if matches!(cond.kind, ExprKind::Let(_)) { + if contains_let_expr(cond) { return true; } // Bubble up for normal if branch @@ -1054,6 +1054,11 @@ fn is_in_pattern_branch_tail(tcx: TyCtxt<'_>, mut id: HirId) -> bool { } }, + // break x or break { x } + ExprKind::Break(_, Some(break_expr)) if break_expr.hir_id == id => { + return true; + }, + _ => return false, }, @@ -1062,6 +1067,20 @@ fn is_in_pattern_branch_tail(tcx: TyCtxt<'_>, mut id: HirId) -> bool { } } +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() @@ -1102,11 +1121,16 @@ 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}"))); diff --git a/tests/ui/ref_binding_to_reference.fixed b/tests/ui/ref_binding_to_reference.fixed index 147079de13fa..47efe8d9cb24 100644 --- a/tests/ui/ref_binding_to_reference.fixed +++ b/tests/ui/ref_binding_to_reference.fixed @@ -1,6 +1,6 @@ #![warn(clippy::ref_binding_to_reference)] -#![expect(clippy::explicit_auto_deref)] -#![allow(clippy::needless_borrowed_reference, clippy::collapsible_match)] +#![expect(clippy::collapsible_match, clippy::explicit_auto_deref, clippy::never_loop)] +#![allow(clippy::needless_borrowed_reference)] fn f1(_: &str) {} macro_rules! m2 { @@ -174,4 +174,49 @@ mod issue17370 { 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; + }; + } } diff --git a/tests/ui/ref_binding_to_reference.rs b/tests/ui/ref_binding_to_reference.rs index d4bc967aeb40..4b7222a692a2 100644 --- a/tests/ui/ref_binding_to_reference.rs +++ b/tests/ui/ref_binding_to_reference.rs @@ -1,6 +1,6 @@ #![warn(clippy::ref_binding_to_reference)] -#![expect(clippy::explicit_auto_deref)] -#![allow(clippy::needless_borrowed_reference, clippy::collapsible_match)] +#![expect(clippy::collapsible_match, clippy::explicit_auto_deref, clippy::never_loop)] +#![allow(clippy::needless_borrowed_reference)] fn f1(_: &str) {} macro_rules! m2 { @@ -174,4 +174,49 @@ mod issue17370 { 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; + }; + } }