Auto merge of #86965 - sexxi-goose:rfc2229-improve-lint, r=nikomatsakis,lqd

Improves migrations lint for RFC2229

This PR improves the current disjoint capture migration lint by providing more information on why drop order or auto trait implementation for a closure is impacted by the use of the new feature.

The drop order migration lint will now look something like this:
```
error: changes to closure capture in Rust 2021 will affect drop order
  --> $DIR/significant_drop.rs:163:21
   |
LL |             let c = || {
   |                     ^^
...
LL |                 tuple.0;
   |                 ------- in Rust 2018, closure captures all of `tuple`, but in Rust 2021, it only captures `tuple.0`
...
LL |         }
   |         - in Rust 2018, `tuple` would be dropped here, but in Rust 2021, only `tuple.0` would be dropped here alongside the closure
```

The auto trait migration lint will now look something like this:
```
error: changes to closure capture in Rust 2021 will affect `Send` trait implementation for closure
  --> $DIR/auto_traits.rs:14:19
   |
LL |     thread::spawn(move || unsafe {
   |                   ^^^^^^^^^^^^^^ in Rust 2018, this closure would implement `Send` as `fptr` implements `Send`, but in Rust 2021, this closure would no longer implement `Send` as `fptr.0` does not implement `Send`
...
LL |         *fptr.0 = 20;
   |         ------- in Rust 2018, closure captures all of `fptr`, but in Rust 2021, it only captures `fptr.0`
```

r? `@nikomatsakis`

Closes https://github.com/rust-lang/project-rfc-2229/issues/54

1 files changed, 243 insertions, 154 deletions

diff --git a/compiler/rustc_typeck/src/check/upvar.rs b/compiler/rustc_typeck/src/check/upvar.rs
index 4d6d1da194f..c42ca936e97 100644
--- a/compiler/rustc_typeck/src/check/upvar.rs
+++ b/compiler/rustc_typeck/src/check/upvar.rs
@@ -50,6 +50,7 @@ use rustc_span::sym;
 use rustc_span::{MultiSpan, Span, Symbol};
 use rustc_trait_selection::infer::InferCtxtExt;
 
+use rustc_data_structures::stable_map::FxHashMap;
 use rustc_data_structures::stable_set::FxHashSet;
 use rustc_index::vec::Idx;
 use rustc_target::abi::VariantIdx;
@@ -81,6 +82,22 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
     }
 }
 
+/// Intermediate format to store the hir_id pointing to the use that resulted in the
+/// corresponding place being captured and a String which contains the captured value's
+/// name (i.e: a.b.c)
+type CapturesInfo = (Option<hir::HirId>, String);
+
+/// Intermediate format to store information needed to generate migration lint. The tuple
+/// contains the hir_id pointing to the use that resulted in the
+/// corresponding place being captured, a String which contains the captured value's
+/// name (i.e: a.b.c) and a String which contains the reason why migration is needed for that
+/// capture
+type MigrationNeededForCapture = (Option<hir::HirId>, String, String);
+
+/// Intermediate format to store the hir id of the root variable and a HashSet containing
+/// information on why the root variable should be fully captured
+type MigrationDiagnosticInfo = (hir::HirId, Vec<MigrationNeededForCapture>);
+
 struct InferBorrowKindVisitor<'a, 'tcx> {
     fcx: &'a FnCtxt<'a, 'tcx>,
 }
@@ -498,18 +515,58 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
 
             let local_def_id = closure_def_id.expect_local();
             let closure_hir_id = self.tcx.hir().local_def_id_to_hir_id(local_def_id);
+            let closure_span = self.tcx.hir().span(closure_hir_id);
+            let closure_head_span = self.tcx.sess.source_map().guess_head_span(closure_span);
             self.tcx.struct_span_lint_hir(
                 lint::builtin::RUST_2021_INCOMPATIBLE_CLOSURE_CAPTURES,
                 closure_hir_id,
-                span,
+                closure_head_span,
                 |lint| {
                     let mut diagnostics_builder = lint.build(
                         format!(
-                            "{} will change in Rust 2021",
+                            "changes to closure capture in Rust 2021 will affect {}",
                             reasons
                         )
                         .as_str(),
                     );
+                    for (var_hir_id, diagnostics_info) in need_migrations.iter() {
+                        // Labels all the usage of the captured variable and why they are responsible
+                        // for migration being needed
+                        for (captured_hir_id, captured_name, reasons) in diagnostics_info.iter() {
+                            if let Some(captured_hir_id) = captured_hir_id {
+                                let cause_span = self.tcx.hir().span(*captured_hir_id);
+                                diagnostics_builder.span_label(cause_span, format!("in Rust 2018, closure captures all of `{}`, but in Rust 2021, it only captures `{}`",
+                                    self.tcx.hir().name(*var_hir_id),
+                                    captured_name,
+                                ));
+                            }
+
+                            // Add a label pointing to where a captured variable affected by drop order
+                            // is dropped
+                            if reasons.contains("drop order") {
+                                let drop_location_span = drop_location_span(self.tcx, &closure_hir_id);
+
+                                diagnostics_builder.span_label(drop_location_span, format!("in Rust 2018, `{}` would be dropped here, but in Rust 2021, only `{}` would be dropped here alongside the closure",
+                                    self.tcx.hir().name(*var_hir_id),
+                                    captured_name,
+                                ));
+                            }
+
+                            // Add a label explaining why a closure no longer implements a trait
+                            if reasons.contains("trait implementation") {
+                                let missing_trait = &reasons[..reasons.find("trait implementation").unwrap() - 1];
+
+                                diagnostics_builder.span_label(closure_head_span, format!("in Rust 2018, this closure would implement {} as `{}` implements {}, but in Rust 2021, this closure would no longer implement {} as `{}` does not implement {}",
+                                    missing_trait,
+                                    self.tcx.hir().name(*var_hir_id),
+                                    missing_trait,
+                                    missing_trait,
+                                    captured_name,
+                                    missing_trait,
+                                ));
+                            }
+                        }
+                    }
                     diagnostics_builder.note("for more information, see <https://doc.rust-lang.org/nightly/edition-guide/rust-2021/disjoint-capture-in-closures.html>");
                     let closure_body_span = self.tcx.hir().span(body_id.hir_id);
                     let (sugg, app) =
@@ -556,13 +613,13 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
 
         if auto_trait_reasons.len() > 0 {
             reasons = format!(
-                "{} trait implementation",
+                "{} trait implementation for closure",
                 auto_trait_reasons.clone().into_iter().collect::<Vec<&str>>().join(", ")
             );
         }
 
         if auto_trait_reasons.len() > 0 && drop_reason {
-            reasons = format!("{}, and ", reasons);
+            reasons = format!("{} and ", reasons);
         }
 
         if drop_reason {
@@ -572,20 +629,35 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
         reasons
     }
 
-    /// Returns true if migration is needed for trait for the provided var_hir_id
-    fn need_2229_migrations_for_trait(
+    /// Figures out the list of root variables (and their types) that aren't completely
+    /// captured by the closure when `capture_disjoint_fields` is enabled and auto-traits
+    /// differ between the root variable and the captured paths.
+    ///
+    /// Returns a tuple containing a HashMap of CapturesInfo that maps to a HashSet of trait names
+    /// if migration is needed for traits for the provided var_hir_id, otherwise returns None
+    fn compute_2229_migrations_for_trait(
         &self,
         min_captures: Option<&ty::RootVariableMinCaptureList<'tcx>>,
         var_hir_id: hir::HirId,
-        check_trait: Option<DefId>,
         closure_clause: hir::CaptureBy,
-    ) -> bool {
+    ) -> Option<FxHashMap<CapturesInfo, FxHashSet<&str>>> {
+        let auto_traits_def_id = vec![
+            self.tcx.lang_items().clone_trait(),
+            self.tcx.lang_items().sync_trait(),
+            self.tcx.get_diagnostic_item(sym::send_trait),
+            self.tcx.lang_items().unpin_trait(),
+            self.tcx.get_diagnostic_item(sym::unwind_safe_trait),
+            self.tcx.get_diagnostic_item(sym::ref_unwind_safe_trait),
+        ];
+        let auto_traits =
+            vec!["`Clone`", "`Sync`", "`Send`", "`Unpin`", "`UnwindSafe`", "`RefUnwindSafe`"];
+
         let root_var_min_capture_list = if let Some(root_var_min_capture_list) =
             min_captures.and_then(|m| m.get(&var_hir_id))
         {
             root_var_min_capture_list
         } else {
-            return false;
+            return None;
         };
 
         let ty = self.infcx.resolve_vars_if_possible(self.node_ty(var_hir_id));
@@ -604,19 +676,26 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
             }
         };
 
-        let obligation_should_hold = check_trait
-            .map(|check_trait| {
-                self.infcx
-                    .type_implements_trait(
-                        check_trait,
-                        ty,
-                        self.tcx.mk_substs_trait(ty, &[]),
-                        self.param_env,
-                    )
-                    .must_apply_modulo_regions()
-            })
-            .unwrap_or(false);
+        let mut obligations_should_hold = Vec::new();
+        // Checks if a root variable implements any of the auto traits
+        for check_trait in auto_traits_def_id.iter() {
+            obligations_should_hold.push(
+                check_trait
+                    .map(|check_trait| {
+                        self.infcx
+                            .type_implements_trait(
+                                check_trait,
+                                ty,
+                                self.tcx.mk_substs_trait(ty, &[]),
+                                self.param_env,
+                            )
+                            .must_apply_modulo_regions()
+                    })
+                    .unwrap_or(false),
+            );
+        }
 
+        let mut problematic_captures = FxHashMap::default();
         // Check whether captured fields also implement the trait
         for capture in root_var_min_capture_list.iter() {
             let ty = apply_capture_kind_on_capture_ty(
@@ -625,106 +704,46 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
                 capture.info.capture_kind,
             );
 
-            let obligation_holds_for_capture = check_trait
-                .map(|check_trait| {
-                    self.infcx
-                        .type_implements_trait(
-                            check_trait,
-                            ty,
-                            self.tcx.mk_substs_trait(ty, &[]),
-                            self.param_env,
-                        )
-                        .must_apply_modulo_regions()
-                })
-                .unwrap_or(false);
-
-            if !obligation_holds_for_capture && obligation_should_hold {
-                return true;
+            // Checks if a capture implements any of the auto traits
+            let mut obligations_holds_for_capture = Vec::new();
+            for check_trait in auto_traits_def_id.iter() {
+                obligations_holds_for_capture.push(
+                    check_trait
+                        .map(|check_trait| {
+                            self.infcx
+                                .type_implements_trait(
+                                    check_trait,
+                                    ty,
+                                    self.tcx.mk_substs_trait(ty, &[]),
+                                    self.param_env,
+                                )
+                                .must_apply_modulo_regions()
+                        })
+                        .unwrap_or(false),
+                );
             }
-        }
-        false
-    }
-
-    /// Figures out the list of root variables (and their types) that aren't completely
-    /// captured by the closure when `capture_disjoint_fields` is enabled and auto-traits
-    /// differ between the root variable and the captured paths.
-    ///
-    /// The output list would include a root variable if:
-    /// - It would have been captured into the closure when `capture_disjoint_fields` wasn't
-    ///   enabled, **and**
-    /// - It wasn't completely captured by the closure, **and**
-    /// - One of the paths captured does not implement all the auto-traits its root variable
-    ///   implements.
-    fn compute_2229_migrations_for_trait(
-        &self,
-        min_captures: Option<&ty::RootVariableMinCaptureList<'tcx>>,
-        var_hir_id: hir::HirId,
-        closure_clause: hir::CaptureBy,
-    ) -> Option<FxHashSet<&str>> {
-        let tcx = self.infcx.tcx;
-
-        // Check whether catpured fields also implement the trait
-        let mut auto_trait_reasons = FxHashSet::default();
-
-        if self.need_2229_migrations_for_trait(
-            min_captures,
-            var_hir_id,
-            tcx.lang_items().clone_trait(),
-            closure_clause,
-        ) {
-            auto_trait_reasons.insert("`Clone`");
-        }
-
-        if self.need_2229_migrations_for_trait(
-            min_captures,
-            var_hir_id,
-            tcx.lang_items().sync_trait(),
-            closure_clause,
-        ) {
-            auto_trait_reasons.insert("`Sync`");
-        }
 
-        if self.need_2229_migrations_for_trait(
-            min_captures,
-            var_hir_id,
-            tcx.get_diagnostic_item(sym::send_trait),
-            closure_clause,
-        ) {
-            auto_trait_reasons.insert("`Send`");
-        }
+            let mut capture_problems = FxHashSet::default();
 
-        if self.need_2229_migrations_for_trait(
-            min_captures,
-            var_hir_id,
-            tcx.lang_items().unpin_trait(),
-            closure_clause,
-        ) {
-            auto_trait_reasons.insert("`Unpin`");
-        }
-
-        if self.need_2229_migrations_for_trait(
-            min_captures,
-            var_hir_id,
-            tcx.get_diagnostic_item(sym::unwind_safe_trait),
-            closure_clause,
-        ) {
-            auto_trait_reasons.insert("`UnwindSafe`");
-        }
+            // Checks if for any of the auto traits, one or more trait is implemented
+            // by the root variable but not by the capture
+            for (idx, _) in obligations_should_hold.iter().enumerate() {
+                if !obligations_holds_for_capture[idx] && obligations_should_hold[idx] {
+                    capture_problems.insert(auto_traits[idx]);
+                }
+            }
 
-        if self.need_2229_migrations_for_trait(
-            min_captures,
-            var_hir_id,
-            tcx.get_diagnostic_item(sym::ref_unwind_safe_trait),
-            closure_clause,
-        ) {
-            auto_trait_reasons.insert("`RefUnwindSafe`");
+            if capture_problems.len() > 0 {
+                problematic_captures.insert(
+                    (capture.info.path_expr_id, capture.to_string(self.tcx)),
+                    capture_problems,
+                );
+            }
         }
-
-        if auto_trait_reasons.len() > 0 {
-            return Some(auto_trait_reasons);
+        if problematic_captures.len() > 0 {
+            return Some(problematic_captures);
         }
-
-        return None;
+        None
     }
 
     /// Figures out the list of root variables (and their types) that aren't completely
@@ -737,8 +756,8 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
     /// - It wasn't completely captured by the closure, **and**
     /// - One of the paths starting at this root variable, that is not captured needs Drop.
     ///
-    /// This function only returns true for significant drops. A type is considerent to have a
-    /// significant drop if it's Drop implementation is not annotated by `rustc_insignificant_dtor`.
+    /// This function only returns a HashSet of CapturesInfo for significant drops. If there
+    /// are no significant drops than None is returned
     fn compute_2229_migrations_for_drop(
         &self,
         closure_def_id: DefId,
@@ -746,11 +765,11 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
         min_captures: Option<&ty::RootVariableMinCaptureList<'tcx>>,
         closure_clause: hir::CaptureBy,
         var_hir_id: hir::HirId,
-    ) -> bool {
+    ) -> Option<FxHashSet<CapturesInfo>> {
         let ty = self.infcx.resolve_vars_if_possible(self.node_ty(var_hir_id));
 
         if !ty.has_significant_drop(self.tcx, self.tcx.param_env(closure_def_id.expect_local())) {
-            return false;
+            return None;
         }
 
         let root_var_min_capture_list = if let Some(root_var_min_capture_list) =
@@ -763,21 +782,29 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
 
             match closure_clause {
                 // Only migrate if closure is a move closure
-                hir::CaptureBy::Value => return true,
+                hir::CaptureBy::Value => return Some(FxHashSet::default()),
                 hir::CaptureBy::Ref => {}
             }
 
-            return false;
+            return None;
         };
 
-        let projections_list = root_var_min_capture_list
-            .iter()
-            .filter_map(|captured_place| match captured_place.info.capture_kind {
+        let mut projections_list = Vec::new();
+        let mut diagnostics_info = FxHashSet::default();
+
+        for captured_place in root_var_min_capture_list.iter() {
+            match captured_place.info.capture_kind {
                 // Only care about captures that are moved into the closure
-                ty::UpvarCapture::ByValue(..) => Some(captured_place.place.projections.as_slice()),
-                ty::UpvarCapture::ByRef(..) => None,
-            })
-            .collect::<Vec<_>>();
+                ty::UpvarCapture::ByValue(..) => {
+                    projections_list.push(captured_place.place.projections.as_slice());
+                    diagnostics_info.insert((
+                        captured_place.info.path_expr_id,
+                        captured_place.to_string(self.tcx),
+                    ));
+                }
+                ty::UpvarCapture::ByRef(..) => {}
+            }
+        }
 
         let is_moved = !projections_list.is_empty();
 
@@ -793,10 +820,10 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
                 projections_list,
             )
         {
-            return true;
+            return Some(diagnostics_info);
         }
 
-        return false;
+        return None;
     }
 
     /// Figures out the list of root variables (and their types) that aren't completely
@@ -812,15 +839,16 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
     /// - One of the paths captured does not implement all the auto-traits its root variable
     ///   implements.
     ///
-    /// Returns a tuple containing a vector of HirIds as well as a String containing the reason
-    /// why root variables whose HirId is contained in the vector should be fully captured.
+    /// Returns a tuple containing a vector of MigrationDiagnosticInfo, as well as a String
+    /// containing the reason why root variables whose HirId is contained in the vector should
+    /// be captured
     fn compute_2229_migrations(
         &self,
         closure_def_id: DefId,
         closure_span: Span,
         closure_clause: hir::CaptureBy,
         min_captures: Option<&ty::RootVariableMinCaptureList<'tcx>>,
-    ) -> (Vec<hir::HirId>, String) {
+    ) -> (Vec<MigrationDiagnosticInfo>, String) {
         let upvars = if let Some(upvars) = self.tcx.upvars_mentioned(closure_def_id) {
             upvars
         } else {
@@ -828,38 +856,79 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
         };
 
         let mut need_migrations = Vec::new();
-        let mut auto_trait_reasons = FxHashSet::default();
-        let mut drop_reorder_reason = false;
+        let mut auto_trait_migration_reasons = FxHashSet::default();
+        let mut drop_migration_needed = false;
 
         // Perform auto-trait analysis
         for (&var_hir_id, _) in upvars.iter() {
-            let mut need_migration = false;
-            if let Some(trait_migration_cause) =
+            let mut responsible_captured_hir_ids = Vec::new();
+
+            let auto_trait_diagnostic = if let Some(diagnostics_info) =
                 self.compute_2229_migrations_for_trait(min_captures, var_hir_id, closure_clause)
             {
-                need_migration = true;
-                auto_trait_reasons.extend(trait_migration_cause);
+                diagnostics_info
+            } else {
+                FxHashMap::default()
+            };
+
+            let drop_reorder_diagnostic = if let Some(diagnostics_info) = self
+                .compute_2229_migrations_for_drop(
+                    closure_def_id,
+                    closure_span,
+                    min_captures,
+                    closure_clause,
+                    var_hir_id,
+                ) {
+                drop_migration_needed = true;
+                diagnostics_info
+            } else {
+                FxHashSet::default()
+            };
+
+            // Combine all the captures responsible for needing migrations into one HashSet
+            let mut capture_diagnostic = drop_reorder_diagnostic.clone();
+            for key in auto_trait_diagnostic.keys() {
+                capture_diagnostic.insert(key.clone());
             }
 
-            if self.compute_2229_migrations_for_drop(
-                closure_def_id,
-                closure_span,
-                min_captures,
-                closure_clause,
-                var_hir_id,
-            ) {
-                need_migration = true;
-                drop_reorder_reason = true;
+            let mut capture_diagnostic = capture_diagnostic.into_iter().collect::<Vec<_>>();
+            capture_diagnostic.sort();
+            for captured_info in capture_diagnostic.iter() {
+                // Get the auto trait reasons of why migration is needed because of that capture, if there are any
+                let capture_trait_reasons =
+                    if let Some(reasons) = auto_trait_diagnostic.get(captured_info) {
+                        reasons.clone()
+                    } else {
+                        FxHashSet::default()
+                    };
+
+                // Check if migration is needed because of drop reorder as a result of that capture
+                let capture_drop_reorder_reason = drop_reorder_diagnostic.contains(captured_info);
+
+                // Combine all the reasons of why the root variable should be captured as a result of
+                // auto trait implementation issues
+                auto_trait_migration_reasons.extend(capture_trait_reasons.clone());
+
+                responsible_captured_hir_ids.push((
+                    captured_info.0,
+                    captured_info.1.clone(),
+                    self.compute_2229_migrations_reasons(
+                        capture_trait_reasons,
+                        capture_drop_reorder_reason,
+                    ),
+                ));
             }
 
-            if need_migration {
-                need_migrations.push(var_hir_id);
+            if capture_diagnostic.len() > 0 {
+                need_migrations.push((var_hir_id, responsible_captured_hir_ids));
             }
         }
-
         (
             need_migrations,
-            self.compute_2229_migrations_reasons(auto_trait_reasons, drop_reorder_reason),
+            self.compute_2229_migrations_reasons(
+                auto_trait_migration_reasons,
+                drop_migration_needed,
+            ),
         )
     }
 
@@ -1320,6 +1389,26 @@ fn apply_capture_kind_on_capture_ty(
     }
 }
 
+/// Returns the Span of where the value with the provided HirId would be dropped
+fn drop_location_span(tcx: TyCtxt<'tcx>, hir_id: &hir::HirId) -> Span {
+    let owner_id = tcx.hir().get_enclosing_scope(*hir_id).unwrap();
+
+    let owner_node = tcx.hir().get(owner_id);
+    let owner_span = match owner_node {
+        hir::Node::Item(item) => match item.kind {
+            hir::ItemKind::Fn(_, _, owner_id) => tcx.hir().span(owner_id.hir_id),
+            _ => {
+                bug!("Drop location span error: need to handle more ItemKind {:?}", item.kind);
+            }
+        },
+        hir::Node::Block(block) => tcx.hir().span(block.hir_id),
+        _ => {
+            bug!("Drop location span error: need to handle more Node {:?}", owner_node);
+        }
+    };
+    tcx.sess.source_map().end_point(owner_span)
+}
+
 struct InferBorrowKind<'a, 'tcx> {
     fcx: &'a FnCtxt<'a, 'tcx>,
 
@@ -1877,10 +1966,10 @@ fn should_do_rust_2021_incompatible_closure_captures_analysis(
 /// - s2: Comma separated names of the variables being migrated.
 fn migration_suggestion_for_2229(
     tcx: TyCtxt<'_>,
-    need_migrations: &Vec<hir::HirId>,
+    need_migrations: &Vec<MigrationDiagnosticInfo>,
 ) -> (String, String) {
     let need_migrations_variables =
-        need_migrations.iter().map(|v| var_name(tcx, *v)).collect::<Vec<_>>();
+        need_migrations.iter().map(|(v, _)| var_name(tcx, *v)).collect::<Vec<_>>();
 
     let migration_ref_concat =
         need_migrations_variables.iter().map(|v| format!("&{}", v)).collect::<Vec<_>>().join(", ");