From 9edf1e76fee1b8a79acdfd16cfa9dc06925e45c6 Mon Sep 17 00:00:00 2001 From: lapla-cogito Date: Fri, 24 Jan 2025 08:43:27 +0900 Subject: [PATCH] autofix for `significant_drop_in_scrutinee` --- .../matches/significant_drop_in_scrutinee.rs | 28 +- tests/ui/significant_drop_in_scrutinee.fixed | 885 ++++++++++++++++++ tests/ui/significant_drop_in_scrutinee.rs | 12 - tests/ui/significant_drop_in_scrutinee.stderr | 97 +- .../ui/significant_drop_in_scrutinee_nofix.rs | 14 + ...significant_drop_in_scrutinee_nofix.stderr | 15 + 6 files changed, 979 insertions(+), 72 deletions(-) create mode 100644 tests/ui/significant_drop_in_scrutinee.fixed create mode 100644 tests/ui/significant_drop_in_scrutinee_nofix.rs create mode 100644 tests/ui/significant_drop_in_scrutinee_nofix.stderr diff --git a/clippy_lints/src/matches/significant_drop_in_scrutinee.rs b/clippy_lints/src/matches/significant_drop_in_scrutinee.rs index 35f2e780d2e2..e6d8268ba81e 100644 --- a/clippy_lints/src/matches/significant_drop_in_scrutinee.rs +++ b/clippy_lints/src/matches/significant_drop_in_scrutinee.rs @@ -104,10 +104,10 @@ fn check<'tcx>( let mut helper = SigDropHelper::new(cx); let suggestions = helper.find_sig_drop(scrutinee); - for found in suggestions { + for (i, found) in suggestions.iter().enumerate() { span_lint_and_then(cx, SIGNIFICANT_DROP_IN_SCRUTINEE, found.found_span, message, |diag| { match sugg { - Suggestion::Emit => set_suggestion(diag, cx, expr, found), + Suggestion::Emit => set_suggestion(diag, cx, expr, *found, suggestions.len(), i), Suggestion::DontEmit => (), } @@ -121,15 +121,27 @@ fn check<'tcx>( } } -fn set_suggestion<'tcx>(diag: &mut Diag<'_, ()>, cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>, found: FoundSigDrop) { +fn set_suggestion<'tcx>( + diag: &mut Diag<'_, ()>, + cx: &LateContext<'tcx>, + expr: &'tcx Expr<'tcx>, + found: FoundSigDrop, + total_sugg: usize, + curr_sugg_idx: usize, +) { let original = snippet(cx, found.found_span, ".."); let trailing_indent = " ".repeat(indent_of(cx, found.found_span).unwrap_or(0)); let replacement = { let (def_part, deref_part) = if found.is_unit_return_val { - ("", String::new()) + (String::new(), String::new()) + } else if total_sugg == 1 { + ("let value = ".to_string(), "*".repeat(found.peel_ref_times)) } else { - ("let value = ", "*".repeat(found.peel_ref_times)) + ( + format!("let value{} = ", curr_sugg_idx + 1), + "*".repeat(found.peel_ref_times), + ) }; format!("{def_part}{deref_part}{original};\n{trailing_indent}") }; @@ -143,7 +155,11 @@ fn set_suggestion<'tcx>(diag: &mut Diag<'_, ()>, cx: &LateContext<'tcx>, expr: & let scrutinee_replacement = if found.is_unit_return_val { "()".to_owned() } else if found.peel_ref_times == 0 { - "value".to_owned() + if total_sugg == 1 { + "value".to_owned() + } else { + format!("value{}", curr_sugg_idx + 1) + } } else { let ref_part = "&".repeat(found.peel_ref_times); format!("({ref_part}value)") diff --git a/tests/ui/significant_drop_in_scrutinee.fixed b/tests/ui/significant_drop_in_scrutinee.fixed new file mode 100644 index 000000000000..e29a65c62f85 --- /dev/null +++ b/tests/ui/significant_drop_in_scrutinee.fixed @@ -0,0 +1,885 @@ +#![warn(clippy::significant_drop_in_scrutinee)] +#![allow(dead_code, unused_assignments)] +#![allow( + clippy::match_single_binding, + clippy::single_match, + clippy::uninlined_format_args, + clippy::needless_lifetimes +)] + +use std::num::ParseIntError; +use std::ops::Deref; +use std::sync::atomic::{AtomicU64, Ordering}; +use std::sync::{Mutex, MutexGuard, RwLock}; + +struct State {} + +impl State { + fn foo(&self) -> bool { + true + } + + fn bar(&self) {} +} + +fn should_not_trigger_lint_with_mutex_guard_outside_match() { + let mutex = Mutex::new(State {}); + + // Should not trigger lint because the temporary should drop at the `;` on line before the match + let is_foo = mutex.lock().unwrap().foo(); + match is_foo { + true => { + mutex.lock().unwrap().bar(); + }, + false => {}, + }; +} + +fn should_not_trigger_lint_with_mutex_guard_when_taking_ownership_in_match() { + let mutex = Mutex::new(State {}); + + // Should not trigger lint because the scrutinee is explicitly returning the MutexGuard, + // so its lifetime should not be surprising. + match mutex.lock() { + Ok(guard) => { + guard.foo(); + mutex.lock().unwrap().bar(); + }, + _ => {}, + }; +} + +fn should_trigger_lint_with_mutex_guard_in_match_scrutinee() { + let mutex = Mutex::new(State {}); + + // Should trigger lint because the lifetime of the temporary MutexGuard is surprising because it + // is preserved until the end of the match, but there is no clear indication that this is the + // case. + let value = mutex.lock().unwrap().foo(); + match value { + //~^ ERROR: temporary with significant `Drop` in `match` scrutinee will live until the + //~| NOTE: this might lead to deadlocks or other unexpected behavior + true => { + mutex.lock().unwrap().bar(); + }, + false => {}, + }; +} + +fn should_not_trigger_lint_with_mutex_guard_in_match_scrutinee_when_lint_allowed() { + let mutex = Mutex::new(State {}); + + // Lint should not be triggered because it is "allowed" below. + #[allow(clippy::significant_drop_in_scrutinee)] + match mutex.lock().unwrap().foo() { + true => { + mutex.lock().unwrap().bar(); + }, + false => {}, + }; +} + +fn should_not_trigger_lint_for_insignificant_drop() { + // Should not trigger lint because there are no temporaries whose drops have a significant + // side effect. + match 1u64.to_string().is_empty() { + true => { + println!("It was empty") + }, + false => { + println!("It was not empty") + }, + } +} + +struct StateWithMutex { + m: Mutex, +} + +struct MutexGuardWrapper<'a> { + mg: MutexGuard<'a, u64>, +} + +impl<'a> MutexGuardWrapper<'a> { + fn get_the_value(&self) -> u64 { + *self.mg.deref() + } +} + +struct MutexGuardWrapperWrapper<'a> { + mg: MutexGuardWrapper<'a>, +} + +impl<'a> MutexGuardWrapperWrapper<'a> { + fn get_the_value(&self) -> u64 { + *self.mg.mg.deref() + } +} + +impl StateWithMutex { + fn lock_m(&self) -> MutexGuardWrapper<'_> { + MutexGuardWrapper { + mg: self.m.lock().unwrap(), + } + } + + fn lock_m_m(&self) -> MutexGuardWrapperWrapper<'_> { + MutexGuardWrapperWrapper { + mg: MutexGuardWrapper { + mg: self.m.lock().unwrap(), + }, + } + } + + fn foo(&self) -> bool { + true + } + + fn bar(&self) {} +} + +fn should_trigger_lint_with_wrapped_mutex() { + let s = StateWithMutex { m: Mutex::new(1) }; + + // Should trigger lint because a temporary contains a type with a significant drop and its + // lifetime is not obvious. Additionally, it is not obvious from looking at the scrutinee that + // the temporary contains such a type, making it potentially even more surprising. + let value = s.lock_m().get_the_value(); + match value { + //~^ ERROR: temporary with significant `Drop` in `match` scrutinee will live until the + //~| NOTE: this might lead to deadlocks or other unexpected behavior + 1 => { + println!("Got 1. Is it still 1?"); + println!("{}", s.lock_m().get_the_value()); + }, + 2 => { + println!("Got 2. Is it still 2?"); + println!("{}", s.lock_m().get_the_value()); + }, + _ => {}, + } + println!("All done!"); +} + +fn should_trigger_lint_with_double_wrapped_mutex() { + let s = StateWithMutex { m: Mutex::new(1) }; + + // Should trigger lint because a temporary contains a type which further contains a type with a + // significant drop and its lifetime is not obvious. Additionally, it is not obvious from + // looking at the scrutinee that the temporary contains such a type, making it potentially even + // more surprising. + let value = s.lock_m_m().get_the_value(); + match value { + //~^ ERROR: temporary with significant `Drop` in `match` scrutinee will live until the + //~| NOTE: this might lead to deadlocks or other unexpected behavior + 1 => { + println!("Got 1. Is it still 1?"); + println!("{}", s.lock_m().get_the_value()); + }, + 2 => { + println!("Got 2. Is it still 2?"); + println!("{}", s.lock_m().get_the_value()); + }, + _ => {}, + } + println!("All done!"); +} + +struct Counter { + i: AtomicU64, +} + +#[clippy::has_significant_drop] +struct CounterWrapper<'a> { + counter: &'a Counter, +} + +impl<'a> CounterWrapper<'a> { + fn new(counter: &Counter) -> CounterWrapper { + counter.i.fetch_add(1, Ordering::Relaxed); + CounterWrapper { counter } + } +} + +impl<'a> Drop for CounterWrapper<'a> { + fn drop(&mut self) { + self.counter.i.fetch_sub(1, Ordering::Relaxed); + } +} + +impl Counter { + fn temp_increment(&self) -> Vec { + vec![CounterWrapper::new(self), CounterWrapper::new(self)] + } +} + +fn should_trigger_lint_for_vec() { + let counter = Counter { i: AtomicU64::new(0) }; + + // Should trigger lint because the temporary in the scrutinee returns a collection of types + // which have significant drops. The types with significant drops are also non-obvious when + // reading the expression in the scrutinee. + let value = counter.temp_increment().len(); + match value { + //~^ ERROR: temporary with significant `Drop` in `match` scrutinee will live until the + //~| NOTE: this might lead to deadlocks or other unexpected behavior + 2 => { + let current_count = counter.i.load(Ordering::Relaxed); + println!("Current count {}", current_count); + assert_eq!(current_count, 0); + }, + 1 => {}, + 3 => {}, + _ => {}, + }; +} + +struct StateWithField { + s: String, +} + +// Should trigger lint only on the type in the tuple which is created using a temporary +// with a significant drop. Additionally, this test ensures that the format of the tuple +// is preserved correctly in the suggestion. +fn should_trigger_lint_for_tuple_in_scrutinee() { + let mutex1 = Mutex::new(StateWithField { s: "one".to_owned() }); + + { + let value = mutex1.lock().unwrap().s.len(); + match (value, true) { + //~^ ERROR: temporary with significant `Drop` in `match` scrutinee will live until + //~| NOTE: this might lead to deadlocks or other unexpected behavior + (3, _) => { + println!("started"); + mutex1.lock().unwrap().s.len(); + println!("done"); + }, + (_, _) => {}, + }; + + let value = mutex1.lock().unwrap().s.len(); + match (true, value, true) { + //~^ ERROR: temporary with significant `Drop` in `match` scrutinee will live until + //~| NOTE: this might lead to deadlocks or other unexpected behavior + (_, 3, _) => { + println!("started"); + mutex1.lock().unwrap().s.len(); + println!("done"); + }, + (_, _, _) => {}, + }; + + let mutex2 = Mutex::new(StateWithField { s: "two".to_owned() }); + let value2 = mutex2.lock().unwrap().s.len(); + let value1 = mutex1.lock().unwrap().s.len(); + match (value1, true, value2) { + //~^ ERROR: temporary with significant `Drop` in `match` scrutinee will live until + //~| NOTE: this might lead to deadlocks or other unexpected behavior + //~| ERROR: temporary with significant `Drop` in `match` scrutinee will live until + //~| NOTE: this might lead to deadlocks or other unexpected behavior + (3, _, 3) => { + println!("started"); + mutex1.lock().unwrap().s.len(); + mutex2.lock().unwrap().s.len(); + println!("done"); + }, + (_, _, _) => {}, + }; + } +} + +// Should not trigger lint since `String::as_str` returns a reference (i.e., `&str`) +// to the locked data (i.e., the `String`) and it is not surprising that matching such +// a reference needs to keep the data locked until the end of the match block. +fn should_not_trigger_lint_for_string_as_str() { + let mutex1 = Mutex::new(StateWithField { s: "one".to_owned() }); + + { + let mutex2 = Mutex::new(StateWithField { s: "two".to_owned() }); + let mutex3 = Mutex::new(StateWithField { s: "three".to_owned() }); + + match mutex3.lock().unwrap().s.as_str() { + "three" => { + println!("started"); + mutex1.lock().unwrap().s.len(); + mutex2.lock().unwrap().s.len(); + println!("done"); + }, + _ => {}, + }; + + match (true, mutex3.lock().unwrap().s.as_str()) { + (_, "three") => { + println!("started"); + mutex1.lock().unwrap().s.len(); + mutex2.lock().unwrap().s.len(); + println!("done"); + }, + (_, _) => {}, + }; + } +} + +// Should trigger lint when either side of a binary operation creates a temporary with a +// significant drop. +// To avoid potential unnecessary copies or creating references that would trigger the significant +// drop problem, the lint recommends moving the entire binary operation. +fn should_trigger_lint_for_accessing_field_in_mutex_in_one_side_of_binary_op() { + let mutex = Mutex::new(StateWithField { s: "state".to_owned() }); + + let value = mutex.lock().unwrap().s.len(); + match value > 1 { + //~^ ERROR: temporary with significant `Drop` in `match` scrutinee will live until the + //~| NOTE: this might lead to deadlocks or other unexpected behavior + true => { + mutex.lock().unwrap().s.len(); + }, + false => {}, + }; + + let value = mutex.lock().unwrap().s.len(); + match 1 < value { + //~^ ERROR: temporary with significant `Drop` in `match` scrutinee will live until the + //~| NOTE: this might lead to deadlocks or other unexpected behavior + true => { + mutex.lock().unwrap().s.len(); + }, + false => {}, + }; +} + +// Should trigger lint when both sides of a binary operation creates a temporary with a +// significant drop. +// To avoid potential unnecessary copies or creating references that would trigger the significant +// drop problem, the lint recommends moving the entire binary operation. +fn should_trigger_lint_for_accessing_fields_in_mutex_in_both_sides_of_binary_op() { + let mutex1 = Mutex::new(StateWithField { s: "state".to_owned() }); + let mutex2 = Mutex::new(StateWithField { + s: "statewithfield".to_owned(), + }); + + let value2 = mutex2.lock().unwrap().s.len(); + let value1 = mutex1.lock().unwrap().s.len(); + match value1 < value2 { + //~^ ERROR: temporary with significant `Drop` in `match` scrutinee will live until the + //~| NOTE: this might lead to deadlocks or other unexpected behavior + true => { + println!( + "{} < {}", + mutex1.lock().unwrap().s.len(), + mutex2.lock().unwrap().s.len() + ); + }, + false => {}, + }; + + let value2 = mutex2.lock().unwrap().s.len(); + let value1 = mutex1.lock().unwrap().s.len(); + match value1 >= value2 { + //~^ ERROR: temporary with significant `Drop` in `match` scrutinee will live until the + //~| NOTE: this might lead to deadlocks or other unexpected behavior + true => { + println!( + "{} >= {}", + mutex1.lock().unwrap().s.len(), + mutex2.lock().unwrap().s.len() + ); + }, + false => {}, + }; +} + +fn should_not_trigger_lint_for_closure_in_scrutinee() { + let mutex1 = Mutex::new(StateWithField { s: "one".to_owned() }); + + let get_mutex_guard = || mutex1.lock().unwrap().s.len(); + + // Should not trigger lint because the temporary with a significant drop will be dropped + // at the end of the closure, so the MutexGuard will be unlocked and not have a potentially + // surprising lifetime. + match get_mutex_guard() > 1 { + true => { + mutex1.lock().unwrap().s.len(); + }, + false => {}, + }; +} + +fn should_trigger_lint_for_return_from_closure_in_scrutinee() { + let mutex1 = Mutex::new(StateWithField { s: "one".to_owned() }); + + let get_mutex_guard = || mutex1.lock().unwrap(); + + // Should trigger lint because the temporary with a significant drop is returned from the + // closure but not used directly in any match arms, so it has a potentially surprising lifetime. + let value = get_mutex_guard().s.len(); + match value > 1 { + //~^ ERROR: temporary with significant `Drop` in `match` scrutinee will live until the + //~| NOTE: this might lead to deadlocks or other unexpected behavior + true => { + mutex1.lock().unwrap().s.len(); + }, + false => {}, + }; +} + +fn should_trigger_lint_for_return_from_match_in_scrutinee() { + let mutex1 = Mutex::new(StateWithField { s: "one".to_owned() }); + let mutex2 = Mutex::new(StateWithField { s: "two".to_owned() }); + + let i = 100; + + // Should trigger lint because the nested match within the scrutinee returns a temporary with a + // significant drop is but not used directly in any match arms, so it has a potentially + // surprising lifetime. + let value = match i { + //~^ ERROR: temporary with significant `Drop` in `match` scrutinee will live until the + //~| NOTE: this might lead to deadlocks or other unexpected behavior + 100 => mutex1.lock().unwrap(), + _ => mutex2.lock().unwrap(), + } + .s + .len(); + match value + > 1 + { + true => { + mutex1.lock().unwrap().s.len(); + }, + false => { + println!("nothing to do here"); + }, + }; +} + +fn should_trigger_lint_for_return_from_if_in_scrutinee() { + let mutex1 = Mutex::new(StateWithField { s: "one".to_owned() }); + let mutex2 = Mutex::new(StateWithField { s: "two".to_owned() }); + + let i = 100; + + // Should trigger lint because the nested if-expression within the scrutinee returns a temporary + // with a significant drop is but not used directly in any match arms, so it has a potentially + // surprising lifetime. + let value = if i > 1 { + //~^ ERROR: temporary with significant `Drop` in `match` scrutinee will live until the + //~| NOTE: this might lead to deadlocks or other unexpected behavior + mutex1.lock().unwrap() + } else { + mutex2.lock().unwrap() + } + .s + .len(); + match value + > 1 + { + true => { + mutex1.lock().unwrap().s.len(); + }, + false => {}, + }; +} + +fn should_not_trigger_lint_for_if_in_scrutinee() { + let mutex = Mutex::new(StateWithField { s: "state".to_owned() }); + + let i = 100; + + // Should not trigger the lint because the temporary with a significant drop *is* dropped within + // the body of the if-expression nested within the match scrutinee, and therefore does not have + // a potentially surprising lifetime. + match if i > 1 { + mutex.lock().unwrap().s.len() > 1 + } else { + false + } { + true => { + mutex.lock().unwrap().s.len(); + }, + false => {}, + }; +} + +struct StateWithBoxedMutexGuard { + u: Mutex, +} + +impl StateWithBoxedMutexGuard { + fn new() -> StateWithBoxedMutexGuard { + StateWithBoxedMutexGuard { u: Mutex::new(42) } + } + fn lock(&self) -> Box> { + Box::new(self.u.lock().unwrap()) + } +} + +fn should_trigger_lint_for_boxed_mutex_guard() { + let s = StateWithBoxedMutexGuard::new(); + + // Should trigger lint because a temporary Box holding a type with a significant drop in a match + // scrutinee may have a potentially surprising lifetime. + let value = *s.lock().deref().deref(); + match (&value) { + //~^ ERROR: temporary with significant `Drop` in `match` scrutinee will live until the + //~| NOTE: this might lead to deadlocks or other unexpected behavior + 0 | 1 => println!("Value was less than 2"), + _ => println!("Value is {}", s.lock().deref()), + }; +} + +struct StateStringWithBoxedMutexGuard { + s: Mutex, +} + +impl StateStringWithBoxedMutexGuard { + fn new() -> StateStringWithBoxedMutexGuard { + StateStringWithBoxedMutexGuard { + s: Mutex::new("A String".to_owned()), + } + } + fn lock(&self) -> Box> { + Box::new(self.s.lock().unwrap()) + } +} + +fn should_not_trigger_lint_for_string_ref() { + let s = StateStringWithBoxedMutexGuard::new(); + + let matcher = String::from("A String"); + + // Should not trigger lint because the second `deref` returns a string reference (`&String`). + // So it is not surprising that matching the reference implies that the lock needs to be held + // until the end of the block. + match s.lock().deref().deref() { + matcher => println!("Value is {}", s.lock().deref()), + _ => println!("Value was not a match"), + }; +} + +struct StateWithIntField { + i: u64, +} + +// Should trigger lint when either side of an assign expression contains a temporary with a +// significant drop, because the temporary's lifetime will be extended to the end of the match. +// To avoid potential unnecessary copies or creating references that would trigger the significant +// drop problem, the lint recommends moving the entire binary operation. +fn should_trigger_lint_in_assign_expr() { + let mutex = Mutex::new(StateWithIntField { i: 10 }); + + let mut i = 100; + + mutex.lock().unwrap().i = i; + match () { + //~^ ERROR: temporary with significant `Drop` in `match` scrutinee will live until the + //~| NOTE: this might lead to deadlocks or other unexpected behavior + _ => { + println!("{}", mutex.lock().unwrap().i); + }, + }; + + let value = mutex.lock().unwrap().i; + match i = value { + //~^ ERROR: temporary with significant `Drop` in `match` scrutinee will live until the + //~| NOTE: this might lead to deadlocks or other unexpected behavior + _ => { + println!("{}", mutex.lock().unwrap().i); + }, + }; + + mutex.lock().unwrap().i += 1; + match () { + //~^ ERROR: temporary with significant `Drop` in `match` scrutinee will live until the + //~| NOTE: this might lead to deadlocks or other unexpected behavior + _ => { + println!("{}", mutex.lock().unwrap().i); + }, + }; + + let value = mutex.lock().unwrap().i; + match i += value { + //~^ ERROR: temporary with significant `Drop` in `match` scrutinee will live until the + //~| NOTE: this might lead to deadlocks or other unexpected behavior + _ => { + println!("{}", mutex.lock().unwrap().i); + }, + }; +} + +#[derive(Debug)] +enum RecursiveEnum { + Foo(Option>), +} + +#[derive(Debug)] +enum GenericRecursiveEnum { + Foo(T, Option>>), +} + +fn should_not_cause_stack_overflow() { + // Test that when a type recursively contains itself, a stack overflow does not occur when + // checking sub-types for significant drops. + let f = RecursiveEnum::Foo(Some(Box::new(RecursiveEnum::Foo(None)))); + match f { + RecursiveEnum::Foo(Some(f)) => { + println!("{:?}", f) + }, + RecursiveEnum::Foo(f) => { + println!("{:?}", f) + }, + } + + let f = GenericRecursiveEnum::Foo(1u64, Some(Box::new(GenericRecursiveEnum::Foo(2u64, None)))); + match f { + GenericRecursiveEnum::Foo(i, Some(f)) => { + println!("{} {:?}", i, f) + }, + GenericRecursiveEnum::Foo(i, f) => { + println!("{} {:?}", i, f) + }, + } +} + +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() }); + let value = rwlock.read().unwrap().to_number(); + match value { + //~^ ERROR: temporary with significant `Drop` in `match` scrutinee will live until the + //~| NOTE: this might lead to deadlocks or other unexpected behavior + Ok(n) => println!("Converted to number: {}", n), + Err(e) => println!("Could not convert {} to number", e), + }; +} + +fn should_not_trigger_lint_for_read_write_lock_for_loop() { + let rwlock = RwLock::>::new(vec!["1".to_string()]); + // Should not trigger lint. Since we're iterating over the data, it's obvious that the lock + // has to be held until the iteration finishes. + // https://github.com/rust-lang/rust-clippy/issues/8987 + for s in rwlock.read().unwrap().iter() { + println!("{}", s); + } +} + +fn do_bar(mutex: &Mutex) { + mutex.lock().unwrap().bar(); +} + +fn should_trigger_lint_without_significant_drop_in_arm() { + let mutex = Mutex::new(State {}); + + // Should trigger lint because the lifetime of the temporary MutexGuard is surprising because it + // is preserved until the end of the match, but there is no clear indication that this is the + // case. + let value = mutex.lock().unwrap().foo(); + match value { + //~^ ERROR: temporary with significant `Drop` in `match` scrutinee will live until the + //~| NOTE: this might lead to deadlocks or other unexpected behavior + true => do_bar(&mutex), + false => {}, + }; +} + +fn should_not_trigger_on_significant_iterator_drop() { + let lines = std::io::stdin().lines(); + for line in lines { + println!("foo: {}", line.unwrap()); + } +} + +// https://github.com/rust-lang/rust-clippy/issues/9072 +fn should_not_trigger_lint_if_place_expr_has_significant_drop() { + let x = Mutex::new(vec![1, 2, 3]); + let x_guard = x.lock().unwrap(); + + match x_guard[0] { + 1 => println!("1!"), + x => println!("{x}"), + } + + match x_guard.len() { + 1 => println!("1!"), + x => println!("{x}"), + } +} + +struct Guard<'a, T>(MutexGuard<'a, T>); + +struct Ref<'a, T>(&'a T); + +impl<'a, T> Guard<'a, T> { + fn guard(&self) -> &MutexGuard { + &self.0 + } + + fn guard_ref(&self) -> Ref> { + Ref(&self.0) + } + + fn take(self) -> Box> { + Box::new(self.0) + } +} + +fn should_not_trigger_for_significant_drop_ref() { + let mutex = Mutex::new(vec![1, 2]); + let guard = Guard(mutex.lock().unwrap()); + + match guard.guard().len() { + 0 => println!("empty"), + _ => println!("not empty"), + } + + match guard.guard_ref().0.len() { + 0 => println!("empty"), + _ => println!("not empty"), + } + + let value = guard.take().len(); + match value { + //~^ ERROR: temporary with significant `Drop` in `match` scrutinee will live until the + //~| NOTE: this might lead to deadlocks or other unexpected behavior + 0 => println!("empty"), + _ => println!("not empty"), + }; +} + +struct Foo<'a>(&'a Vec); + +impl<'a> Foo<'a> { + fn copy_old_lifetime(&self) -> &'a Vec { + self.0 + } + + fn reborrow_new_lifetime(&self) -> &Vec { + self.0 + } +} + +fn should_trigger_lint_if_and_only_if_lifetime_is_irrelevant() { + let vec = Vec::new(); + let mutex = Mutex::new(Foo(&vec)); + + // Should trigger lint even if `copy_old_lifetime()` has a lifetime, as the lifetime of + // `&vec` is unrelated to the temporary with significant drop (i.e., the `MutexGuard`). + let value = mutex.lock().unwrap().copy_old_lifetime(); + for val in value { + //~^ ERROR: temporary with significant `Drop` in `for` loop condition will live until the + //~| NOTE: this might lead to deadlocks or other unexpected behavior + println!("{}", val); + } + + // Should not trigger lint because `reborrow_new_lifetime()` has a lifetime and the + // lifetime is related to the temporary with significant drop (i.e., the `MutexGuard`). + for val in mutex.lock().unwrap().reborrow_new_lifetime() { + println!("{}", val); + } +} + +fn should_not_trigger_lint_for_complex_lifetime() { + let mutex = Mutex::new(vec!["hello".to_owned()]); + let string = "world".to_owned(); + + // Should not trigger lint due to the relevant lifetime. + for c in mutex.lock().unwrap().first().unwrap().chars() { + println!("{}", c); + } + + // Should trigger lint due to the irrelevant lifetime. + // + // FIXME: The lifetime is too complex to analyze. In order to avoid false positives, we do not + // trigger lint. + for c in mutex.lock().unwrap().first().map(|_| &string).unwrap().chars() { + println!("{}", c); + } +} + +fn should_not_trigger_lint_with_explicit_drop() { + let mutex = Mutex::new(vec![1]); + + // Should not trigger lint since the drop explicitly happens. + for val in [drop(mutex.lock().unwrap()), ()] { + println!("{:?}", val); + } + + // Should trigger lint if there is no explicit drop. + let value = mutex.lock().unwrap()[0]; + for val in [value, 2] { + //~^ ERROR: temporary with significant `Drop` in `for` loop condition will live until the + //~| NOTE: this might lead to deadlocks or other unexpected behavior + println!("{:?}", val); + } +} + +fn should_trigger_lint_in_if_let() { + let mutex = Mutex::new(vec![1]); + + let value = mutex.lock().unwrap().first().copied(); + if let Some(val) = value { + //~^ ERROR: temporary with significant `Drop` in `if let` scrutinee will live until the + //~| NOTE: this might lead to deadlocks or other unexpected behavior + println!("{}", val); + } + + // Should not trigger lint without the final `copied()`, because we actually hold a reference + // (i.e., the `val`) to the locked data. + if let Some(val) = mutex.lock().unwrap().first() { + println!("{}", val); + }; +} + +async fn foo_async(mutex: &Mutex) -> Option> { + Some(mutex.lock().unwrap()) +} + +async fn should_trigger_lint_for_async(mutex: Mutex) -> i32 { + let value = *foo_async(&mutex).await.unwrap(); + match value { + n if n < 10 => n, + _ => 10, + } +} + +async fn should_not_trigger_lint_in_async_expansion(mutex: Mutex) -> i32 { + match foo_async(&mutex).await { + Some(guard) => *guard, + _ => 0, + } +} + +fn should_trigger_lint_in_match_expr() { + let mutex = Mutex::new(State {}); + + // Should trigger lint because the lifetime of the temporary MutexGuard is surprising because it + // is preserved until the end of the match, but there is no clear indication that this is the + // case. + let value = mutex.lock().unwrap().foo(); + let _ = match value { + //~^ ERROR: temporary with significant `Drop` in `match` scrutinee will live until the + //~| NOTE: this might lead to deadlocks or other unexpected behavior + true => 0, + false => 1, + }; +} + +fn main() {} diff --git a/tests/ui/significant_drop_in_scrutinee.rs b/tests/ui/significant_drop_in_scrutinee.rs index 39d550398d70..ac703ecf1c56 100644 --- a/tests/ui/significant_drop_in_scrutinee.rs +++ b/tests/ui/significant_drop_in_scrutinee.rs @@ -1,5 +1,3 @@ -// FIXME: Ideally these suggestions would be fixed via rustfix. Blocked by rust-lang/rust#53934 -//@no-rustfix #![warn(clippy::significant_drop_in_scrutinee)] #![allow(dead_code, unused_assignments)] #![allow( @@ -822,16 +820,6 @@ fn should_trigger_lint_in_if_let() { }; } -fn should_trigger_lint_in_while_let() { - let mutex = Mutex::new(vec![1]); - - while let Some(val) = mutex.lock().unwrap().pop() { - //~^ ERROR: temporary with significant `Drop` in `while let` scrutinee will live until the - //~| NOTE: this might lead to deadlocks or other unexpected behavior - println!("{}", val); - } -} - async fn foo_async(mutex: &Mutex) -> Option> { Some(mutex.lock().unwrap()) } diff --git a/tests/ui/significant_drop_in_scrutinee.stderr b/tests/ui/significant_drop_in_scrutinee.stderr index f99d862aa6b2..2508a053f802 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 will live until the end of the `match` expression - --> tests/ui/significant_drop_in_scrutinee.rs:60:11 + --> tests/ui/significant_drop_in_scrutinee.rs:58:11 | LL | match mutex.lock().unwrap().foo() { | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ @@ -20,7 +20,7 @@ LL ~ match value { | error: temporary with significant `Drop` in `match` scrutinee will live until the end of the `match` expression - --> tests/ui/significant_drop_in_scrutinee.rs:148:11 + --> tests/ui/significant_drop_in_scrutinee.rs:146:11 | LL | match s.lock_m().get_the_value() { | ^^^^^^^^^^^^^^^^^^^^^^^^^^ @@ -42,7 +42,7 @@ LL ~ match value { | error: temporary with significant `Drop` in `match` scrutinee will live until the end of the `match` expression - --> tests/ui/significant_drop_in_scrutinee.rs:171:11 + --> tests/ui/significant_drop_in_scrutinee.rs:169:11 | LL | match s.lock_m_m().get_the_value() { | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ @@ -64,7 +64,7 @@ LL ~ match value { | error: temporary with significant `Drop` in `match` scrutinee will live until the end of the `match` expression - --> tests/ui/significant_drop_in_scrutinee.rs:221:11 + --> tests/ui/significant_drop_in_scrutinee.rs:219:11 | LL | match counter.temp_increment().len() { | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ @@ -80,7 +80,7 @@ LL ~ match value { | error: temporary with significant `Drop` in `match` scrutinee will live until the end of the `match` expression - --> tests/ui/significant_drop_in_scrutinee.rs:246:16 + --> tests/ui/significant_drop_in_scrutinee.rs:244:16 | LL | match (mutex1.lock().unwrap().s.len(), true) { | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ @@ -99,7 +99,7 @@ LL ~ match (value, true) { | error: temporary with significant `Drop` in `match` scrutinee will live until the end of the `match` expression - --> tests/ui/significant_drop_in_scrutinee.rs:257:22 + --> tests/ui/significant_drop_in_scrutinee.rs:255:22 | LL | match (true, mutex1.lock().unwrap().s.len(), true) { | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ @@ -118,7 +118,7 @@ LL ~ match (true, value, true) { | error: temporary with significant `Drop` in `match` scrutinee will live until the end of the `match` expression - --> tests/ui/significant_drop_in_scrutinee.rs:269:16 + --> tests/ui/significant_drop_in_scrutinee.rs:267:16 | LL | match (mutex1.lock().unwrap().s.len(), true, mutex2.lock().unwrap().s.len()) { | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ @@ -134,12 +134,12 @@ LL | }; = note: this might lead to deadlocks or other unexpected behavior help: try moving the temporary above the match | -LL ~ let value = mutex1.lock().unwrap().s.len(); -LL ~ match (value, true, mutex2.lock().unwrap().s.len()) { +LL ~ let value1 = mutex1.lock().unwrap().s.len(); +LL ~ match (value1, true, mutex2.lock().unwrap().s.len()) { | error: temporary with significant `Drop` in `match` scrutinee will live until the end of the `match` expression - --> tests/ui/significant_drop_in_scrutinee.rs:269:54 + --> tests/ui/significant_drop_in_scrutinee.rs:267:54 | LL | match (mutex1.lock().unwrap().s.len(), true, mutex2.lock().unwrap().s.len()) { | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ @@ -155,12 +155,12 @@ LL | }; = note: this might lead to deadlocks or other unexpected behavior help: try moving the temporary above the match | -LL ~ let value = mutex2.lock().unwrap().s.len(); -LL ~ match (mutex1.lock().unwrap().s.len(), true, value) { +LL ~ let value2 = mutex2.lock().unwrap().s.len(); +LL ~ match (mutex1.lock().unwrap().s.len(), true, value2) { | error: temporary with significant `Drop` in `match` scrutinee will live until the end of the `match` expression - --> tests/ui/significant_drop_in_scrutinee.rs:324:11 + --> tests/ui/significant_drop_in_scrutinee.rs:322:11 | LL | match mutex.lock().unwrap().s.len() > 1 { | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ @@ -179,7 +179,7 @@ LL ~ match value > 1 { | error: temporary with significant `Drop` in `match` scrutinee will live until the end of the `match` expression - --> tests/ui/significant_drop_in_scrutinee.rs:333:15 + --> tests/ui/significant_drop_in_scrutinee.rs:331:15 | LL | match 1 < mutex.lock().unwrap().s.len() { | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ @@ -198,7 +198,7 @@ LL ~ match 1 < value { | error: temporary with significant `Drop` in `match` scrutinee will live until the end of the `match` expression - --> tests/ui/significant_drop_in_scrutinee.rs:353:11 + --> tests/ui/significant_drop_in_scrutinee.rs:351:11 | LL | match mutex1.lock().unwrap().s.len() < mutex2.lock().unwrap().s.len() { | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ @@ -214,12 +214,12 @@ LL | }; = note: this might lead to deadlocks or other unexpected behavior help: try moving the temporary above the match | -LL ~ let value = mutex1.lock().unwrap().s.len(); -LL ~ match value < mutex2.lock().unwrap().s.len() { +LL ~ let value1 = mutex1.lock().unwrap().s.len(); +LL ~ match value1 < mutex2.lock().unwrap().s.len() { | error: temporary with significant `Drop` in `match` scrutinee will live until the end of the `match` expression - --> tests/ui/significant_drop_in_scrutinee.rs:353:44 + --> tests/ui/significant_drop_in_scrutinee.rs:351:44 | LL | match mutex1.lock().unwrap().s.len() < mutex2.lock().unwrap().s.len() { | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ @@ -235,12 +235,12 @@ LL | }; = note: this might lead to deadlocks or other unexpected behavior help: try moving the temporary above the match | -LL ~ let value = mutex2.lock().unwrap().s.len(); -LL ~ match mutex1.lock().unwrap().s.len() < value { +LL ~ let value2 = mutex2.lock().unwrap().s.len(); +LL ~ match mutex1.lock().unwrap().s.len() < value2 { | error: temporary with significant `Drop` in `match` scrutinee will live until the end of the `match` expression - --> tests/ui/significant_drop_in_scrutinee.rs:366:11 + --> tests/ui/significant_drop_in_scrutinee.rs:364:11 | LL | match mutex1.lock().unwrap().s.len() >= mutex2.lock().unwrap().s.len() { | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ @@ -256,12 +256,12 @@ LL | }; = note: this might lead to deadlocks or other unexpected behavior help: try moving the temporary above the match | -LL ~ let value = mutex1.lock().unwrap().s.len(); -LL ~ match value >= mutex2.lock().unwrap().s.len() { +LL ~ let value1 = mutex1.lock().unwrap().s.len(); +LL ~ match value1 >= mutex2.lock().unwrap().s.len() { | error: temporary with significant `Drop` in `match` scrutinee will live until the end of the `match` expression - --> tests/ui/significant_drop_in_scrutinee.rs:366:45 + --> tests/ui/significant_drop_in_scrutinee.rs:364:45 | LL | match mutex1.lock().unwrap().s.len() >= mutex2.lock().unwrap().s.len() { | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ @@ -277,12 +277,12 @@ LL | }; = note: this might lead to deadlocks or other unexpected behavior help: try moving the temporary above the match | -LL ~ let value = mutex2.lock().unwrap().s.len(); -LL ~ match mutex1.lock().unwrap().s.len() >= value { +LL ~ let value2 = mutex2.lock().unwrap().s.len(); +LL ~ match mutex1.lock().unwrap().s.len() >= value2 { | error: temporary with significant `Drop` in `match` scrutinee will live until the end of the `match` expression - --> tests/ui/significant_drop_in_scrutinee.rs:403:11 + --> tests/ui/significant_drop_in_scrutinee.rs:401:11 | LL | match get_mutex_guard().s.len() > 1 { | ^^^^^^^^^^^^^^^^^^^^^^^^^ @@ -301,7 +301,7 @@ LL ~ match value > 1 { | error: temporary with significant `Drop` in `match` scrutinee will live until the end of the `match` expression - --> tests/ui/significant_drop_in_scrutinee.rs:422:11 + --> tests/ui/significant_drop_in_scrutinee.rs:420:11 | LL | match match i { | ___________^ @@ -334,7 +334,7 @@ LL ~ match value | error: temporary with significant `Drop` in `match` scrutinee will live until the end of the `match` expression - --> tests/ui/significant_drop_in_scrutinee.rs:450:11 + --> tests/ui/significant_drop_in_scrutinee.rs:448:11 | LL | match if i > 1 { | ___________^ @@ -368,7 +368,7 @@ LL ~ match value | error: temporary with significant `Drop` in `match` scrutinee will live until the end of the `match` expression - --> tests/ui/significant_drop_in_scrutinee.rs:506:11 + --> tests/ui/significant_drop_in_scrutinee.rs:504:11 | LL | match s.lock().deref().deref() { | ^^^^^^^^^^^^^^^^^^^^^^^^ @@ -386,7 +386,7 @@ LL ~ match (&value) { | error: temporary with significant `Drop` in `match` scrutinee will live until the end of the `match` expression - --> tests/ui/significant_drop_in_scrutinee.rs:556:11 + --> tests/ui/significant_drop_in_scrutinee.rs:554:11 | LL | match mutex.lock().unwrap().i = i { | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ @@ -405,7 +405,7 @@ LL ~ match () { | error: temporary with significant `Drop` in `match` scrutinee will live until the end of the `match` expression - --> tests/ui/significant_drop_in_scrutinee.rs:564:15 + --> tests/ui/significant_drop_in_scrutinee.rs:562:15 | LL | match i = mutex.lock().unwrap().i { | ^^^^^^^^^^^^^^^^^^^^^^^ @@ -424,7 +424,7 @@ LL ~ match i = value { | error: temporary with significant `Drop` in `match` scrutinee will live until the end of the `match` expression - --> tests/ui/significant_drop_in_scrutinee.rs:572:11 + --> tests/ui/significant_drop_in_scrutinee.rs:570:11 | LL | match mutex.lock().unwrap().i += 1 { | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ @@ -443,7 +443,7 @@ LL ~ match () { | error: temporary with significant `Drop` in `match` scrutinee will live until the end of the `match` expression - --> tests/ui/significant_drop_in_scrutinee.rs:580:16 + --> tests/ui/significant_drop_in_scrutinee.rs:578:16 | LL | match i += mutex.lock().unwrap().i { | ^^^^^^^^^^^^^^^^^^^^^^^ @@ -462,7 +462,7 @@ LL ~ match i += value { | error: temporary with significant `Drop` in `match` scrutinee will live until the end of the `match` expression - --> tests/ui/significant_drop_in_scrutinee.rs:645:11 + --> tests/ui/significant_drop_in_scrutinee.rs:643:11 | LL | match rwlock.read().unwrap().to_number() { | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ @@ -478,7 +478,7 @@ LL ~ match value { | error: temporary with significant `Drop` in `match` scrutinee will live until the end of the `match` expression - --> tests/ui/significant_drop_in_scrutinee.rs:673:11 + --> tests/ui/significant_drop_in_scrutinee.rs:671:11 | LL | match mutex.lock().unwrap().foo() { | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ @@ -494,7 +494,7 @@ LL ~ match value { | error: temporary with significant `Drop` in `match` scrutinee will live until the end of the `match` expression - --> tests/ui/significant_drop_in_scrutinee.rs:736:11 + --> tests/ui/significant_drop_in_scrutinee.rs:734:11 | LL | match guard.take().len() { | ^^^^^^^^^^^^^^^^^^ @@ -510,7 +510,7 @@ LL ~ match value { | error: temporary with significant `Drop` in `for` loop condition will live until the end of the `for` expression - --> tests/ui/significant_drop_in_scrutinee.rs:762:16 + --> tests/ui/significant_drop_in_scrutinee.rs:760:16 | LL | for val in mutex.lock().unwrap().copy_old_lifetime() { | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ @@ -526,7 +526,7 @@ LL ~ for val in value { | error: temporary with significant `Drop` in `for` loop condition will live until the end of the `for` expression - --> tests/ui/significant_drop_in_scrutinee.rs:802:17 + --> tests/ui/significant_drop_in_scrutinee.rs:800:17 | LL | for val in [mutex.lock().unwrap()[0], 2] { | ^^^^^^^^^^^^^^^^^^^^^^^^ @@ -542,7 +542,7 @@ LL ~ for val in [value, 2] { | error: temporary with significant `Drop` in `if let` scrutinee will live until the end of the `if let` expression - --> tests/ui/significant_drop_in_scrutinee.rs:812:24 + --> tests/ui/significant_drop_in_scrutinee.rs:810:24 | LL | if let Some(val) = mutex.lock().unwrap().first().copied() { | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ @@ -557,19 +557,8 @@ LL ~ let value = mutex.lock().unwrap().first().copied(); LL ~ if let Some(val) = value { | -error: temporary with significant `Drop` in `while let` scrutinee will live until the end of the `while let` expression - --> tests/ui/significant_drop_in_scrutinee.rs:828:27 - | -LL | while let Some(val) = mutex.lock().unwrap().pop() { - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ -... -LL | } - | - temporary lives until here - | - = note: this might lead to deadlocks or other unexpected behavior - error: temporary with significant `Drop` in `match` scrutinee will live until the end of the `match` expression - --> tests/ui/significant_drop_in_scrutinee.rs:840:11 + --> tests/ui/significant_drop_in_scrutinee.rs:828:11 | LL | match *foo_async(&mutex).await.unwrap() { | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ @@ -585,7 +574,7 @@ LL ~ match value { | error: temporary with significant `Drop` in `match` scrutinee will live until the end of the `match` expression - --> tests/ui/significant_drop_in_scrutinee.rs:859:19 + --> tests/ui/significant_drop_in_scrutinee.rs:847:19 | LL | let _ = match mutex.lock().unwrap().foo() { | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ @@ -600,5 +589,5 @@ LL ~ let value = mutex.lock().unwrap().foo(); LL ~ let _ = match value { | -error: aborting due to 31 previous errors +error: aborting due to 30 previous errors diff --git a/tests/ui/significant_drop_in_scrutinee_nofix.rs b/tests/ui/significant_drop_in_scrutinee_nofix.rs new file mode 100644 index 000000000000..9ddc4db4a132 --- /dev/null +++ b/tests/ui/significant_drop_in_scrutinee_nofix.rs @@ -0,0 +1,14 @@ +#![warn(clippy::significant_drop_in_scrutinee)] +//@no-rustfix + +use std::sync::Mutex; + +fn should_trigger_lint_in_while_let() { + let mutex = Mutex::new(vec![1]); + + while let Some(val) = mutex.lock().unwrap().pop() { + //~^ ERROR: temporary with significant `Drop` in `while let` scrutinee will live until the + //~| NOTE: this might lead to deadlocks or other unexpected behavior + println!("{}", val); + } +} diff --git a/tests/ui/significant_drop_in_scrutinee_nofix.stderr b/tests/ui/significant_drop_in_scrutinee_nofix.stderr new file mode 100644 index 000000000000..ff8d79be9b60 --- /dev/null +++ b/tests/ui/significant_drop_in_scrutinee_nofix.stderr @@ -0,0 +1,15 @@ +error: temporary with significant `Drop` in `while let` scrutinee will live until the end of the `while let` expression + --> tests/ui/significant_drop_in_scrutinee_nofix.rs:9:27 + | +LL | while let Some(val) = mutex.lock().unwrap().pop() { + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ +... +LL | } + | - temporary lives until here + | + = note: this might lead to deadlocks or other unexpected behavior + = note: `-D clippy::significant-drop-in-scrutinee` implied by `-D warnings` + = help: to override `-D warnings` add `#[allow(clippy::significant_drop_in_scrutinee)]` + +error: aborting due to 1 previous error +