diff options
| -rw-r--r-- | clippy_lints/src/lib.register_all.rs | 2 | ||||
| -rw-r--r-- | clippy_lints/src/lib.register_lints.rs | 2 | ||||
| -rw-r--r-- | clippy_lints/src/lib.register_suspicious.rs | 2 | ||||
| -rw-r--r-- | clippy_lints/src/lib.rs | 2 | ||||
| -rw-r--r-- | clippy_lints/src/matches/mod.rs | 83 | ||||
| -rw-r--r-- | clippy_lints/src/matches/significant_drop_in_scrutinee.rs (renamed from clippy_lints/src/significant_drop_in_scrutinee.rs) | 132 |
6 files changed, 109 insertions, 114 deletions
diff --git a/clippy_lints/src/lib.register_all.rs b/clippy_lints/src/lib.register_all.rs index 15e744229a1..435c0829a67 100644 --- a/clippy_lints/src/lib.register_all.rs +++ b/clippy_lints/src/lib.register_all.rs @@ -151,6 +151,7 @@ store.register_group(true, "clippy::all", Some("clippy_all"), vec![ LintId::of(matches::MATCH_STR_CASE_MISMATCH), LintId::of(matches::NEEDLESS_MATCH), LintId::of(matches::REDUNDANT_PATTERN_MATCHING), + LintId::of(matches::SIGNIFICANT_DROP_IN_SCRUTINEE), LintId::of(matches::SINGLE_MATCH), LintId::of(matches::WILDCARD_IN_OR_PATTERNS), LintId::of(mem_replace::MEM_REPLACE_OPTION_WITH_NONE), @@ -282,7 +283,6 @@ store.register_group(true, "clippy::all", Some("clippy_all"), vec![ LintId::of(self_assignment::SELF_ASSIGNMENT), LintId::of(self_named_constructors::SELF_NAMED_CONSTRUCTORS), LintId::of(serde_api::SERDE_API_MISUSE), - LintId::of(significant_drop_in_scrutinee::SIGNIFICANT_DROP_IN_SCRUTINEE), LintId::of(single_component_path_imports::SINGLE_COMPONENT_PATH_IMPORTS), LintId::of(size_of_in_element_count::SIZE_OF_IN_ELEMENT_COUNT), LintId::of(slow_vector_initialization::SLOW_VECTOR_INITIALIZATION), diff --git a/clippy_lints/src/lib.register_lints.rs b/clippy_lints/src/lib.register_lints.rs index 59ba295a887..7fc6ff8a2fb 100644 --- a/clippy_lints/src/lib.register_lints.rs +++ b/clippy_lints/src/lib.register_lints.rs @@ -276,6 +276,7 @@ store.register_lints(&[ matches::NEEDLESS_MATCH, matches::REDUNDANT_PATTERN_MATCHING, matches::REST_PAT_IN_FULLY_BOUND_STRUCTS, + matches::SIGNIFICANT_DROP_IN_SCRUTINEE, matches::SINGLE_MATCH, matches::SINGLE_MATCH_ELSE, matches::WILDCARD_ENUM_MATCH_ARM, @@ -479,7 +480,6 @@ store.register_lints(&[ shadow::SHADOW_REUSE, shadow::SHADOW_SAME, shadow::SHADOW_UNRELATED, - significant_drop_in_scrutinee::SIGNIFICANT_DROP_IN_SCRUTINEE, single_char_lifetime_names::SINGLE_CHAR_LIFETIME_NAMES, single_component_path_imports::SINGLE_COMPONENT_PATH_IMPORTS, size_of_in_element_count::SIZE_OF_IN_ELEMENT_COUNT, diff --git a/clippy_lints/src/lib.register_suspicious.rs b/clippy_lints/src/lib.register_suspicious.rs index 43ced5070f1..7b13713c36e 100644 --- a/clippy_lints/src/lib.register_suspicious.rs +++ b/clippy_lints/src/lib.register_suspicious.rs @@ -24,12 +24,12 @@ store.register_group(true, "clippy::suspicious", Some("clippy_suspicious"), vec! LintId::of(loops::EMPTY_LOOP), LintId::of(loops::FOR_LOOPS_OVER_FALLIBLES), LintId::of(loops::MUT_RANGE_BOUND), + LintId::of(matches::SIGNIFICANT_DROP_IN_SCRUTINEE), LintId::of(methods::NO_EFFECT_REPLACE), LintId::of(methods::SUSPICIOUS_MAP), LintId::of(mut_key::MUTABLE_KEY_TYPE), LintId::of(octal_escapes::OCTAL_ESCAPES), LintId::of(rc_clone_in_vec_init::RC_CLONE_IN_VEC_INIT), - LintId::of(significant_drop_in_scrutinee::SIGNIFICANT_DROP_IN_SCRUTINEE), LintId::of(suspicious_trait_impl::SUSPICIOUS_ARITHMETIC_IMPL), LintId::of(suspicious_trait_impl::SUSPICIOUS_OP_ASSIGN_IMPL), LintId::of(swap_ptr_to_ref::SWAP_PTR_TO_REF), diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 7335b4a35c8..742409d758b 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -367,7 +367,6 @@ mod self_named_constructors; mod semicolon_if_nothing_returned; mod serde_api; mod shadow; -mod significant_drop_in_scrutinee; mod single_char_lifetime_names; mod single_component_path_imports; mod size_of_in_element_count; @@ -886,7 +885,6 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: store.register_late_pass(|| Box::new(default_union_representation::DefaultUnionRepresentation)); store.register_early_pass(|| Box::new(doc_link_with_quotes::DocLinkWithQuotes)); store.register_late_pass(|| Box::new(only_used_in_recursion::OnlyUsedInRecursion)); - store.register_late_pass(|| Box::new(significant_drop_in_scrutinee::SignificantDropInScrutinee)); let allow_dbg_in_tests = conf.allow_dbg_in_tests; store.register_late_pass(move || Box::new(dbg_macro::DbgMacro::new(allow_dbg_in_tests))); let cargo_ignore_publish = conf.cargo_ignore_publish; diff --git a/clippy_lints/src/matches/mod.rs b/clippy_lints/src/matches/mod.rs index a58c71ca5f8..3c0dc8041d8 100644 --- a/clippy_lints/src/matches/mod.rs +++ b/clippy_lints/src/matches/mod.rs @@ -25,6 +25,7 @@ mod needless_match; mod overlapping_arms; mod redundant_pattern_match; mod rest_pat_in_fully_bound_struct; +mod significant_drop_in_scrutinee; mod single_match; mod wild_in_or_pats; @@ -748,6 +749,82 @@ declare_clippy_lint! { "creation of a case altering match expression with non-compliant arms" } +declare_clippy_lint! { + /// ### What it does + /// Check for temporaries returned from function calls in a match scrutinee that have the + /// `clippy::has_significant_drop` attribute. + /// + /// ### Why is this bad? + /// The `clippy::has_significant_drop` attribute can be added to types whose Drop impls have + /// an important side-effect, such as unlocking a mutex, making it important for users to be + /// able to accurately understand their lifetimes. When a temporary is returned in a function + /// call in a match scrutinee, its lifetime lasts until the end of the match block, which may + /// be surprising. + /// + /// For `Mutex`es this can lead to a deadlock. This happens when the match scrutinee uses a + /// function call that returns a `MutexGuard` and then tries to lock again in one of the match + /// arms. In that case the `MutexGuard` in the scrutinee will not be dropped until the end of + /// the match block and thus will not unlock. + /// + /// ### Example + /// ```rust.ignore + /// # use std::sync::Mutex; + /// + /// # struct State {} + /// + /// # impl State { + /// # fn foo(&self) -> bool { + /// # true + /// # } + /// + /// # fn bar(&self) {} + /// # } + /// + /// + /// let mutex = Mutex::new(State {}); + /// + /// match mutex.lock().unwrap().foo() { + /// true => { + /// mutex.lock().unwrap().bar(); // Deadlock! + /// } + /// false => {} + /// }; + /// + /// println!("All done!"); + /// + /// ``` + /// Use instead: + /// ```rust + /// # use std::sync::Mutex; + /// + /// # struct State {} + /// + /// # impl State { + /// # fn foo(&self) -> bool { + /// # true + /// # } + /// + /// # fn bar(&self) {} + /// # } + /// + /// let mutex = Mutex::new(State {}); + /// + /// let is_foo = mutex.lock().unwrap().foo(); + /// match is_foo { + /// true => { + /// mutex.lock().unwrap().bar(); + /// } + /// false => {} + /// }; + /// + /// println!("All done!"); + /// ``` + #[clippy::version = "1.60.0"] + pub SIGNIFICANT_DROP_IN_SCRUTINEE, + suspicious, + "warns when a temporary of a type with a drop with a significant side-effect might have a surprising lifetime" +} + #[derive(Default)] pub struct Matches { msrv: Option<RustcVersion>, @@ -786,6 +863,7 @@ impl_lint_pass!(Matches => [ MANUAL_UNWRAP_OR, MATCH_ON_VEC_ITEMS, MATCH_STR_CASE_MISMATCH, + SIGNIFICANT_DROP_IN_SCRUTINEE, ]); impl<'tcx> LateLintPass<'tcx> for Matches { @@ -796,9 +874,12 @@ impl<'tcx> LateLintPass<'tcx> for Matches { let from_expansion = expr.span.from_expansion(); if let ExprKind::Match(ex, arms, source) = expr.kind { - if !span_starts_with(cx, expr.span, "match") { + if source == MatchSource::Normal && !span_starts_with(cx, expr.span, "match") { return; } + if matches!(source, MatchSource::Normal | MatchSource::ForLoopDesugar) { + significant_drop_in_scrutinee::check(cx, expr, ex, source); + } collapsible_match::check_match(cx, arms); if !from_expansion { diff --git a/clippy_lints/src/significant_drop_in_scrutinee.rs b/clippy_lints/src/matches/significant_drop_in_scrutinee.rs index 2f7819cb470..a211dc18f9e 100644 --- a/clippy_lints/src/significant_drop_in_scrutinee.rs +++ b/clippy_lints/src/matches/significant_drop_in_scrutinee.rs @@ -5,98 +5,24 @@ use clippy_utils::source::{indent_of, snippet}; use rustc_errors::{Applicability, Diagnostic}; use rustc_hir::intravisit::{walk_expr, Visitor}; use rustc_hir::{Expr, ExprKind, MatchSource}; -use rustc_lint::{LateContext, LateLintPass, LintContext}; +use rustc_lint::{LateContext, LintContext}; use rustc_middle::ty::subst::GenericArgKind; use rustc_middle::ty::{Ty, TypeAndMut}; -use rustc_session::{declare_lint_pass, declare_tool_lint}; use rustc_span::Span; -declare_clippy_lint! { - /// ### What it does - /// Check for temporaries returned from function calls in a match scrutinee that have the - /// `clippy::has_significant_drop` attribute. - /// - /// ### Why is this bad? - /// The `clippy::has_significant_drop` attribute can be added to types whose Drop impls have - /// an important side-effect, such as unlocking a mutex, making it important for users to be - /// able to accurately understand their lifetimes. When a temporary is returned in a function - /// call in a match scrutinee, its lifetime lasts until the end of the match block, which may - /// be surprising. - /// - /// For `Mutex`es this can lead to a deadlock. This happens when the match scrutinee uses a - /// function call that returns a `MutexGuard` and then tries to lock again in one of the match - /// arms. In that case the `MutexGuard` in the scrutinee will not be dropped until the end of - /// the match block and thus will not unlock. - /// - /// ### Example - /// ```rust.ignore - /// # use std::sync::Mutex; - /// - /// # struct State {} - /// - /// # impl State { - /// # fn foo(&self) -> bool { - /// # true - /// # } - /// - /// # fn bar(&self) {} - /// # } - /// - /// - /// let mutex = Mutex::new(State {}); - /// - /// match mutex.lock().unwrap().foo() { - /// true => { - /// mutex.lock().unwrap().bar(); // Deadlock! - /// } - /// false => {} - /// }; - /// - /// println!("All done!"); - /// - /// ``` - /// Use instead: - /// ```rust - /// # use std::sync::Mutex; - /// - /// # struct State {} - /// - /// # impl State { - /// # fn foo(&self) -> bool { - /// # true - /// # } - /// - /// # fn bar(&self) {} - /// # } - /// - /// let mutex = Mutex::new(State {}); - /// - /// let is_foo = mutex.lock().unwrap().foo(); - /// match is_foo { - /// true => { - /// mutex.lock().unwrap().bar(); - /// } - /// false => {} - /// }; - /// - /// println!("All done!"); - /// ``` - #[clippy::version = "1.60.0"] - pub SIGNIFICANT_DROP_IN_SCRUTINEE, - suspicious, - "warns when a temporary of a type with a drop with a significant side-effect might have a surprising lifetime" -} - -declare_lint_pass!(SignificantDropInScrutinee => [SIGNIFICANT_DROP_IN_SCRUTINEE]); +use super::SIGNIFICANT_DROP_IN_SCRUTINEE; -impl<'tcx> LateLintPass<'tcx> for SignificantDropInScrutinee { - fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) { - if let Some((suggestions, message)) = has_significant_drop_in_scrutinee(cx, expr) { - for found in suggestions { - span_lint_and_then(cx, SIGNIFICANT_DROP_IN_SCRUTINEE, found.found_span, message, |diag| { - set_diagnostic(diag, cx, expr, found); - }); - } +pub(super) fn check<'tcx>( + cx: &LateContext<'tcx>, + expr: &'tcx Expr<'tcx>, + scrutinee: &'tcx Expr<'_>, + source: MatchSource, +) { + if let Some((suggestions, message)) = has_significant_drop_in_scrutinee(cx, scrutinee, source) { + for found in suggestions { + span_lint_and_then(cx, SIGNIFICANT_DROP_IN_SCRUTINEE, found.found_span, message, |diag| { + set_diagnostic(diag, cx, expr, found); + }); } } } @@ -148,28 +74,18 @@ fn set_diagnostic<'tcx>(diag: &mut Diagnostic, cx: &LateContext<'tcx>, expr: &'t /// may have a surprising lifetime. fn has_significant_drop_in_scrutinee<'tcx, 'a>( cx: &'a LateContext<'tcx>, - expr: &'tcx Expr<'tcx>, + scrutinee: &'tcx Expr<'tcx>, + source: MatchSource, ) -> Option<(Vec<FoundSigDrop>, &'static str)> { - match expr.kind { - ExprKind::Match(match_expr, _, source) => { - match source { - MatchSource::Normal | MatchSource::ForLoopDesugar => { - let mut helper = SigDropHelper::new(cx); - helper.find_sig_drop(match_expr).map(|drops| { - let message = if source == MatchSource::Normal { - "temporary with significant drop in match scrutinee" - } else { - "temporary with significant drop in for loop" - }; - (drops, message) - }) - }, - // MatchSource of TryDesugar or AwaitDesugar is out of scope for this lint - MatchSource::TryDesugar | MatchSource::AwaitDesugar => None, - } - }, - _ => None, - } + let mut helper = SigDropHelper::new(cx); + helper.find_sig_drop(scrutinee).map(|drops| { + let message = if source == MatchSource::Normal { + "temporary with significant drop in match scrutinee" + } else { + "temporary with significant drop in for loop" + }; + (drops, message) + }) } struct SigDropHelper<'a, 'tcx> { |