Always trigger redundant_pattern_matching, but with a note when drop order might matter.

Author: Jarcho

clippy_lints/src/matches.rs

@@ -421,7 +421,12 @@ declare_clippy_lint! {
     /// **Why is this bad?** It's more concise and clear to just use the proper
     /// utility function
     ///
-    /// **Known problems:** None.
+    /// **Known problems:** This will change the drop order for the matched type. Both `if let` and
+    /// `while let` will drop the value at the end of the block, both `if` and `while` will drop the
+    /// value before entering the block. For most types this change will not matter, but for a few
+    /// types this will not be an acceptable change (e.g. locks). See the
+    /// [reference](https://doc.rust-lang.org/reference/destructors.html#drop-scopes) for more about
+    /// drop order.
     ///
     /// **Example:**
     ///
@@ -1701,57 +1706,219 @@ where
 mod redundant_pattern_match {
     use super::REDUNDANT_PATTERN_MATCHING;
     use clippy_utils::diagnostics::span_lint_and_then;
-    use clippy_utils::source::snippet;
-    use clippy_utils::{is_trait_method, match_qpath, paths};
+    use clippy_utils::source::{snippet, snippet_with_applicability};
+    use clippy_utils::ty::{implements_trait, is_type_diagnostic_item, is_type_lang_item};
+    use clippy_utils::{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, MatchSource, PatKind, QPath};
+    use rustc_hir::{Arm, Expr, ExprKind, LangItem, MatchSource, Node, PatKind, QPath, UnOp};
     use rustc_lint::LateContext;
+    use rustc_middle::ty::{self, subst::GenericArgKind, Ty, TyCtxt, TypeckResults};
     use rustc_span::sym;
 
     pub fn check<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) {
         if let ExprKind::Match(op, arms, ref match_source) = &expr.kind {
             match match_source {
                 MatchSource::Normal => find_sugg_for_match(cx, expr, op, arms),
-                MatchSource::IfLetDesugar { .. } => find_sugg_for_if_let(cx, expr, op, &arms[0], "if"),
-                MatchSource::WhileLetDesugar => find_sugg_for_if_let(cx, expr, op, &arms[0], "while"),
+                MatchSource::IfLetDesugar { contains_else_clause } => {
+                    find_sugg_for_if_let(cx, expr, op, &arms[0], "if", *contains_else_clause)
+                },
+                MatchSource::WhileLetDesugar => find_sugg_for_if_let(cx, expr, op, &arms[0], "while", false),
                 _ => {},
             }
         }
     }
 
+    // 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
+            .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.
+            return 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
+                    .all_fields()
+                    .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)
+            || is_type_lang_item(cx, ty, LangItem::OwnedBox)
+            || is_type_diagnostic_item(cx, ty, sym::Rc)
+            || is_type_diagnostic_item(cx, ty, sym::Arc)
+        {
+            try_get_generic_ty(ty, 0).map_or(true, |ty| type_needs_ordered_drop(cx, ty))
+        } else {
+            true
+        }
+    }
+
+    // Extract the generic arguments out of a type
+    fn try_get_generic_ty(ty: Ty<'_>, index: usize) -> Option<Ty<'_>> {
+        if_chain! {
+            if let ty::Adt(_, subs) = ty.kind();
+            if let Some(sub) = subs.get(index);
+            if let GenericArgKind::Type(sub_ty) = sub.unpack();
+            then {
+                Some(sub_ty)
+            } else {
+                None
+            }
+        }
+    }
+
+    // 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<Ty<'tcx>> {
+        fn f(
+            addr_of: bool,
+            tcx: TyCtxt<'tcx>,
+            typeck: &TypeckResults<'tcx>,
+            expr: &'tcx Expr<'tcx>,
+            result: &mut Vec<Ty<'tcx>>,
+        ) {
+            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);
+                },
+            }
+        }
+
+        let mut res = Vec::new();
+        f(false, cx.tcx, cx.typeck_results(), expr, &mut res);
+        res
+    }
+
     fn find_sugg_for_if_let<'tcx>(
         cx: &LateContext<'tcx>,
         expr: &'tcx Expr<'_>,
-        op: &Expr<'_>,
+        op: &'tcx Expr<'tcx>,
         arm: &Arm<'_>,
         keyword: &'static str,
+        has_else: bool,
     ) {
         // also look inside refs
-        let mut kind = &arms[0].pat.kind;
+        let mut kind = arm.pat.kind;
         // if we have &None for example, peel it so we can detect "if let None = x"
         if let PatKind::Ref(inner, _mutability) = kind {
             kind = &inner.kind;
         }
-        let good_method = match kind {
+        let op_ty = cx.typeck_results().expr_ty(op);
+        let (good_method, inner_ty) = match kind {
             PatKind::TupleStruct(ref path, ref patterns, _) if patterns.len() == 1 => {
                 if let PatKind::Wild = patterns[0].kind {
-                    if cx.typeck_results().expr_ty(op).needs_drop(cx.tcx, cx.param_env) {
-                        // if let doesn't drop the value until after the block ends
-                        return;
-                    } else if match_qpath(path, &paths::RESULT_OK) {
-                        "is_ok()"
+                    if match_qpath(path, &paths::RESULT_OK) {
+                        ("is_ok()", try_get_generic_ty(op_ty, 0).unwrap_or(op_ty))
                     } else if match_qpath(path, &paths::RESULT_ERR) {
-                        "is_err()"
+                        ("is_err()", try_get_generic_ty(op_ty, 1).unwrap_or(op_ty))
                     } else if match_qpath(path, &paths::OPTION_SOME) {
-                        "is_some()"
+                        ("is_some()", op_ty)
                     } else if match_qpath(path, &paths::POLL_READY) {
-                        "is_ready()"
+                        ("is_ready()", op_ty)
                     } else if match_qpath(path, &paths::IPADDR_V4) {
-                        "is_ipv4()"
+                        ("is_ipv4()", op_ty)
                     } else if match_qpath(path, &paths::IPADDR_V6) {
-                        "is_ipv6()"
+                        ("is_ipv6()", op_ty)
                     } else {
                         return;
                     }
@@ -1760,17 +1927,37 @@ mod redundant_pattern_match {
                 }
             },
             PatKind::Path(ref path) => {
-                if match_qpath(path, &paths::OPTION_NONE) {
+                let method = if match_qpath(path, &paths::OPTION_NONE) {
                     "is_none()"
                 } else if match_qpath(path, &paths::POLL_PENDING) {
                     "is_pending()"
                 } else {
                     return;
-                }
+                };
+                // drop for `None` and `Pending` do nothing
+                (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
+        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(..)))
+            }) {
+            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));
+
         // check that `while_let_on_iterator` lint does not trigger
         if_chain! {
             if keyword == "while";
@@ -1803,12 +1990,20 @@ mod redundant_pattern_match {
                 // while let ... = ... { ... }
                 // ^^^^^^^^^^^^^^^^^^^
                 let span = expr_span.until(op_span.shrink_to_hi());
-                diag.span_suggestion(
-                    span,
-                    "try this",
-                    format!("{} {}.{}", keyword, snippet(cx, op_span, "_"), good_method),
-                    Applicability::MachineApplicable, // snippet
-                );
+
+                let mut app = if needs_drop {
+                    Applicability::MaybeIncorrect
+                } else {
+                    Applicability::MachineApplicable
+                };
+                let sugg = snippet_with_applicability(cx, op_span, "_", &mut app);
+
+                diag.span_suggestion(span, "try this", format!("{} {}.{}", keyword, sugg, good_method), app);
+
+                if needs_drop {
+                    diag.note("this will change drop order of the result, as well as all temporaries");
+                    diag.note("add `#[allow(clippy::redundant_pattern_matching)]` if this is important");
+                }
             },
         );
     }

tests/ui/redundant_pattern_matching_drop_order.fixed

@@ -0,0 +1,58 @@
+// run-rustfix
+
+// Issue #5746
+#![warn(clippy::redundant_pattern_matching)]
+#![allow(clippy::if_same_then_else)]
+use std::task::Poll::{Pending, Ready};
+
+fn main() {
+    let m = std::sync::Mutex::new((0, 0));
+
+    // Result
+    if m.lock().is_ok() {}
+    if Err::<(), _>(m.lock().unwrap().0).is_err() {}
+
+    {
+        if Ok::<_, std::sync::MutexGuard<()>>(()).is_ok() {}
+    }
+    if Ok::<_, std::sync::MutexGuard<()>>(()).is_ok() {
+    } else {
+    }
+    if Ok::<_, std::sync::MutexGuard<()>>(()).is_ok() {}
+    if Err::<std::sync::MutexGuard<()>, _>(()).is_err() {}
+
+    if Ok::<_, ()>(String::new()).is_ok() {}
+    if Err::<(), _>((String::new(), ())).is_err() {}
+
+    // Option
+    if Some(m.lock()).is_some() {}
+    if Some(m.lock().unwrap().0).is_some() {}
+
+    {
+        if None::<std::sync::MutexGuard<()>>.is_none() {}
+    }
+    if None::<std::sync::MutexGuard<()>>.is_none() {
+    } else {
+    }
+
+    if None::<std::sync::MutexGuard<()>>.is_none() {}
+
+    if Some(String::new()).is_some() {}
+    if Some((String::new(), ())).is_some() {}
+
+    // Poll
+    if Ready(m.lock()).is_ready() {}
+    if Ready(m.lock().unwrap().0).is_ready() {}
+
+    {
+        if Pending::<std::sync::MutexGuard<()>>.is_pending() {}
+    }
+    if Pending::<std::sync::MutexGuard<()>>.is_pending() {
+    } else {
+    }
+
+    if Pending::<std::sync::MutexGuard<()>>.is_pending() {}
+
+    if Ready(String::new()).is_ready() {}
+    if Ready((String::new(), ())).is_ready() {}
+}