From e33d87d4a0a188aa03d7d07700dbf296cf76581a Mon Sep 17 00:00:00 2001
From: Preston From <prestonfrom@gmail.com>
Date: Tue, 24 May 2022 01:06:33 -0600
Subject: [PATCH] When setting suggestion, add suggestion for MoveAndClone for
non-ref
When trying to set the current suggestion, if the type of the expression
is not a reference and it is not trivially pure clone copy, we should still
trigger and emit a lint message. Since this fix may require cloning an
expensive-to-clone type, do not attempt to offer a suggested fix.
This change means that matches generated from TryDesugar and AwaitDesugar
would normally trigger a lint, but they are out of scope for this lint,
so we will explicitly ignore matches with sources of TryDesugar or
AwaitDesugar.
changelog: Update for [`significant_drop_in_scrutinee`] to correctly
emit lint messages for cases where the type is not a reference and
not trivially pure clone copy.
---
.../src/significant_drop_in_scrutinee.rs | 107 ++++++++++--------
clippy_utils/src/lib.rs | 3 +-
tests/ui/significant_drop_in_scrutinee.rs | 39 +++++++
tests/ui/significant_drop_in_scrutinee.stderr | 60 ++++++----
4 files changed, 134 insertions(+), 75 deletions(-)
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<Vec<FoundSigDrop>> {
- 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<Mutex<FxHashMap<LocalDefId, Vec<Symbo
fn with_test_item_names<'tcx>(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<LocalDefId, Vec<Symbol>>> = 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<u64, ParseIntError> {
+ // 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::<u64>()?;
+ println!("{}", result);
+ rwlock.write().unwrap().push('2');
+ Ok(result)
+}
+
+struct ResultReturner {
+ s: String,
+}
+
+impl ResultReturner {
+ fn to_number(&self) -> Result<i64, ParseIntError> {
+ self.s.parse::<i64>()
+ }
+}
+
+fn should_trigger_lint_for_non_ref_move_and_clone_suggestion() {
+ let rwlock = RwLock::<ResultReturner>::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::<Vec<String>>::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