mirror of
https://github.com/rust-lang/rust-clippy
synced 2024-11-23 21:23:56 +00:00
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.
This commit is contained in:
Preston From
parent
3cc50a4a08
commit
e33d87d4a0
4 changed files with 134 additions and 75 deletions
|
|
@ -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;
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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();
|
||||
|
|
|
|||
|
|
@ -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() {}
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
||||
|
|
|
|||
Loading…
Reference in a new issue