Skip to content

NLL: Deduplicate errors for incorrect move in loop #53995

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Sep 19, 2018
Merged

NLL: Deduplicate errors for incorrect move in loop #53995

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
874e08b
Log when buffering a diagnostic.
davidtwco Sep 6, 2018
783bad4
De-duplicate moved variable errors.
davidtwco Sep 6, 2018
88ca341
Switched from FxHashMap to BTreeMap to preserve ordering when iterating.
davidtwco Sep 11, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/Cargo.lock
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
1 change: 1 addition & 0 deletions src/librustc_errors/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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" }
Expand Down
3 changes: 3 additions & 0 deletions src/librustc_errors/diagnostic_builder.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,9 @@ impl<'a> DiagnosticBuilder<'a> {
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);
}

Expand Down
2 changes: 2 additions & 0 deletions src/librustc_errors/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@ extern crate atty;
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;
Expand Down
26 changes: 23 additions & 3 deletions src/librustc_mir/borrow_check/error_reporting.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,12 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
(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));
Expand All @@ -49,15 +55,15 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
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
);
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),
Expand All @@ -80,6 +86,14 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {

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(
Expand Down Expand Up @@ -167,7 +181,13 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
}
}

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();
}
}
}

Expand Down
31 changes: 27 additions & 4 deletions src/librustc_mir/borrow_check/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,11 +33,13 @@ use rustc_data_structures::indexed_vec::Idx;
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;
Expand Down Expand Up @@ -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(),
Expand Down Expand Up @@ -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());

Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -801,7 +824,7 @@ enum LocalMutationIsAllowed {
No,
}

#[derive(Copy, Clone)]
#[derive(Copy, Clone, Debug)]
enum InitializationRequiringAction {
Update,
Borrow,
Expand Down
3 changes: 0 additions & 3 deletions src/test/ui/borrowck/issue-41962.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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]
}
}
33 changes: 1 addition & 32 deletions src/test/ui/borrowck/issue-41962.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
|
Expand All @@ -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`.
43 changes: 1 addition & 42 deletions src/test/ui/issues/issue-17385.nll.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,23 +1,3 @@
error[E0382]: use of moved value: `foo`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm. Do you have an intuition as to which of the errors are being preferred for presentation by this PR? I am a little surprised to see it favoring the error on line 29 here over the ones on line 28.

I guess I'd want to make sure we don't run into a scenario where a beginner erroneously infers that some statement is not performing a move (or does not have a use of the local, whatever), when in fact there is a move/use at that statement, but the diagnostic system is filtering the error mentioning it out from the error report.

Copy link
Member Author

@davidtwco davidtwco Sep 6, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR groups errors by the MoveOutIndex(s) that are relevant (however this error reporting function was coming to that conclusion previously) and then emitting, for that grouping, the error that corresponds to the place that is most "specific".

In this case, a more "specific" means a Place that is not a prefix of a less "specific" Place. So I just decline to create and buffer an error if the Place for that error is a prefix of the Place that I already have a buffered error for.

There's a log statement that indicates an error is being suppressed and you still get the log statement showing which arguments the reporting function was called with before it suppresses it. There's also a comment attempting to explain the above logic on the field that stores the buffered errors.

Copy link
Contributor

@nikomatsakis nikomatsakis Sep 7, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, @pnkfelix has an interesting point — or, at least, I that citing some point in a match arm as a "use" is more confusing than citing the match foo { itself.

Did you experiment with preferring the most general move? e.g., if the common prefix of the grouping is foo, perhaps preferring moves of foo?

That said, I don't think we should dump a ton of errors relating to a single move — I think that is more likely to confuse users. I could imagine trying to include more than one "use after move" point within the error itself.

e.g., we might display

 --> $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
LL | X(1) => (),
   |   - value also used here after move
   = note: move occurs because `foo` has type `X`, which does not implement the `Copy` trait

sorry the indentation doesn't line up, hopefully you get the idea

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or perhaps with a kind of "note" like

 --> $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
   = note: value is also used in other places after the move
LL | X(1) => (),
   |   -

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't try the the most general move. When I was digging into the example case from the issue, I thought the best error there was the one with the "used in previous iteration of loop" message, and I noticed it was on the most specific place, which is why I used that as my heuristic.

--> $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
|
Expand All @@ -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`.
10 changes: 1 addition & 9 deletions src/test/ui/liveness/liveness-move-in-while.nll.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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`.
11 changes: 11 additions & 0 deletions src/test/ui/nll/issue-53807.nll.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
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`.
17 changes: 17 additions & 0 deletions src/test/ui/nll/issue-53807.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
// 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 {
}
}
}
21 changes: 21 additions & 0 deletions src/test/ui/nll/issue-53807.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
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`.
15 changes: 1 addition & 14 deletions src/test/ui/no-capture-arc.nll.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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`.
15 changes: 1 addition & 14 deletions src/test/ui/no-reuse-move-arc.nll.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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`.