Auto merge of #6568 - Jarcho:redundant_pattern_matching, r=flip1995

Fix: redundant_pattern_matching drop order

Fixes #5746

A note about the change in drop order is added when the scrutinee (or any temporary in the expression) isn't known to be safe to drop in any order (i.e. doesn't implement the `Drop` trait, or contain such a type). There is a whitelist for some `std` types, but it's incomplete. Currently just `Vec<_>`, `Box<_>`, `Rc<_>` and `Arc<_>`, but only if the contained type is also safe to drop in any order.

Another lint for when the drop order changes could be added as allowed by default, but the drop order requirement is pretty subtle in this case. I think the note added to the lint should be enough to make someone think before applying the change.

changelog: Added a note to `redundant_pattern_matching` when the change in drop order might matter

Author: bors

clippy_lints/src/matches.rs

@@ -423,7 +423,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:**
     ///
@@ -1703,55 +1708,206 @@ 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::source::{snippet, snippet_with_applicability};
+    use clippy_utils::ty::{implements_trait, is_type_diagnostic_item, is_type_lang_item, match_type};
     use clippy_utils::{is_lang_ctor, is_qpath_def_path, is_trait_method, paths};
     use if_chain::if_chain;
     use rustc_ast::ast::LitKind;
     use rustc_errors::Applicability;
     use rustc_hir::LangItem::{OptionNone, OptionSome, PollPending, PollReady, ResultErr, ResultOk};
-    use rustc_hir::{Arm, Expr, ExprKind, MatchSource, PatKind, QPath};
+    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};
     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, "if"),
-                MatchSource::WhileLetDesugar => find_sugg_for_if_let(cx, expr, op, arms, "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),
                 _ => {},
             }
         }
     }
 
+    /// 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(cx: &LateContext<'tcx>, ty: Ty<'tcx>) -> bool {
+        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(_) => 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.
+        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)
+        {
+            // 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
+        }
+    }
+
+    // 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
+            }
+        }
+    }
+
+    // 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<Self::Map> {
+                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 v = V { cx, res: false };
+        v.visit_expr(expr);
+        v.res
+    }
+
     fn find_sugg_for_if_let<'tcx>(
         cx: &LateContext<'tcx>,
         expr: &'tcx Expr<'_>,
-        op: &Expr<'_>,
-        arms: &[Arm<'_>],
+        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);
+        // 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, [sub_pat], _) => {
                 if let PatKind::Wild = sub_pat.kind {
                     if is_lang_ctor(cx, path, ResultOk) {
-                        "is_ok()"
+                        ("is_ok()", try_get_generic_ty(op_ty, 0).unwrap_or(op_ty))
                     } else if is_lang_ctor(cx, path, ResultErr) {
-                        "is_err()"
+                        ("is_err()", try_get_generic_ty(op_ty, 1).unwrap_or(op_ty))
                     } else if is_lang_ctor(cx, path, OptionSome) {
-                        "is_some()"
+                        ("is_some()", op_ty)
                     } else if is_lang_ctor(cx, path, PollReady) {
-                        "is_ready()"
+                        ("is_ready()", op_ty)
                     } else if is_qpath_def_path(cx, path, sub_pat.hir_id, &paths::IPADDR_V4) {
-                        "is_ipv4()"
+                        ("is_ipv4()", op_ty)
                     } else if is_qpath_def_path(cx, path, sub_pat.hir_id, &paths::IPADDR_V6) {
-                        "is_ipv6()"
+                        ("is_ipv6()", op_ty)
                     } else {
                         return;
                     }
@@ -1760,17 +1916,36 @@ mod redundant_pattern_match {
                 }
             },
             PatKind::Path(ref path) => {
-                if is_lang_ctor(cx, path, OptionNone) {
+                let method = if is_lang_ctor(cx, path, OptionNone) {
                     "is_none()"
                 } else if is_lang_ctor(cx, path, PollPending) {
                     "is_pending()"
                 } else {
                     return;
-                }
+                };
+                // `None` and `Pending` don't have an inner type.
+                (method, cx.tcx.types.unit)
             },
             _ => return,
         };
 
+        // 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" && matches!(cx.tcx.hir().parent_iter(expr.hir_id).next(), Some((_, Node::Block(..)))))
+        {
+            op_ty
+        } else {
+            inner_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! {
             if keyword == "while";
@@ -1789,7 +1964,7 @@ mod redundant_pattern_match {
         span_lint_and_then(
             cx,
             REDUNDANT_PATTERN_MATCHING,
-            arms[0].pat.span,
+            arm.pat.span,
             &format!("redundant pattern matching, consider using `{}`", good_method),
             |diag| {
                 // while let ... = ... { ... }
@@ -1803,12 +1978,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() {}
+}