Improve diagnostics for Precise Capture

Author: ChrisPardy

compiler/rustc_middle/src/ty/mod.rs

@@ -741,8 +741,20 @@ pub struct CapturedPlace<'tcx> {
 pub struct CaptureInfo<'tcx> {
     /// Expr Id pointing to use that resulted in selecting the current capture kind
     ///
+    /// Eg:
+    /// ```rust,no_run
+    /// let mut t = (0,1);
+    ///
+    /// let c = || {
+    ///     println!("{}",t); // L1
+    ///     t.1 = 4; // L2
+    /// };
+    /// ```
+    /// `capture_kind_expr_id` will point to the use on L2 and `path_expr_id` will point to the
+    /// use on L1.
+    ///
     /// If the user doesn't enable feature `capture_disjoint_fields` (RFC 2229) then, it is
-    /// possible that we don't see the use of a particular place resulting in expr_id being
+    /// possible that we don't see the use of a particular place resulting in capture_kind_expr_id being
     /// None. In such case we fallback on uvpars_mentioned for span.
     ///
     /// Eg:
@@ -756,7 +768,12 @@ pub struct CaptureInfo<'tcx> {
     ///
     /// In this example, if `capture_disjoint_fields` is **not** set, then x will be captured,
     /// but we won't see it being used during capture analysis, since it's essentially a discard.
-    pub expr_id: Option<hir::HirId>,
+    pub capture_kind_expr_id: Option<hir::HirId>,
+    /// Expr Id pointing to use that resulted the corresponding place being captured
+    ///
+    /// See `capture_kind_expr_id` for example.
+    ///
+    pub path_expr_id: Option<hir::HirId>,
 
     /// Capture mode that was selected
     pub capture_kind: UpvarCapture<'tcx>,

compiler/rustc_typeck/src/check/upvar.rs

@@ -42,7 +42,7 @@ use rustc_infer::infer::UpvarRegion;
 use rustc_middle::hir::place::{Place, PlaceBase, PlaceWithHirId, ProjectionKind};
 use rustc_middle::ty::{self, Ty, TyCtxt, UpvarSubsts};
 use rustc_span::sym;
-use rustc_span::{Span, Symbol};
+use rustc_span::{MultiSpan, Span, Symbol};
 
 /// Describe the relationship between the paths of two places
 /// eg:
@@ -135,7 +135,11 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
 
                     let upvar_id = ty::UpvarId::new(var_hir_id, local_def_id);
                     let capture_kind = self.init_capture_kind(capture_clause, upvar_id, span);
-                    let info = ty::CaptureInfo { expr_id: None, capture_kind };
+                    let info = ty::CaptureInfo {
+                        capture_kind_expr_id: None,
+                        path_expr_id: None,
+                        capture_kind,
+                    };
 
                     capture_information.insert(place, info);
                 }
@@ -298,8 +302,11 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
                         if let Some(capture_kind) = upvar_capture_map.get(&upvar_id) {
                             // upvar_capture_map only stores the UpvarCapture (CaptureKind),
                             // so we create a fake capture info with no expression.
-                            let fake_capture_info =
-                                ty::CaptureInfo { expr_id: None, capture_kind: *capture_kind };
+                            let fake_capture_info = ty::CaptureInfo {
+                                capture_kind_expr_id: None,
+                                path_expr_id: None,
+                                capture_kind: *capture_kind,
+                            };
                             determine_capture_info(fake_capture_info, capture_info).capture_kind
                         } else {
                             capture_info.capture_kind
@@ -349,20 +356,44 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
     ///
     /// ```
     /// {
-    ///       Place(base: hir_id_s, projections: [], ....) -> (hir_id_L5, ByValue),
-    ///       Place(base: hir_id_p, projections: [Field(0, 0)], ...) -> (hir_id_L2, ByRef(MutBorrow))
-    ///       Place(base: hir_id_p, projections: [Field(1, 0)], ...) -> (hir_id_L3, ByRef(ImmutBorrow))
-    ///       Place(base: hir_id_p, projections: [], ...) -> (hir_id_L4, ByRef(ImmutBorrow))
+    ///       Place(base: hir_id_s, projections: [], ....) -> {
+    ///                                                            capture_kind_expr: hir_id_L5,
+    ///                                                            path_expr_id: hir_id_L5,
+    ///                                                            capture_kind: ByValue
+    ///                                                       },
+    ///       Place(base: hir_id_p, projections: [Field(0, 0)], ...) -> {
+    ///                                                                     capture_kind_expr: hir_id_L2,
+    ///                                                                     path_expr_id: hir_id_L2,
+    ///                                                                     capture_kind: ByValue
+    ///                                                                 },
+    ///       Place(base: hir_id_p, projections: [Field(1, 0)], ...) -> {
+    ///                                                                     capture_kind_expr: hir_id_L3,
+    ///                                                                     path_expr_id: hir_id_L3,
+    ///                                                                     capture_kind: ByValue
+    ///                                                                 },
+    ///       Place(base: hir_id_p, projections: [], ...) -> {
+    ///                                                          capture_kind_expr: hir_id_L4,
+    ///                                                          path_expr_id: hir_id_L4,
+    ///                                                          capture_kind: ByValue
+    ///                                                      },
     /// ```
     ///
     /// After the min capture analysis, we get:
     /// ```
     /// {
     ///       hir_id_s -> [
-    ///            Place(base: hir_id_s, projections: [], ....) -> (hir_id_L4, ByValue)
+    ///            Place(base: hir_id_s, projections: [], ....) -> {
+    ///                                                                capture_kind_expr: hir_id_L5,
+    ///                                                                path_expr_id: hir_id_L5,
+    ///                                                                capture_kind: ByValue
+    ///                                                            },
     ///       ],
     ///       hir_id_p -> [
-    ///            Place(base: hir_id_p, projections: [], ...) -> (hir_id_L2, ByRef(MutBorrow)),
+    ///            Place(base: hir_id_p, projections: [], ...) -> {
+    ///                                                               capture_kind_expr: hir_id_L2,
+    ///                                                               path_expr_id: hir_id_L4,
+    ///                                                               capture_kind: ByValue
+    ///                                                           },
     ///       ],
     /// ```
     fn compute_min_captures(
@@ -415,8 +446,13 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
                     // current place is ancestor of possible_descendant
                     PlaceAncestryRelation::Ancestor => {
                         descendant_found = true;
+                        let backup_path_expr_id = updated_capture_info.path_expr_id;
+
                         updated_capture_info =
                             determine_capture_info(updated_capture_info, possible_descendant.info);
+
+                        // we need to keep the ancestor's `path_expr_id`
+                        updated_capture_info.path_expr_id = backup_path_expr_id;
                         false
                     }
 
@@ -431,9 +467,13 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
                         // current place is descendant of possible_ancestor
                         PlaceAncestryRelation::Descendant => {
                             ancestor_found = true;
+                            let backup_path_expr_id = possible_ancestor.info.path_expr_id;
                             possible_ancestor.info =
                                 determine_capture_info(possible_ancestor.info, capture_info);
 
+                            // we need to keep the ancestor's `path_expr_id`
+                            possible_ancestor.info.path_expr_id = backup_path_expr_id;
+
                             // Only one ancestor of the current place will be in the list.
                             break;
                         }
@@ -508,7 +548,8 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
                 let capture_str = construct_capture_info_string(self.tcx, place, capture_info);
                 let output_str = format!("Capturing {}", capture_str);
 
-                let span = capture_info.expr_id.map_or(closure_span, |e| self.tcx.hir().span(e));
+                let span =
+                    capture_info.path_expr_id.map_or(closure_span, |e| self.tcx.hir().span(e));
                 diag.span_note(span, &output_str);
             }
             diag.emit();
@@ -532,9 +573,32 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
                             construct_capture_info_string(self.tcx, place, capture_info);
                         let output_str = format!("Min Capture {}", capture_str);
 
-                        let span =
-                            capture_info.expr_id.map_or(closure_span, |e| self.tcx.hir().span(e));
-                        diag.span_note(span, &output_str);
+                        if capture.info.path_expr_id != capture.info.capture_kind_expr_id {
+                            let path_span = capture_info
+                                .path_expr_id
+                                .map_or(closure_span, |e| self.tcx.hir().span(e));
+                            let capture_kind_span = capture_info
+                                .capture_kind_expr_id
+                                .map_or(closure_span, |e| self.tcx.hir().span(e));
+
+                            let mut multi_span: MultiSpan =
+                                MultiSpan::from_spans(vec![path_span, capture_kind_span]);
+
+                            let capture_kind_label =
+                                construct_capture_kind_reason_string(self.tcx, place, capture_info);
+                            let path_label = construct_path_string(self.tcx, place);
+
+                            multi_span.push_span_label(path_span, path_label);
+                            multi_span.push_span_label(capture_kind_span, capture_kind_label);
+
+                            diag.span_note(multi_span, &output_str);
+                        } else {
+                            let span = capture_info
+                                .path_expr_id
+                                .map_or(closure_span, |e| self.tcx.hir().span(e));
+
+                            diag.span_note(span, &output_str);
+                        };
                     }
                 }
                 diag.emit();
@@ -632,7 +696,8 @@ impl<'a, 'tcx> InferBorrowKind<'a, 'tcx> {
         );
 
         let capture_info = ty::CaptureInfo {
-            expr_id: Some(diag_expr_id),
+            capture_kind_expr_id: Some(diag_expr_id),
+            path_expr_id: Some(diag_expr_id),
             capture_kind: ty::UpvarCapture::ByValue(Some(usage_span)),
         };
 
@@ -752,7 +817,8 @@ impl<'a, 'tcx> InferBorrowKind<'a, 'tcx> {
             let new_upvar_borrow = ty::UpvarBorrow { kind, region: curr_upvar_borrow.region };
 
             let capture_info = ty::CaptureInfo {
-                expr_id: Some(diag_expr_id),
+                capture_kind_expr_id: Some(diag_expr_id),
+                path_expr_id: Some(diag_expr_id),
                 capture_kind: ty::UpvarCapture::ByRef(new_upvar_borrow),
             };
             let updated_info = determine_capture_info(curr_capture_info, capture_info);
@@ -814,7 +880,11 @@ impl<'a, 'tcx> InferBorrowKind<'a, 'tcx> {
                 self.fcx.init_capture_kind(self.capture_clause, upvar_id, self.closure_span);
 
             let expr_id = Some(diag_expr_id);
-            let capture_info = ty::CaptureInfo { expr_id, capture_kind };
+            let capture_info = ty::CaptureInfo {
+                capture_kind_expr_id: expr_id,
+                path_expr_id: expr_id,
+                capture_kind,
+            };
 
             debug!("Capturing new place {:?}, capture_info={:?}", place_with_id, capture_info);
 
@@ -880,11 +950,7 @@ impl<'a, 'tcx> euv::Delegate<'tcx> for InferBorrowKind<'a, 'tcx> {
     }
 }
 
-fn construct_capture_info_string(
-    tcx: TyCtxt<'_>,
-    place: &Place<'tcx>,
-    capture_info: &ty::CaptureInfo<'tcx>,
-) -> String {
+fn construct_place_string(tcx: TyCtxt<'_>, place: &Place<'tcx>) -> String {
     let variable_name = match place.base {
         PlaceBase::Upvar(upvar_id) => var_name(tcx, upvar_id.var_path.hir_id).to_string(),
         _ => bug!("Capture_information should only contain upvars"),
@@ -904,11 +970,42 @@ fn construct_capture_info_string(
         projections_str.push_str(proj.as_str());
     }
 
+    format!("{}[{}]", variable_name, projections_str)
+}
+
+fn construct_capture_kind_reason_string(
+    tcx: TyCtxt<'_>,
+    place: &Place<'tcx>,
+    capture_info: &ty::CaptureInfo<'tcx>,
+) -> String {
+    let place_str = construct_place_string(tcx, &place);
+
     let capture_kind_str = match capture_info.capture_kind {
         ty::UpvarCapture::ByValue(_) => "ByValue".into(),
         ty::UpvarCapture::ByRef(borrow) => format!("{:?}", borrow.kind),
     };
-    format!("{}[{}] -> {}", variable_name, projections_str, capture_kind_str)
+
+    format!("{} captured as {} here", place_str, capture_kind_str)
+}
+
+fn construct_path_string(tcx: TyCtxt<'_>, place: &Place<'tcx>) -> String {
+    let place_str = construct_place_string(tcx, &place);
+
+    format!("{} used here", place_str)
+}
+
+fn construct_capture_info_string(
+    tcx: TyCtxt<'_>,
+    place: &Place<'tcx>,
+    capture_info: &ty::CaptureInfo<'tcx>,
+) -> String {
+    let place_str = construct_place_string(tcx, &place);
+
+    let capture_kind_str = match capture_info.capture_kind {
+        ty::UpvarCapture::ByValue(_) => "ByValue".into(),
+        ty::UpvarCapture::ByRef(borrow) => format!("{:?}", borrow.kind),
+    };
+    format!("{} -> {}", place_str, capture_kind_str)
 }
 
 fn var_name(tcx: TyCtxt<'_>, var_hir_id: hir::HirId) -> Symbol {
@@ -920,7 +1017,9 @@ fn var_name(tcx: TyCtxt<'_>, var_hir_id: hir::HirId) -> Symbol {
 /// (Note: CaptureInfo contains CaptureKind and an expression that led to capture it in that way)
 ///
 /// If both `CaptureKind`s are considered equivalent, then the CaptureInfo is selected based
-/// on the `CaptureInfo` containing an associated expression id.
+/// on the `CaptureInfo` containing an associated `capture_kind_expr_id`.
+///
+/// It is the caller's duty to figure out which path_expr_id to use.
 ///
 /// If both the CaptureKind and Expression are considered to be equivalent,
 /// then `CaptureInfo` A is preferred. This can be useful in cases where we want to priortize
@@ -971,7 +1070,7 @@ fn determine_capture_info(
     };
 
     if eq_capture_kind {
-        match (capture_info_a.expr_id, capture_info_b.expr_id) {
+        match (capture_info_a.capture_kind_expr_id, capture_info_b.capture_kind_expr_id) {
             (Some(_), _) | (None, None) => capture_info_a,
             (None, Some(_)) => capture_info_b,
         }

compiler/rustc_typeck/src/check/writeback.rs

@@ -348,7 +348,7 @@ impl<'cx, 'tcx> WritebackCx<'cx, 'tcx> {
                 let min_list_wb = min_list
                     .iter()
                     .map(|captured_place| {
-                        let locatable = captured_place.info.expr_id.unwrap_or(
+                        let locatable = captured_place.info.path_expr_id.unwrap_or(
                             self.tcx().hir().local_def_id_to_hir_id(closure_def_id.expect_local()),
                         );
 

compiler/rustc_typeck/src/expr_use_visitor.rs

@@ -630,7 +630,7 @@ impl<'a, 'tcx> ExprUseVisitor<'a, 'tcx> {
                         PlaceBase::Local(*var_hir_id)
                     };
                     let place_with_id = PlaceWithHirId::new(
-                        capture_info.expr_id.unwrap_or(closure_expr.hir_id),
+                        capture_info.path_expr_id.unwrap_or(closure_expr.hir_id),
                         place.base_ty,
                         place_base,
                         place.projections.clone(),

src/test/ui/closures/2229_closure_analysis/capture-analysis-1.rs

@@ -0,0 +1,36 @@
+
+#![feature(capture_disjoint_fields)]
+//~^ WARNING: the feature `capture_disjoint_fields` is incomplete
+//~| NOTE: `#[warn(incomplete_features)]` on by default
+//~| NOTE: see issue #53488 <https://github.com/rust-lang/rust/issues/53488>
+#![feature(rustc_attrs)]
+
+#[derive(Debug)]
+struct Point {
+    x: i32,
+    y: i32,
+}
+
+fn main() {
+    let p = Point { x: 10, y: 10 };
+    let q = Point { x: 10, y: 10 };
+
+    let c = #[rustc_capture_analysis]
+    //~^ ERROR: attributes on expressions are experimental
+    //~| NOTE: see issue #15701 <https://github.com/rust-lang/rust/issues/15701>
+    || {
+    //~^ First Pass analysis includes:
+    //~| Min Capture analysis includes:
+        println!("{:?}", p);
+        //~^ NOTE: Capturing p[] -> ImmBorrow
+        //~| NOTE: Min Capture p[] -> ImmBorrow
+        println!("{:?}", p.x);
+        //~^ NOTE: Capturing p[(0, 0)] -> ImmBorrow
+
+        println!("{:?}", q.x);
+        //~^ NOTE: Capturing q[(0, 0)] -> ImmBorrow
+        println!("{:?}", q);
+        //~^ NOTE: Capturing q[] -> ImmBorrow
+        //~| NOTE: Min Capture q[] -> ImmBorrow
+    };
+}