Skip to content

Custom error when moving arg outside of its closure #47144

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.

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 6 commits into from
Jan 22, 2018
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
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/librustc/infer/error_reporting/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1067,6 +1067,7 @@ impl<'a, 'gcx, 'tcx> InferCtxt<'a, 'gcx, 'tcx> {
sub_region: Region<'tcx>,
sup_origin: SubregionOrigin<'tcx>,
sup_region: Region<'tcx>) {

let mut err = self.report_inference_failure(var_origin);

self.tcx.note_and_explain_region(region_scope_tree, &mut err,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ impl<'a, 'gcx, 'tcx> NiceRegionError<'a, 'gcx, 'tcx> {
///
/// It will later be extended to trait objects.
pub(super) fn try_report_anon_anon_conflict(&self) -> Option<ErrorReported> {
let NiceRegionError { span, sub, sup, .. } = *self;
let (span, sub, sup) = self.get_regions();

// Determine whether the sub and sup consist of both anonymous (elided) regions.
let anon_reg_sup = self.is_suitable_region(sup)?;
Expand Down
40 changes: 29 additions & 11 deletions src/librustc/infer/error_reporting/nice_region_error/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,46 +18,64 @@ use util::common::ErrorReported;
mod different_lifetimes;
mod find_anon_type;
mod named_anon_conflict;
mod outlives_closure;
mod util;

impl<'cx, 'gcx, 'tcx> InferCtxt<'cx, 'gcx, 'tcx> {
pub fn try_report_nice_region_error(&self, error: &RegionResolutionError<'tcx>) -> bool {
let (span, sub, sup) = match *error {
ConcreteFailure(ref origin, sub, sup) => (origin.span(), sub, sup),
SubSupConflict(_, ref origin, sub, _, sup) => (origin.span(), sub, sup),
_ => return false, // inapplicable
};
match *error {
ConcreteFailure(..) | SubSupConflict(..) => {}
_ => return false, // inapplicable
}

if let Some(tables) = self.in_progress_tables {
let tables = tables.borrow();
NiceRegionError::new(self.tcx, span, sub, sup, Some(&tables)).try_report().is_some()
NiceRegionError::new(self.tcx, error.clone(), Some(&tables)).try_report().is_some()
} else {
NiceRegionError::new(self.tcx, span, sub, sup, None).try_report().is_some()
NiceRegionError::new(self.tcx, error.clone(), None).try_report().is_some()
}
}
}

pub struct NiceRegionError<'cx, 'gcx: 'tcx, 'tcx: 'cx> {
tcx: TyCtxt<'cx, 'gcx, 'tcx>,
span: Span,
sub: ty::Region<'tcx>,
sup: ty::Region<'tcx>,
error: Option<RegionResolutionError<'tcx>>,
regions: Option<(Span, ty::Region<'tcx>, ty::Region<'tcx>)>,
tables: Option<&'cx ty::TypeckTables<'tcx>>,
}

impl<'cx, 'gcx, 'tcx> NiceRegionError<'cx, 'gcx, 'tcx> {
pub fn new(
tcx: TyCtxt<'cx, 'gcx, 'tcx>,
error: RegionResolutionError<'tcx>,
tables: Option<&'cx ty::TypeckTables<'tcx>>,
) -> Self {
Self { tcx, error: Some(error), regions: None, tables }
}

pub fn new_from_span(
tcx: TyCtxt<'cx, 'gcx, 'tcx>,
span: Span,
sub: ty::Region<'tcx>,
sup: ty::Region<'tcx>,
tables: Option<&'cx ty::TypeckTables<'tcx>>,
) -> Self {
Self { tcx, span, sub, sup, tables }
Self { tcx, error: None, regions: Some((span, sub, sup)), tables }
}

pub fn try_report(&self) -> Option<ErrorReported> {
self.try_report_named_anon_conflict()
.or_else(|| self.try_report_anon_anon_conflict())
.or_else(|| self.try_report_outlives_closure())
}

pub fn get_regions(&self) -> (Span, ty::Region<'tcx>, ty::Region<'tcx>) {
match (&self.error, self.regions) {
(&Some(ConcreteFailure(ref origin, sub, sup)), None) => (origin.span(), sub, sup),
(&Some(SubSupConflict(_, ref origin, sub, _, sup)), None) => (origin.span(), sub, sup),
(None, Some((span, sub, sup))) => (span, sub, sup),
(Some(_), Some(_)) => panic!("incorrectly built NiceRegionError"),
_ => panic!("trying to report on an incorrect lifetime failure"),
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ impl<'a, 'gcx, 'tcx> NiceRegionError<'a, 'gcx, 'tcx> {
/// When given a `ConcreteFailure` for a function with arguments containing a named region and
/// an anonymous region, emit an descriptive diagnostic error.
pub(super) fn try_report_named_anon_conflict(&self) -> Option<ErrorReported> {
let NiceRegionError { span, sub, sup, .. } = *self;
let (span, sub, sup) = self.get_regions();

debug!(
"try_report_named_anon_conflict(sub={:?}, sup={:?})",
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,94 @@
// 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.

//! Error Reporting for Anonymous Region Lifetime Errors
//! where both the regions are anonymous.

use infer::error_reporting::nice_region_error::NiceRegionError;
use infer::SubregionOrigin;
use ty::RegionKind;
use hir::{Expr, ExprClosure};
use hir::map::NodeExpr;
use util::common::ErrorReported;
use infer::lexical_region_resolve::RegionResolutionError::SubSupConflict;

impl<'a, 'gcx, 'tcx> NiceRegionError<'a, 'gcx, 'tcx> {
/// Print the error message for lifetime errors when binding excapes a closure.
///
/// Consider a case where we have
///
/// ```no_run
/// fn with_int<F>(f: F) where F: FnOnce(&isize) {
/// let x = 3;
/// f(&x);
/// }
/// fn main() {
/// let mut x = None;
/// with_int(|y| x = Some(y));
/// }
/// ```
///
/// the output will be
///
/// ```text
/// let mut x = None;
/// ----- borrowed data cannot be stored into here...
/// with_int(|y| x = Some(y));
/// --- ^ cannot be stored outside of its closure
/// |
/// ...because it cannot outlive this closure
/// ```
pub(super) fn try_report_outlives_closure(&self) -> Option<ErrorReported> {
if let Some(SubSupConflict(origin,
ref sub_origin,
_,
ref sup_origin,
sup_region)) = self.error {

// #45983: when trying to assign the contents of an argument to a binding outside of a
// closure, provide a specific message pointing this out.
if let (&SubregionOrigin::BindingTypeIsNotValidAtDecl(ref external_span),
&RegionKind::ReFree(ref free_region)) = (&sub_origin, sup_region) {
let hir = &self.tcx.hir;
if let Some(node_id) = hir.as_local_node_id(free_region.scope) {
match hir.get(node_id) {
NodeExpr(Expr {
node: ExprClosure(_, _, _, closure_span, false),
..
}) => {
let sup_sp = sup_origin.span();
let origin_sp = origin.span();
let mut err = self.tcx.sess.struct_span_err(
sup_sp,
"borrowed data cannot be stored outside of its closure");
err.span_label(sup_sp, "cannot be stored outside of its closure");
if sup_sp == origin_sp {
err.span_label(*external_span,
"borrowed data cannot be stored into here...");
err.span_label(*closure_span,
"...because it cannot outlive this closure");
} else {
err.span_label(*closure_span,
"borrowed data cannot outlive this closure");
err.span_label(origin_sp,
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should just drop this label. I don't really think it conveys useful information to the end-user. The origin_sp is not especially meaningful -- it's basically a point where the region is relevant, but not necessarily a particularly good choice of point.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I feel uneasy about removing the span completely because it's the original error span, and want to point to it in some way. Modified so that it occurs in fewer cases, but will keep it for the #7573 case. I'm planning to revisit that and the type mismatch cases at a later PR.

"cannot infer an appropriate lifetime");
}
err.emit();
return Some(ErrorReported);
}
_ => {}
}
}
}
}
None
}
}

2 changes: 1 addition & 1 deletion src/librustc_mir/borrow_check/nll/region_infer/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -989,7 +989,7 @@ impl<'tcx> RegionInferenceContext<'tcx> {

if let (Some(f), Some(o)) = (fr_name, outlived_fr_name) {
let tables = infcx.tcx.typeck_tables_of(mir_def_id);
let nice = NiceRegionError::new(infcx.tcx, blame_span, o, f, Some(tables));
let nice = NiceRegionError::new_from_span(infcx.tcx, blame_span, o, f, Some(tables));
if let Some(ErrorReported) = nice.try_report() {
return;
}
Expand Down
19 changes: 19 additions & 0 deletions src/test/ui/borrowck/issue-45983.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
// 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.

fn give_any<F: for<'r> FnOnce(&'r ())>(f: F) {
f(&());
}

fn main() {
let x = None;
give_any(|y| x = Some(y));
//~^ ERROR borrowed data cannot be stored outside of its closure
}
12 changes: 12 additions & 0 deletions src/test/ui/borrowck/issue-45983.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
error: borrowed data cannot be stored outside of its closure
--> $DIR/issue-45983.rs:17:27
|
16 | let x = None;
| - borrowed data cannot be stored into here...
17 | give_any(|y| x = Some(y));
| --- ^ cannot be stored outside of its closure
| |
| ...because it cannot outlive this closure

error: aborting due to previous error
Copy link
Contributor

Choose a reason for hiding this comment

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

can you add a ui test for the other case you mentioned? The one where you push into a vector that exists outside the closure?

Copy link
Contributor Author

Choose a reason for hiding this comment

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


Original file line number Diff line number Diff line change
Expand Up @@ -24,9 +24,13 @@ impl CrateId {
}

pub fn remove_package_from_database() {
let mut lines_to_use: Vec<&CrateId> = Vec::new(); //~ ERROR E0495
let mut lines_to_use: Vec<&CrateId> = Vec::new();
//~^ NOTE cannot infer an appropriate lifetime
let push_id = |installed_id: &CrateId| {
//~^ NOTE borrowed data cannot outlive this closure
lines_to_use.push(installed_id);
//~^ ERROR borrowed data cannot be stored outside of its closure
//~| NOTE cannot be stored outside of its closure
};
list_database(push_id);

Expand Down
14 changes: 14 additions & 0 deletions src/test/ui/borrowck/issue-7573.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
error: borrowed data cannot be stored outside of its closure
--> $DIR/issue-7573.rs:31:27
|
27 | let mut lines_to_use: Vec<&CrateId> = Vec::new();
| - cannot infer an appropriate lifetime
28 | //~^ NOTE cannot infer an appropriate lifetime
29 | let push_id = |installed_id: &CrateId| {
| ------------------------ borrowed data cannot outlive this closure
30 | //~^ NOTE borrowed data cannot outlive this closure
31 | lines_to_use.push(installed_id);
| ^^^^^^^^^^^^ cannot be stored outside of its closure

error: aborting due to previous error

Original file line number Diff line number Diff line change
Expand Up @@ -16,5 +16,5 @@ fn with_int<F>(f: F) where F: FnOnce(&isize) {
fn main() {
let mut x = None;
with_int(|y| x = Some(y));
//~^ ERROR cannot infer
//~^ ERROR borrowed data cannot be stored outside of its closure
}
12 changes: 12 additions & 0 deletions src/test/ui/borrowck/regions-escape-bound-fn-2.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
error: borrowed data cannot be stored outside of its closure
--> $DIR/regions-escape-bound-fn-2.rs:18:27
|
17 | let mut x = None;
| ----- borrowed data cannot be stored into here...
18 | with_int(|y| x = Some(y));
| --- ^ cannot be stored outside of its closure
| |
| ...because it cannot outlive this closure

error: aborting due to previous error

Original file line number Diff line number Diff line change
Expand Up @@ -15,5 +15,6 @@ fn with_int<F>(f: F) where F: FnOnce(&isize) {

fn main() {
let mut x: Option<&isize> = None;
with_int(|y| x = Some(y)); //~ ERROR cannot infer
with_int(|y| x = Some(y));
//~^ ERROR borrowed data cannot be stored outside of its closure
}
12 changes: 12 additions & 0 deletions src/test/ui/borrowck/regions-escape-bound-fn.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
error: borrowed data cannot be stored outside of its closure
--> $DIR/regions-escape-bound-fn.rs:18:27
|
18 | with_int(|y| x = Some(y));
| --- -----^-
| | | |
| | | cannot be stored outside of its closure
| | cannot infer an appropriate lifetime
| borrowed data cannot outlive this closure
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nikomatsakis I'm not entirely happy with the output on this case :-/

This triggers the case I originally neglected and was trying to fix...

Should we keep this PR on hold until I have time to look at how to differentiate between these two cases for a more appropriate output? In my mind, this case should completely ignore the lifetime issue for Some(_) and point at the destination binding, like in the output of the original case for this PR:


error: borrowed data cannot be stored outside of its closure
  --> $DIR/regions-escape-bound-fn.rs:18:27
   |
17 |     let mut x: Option<&isize> = None;
   |            - borrowed data cannot be stored into here...
18 |     with_int(|y| x = Some(y));
   |              ---          ^ cannot be stored outside of its closure
   |              |
   |              ...because it cannot outlive this closure

It should be possible to do so as the current diagnostic does point at x's declaration:

error[E0495]: cannot infer an appropriate lifetime for automatic coercion due to conflicting requirements
 --> src/main.rs:8:22
  |
8 |     with_int(|y| x = Some(y));
  |                      ^^^^^^^
  |
note: first, the lifetime cannot outlive the anonymous lifetime #2 defined on the body at 8:14...
 --> src/main.rs:8:14
  |
8 |     with_int(|y| x = Some(y));
  |              ^^^^^^^^^^^^^^^
note: ...so that reference does not outlive borrowed content
 --> src/main.rs:8:27
  |
8 |     with_int(|y| x = Some(y));
  |                           ^
note: but, the lifetime must be valid for the block suffix following statement 0 at 7:5...
 --> src/main.rs:7:5
  |
7 | /     let mut x: Option<&isize> = None;
8 | |     with_int(|y| x = Some(y));
9 | |     //~^ ERROR borrowed data cannot be stored outside of its closure
10| | }
  | |_^
note: ...so that variable is valid at time of its declaration
 --> src/main.rs:7:9
  |
7 |     let mut x: Option<&isize> = None;
  |         ^^^^^

Also, should I create a new error code for each of these cases, one shared between all of them or reuse the existing one?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this output is way better than the original output. The main change I would do is not to print the "cannot infer an appropriate lifetime" label, which seems to add zero value. Saying that the data cannot escape the closure and indicating the closure is pretty awesome.


error: aborting due to previous error

Original file line number Diff line number Diff line change
Expand Up @@ -13,5 +13,6 @@ fn with_int(f: &mut FnMut(&isize)) {

fn main() {
let mut x: Option<&isize> = None;
with_int(&mut |y| x = Some(y)); //~ ERROR cannot infer
with_int(&mut |y| x = Some(y));
//~^ ERROR borrowed data cannot be stored outside of its closure
}
12 changes: 12 additions & 0 deletions src/test/ui/borrowck/regions-escape-unboxed-closure.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
error: borrowed data cannot be stored outside of its closure
--> $DIR/regions-escape-unboxed-closure.rs:16:32
|
16 | with_int(&mut |y| x = Some(y));
| --- -----^-
| | | |
| | | cannot be stored outside of its closure
| | cannot infer an appropriate lifetime
| borrowed data cannot outlive this closure

error: aborting due to previous error

Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ fn expect_bound_supply_nothing() {
// it to escape into `f`:
let mut f: Option<&u32> = None;
closure_expecting_bound(|x| {
f = Some(x); //~ ERROR E0495
f = Some(x); //~ ERROR borrowed data cannot be stored outside of its closure
});
}

Expand All @@ -35,7 +35,7 @@ fn expect_bound_supply_bound() {
// closure:
let mut f: Option<&u32> = None;
closure_expecting_bound(|x: &u32| {
f = Some(x); //~ ERROR E0495
f = Some(x); //~ ERROR borrowed data cannot be stored outside of its closure
});
}

Expand All @@ -50,7 +50,7 @@ fn expect_bound_supply_named<'x>() {

// And we still cannot let `x` escape into `f`.
f = Some(x);
//~^ ERROR cannot infer
//~^ ERROR borrowed data cannot be stored outside of its closure
});
}

Expand Down
Loading