From 850ef87d7d634ecc2f4f59c5932c0f13773dcc99 Mon Sep 17 00:00:00 2001 From: Caio Date: Tue, 7 Feb 2023 15:30:27 -0300 Subject: [PATCH] [significant_drop_tightening] Add MVP --- CHANGELOG.md | 1 + clippy_lints/src/declared_lints.rs | 1 + clippy_lints/src/lib.rs | 4 + clippy_lints/src/sig_drop_aux.rs | 60 +++++ .../src/significant_drop_tightening.rs | 240 ++++++++++++++++++ tests/ui/significant_drop_tightening.rs | 16 ++ tests/ui/significant_drop_tightening.stderr | 24 ++ 7 files changed, 346 insertions(+) create mode 100644 clippy_lints/src/sig_drop_aux.rs create mode 100644 clippy_lints/src/significant_drop_tightening.rs create mode 100644 tests/ui/significant_drop_tightening.rs create mode 100644 tests/ui/significant_drop_tightening.stderr diff --git a/CHANGELOG.md b/CHANGELOG.md index 659e8aebcd57..2367524ca724 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4734,6 +4734,7 @@ Released 2018-09-13 [`should_assert_eq`]: https://rust-lang.github.io/rust-clippy/master/index.html#should_assert_eq [`should_implement_trait`]: https://rust-lang.github.io/rust-clippy/master/index.html#should_implement_trait [`significant_drop_in_scrutinee`]: https://rust-lang.github.io/rust-clippy/master/index.html#significant_drop_in_scrutinee +[`significant_drop_tightening`]: https://rust-lang.github.io/rust-clippy/master/index.html#significant_drop_tightening [`similar_names`]: https://rust-lang.github.io/rust-clippy/master/index.html#similar_names [`single_char_add_str`]: https://rust-lang.github.io/rust-clippy/master/index.html#single_char_add_str [`single_char_lifetime_names`]: https://rust-lang.github.io/rust-clippy/master/index.html#single_char_lifetime_names diff --git a/clippy_lints/src/declared_lints.rs b/clippy_lints/src/declared_lints.rs index 457a25826e79..cb71207efeba 100644 --- a/clippy_lints/src/declared_lints.rs +++ b/clippy_lints/src/declared_lints.rs @@ -137,6 +137,7 @@ pub(crate) static LINTS: &[&crate::LintInfo] = &[ crate::drop_forget_ref::FORGET_REF_INFO, crate::drop_forget_ref::UNDROPPED_MANUALLY_DROPS_INFO, crate::duplicate_mod::DUPLICATE_MOD_INFO, + crate::significant_drop_tightening::SIGNIFICANT_DROP_TIGHTENING_INFO, crate::else_if_without_else::ELSE_IF_WITHOUT_ELSE_INFO, crate::empty_drop::EMPTY_DROP_INFO, crate::empty_enum::EMPTY_ENUM_INFO, diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 2f5c4adca9f1..daaa269a2c8e 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -3,6 +3,7 @@ #![feature(box_patterns)] #![feature(control_flow_enum)] #![feature(drain_filter)] +#![feature(if_let_guard)] #![feature(iter_intersperse)] #![feature(let_chains)] #![feature(lint_reasons)] @@ -264,6 +265,8 @@ mod semicolon_block; mod semicolon_if_nothing_returned; mod serde_api; mod shadow; +mod sig_drop_aux; +mod significant_drop_tightening; mod single_char_lifetime_names; mod single_component_path_imports; mod size_of_in_element_count; @@ -559,6 +562,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: store.register_late_pass(|_| Box::new(eta_reduction::EtaReduction)); store.register_late_pass(|_| Box::new(mut_mut::MutMut)); store.register_late_pass(|_| Box::new(mut_reference::UnnecessaryMutPassed)); + store.register_late_pass(|_| Box::new(significant_drop_tightening::SignificantDropTightening::new())); store.register_late_pass(|_| Box::new(len_zero::LenZero)); store.register_late_pass(|_| Box::new(attrs::Attributes)); store.register_late_pass(|_| Box::new(blocks_in_if_conditions::BlocksInIfConditions)); diff --git a/clippy_lints/src/sig_drop_aux.rs b/clippy_lints/src/sig_drop_aux.rs new file mode 100644 index 000000000000..fa833294d4a3 --- /dev/null +++ b/clippy_lints/src/sig_drop_aux.rs @@ -0,0 +1,60 @@ +// Auxiliary structures for lints involving `#[clippy::has_significant_drop]`. + +use crate::FxHashSet; +use clippy_utils::get_attr; +use rustc_lint::{LateContext, LintContext}; +use rustc_middle::ty::{subst::GenericArgKind, Ty, TypeAndMut}; + +pub(crate) struct SigDropChecker<'cx, 'sdt, 'tcx> { + cx: &'cx LateContext<'tcx>, + seen_types: &'sdt mut FxHashSet>, +} + +impl<'cx, 'sdt, 'tcx> SigDropChecker<'cx, 'sdt, 'tcx> { + pub(crate) fn new(cx: &'cx LateContext<'tcx>, seen_types: &'sdt mut FxHashSet>) -> Self { + seen_types.clear(); + Self { cx, seen_types } + } + + pub(crate) fn has_sig_drop_attr(&mut self, ty: Ty<'tcx>) -> bool { + if let Some(adt) = ty.ty_adt_def() { + if get_attr( + self.cx.sess(), + self.cx.tcx.get_attrs_unchecked(adt.did()), + "has_significant_drop", + ) + .count() + > 0 + { + return true; + } + } + match ty.kind() { + rustc_middle::ty::Adt(a, b) => { + for f in a.all_fields() { + let ty = f.ty(self.cx.tcx, b); + if !self.has_seen_ty(ty) && self.has_sig_drop_attr(ty) { + return true; + } + } + for generic_arg in b.iter() { + if let GenericArgKind::Type(ty) = generic_arg.unpack() { + if self.has_sig_drop_attr(ty) { + return true; + } + } + } + false + }, + rustc_middle::ty::Array(ty, _) + | rustc_middle::ty::RawPtr(TypeAndMut { ty, .. }) + | rustc_middle::ty::Ref(_, ty, _) + | rustc_middle::ty::Slice(ty) => self.has_sig_drop_attr(*ty), + _ => false, + } + } + + fn has_seen_ty(&mut self, ty: Ty<'tcx>) -> bool { + !self.seen_types.insert(ty) + } +} diff --git a/clippy_lints/src/significant_drop_tightening.rs b/clippy_lints/src/significant_drop_tightening.rs new file mode 100644 index 000000000000..58a7e0538415 --- /dev/null +++ b/clippy_lints/src/significant_drop_tightening.rs @@ -0,0 +1,240 @@ +use crate::{sig_drop_aux::SigDropChecker, FxHashSet}; +use clippy_utils::diagnostics::span_lint_and_then; +use clippy_utils::source::{indent_of, snippet}; +use rustc_errors::{Applicability, Diagnostic}; +use rustc_hir as hir; +use rustc_hir::intravisit::{walk_expr, Visitor}; +use rustc_lint::{LateContext, LateLintPass}; +use rustc_middle::ty::Ty; +use rustc_session::{declare_tool_lint, impl_lint_pass}; +use rustc_span::{Span, DUMMY_SP}; + +declare_clippy_lint! { + /// ### What it does + /// + /// Searches for elements marked with `#[clippy::significant_drop]` that could be early + /// dropped but are in fact dropped at the end of their scopes. In other words, enforces the + /// "tightening" of their possible lifetimes. + /// + /// ### Why is this bad? + /// + /// Elements marked with `#[clippy::has_significant_drop]` are generally synchronizing primitives + /// that manage shared resources, as such, it is desired to release them as soon as possible + /// to avoid unnecessary resource contention. + /// + /// ### Example + /// + /// ```rust,ignore + /// fn main() { + /// let lock = some_sync_resource.lock(); + /// let owned_rslt = lock.do_stuff_with_resource(); + /// // Only `owned_rslt` is needed but `lock` is still held. + /// do_heavy_computation_that_takes_time(owned_rslt); + /// } + /// ``` + /// + /// Use instead: + /// + /// ```rust,ignore + /// fn main() { + /// let owned_rslt = some_sync_resource.lock().do_stuff_with_resource(); + /// do_heavy_computation_that_takes_time(owned_rslt); + /// } + /// ``` + #[clippy::version = "1.67.0"] + pub SIGNIFICANT_DROP_TIGHTENING, + nursery, + "Searches for elements marked with `#[clippy::has_significant_drop]` that could be early dropped but are in fact dropped at the end of their scopes" +} + +impl_lint_pass!(SignificantDropTightening<'_> => [SIGNIFICANT_DROP_TIGHTENING]); + +pub struct SignificantDropTightening<'tcx> { + /// Auxiliary structure used to avoid having to verify the same type multiple times. + seen_types: FxHashSet>, +} + +impl<'tcx> SignificantDropTightening<'tcx> { + pub(crate) fn new() -> Self { + Self { + seen_types: <_>::default(), + } + } + + /// Tries to find types marked with `#[has_significant_drop]` of an expression `expr` that is + /// originated from `stmt` and then performs common logic on `sdap`. + fn manage_find( + &mut self, + cx: &LateContext<'tcx>, + expr: &'tcx hir::Expr<'_>, + idx: usize, + sdap: &mut SigDropAuxParams, + stmt: &'tcx hir::Stmt<'_>, + cb: impl Fn(&mut SigDropAuxParams), + ) { + let mut sig_drop_finder = SigDropFinder::new(cx, &mut self.seen_types); + sig_drop_finder.visit_expr(expr); + if sig_drop_finder.has_sig_drop { + cb(sdap); + sdap.last_use_of_sig_drop_stmt_idx = idx; + sdap.last_use_of_sig_drop_stmt_span = stmt.span; + sdap.number_of_sig_drops_stmts = sdap.number_of_sig_drops_stmts.wrapping_add(1); + } + } + + /// Shows a generic overall message as well as specialized messages depending on the usage. + fn set_suggestions(cx: &LateContext<'tcx>, block_span: Span, diag: &mut Diagnostic, sdap: &SigDropAuxParams) { + let init_bind = snippet(cx, sdap.sig_drop_init_bind_span, ".."); + if sdap.number_of_sig_drops_stmts > 2 { + let indent = " ".repeat(indent_of(cx, sdap.last_use_of_sig_drop_stmt_span).unwrap_or(0)); + diag.span_suggestion( + sdap.last_use_of_sig_drop_stmt_span.shrink_to_hi(), + "Drop the temporary after the end of its last usage", + format!("\n{indent}drop({});", init_bind), + Applicability::MaybeIncorrect, + ); + } + diag.note("this might lead to unnecessary resource contention"); + diag.span_label( + block_span, + format!("temporary `{init_bind}` is currently being dropped at the end of its contained scope"), + ); + } +} + +impl<'tcx> LateLintPass<'tcx> for SignificantDropTightening<'tcx> { + fn check_block(&mut self, cx: &LateContext<'tcx>, block: &'tcx hir::Block<'_>) { + let mut sdap = SigDropAuxParams::default(); + for (idx, stmt) in block.stmts.iter().enumerate() { + match stmt.kind { + hir::StmtKind::Expr(expr) => self.manage_find(cx, expr, idx, &mut sdap, stmt, |_| {}), + hir::StmtKind::Local(local) if let Some(expr) = local.init => self.manage_find( + cx, + expr, + idx, + &mut sdap, + stmt, + |local_sdap| { + if local_sdap.number_of_sig_drops_stmts == 0 { + local_sdap.sig_drop_init_bind_span = local.pat.span; + } + } + ), + hir::StmtKind::Semi(expr) => self.manage_find(cx, expr, idx, &mut sdap, stmt, |_| {}), + _ => continue + }; + } + if sdap.number_of_sig_drops_stmts > 0 + && { + let last_stmts_idx = block.stmts.len().wrapping_sub(1); + sdap.last_use_of_sig_drop_stmt_idx != last_stmts_idx + } + { + span_lint_and_then( + cx, + SIGNIFICANT_DROP_TIGHTENING, + sdap.sig_drop_init_bind_span, + "temporary with significant `Drop` can be early dropped", + |diag| { + Self::set_suggestions(cx, block.span, diag, &sdap); + }, + ); + } + } +} + +/// Auxiliary parameters used on each block check. +struct SigDropAuxParams { + /// Index of the last visited statement within a block that has any referenced inner type + /// marked with `#[has_significant_drop]`. + last_use_of_sig_drop_stmt_idx: usize, + /// Similar to `last_use_of_sig_drop_stmt_idx`. + last_use_of_sig_drop_stmt_span: Span, + /// Total number of statements within a block that have any referenced inner type marked with + /// `#[has_significant_drop]`. + number_of_sig_drops_stmts: usize, + /// The binding or variable that references the initial construction of the type marked with + /// `#[has_significant_drop]`. + sig_drop_init_bind_span: Span, +} + +impl Default for SigDropAuxParams { + fn default() -> Self { + Self { + last_use_of_sig_drop_stmt_idx: 0, + last_use_of_sig_drop_stmt_span: DUMMY_SP, + number_of_sig_drops_stmts: 0, + sig_drop_init_bind_span: DUMMY_SP, + } + } +} + +/// Performs recursive calls to find any inner type marked with `#[has_significant_drop]`. +struct SigDropFinder<'cx, 'sdt, 'tcx> { + cx: &'cx LateContext<'tcx>, + has_sig_drop: bool, + sig_drop_checker: SigDropChecker<'cx, 'sdt, 'tcx>, +} + +impl<'cx, 'sdt, 'tcx> SigDropFinder<'cx, 'sdt, 'tcx> { + fn new(cx: &'cx LateContext<'tcx>, seen_types: &'sdt mut FxHashSet>) -> Self { + Self { + cx, + has_sig_drop: false, + sig_drop_checker: SigDropChecker::new(cx, seen_types), + } + } +} + +impl<'cx, 'sdt, 'tcx> Visitor<'tcx> for SigDropFinder<'cx, 'sdt, 'tcx> { + fn visit_expr(&mut self, ex: &'tcx hir::Expr<'_>) { + if self + .sig_drop_checker + .has_sig_drop_attr(self.cx.typeck_results().expr_ty(ex)) + { + self.has_sig_drop = true; + return; + } + + match ex.kind { + hir::ExprKind::MethodCall(_, expr, ..) => { + self.visit_expr(expr); + }, + hir::ExprKind::Array(..) + | hir::ExprKind::Assign(..) + | hir::ExprKind::AssignOp(..) + | hir::ExprKind::Binary(..) + | hir::ExprKind::Box(..) + | hir::ExprKind::Call(..) + | hir::ExprKind::Field(..) + | hir::ExprKind::If(..) + | hir::ExprKind::Index(..) + | hir::ExprKind::Match(..) + | hir::ExprKind::Repeat(..) + | hir::ExprKind::Ret(..) + | hir::ExprKind::Tup(..) + | hir::ExprKind::Unary(..) + | hir::ExprKind::Yield(..) => { + walk_expr(self, ex); + }, + hir::ExprKind::AddrOf(_, _, _) + | hir::ExprKind::Block(_, _) + | hir::ExprKind::Break(_, _) + | hir::ExprKind::Cast(_, _) + | hir::ExprKind::Closure { .. } + | hir::ExprKind::ConstBlock(_) + | hir::ExprKind::Continue(_) + | hir::ExprKind::DropTemps(_) + | hir::ExprKind::Err + | hir::ExprKind::InlineAsm(_) + | hir::ExprKind::Let(_) + | hir::ExprKind::Lit(_) + | hir::ExprKind::Loop(_, _, _, _) + | hir::ExprKind::Path(_) + | hir::ExprKind::Struct(_, _, _) + | hir::ExprKind::Type(_, _) => { + return; + }, + } + } +} diff --git a/tests/ui/significant_drop_tightening.rs b/tests/ui/significant_drop_tightening.rs new file mode 100644 index 000000000000..22ced86dadbe --- /dev/null +++ b/tests/ui/significant_drop_tightening.rs @@ -0,0 +1,16 @@ +#![warn(clippy::significant_drop_tightening)] + +use std::sync::Mutex; + +fn unnecessary_contention_with_multiple_owned_results() { + let mutex = Mutex::new(1i32); + let lock = mutex.lock().unwrap(); + let rslt0 = lock.abs(); + let rslt1 = lock.is_positive(); + do_heavy_computation_that_takes_time((rslt0, rslt1)); +} + +// Marker used for illustration purposes. +fn do_heavy_computation_that_takes_time(_: T) {} + +fn main() {} diff --git a/tests/ui/significant_drop_tightening.stderr b/tests/ui/significant_drop_tightening.stderr new file mode 100644 index 000000000000..07f1edeeeaf3 --- /dev/null +++ b/tests/ui/significant_drop_tightening.stderr @@ -0,0 +1,24 @@ +error: temporary with significant `Drop` can be early dropped + --> $DIR/significant_drop_tightening.rs:7:9 + | +LL | fn unnecessary_contention_with_multiple_owned_results() { + | _________________________________________________________- +LL | | let mutex = Mutex::new(1i32); +LL | | let lock = mutex.lock().unwrap(); + | | ^^^^ +LL | | let rslt0 = lock.abs(); +LL | | let rslt1 = lock.is_positive(); +LL | | do_heavy_computation_that_takes_time((rslt0, rslt1)); +LL | | } + | |_- temporary `lock` is currently being dropped at the end of its contained scope + | + = note: this might lead to unnecessary resource contention + = note: `-D clippy::significant-drop-tightening` implied by `-D warnings` +help: Drop the temporary after the end of its last usage + | +LL ~ let rslt1 = lock.is_positive(); +LL + drop(lock); + | + +error: aborting due to previous error +