From e9029cec7b7acf4e94aaef051bb85aabee64394a Mon Sep 17 00:00:00 2001 From: Santiago Pastorino Date: Sat, 15 Sep 2018 12:29:29 -0300 Subject: [PATCH 1/5] Inspect parents paths when checking for moves --- .../borrow_check/error_reporting.rs | 13 ++++++++- src/librustc_mir/dataflow/move_paths/mod.rs | 14 ++++++++++ src/test/ui/nll/issue-52669.rs | 28 +++++++++++++++++++ src/test/ui/nll/issue-52669.stderr | 13 +++++++++ src/test/ui/nll/move-subpaths-moves-root.rs | 17 +++++++++++ .../ui/nll/move-subpaths-moves-root.stderr | 13 +++++++++ 6 files changed, 97 insertions(+), 1 deletion(-) create mode 100644 src/test/ui/nll/issue-52669.rs create mode 100644 src/test/ui/nll/issue-52669.stderr create mode 100644 src/test/ui/nll/move-subpaths-moves-root.rs create mode 100644 src/test/ui/nll/move-subpaths-moves-root.stderr diff --git a/src/librustc_mir/borrow_check/error_reporting.rs b/src/librustc_mir/borrow_check/error_reporting.rs index 4671332f2824c..b52a9c16ff7ea 100644 --- a/src/librustc_mir/borrow_check/error_reporting.rs +++ b/src/librustc_mir/borrow_check/error_reporting.rs @@ -564,9 +564,20 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { // created by `StorageDead` and at the beginning // of a function. } else { + // If we are found a use of a.b.c which was in error, then we want to look for + // moves not only of a.b.c but also a.b and a. + // + // Note that the moves data already includes "parent" paths, so we don't have to + // worry about the other case: that is, if there is a move of a.b.c, it is already + // marked as a move of a.b and a as well, so we will generate the correct errors + // there. + let mut mpis = vec![mpi]; + let move_paths = &self.move_data.move_paths; + mpis.extend(move_paths[mpi].parents(move_paths)); + for moi in &self.move_data.loc_map[l] { debug!("report_use_of_moved_or_uninitialized: moi={:?}", moi); - if self.move_data.moves[*moi].path == mpi { + if mpis.contains(&self.move_data.moves[*moi].path) { debug!("report_use_of_moved_or_uninitialized: found"); result.push(*moi); diff --git a/src/librustc_mir/dataflow/move_paths/mod.rs b/src/librustc_mir/dataflow/move_paths/mod.rs index 7d7da6c96e869..58a2b9361032e 100644 --- a/src/librustc_mir/dataflow/move_paths/mod.rs +++ b/src/librustc_mir/dataflow/move_paths/mod.rs @@ -97,6 +97,20 @@ pub struct MovePath<'tcx> { pub place: Place<'tcx>, } +impl<'tcx> MovePath<'tcx> { + pub fn parents(&self, move_paths: &IndexVec) -> Vec { + let mut parents = Vec::new(); + + let mut curr_parent = self.parent; + while let Some(parent_mpi) = curr_parent { + parents.push(parent_mpi); + curr_parent = move_paths[parent_mpi].parent; + } + + parents + } +} + impl<'tcx> fmt::Debug for MovePath<'tcx> { fn fmt(&self, w: &mut fmt::Formatter) -> fmt::Result { write!(w, "MovePath {{")?; diff --git a/src/test/ui/nll/issue-52669.rs b/src/test/ui/nll/issue-52669.rs new file mode 100644 index 0000000000000..17a59997e91ea --- /dev/null +++ b/src/test/ui/nll/issue-52669.rs @@ -0,0 +1,28 @@ +// 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 or the MIT license +// , at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. + +#![feature(nll)] + +struct A { + b: B, +} + +#[derive(Clone)] +struct B; + +fn foo(_: A) {} + +fn bar(mut a: A) -> B { + a.b = B; + foo(a); + a.b.clone() +} + +fn main() {} diff --git a/src/test/ui/nll/issue-52669.stderr b/src/test/ui/nll/issue-52669.stderr new file mode 100644 index 0000000000000..ca1576fad7a86 --- /dev/null +++ b/src/test/ui/nll/issue-52669.stderr @@ -0,0 +1,13 @@ +error[E0382]: borrow of moved value: `a.b` + --> $DIR/issue-52669.rs:25:5 + | +LL | foo(a); + | - value moved here +LL | a.b.clone() + | ^^^ value borrowed here after move + | + = note: move occurs because `a` has type `A`, which does not implement the `Copy` trait + +error: aborting due to previous error + +For more information about this error, try `rustc --explain E0382`. diff --git a/src/test/ui/nll/move-subpaths-moves-root.rs b/src/test/ui/nll/move-subpaths-moves-root.rs new file mode 100644 index 0000000000000..7a4e518f97778 --- /dev/null +++ b/src/test/ui/nll/move-subpaths-moves-root.rs @@ -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 or the MIT license +// , at your +// option. This file may not be copied, modified, or distributed +// except according to those terms. + +#![feature(nll)] + +fn main() { + let x = (vec![1, 2, 3], ); + drop(x.0); + drop(x); +} diff --git a/src/test/ui/nll/move-subpaths-moves-root.stderr b/src/test/ui/nll/move-subpaths-moves-root.stderr new file mode 100644 index 0000000000000..76a1279750ff3 --- /dev/null +++ b/src/test/ui/nll/move-subpaths-moves-root.stderr @@ -0,0 +1,13 @@ +error[E0382]: use of moved value: `x` + --> $DIR/move-subpaths-moves-root.rs:16:10 + | +LL | drop(x.0); + | --- value moved here +LL | drop(x); + | ^ value used here after move + | + = note: move occurs because `x.0` has type `std::vec::Vec`, which does not implement the `Copy` trait + +error: aborting due to previous error + +For more information about this error, try `rustc --explain E0382`. From 1f0fbddfff60f7f5d35b9008ab3f0ffb8133bb52 Mon Sep 17 00:00:00 2001 From: "Felix S. Klock II" Date: Tue, 18 Sep 2018 01:47:53 +0200 Subject: [PATCH 2/5] Fine tune dianostics for when a borrow conflicts with a destructor that needs exclusive access. In particular: 1. Extend `WriteKind::StorageDeadOrDrop` with state to track whether we are running a destructor or just freeing backing storage. (As part of this, when we drop a Box<....> where `T` does not need drop, we now signal that the drop of `T` is a kind of storage dead rather than a drop.) 2. When reporting that a value does not live long enough, check if we're doing an "interesting" drop, i.e. we aren't just trivally freeing the borrowed state, but rather a user-defined dtor will run and potentially require exclusive aces to the borrowed state. 3. Added a new diagnosic to describe the scenario here. --- .../borrow_check/error_reporting.rs | 93 ++++++++++++++++++- src/librustc_mir/borrow_check/mod.rs | 44 +++++---- .../borrow_check/nll/explain_borrow/mod.rs | 2 +- .../borrow_check/nll/invalidation.rs | 8 +- src/librustc_mir/diagnostics.rs | 45 +++++++++ src/librustc_mir/util/borrowck_errors.rs | 16 ++++ 6 files changed, 187 insertions(+), 21 deletions(-) diff --git a/src/librustc_mir/borrow_check/error_reporting.rs b/src/librustc_mir/borrow_check/error_reporting.rs index 4671332f2824c..73db525eb5691 100644 --- a/src/librustc_mir/borrow_check/error_reporting.rs +++ b/src/librustc_mir/borrow_check/error_reporting.rs @@ -8,7 +8,8 @@ // option. This file may not be copied, modified, or distributed // except according to those terms. -use borrow_check::WriteKind; +use borrow_check::{WriteKind, StorageDeadOrDrop}; +use borrow_check::prefixes::IsPrefixOf; use rustc::middle::region::ScopeTree; use rustc::mir::VarBindingForm; use rustc::mir::{BindingForm, BorrowKind, ClearCrossCrate, Field, Local}; @@ -374,6 +375,14 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { err.buffer(&mut self.errors_buffer); } + /// Reports StorageDeadOrDrop of `place` conflicts with `borrow`. + /// + /// This means that some data referenced by `borrow` needs to live + /// past the point where the StorageDeadOrDrop of `place` occurs. + /// This is usually interpreted as meaning that `place` has too + /// short a lifetime. (But sometimes it is more useful to report + /// it as a more direct conflict between the execution of a + /// `Drop::drop` with an aliasing borrow.) pub(super) fn report_borrowed_value_does_not_live_long_enough( &mut self, context: Context, @@ -381,6 +390,12 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { place_span: (&Place<'tcx>, Span), kind: Option, ) { + debug!("report_borrowed_value_does_not_live_long_enough(\ + {:?}, {:?}, {:?}, {:?}\ + )", + context, borrow, place_span, kind + ); + let drop_span = place_span.1; let scope_tree = self.tcx.region_scope_tree(self.mir_def_id); let root_place = self @@ -412,6 +427,19 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { let borrow_reason = self.find_why_borrow_contains_point(context, borrow); + if let Some(WriteKind::StorageDeadOrDrop(StorageDeadOrDrop::Destructor)) = kind + { + // If a borrow of path `B` conflicts with drop of `D` (and + // we're not in the uninteresting case where `B` is a + // prefix of `D`), then report this as a more interesting + // destructor conflict. + if !borrow.borrowed_place.is_prefix_of(place_span.0) { + self.report_borrow_conflicts_with_destructor( + context, borrow, borrow_reason, place_span, kind); + return; + } + } + let mut err = match &self.describe_place(&borrow.borrowed_place) { Some(_) if self.is_place_thread_local(root_place) => { self.report_thread_local_value_does_not_live_long_enough(drop_span, borrow_span) @@ -475,6 +503,69 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { err } + pub(super) fn report_borrow_conflicts_with_destructor( + &mut self, + context: Context, + borrow: &BorrowData<'tcx>, + borrow_reason: BorrowContainsPointReason<'tcx>, + place_span: (&Place<'tcx>, Span), + kind: Option, + ) { + debug!( + "report_borrow_conflicts_with_destructor(\ + {:?}, {:?}, {:?}, {:?} {:?}\ + )", + context, borrow, borrow_reason, place_span, kind, + ); + + let borrow_spans = self.retrieve_borrow_spans(borrow); + let borrow_span = borrow_spans.var_or_use(); + + let mut err = self.tcx.cannot_borrow_across_destructor(borrow_span, Origin::Mir); + + let drop_span = place_span.1; + + let (what_was_dropped, dropped_ty) = { + let place = place_span.0; + let desc = match self.describe_place(place) { + Some(name) => format!("`{}`", name.as_str()), + None => format!("temporary value"), + }; + let ty = place.ty(self.mir, self.tcx).to_ty(self.tcx); + (desc, ty) + }; + + let label = match dropped_ty.sty { + ty::Adt(adt, _) if adt.has_dtor(self.tcx) && !adt.is_box() => { + match self.describe_place(&borrow.borrowed_place) { + Some(borrowed) => + format!("here, drop of {D} needs exclusive access to `{B}`, \ + because the type `{T}` implements the `Drop` trait", + D=what_was_dropped, T=dropped_ty, B=borrowed), + None => + format!("here is drop of {D}; whose type `{T}` implements the `Drop` trait", + D=what_was_dropped, T=dropped_ty), + } + } + _ => format!("drop of {D} occurs here", D=what_was_dropped), + }; + err.span_label(drop_span, label); + + // Only give this note and suggestion if they could be relevant + match borrow_reason { + BorrowContainsPointReason::Liveness {..} + | BorrowContainsPointReason::DropLiveness {..} => { + err.note("consider using a `let` binding to create a longer lived value"); + } + BorrowContainsPointReason::OutlivesFreeRegion {..} => (), + } + + self.report_why_borrow_contains_point( + &mut err, borrow_reason, kind.map(|k| (k, place_span.0))); + + err.buffer(&mut self.errors_buffer); + } + fn report_thread_local_value_does_not_live_long_enough( &mut self, drop_span: Span, diff --git a/src/librustc_mir/borrow_check/mod.rs b/src/librustc_mir/borrow_check/mod.rs index 3c694fe7b4e58..22cdc5d6f40ee 100644 --- a/src/librustc_mir/borrow_check/mod.rs +++ b/src/librustc_mir/borrow_check/mod.rs @@ -551,7 +551,8 @@ impl<'cx, 'gcx, 'tcx> DataflowResultsConsumer<'cx, 'tcx> for MirBorrowckCtxt<'cx self.access_place( ContextKind::StorageDead.new(location), (&Place::Local(local), span), - (Shallow(None), Write(WriteKind::StorageDeadOrDrop)), + (Shallow(None), Write(WriteKind::StorageDeadOrDrop( + StorageDeadOrDrop::LocalStorageDead))), LocalMutationIsAllowed::Yes, flow_state, ); @@ -778,12 +779,21 @@ enum ReadKind { /// (For informational purposes only) #[derive(Copy, Clone, PartialEq, Eq, Debug)] enum WriteKind { - StorageDeadOrDrop, + StorageDeadOrDrop(StorageDeadOrDrop), MutableBorrow(BorrowKind), Mutate, Move, } +/// Specify whether which case a StorageDeadOrDrop is in. +/// (For informational purposes only) +#[derive(Copy, Clone, PartialEq, Eq, Debug)] +enum StorageDeadOrDrop { + LocalStorageDead, + BoxedStorageDead, + Destructor, +} + /// When checking permissions for a place access, this flag is used to indicate that an immutable /// local place can be mutated. /// @@ -1012,7 +1022,8 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { self.access_place( ContextKind::Drop.new(loc), (drop_place, span), - (Deep, Write(WriteKind::StorageDeadOrDrop)), + (Deep, Write(WriteKind::StorageDeadOrDrop( + StorageDeadOrDrop::Destructor))), LocalMutationIsAllowed::Yes, flow_state, ); @@ -1039,15 +1050,17 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { // Why? Because we do not schedule/emit // `Drop(x)` in the MIR unless `x` needs drop in // the first place. - // - // FIXME: Its possible this logic actually should - // be attached to the `StorageDead` statement - // rather than the `Drop`. See discussion on PR - // #52782. self.access_place( ContextKind::Drop.new(loc), (drop_place, span), - (Shallow(None), Write(WriteKind::StorageDeadOrDrop)), + (Shallow(None), Write(WriteKind::StorageDeadOrDrop( + // rust-lang/rust#52059: distinguish + // between invaliding the backing storage + // vs running a destructor. + // + // See also: rust-lang/rust#52782, + // specifically #discussion_r206658948 + StorageDeadOrDrop::BoxedStorageDead))), LocalMutationIsAllowed::Yes, flow_state, ); @@ -1215,14 +1228,13 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { error_reported = true; this.report_conflicting_borrow(context, place_span, bk, &borrow) } - WriteKind::StorageDeadOrDrop => { + WriteKind::StorageDeadOrDrop(_) => { error_reported = true; this.report_borrowed_value_does_not_live_long_enough( context, borrow, place_span, - Some(kind), - ); + Some(kind)) } WriteKind::Mutate => { error_reported = true; @@ -1464,7 +1476,7 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { } } - /// Returns whether a borrow of this place is invalidated when the function + /// Checks whether a borrow of this place is invalidated when the function /// exits fn check_for_invalidation_at_exit( &mut self, @@ -1889,9 +1901,9 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { Reservation(wk @ WriteKind::Move) | Write(wk @ WriteKind::Move) - | Reservation(wk @ WriteKind::StorageDeadOrDrop) + | Reservation(wk @ WriteKind::StorageDeadOrDrop(_)) | Reservation(wk @ WriteKind::MutableBorrow(BorrowKind::Shared)) - | Write(wk @ WriteKind::StorageDeadOrDrop) + | Write(wk @ WriteKind::StorageDeadOrDrop(_)) | Write(wk @ WriteKind::MutableBorrow(BorrowKind::Shared)) => { if let Err(_place_err) = self.is_mutable(place, is_local_mutation_allowed) { if self.tcx.migrate_borrowck() { @@ -1906,7 +1918,7 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { error_access = match wk { WriteKind::MutableBorrow(_) => AccessKind::MutableBorrow, WriteKind::Move => AccessKind::Move, - WriteKind::StorageDeadOrDrop | + WriteKind::StorageDeadOrDrop(_) | WriteKind::Mutate => AccessKind::Mutate, }; self.report_mutability_error( diff --git a/src/librustc_mir/borrow_check/nll/explain_borrow/mod.rs b/src/librustc_mir/borrow_check/nll/explain_borrow/mod.rs index 414cb1d6f05c2..8a44895a97c61 100644 --- a/src/librustc_mir/borrow_check/nll/explain_borrow/mod.rs +++ b/src/librustc_mir/borrow_check/nll/explain_borrow/mod.rs @@ -154,7 +154,7 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> { format!("borrow later used here, when `{}` is dropped", local_name), ); - if let Some((WriteKind::StorageDeadOrDrop, place)) = kind_place { + if let Some((WriteKind::StorageDeadOrDrop(_), place)) = kind_place { if let Place::Local(borrowed_local) = place { let dropped_local_scope = mir.local_decls[local].visibility_scope; let borrowed_local_scope = diff --git a/src/librustc_mir/borrow_check/nll/invalidation.rs b/src/librustc_mir/borrow_check/nll/invalidation.rs index 71345f22e443b..81f7fa89e9945 100644 --- a/src/librustc_mir/borrow_check/nll/invalidation.rs +++ b/src/librustc_mir/borrow_check/nll/invalidation.rs @@ -16,7 +16,7 @@ use borrow_check::{ReadOrWrite, Activation, Read, Reservation, Write}; use borrow_check::{Context, ContextKind}; use borrow_check::{LocalMutationIsAllowed, MutateMode}; use borrow_check::ArtificialField; -use borrow_check::{ReadKind, WriteKind}; +use borrow_check::{ReadKind, WriteKind, StorageDeadOrDrop}; use borrow_check::nll::facts::AllFacts; use borrow_check::path_utils::*; use dataflow::move_paths::indexes::BorrowIndex; @@ -154,7 +154,8 @@ impl<'cg, 'cx, 'tcx, 'gcx> Visitor<'tcx> for InvalidationGenerator<'cg, 'cx, 'tc self.access_place( ContextKind::StorageDead.new(location), &Place::Local(local), - (Shallow(None), Write(WriteKind::StorageDeadOrDrop)), + (Shallow(None), Write(WriteKind::StorageDeadOrDrop( + StorageDeadOrDrop::LocalStorageDead))), LocalMutationIsAllowed::Yes, ); } @@ -347,7 +348,8 @@ impl<'cg, 'cx, 'tcx, 'gcx> InvalidationGenerator<'cg, 'cx, 'tcx, 'gcx> { self.access_place( ContextKind::Drop.new(loc), drop_place, - (Deep, Write(WriteKind::StorageDeadOrDrop)), + (Deep, Write(WriteKind::StorageDeadOrDrop( + StorageDeadOrDrop::Destructor))), LocalMutationIsAllowed::Yes, ); } diff --git a/src/librustc_mir/diagnostics.rs b/src/librustc_mir/diagnostics.rs index ae4713a65de63..a09eece026616 100644 --- a/src/librustc_mir/diagnostics.rs +++ b/src/librustc_mir/diagnostics.rs @@ -2187,6 +2187,51 @@ fn main() { ``` "##, +E0713: r##" +This error occurs when an attempt is made to borrow state past the end of the +lifetime of a type that implements the `Drop` trait. + +Example of erroneous code: + +```compile_fail,E0713 +pub struct S<'a> { data: &'a mut String } + +impl<'a> Drop for S<'a> { + fn drop(&mut self) { self.data.push_str("being dropped"); } +} + +fn demo(s: S) -> &mut String { let p = &mut *s.data; p } +``` + +Here, `demo` tries to borrow the string data held within its +argument `s` and then return that borrow. However, `S` is +declared as implementing `Drop`. + +Structs implementing the `Drop` trait have an implicit destructor that +gets called when they go out of scope. This destructor gets exclusive +access to the fields of the struct when it runs. + +This means that when `s` reaches the end of `demo`, its destructor +gets exclusive access to its `&mut`-borrowed string data. allowing +another borrow of that string data (`p`), to exist across the drop of +`s` would be a violation of the principle that `&mut`-borrows have +exclusive, unaliased access to their referenced data. + +This error can be fixed by changing `demo` so that the destructor does +not run while the string-data is borrowed; for example by taking `S` +by reference: + +``` +pub struct S<'a> { data: &'a mut String } + +impl<'a> Drop for S<'a> { + fn drop(&mut self) { self.data.push_str("being dropped"); } +} + +fn demo(s: &mut S) -> &mut String { let p = &mut *(*s).data; p } +``` +"##, + } register_diagnostics! { diff --git a/src/librustc_mir/util/borrowck_errors.rs b/src/librustc_mir/util/borrowck_errors.rs index b67780ccdbc10..82617ee107470 100644 --- a/src/librustc_mir/util/borrowck_errors.rs +++ b/src/librustc_mir/util/borrowck_errors.rs @@ -573,6 +573,22 @@ pub trait BorrowckErrors<'cx>: Sized + Copy { self.cancel_if_wrong_origin(err, o) } + fn cannot_borrow_across_destructor( + self, + borrow_span: Span, + o: Origin, + ) -> DiagnosticBuilder<'cx> { + let err = struct_span_err!( + self, + borrow_span, + E0713, + "borrow may still be in use when destructor runs{OGN}", + OGN = o + ); + + self.cancel_if_wrong_origin(err, o) + } + fn path_does_not_live_long_enough( self, span: Span, From 673cd6efbe176e85c1198d9640f457aef2a53071 Mon Sep 17 00:00:00 2001 From: "Felix S. Klock II" Date: Tue, 18 Sep 2018 01:52:31 +0200 Subject: [PATCH 3/5] Updates to tests reflecting the diangostic changes in previous commit. It is worth pointing out that the reason that so few diagnostics are effected is because of the filter I put in where it only goes down the new path if the borrowed place is *not* a prefix of the dropped place. (Without that filter, a *lot* of the tests would need this change, and it would probably be a net loss for the UX, since you'd see it even in cases like borrows of generic types where there is no explicit mention of `Drop`.) --- .../borrowck-fn-in-const-c.nll.stderr | 8 +++--- ...96-scribble-on-boxed-borrow.migrate.stderr | 26 +++++++++---------- ...-45696-scribble-on-boxed-borrow.nll.stderr | 26 +++++++++---------- .../issue-45696-scribble-on-boxed-borrow.rs | 12 ++++----- src/test/ui/nll/issue-31567.rs | 2 +- src/test/ui/nll/issue-31567.stderr | 10 +++---- .../span/borrowck-ref-into-rvalue.nll.stderr | 12 ++++----- .../ui/span/issue28498-reject-ex1.nll.stderr | 9 ++++--- 8 files changed, 53 insertions(+), 52 deletions(-) diff --git a/src/test/ui/borrowck/borrowck-fn-in-const-c.nll.stderr b/src/test/ui/borrowck/borrowck-fn-in-const-c.nll.stderr index 827b66cf068b4..5c70294a1b90c 100644 --- a/src/test/ui/borrowck/borrowck-fn-in-const-c.nll.stderr +++ b/src/test/ui/borrowck/borrowck-fn-in-const-c.nll.stderr @@ -1,13 +1,13 @@ -error[E0597]: `local.inner` does not live long enough +error[E0713]: borrow may still be in use when destructor runs --> $DIR/borrowck-fn-in-const-c.rs:27:16 | LL | return &local.inner; //~ ERROR does not live long enough - | ^^^^^^^^^^^^ borrowed value does not live long enough + | ^^^^^^^^^^^^ LL | } - | - `local.inner` dropped here while still borrowed + | - here, drop of `local` needs exclusive access to `local.inner`, because the type `DropString` implements the `Drop` trait | = note: borrowed value must be valid for the static lifetime... error: aborting due to previous error -For more information about this error, try `rustc --explain E0597`. +For more information about this error, try `rustc --explain E0713`. diff --git a/src/test/ui/issues/issue-45696-scribble-on-boxed-borrow.migrate.stderr b/src/test/ui/issues/issue-45696-scribble-on-boxed-borrow.migrate.stderr index da0dfac2d18b1..70d819f0f4678 100644 --- a/src/test/ui/issues/issue-45696-scribble-on-boxed-borrow.migrate.stderr +++ b/src/test/ui/issues/issue-45696-scribble-on-boxed-borrow.migrate.stderr @@ -1,11 +1,11 @@ -warning[E0597]: `*s.0` does not live long enough +warning[E0713]: borrow may still be in use when destructor runs --> $DIR/issue-45696-scribble-on-boxed-borrow.rs:63:5 | -LL | &mut *s.0 //[nll]~ ERROR `*s.0` does not live long enough [E0597] - | ^^^^^^^^^ borrowed value does not live long enough +LL | &mut *s.0 //[nll]~ ERROR borrow may still be in use when destructor runs [E0713] + | ^^^^^^^^^ ... LL | } - | - `*s.0` dropped here while still borrowed + | - here, drop of `s` needs exclusive access to `*s.0`, because the type `Scribble<'_>` implements the `Drop` trait | note: borrowed value must be valid for the lifetime 'a as defined on the function body at 62:14... --> $DIR/issue-45696-scribble-on-boxed-borrow.rs:62:14 @@ -16,14 +16,14 @@ LL | fn scribbled<'a>(s: Scribble<'a>) -> &'a mut u32 { It represents potential unsoundness in your code. This warning will become a hard error in the future. -warning[E0597]: `*s.0` does not live long enough +warning[E0713]: borrow may still be in use when destructor runs --> $DIR/issue-45696-scribble-on-boxed-borrow.rs:73:5 | -LL | &mut *(*s).0 //[nll]~ ERROR `*s.0` does not live long enough [E0597] - | ^^^^^^^^^^^^ borrowed value does not live long enough +LL | &mut *(*s).0 //[nll]~ ERROR borrow may still be in use when destructor runs [E0713] + | ^^^^^^^^^^^^ ... LL | } - | - `*s.0` dropped here while still borrowed + | - here, drop of `*s` needs exclusive access to `*s.0`, because the type `Scribble<'_>` implements the `Drop` trait | note: borrowed value must be valid for the lifetime 'a as defined on the function body at 72:20... --> $DIR/issue-45696-scribble-on-boxed-borrow.rs:72:20 @@ -34,14 +34,14 @@ LL | fn boxed_scribbled<'a>(s: Box>) -> &'a mut u32 { It represents potential unsoundness in your code. This warning will become a hard error in the future. -warning[E0597]: `*s.0` does not live long enough +warning[E0713]: borrow may still be in use when destructor runs --> $DIR/issue-45696-scribble-on-boxed-borrow.rs:83:5 | -LL | &mut *(**s).0 //[nll]~ ERROR `*s.0` does not live long enough [E0597] - | ^^^^^^^^^^^^^ borrowed value does not live long enough +LL | &mut *(**s).0 //[nll]~ ERROR borrow may still be in use when destructor runs [E0713] + | ^^^^^^^^^^^^^ ... LL | } - | - `*s.0` dropped here while still borrowed + | - here, drop of `**s` needs exclusive access to `*s.0`, because the type `Scribble<'_>` implements the `Drop` trait | note: borrowed value must be valid for the lifetime 'a as defined on the function body at 82:26... --> $DIR/issue-45696-scribble-on-boxed-borrow.rs:82:26 @@ -66,4 +66,4 @@ LL | | } error: aborting due to previous error -For more information about this error, try `rustc --explain E0597`. +For more information about this error, try `rustc --explain E0713`. diff --git a/src/test/ui/issues/issue-45696-scribble-on-boxed-borrow.nll.stderr b/src/test/ui/issues/issue-45696-scribble-on-boxed-borrow.nll.stderr index 09cbc2f945129..72ec5affb182b 100644 --- a/src/test/ui/issues/issue-45696-scribble-on-boxed-borrow.nll.stderr +++ b/src/test/ui/issues/issue-45696-scribble-on-boxed-borrow.nll.stderr @@ -1,11 +1,11 @@ -error[E0597]: `*s.0` does not live long enough +error[E0713]: borrow may still be in use when destructor runs --> $DIR/issue-45696-scribble-on-boxed-borrow.rs:63:5 | -LL | &mut *s.0 //[nll]~ ERROR `*s.0` does not live long enough [E0597] - | ^^^^^^^^^ borrowed value does not live long enough +LL | &mut *s.0 //[nll]~ ERROR borrow may still be in use when destructor runs [E0713] + | ^^^^^^^^^ ... LL | } - | - `*s.0` dropped here while still borrowed + | - here, drop of `s` needs exclusive access to `*s.0`, because the type `Scribble<'_>` implements the `Drop` trait | note: borrowed value must be valid for the lifetime 'a as defined on the function body at 62:14... --> $DIR/issue-45696-scribble-on-boxed-borrow.rs:62:14 @@ -13,14 +13,14 @@ note: borrowed value must be valid for the lifetime 'a as defined on the functio LL | fn scribbled<'a>(s: Scribble<'a>) -> &'a mut u32 { | ^^ -error[E0597]: `*s.0` does not live long enough +error[E0713]: borrow may still be in use when destructor runs --> $DIR/issue-45696-scribble-on-boxed-borrow.rs:73:5 | -LL | &mut *(*s).0 //[nll]~ ERROR `*s.0` does not live long enough [E0597] - | ^^^^^^^^^^^^ borrowed value does not live long enough +LL | &mut *(*s).0 //[nll]~ ERROR borrow may still be in use when destructor runs [E0713] + | ^^^^^^^^^^^^ ... LL | } - | - `*s.0` dropped here while still borrowed + | - here, drop of `*s` needs exclusive access to `*s.0`, because the type `Scribble<'_>` implements the `Drop` trait | note: borrowed value must be valid for the lifetime 'a as defined on the function body at 72:20... --> $DIR/issue-45696-scribble-on-boxed-borrow.rs:72:20 @@ -28,14 +28,14 @@ note: borrowed value must be valid for the lifetime 'a as defined on the functio LL | fn boxed_scribbled<'a>(s: Box>) -> &'a mut u32 { | ^^ -error[E0597]: `*s.0` does not live long enough +error[E0713]: borrow may still be in use when destructor runs --> $DIR/issue-45696-scribble-on-boxed-borrow.rs:83:5 | -LL | &mut *(**s).0 //[nll]~ ERROR `*s.0` does not live long enough [E0597] - | ^^^^^^^^^^^^^ borrowed value does not live long enough +LL | &mut *(**s).0 //[nll]~ ERROR borrow may still be in use when destructor runs [E0713] + | ^^^^^^^^^^^^^ ... LL | } - | - `*s.0` dropped here while still borrowed + | - here, drop of `**s` needs exclusive access to `*s.0`, because the type `Scribble<'_>` implements the `Drop` trait | note: borrowed value must be valid for the lifetime 'a as defined on the function body at 82:26... --> $DIR/issue-45696-scribble-on-boxed-borrow.rs:82:26 @@ -45,4 +45,4 @@ LL | fn boxed_boxed_scribbled<'a>(s: Box>>) -> &'a mut u32 { error: aborting due to 3 previous errors -For more information about this error, try `rustc --explain E0597`. +For more information about this error, try `rustc --explain E0713`. diff --git a/src/test/ui/issues/issue-45696-scribble-on-boxed-borrow.rs b/src/test/ui/issues/issue-45696-scribble-on-boxed-borrow.rs index 5a4874249e2f4..2af05172d24db 100644 --- a/src/test/ui/issues/issue-45696-scribble-on-boxed-borrow.rs +++ b/src/test/ui/issues/issue-45696-scribble-on-boxed-borrow.rs @@ -60,8 +60,8 @@ fn boxed_boxed_borrowed_scribble<'a>(s: Box>) -> &'a mut u // this should be an error. (Which is perhaps the essence of why // rust-lang/rust#45696 arose in the first place.) fn scribbled<'a>(s: Scribble<'a>) -> &'a mut u32 { - &mut *s.0 //[nll]~ ERROR `*s.0` does not live long enough [E0597] - //[migrate]~^ WARNING `*s.0` does not live long enough [E0597] + &mut *s.0 //[nll]~ ERROR borrow may still be in use when destructor runs [E0713] + //[migrate]~^ WARNING borrow may still be in use when destructor runs [E0713] //[migrate]~| WARNING This error has been downgraded to a warning for backwards compatibility } @@ -70,8 +70,8 @@ fn scribbled<'a>(s: Scribble<'a>) -> &'a mut u32 { // (But again, AST-borrowck was not smart enogh to know that this // should be an error.) fn boxed_scribbled<'a>(s: Box>) -> &'a mut u32 { - &mut *(*s).0 //[nll]~ ERROR `*s.0` does not live long enough [E0597] - //[migrate]~^ WARNING `*s.0` does not live long enough [E0597] + &mut *(*s).0 //[nll]~ ERROR borrow may still be in use when destructor runs [E0713] + //[migrate]~^ WARNING borrow may still be in use when destructor runs [E0713] //[migrate]~| WARNING This error has been downgraded to a warning for backwards compatibility } @@ -80,8 +80,8 @@ fn boxed_scribbled<'a>(s: Box>) -> &'a mut u32 { // (But again, AST-borrowck was not smart enogh to know that this // should be an error.) fn boxed_boxed_scribbled<'a>(s: Box>>) -> &'a mut u32 { - &mut *(**s).0 //[nll]~ ERROR `*s.0` does not live long enough [E0597] - //[migrate]~^ WARNING `*s.0` does not live long enough [E0597] + &mut *(**s).0 //[nll]~ ERROR borrow may still be in use when destructor runs [E0713] + //[migrate]~^ WARNING borrow may still be in use when destructor runs [E0713] //[migrate]~| WARNING This error has been downgraded to a warning for backwards compatibility } diff --git a/src/test/ui/nll/issue-31567.rs b/src/test/ui/nll/issue-31567.rs index a0d1faf1f0e84..3508f3033e62e 100644 --- a/src/test/ui/nll/issue-31567.rs +++ b/src/test/ui/nll/issue-31567.rs @@ -19,7 +19,7 @@ struct VecWrapper<'a>(&'a mut S); struct S(Box); fn get_dangling<'a>(v: VecWrapper<'a>) -> &'a u32 { - let s_inner: &'a S = &*v.0; //~ ERROR `*v.0` does not live long enough + let s_inner: &'a S = &*v.0; //~ ERROR borrow may still be in use when destructor runs [E0713] &s_inner.0 } diff --git a/src/test/ui/nll/issue-31567.stderr b/src/test/ui/nll/issue-31567.stderr index 532bc493e7dbc..63330f3031981 100644 --- a/src/test/ui/nll/issue-31567.stderr +++ b/src/test/ui/nll/issue-31567.stderr @@ -1,11 +1,11 @@ -error[E0597]: `*v.0` does not live long enough +error[E0713]: borrow may still be in use when destructor runs --> $DIR/issue-31567.rs:22:26 | -LL | let s_inner: &'a S = &*v.0; //~ ERROR `*v.0` does not live long enough - | ^^^^^ borrowed value does not live long enough +LL | let s_inner: &'a S = &*v.0; //~ ERROR borrow may still be in use when destructor runs [E0713] + | ^^^^^ LL | &s_inner.0 LL | } - | - `*v.0` dropped here while still borrowed + | - here, drop of `v` needs exclusive access to `*v.0`, because the type `VecWrapper<'_>` implements the `Drop` trait | note: borrowed value must be valid for the lifetime 'a as defined on the function body at 21:17... --> $DIR/issue-31567.rs:21:17 @@ -15,4 +15,4 @@ LL | fn get_dangling<'a>(v: VecWrapper<'a>) -> &'a u32 { error: aborting due to previous error -For more information about this error, try `rustc --explain E0597`. +For more information about this error, try `rustc --explain E0713`. diff --git a/src/test/ui/span/borrowck-ref-into-rvalue.nll.stderr b/src/test/ui/span/borrowck-ref-into-rvalue.nll.stderr index c565842c2c002..c94558f12bbde 100644 --- a/src/test/ui/span/borrowck-ref-into-rvalue.nll.stderr +++ b/src/test/ui/span/borrowck-ref-into-rvalue.nll.stderr @@ -1,11 +1,11 @@ -error[E0597]: borrowed value does not live long enough - --> $DIR/borrowck-ref-into-rvalue.rs:13:11 +error[E0713]: borrow may still be in use when destructor runs + --> $DIR/borrowck-ref-into-rvalue.rs:14:14 | -LL | match Some("Hello".to_string()) { - | ^^^^^^^^^^^^^^^^^^^^^^^^^ temporary value does not live long enough +LL | Some(ref m) => { + | ^^^^^ ... LL | } - | - temporary value only lives until here + | - drop of temporary value occurs here LL | println!("{}", *msg); | ---- borrow later used here | @@ -13,4 +13,4 @@ LL | println!("{}", *msg); error: aborting due to previous error -For more information about this error, try `rustc --explain E0597`. +For more information about this error, try `rustc --explain E0713`. diff --git a/src/test/ui/span/issue28498-reject-ex1.nll.stderr b/src/test/ui/span/issue28498-reject-ex1.nll.stderr index 27eb4a3afed6d..1f72b78ebc76d 100644 --- a/src/test/ui/span/issue28498-reject-ex1.nll.stderr +++ b/src/test/ui/span/issue28498-reject-ex1.nll.stderr @@ -1,17 +1,18 @@ -error[E0597]: `foo.data` does not live long enough +error[E0713]: borrow may still be in use when destructor runs --> $DIR/issue28498-reject-ex1.rs:44:29 | LL | foo.data[0].1.set(Some(&foo.data[1])); - | ^^^^^^^^ borrowed value does not live long enough + | ^^^^^^^^ ... LL | } | - | | - | `foo.data` dropped here while still borrowed + | here, drop of `foo` needs exclusive access to `foo.data`, because the type `Foo>` implements the `Drop` trait | borrow later used here, when `foo` is dropped | + = note: consider using a `let` binding to create a longer lived value = note: values in a scope are dropped in the opposite order they are defined error: aborting due to previous error -For more information about this error, try `rustc --explain E0597`. +For more information about this error, try `rustc --explain E0713`. From e3b611f61954c565b550a6f6ae42715f56db0d85 Mon Sep 17 00:00:00 2001 From: "Felix S. Klock II" Date: Tue, 18 Sep 2018 02:03:54 +0200 Subject: [PATCH 4/5] Regression test for this particular change. --- ...59-report-when-borrow-and-drop-conflict.rs | 33 ++++++++++ ...eport-when-borrow-and-drop-conflict.stderr | 61 +++++++++++++++++++ 2 files changed, 94 insertions(+) create mode 100644 src/test/ui/nll/issue-52059-report-when-borrow-and-drop-conflict.rs create mode 100644 src/test/ui/nll/issue-52059-report-when-borrow-and-drop-conflict.stderr diff --git a/src/test/ui/nll/issue-52059-report-when-borrow-and-drop-conflict.rs b/src/test/ui/nll/issue-52059-report-when-borrow-and-drop-conflict.rs new file mode 100644 index 0000000000000..fff73c6d0fa9c --- /dev/null +++ b/src/test/ui/nll/issue-52059-report-when-borrow-and-drop-conflict.rs @@ -0,0 +1,33 @@ +// rust-lang/rust#52059: Regardless of whether you are moving out of a +// Drop type or just introducing an inadvertant alias via a borrow of +// one of its fields, it is useful to be reminded of the significance +// of the fact that the type implements Drop. + +#![feature(nll)] +#![allow(dead_code)] + +pub struct S<'a> { url: &'a mut String } + +impl<'a> Drop for S<'a> { fn drop(&mut self) { } } + +fn finish_1(s: S) -> &mut String { + s.url +} +//~^^ ERROR borrow may still be in use when destructor runs + +fn finish_2(s: S) -> &mut String { + let p = &mut *s.url; p +} +//~^^ ERROR borrow may still be in use when destructor runs + +fn finish_3(s: S) -> &mut String { + let p: &mut _ = s.url; p +} +//~^^ ERROR borrow may still be in use when destructor runs + +fn finish_4(s: S) -> &mut String { + let p = s.url; p +} +//~^^ ERROR cannot move out of type `S<'_>`, which implements the `Drop` trait + +fn main() {} diff --git a/src/test/ui/nll/issue-52059-report-when-borrow-and-drop-conflict.stderr b/src/test/ui/nll/issue-52059-report-when-borrow-and-drop-conflict.stderr new file mode 100644 index 0000000000000..71c97b7ad6b80 --- /dev/null +++ b/src/test/ui/nll/issue-52059-report-when-borrow-and-drop-conflict.stderr @@ -0,0 +1,61 @@ +error[E0713]: borrow may still be in use when destructor runs + --> $DIR/issue-52059-report-when-borrow-and-drop-conflict.rs:14:5 + | +LL | s.url + | ^^^^^ +LL | } + | - here, drop of `s` needs exclusive access to `*s.url`, because the type `S<'_>` implements the `Drop` trait + | +note: borrowed value must be valid for the anonymous lifetime #1 defined on the function body at 13:1... + --> $DIR/issue-52059-report-when-borrow-and-drop-conflict.rs:13:1 + | +LL | / fn finish_1(s: S) -> &mut String { +LL | | s.url +LL | | } + | |_^ + +error[E0713]: borrow may still be in use when destructor runs + --> $DIR/issue-52059-report-when-borrow-and-drop-conflict.rs:19:13 + | +LL | let p = &mut *s.url; p + | ^^^^^^^^^^^ +LL | } + | - here, drop of `s` needs exclusive access to `*s.url`, because the type `S<'_>` implements the `Drop` trait + | +note: borrowed value must be valid for the anonymous lifetime #1 defined on the function body at 18:1... + --> $DIR/issue-52059-report-when-borrow-and-drop-conflict.rs:18:1 + | +LL | / fn finish_2(s: S) -> &mut String { +LL | | let p = &mut *s.url; p +LL | | } + | |_^ + +error[E0713]: borrow may still be in use when destructor runs + --> $DIR/issue-52059-report-when-borrow-and-drop-conflict.rs:24:21 + | +LL | let p: &mut _ = s.url; p + | ^^^^^ +LL | } + | - here, drop of `s` needs exclusive access to `*s.url`, because the type `S<'_>` implements the `Drop` trait + | +note: borrowed value must be valid for the anonymous lifetime #1 defined on the function body at 23:1... + --> $DIR/issue-52059-report-when-borrow-and-drop-conflict.rs:23:1 + | +LL | / fn finish_3(s: S) -> &mut String { +LL | | let p: &mut _ = s.url; p +LL | | } + | |_^ + +error[E0509]: cannot move out of type `S<'_>`, which implements the `Drop` trait + --> $DIR/issue-52059-report-when-borrow-and-drop-conflict.rs:29:13 + | +LL | let p = s.url; p + | ^^^^^ + | | + | cannot move out of here + | help: consider borrowing here: `&s.url` + +error: aborting due to 4 previous errors + +Some errors occurred: E0509, E0713. +For more information about an error, try `rustc --explain E0509`. From c9cf4993307b9623580b1fe7f12aa87df1225fb8 Mon Sep 17 00:00:00 2001 From: "Felix S. Klock II" Date: Tue, 18 Sep 2018 12:18:31 +0200 Subject: [PATCH 5/5] Address following error from rustdoc tests: error[E0106]: missing lifetime specifier --> /checkout/obj/build/x86_64-unknown-linux-gnu/test/error-index.md:11424:23 | 9 | fn demo(s: &mut S) -> &mut String { let p = &mut *(*s).data; p } | ^ expected lifetime parameter | = help: this function's return type contains a borrowed value, but the signature does not say which one of `s`'s 2 lifetimes it is borrowed from --- src/librustc_mir/diagnostics.rs | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/src/librustc_mir/diagnostics.rs b/src/librustc_mir/diagnostics.rs index a09eece026616..24197c9e4b88f 100644 --- a/src/librustc_mir/diagnostics.rs +++ b/src/librustc_mir/diagnostics.rs @@ -2194,13 +2194,15 @@ lifetime of a type that implements the `Drop` trait. Example of erroneous code: ```compile_fail,E0713 +#![feature(nll)] + pub struct S<'a> { data: &'a mut String } impl<'a> Drop for S<'a> { fn drop(&mut self) { self.data.push_str("being dropped"); } } -fn demo(s: S) -> &mut String { let p = &mut *s.data; p } +fn demo<'a>(s: S<'a>) -> &'a mut String { let p = &mut *s.data; p } ``` Here, `demo` tries to borrow the string data held within its @@ -2222,14 +2224,24 @@ not run while the string-data is borrowed; for example by taking `S` by reference: ``` +#![feature(nll)] + pub struct S<'a> { data: &'a mut String } impl<'a> Drop for S<'a> { fn drop(&mut self) { self.data.push_str("being dropped"); } } -fn demo(s: &mut S) -> &mut String { let p = &mut *(*s).data; p } +fn demo<'a>(s: &'a mut S<'a>) -> &'a mut String { let p = &mut *(*s).data; p } ``` + +Note that this approach needs a reference to S with lifetime `'a`. +Nothing shorter than `'a` will suffice: a shorter lifetime would imply +that after `demo` finishes excuting, something else (such as the +destructor!) could access `s.data` after the end of that shorter +lifetime, which would again violate the `&mut`-borrow's exclusive +access. + "##, }