Auto merge of #131326 - dingxiangfei2009:issue-130836-attempt-2, r=nikomatsakis

Reduce false positives of tail-expr-drop-order from consumed values (attempt #2)

r? `@nikomatsakis`

Tracked by #123739.

Related to #129864 but not replacing, yet.

Related to #130836.

This is an implementation of the approach suggested in the [Zulip stream](https://rust-lang.zulipchat.com/#narrow/stream/213817-t-lang/topic/temporary.20drop.20order.20changes). A new MIR statement `BackwardsIncompatibleDrop` is added to the MIR syntax. The lint now works by inspecting possibly live move paths before at the `BackwardsIncompatibleDrop` location and the actual drop under the current edition, which should be one before Edition 2024 in practice.

8 files changed, 104 insertions, 9 deletions

diff --git a/compiler/rustc_middle/src/middle/region.rs b/compiler/rustc_middle/src/middle/region.rs
index 4e44de33611..114211b27c1 100644
--- a/compiler/rustc_middle/src/middle/region.rs
+++ b/compiler/rustc_middle/src/middle/region.rs
@@ -236,6 +236,11 @@ pub struct ScopeTree {
     /// during type check based on a traversal of the AST.
     pub rvalue_candidates: HirIdMap<RvalueCandidateType>,
 
+    /// Backwards incompatible scoping that will be introduced in future editions.
+    /// This information is used later for linting to identify locals and
+    /// temporary values that will receive backwards-incompatible drop orders.
+    pub backwards_incompatible_scope: UnordMap<hir::ItemLocalId, Scope>,
+
     /// If there are any `yield` nested within a scope, this map
     /// stores the `Span` of the last one and its index in the
     /// postorder of the Visitor traversal on the HIR.
diff --git a/compiler/rustc_middle/src/mir/pretty.rs b/compiler/rustc_middle/src/mir/pretty.rs
index f0e2b7a376c..2bfcd0a6227 100644
--- a/compiler/rustc_middle/src/mir/pretty.rs
+++ b/compiler/rustc_middle/src/mir/pretty.rs
@@ -834,6 +834,11 @@ impl Debug for Statement<'_> {
             Intrinsic(box ref intrinsic) => write!(fmt, "{intrinsic}"),
             ConstEvalCounter => write!(fmt, "ConstEvalCounter"),
             Nop => write!(fmt, "nop"),
+            BackwardIncompatibleDropHint { ref place, reason: _ } => {
+                // For now, we don't record the reason because there is only one use case,
+                // which is to report breaking change in drop order by Edition 2024
+                write!(fmt, "backward incompatible drop({place:?})")
+            }
         }
     }
 }
diff --git a/compiler/rustc_middle/src/mir/syntax.rs b/compiler/rustc_middle/src/mir/syntax.rs
index 2083279e128..fea940ea47c 100644
--- a/compiler/rustc_middle/src/mir/syntax.rs
+++ b/compiler/rustc_middle/src/mir/syntax.rs
@@ -432,6 +432,18 @@ pub enum StatementKind<'tcx> {
 
     /// No-op. Useful for deleting instructions without affecting statement indices.
     Nop,
+
+    /// Marker statement indicating where `place` would be dropped.
+    /// This is semantically equivalent to `Nop`, so codegen and MIRI should interpret this
+    /// statement as such.
+    /// The only use case of this statement is for linting in MIR to detect temporary lifetime
+    /// changes.
+    BackwardIncompatibleDropHint {
+        /// Place to drop
+        place: Box<Place<'tcx>>,
+        /// Reason for backward incompatibility
+        reason: BackwardIncompatibleDropReason,
+    },
 }
 
 impl StatementKind<'_> {
@@ -452,6 +464,7 @@ impl StatementKind<'_> {
             StatementKind::Intrinsic(..) => "Intrinsic",
             StatementKind::ConstEvalCounter => "ConstEvalCounter",
             StatementKind::Nop => "Nop",
+            StatementKind::BackwardIncompatibleDropHint { .. } => "BackwardIncompatibleDropHint",
         }
     }
 }
@@ -897,6 +910,21 @@ pub enum TerminatorKind<'tcx> {
     },
 }
 
+#[derive(
+    Clone,
+    Debug,
+    TyEncodable,
+    TyDecodable,
+    Hash,
+    HashStable,
+    PartialEq,
+    TypeFoldable,
+    TypeVisitable
+)]
+pub enum BackwardIncompatibleDropReason {
+    Edition2024,
+}
+
 impl TerminatorKind<'_> {
     /// Returns a simple string representation of a `TerminatorKind` variant, independent of any
     /// values it might hold (e.g. `TerminatorKind::Call` always returns `"Call"`).
diff --git a/compiler/rustc_middle/src/mir/visit.rs b/compiler/rustc_middle/src/mir/visit.rs
index 9f9ee8497b6..62c340d99e3 100644
--- a/compiler/rustc_middle/src/mir/visit.rs
+++ b/compiler/rustc_middle/src/mir/visit.rs
@@ -452,6 +452,7 @@ macro_rules! make_mir_visitor {
                     }
                     StatementKind::ConstEvalCounter => {}
                     StatementKind::Nop => {}
+                    StatementKind::BackwardIncompatibleDropHint { .. } => {}
                 }
             }
 
diff --git a/compiler/rustc_middle/src/query/keys.rs b/compiler/rustc_middle/src/query/keys.rs
index 8fd99303622..970dc72e1ff 100644
--- a/compiler/rustc_middle/src/query/keys.rs
+++ b/compiler/rustc_middle/src/query/keys.rs
@@ -360,6 +360,14 @@ impl<'tcx> Key for (ty::ParamEnv<'tcx>, ty::TraitRef<'tcx>) {
     }
 }
 
+impl<'tcx> Key for ty::ParamEnvAnd<'tcx, Ty<'tcx>> {
+    type Cache<V> = DefaultCache<Self, V>;
+
+    fn default_span(&self, _tcx: TyCtxt<'_>) -> Span {
+        DUMMY_SP
+    }
+}
+
 impl<'tcx> Key for ty::TraitRef<'tcx> {
     type Cache<V> = DefaultCache<Self, V>;
 
diff --git a/compiler/rustc_middle/src/query/mod.rs b/compiler/rustc_middle/src/query/mod.rs
index 9ae519dfe7e..3fdb38a433e 100644
--- a/compiler/rustc_middle/src/query/mod.rs
+++ b/compiler/rustc_middle/src/query/mod.rs
@@ -1448,6 +1448,28 @@ rustc_queries! {
         cache_on_disk_if { false }
     }
 
+    /// Returns a list of types which (a) have a potentially significant destructor
+    /// and (b) may be dropped as a result of dropping a value of some type `ty`
+    /// (in the given environment).
+    ///
+    /// The idea of "significant" drop is somewhat informal and is used only for
+    /// diagnostics and edition migrations. The idea is that a significant drop may have
+    /// some visible side-effect on execution; freeing memory is NOT considered a side-effect.
+    /// The rules are as follows:
+    /// * Type with no explicit drop impl do not have significant drop.
+    /// * Types with a drop impl are assumed to have significant drop unless they have a `#[rustc_insignificant_dtor]` annotation.
+    ///
+    /// Note that insignificant drop is a "shallow" property. A type like `Vec<LockGuard>` does not
+    /// have significant drop but the type `LockGuard` does, and so if `ty  = Vec<LockGuard>`
+    /// then the return value would be `&[LockGuard]`.
+    /// *IMPORTANT*: *DO NOT* run this query before promoted MIR body is constructed,
+    /// because this query partially depends on that query.
+    /// Otherwise, there is a risk of query cycles.
+    query list_significant_drop_tys(ty: ty::ParamEnvAnd<'tcx, Ty<'tcx>>) -> &'tcx ty::List<Ty<'tcx>> {
+        desc { |tcx| "computing when `{}` has a significant destructor", ty.value }
+        cache_on_disk_if { false }
+    }
+
     /// Computes the layout of a type. Note that this implicitly
     /// executes in "reveal all" mode, and will normalize the input type.
     query layout_of(
diff --git a/compiler/rustc_middle/src/thir.rs b/compiler/rustc_middle/src/thir.rs
index 8f26e05cb72..06336d57704 100644
--- a/compiler/rustc_middle/src/thir.rs
+++ b/compiler/rustc_middle/src/thir.rs
@@ -247,13 +247,24 @@ pub struct Expr<'tcx> {
     pub ty: Ty<'tcx>,
 
     /// The lifetime of this expression if it should be spilled into a
-    /// temporary; should be `None` only if in a constant context
-    pub temp_lifetime: Option<region::Scope>,
+    /// temporary
+    pub temp_lifetime: TempLifetime,
 
     /// span of the expression in the source
     pub span: Span,
 }
 
+/// Temporary lifetime information for THIR expressions
+#[derive(Clone, Copy, Debug, HashStable)]
+pub struct TempLifetime {
+    /// Lifetime for temporaries as expected.
+    /// This should be `None` in a constant context.
+    pub temp_lifetime: Option<region::Scope>,
+    /// If `Some(lt)`, indicates that the lifetime of this temporary will change to `lt` in a future edition.
+    /// If `None`, then no changes are expected, or lints are disabled.
+    pub backwards_incompatible: Option<region::Scope>,
+}
+
 #[derive(Clone, Debug, HashStable)]
 pub enum ExprKind<'tcx> {
     /// `Scope`s are used to explicitly mark destruction scopes,
@@ -1087,7 +1098,7 @@ mod size_asserts {
     use super::*;
     // tidy-alphabetical-start
     static_assert_size!(Block, 48);
-    static_assert_size!(Expr<'_>, 64);
+    static_assert_size!(Expr<'_>, 72);
     static_assert_size!(ExprKind<'_>, 40);
     static_assert_size!(Pat<'_>, 64);
     static_assert_size!(PatKind<'_>, 48);
diff --git a/compiler/rustc_middle/src/ty/rvalue_scopes.rs b/compiler/rustc_middle/src/ty/rvalue_scopes.rs
index 37dcf7c0d64..57c2d7623d2 100644
--- a/compiler/rustc_middle/src/ty/rvalue_scopes.rs
+++ b/compiler/rustc_middle/src/ty/rvalue_scopes.rs
@@ -18,15 +18,17 @@ impl RvalueScopes {
     }
 
     /// Returns the scope when the temp created by `expr_id` will be cleaned up.
+    /// It also emits a lint on potential backwards incompatible change to the temporary scope
+    /// which is *for now* always shortening.
     pub fn temporary_scope(
         &self,
         region_scope_tree: &ScopeTree,
         expr_id: hir::ItemLocalId,
-    ) -> Option<Scope> {
+    ) -> (Option<Scope>, Option<Scope>) {
         // Check for a designated rvalue scope.
         if let Some(&s) = self.map.get(&expr_id) {
             debug!("temporary_scope({expr_id:?}) = {s:?} [custom]");
-            return s;
+            return (s, None);
         }
 
         // Otherwise, locate the innermost terminating scope
@@ -34,27 +36,40 @@ impl RvalueScopes {
         // have an enclosing scope, hence no scope will be
         // returned.
         let mut id = Scope { id: expr_id, data: ScopeData::Node };
+        let mut backwards_incompatible = None;
 
         while let Some(&(p, _)) = region_scope_tree.parent_map.get(&id) {
             match p.data {
                 ScopeData::Destruction => {
                     debug!("temporary_scope({expr_id:?}) = {id:?} [enclosing]");
-                    return Some(id);
+                    return (Some(id), backwards_incompatible);
                 }
                 ScopeData::IfThenRescope => {
                     debug!("temporary_scope({expr_id:?}) = {p:?} [enclosing]");
-                    return Some(p);
+                    return (Some(p), backwards_incompatible);
                 }
                 ScopeData::Node
                 | ScopeData::CallSite
                 | ScopeData::Arguments
                 | ScopeData::IfThen
-                | ScopeData::Remainder(_) => id = p,
+                | ScopeData::Remainder(_) => {
+                    // If we haven't already passed through a backwards-incompatible node,
+                    // then check if we are passing through one now and record it if so.
+                    // This is for now only working for cases where a temporary lifetime is
+                    // *shortened*.
+                    if backwards_incompatible.is_none() {
+                        backwards_incompatible = region_scope_tree
+                            .backwards_incompatible_scope
+                            .get(&p.item_local_id())
+                            .copied();
+                    }
+                    id = p
+                }
             }
         }
 
         debug!("temporary_scope({expr_id:?}) = None");
-        None
+        (None, backwards_incompatible)
     }
 
     /// Make an association between a sub-expression and an extended lifetime