diff --git a/compiler/rustc_lint/src/if_let_rescope.rs b/compiler/rustc_lint/src/if_let_rescope.rs index c1aa95667da68..23f037f3692ab 100644 --- a/compiler/rustc_lint/src/if_let_rescope.rs +++ b/compiler/rustc_lint/src/if_let_rescope.rs @@ -1,7 +1,7 @@ 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, @@ -9,6 +9,7 @@ use rustc_errors::{ 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; +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 { + 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 { + 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; + + 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), } } } diff --git a/tests/ui/drop/lint-if-let-rescope-false-positives.rs b/tests/ui/drop/lint-if-let-rescope-false-positives.rs new file mode 100644 index 0000000000000..77b7df4bc3bab --- /dev/null +++ b/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 { + Some(1) + } +} + +fn consume(_: impl Sized) -> Option { 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 {} +} diff --git a/tests/ui/drop/lint-if-let-rescope.fixed b/tests/ui/drop/lint-if-let-rescope.fixed index 182190aa323bc..79858e6f225aa 100644 --- a/tests/ui/drop/lint-if-let-rescope.fixed +++ b/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] diff --git a/tests/ui/drop/lint-if-let-rescope.rs b/tests/ui/drop/lint-if-let-rescope.rs index e1b38be0a0f50..9d873c65426f6 100644 --- a/tests/ui/drop/lint-if-let-rescope.rs +++ b/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] diff --git a/tests/ui/drop/lint-if-let-rescope.stderr b/tests/ui/drop/lint-if-let-rescope.stderr index 2b0fcb7a938a6..b17239976cc0d 100644 --- a/tests/ui/drop/lint-if-let-rescope.stderr +++ b/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 +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