rework diagnostic reporting to be more structured

(cherry picked from commit 76bc027)

Author: nikomatsakis

compiler/rustc_typeck/src/check/upvar.rs

@@ -88,19 +88,53 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
 /// name (i.e: a.b.c)
 #[derive(Clone, Debug, PartialEq, Eq, PartialOrd, Ord, Hash)]
 enum CapturesInfo {
+    /// We previously captured all of `x`, but now we capture some sub-path.
     CapturingLess { source_expr: Option<hir::HirId>, var_name: String },
+    //CapturingNothing {
+    //    // where the variable appears in the closure (but is not captured)
+    //    use_span: Span,
+    //},
 }
 
-/// 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);
+/// Reasons that we might issue a migration warning.
+#[derive(Clone, Debug, Default, PartialEq, Eq, PartialOrd, Ord, Hash)]
+struct MigrationWarningReason {
+    /// When we used to capture `x` in its entirety, we implemented the auto-trait(s)
+    /// in this vec, but now we don't.
+    auto_traits: Vec<&'static str>,
+
+    /// When we used to capture `x` in its entirety, we would execute some destructors
+    /// at a different time.
+    drop_order: bool,
+}
+
+impl MigrationWarningReason {
+    fn migration_message(&self) -> String {
+        let base = "changes to closure capture in Rust 2021 will affect";
+        if !self.auto_traits.is_empty() && self.drop_order {
+            format!("{} drop order and which traits the closure implements", base)
+        } else if self.drop_order {
+            format!("{} drop order", base)
+        } else {
+            format!("{} which traits the closure implements", base)
+        }
+    }
+}
+
+/// Intermediate format to store information needed to generate a note in the migration lint.
+struct MigrationLintNote {
+    captures_info: CapturesInfo,
+
+    /// reasons why migration is needed for this capture
+    reason: MigrationWarningReason,
+}
 
 /// 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 NeededMigration {
+    var_hir_id: hir::HirId,
+    diagnostics_info: Vec<MigrationLintNote>,
+}
 
 struct InferBorrowKindVisitor<'a, 'tcx> {
     fcx: &'a FnCtxt<'a, 'tcx>,
@@ -710,47 +744,50 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
                  closure_head_span,
                 |lint| {
                     let mut diagnostics_builder = lint.build(
-                        format!(
-                            "changes to closure capture in Rust 2021 will affect {}",
-                            reasons
-                        )
-                        .as_str(),
+                        &reasons.migration_message(),
                     );
-                    for (var_hir_id, diagnostics_info) in need_migrations.iter() {
+                    for NeededMigration { var_hir_id, diagnostics_info } in &need_migrations {
                         // 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, this closure captures all of `{}`, but in Rust 2021, it will only capture `{}`",
-                                    self.tcx.hir().name(*var_hir_id),
-                                    captured_name,
-                                ));
+                        for lint_note in diagnostics_info.iter() {
+                            match &lint_note.captures_info {
+                                CapturesInfo::CapturingLess { source_expr: Some(capture_expr_id), var_name: captured_name } => {
+                                    let cause_span = self.tcx.hir().span(*capture_expr_id);
+                                    diagnostics_builder.span_label(cause_span, format!("in Rust 2018, this closure captures all of `{}`, but in Rust 2021, it will only capture `{}`",
+                                        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") {
+                            if lint_note.reason.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, `{}` is dropped here, but in Rust 2021, only `{}` will be dropped here as part of the closure",
-                                    self.tcx.hir().name(*var_hir_id),
-                                    captured_name,
-                                ));
+                                match &lint_note.captures_info {
+                                    CapturesInfo::CapturingLess { var_name: captured_name, .. } => {
+                                        diagnostics_builder.span_label(drop_location_span, format!("in Rust 2018, `{}` is dropped here, but in Rust 2021, only `{}` will be dropped here as part of 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 implements {} as `{}` implements {}, but in Rust 2021, this closure will no longer implement {} as `{}` does not implement {}",
-                                    missing_trait,
-                                    self.tcx.hir().name(*var_hir_id),
-                                    missing_trait,
-                                    missing_trait,
-                                    captured_name,
-                                    missing_trait,
-                                ));
+                            for &missing_trait in &lint_note.reason.auto_traits {
+                                // not capturing something anymore cannot cause a trait to fail to be implemented:
+                                match &lint_note.captures_info {
+                                    CapturesInfo::CapturingLess { var_name: captured_name, .. } => {
+                                        diagnostics_builder.span_label(closure_head_span, format!("in Rust 2018, this closure implements {missing_trait} as `{x}` implements {missing_trait}, but in Rust 2021, this closure will no longer implement {missing_trait} as `{p}` does not implement {missing_trait}",
+                                            missing_trait = missing_trait,
+                                            x = self.tcx.hir().name(*var_hir_id),
+                                            p = captured_name,
+                                        ));
+                                    }
+                                }
                             }
                         }
                     }
@@ -843,25 +880,16 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
     /// Combines all the reasons for 2229 migrations
     fn compute_2229_migrations_reasons(
         &self,
-        auto_trait_reasons: FxHashSet<&str>,
-        drop_reason: bool,
-    ) -> String {
-        let mut reasons = String::new();
-
-        if !auto_trait_reasons.is_empty() {
-            reasons = format!(
-                "{} trait implementation for closure",
-                auto_trait_reasons.clone().into_iter().collect::<Vec<&str>>().join(", ")
-            );
-        }
+        auto_trait_reasons: FxHashSet<&'static str>,
+        drop_order: bool,
+    ) -> MigrationWarningReason {
+        let mut reasons = MigrationWarningReason::default();
 
-        if !auto_trait_reasons.is_empty() && drop_reason {
-            reasons = format!("{} and ", reasons);
+        for auto_trait in auto_trait_reasons {
+            reasons.auto_traits.push(auto_trait);
         }
 
-        if drop_reason {
-            reasons = format!("{}drop order", reasons);
-        }
+        reasons.drop_order = drop_order;
 
         reasons
     }
@@ -877,7 +905,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
         min_captures: Option<&ty::RootVariableMinCaptureList<'tcx>>,
         var_hir_id: hir::HirId,
         closure_clause: hir::CaptureBy,
-    ) -> Option<FxHashMap<CapturesInfo, FxHashSet<&str>>> {
+    ) -> Option<FxHashMap<CapturesInfo, FxHashSet<&'static str>>> {
         let auto_traits_def_id = vec![
             self.tcx.lang_items().clone_trait(),
             self.tcx.lang_items().sync_trait(),
@@ -1026,7 +1054,13 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
 
             match closure_clause {
                 // Only migrate if closure is a move closure
-                hir::CaptureBy::Value => return Some(FxHashSet::default()),
+                hir::CaptureBy::Value => {
+                    let diagnostics_info = FxHashSet::default();
+                    //diagnostics_info.insert(CapturesInfo::CapturingNothing);
+                    //let upvars = self.tcx.upvars_mentioned(closure_def_id).expect("must be an upvar");
+                    //let _span = upvars[&var_hir_id];
+                    return Some(diagnostics_info);
+                }
                 hir::CaptureBy::Ref => {}
             }
 
@@ -1099,11 +1133,11 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
         closure_span: Span,
         closure_clause: hir::CaptureBy,
         min_captures: Option<&ty::RootVariableMinCaptureList<'tcx>>,
-    ) -> (Vec<MigrationDiagnosticInfo>, String) {
+    ) -> (Vec<NeededMigration>, MigrationWarningReason) {
         let upvars = if let Some(upvars) = self.tcx.upvars_mentioned(closure_def_id) {
             upvars
         } else {
-            return (Vec::new(), format!(""));
+            return (Vec::new(), MigrationWarningReason::default());
         };
 
         let mut need_migrations = Vec::new();
@@ -1112,7 +1146,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
 
         // Perform auto-trait analysis
         for (&var_hir_id, _) in upvars.iter() {
-            let mut responsible_captured_hir_ids = Vec::new();
+            let mut diagnostics_info = Vec::new();
 
             let auto_trait_diagnostic = if let Some(diagnostics_info) =
                 self.compute_2229_migrations_for_trait(min_captures, var_hir_id, closure_clause)
@@ -1144,38 +1178,33 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
 
             let mut capture_diagnostic = capture_diagnostic.into_iter().collect::<Vec<_>>();
             capture_diagnostic.sort();
-            for captured_info in capture_diagnostic.iter() {
+            for captures_info in capture_diagnostic {
                 // 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) {
+                    if let Some(reasons) = auto_trait_diagnostic.get(&captures_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);
+                let capture_drop_reorder_reason = drop_reorder_diagnostic.contains(&captures_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());
 
-                match captured_info {
-                    CapturesInfo::CapturingLess { source_expr, var_name } => {
-                        responsible_captured_hir_ids.push((
-                            *source_expr,
-                            var_name.clone(),
-                            self.compute_2229_migrations_reasons(
-                                capture_trait_reasons,
-                                capture_drop_reorder_reason,
-                            ),
-                        ));
-                    }
-                }
+                diagnostics_info.push(MigrationLintNote {
+                    captures_info,
+                    reason: self.compute_2229_migrations_reasons(
+                        capture_trait_reasons,
+                        capture_drop_reorder_reason,
+                    ),
+                });
             }
 
-            if !capture_diagnostic.is_empty() {
-                need_migrations.push((var_hir_id, responsible_captured_hir_ids));
+            if !diagnostics_info.is_empty() {
+                need_migrations.push(NeededMigration { var_hir_id, diagnostics_info });
             }
         }
         (
@@ -2138,10 +2167,12 @@ 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<MigrationDiagnosticInfo>,
+    need_migrations: &Vec<NeededMigration>,
 ) -> (String, String) {
-    let need_migrations_variables =
-        need_migrations.iter().map(|(v, _)| var_name(tcx, *v)).collect::<Vec<_>>();
+    let need_migrations_variables = need_migrations
+        .iter()
+        .map(|NeededMigration { var_hir_id: v, .. }| var_name(tcx, *v))
+        .collect::<Vec<_>>();
 
     let migration_ref_concat =
         need_migrations_variables.iter().map(|v| format!("&{}", v)).collect::<Vec<_>>().join(", ");

src/test/ui/closures/2229_closure_analysis/migrations/auto_traits.fixed

@@ -20,7 +20,7 @@ fn test_send_trait() {
     let mut f = 10;
     let fptr = SendPointer(&mut f as *mut i32);
     thread::spawn(move || { let _ = &fptr; unsafe {
-        //~^ ERROR: `Send` trait implementation for closure
+        //~^ ERROR: changes to closure capture
         //~| NOTE: in Rust 2018, this closure implements `Send` as `fptr` implements `Send`, but in Rust 2021, this closure will no longer implement `Send` as `fptr.0` does not implement `Send`
         //~| NOTE: for more information, see
         //~| HELP: add a dummy let to cause `fptr` to be fully captured
@@ -40,8 +40,9 @@ fn test_sync_trait() {
     let f = CustomInt(&mut f as *mut i32);
     let fptr = SyncPointer(f);
     thread::spawn(move || { let _ = &fptr; unsafe {
-        //~^ ERROR: `Sync`, `Send` trait implementation for closure
-        //~| NOTE: in Rust 2018, this closure implements `Sync`, `Send` as `fptr` implements `Sync`, `Send`, but in Rust 2021, this closure will no longer implement `Sync`, `Send` as `fptr.0.0` does not implement `Sync`, `Send`
+        //~^ ERROR: changes to closure capture
+        //~| NOTE: in Rust 2018, this closure implements `Sync` as `fptr` implements `Sync`, but in Rust 2021, this closure will no longer implement `Sync` as `fptr.0.0` does not implement `Sync`
+        //~| NOTE: in Rust 2018, this closure implements `Send` as `fptr` implements `Send`, but in Rust 2021, this closure will no longer implement `Send` as `fptr.0.0` does not implement `Send`
         //~| NOTE: for more information, see
         //~| HELP: add a dummy let to cause `fptr` to be fully captured
         *fptr.0.0 = 20;
@@ -65,7 +66,7 @@ fn test_clone_trait() {
     let f = U(S(Foo(0)), T(0));
     let c = || {
         let _ = &f;
-        //~^ ERROR: `Clone` trait implementation for closure and drop order
+        //~^ ERROR: changes to closure capture in Rust 2021 will affect drop order and which traits the closure implements
         //~| NOTE: in Rust 2018, this closure implements `Clone` as `f` implements `Clone`, but in Rust 2021, this closure will no longer implement `Clone` as `f.1` does not implement `Clone`
         //~| NOTE: for more information, see
         //~| HELP: add a dummy let to cause `f` to be fully captured

src/test/ui/closures/2229_closure_analysis/migrations/auto_traits.rs

@@ -20,7 +20,7 @@ fn test_send_trait() {
     let mut f = 10;
     let fptr = SendPointer(&mut f as *mut i32);
     thread::spawn(move || unsafe {
-        //~^ ERROR: `Send` trait implementation for closure
+        //~^ ERROR: changes to closure capture
         //~| NOTE: in Rust 2018, this closure implements `Send` as `fptr` implements `Send`, but in Rust 2021, this closure will no longer implement `Send` as `fptr.0` does not implement `Send`
         //~| NOTE: for more information, see
         //~| HELP: add a dummy let to cause `fptr` to be fully captured
@@ -40,8 +40,9 @@ fn test_sync_trait() {
     let f = CustomInt(&mut f as *mut i32);
     let fptr = SyncPointer(f);
     thread::spawn(move || unsafe {
-        //~^ ERROR: `Sync`, `Send` trait implementation for closure
-        //~| NOTE: in Rust 2018, this closure implements `Sync`, `Send` as `fptr` implements `Sync`, `Send`, but in Rust 2021, this closure will no longer implement `Sync`, `Send` as `fptr.0.0` does not implement `Sync`, `Send`
+        //~^ ERROR: changes to closure capture
+        //~| NOTE: in Rust 2018, this closure implements `Sync` as `fptr` implements `Sync`, but in Rust 2021, this closure will no longer implement `Sync` as `fptr.0.0` does not implement `Sync`
+        //~| NOTE: in Rust 2018, this closure implements `Send` as `fptr` implements `Send`, but in Rust 2021, this closure will no longer implement `Send` as `fptr.0.0` does not implement `Send`
         //~| NOTE: for more information, see
         //~| HELP: add a dummy let to cause `fptr` to be fully captured
         *fptr.0.0 = 20;
@@ -64,7 +65,7 @@ impl Clone for U {
 fn test_clone_trait() {
     let f = U(S(Foo(0)), T(0));
     let c = || {
-        //~^ ERROR: `Clone` trait implementation for closure and drop order
+        //~^ ERROR: changes to closure capture in Rust 2021 will affect drop order and which traits the closure implements
         //~| NOTE: in Rust 2018, this closure implements `Clone` as `f` implements `Clone`, but in Rust 2021, this closure will no longer implement `Clone` as `f.1` does not implement `Clone`
         //~| NOTE: for more information, see
         //~| HELP: add a dummy let to cause `f` to be fully captured