Rollup merge of #57102 - davidtwco:issue-57100, r=nikomatsakis

NLL: Add union justifications to conflicting borrows.

Fixes #57100.

This PR adds justifications to error messages for conflicting borrows of union fields.

Where previously an error message would say ``cannot borrow `u.b` as mutable..``, it now says ``cannot borrow `u` (via `u.b`) as mutable..``.

r? @pnkfelix

Author: Centril

src/librustc_borrowck/borrowck/check_loans.rs

@@ -557,12 +557,8 @@ impl<'a, 'tcx> CheckLoanCtxt<'a, 'tcx> {
                 if new_loan.loan_path.has_fork(&old_loan.loan_path) && common.is_some() {
                     let nl = self.bccx.loan_path_to_string(&common.unwrap());
                     let ol = nl.clone();
-                    let new_loan_msg = format!(" (via `{}`)",
-                                               self.bccx.loan_path_to_string(
-                                                   &new_loan.loan_path));
-                    let old_loan_msg = format!(" (via `{}`)",
-                                               self.bccx.loan_path_to_string(
-                                                   &old_loan.loan_path));
+                    let new_loan_msg = self.bccx.loan_path_to_string(&new_loan.loan_path);
+                    let old_loan_msg = self.bccx.loan_path_to_string(&old_loan.loan_path);
                     (nl, ol, new_loan_msg, old_loan_msg)
                 } else {
                     (self.bccx.loan_path_to_string(&new_loan.loan_path),

src/librustc_mir/borrow_check/error_reporting.rs

@@ -327,10 +327,8 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
             "closure"
         };
 
-        let desc_place = self.describe_place(place).unwrap_or_else(|| "_".to_owned());
-        let tcx = self.infcx.tcx;
-
-        let first_borrow_desc;
+        let (desc_place, msg_place, msg_borrow, union_type_name) =
+            self.describe_place_for_conflicting_borrow(place, &issued_borrow.borrowed_place);
 
         let explanation = self.explain_why_borrow_contains_point(context, issued_borrow, None);
         let second_borrow_desc = if explanation.is_explained() {
@@ -340,6 +338,8 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
         };
 
         // FIXME: supply non-"" `opt_via` when appropriate
+        let tcx = self.infcx.tcx;
+        let first_borrow_desc;
         let mut err = match (
             gen_borrow_kind,
             "immutable",
@@ -353,12 +353,12 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
                 tcx.cannot_reborrow_already_borrowed(
                     span,
                     &desc_place,
-                    "",
+                    &msg_place,
                     lft,
                     issued_span,
                     "it",
                     rgt,
-                    "",
+                    &msg_borrow,
                     None,
                     Origin::Mir,
                 )
@@ -368,12 +368,12 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
                 tcx.cannot_reborrow_already_borrowed(
                     span,
                     &desc_place,
-                    "",
+                    &msg_place,
                     lft,
                     issued_span,
                     "it",
                     rgt,
-                    "",
+                    &msg_borrow,
                     None,
                     Origin::Mir,
                 )
@@ -384,9 +384,9 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
                 tcx.cannot_mutably_borrow_multiply(
                     span,
                     &desc_place,
-                    "",
+                    &msg_place,
                     issued_span,
-                    "",
+                    &msg_borrow,
                     None,
                     Origin::Mir,
                 )
@@ -510,12 +510,118 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
             );
         }
 
+        if union_type_name != "" {
+            err.note(&format!(
+                "`{}` is a field of the union `{}`, so it overlaps the field `{}`",
+                msg_place, union_type_name, msg_borrow,
+            ));
+        }
+
         explanation
             .add_explanation_to_diagnostic(self.infcx.tcx, self.mir, &mut err, first_borrow_desc);
 
         err.buffer(&mut self.errors_buffer);
     }
 
+    /// Returns the description of the root place for a conflicting borrow and the full
+    /// descriptions of the places that caused the conflict.
+    ///
+    /// In the simplest case, where there are no unions involved, if a mutable borrow of `x` is
+    /// attempted while a shared borrow is live, then this function will return:
+    ///
+    ///     ("x", "", "")
+    ///
+    /// In the simple union case, if a mutable borrow of a union field `x.z` is attempted while
+    /// a shared borrow of another field `x.y`, then this function will return:
+    ///
+    ///     ("x", "x.z", "x.y")
+    ///
+    /// In the more complex union case, where the union is a field of a struct, then if a mutable
+    /// borrow of a union field in a struct `x.u.z` is attempted while a shared borrow of
+    /// another field `x.u.y`, then this function will return:
+    ///
+    ///     ("x.u", "x.u.z", "x.u.y")
+    ///
+    /// This is used when creating error messages like below:
+    ///
+    /// >  cannot borrow `a.u` (via `a.u.z.c`) as immutable because it is also borrowed as
+    /// >  mutable (via `a.u.s.b`) [E0502]
+    pub(super) fn describe_place_for_conflicting_borrow(
+        &self,
+        first_borrowed_place: &Place<'tcx>,
+        second_borrowed_place: &Place<'tcx>,
+    ) -> (String, String, String, String) {
+        // Define a small closure that we can use to check if the type of a place
+        // is a union.
+        let is_union = |place: &Place<'tcx>| -> bool {
+            place.ty(self.mir, self.infcx.tcx)
+                .to_ty(self.infcx.tcx)
+                .ty_adt_def()
+                .map(|adt| adt.is_union())
+                .unwrap_or(false)
+        };
+
+        // Start with an empty tuple, so we can use the functions on `Option` to reduce some
+        // code duplication (particularly around returning an empty description in the failure
+        // case).
+        Some(())
+            .filter(|_| {
+                // If we have a conflicting borrow of the same place, then we don't want to add
+                // an extraneous "via x.y" to our diagnostics, so filter out this case.
+                first_borrowed_place != second_borrowed_place
+            })
+            .and_then(|_| {
+                // We're going to want to traverse the first borrowed place to see if we can find
+                // field access to a union. If we find that, then we will keep the place of the
+                // union being accessed and the field that was being accessed so we can check the
+                // second borrowed place for the same union and a access to a different field.
+                let mut current = first_borrowed_place;
+                while let Place::Projection(box PlaceProjection { base, elem }) = current {
+                    match elem {
+                        ProjectionElem::Field(field, _) if is_union(base) => {
+                            return Some((base, field));
+                        },
+                        _ => current = base,
+                    }
+                }
+                None
+            })
+            .and_then(|(target_base, target_field)| {
+                // With the place of a union and a field access into it, we traverse the second
+                // borrowed place and look for a access to a different field of the same union.
+                let mut current = second_borrowed_place;
+                while let Place::Projection(box PlaceProjection { base, elem }) = current {
+                    match elem {
+                        ProjectionElem::Field(field, _) if {
+                            is_union(base) && field != target_field && base == target_base
+                        } => {
+                            let desc_base = self.describe_place(base)
+                                .unwrap_or_else(|| "_".to_owned());
+                            let desc_first = self.describe_place(first_borrowed_place)
+                                .unwrap_or_else(|| "_".to_owned());
+                            let desc_second = self.describe_place(second_borrowed_place)
+                                .unwrap_or_else(|| "_".to_owned());
+
+                            // Also compute the name of the union type, eg. `Foo` so we
+                            // can add a helpful note with it.
+                            let ty = base.ty(self.mir, self.infcx.tcx).to_ty(self.infcx.tcx);
+
+                            return Some((desc_base, desc_first, desc_second, ty.to_string()));
+                        },
+                        _ => current = base,
+                    }
+                }
+                None
+            })
+            .unwrap_or_else(|| {
+                // If we didn't find a field access into a union, or both places match, then
+                // only return the description of the first place.
+                let desc_place = self.describe_place(first_borrowed_place)
+                    .unwrap_or_else(|| "_".to_owned());
+                (desc_place, "".to_string(), "".to_string(), "".to_string())
+            })
+    }
+
     /// Reports StorageDeadOrDrop of `place` conflicts with `borrow`.
     ///
     /// This means that some data referenced by `borrow` needs to live

src/librustc_mir/util/borrowck_errors.rs

@@ -138,13 +138,15 @@ pub trait BorrowckErrors<'cx>: Sized + Copy {
         old_load_end_span: Option<Span>,
         o: Origin,
     ) -> DiagnosticBuilder<'cx> {
+        let via = |msg: &str|
+            if msg.is_empty() { msg.to_string() } else { format!(" (via `{}`)", msg) };
         let mut err = struct_span_err!(
             self,
             new_loan_span,
             E0499,
             "cannot borrow `{}`{} as mutable more than once at a time{OGN}",
             desc,
-            opt_via,
+            via(opt_via),
             OGN = o
         );
         if old_loan_span == new_loan_span {
@@ -164,11 +166,11 @@ pub trait BorrowckErrors<'cx>: Sized + Copy {
         } else {
             err.span_label(
                 old_loan_span,
-                format!("first mutable borrow occurs here{}", old_opt_via),
+                format!("first mutable borrow occurs here{}", via(old_opt_via)),
             );
             err.span_label(
                 new_loan_span,
-                format!("second mutable borrow occurs here{}", opt_via),
+                format!("second mutable borrow occurs here{}", via(opt_via)),
             );
             if let Some(old_load_end_span) = old_load_end_span {
                 err.span_label(old_load_end_span, "first borrow ends here");
@@ -292,27 +294,46 @@ pub trait BorrowckErrors<'cx>: Sized + Copy {
         old_load_end_span: Option<Span>,
         o: Origin,
     ) -> DiagnosticBuilder<'cx> {
+        let via = |msg: &str|
+            if msg.is_empty() { msg.to_string() } else { format!(" (via `{}`)", msg) };
         let mut err = struct_span_err!(
             self,
             span,
             E0502,
-            "cannot borrow `{}`{} as {} because {} is also borrowed as {}{}{OGN}",
+            "cannot borrow `{}`{} as {} because {} is also borrowed \
+             as {}{}{OGN}",
             desc_new,
-            msg_new,
+            via(msg_new),
             kind_new,
             noun_old,
             kind_old,
-            msg_old,
+            via(msg_old),
             OGN = o
         );
-        err.span_label(span, format!("{} borrow occurs here{}", kind_new, msg_new));
-        err.span_label(
-            old_span,
-            format!("{} borrow occurs here{}", kind_old, msg_old),
-        );
+
+        if msg_new == "" {
+            // If `msg_new` is empty, then this isn't a borrow of a union field.
+            err.span_label(span, format!("{} borrow occurs here", kind_new));
+            err.span_label(old_span, format!("{} borrow occurs here", kind_old));
+        } else {
+            // If `msg_new` isn't empty, then this a borrow of a union field.
+            err.span_label(
+                span,
+                format!(
+                    "{} borrow of `{}` -- which overlaps with `{}` -- occurs here",
+                    kind_new, msg_new, msg_old,
+                )
+            );
+            err.span_label(
+                old_span,
+                format!("{} borrow occurs here{}", kind_old, via(msg_old)),
+            );
+        }
+
         if let Some(old_load_end_span) = old_load_end_span {
             err.span_label(old_load_end_span, format!("{} borrow ends here", kind_old));
         }
+
         self.cancel_if_wrong_origin(err, o)
     }
 

src/test/ui/borrowck/borrowck-borrow-from-owned-ptr.stderr

@@ -138,7 +138,7 @@ error[E0502]: cannot borrow `foo` (via `foo.bar2`) as immutable because `foo` is
 LL |     let bar1 = &mut foo.bar1;
    |                     -------- mutable borrow occurs here (via `foo.bar1`)
 LL |     let _foo1 = &foo.bar2; //~ ERROR cannot borrow
-   |                  ^^^^^^^^ immutable borrow occurs here (via `foo.bar2`)
+   |                  ^^^^^^^^ immutable borrow of `foo.bar2` -- which overlaps with `foo.bar1` -- occurs here
 LL |     *bar1;
 LL | }
    | - mutable borrow ends here

src/test/ui/borrowck/borrowck-box-insensitivity.ast.stderr

@@ -61,7 +61,7 @@ error[E0502]: cannot borrow `a` (via `a.y`) as immutable because `a` is also bor
 LL |     let _x = &mut a.x;
    |                   --- mutable borrow occurs here (via `a.x`)
 LL |     let _y = &a.y; //[ast]~ ERROR cannot borrow
-   |               ^^^ immutable borrow occurs here (via `a.y`)
+   |               ^^^ immutable borrow of `a.y` -- which overlaps with `a.x` -- occurs here
 ...
 LL | }
    | - mutable borrow ends here
@@ -72,7 +72,7 @@ error[E0502]: cannot borrow `a` (via `a.y`) as mutable because `a` is also borro
 LL |     let _x = &a.x;
    |               --- immutable borrow occurs here (via `a.x`)
 LL |     let _y = &mut a.y; //[ast]~ ERROR cannot borrow
-   |                   ^^^ mutable borrow occurs here (via `a.y`)
+   |                   ^^^ mutable borrow of `a.y` -- which overlaps with `a.x` -- occurs here
 ...
 LL | }
    | - immutable borrow ends here

src/test/ui/borrowck/borrowck-box-insensitivity.rs

@@ -83,14 +83,14 @@ fn borrow_after_mut_borrow() {
     let mut a: Box<_> = box A { x: box 0, y: 1 };
     let _x = &mut a.x;
     let _y = &a.y; //[ast]~ ERROR cannot borrow
-    //[ast]~^ immutable borrow occurs here (via `a.y`)
+    //[ast]~^ immutable borrow of `a.y` -- which overlaps with `a.x` -- occurs here
     use_mut(_x);
 }
 fn mut_borrow_after_borrow() {
     let mut a: Box<_> = box A { x: box 0, y: 1 };
     let _x = &a.x;
     let _y = &mut a.y; //[ast]~ ERROR cannot borrow
-    //[ast]~^ mutable borrow occurs here (via `a.y`)
+    //[ast]~^ mutable borrow of `a.y` -- which overlaps with `a.x` -- occurs here
     use_imm(_x);
 }
 fn copy_after_move_nested() {