apply suggestions

dingxiangfei2009 · dingxiangfei2009 · commit a6fee138f2b9 · 2024-11-11T06:50:53.000+08:00
diff --git a/compiler/rustc_middle/src/mir/syntax.rs b/compiler/rustc_middle/src/mir/syntax.rs
@@ -440,7 +440,7 @@ pub enum StatementKind<'tcx> {
     /// No-op. Useful for deleting instructions without affecting statement indices.
     Nop,
 
-    /// Marker statement for backward-incompatible drops in incoming future editions.
+    /// 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
diff --git a/compiler/rustc_middle/src/query/mod.rs b/compiler/rustc_middle/src/query/mod.rs
@@ -1447,12 +1447,20 @@ rustc_queries! {
         cache_on_disk_if { false }
     }
 
-    /// A list of types where a type requires drop if and only if any of those types
-    /// has significant drop. A type marked with the attribute `rustc_insignificant_dtor`
-    /// is considered to not be significant. A drop is significant if it is implemented
-    /// by the user or does anything that will have any observable behavior (other than
-    /// freeing up memory). If the type is known to have a significant destructor then
-    /// `Err(AlwaysRequiresDrop)` is returned.
+    /// 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.
diff --git a/compiler/rustc_mir_transform/messages.ftl b/compiler/rustc_mir_transform/messages.ftl
@@ -29,19 +29,19 @@ mir_transform_tail_expr_drop_order = relative drop order changing in Rust 2024
     .temporaries = in Rust 2024, this temporary value will be dropped first
     .observers = in Rust 2024, this local variable or temporary value will be dropped second
     .note_dtors =
-        dropping the temporary value runs this custom `Drop` impl, which will run first in Rust 2024
+        dropping the temporary value runs this custom `Drop` impl, which we could not prove to be side-effect free
     .note_observer_dtors =
-        dropping the local runs this custom `Drop` impl, which will run second in Rust 2024
+        dropping the local runs this custom `Drop` impl, which we could not prove to be side-effect free
     .drop_location =
         now the temporary value is dropped here, before the local variables in the block or statement
-    .note_epilogue = most of the time, changing drop order is harmless; inspect the `impl Drop`s for side effects
+    .note_epilogue = most of the time, changing drop order is harmless; inspect the `impl Drop`s for side effects like releasing locks or sending messages
     .label_local_epilogue = {$is_dropped_first_edition_2024 ->
         [true] up until Edition 2021 `{$name}` is dropped last but will be dropped earlier in Edition 2024
-        *[false] `{$name}` will be dropped later since Edition 2024
+        *[false] `{$name}` will be dropped later as of Edition 2024
     }
 
 mir_transform_tail_expr_dtor = {$dtor_kind ->
-    [dyn] `{$name}` may invoke a custom destructor because contains a trait object
+    [dyn] `{$name}` may invoke a custom destructor because it contains a trait object
     *[concrete] `{$name}` invokes this custom destructor
     }
 
diff --git a/compiler/rustc_mir_transform/src/lint_tail_expr_drop_order.rs b/compiler/rustc_mir_transform/src/lint_tail_expr_drop_order.rs
@@ -30,10 +30,14 @@ fn place_has_common_prefix<'tcx>(left: &Place<'tcx>, right: &Place<'tcx>) -> boo
         && left.projection.iter().zip(right.projection).all(|(left, right)| left == right)
 }
 
+/// Cache entry of `drop` at a `BasicBlock`
 #[derive(Debug, Clone, Copy)]
 enum MovePathIndexAtBlock {
+    /// We know nothing yet
     Unknown,
+    /// We know that the `drop` here has no effect
     None,
+    /// We know that the `drop` here will invoke a destructor
     Some(MovePathIndex),
 }
 
@@ -80,9 +84,14 @@ impl<'a, 'mir, 'tcx> DropsReachable<'a, 'mir, 'tcx> {
                         block,
                         statement_index: data.statements.len(),
                     });
+
+                    // Check if the drop of `place` under inspection is really in effect.
+                    // This is true only when `place` may have been initialized along a control flow path from a BID to the drop program point today.
+                    // In other words, this is where the drop of `place` will happen in the future instead.
                     if let MaybeReachable::Reachable(maybe_init) = self.maybe_init.get()
                         && maybe_init.contains(idx)
                     {
+                        // We also cache the drop information, so that we do not need to check on data-flow cursor again
                         self.block_drop_value_info[block] = MovePathIndexAtBlock::Some(idx);
                         dropped_local_here.borrow_mut().insert(idx);
                     } else {
@@ -143,6 +152,10 @@ impl<'a, 'mir, 'tcx> DropsReachable<'a, 'mir, 'tcx> {
     }
 }
 
+/// An additional filter to exclude well-known types from the ecosystem
+/// because their drops are trivial.
+/// This returns additional types to check if the drops are delegated to those.
+/// A typical example is `hashbrown::HashMap<K, V>`, whose drop is delegated to `K` and `V`.
 fn true_significant_drop_ty<'tcx>(
     tcx: TyCtxt<'tcx>,
     ty: Ty<'tcx>,
@@ -200,6 +213,8 @@ fn true_significant_drop_ty<'tcx>(
     }
 }
 
+/// Returns the list of types with a "potentially sigificant" that may be dropped
+/// by dropping a value of type `ty`.
 #[instrument(level = "debug", skip(tcx, param_env))]
 fn extract_component_raw<'tcx>(
     tcx: TyCtxt<'tcx>,
@@ -237,6 +252,9 @@ fn extract_component_with_significant_dtor<'tcx>(
     tys.into_iter().collect()
 }
 
+/// Extract the span of the custom destructor of a type
+/// especially the span of the `impl Drop` header or its entire block
+/// when we are working with current local crate.
 #[instrument(level = "debug", skip(tcx))]
 fn ty_dtor_span<'tcx>(tcx: TyCtxt<'tcx>, ty: Ty<'tcx>) -> Option<Span> {
     match ty.kind() {
@@ -291,6 +309,9 @@ fn ty_dtor_span<'tcx>(tcx: TyCtxt<'tcx>, ty: Ty<'tcx>) -> Option<Span> {
     }
 }
 
+/// Check if a moved place at `idx` is a part of a BID.
+/// The use of this check is that we will consider drops on these
+/// as a drop of the overall BID and, thus, we can exclude it from the diagnosis.
 fn place_descendent_of_bids<'tcx>(
     mut idx: MovePathIndex,
     move_data: &MoveData<'tcx>,
@@ -309,6 +330,7 @@ fn place_descendent_of_bids<'tcx>(
     }
 }
 
+/// The core of the lint `tail-expr-drop-order`
 pub(crate) fn run_lint<'tcx>(tcx: TyCtxt<'tcx>, def_id: LocalDefId, body: &Body<'tcx>) {
     if matches!(tcx.def_kind(def_id), rustc_hir::def::DefKind::SyntheticCoroutineBody) {
         // A synthetic coroutine has no HIR body and it is enough to just analyse the original body
@@ -326,8 +348,10 @@ pub(crate) fn run_lint<'tcx>(tcx: TyCtxt<'tcx>, def_id: LocalDefId, body: &Body<
         return;
     }
     // ## About BIDs in blocks ##
-    // We are using blocks to identify locals with the same scope targeted by backwards-incompatible drops (BID)
-    // because they tend to be scheduled in the same drop ladder block.
+    // Track the set of blocks that contain a backwards-incompatible drop (BID)
+    // and, for each block, the vector of locations.
+    //
+    // We group them per-block because they tend to scheduled in the same drop ladder block.
     let mut bid_per_block = IndexMap::default();
     let mut bid_places = UnordSet::new();
     let param_env = tcx.param_env(def_id).with_reveal_all_normalized(tcx);
@@ -358,6 +382,10 @@ pub(crate) fn run_lint<'tcx>(tcx: TyCtxt<'tcx>, def_id: LocalDefId, body: &Body<
     dump_mir(tcx, false, "lint_tail_expr_drop_order", &0 as _, body, |_, _| Ok(()));
     let locals_with_user_names = collect_user_names(body);
     let is_closure_like = tcx.is_closure_like(def_id.to_def_id());
+
+    // Compute the "maybe initialized" information for this body.
+    // When we encounter a DROP of some place P we only care
+    // about the drop if `P` may be initialized.
     let move_data = MoveData::gather_moves(body, tcx, |_| true);
     let maybe_init = MaybeInitializedPlaces::new(tcx, body, &move_data);
     let mut maybe_init = maybe_init.iterate_to_fixpoint(tcx, body, None).into_results_cursor(body);
@@ -370,6 +398,12 @@ pub(crate) fn run_lint<'tcx>(tcx: TyCtxt<'tcx>, def_id: LocalDefId, body: &Body<
         let mut drop_span = None;
         for &(_, place) in candidates.iter() {
             let mut collected_drops = ChunkedBitSet::new_empty(move_data.move_paths.len());
+            // ## On detecting change in relative drop order ##
+            // Iterate through each BID-containing block `block`.
+            // If the place `P` targeted by the BID is "maybe initialized",
+            // then search forward to find the actual `DROP(P)` point.
+            // Everything dropped between the BID and the actual drop point
+            // is something whose relative drop order will change.
             DropsReachable {
                 body,
                 place,
@@ -381,37 +415,62 @@ pub(crate) fn run_lint<'tcx>(tcx: TyCtxt<'tcx>, def_id: LocalDefId, body: &Body<
                 visited: Default::default(),
             }
             .visit(block);
-
+            // Compute the set `all_locals_dropped` of local variables that are dropped
+            // after the BID point but before the current drop point.
+            //
+            // These are the variables whose drop impls will be reordered with respect
+            // to `place`.
             all_locals_dropped.union(&collected_drops);
         }
+
+        // We shall now exclude some local bindings for the following cases.
         {
             let mut to_exclude = ChunkedBitSet::new_empty(all_locals_dropped.domain_size());
             // We will now do subtraction from the candidate dropped locals, because of the following reasons.
             for path_idx in all_locals_dropped.iter() {
                 let move_path = &move_data.move_paths[path_idx];
                 let dropped_local = move_path.place.local;
                 // a) A return value _0 will eventually be used
+                // Example:
+                // fn f() -> Droppy {
+                //     let _x = Droppy;
+                //     Droppy
+                // }
+                // _0 holds the literal `Droppy` and rightfully `_x` has to be dropped first
                 if dropped_local == Local::ZERO {
                     debug!(?dropped_local, "skip return value");
                     to_exclude.insert(path_idx);
                     continue;
                 }
                 // b) If we are analysing a closure, the captures are still dropped last.
                 // This is part of the closure capture lifetime contract.
+                // They are similar to the return value _0 with respect to lifetime rules.
                 if is_closure_like && matches!(dropped_local, ty::CAPTURE_STRUCT_LOCAL) {
                     debug!(?dropped_local, "skip closure captures");
                     to_exclude.insert(path_idx);
                     continue;
                 }
                 // c) Sometimes we collect places that are projections into the BID locals,
                 // so they are considered dropped now.
+                // Example:
+                // struct NotVeryDroppy(Droppy);
+                // impl Drop for Droppy {..}
+                // fn f() -> NotVeryDroppy {
+                //    let x = NotVeryDroppy(droppy());
+                //    {
+                //        let y: Droppy = x.0;
+                //        NotVeryDroppy(y)
+                //    }
+                // }
+                // `y` takes `x.0`, which invalidates `x` as a complete `NotVeryDroppy`
+                // so there is no point in linting against `x` any more.
                 if place_descendent_of_bids(path_idx, &move_data, &bid_places) {
                     debug!(?dropped_local, "skip descendent of bids");
                     to_exclude.insert(path_idx);
                     continue;
                 }
                 let observer_ty = move_path.place.ty(body, tcx).ty;
-                // d) The collect local has no custom destructor.
+                // d) The collected local has no custom destructor that passes our ecosystem filter.
                 if ty_dropped_components
                     .entry(observer_ty)
                     .or_insert_with(|| {
@@ -440,6 +499,9 @@ pub(crate) fn run_lint<'tcx>(tcx: TyCtxt<'tcx>, def_id: LocalDefId, body: &Body<
             // No drop effect is observable, so let us move on.
             continue;
         }
+
+        // ## The final work to assemble the diagnosis ##
+        // First collect or generate fresh names for local variable bindings and temporary values.
         let local_names = assign_observables_names(
             all_locals_dropped
                 .iter()
@@ -544,6 +606,7 @@ pub(crate) fn run_lint<'tcx>(tcx: TyCtxt<'tcx>, def_id: LocalDefId, body: &Body<
     }
 }
 
+/// Extract binding names if available for diagnosis
 fn collect_user_names(body: &Body<'_>) -> IndexMap<Local, Symbol> {
     let mut names = IndexMap::default();
     for var_debug_info in &body.var_debug_info {
@@ -556,6 +619,7 @@ fn collect_user_names(body: &Body<'_>) -> IndexMap<Local, Symbol> {
     names
 }
 
+/// Assign names for anonymous or temporary values for diagnosis
 fn assign_observables_names(
     locals: impl IntoIterator<Item = Local>,
     user_names: &IndexMap<Local, Symbol>,
@@ -599,6 +663,7 @@ struct LocalLabel<'a> {
     destructors: Vec<DestructorLabel<'a>>,
 }
 
+/// A custom `Subdiagnostic` implementation so that the notes are delivered in a specific order
 impl Subdiagnostic for LocalLabel<'_> {
     fn add_to_diag_with<
         G: rustc_errors::EmissionGuarantee,
diff --git a/tests/ui/drop/lint-tail-expr-drop-order.rs b/tests/ui/drop/lint-tail-expr-drop-order.rs
@@ -37,7 +37,7 @@ impl LoudDropper {
 fn should_lint() -> i32 {
     let x = LoudDropper;
     //~^ NOTE: `x` calls a custom destructor
-    //~| NOTE: `x` will be dropped later since Edition 2024
+    //~| NOTE: `x` will be dropped later as of Edition 2024
     // Should lint
     x.get() + LoudDropper.get()
     //~^ ERROR: relative drop order changing in Rust 2024
@@ -62,7 +62,7 @@ fn should_lint_in_nested_items() {
     fn should_lint_me() -> i32 {
         let x = LoudDropper;
         //~^ NOTE: `x` calls a custom destructor
-        //~| NOTE: `x` will be dropped later since Edition 2024
+        //~| NOTE: `x` will be dropped later as of Edition 2024
         // Should lint
         x.get() + LoudDropper.get()
         //~^ ERROR: relative drop order changing in Rust 2024
@@ -90,7 +90,7 @@ fn should_not_lint() -> i32 {
 fn should_lint_in_nested_block() -> i32 {
     let x = LoudDropper;
     //~^ NOTE: `x` calls a custom destructor
-    //~| NOTE: `x` will be dropped later since Edition 2024
+    //~| NOTE: `x` will be dropped later as of Edition 2024
     { LoudDropper.get() }
     //~^ ERROR: relative drop order changing in Rust 2024
     //~| NOTE: this value will be stored in a temporary; let us call it `#1`
@@ -143,7 +143,7 @@ fn should_lint_into_async_body() -> i32 {
 
     let future = f();
     //~^ NOTE: `future` calls a custom destructor
-    //~| NOTE: `future` will be dropped later since Edition 2024
+    //~| NOTE: `future` will be dropped later as of Edition 2024
     LoudDropper.get()
     //~^ ERROR: relative drop order changing in Rust 2024
     //~| WARN: this changes meaning in Rust 2024
@@ -160,7 +160,7 @@ fn should_lint_generics<T: Default>() -> &'static str {
     }
     let x = T::default();
     //~^ NOTE: `x` calls a custom destructor
-    //~| NOTE: `x` will be dropped later since Edition 2024
+    //~| NOTE: `x` will be dropped later as of Edition 2024
     extract(&T::default())
     //~^ ERROR: relative drop order changing in Rust 2024
     //~| WARN: this changes meaning in Rust 2024
@@ -174,7 +174,7 @@ fn should_lint_generics<T: Default>() -> &'static str {
 fn should_lint_adt() -> i32 {
     let x: Result<LoudDropper, ()> = Ok(LoudDropper);
     //~^ NOTE: `x` calls a custom destructor
-    //~| NOTE: `x` will be dropped later since Edition 2024
+    //~| NOTE: `x` will be dropped later as of Edition 2024
     LoudDropper.get()
     //~^ ERROR: relative drop order changing in Rust 2024
     //~| WARN: this changes meaning in Rust 2024
@@ -218,7 +218,7 @@ fn should_lint_with_dtor_span() -> i32 {
 
     let x = LoudDropper2;
     //~^ NOTE: `x` calls a custom destructor
-    //~| NOTE: `x` will be dropped later since Edition 2024
+    //~| NOTE: `x` will be dropped later as of Edition 2024
     LoudDropper3.get()
     //~^ ERROR: relative drop order changing in Rust 2024
     //~| NOTE: this value will be stored in a temporary; let us call it `#1`
@@ -243,7 +243,7 @@ fn should_lint_with_transient_drops() {
         {
             let _x = LoudDropper;
             //~^ NOTE: `_x` calls a custom destructor
-            //~| NOTE: `_x` will be dropped later since Edition 2024
+            //~| NOTE: `_x` will be dropped later as of Edition 2024
         },
     ));
     //~^ NOTE: now the temporary value is dropped here, before the local variables in the block or statement
diff --git a/tests/ui/drop/lint-tail-expr-drop-order.stderr b/tests/ui/drop/lint-tail-expr-drop-order.stderr
