From 25d211ad820ade6cadf1afa957baaaace0db8fd7 Mon Sep 17 00:00:00 2001 From: Jason Newcomb Date: Thu, 18 Feb 2021 21:10:43 -0500 Subject: [PATCH] Code cleanup and additional std types checked --- clippy_lints/src/matches.rs | 237 +++++++++++++++++------------------- 1 file changed, 110 insertions(+), 127 deletions(-) diff --git a/clippy_lints/src/matches.rs b/clippy_lints/src/matches.rs index 50fe64d8f2c7..a1cfd2c9d291 100644 --- a/clippy_lints/src/matches.rs +++ b/clippy_lints/src/matches.rs @@ -1707,14 +1707,17 @@ mod redundant_pattern_match { use super::REDUNDANT_PATTERN_MATCHING; use clippy_utils::diagnostics::span_lint_and_then; use clippy_utils::source::{snippet, snippet_with_applicability}; - use clippy_utils::ty::{is_type_diagnostic_item, is_type_lang_item}; + use clippy_utils::ty::{is_type_diagnostic_item, is_type_lang_item, match_type}; use clippy_utils::{implements_trait, is_trait_method, match_qpath, match_trait_method, paths}; use if_chain::if_chain; use rustc_ast::ast::LitKind; use rustc_errors::Applicability; - use rustc_hir::{Arm, Expr, ExprKind, LangItem, MatchSource, Node, PatKind, QPath, UnOp}; + use rustc_hir::{ + intravisit::{walk_expr, ErasedMap, NestedVisitorMap, Visitor}, + Arm, Block, Expr, ExprKind, LangItem, MatchSource, Node, PatKind, QPath, + }; use rustc_lint::LateContext; - use rustc_middle::ty::{self, subst::GenericArgKind, Ty, TyCtxt, TypeckResults}; + use rustc_middle::ty::{self, subst::GenericArgKind, Ty}; use rustc_span::sym; pub fn check<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) { @@ -1733,9 +1736,8 @@ mod redundant_pattern_match { // Check if the drop order for a type matters fn type_needs_ordered_drop(cx: &LateContext<'tcx>, ty: Ty<'tcx>) -> bool { if !ty.needs_drop(cx.tcx, cx.param_env) { - return false; - } - if cx + false + } else if cx .tcx .lang_items() .drop_trait() @@ -1743,7 +1745,7 @@ mod redundant_pattern_match { { // This type doesn't implement drop, so no side effects here. // Check if any component type has any. - return match ty.kind() { + match ty.kind() { ty::Tuple(_) => ty.tuple_fields().any(|ty| type_needs_ordered_drop(cx, ty)), ty::Array(ty, _) => type_needs_ordered_drop(cx, ty), ty::Adt(adt, subs) => adt @@ -1751,16 +1753,25 @@ mod redundant_pattern_match { .map(|f| f.ty(cx.tcx, subs)) .any(|ty| type_needs_ordered_drop(cx, ty)), _ => true, - }; + } } - // Check for std types which implement drop, but only for memory allocation. - if is_type_diagnostic_item(cx, ty, sym::vec_type) + else if is_type_diagnostic_item(cx, ty, sym::vec_type) || 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) + || match_type(cx, ty, &paths::BTREEMAP) + || match_type(cx, ty, &paths::LINKED_LIST) + || match_type(cx, ty, &paths::WEAK_RC) + || match_type(cx, ty, &paths::WEAK_ARC) { - try_get_generic_ty(ty, 0).map_or(true, |ty| type_needs_ordered_drop(cx, ty)) + // Check all of the generic arguments. + if let ty::Adt(_, subs) = ty.kind() { + subs.types().any(|ty| type_needs_ordered_drop(cx, ty)) + } else { + true + } } else { true } @@ -1780,113 +1791,84 @@ mod redundant_pattern_match { } } - // Gets all the the temporaries in an expression which need to be dropped afterwards. - #[allow(clippy::too_many_lines)] - fn get_temporary_tys(cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) -> Vec> { - fn f( - addr_of: bool, - tcx: TyCtxt<'tcx>, - typeck: &TypeckResults<'tcx>, - expr: &'tcx Expr<'tcx>, - result: &mut Vec>, - ) { - match expr.kind { - ExprKind::ConstBlock(_) - | ExprKind::Tup([]) - | ExprKind::Array([]) - | ExprKind::Lit(_) - | ExprKind::Path(_) - | ExprKind::Assign(..) - | ExprKind::AssignOp(..) - | ExprKind::Break(..) - | ExprKind::Continue(_) - | ExprKind::Ret(_) - | ExprKind::InlineAsm(_) - | ExprKind::LlvmInlineAsm(_) - | ExprKind::Yield(..) - | ExprKind::Err - | ExprKind::MethodCall(_, _, [], _) => (), - ExprKind::MethodCall(_, _, [self_arg, args @ ..], _) => { - if addr_of { - result.push(typeck.expr_ty(expr)); - } - let ref_self = typeck - .type_dependent_def_id(expr.hir_id) - .map_or(false, |id| tcx.fn_sig(id).skip_binder().inputs()[0].is_ref()); - f(ref_self, tcx, typeck, self_arg, result); - for arg in args { - f(false, tcx, typeck, arg, result); - } - }, - ExprKind::AddrOf(_, _, expr) | ExprKind::Field(expr, _) => f(true, tcx, typeck, expr, result), - ExprKind::Box(expr) | ExprKind::Cast(expr, _) => f(false, tcx, typeck, expr, result), - ExprKind::Array(exprs @ [_, ..]) | ExprKind::Tup(exprs @ [_, ..]) | ExprKind::Call(_, exprs) => { - if addr_of { - result.push(typeck.expr_ty(expr)); - } - for expr in exprs { - f(false, tcx, typeck, expr, result); - } - }, - ExprKind::Binary(_, lhs, rhs) => { - if addr_of { - result.push(typeck.expr_ty(expr)); - } - f(false, tcx, typeck, lhs, result); - f(false, tcx, typeck, rhs, result); - }, - ExprKind::Unary(UnOp::UnDeref, e) => { - f(typeck.expr_ty(e).is_ref(), tcx, typeck, e, result); - }, - ExprKind::Type(e, _) | ExprKind::Unary(_, e) => { - if addr_of { - result.push(typeck.expr_ty(expr)); - } - f(false, tcx, typeck, e, result); - }, - ExprKind::Index(base, index) => { - f(true, tcx, typeck, base, result); - f(false, tcx, typeck, index, result); - }, - ExprKind::DropTemps(_) | ExprKind::Closure(..) => { - if addr_of { - result.push(typeck.expr_ty(expr)); - } - }, - ExprKind::Match(e, _, _) => { - if addr_of { - result.push(typeck.expr_ty(expr)); - } - f(true, tcx, typeck, e, result); - }, - ExprKind::Block(b, _) | ExprKind::Loop(b, _, _) => { - if addr_of { - result.push(typeck.expr_ty(expr)); - } - if let Some(expr) = b.expr { - f(false, tcx, typeck, expr, result); - } - }, - ExprKind::Struct(_, fields, _) => { - if addr_of { - result.push(typeck.expr_ty(expr)); - } - for field in fields { - f(false, tcx, typeck, field.expr, result); - } - }, - ExprKind::Repeat(expr, _) => { - if addr_of { - result.push(typeck.expr_ty(expr)); - } - f(false, tcx, typeck, expr, result); - }, + // Checks if there are any temporaries created in the given expression for which drop order + // matters. + fn temporaries_need_ordered_drop(cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) -> bool { + struct V<'a, 'tcx> { + cx: &'a LateContext<'tcx>, + res: bool, + } + impl<'a, 'tcx> Visitor<'tcx> for V<'a, 'tcx> { + type Map = ErasedMap<'tcx>; + fn nested_visit_map(&mut self) -> NestedVisitorMap { + NestedVisitorMap::None + } + + fn visit_expr(&mut self, expr: &'tcx Expr<'tcx>) { + match expr.kind { + // Taking the reference of a value leaves a temporary + // e.g. In `&String::new()` the string is a temporary value. + // Remaining fields are temporary values + // 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)) { + self.res = true; + } else { + self.visit_expr(expr); + } + } + }, + // the base type is alway taken by reference. + // 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)) { + self.res = true; + } else { + self.visit_expr(base); + } + } + self.visit_expr(index); + }, + // Method calls can take self by reference. + // e.g. In `String::new().len()` the string is a temporary value. + ExprKind::MethodCall(_, _, [self_arg, args @ ..], _) => { + if !matches!(self_arg.kind, ExprKind::Path(_)) { + let self_by_ref = self + .cx + .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)) + { + self.res = true; + } else { + self.visit_expr(self_arg) + } + } + args.iter().for_each(|arg| self.visit_expr(arg)); + }, + // Either explicitly drops values, or changes control flow. + ExprKind::DropTemps(_) + | ExprKind::Ret(_) + | ExprKind::Break(..) + | ExprKind::Yield(..) + | ExprKind::Block(Block { expr: None, .. }, _) + | ExprKind::Loop(..) => (), + + // Only consider the final expression. + ExprKind::Block(Block { expr: Some(expr), .. }, _) => self.visit_expr(expr), + + _ => walk_expr(self, expr), + } } } - let mut res = Vec::new(); - f(false, cx.tcx, cx.typeck_results(), expr, &mut res); - res + let mut v = V { cx, res: false }; + v.visit_expr(expr); + v.res } fn find_sugg_for_if_let<'tcx>( @@ -1904,6 +1886,8 @@ mod redundant_pattern_match { kind = &inner.kind; } let op_ty = cx.typeck_results().expr_ty(op); + // Determine which function should be used, and the type contained by the corresponding + // variant. let (good_method, inner_ty) = match kind { PatKind::TupleStruct(ref path, ref patterns, _) if patterns.len() == 1 => { if let PatKind::Wild = patterns[0].kind { @@ -1934,29 +1918,28 @@ mod redundant_pattern_match { } else { return; }; - // drop for `None` and `Pending` do nothing + // `None` and `Pending` don't have an inner type. (method, cx.tcx.types.unit) }, _ => return, }; - // if this is the last expression in a block then the lifetime of the expression is extended - // past the end of the block + // If this is the last expression in a block or there is an else clause then the whole + // type needs to be considered, not just the inner type of the branch being matched on. + // Note the last expression in a block is dropped after all local bindings. let check_ty = if has_else - || (keyword == "if" && { - let map = cx.tcx.hir(); - let id = map.get_parent_node(expr.hir_id); - id != expr.hir_id && matches!(map.find(id), Some(Node::Block(..))) - }) { + || (keyword == "if" && matches!(cx.tcx.hir().parent_iter(expr.hir_id).next(), Some((_, Node::Block(..))))) + { op_ty } else { inner_ty }; - let needs_drop = type_needs_ordered_drop(cx, check_ty) - || get_temporary_tys(cx, op) - .iter() - .any(|ty| type_needs_ordered_drop(cx, ty)); + // All temporaries created in the scrutinee expression are dropped at the same time as the + // 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, op); // check that `while_let_on_iterator` lint does not trigger if_chain! {