Skip to content

Commit

Permalink
needless_late_init: ignore if let, let mut and significant drops
Browse files Browse the repository at this point in the history
  • Loading branch information
@Alexendoo
Alexendoo committed Apr 26, 2022
1 parent 9938daf commit 1d1fecf
Show file tree
Hide file tree
Showing 8 changed files with 221 additions and 139 deletions.
66 changes: 6 additions & 60 deletions clippy_lints/src/matches/redundant_pattern_match.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,17 +2,16 @@ use super::REDUNDANT_PATTERN_MATCHING;
use clippy_utils::diagnostics::span_lint_and_then;
use clippy_utils::source::snippet;
use clippy_utils::sugg::Sugg;
use clippy_utils::ty::{implements_trait, is_type_diagnostic_item, is_type_lang_item, match_type};
use clippy_utils::ty::needs_ordered_drop;
use clippy_utils::{higher, match_def_path};
use clippy_utils::{is_lang_ctor, is_trait_method, paths};
use if_chain::if_chain;
use rustc_ast::ast::LitKind;
use rustc_data_structures::fx::FxHashSet;
use rustc_errors::Applicability;
use rustc_hir::LangItem::{OptionNone, PollPending};
use rustc_hir::{
intravisit::{walk_expr, Visitor},
Arm, Block, Expr, ExprKind, LangItem, Node, Pat, PatKind, QPath, UnOp,
Arm, Block, Expr, ExprKind, Node, Pat, PatKind, QPath, UnOp,
};
use rustc_lint::LateContext;
use rustc_middle::ty::{self, subst::GenericArgKind, DefIdTree, Ty};
Expand All @@ -32,59 +31,6 @@ pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) {
}
}

/// Checks if the drop order for a type matters. Some std types implement drop solely to
/// deallocate memory. For these types, and composites containing them, changing the drop order
/// won't result in any observable side effects.
fn type_needs_ordered_drop<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>) -> bool {
type_needs_ordered_drop_inner(cx, ty, &mut FxHashSet::default())
}

fn type_needs_ordered_drop_inner<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>, seen: &mut FxHashSet<Ty<'tcx>>) -> bool {
if !seen.insert(ty) {
return false;
}
if !ty.needs_drop(cx.tcx, cx.param_env) {
false
} else if !cx
.tcx
.lang_items()
.drop_trait()
.map_or(false, |id| implements_trait(cx, ty, id, &[]))
{
// This type doesn't implement drop, so no side effects here.
// Check if any component type has any.
match ty.kind() {
ty::Tuple(fields) => fields.iter().any(|ty| type_needs_ordered_drop_inner(cx, ty, seen)),
ty::Array(ty, _) => type_needs_ordered_drop_inner(cx, *ty, seen),
ty::Adt(adt, subs) => adt
.all_fields()
.map(|f| f.ty(cx.tcx, subs))
.any(|ty| type_needs_ordered_drop_inner(cx, ty, seen)),
_ => true,
}
}
// Check for std types which implement drop, but only for memory allocation.
else if is_type_diagnostic_item(cx, ty, sym::Vec)
|| is_type_lang_item(cx, ty, LangItem::OwnedBox)
|| is_type_diagnostic_item(cx, ty, sym::Rc)
|| is_type_diagnostic_item(cx, ty, sym::Arc)
|| is_type_diagnostic_item(cx, ty, sym::cstring_type)
|| is_type_diagnostic_item(cx, ty, sym::BTreeMap)
|| is_type_diagnostic_item(cx, ty, sym::LinkedList)
|| match_type(cx, ty, &paths::WEAK_RC)
|| match_type(cx, ty, &paths::WEAK_ARC)
{
// Check all of the generic arguments.
if let ty::Adt(_, subs) = ty.kind() {
subs.types().any(|ty| type_needs_ordered_drop_inner(cx, ty, seen))
} else {
true
}
} else {
true
}
}

// Extract the generic arguments out of a type
fn try_get_generic_ty(ty: Ty<'_>, index: usize) -> Option<Ty<'_>> {
if_chain! {
Expand Down Expand Up @@ -115,7 +61,7 @@ fn temporaries_need_ordered_drop<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<
// e.g. In `(String::new(), 0).1` the string is a temporary value.
ExprKind::AddrOf(_, _, expr) | ExprKind::Field(expr, _) => {
if !matches!(expr.kind, ExprKind::Path(_)) {
if type_needs_ordered_drop(self.cx, self.cx.typeck_results().expr_ty(expr)) {
if needs_ordered_drop(self.cx, self.cx.typeck_results().expr_ty(expr)) {
self.res = true;
} else {
self.visit_expr(expr);
Expand All @@ -126,7 +72,7 @@ fn temporaries_need_ordered_drop<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<
// e.g. In `(vec![0])[0]` the vector is a temporary value.
ExprKind::Index(base, index) => {
if !matches!(base.kind, ExprKind::Path(_)) {
if type_needs_ordered_drop(self.cx, self.cx.typeck_results().expr_ty(base)) {
if needs_ordered_drop(self.cx, self.cx.typeck_results().expr_ty(base)) {
self.res = true;
} else {
self.visit_expr(base);
Expand All @@ -143,7 +89,7 @@ fn temporaries_need_ordered_drop<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<
.typeck_results()
.type_dependent_def_id(expr.hir_id)
.map_or(false, |id| self.cx.tcx.fn_sig(id).skip_binder().inputs()[0].is_ref());
if self_by_ref && type_needs_ordered_drop(self.cx, self.cx.typeck_results().expr_ty(self_arg)) {
if self_by_ref && needs_ordered_drop(self.cx, self.cx.typeck_results().expr_ty(self_arg)) {
self.res = true;
} else {
self.visit_expr(self_arg);
Expand Down Expand Up @@ -243,7 +189,7 @@ fn find_sugg_for_if_let<'tcx>(
// scrutinee would be, so they have to be considered as well.
// e.g. in `if let Some(x) = foo.lock().unwrap().baz.as_ref() { .. }` the lock will be held
// for the duration if body.
let needs_drop = type_needs_ordered_drop(cx, check_ty) || temporaries_need_ordered_drop(cx, let_expr);
let needs_drop = needs_ordered_drop(cx, check_ty) || temporaries_need_ordered_drop(cx, let_expr);

// check that `while_let_on_iterator` lint does not trigger
if_chain! {
Expand Down
40 changes: 36 additions & 4 deletions clippy_lints/src/needless_late_init.rs
View file
Original file line number Diff line number Diff line change
@@ -1,10 +1,14 @@
use clippy_utils::diagnostics::span_lint_and_then;
use clippy_utils::path_to_local;
use clippy_utils::source::snippet_opt;
use clippy_utils::visitors::{expr_visitor, is_local_used};
use clippy_utils::ty::needs_ordered_drop;
use clippy_utils::visitors::{expr_visitor, expr_visitor_no_bodies, is_local_used};
use rustc_errors::Applicability;
use rustc_hir::intravisit::Visitor;
use rustc_hir::{Block, Expr, ExprKind, HirId, Local, LocalSource, MatchSource, Node, Pat, PatKind, Stmt, StmtKind};
use rustc_hir::{
BindingAnnotation, Block, Expr, ExprKind, HirId, Local, LocalSource, MatchSource, Node, Pat, PatKind, Stmt,
StmtKind,
};
use rustc_lint::{LateContext, LateLintPass};
use rustc_session::{declare_lint_pass, declare_tool_lint};
use rustc_span::Span;
Expand Down Expand Up @@ -73,6 +77,31 @@ fn contains_assign_expr<'tcx>(cx: &LateContext<'tcx>, stmt: &'tcx Stmt<'tcx>) ->
seen
}

fn contains_let(cond: &Expr<'_>) -> bool {
let mut seen = false;
expr_visitor_no_bodies(|expr| {
if let ExprKind::Let(_) = expr.kind {
seen = true;
}

!seen
})
.visit_expr(cond);

seen
}

fn stmt_needs_ordered_drop(cx: &LateContext<'_>, stmt: &Stmt<'_>) -> bool {
let StmtKind::Local(local) = stmt.kind else { return false };
!local.pat.walk_short(|pat| {
if let PatKind::Binding(.., None) = pat.kind {
!needs_ordered_drop(cx, cx.typeck_results().pat_ty(pat))
} else {
true
}
})
}

#[derive(Debug)]
struct LocalAssign {
lhs_id: HirId,
Expand Down Expand Up @@ -187,11 +216,14 @@ fn first_usage<'tcx>(
local_stmt_id: HirId,
block: &'tcx Block<'tcx>,
) -> Option<Usage<'tcx>> {
let significant_drop = needs_ordered_drop(cx, cx.typeck_results().node_type(binding_id));

block
.stmts
.iter()
.skip_while(|stmt| stmt.hir_id != local_stmt_id)
.skip(1)
.take_while(|stmt| !significant_drop || !stmt_needs_ordered_drop(cx, stmt))
.find(|&stmt| is_local_used(cx, stmt, binding_id))
.and_then(|stmt| match stmt.kind {
StmtKind::Expr(expr) => Some(Usage {
Expand Down Expand Up @@ -258,7 +290,7 @@ fn check<'tcx>(
},
);
},
ExprKind::If(_, then_expr, Some(else_expr)) => {
ExprKind::If(cond, then_expr, Some(else_expr)) if !contains_let(cond) => {
let (applicability, suggestions) = assignment_suggestions(cx, binding_id, [then_expr, else_expr])?;

span_lint_and_then(
Expand Down Expand Up @@ -338,7 +370,7 @@ impl<'tcx> LateLintPass<'tcx> for NeedlessLateInit {
if let Local {
init: None,
pat: &Pat {
kind: PatKind::Binding(_, binding_id, _, None),
kind: PatKind::Binding(BindingAnnotation::Unannotated, binding_id, _, None),
..
},
source: LocalSource::Normal,
Expand Down
71 changes: 68 additions & 3 deletions clippy_utils/src/ty.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,11 @@
#![allow(clippy::module_name_repetitions)]

use rustc_ast::ast::Mutability;
use rustc_data_structures::fx::FxHashMap;
use rustc_data_structures::fx::{FxHashMap, FxHashSet};
use rustc_hir as hir;
use rustc_hir::def::{CtorKind, DefKind, Res};
use rustc_hir::def_id::DefId;
use rustc_hir::{Expr, TyKind, Unsafety};
use rustc_hir::{Expr, LangItem, TyKind, Unsafety};
use rustc_infer::infer::TyCtxtInferExt;
use rustc_lint::LateContext;
use rustc_middle::mir::interpret::{ConstValue, Scalar};
Expand All @@ -22,7 +22,7 @@ use rustc_trait_selection::infer::InferCtxtExt;
use rustc_trait_selection::traits::query::normalize::AtExt;
use std::iter;

use crate::{match_def_path, must_use_attr, path_res};
use crate::{match_def_path, must_use_attr, path_res, paths};

// Checks if the given type implements copy.
pub fn is_copy<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>) -> bool {
Expand Down Expand Up @@ -83,6 +83,20 @@ pub fn get_associated_type<'tcx>(
})
}

/// Get the diagnostic name of a type, e.g. `sym::HashMap`. To check if a type
/// implements a trait marked with a diagnostic item use [`implements_trait`].
///
/// For a further exploitation what diagnostic items are see [diagnostic items] in
/// rustc-dev-guide.
///
/// [Diagnostic Items]: https://rustc-dev-guide.rust-lang.org/diagnostics/diagnostic-items.html
pub fn get_type_diagnostic_name(cx: &LateContext<'_>, ty: Ty<'_>) -> Option<Symbol> {
match ty.kind() {
ty::Adt(adt, _) => cx.tcx.get_diagnostic_name(adt.did()),
_ => None,
}
}

/// Returns true if ty has `iter` or `iter_mut` methods
pub fn has_iter_method(cx: &LateContext<'_>, probably_ref_ty: Ty<'_>) -> Option<Symbol> {
// FIXME: instead of this hard-coded list, we should check if `<adt>::iter`
Expand Down Expand Up @@ -319,6 +333,57 @@ pub fn match_type(cx: &LateContext<'_>, ty: Ty<'_>, path: &[&str]) -> bool {
}
}

/// Checks if the drop order for a type matters. Some std types implement drop solely to
/// deallocate memory. For these types, and composites containing them, changing the drop order
/// won't result in any observable side effects.
pub fn needs_ordered_drop<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>) -> bool {
fn needs_ordered_drop_inner<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>, seen: &mut FxHashSet<Ty<'tcx>>) -> bool {
if !seen.insert(ty) {
return false;
}
if !ty.has_significant_drop(cx.tcx, cx.param_env) {
false
}
// Check for std types which implement drop, but only for memory allocation.
else if is_type_lang_item(cx, ty, LangItem::OwnedBox)
|| matches!(
get_type_diagnostic_name(cx, ty),
Some(sym::HashSet | sym::Rc | sym::Arc | sym::cstring_type)
)
|| match_type(cx, ty, &paths::WEAK_RC)
|| match_type(cx, ty, &paths::WEAK_ARC)
{
// Check all of the generic arguments.
if let ty::Adt(_, subs) = ty.kind() {
subs.types().any(|ty| needs_ordered_drop_inner(cx, ty, seen))
} else {
true
}
} else if !cx
.tcx
.lang_items()
.drop_trait()
.map_or(false, |id| implements_trait(cx, ty, id, &[]))
{
// This type doesn't implement drop, so no side effects here.
// Check if any component type has any.
match ty.kind() {
ty::Tuple(fields) => fields.iter().any(|ty| needs_ordered_drop_inner(cx, ty, seen)),
ty::Array(ty, _) => needs_ordered_drop_inner(cx, *ty, seen),
ty::Adt(adt, subs) => adt
.all_fields()
.map(|f| f.ty(cx.tcx, subs))
.any(|ty| needs_ordered_drop_inner(cx, ty, seen)),
_ => true,
}
} else {
true
}
}

needs_ordered_drop_inner(cx, ty, &mut FxHashSet::default())
}

/// Peels off all references on the type. Returns the underlying type and the number of references
/// removed.
pub fn peel_mid_ty_refs(ty: Ty<'_>) -> (Ty<'_>, usize) {
Expand Down
Loading

0 comments on commit 1d1fecf

Please sign in to comment.