diff --git a/clippy_lints/src/significant_drop_in_scrutinee.rs b/clippy_lints/src/significant_drop_in_scrutinee.rs index 424b361a905..6b45b6d4452 100644 --- a/clippy_lints/src/significant_drop_in_scrutinee.rs +++ b/clippy_lints/src/significant_drop_in_scrutinee.rs @@ -4,7 +4,7 @@ use clippy_utils::get_attr; use clippy_utils::source::{indent_of, snippet}; use rustc_errors::{Applicability, Diagnostic}; use rustc_hir::intravisit::{walk_expr, Visitor}; -use rustc_hir::{Expr, ExprKind}; +use rustc_hir::{Expr, ExprKind, MatchSource}; use rustc_lint::{LateContext, LateLintPass, LintContext}; use rustc_middle::ty::subst::GenericArgKind; use rustc_middle::ty::{Ty, TypeAndMut}; @@ -154,9 +154,17 @@ fn has_significant_drop_in_scrutinee<'tcx, 'a>( cx: &'a LateContext<'tcx>, expr: &'tcx Expr<'tcx>, ) -> Option> { - let mut helper = SigDropHelper::new(cx); match expr.kind { - ExprKind::Match(match_expr, _, _) => helper.find_sig_drop(match_expr), + ExprKind::Match(match_expr, _, source) => { + match source { + MatchSource::Normal | MatchSource::ForLoopDesugar => { + let mut helper = SigDropHelper::new(cx); + helper.find_sig_drop(match_expr) + }, + // MatchSource of TryDesugar or AwaitDesugar is out of scope for this lint + MatchSource::TryDesugar | MatchSource::AwaitDesugar => None, + } + }, _ => None, } } @@ -213,6 +221,19 @@ impl<'a, 'tcx> SigDropHelper<'a, 'tcx> { self.sig_drop_spans.take() } + fn replace_current_sig_drop( + &mut self, + found_span: Span, + is_unit_return_val: bool, + lint_suggestion: LintSuggestion, + ) { + self.current_sig_drop.replace(FoundSigDrop { + found_span, + is_unit_return_val, + lint_suggestion, + }); + } + /// This will try to set the current suggestion (so it can be moved into the suggestions vec /// later). If `allow_move_and_clone` is false, the suggestion *won't* be set -- this gives us /// an opportunity to look for another type in the chain that will be trivially copyable. @@ -229,25 +250,15 @@ impl<'a, 'tcx> SigDropHelper<'a, 'tcx> { // but let's avoid any chance of an ICE if let Some(TypeAndMut { ty, .. }) = ty.builtin_deref(true) { if ty.is_trivially_pure_clone_copy() { - self.current_sig_drop.replace(FoundSigDrop { - found_span: expr.span, - is_unit_return_val: false, - lint_suggestion: LintSuggestion::MoveAndDerefToCopy, - }); + self.replace_current_sig_drop(expr.span, false, LintSuggestion::MoveAndDerefToCopy); } else if allow_move_and_clone { - self.current_sig_drop.replace(FoundSigDrop { - found_span: expr.span, - is_unit_return_val: false, - lint_suggestion: LintSuggestion::MoveAndClone, - }); + self.replace_current_sig_drop(expr.span, false, LintSuggestion::MoveAndClone); } } } else if ty.is_trivially_pure_clone_copy() { - self.current_sig_drop.replace(FoundSigDrop { - found_span: expr.span, - is_unit_return_val: false, - lint_suggestion: LintSuggestion::MoveOnly, - }); + self.replace_current_sig_drop(expr.span, false, LintSuggestion::MoveOnly); + } else if allow_move_and_clone { + self.replace_current_sig_drop(expr.span, false, LintSuggestion::MoveAndClone); } } @@ -279,11 +290,7 @@ impl<'a, 'tcx> SigDropHelper<'a, 'tcx> { // If either side had a significant drop, suggest moving the entire scrutinee to avoid // unnecessary copies and to simplify cases where both sides have significant drops. if self.has_significant_drop { - self.current_sig_drop.replace(FoundSigDrop { - found_span: span, - is_unit_return_val, - lint_suggestion: LintSuggestion::MoveOnly, - }); + self.replace_current_sig_drop(span, is_unit_return_val, LintSuggestion::MoveOnly); } self.special_handling_for_binary_op = false; @@ -363,34 +370,34 @@ impl<'a, 'tcx> Visitor<'tcx> for SigDropHelper<'a, 'tcx> { } } ExprKind::Box(..) | - ExprKind::Array(..) | - ExprKind::Call(..) | - ExprKind::Unary(..) | - ExprKind::If(..) | - ExprKind::Match(..) | - ExprKind::Field(..) | - ExprKind::Index(..) | - ExprKind::Ret(..) | - ExprKind::Repeat(..) | - ExprKind::Yield(..) | - ExprKind::MethodCall(..) => walk_expr(self, ex), + ExprKind::Array(..) | + ExprKind::Call(..) | + ExprKind::Unary(..) | + ExprKind::If(..) | + ExprKind::Match(..) | + ExprKind::Field(..) | + ExprKind::Index(..) | + ExprKind::Ret(..) | + ExprKind::Repeat(..) | + ExprKind::Yield(..) | + ExprKind::MethodCall(..) => walk_expr(self, ex), ExprKind::AddrOf(_, _, _) | - ExprKind::Block(_, _) | - ExprKind::Break(_, _) | - ExprKind::Cast(_, _) | - // Don't want to check the closure itself, only invocation, which is covered by MethodCall - ExprKind::Closure(_, _, _, _, _) | - ExprKind::ConstBlock(_) | - ExprKind::Continue(_) | - ExprKind::DropTemps(_) | - ExprKind::Err | - ExprKind::InlineAsm(_) | - ExprKind::Let(_) | - ExprKind::Lit(_) | - ExprKind::Loop(_, _, _, _) | - ExprKind::Path(_) | - ExprKind::Struct(_, _, _) | - ExprKind::Type(_, _) => { + ExprKind::Block(_, _) | + ExprKind::Break(_, _) | + ExprKind::Cast(_, _) | + // Don't want to check the closure itself, only invocation, which is covered by MethodCall + ExprKind::Closure(_, _, _, _, _) | + ExprKind::ConstBlock(_) | + ExprKind::Continue(_) | + ExprKind::DropTemps(_) | + ExprKind::Err | + ExprKind::InlineAsm(_) | + ExprKind::Let(_) | + ExprKind::Lit(_) | + ExprKind::Loop(_, _, _, _) | + ExprKind::Path(_) | + ExprKind::Struct(_, _, _) | + ExprKind::Type(_, _) => { return; } } diff --git a/clippy_utils/src/lib.rs b/clippy_utils/src/lib.rs index adb37cc9d75..0d61231ada7 100644 --- a/clippy_utils/src/lib.rs +++ b/clippy_utils/src/lib.rs @@ -2073,7 +2073,8 @@ static TEST_ITEM_NAMES_CACHE: SyncOnceCell(tcx: TyCtxt<'tcx>, module: LocalDefId, f: impl Fn(&[Symbol]) -> bool) -> bool { let cache = TEST_ITEM_NAMES_CACHE.get_or_init(|| Mutex::new(FxHashMap::default())); let mut map: MutexGuard<'_, FxHashMap>> = cache.lock().unwrap(); - match map.entry(module) { + let value = map.entry(module); + match value { Entry::Occupied(entry) => f(entry.get()), Entry::Vacant(entry) => { let mut names = Vec::new(); diff --git a/tests/ui/significant_drop_in_scrutinee.rs b/tests/ui/significant_drop_in_scrutinee.rs index f83a6dd0eb2..4347610f393 100644 --- a/tests/ui/significant_drop_in_scrutinee.rs +++ b/tests/ui/significant_drop_in_scrutinee.rs @@ -7,8 +7,10 @@ #![allow(unused_assignments)] #![allow(dead_code)] +use std::num::ParseIntError; use std::ops::Deref; use std::sync::atomic::{AtomicU64, Ordering}; +use std::sync::RwLock; use std::sync::{Mutex, MutexGuard}; struct State {} @@ -552,4 +554,41 @@ fn should_not_cause_stack_overflow() { } } +fn should_not_produce_lint_for_try_desugar() -> Result { + // TryDesugar (i.e. using `?` for a Result type) will turn into a match but is out of scope + // for this lint + let rwlock = RwLock::new("1".to_string()); + let result = rwlock.read().unwrap().parse::()?; + println!("{}", result); + rwlock.write().unwrap().push('2'); + Ok(result) +} + +struct ResultReturner { + s: String, +} + +impl ResultReturner { + fn to_number(&self) -> Result { + self.s.parse::() + } +} + +fn should_trigger_lint_for_non_ref_move_and_clone_suggestion() { + let rwlock = RwLock::::new(ResultReturner { s: "1".to_string() }); + match rwlock.read().unwrap().to_number() { + Ok(n) => println!("Converted to number: {}", n), + Err(e) => println!("Could not convert {} to number", e), + }; +} + +fn should_trigger_lint_for_read_write_lock_for_loop() { + // For-in loops desugar to match expressions and are prone to the type of deadlock this lint is + // designed to look for. + let rwlock = RwLock::>::new(vec!["1".to_string()]); + for s in rwlock.read().unwrap().iter() { + println!("{}", s); + } +} + fn main() {} diff --git a/tests/ui/significant_drop_in_scrutinee.stderr b/tests/ui/significant_drop_in_scrutinee.stderr index af160564985..77942325244 100644 --- a/tests/ui/significant_drop_in_scrutinee.stderr +++ b/tests/ui/significant_drop_in_scrutinee.stderr @@ -1,5 +1,5 @@ error: temporary with significant drop in match scrutinee - --> $DIR/significant_drop_in_scrutinee.rs:57:11 + --> $DIR/significant_drop_in_scrutinee.rs:59:11 | LL | match mutex.lock().unwrap().foo() { | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ @@ -12,7 +12,7 @@ LL ~ match value { | error: temporary with significant drop in match scrutinee - --> $DIR/significant_drop_in_scrutinee.rs:130:11 + --> $DIR/significant_drop_in_scrutinee.rs:132:11 | LL | match s.lock_m().get_the_value() { | ^^^^^^^^^^^^^^^^^^^^^^^^^^ @@ -24,7 +24,7 @@ LL ~ match value { | error: temporary with significant drop in match scrutinee - --> $DIR/significant_drop_in_scrutinee.rs:151:11 + --> $DIR/significant_drop_in_scrutinee.rs:153:11 | LL | match s.lock_m_m().get_the_value() { | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ @@ -36,7 +36,7 @@ LL ~ match value { | error: temporary with significant drop in match scrutinee - --> $DIR/significant_drop_in_scrutinee.rs:199:11 + --> $DIR/significant_drop_in_scrutinee.rs:201:11 | LL | match counter.temp_increment().len() { | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ @@ -48,7 +48,7 @@ LL ~ match value { | error: temporary with significant drop in match scrutinee - --> $DIR/significant_drop_in_scrutinee.rs:222:16 + --> $DIR/significant_drop_in_scrutinee.rs:224:16 | LL | match (mutex1.lock().unwrap().s.len(), true) { | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ @@ -60,7 +60,7 @@ LL ~ match (value, true) { | error: temporary with significant drop in match scrutinee - --> $DIR/significant_drop_in_scrutinee.rs:231:22 + --> $DIR/significant_drop_in_scrutinee.rs:233:22 | LL | match (true, mutex1.lock().unwrap().s.len(), true) { | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ @@ -72,7 +72,7 @@ LL ~ match (true, value, true) { | error: temporary with significant drop in match scrutinee - --> $DIR/significant_drop_in_scrutinee.rs:241:16 + --> $DIR/significant_drop_in_scrutinee.rs:243:16 | LL | match (mutex1.lock().unwrap().s.len(), true, mutex2.lock().unwrap().s.len()) { | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ @@ -84,7 +84,7 @@ LL ~ match (value, true, mutex2.lock().unwrap().s.len()) { | error: temporary with significant drop in match scrutinee - --> $DIR/significant_drop_in_scrutinee.rs:241:54 + --> $DIR/significant_drop_in_scrutinee.rs:243:54 | LL | match (mutex1.lock().unwrap().s.len(), true, mutex2.lock().unwrap().s.len()) { | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ @@ -96,19 +96,19 @@ LL ~ match (mutex1.lock().unwrap().s.len(), true, value) { | error: temporary with significant drop in match scrutinee - --> $DIR/significant_drop_in_scrutinee.rs:252:15 + --> $DIR/significant_drop_in_scrutinee.rs:254:15 | LL | match mutex3.lock().unwrap().s.as_str() { | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ error: temporary with significant drop in match scrutinee - --> $DIR/significant_drop_in_scrutinee.rs:262:22 + --> $DIR/significant_drop_in_scrutinee.rs:264:22 | LL | match (true, mutex3.lock().unwrap().s.as_str()) { | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ error: temporary with significant drop in match scrutinee - --> $DIR/significant_drop_in_scrutinee.rs:281:11 + --> $DIR/significant_drop_in_scrutinee.rs:283:11 | LL | match mutex.lock().unwrap().s.len() > 1 { | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ @@ -120,7 +120,7 @@ LL ~ match value { | error: temporary with significant drop in match scrutinee - --> $DIR/significant_drop_in_scrutinee.rs:288:11 + --> $DIR/significant_drop_in_scrutinee.rs:290:11 | LL | match 1 < mutex.lock().unwrap().s.len() { | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ @@ -132,7 +132,7 @@ LL ~ match value { | error: temporary with significant drop in match scrutinee - --> $DIR/significant_drop_in_scrutinee.rs:306:11 + --> $DIR/significant_drop_in_scrutinee.rs:308:11 | LL | match mutex1.lock().unwrap().s.len() < mutex2.lock().unwrap().s.len() { | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ @@ -144,7 +144,7 @@ LL ~ match value { | error: temporary with significant drop in match scrutinee - --> $DIR/significant_drop_in_scrutinee.rs:317:11 + --> $DIR/significant_drop_in_scrutinee.rs:319:11 | LL | match mutex1.lock().unwrap().s.len() >= mutex2.lock().unwrap().s.len() { | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ @@ -156,7 +156,7 @@ LL ~ match value { | error: temporary with significant drop in match scrutinee - --> $DIR/significant_drop_in_scrutinee.rs:352:11 + --> $DIR/significant_drop_in_scrutinee.rs:354:11 | LL | match get_mutex_guard().s.len() > 1 { | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ @@ -168,7 +168,7 @@ LL ~ match value { | error: temporary with significant drop in match scrutinee - --> $DIR/significant_drop_in_scrutinee.rs:369:11 + --> $DIR/significant_drop_in_scrutinee.rs:371:11 | LL | match match i { | ___________^ @@ -191,7 +191,7 @@ LL + .len() ... error: temporary with significant drop in match scrutinee - --> $DIR/significant_drop_in_scrutinee.rs:395:11 + --> $DIR/significant_drop_in_scrutinee.rs:397:11 | LL | match if i > 1 { | ___________^ @@ -214,7 +214,7 @@ LL + .s ... error: temporary with significant drop in match scrutinee - --> $DIR/significant_drop_in_scrutinee.rs:449:11 + --> $DIR/significant_drop_in_scrutinee.rs:451:11 | LL | match s.lock().deref().deref() { | ^^^^^^^^^^^^^^^^^^^^^^^^ @@ -226,13 +226,13 @@ LL ~ match value { | error: temporary with significant drop in match scrutinee - --> $DIR/significant_drop_in_scrutinee.rs:477:11 + --> $DIR/significant_drop_in_scrutinee.rs:479:11 | LL | match s.lock().deref().deref() { | ^^^^^^^^^^^^^^^^^^^^^^^^ error: temporary with significant drop in match scrutinee - --> $DIR/significant_drop_in_scrutinee.rs:496:11 + --> $DIR/significant_drop_in_scrutinee.rs:498:11 | LL | match mutex.lock().unwrap().i = i { | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ @@ -244,7 +244,7 @@ LL ~ match () { | error: temporary with significant drop in match scrutinee - --> $DIR/significant_drop_in_scrutinee.rs:502:11 + --> $DIR/significant_drop_in_scrutinee.rs:504:11 | LL | match i = mutex.lock().unwrap().i { | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ @@ -256,7 +256,7 @@ LL ~ match () { | error: temporary with significant drop in match scrutinee - --> $DIR/significant_drop_in_scrutinee.rs:508:11 + --> $DIR/significant_drop_in_scrutinee.rs:510:11 | LL | match mutex.lock().unwrap().i += 1 { | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ @@ -268,7 +268,7 @@ LL ~ match () { | error: temporary with significant drop in match scrutinee - --> $DIR/significant_drop_in_scrutinee.rs:514:11 + --> $DIR/significant_drop_in_scrutinee.rs:516:11 | LL | match i += mutex.lock().unwrap().i { | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ @@ -279,5 +279,17 @@ LL ~ i += mutex.lock().unwrap().i; LL ~ match () { | -error: aborting due to 23 previous errors +error: temporary with significant drop in match scrutinee + --> $DIR/significant_drop_in_scrutinee.rs:579:11 + | +LL | match rwlock.read().unwrap().to_number() { + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +error: temporary with significant drop in match scrutinee + --> $DIR/significant_drop_in_scrutinee.rs:589:14 + | +LL | for s in rwlock.read().unwrap().iter() { + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +error: aborting due to 25 previous errors