Auto merge of rust-lang#8666 - Jarcho:while_let_loop_7913, r=dswij

Don't lint `while_let_loop` when significant drop order would change

fixes rust-lang#7226
fixes rust-lang#7913
fixes rust-lang#5717

For rust-lang#5717 it may not stay fully fixed. This is only completely fixed right now due to all the allowed drop impls have `#[may_dangle]` on their drop impls. This may get changed in the future based on how significant drops are determined, but the example listed with `RefCell` shouldn't break.

changelog: Don't lint `while_let_loop` when the order of significant drops would change

Author: bors

clippy_lints/src/loops/while_let_loop.rs

@@ -2,71 +2,60 @@ use super::WHILE_LET_LOOP;
 use clippy_utils::diagnostics::span_lint_and_sugg;
 use clippy_utils::higher;
 use clippy_utils::source::snippet_with_applicability;
+use clippy_utils::ty::needs_ordered_drop;
+use clippy_utils::visitors::any_temporaries_need_ordered_drop;
 use rustc_errors::Applicability;
-use rustc_hir::{Block, Expr, ExprKind, MatchSource, Pat, StmtKind};
-use rustc_lint::{LateContext, LintContext};
-use rustc_middle::lint::in_external_macro;
+use rustc_hir::{Block, Expr, ExprKind, Local, MatchSource, Pat, StmtKind};
+use rustc_lint::LateContext;
 
 pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>, loop_block: &'tcx Block<'_>) {
-    // extract the expression from the first statement (if any) in a block
-    let inner_stmt_expr = extract_expr_from_first_stmt(loop_block);
-    // or extract the first expression (if any) from the block
-    if let Some(inner) = inner_stmt_expr.or_else(|| extract_first_expr(loop_block)) {
-        if let Some(higher::IfLet {
-            let_pat,
-            let_expr,
-            if_else: Some(if_else),
-            ..
-        }) = higher::IfLet::hir(cx, inner)
-        {
-            if is_simple_break_expr(if_else) {
-                could_be_while_let(cx, expr, let_pat, let_expr);
+    let (init, has_trailing_exprs) = match (loop_block.stmts, loop_block.expr) {
+        ([stmt, stmts @ ..], expr) => {
+            if let StmtKind::Local(&Local { init: Some(e), .. }) | StmtKind::Semi(e) | StmtKind::Expr(e) = stmt.kind {
+                (e, !stmts.is_empty() || expr.is_some())
+            } else {
+                return;
             }
-        }
-
-        if let ExprKind::Match(matchexpr, arms, MatchSource::Normal) = inner.kind {
-            if arms.len() == 2
-                && arms[0].guard.is_none()
-                && arms[1].guard.is_none()
-                && is_simple_break_expr(arms[1].body)
-            {
-                could_be_while_let(cx, expr, arms[0].pat, matchexpr);
-            }
-        }
-    }
-}
+        },
+        ([], Some(e)) => (e, false),
+        _ => return,
+    };
 
-/// If a block begins with a statement (possibly a `let` binding) and has an
-/// expression, return it.
-fn extract_expr_from_first_stmt<'tcx>(block: &Block<'tcx>) -> Option<&'tcx Expr<'tcx>> {
-    if let Some(first_stmt) = block.stmts.get(0) {
-        if let StmtKind::Local(local) = first_stmt.kind {
-            return local.init;
-        }
+    if let Some(if_let) = higher::IfLet::hir(cx, init)
+        && let Some(else_expr) = if_let.if_else
+        && is_simple_break_expr(else_expr)
+    {
+        could_be_while_let(cx, expr, if_let.let_pat, if_let.let_expr, has_trailing_exprs);
+    } else if let ExprKind::Match(scrutinee, [arm1, arm2], MatchSource::Normal) = init.kind
+        && arm1.guard.is_none()
+        && arm2.guard.is_none()
+        && is_simple_break_expr(arm2.body)
+    {
+        could_be_while_let(cx, expr, arm1.pat, scrutinee, has_trailing_exprs);
     }
-    None
 }
 
-/// If a block begins with an expression (with or without semicolon), return it.
-fn extract_first_expr<'tcx>(block: &Block<'tcx>) -> Option<&'tcx Expr<'tcx>> {
-    match block.expr {
-        Some(expr) if block.stmts.is_empty() => Some(expr),
-        None if !block.stmts.is_empty() => match block.stmts[0].kind {
-            StmtKind::Expr(expr) | StmtKind::Semi(expr) => Some(expr),
-            StmtKind::Local(..) | StmtKind::Item(..) => None,
-        },
-        _ => None,
-    }
+/// Returns `true` if expr contains a single break expression without a label or eub-expression.
+fn is_simple_break_expr(e: &Expr<'_>) -> bool {
+    matches!(peel_blocks(e).kind, ExprKind::Break(dest, None) if dest.label.is_none())
 }
 
-/// Returns `true` if expr contains a single break expr without destination label
-/// and
-/// passed expression. The expression may be within a block.
-fn is_simple_break_expr(expr: &Expr<'_>) -> bool {
-    match expr.kind {
-        ExprKind::Break(dest, ref passed_expr) if dest.label.is_none() && passed_expr.is_none() => true,
-        ExprKind::Block(b, _) => extract_first_expr(b).map_or(false, is_simple_break_expr),
-        _ => false,
+/// Removes any blocks containing only a single expression.
+fn peel_blocks<'tcx>(e: &'tcx Expr<'tcx>) -> &'tcx Expr<'tcx> {
+    if let ExprKind::Block(b, _) = e.kind {
+        match (b.stmts, b.expr) {
+            ([s], None) => {
+                if let StmtKind::Expr(e) | StmtKind::Semi(e) = s.kind {
+                    peel_blocks(e)
+                } else {
+                    e
+                }
+            },
+            ([], Some(e)) => peel_blocks(e),
+            _ => e,
+        }
+    } else {
+        e
     }
 }
 
@@ -75,8 +64,13 @@ fn could_be_while_let<'tcx>(
     expr: &'tcx Expr<'_>,
     let_pat: &'tcx Pat<'_>,
     let_expr: &'tcx Expr<'_>,
+    has_trailing_exprs: bool,
 ) {
-    if in_external_macro(cx.sess(), expr.span) {
+    if has_trailing_exprs
+        && (needs_ordered_drop(cx, cx.typeck_results().expr_ty(let_expr))
+            || any_temporaries_need_ordered_drop(cx, let_expr))
+    {
+        // Switching to a `while let` loop will extend the lifetime of some values.
         return;
     }
 

clippy_lints/src/matches/redundant_pattern_match.rs

@@ -3,16 +3,13 @@ use clippy_utils::diagnostics::span_lint_and_then;
 use clippy_utils::source::snippet;
 use clippy_utils::sugg::Sugg;
 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 clippy_utils::visitors::any_temporaries_need_ordered_drop;
+use clippy_utils::{higher, is_lang_ctor, is_trait_method, match_def_path, paths};
 use if_chain::if_chain;
 use rustc_ast::ast::LitKind;
 use rustc_errors::Applicability;
 use rustc_hir::LangItem::{OptionNone, PollPending};
-use rustc_hir::{
-    intravisit::{walk_expr, Visitor},
-    Arm, Block, Expr, ExprKind, Node, Pat, PatKind, QPath, UnOp,
-};
+use rustc_hir::{Arm, Expr, ExprKind, Node, Pat, PatKind, QPath, UnOp};
 use rustc_lint::LateContext;
 use rustc_middle::ty::{self, subst::GenericArgKind, DefIdTree, Ty};
 use rustc_span::sym;
@@ -47,79 +44,6 @@ fn try_get_generic_ty(ty: Ty<'_>, index: usize) -> Option<Ty<'_>> {
     }
 }
 
-// Checks if there are any temporaries created in the given expression for which drop order
-// matters.
-fn temporaries_need_ordered_drop<'tcx>(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> {
-        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 needs_ordered_drop(self.cx, self.cx.typeck_results().expr_ty(expr)) {
-                            self.res = true;
-                        } else {
-                            self.visit_expr(expr);
-                        }
-                    }
-                },
-                // the base type is always 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 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 && 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<'_>,
@@ -191,7 +115,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 = needs_ordered_drop(cx, check_ty) || temporaries_need_ordered_drop(cx, let_expr);
+    let needs_drop = needs_ordered_drop(cx, check_ty) || any_temporaries_need_ordered_drop(cx, let_expr);
 
     // check that `while_let_on_iterator` lint does not trigger
     if_chain! {

clippy_utils/src/visitors.rs

@@ -1,16 +1,18 @@
+use crate::ty::needs_ordered_drop;
 use crate::{get_enclosing_block, path_to_local_id};
 use core::ops::ControlFlow;
 use rustc_hir as hir;
-use rustc_hir::def::{DefKind, Res};
+use rustc_hir::def::{CtorKind, DefKind, Res};
 use rustc_hir::intravisit::{self, walk_block, walk_expr, Visitor};
 use rustc_hir::{
-    Arm, Block, BlockCheckMode, Body, BodyId, Expr, ExprKind, HirId, ItemId, ItemKind, Stmt, UnOp, UnsafeSource,
-    Unsafety,
+    Arm, Block, BlockCheckMode, Body, BodyId, Expr, ExprKind, HirId, ItemId, ItemKind, Let, QPath, Stmt, UnOp,
+    UnsafeSource, Unsafety,
 };
 use rustc_lint::LateContext;
 use rustc_middle::hir::map::Map;
 use rustc_middle::hir::nested_filter;
-use rustc_middle::ty;
+use rustc_middle::ty::adjustment::Adjust;
+use rustc_middle::ty::{self, Ty, TypeckResults};
 
 /// Convenience method for creating a `Visitor` with just `visit_expr` overridden and nested
 /// bodies (i.e. closures) are visited.
@@ -494,3 +496,124 @@ pub fn for_each_local_use_after_expr<'tcx, B>(
         ControlFlow::Continue(())
     }
 }
+
+// Calls the given function for every unconsumed temporary created by the expression. Note the
+// function is only guaranteed to be called for types which need to be dropped, but it may be called
+// for other types.
+pub fn for_each_unconsumed_temporary<'tcx, B>(
+    cx: &LateContext<'tcx>,
+    e: &'tcx Expr<'tcx>,
+    mut f: impl FnMut(Ty<'tcx>) -> ControlFlow<B>,
+) -> ControlFlow<B> {
+    // Todo: Handle partially consumed values.
+    fn helper<'tcx, B>(
+        typeck: &'tcx TypeckResults<'tcx>,
+        consume: bool,
+        e: &'tcx Expr<'tcx>,
+        f: &mut impl FnMut(Ty<'tcx>) -> ControlFlow<B>,
+    ) -> ControlFlow<B> {
+        if !consume
+            || matches!(
+                typeck.expr_adjustments(e),
+                [adjust, ..] if matches!(adjust.kind, Adjust::Borrow(_) | Adjust::Deref(_))
+            )
+        {
+            match e.kind {
+                ExprKind::Path(QPath::Resolved(None, p))
+                    if matches!(p.res, Res::Def(DefKind::Ctor(_, CtorKind::Const), _)) =>
+                {
+                    f(typeck.expr_ty(e))?;
+                },
+                ExprKind::Path(_)
+                | ExprKind::Unary(UnOp::Deref, _)
+                | ExprKind::Index(..)
+                | ExprKind::Field(..)
+                | ExprKind::AddrOf(..) => (),
+                _ => f(typeck.expr_ty(e))?,
+            }
+        }
+        match e.kind {
+            ExprKind::AddrOf(_, _, e)
+            | ExprKind::Field(e, _)
+            | ExprKind::Unary(UnOp::Deref, e)
+            | ExprKind::Match(e, ..)
+            | ExprKind::Let(&Let { init: e, .. }) => {
+                helper(typeck, false, e, f)?;
+            },
+            ExprKind::Block(&Block { expr: Some(e), .. }, _)
+            | ExprKind::Box(e)
+            | ExprKind::Cast(e, _)
+            | ExprKind::Unary(_, e) => {
+                helper(typeck, true, e, f)?;
+            },
+            ExprKind::Call(callee, args) => {
+                helper(typeck, true, callee, f)?;
+                for arg in args {
+                    helper(typeck, true, arg, f)?;
+                }
+            },
+            ExprKind::MethodCall(_, args, _) | ExprKind::Tup(args) | ExprKind::Array(args) => {
+                for arg in args {
+                    helper(typeck, true, arg, f)?;
+                }
+            },
+            ExprKind::Index(borrowed, consumed)
+            | ExprKind::Assign(borrowed, consumed, _)
+            | ExprKind::AssignOp(_, borrowed, consumed) => {
+                helper(typeck, false, borrowed, f)?;
+                helper(typeck, true, consumed, f)?;
+            },
+            ExprKind::Binary(_, lhs, rhs) => {
+                helper(typeck, true, lhs, f)?;
+                helper(typeck, true, rhs, f)?;
+            },
+            ExprKind::Struct(_, fields, default) => {
+                for field in fields {
+                    helper(typeck, true, field.expr, f)?;
+                }
+                if let Some(default) = default {
+                    helper(typeck, false, default, f)?;
+                }
+            },
+            ExprKind::If(cond, then, else_expr) => {
+                helper(typeck, true, cond, f)?;
+                helper(typeck, true, then, f)?;
+                if let Some(else_expr) = else_expr {
+                    helper(typeck, true, else_expr, f)?;
+                }
+            },
+            ExprKind::Type(e, _) => {
+                helper(typeck, consume, e, f)?;
+            },
+
+            // Either drops temporaries, jumps out of the current expression, or has no sub expression.
+            ExprKind::DropTemps(_)
+            | ExprKind::Ret(_)
+            | ExprKind::Break(..)
+            | ExprKind::Yield(..)
+            | ExprKind::Block(..)
+            | ExprKind::Loop(..)
+            | ExprKind::Repeat(..)
+            | ExprKind::Lit(_)
+            | ExprKind::ConstBlock(_)
+            | ExprKind::Closure { .. }
+            | ExprKind::Path(_)
+            | ExprKind::Continue(_)
+            | ExprKind::InlineAsm(_)
+            | ExprKind::Err => (),
+        }
+        ControlFlow::Continue(())
+    }
+    helper(cx.typeck_results(), true, e, &mut f)
+}
+
+pub fn any_temporaries_need_ordered_drop<'tcx>(cx: &LateContext<'tcx>, e: &'tcx Expr<'tcx>) -> bool {
+    for_each_unconsumed_temporary(cx, e, |ty| {
+        if needs_ordered_drop(cx, ty) {
+            ControlFlow::Break(())
+        } else {
+            ControlFlow::Continue(())
+        }
+    })
+    .is_break()
+}