Auto merge of #53995 - davidtwco:issue-53807, r=nikomatsakis

NLL: Deduplicate errors for incorrect move in loop

Fixes #53807.

r? @nikomatsakis

src/Cargo.lock

@@ -2183,6 +2183,7 @@ name = "rustc_errors"
 version = "0.0.0"
 dependencies = [
  "atty 0.2.11 (registry+https://github.com/rust-lang/crates.io-index)",
+ "log 0.4.4 (registry+https://github.com/rust-lang/crates.io-index)",
  "rustc_cratesio_shim 0.0.0",
  "rustc_data_structures 0.0.0",
  "serialize 0.0.0",

src/librustc_errors/Cargo.toml

@@ -9,6 +9,7 @@ path = "lib.rs"
 crate-type = ["dylib"]
 
 [dependencies]
+log = "0.4"
 serialize = { path = "../libserialize" }
 syntax_pos = { path = "../libsyntax_pos" }
 rustc_data_structures = { path = "../librustc_data_structures" }

src/librustc_errors/diagnostic_builder.rs

@@ -108,6 +108,9 @@ pub fn buffer(mut self, buffered_diagnostics: &mut Vec<Diagnostic>) {
             diagnostic = ::std::ptr::read(&self.diagnostic);
             ::std::mem::forget(self);
         };
+        // Logging here is useful to help track down where in logs an error was
+        // actually emitted.
+        debug!("buffer: diagnostic={:?}", diagnostic);
         buffered_diagnostics.push(diagnostic);
     }
 

src/librustc_errors/lib.rs

@@ -23,6 +23,8 @@
 extern crate termcolor;
 #[cfg(unix)]
 extern crate libc;
+#[macro_use]
+extern crate log;
 extern crate rustc_data_structures;
 extern crate serialize as rustc_serialize;
 extern crate syntax_pos;

src/librustc_mir/borrow_check/error_reporting.rs

@@ -38,6 +38,12 @@ pub(super) fn report_use_of_moved_or_uninitialized(
         (place, span): (&Place<'tcx>, Span),
         mpi: MovePathIndex,
     ) {
+        debug!(
+            "report_use_of_moved_or_uninitialized: context={:?} desired_action={:?} place={:?} \
+            span={:?} mpi={:?}",
+            context, desired_action, place, span, mpi
+        );
+
         let use_spans = self
             .move_spans(place, context.loc)
             .or_else(|| self.borrow_spans(span, context.loc));
@@ -49,7 +55,7 @@ pub(super) fn report_use_of_moved_or_uninitialized(
         if mois.is_empty() {
             let root_place = self.prefixes(&place, PrefixSet::All).last().unwrap();
 
-            if self.moved_error_reported.contains(&root_place.clone()) {
+            if self.uninitialized_error_reported.contains(&root_place.clone()) {
                 debug!(
                     "report_use_of_moved_or_uninitialized place: error about {:?} suppressed",
                     root_place
@@ -57,7 +63,7 @@ pub(super) fn report_use_of_moved_or_uninitialized(
                 return;
             }
 
-            self.moved_error_reported.insert(root_place.clone());
+            self.uninitialized_error_reported.insert(root_place.clone());
 
             let item_msg = match self.describe_place_with_options(place, IncludingDowncast(true)) {
                 Some(name) => format!("`{}`", name),
@@ -80,6 +86,14 @@ pub(super) fn report_use_of_moved_or_uninitialized(
 
             err.buffer(&mut self.errors_buffer);
         } else {
+            if let Some((reported_place, _)) = self.move_error_reported.get(&mois) {
+                if self.prefixes(&reported_place, PrefixSet::All).any(|p| p == place) {
+                    debug!("report_use_of_moved_or_uninitialized place: error suppressed \
+                           mois={:?}", mois);
+                    return;
+                }
+            }
+
             let msg = ""; //FIXME: add "partially " or "collaterally "
 
             let mut err = self.tcx.cannot_act_on_moved_value(
@@ -167,7 +181,13 @@ pub(super) fn report_use_of_moved_or_uninitialized(
                 }
             }
 
-            err.buffer(&mut self.errors_buffer);
+            if let Some((_, mut old_err)) = self.move_error_reported.insert(
+                mois,
+                (place.clone(), err)
+            ) {
+                // Cancel the old error so it doesn't ICE.
+                old_err.cancel();
+            }
         }
     }
 

src/librustc_mir/borrow_check/mod.rs

@@ -33,11 +33,13 @@
 use smallvec::SmallVec;
 
 use std::rc::Rc;
+use std::collections::BTreeMap;
 
 use syntax_pos::Span;
 
 use dataflow::indexes::BorrowIndex;
 use dataflow::move_paths::{HasMoveData, LookupResult, MoveData, MoveError, MovePathIndex};
+use dataflow::move_paths::indexes::MoveOutIndex;
 use dataflow::Borrows;
 use dataflow::DataflowResultsConsumer;
 use dataflow::FlowAtLocation;
@@ -255,7 +257,8 @@ fn do_mir_borrowck<'a, 'gcx, 'tcx>(
         locals_are_invalidated_at_exit,
         access_place_error_reported: FxHashSet(),
         reservation_error_reported: FxHashSet(),
-        moved_error_reported: FxHashSet(),
+        move_error_reported: BTreeMap::new(),
+        uninitialized_error_reported: FxHashSet(),
         errors_buffer,
         nonlexical_regioncx: regioncx,
         used_mut: FxHashSet(),
@@ -333,6 +336,11 @@ fn do_mir_borrowck<'a, 'gcx, 'tcx>(
         }
     }
 
+    // Buffer any move errors that we collected and de-duplicated.
+    for (_, (_, diag)) in mbcx.move_error_reported {
+        diag.buffer(&mut mbcx.errors_buffer);
+    }
+
     if mbcx.errors_buffer.len() > 0 {
         mbcx.errors_buffer.sort_by_key(|diag| diag.span.primary_span());
 
@@ -408,9 +416,24 @@ pub struct MirBorrowckCtxt<'cx, 'gcx: 'tcx, 'tcx: 'cx> {
     /// but it is currently inconvenient to track down the BorrowIndex
     /// at the time we detect and report a reservation error.
     reservation_error_reported: FxHashSet<Place<'tcx>>,
-    /// This field keeps track of errors reported in the checking of moved variables,
+    /// This field keeps track of move errors that are to be reported for given move indicies.
+    ///
+    /// There are situations where many errors can be reported for a single move out (see #53807)
+    /// and we want only the best of those errors.
+    ///
+    /// The `report_use_of_moved_or_uninitialized` function checks this map and replaces the
+    /// diagnostic (if there is one) if the `Place` of the error being reported is a prefix of the
+    /// `Place` of the previous most diagnostic. This happens instead of buffering the error. Once
+    /// all move errors have been reported, any diagnostics in this map are added to the buffer
+    /// to be emitted.
+    ///
+    /// `BTreeMap` is used to preserve the order of insertions when iterating. This is necessary
+    /// when errors in the map are being re-added to the error buffer so that errors with the
+    /// same primary span come out in a consistent order.
+    move_error_reported: BTreeMap<Vec<MoveOutIndex>, (Place<'tcx>, DiagnosticBuilder<'cx>)>,
+    /// This field keeps track of errors reported in the checking of uninitialized variables,
     /// so that we don't report seemingly duplicate errors.
-    moved_error_reported: FxHashSet<Place<'tcx>>,
+    uninitialized_error_reported: FxHashSet<Place<'tcx>>,
     /// Errors to be reported buffer
     errors_buffer: Vec<Diagnostic>,
     /// This field keeps track of all the local variables that are declared mut and are mutated.
@@ -801,7 +824,7 @@ enum LocalMutationIsAllowed {
     No,
 }
 
-#[derive(Copy, Clone)]
+#[derive(Copy, Clone, Debug)]
 enum InitializationRequiringAction {
     Update,
     Borrow,

src/test/ui/borrowck/issue-41962.rs

@@ -18,9 +18,6 @@ pub fn main(){
         }
         //~^^ ERROR use of partially moved value: `maybe` (Ast) [E0382]
         //~| ERROR use of moved value: `(maybe as std::prelude::v1::Some).0` (Ast) [E0382]
-        //~| ERROR use of moved value: `maybe` (Mir) [E0382]
-        //~| ERROR use of moved value: `maybe` (Mir) [E0382]
         //~| ERROR use of moved value (Mir) [E0382]
-        //~| ERROR borrow of moved value: `maybe` (Mir) [E0382]
     }
 }

src/test/ui/borrowck/issue-41962.stderr

@@ -16,17 +16,6 @@ LL |         if let Some(thing) = maybe {
    |
    = note: move occurs because the value has type `std::vec::Vec<bool>`, which does not implement the `Copy` trait
 
-error[E0382]: use of moved value: `maybe` (Mir)
-  --> $DIR/issue-41962.rs:17:16
-   |
-LL |         if let Some(thing) = maybe {
-   |                ^^^^^-----^
-   |                |    |
-   |                |    value moved here
-   |                value used here after move
-   |
-   = note: move occurs because value has type `std::vec::Vec<bool>`, which does not implement the `Copy` trait
-
 error[E0382]: use of moved value (Mir)
   --> $DIR/issue-41962.rs:17:21
    |
@@ -35,26 +24,6 @@ LL |         if let Some(thing) = maybe {
    |
    = note: move occurs because value has type `std::vec::Vec<bool>`, which does not implement the `Copy` trait
 
-error[E0382]: use of moved value: `maybe` (Mir)
-  --> $DIR/issue-41962.rs:17:30
-   |
-LL |         if let Some(thing) = maybe {
-   |                     -----    ^^^^^ value used here after move
-   |                     |
-   |                     value moved here
-   |
-   = note: move occurs because value has type `std::vec::Vec<bool>`, which does not implement the `Copy` trait
-
-error[E0382]: borrow of moved value: `maybe` (Mir)
-  --> $DIR/issue-41962.rs:17:30
-   |
-LL |         if let Some(thing) = maybe {
-   |                     -----    ^^^^^ value borrowed here after move
-   |                     |
-   |                     value moved here
-   |
-   = note: move occurs because value has type `std::vec::Vec<bool>`, which does not implement the `Copy` trait
-
-error: aborting due to 6 previous errors
+error: aborting due to 3 previous errors
 
 For more information about this error, try `rustc --explain E0382`.

src/test/ui/issues/issue-17385.nll.stderr

-error[E0382]: use of moved value: `foo`
-  --> $DIR/issue-17385.rs:28:11
-   |
-LL |     drop(foo);
-   |          --- value moved here
-LL |     match foo { //~ ERROR use of moved value
-   |           ^^^ value used here after move
-   |
-   = note: move occurs because `foo` has type `X`, which does not implement the `Copy` trait
-
-error[E0382]: borrow of moved value: `foo`
-  --> $DIR/issue-17385.rs:28:11
-   |
-LL |     drop(foo);
-   |          --- value moved here
-LL |     match foo { //~ ERROR use of moved value
-   |           ^^^ value borrowed here after move
-   |
-   = note: move occurs because `foo` has type `X`, which does not implement the `Copy` trait
-
 error[E0382]: use of moved value: `foo.0`
   --> $DIR/issue-17385.rs:29:11
    |
@@ -39,27 +19,6 @@ LL |     match e { //~ ERROR use of moved value
    |
    = note: move occurs because `e` has type `Enum`, which does not implement the `Copy` trait
 
-error[E0382]: borrow of moved value: `e`
-  --> $DIR/issue-17385.rs:35:11
-   |
-LL |     drop(e);
-   |          - value moved here
-LL |     match e { //~ ERROR use of moved value
-   |           ^ value borrowed here after move
-   |
-   = note: move occurs because `e` has type `Enum`, which does not implement the `Copy` trait
-
-error[E0382]: use of moved value: `e`
-  --> $DIR/issue-17385.rs:36:9
-   |
-LL |     drop(e);
-   |          - value moved here
-LL |     match e { //~ ERROR use of moved value
-LL |         Enum::Variant1 => unreachable!(),
-   |         ^^^^^^^^^^^^^^ value used here after move
-   |
-   = note: move occurs because `e` has type `Enum`, which does not implement the `Copy` trait
-
-error: aborting due to 6 previous errors
+error: aborting due to 2 previous errors
 
 For more information about this error, try `rustc --explain E0382`.

src/test/ui/liveness/liveness-move-in-while.nll.stderr

@@ -8,14 +8,6 @@ LL |         while true { while true { while true { x = y; x.clone(); } } }
    |
    = note: move occurs because `y` has type `std::boxed::Box<isize>`, which does not implement the `Copy` trait
 
-error[E0382]: use of moved value: `y`
-  --> $DIR/liveness-move-in-while.rs:18:52
-   |
-LL |         while true { while true { while true { x = y; x.clone(); } } }
-   |                                                    ^ value moved here in previous iteration of loop
-   |
-   = note: move occurs because `y` has type `std::boxed::Box<isize>`, which does not implement the `Copy` trait
-
-error: aborting due to 2 previous errors
+error: aborting due to previous error
 
 For more information about this error, try `rustc --explain E0382`.

src/test/ui/nll/issue-53807.nll.stderr

+error[E0382]: use of moved value
+  --> $DIR/issue-53807.rs:14:21
+   |
+LL |         if let Some(thing) = maybe {
+   |                     ^^^^^ value moved here in previous iteration of loop
+   |
+   = note: move occurs because value has type `std::vec::Vec<bool>`, which does not implement the `Copy` trait
+
+error: aborting due to previous error
+
+For more information about this error, try `rustc --explain E0382`.

src/test/ui/nll/issue-53807.rs

+// Copyright 2018 The Rust Project Developers. See the COPYRIGHT
+// file at the top-level directory of this distribution and at
+// http://rust-lang.org/COPYRIGHT.
+//
+// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
+// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
+// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
+// option. This file may not be copied, modified, or distributed
+// except according to those terms.
+
+pub fn main(){
+    let maybe = Some(vec![true, true]);
+     loop {
+        if let Some(thing) = maybe {
+        }
+    }
+}

src/test/ui/nll/issue-53807.stderr

+error[E0382]: use of partially moved value: `maybe`
+  --> $DIR/issue-53807.rs:14:30
+   |
+LL |         if let Some(thing) = maybe {
+   |                     -----    ^^^^^ value used here after move
+   |                     |
+   |                     value moved here
+   |
+   = note: move occurs because the value has type `std::vec::Vec<bool>`, which does not implement the `Copy` trait
+
+error[E0382]: use of moved value: `(maybe as std::prelude::v1::Some).0`
+  --> $DIR/issue-53807.rs:14:21
+   |
+LL |         if let Some(thing) = maybe {
+   |                     ^^^^^ value moved here in previous iteration of loop
+   |
+   = note: move occurs because the value has type `std::vec::Vec<bool>`, which does not implement the `Copy` trait
+
+error: aborting due to 2 previous errors
+
+For more information about this error, try `rustc --explain E0382`.

src/test/ui/no-capture-arc.nll.stderr

@@ -11,19 +11,6 @@ LL |     assert_eq!((*arc_v)[2], 3);
    |
    = note: move occurs because `arc_v` has type `std::sync::Arc<std::vec::Vec<i32>>`, which does not implement the `Copy` trait
 
-error[E0382]: borrow of moved value: `arc_v`
-  --> $DIR/no-capture-arc.rs:26:23
-   |
-LL |     thread::spawn(move|| {
-   |                   ------ value moved into closure here
-LL |         assert_eq!((*arc_v)[3], 4);
-   |                      ----- variable moved due to use in closure
-...
-LL |     println!("{:?}", *arc_v);
-   |                       ^^^^^ value borrowed here after move
-   |
-   = note: move occurs because `arc_v` has type `std::sync::Arc<std::vec::Vec<i32>>`, which does not implement the `Copy` trait
-
-error: aborting due to 2 previous errors
+error: aborting due to previous error
 
 For more information about this error, try `rustc --explain E0382`.

src/test/ui/no-reuse-move-arc.nll.stderr

@@ -11,19 +11,6 @@ LL |     assert_eq!((*arc_v)[2], 3); //~ ERROR use of moved value: `arc_v`
    |
    = note: move occurs because `arc_v` has type `std::sync::Arc<std::vec::Vec<i32>>`, which does not implement the `Copy` trait
 
-error[E0382]: borrow of moved value: `arc_v`
-  --> $DIR/no-reuse-move-arc.rs:24:23
-   |
-LL |     thread::spawn(move|| {
-   |                   ------ value moved into closure here
-LL |         assert_eq!((*arc_v)[3], 4);
-   |                      ----- variable moved due to use in closure
-...
-LL |     println!("{:?}", *arc_v); //~ ERROR use of moved value: `arc_v`
-   |                       ^^^^^ value borrowed here after move
-   |
-   = note: move occurs because `arc_v` has type `std::sync::Arc<std::vec::Vec<i32>>`, which does not implement the `Copy` trait
-
-error: aborting due to 2 previous errors
+error: aborting due to previous error
 
 For more information about this error, try `rustc --explain E0382`.