Rollup merge of #137444 - compiler-errors:drop-lint, r=oli-obk

Improve behavior of `IF_LET_RESCOPE` around temporaries and place expressions

Heavily reworks the `IF_LET_RESCOPE` to be more sensitive around 1. temporaries that get consumed/terminated and therefore should not trigger the lint, and 2. borrows of place expressions, which are not temporary values.

Fixes #137411

Author: fmease

compiler/rustc_lint/src/if_let_rescope.rs

@@ -1,14 +1,15 @@
 use std::iter::repeat;
 use std::ops::ControlFlow;
 
-use hir::intravisit::Visitor;
+use hir::intravisit::{self, Visitor};
 use rustc_ast::Recovered;
 use rustc_errors::{
     Applicability, Diag, EmissionGuarantee, SubdiagMessageOp, Subdiagnostic, SuggestionStyle,
 };
 use rustc_hir::{self as hir, HirIdSet};
 use rustc_macros::LintDiagnostic;
 use rustc_middle::ty::TyCtxt;
+use rustc_middle::ty::adjustment::Adjust;
 use rustc_session::lint::{FutureIncompatibilityReason, LintId};
 use rustc_session::{declare_lint, impl_lint_pass};
 use rustc_span::Span;
@@ -160,7 +161,7 @@ impl IfLetRescope {
                 let lifetime_end = source_map.end_point(conseq.span);
 
                 if let ControlFlow::Break(significant_dropper) =
-                    (FindSignificantDropper { cx }).visit_expr(init)
+                    (FindSignificantDropper { cx }).check_if_let_scrutinee(init)
                 {
                     first_if_to_lint = first_if_to_lint.or_else(|| Some((span, expr.hir_id)));
                     significant_droppers.push(significant_dropper);
@@ -363,96 +364,97 @@ enum SingleArmMatchBegin {
     WithoutOpenBracket(Span),
 }
 
-struct FindSignificantDropper<'tcx, 'a> {
+struct FindSignificantDropper<'a, 'tcx> {
     cx: &'a LateContext<'tcx>,
 }
 
-impl<'tcx, 'a> Visitor<'tcx> for FindSignificantDropper<'tcx, 'a> {
-    type Result = ControlFlow<Span>;
+impl<'tcx> FindSignificantDropper<'_, 'tcx> {
+    /// Check the scrutinee of an `if let` to see if it promotes any temporary values
+    /// that would change drop order in edition 2024. Specifically, it checks the value
+    /// of the scrutinee itself, and also recurses into the expression to find any ref
+    /// exprs (or autoref) which would promote temporaries that would be scoped to the
+    /// end of this `if`.
+    fn check_if_let_scrutinee(&mut self, init: &'tcx hir::Expr<'tcx>) -> ControlFlow<Span> {
+        self.check_promoted_temp_with_drop(init)?;
+        self.visit_expr(init)
+    }
 
-    fn visit_expr(&mut self, expr: &'tcx hir::Expr<'tcx>) -> Self::Result {
-        if self
+    /// Check that an expression is not a promoted temporary with a significant
+    /// drop impl.
+    ///
+    /// An expression is a promoted temporary if it has an addr taken (i.e. `&expr` or autoref)
+    /// or is the scrutinee of the `if let`, *and* the expression is not a place
+    /// expr, and it has a significant drop.
+    fn check_promoted_temp_with_drop(&self, expr: &'tcx hir::Expr<'tcx>) -> ControlFlow<Span> {
+        if !expr.is_place_expr(|base| {
+            self.cx
+                .typeck_results()
+                .adjustments()
+                .get(base.hir_id)
+                .is_some_and(|x| x.iter().any(|adj| matches!(adj.kind, Adjust::Deref(_))))
+        }) && self
             .cx
             .typeck_results()
             .expr_ty(expr)
             .has_significant_drop(self.cx.tcx, self.cx.typing_env())
         {
-            return ControlFlow::Break(expr.span);
+            ControlFlow::Break(expr.span)
+        } else {
+            ControlFlow::Continue(())
         }
-        match expr.kind {
-            hir::ExprKind::ConstBlock(_)
-            | hir::ExprKind::Lit(_)
-            | hir::ExprKind::Path(_)
-            | hir::ExprKind::Assign(_, _, _)
-            | hir::ExprKind::AssignOp(_, _, _)
-            | hir::ExprKind::Break(_, _)
-            | hir::ExprKind::Continue(_)
-            | hir::ExprKind::Ret(_)
-            | hir::ExprKind::Become(_)
-            | hir::ExprKind::InlineAsm(_)
-            | hir::ExprKind::OffsetOf(_, _)
-            | hir::ExprKind::Repeat(_, _)
-            | hir::ExprKind::Err(_)
-            | hir::ExprKind::Struct(_, _, _)
-            | hir::ExprKind::Closure(_)
-            | hir::ExprKind::Block(_, _)
-            | hir::ExprKind::DropTemps(_)
-            | hir::ExprKind::Loop(_, _, _, _) => ControlFlow::Continue(()),
+    }
+}
 
-            hir::ExprKind::Tup(exprs) | hir::ExprKind::Array(exprs) => {
-                for expr in exprs {
-                    self.visit_expr(expr)?;
-                }
-                ControlFlow::Continue(())
-            }
-            hir::ExprKind::Call(callee, args) => {
-                self.visit_expr(callee)?;
-                for expr in args {
-                    self.visit_expr(expr)?;
-                }
-                ControlFlow::Continue(())
-            }
-            hir::ExprKind::MethodCall(_, receiver, args, _) => {
-                self.visit_expr(receiver)?;
-                for expr in args {
-                    self.visit_expr(expr)?;
+impl<'tcx> Visitor<'tcx> for FindSignificantDropper<'_, 'tcx> {
+    type Result = ControlFlow<Span>;
+
+    fn visit_block(&mut self, b: &'tcx hir::Block<'tcx>) -> Self::Result {
+        // Blocks introduce temporary terminating scope for all of its
+        // statements, so just visit the tail expr, skipping over any
+        // statements. This prevents false positives like `{ let x = &Drop; }`.
+        if let Some(expr) = b.expr { self.visit_expr(expr) } else { ControlFlow::Continue(()) }
+    }
+
+    fn visit_expr(&mut self, expr: &'tcx hir::Expr<'tcx>) -> Self::Result {
+        // Check for promoted temporaries from autoref, e.g.
+        // `if let None = TypeWithDrop.as_ref() {} else {}`
+        // where `fn as_ref(&self) -> Option<...>`.
+        for adj in self.cx.typeck_results().expr_adjustments(expr) {
+            match adj.kind {
+                // Skip when we hit the first deref expr.
+                Adjust::Deref(_) => break,
+                Adjust::Borrow(_) => {
+                    self.check_promoted_temp_with_drop(expr)?;
                 }
-                ControlFlow::Continue(())
-            }
-            hir::ExprKind::Index(left, right, _) | hir::ExprKind::Binary(_, left, right) => {
-                self.visit_expr(left)?;
-                self.visit_expr(right)
+                _ => {}
             }
-            hir::ExprKind::Unary(_, expr)
-            | hir::ExprKind::Cast(expr, _)
-            | hir::ExprKind::Type(expr, _)
-            | hir::ExprKind::UnsafeBinderCast(_, expr, _)
-            | hir::ExprKind::Yield(expr, _)
-            | hir::ExprKind::AddrOf(_, _, expr)
-            | hir::ExprKind::Match(expr, _, _)
-            | hir::ExprKind::Field(expr, _)
-            | hir::ExprKind::Let(&hir::LetExpr {
-                init: expr,
-                span: _,
-                pat: _,
-                ty: _,
-                recovered: Recovered::No,
-            }) => self.visit_expr(expr),
-            hir::ExprKind::Let(_) => ControlFlow::Continue(()),
+        }
 
-            hir::ExprKind::If(cond, _, _) => {
-                if let hir::ExprKind::Let(hir::LetExpr {
-                    init,
-                    span: _,
-                    pat: _,
-                    ty: _,
-                    recovered: Recovered::No,
-                }) = cond.kind
-                {
-                    self.visit_expr(init)?;
-                }
-                ControlFlow::Continue(())
+        match expr.kind {
+            // Account for cases like `if let None = Some(&Drop) {} else {}`.
+            hir::ExprKind::AddrOf(_, _, expr) => {
+                self.check_promoted_temp_with_drop(expr)?;
+                intravisit::walk_expr(self, expr)
+            }
+            // `(Drop, ()).1` introduces a temporary and then moves out of
+            // part of it, therefore we should check it for temporaries.
+            // FIXME: This may have false positives if we move the part
+            // that actually has drop, but oh well.
+            hir::ExprKind::Index(expr, _, _) | hir::ExprKind::Field(expr, _) => {
+                self.check_promoted_temp_with_drop(expr)?;
+                intravisit::walk_expr(self, expr)
             }
+            // If always introduces a temporary terminating scope for its cond and arms,
+            // so don't visit them.
+            hir::ExprKind::If(..) => ControlFlow::Continue(()),
+            // Match introduces temporary terminating scopes for arms, so don't visit
+            // them, and only visit the scrutinee to account for cases like:
+            // `if let None = match &Drop { _ => Some(1) } {} else {}`.
+            hir::ExprKind::Match(scrut, _, _) => self.visit_expr(scrut),
+            // Self explanatory.
+            hir::ExprKind::DropTemps(_) => ControlFlow::Continue(()),
+            // Otherwise, walk into the expr's parts.
+            _ => intravisit::walk_expr(self, expr),
         }
     }
 }

tests/ui/drop/lint-if-let-rescope-false-positives.rs

@@ -0,0 +1,33 @@
+//@ edition: 2021
+//@ check-pass
+
+#![deny(if_let_rescope)]
+
+struct Drop;
+impl std::ops::Drop for Drop {
+    fn drop(&mut self) {
+        println!("drop")
+    }
+}
+
+impl Drop {
+    fn as_ref(&self) -> Option<i32> {
+        Some(1)
+    }
+}
+
+fn consume(_: impl Sized) -> Option<i32> { Some(1) }
+
+fn main() {
+    let drop = Drop;
+
+    // Make sure we don't drop if we don't actually make a temporary.
+    if let None = drop.as_ref() {} else {}
+
+    // Make sure we don't lint if we consume the droppy value.
+    if let None = consume(Drop) {} else {}
+
+    // Make sure we don't lint on field exprs of place exprs.
+    let tup_place = (Drop, ());
+    if let None = consume(tup_place.1) {} else {}
+}

tests/ui/drop/lint-if-let-rescope.fixed

@@ -94,6 +94,12 @@ fn main() {
         //~| HELP: a `match` with a single arm can preserve the drop order up to Edition 2021
     }
 
+    match Some((droppy(), ()).1) { Some(_value) => {} _ => {}}
+    //~^ ERROR: `if let` assigns a shorter lifetime since Edition 2024
+    //~| WARN: this changes meaning in Rust 2024
+    //~| HELP: the value is now dropped here in Edition 2024
+    //~| HELP: a `match` with a single arm can preserve the drop order up to Edition 2021
+
     // We want to keep the `if let`s below as direct descendents of match arms,
     // so the formatting is suppressed.
     #[rustfmt::skip]

tests/ui/drop/lint-if-let-rescope.rs

@@ -94,6 +94,12 @@ fn main() {
         //~| HELP: a `match` with a single arm can preserve the drop order up to Edition 2021
     }
 
+    if let Some(_value) = Some((droppy(), ()).1) {} else {}
+    //~^ ERROR: `if let` assigns a shorter lifetime since Edition 2024
+    //~| WARN: this changes meaning in Rust 2024
+    //~| HELP: the value is now dropped here in Edition 2024
+    //~| HELP: a `match` with a single arm can preserve the drop order up to Edition 2021
+
     // We want to keep the `if let`s below as direct descendents of match arms,
     // so the formatting is suppressed.
     #[rustfmt::skip]

tests/ui/drop/lint-if-let-rescope.stderr

@@ -175,5 +175,26 @@ LL -     while (if let Some(_value) = droppy().get() { false } else { true }) {
 LL +     while (match droppy().get() { Some(_value) => { false } _ => { true }}) {
    |
 
-error: aborting due to 7 previous errors
+error: `if let` assigns a shorter lifetime since Edition 2024
+  --> $DIR/lint-if-let-rescope.rs:97:8
+   |
+LL |     if let Some(_value) = Some((droppy(), ()).1) {} else {}
+   |        ^^^^^^^^^^^^^^^^^^^^^^^^--------------^^^
+   |                                |
+   |                                this value has a significant drop implementation which may observe a major change in drop order and requires your discretion
+   |
+   = warning: this changes meaning in Rust 2024
+   = note: for more information, see <https://doc.rust-lang.org/nightly/edition-guide/rust-2024/temporary-if-let-scope.html>
+help: the value is now dropped here in Edition 2024
+  --> $DIR/lint-if-let-rescope.rs:97:51
+   |
+LL |     if let Some(_value) = Some((droppy(), ()).1) {} else {}
+   |                                                   ^
+help: a `match` with a single arm can preserve the drop order up to Edition 2021
+   |
+LL -     if let Some(_value) = Some((droppy(), ()).1) {} else {}
+LL +     match Some((droppy(), ()).1) { Some(_value) => {} _ => {}}
+   |
+
+error: aborting due to 8 previous errors