Skip to content
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

Suggest move for closures and async blocks in more cases. #70906

Merged
merged 1 commit into from
Apr 9, 2020
Merged
Show file tree
Hide file tree
Changes from all 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
58 changes: 20 additions & 38 deletions src/librustc_mir/borrow_check/diagnostics/conflict_errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -760,47 +760,26 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
(
Some(ref name),
BorrowExplanation::MustBeValidFor {
category: category @ ConstraintCategory::Return,
category:
category
@
(ConstraintCategory::Return
| ConstraintCategory::CallArgument
| ConstraintCategory::OpaqueType),
Comment on lines +763 to +768
Copy link
Contributor

@Centril Centril Apr 7, 2020

Choose a reason for hiding this comment

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

Highlighting this for a rustfmt issue (rust-lang/rustfmt#4110) I'm filing (no need to do anything about it in this PR).

from_closure: false,
ref region_name,
span,
..
},
)
| (
Some(ref name),
BorrowExplanation::MustBeValidFor {
category: category @ ConstraintCategory::CallArgument,
from_closure: false,
ref region_name,
span,
..
},
) if borrow_spans.for_closure() => self.report_escaping_closure_capture(
borrow_spans,
borrow_span,
region_name,
category,
span,
&format!("`{}`", name),
),
(
Some(ref name),
BorrowExplanation::MustBeValidFor {
category: category @ ConstraintCategory::OpaqueType,
from_closure: false,
ref region_name,
) if borrow_spans.for_generator() | borrow_spans.for_closure() => self
.report_escaping_closure_capture(
borrow_spans,
borrow_span,
region_name,
category,
span,
..
},
) if borrow_spans.for_generator() => self.report_escaping_closure_capture(
borrow_spans,
borrow_span,
region_name,
category,
span,
&format!("`{}`", name),
),
&format!("`{}`", name),
),
(
ref name,
BorrowExplanation::MustBeValidFor {
Expand Down Expand Up @@ -1187,7 +1166,6 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
) -> DiagnosticBuilder<'cx> {
let tcx = self.infcx.tcx;
let args_span = use_span.args_or_use();
let mut err = self.cannot_capture_in_long_lived_closure(args_span, captured_var, var_span);

let suggestion = match tcx.sess.source_map().span_to_snippet(args_span) {
Ok(mut string) => {
Expand All @@ -1213,6 +1191,9 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
},
None => "closure",
};

let mut err =
self.cannot_capture_in_long_lived_closure(args_span, kind, captured_var, var_span);
err.span_suggestion(
args_span,
&format!(
Expand All @@ -1225,8 +1206,9 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
);

let msg = match category {
ConstraintCategory::Return => "closure is returned here".to_string(),
ConstraintCategory::OpaqueType => "generator is returned here".to_string(),
ConstraintCategory::Return | ConstraintCategory::OpaqueType => {
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 the existing behavior is wrong, users of async should not see generator in error messages.
But not sure what wording should be used instead.

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 that we'll 1) need to be able to differentiate between closures and generators here in the first place and 2) in the OpaqueType it should be "async closure is returned here", right?

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed that saying generators in an error message is basically never right, regardless.

format!("{} is returned here", kind)
}
ConstraintCategory::CallArgument => {
fr_name.highlight_region_name(&mut err);
format!("function requires argument type to outlive `{}`", fr_name)
Expand Down
1 change: 1 addition & 0 deletions src/librustc_mir/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ Rust MIR: a lowered representation of Rust.
#![feature(stmt_expr_attributes)]
#![feature(trait_alias)]
#![feature(option_expect_none)]
#![feature(or_patterns)]
#![recursion_limit = "256"]

#[macro_use]
Expand Down
4 changes: 3 additions & 1 deletion src/librustc_mir/util/borrowck_errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -431,16 +431,18 @@ impl<'cx, 'tcx> crate::borrow_check::MirBorrowckCtxt<'cx, 'tcx> {
crate fn cannot_capture_in_long_lived_closure(
&self,
closure_span: Span,
closure_kind: &str,
borrowed_path: &str,
capture_span: Span,
) -> DiagnosticBuilder<'cx> {
let mut err = struct_span_err!(
self,
closure_span,
E0373,
"closure may outlive the current function, \
"{} may outlive the current function, \
but it borrows {}, \
which is owned by the current function",
closure_kind,
borrowed_path,
);
err.span_label(capture_span, format!("{} is borrowed here", borrowed_path))
Expand Down
Original file line number Diff line number Diff line change
@@ -1,12 +1,18 @@
// edition:2018
// run-rustfix

fn foo() -> Box<impl std::future::Future<Output = u32>> {
fn test_boxed() -> Box<impl std::future::Future<Output = u32>> {
let x = 0u32;
Box::new(async move { x } )
//~^ ERROR E0373
}

fn test_ref(x: &u32) -> impl std::future::Future<Output = u32> + '_ {
async move { *x }
//~^ ERROR E0373
}

fn main() {
let _foo = foo();
let _ = test_boxed();
let _ = test_ref(&0u32);
}
10 changes: 8 additions & 2 deletions src/test/ui/async-await/async-borrowck-escaping-block-error.rs
Original file line number Diff line number Diff line change
@@ -1,12 +1,18 @@
// edition:2018
// run-rustfix

fn foo() -> Box<impl std::future::Future<Output = u32>> {
fn test_boxed() -> Box<impl std::future::Future<Output = u32>> {
let x = 0u32;
Box::new(async { x } )
//~^ ERROR E0373
}

fn test_ref(x: &u32) -> impl std::future::Future<Output = u32> + '_ {
async { *x }
//~^ ERROR E0373
}

fn main() {
let _foo = foo();
let _ = test_boxed();
let _ = test_ref(&0u32);
}
31 changes: 25 additions & 6 deletions src/test/ui/async-await/async-borrowck-escaping-block-error.stderr
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
error[E0373]: closure may outlive the current function, but it borrows `x`, which is owned by the current function
error[E0373]: async block may outlive the current function, but it borrows `x`, which is owned by the current function
--> $DIR/async-borrowck-escaping-block-error.rs:6:20
|
LL | Box::new(async { x } )
Expand All @@ -7,16 +7,35 @@ LL | Box::new(async { x } )
| | `x` is borrowed here
| may outlive borrowed value `x`
|
note: generator is returned here
--> $DIR/async-borrowck-escaping-block-error.rs:4:13
note: async block is returned here
--> $DIR/async-borrowck-escaping-block-error.rs:4:20
|
LL | fn foo() -> Box<impl std::future::Future<Output = u32>> {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
LL | fn test_boxed() -> Box<impl std::future::Future<Output = u32>> {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
help: to force the async block to take ownership of `x` (and any other referenced variables), use the `move` keyword
|
LL | Box::new(async move { x } )
| ^^^^^^^^^^

error: aborting due to previous error
error[E0373]: async block may outlive the current function, but it borrows `x`, which is owned by the current function
--> $DIR/async-borrowck-escaping-block-error.rs:11:11
|
LL | async { *x }
| ^^^-^^
| | |
| | `x` is borrowed here
| may outlive borrowed value `x`
|
note: async block is returned here
--> $DIR/async-borrowck-escaping-block-error.rs:11:5
|
LL | async { *x }
| ^^^^^^^^^^^^
help: to force the async block to take ownership of `x` (and any other referenced variables), use the `move` keyword
|
LL | async move { *x }
| ^^^^^^^^^^^

error: aborting due to 2 previous errors

For more information about this error, try `rustc --explain E0373`.
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
// edition:2018
#![feature(async_closure,async_await)]
#![feature(async_closure)]
fn foo() -> Box<dyn std::future::Future<Output = u32>> {
let x = 0u32;
Box::new((async || x)())
Expand Down
2 changes: 1 addition & 1 deletion src/test/ui/impl-trait/does-not-live-long-enough.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ struct List {
impl List {
fn started_with<'a>(&'a self, prefix: &'a str) -> impl Iterator<Item=&'a str> {
self.data.iter().filter(|s| s.starts_with(prefix)).map(|s| s.as_ref())
//~^ ERROR does not live long enough
//~^ ERROR E0373
}
}

Expand Down
26 changes: 13 additions & 13 deletions src/test/ui/impl-trait/does-not-live-long-enough.stderr
Original file line number Diff line number Diff line change
@@ -1,21 +1,21 @@
error[E0597]: `prefix` does not live long enough
--> $DIR/does-not-live-long-enough.rs:6:51
error[E0373]: closure may outlive the current function, but it borrows `prefix`, which is owned by the current function
--> $DIR/does-not-live-long-enough.rs:6:33
|
LL | fn started_with<'a>(&'a self, prefix: &'a str) -> impl Iterator<Item=&'a str> {
| -- lifetime `'a` defined here --------------------------- opaque type requires that `prefix` is borrowed for `'a`
LL | self.data.iter().filter(|s| s.starts_with(prefix)).map(|s| s.as_ref())
| --- ^^^^^^ borrowed value does not live long enough
| ^^^ ------ `prefix` is borrowed here
| |
| value captured here
LL |
LL | }
| - `prefix` dropped here while still borrowed
| may outlive borrowed value `prefix`
|
note: closure is returned here
--> $DIR/does-not-live-long-enough.rs:5:55
|
help: you can add a bound to the opaque type to make it last less than `'static` and match `'a`
LL | fn started_with<'a>(&'a self, prefix: &'a str) -> impl Iterator<Item=&'a str> {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^
help: to force the closure to take ownership of `prefix` (and any other referenced variables), use the `move` keyword
|
LL | fn started_with<'a>(&'a self, prefix: &'a str) -> impl Iterator<Item=&'a str> + 'a {
| ^^^^
LL | self.data.iter().filter(move |s| s.starts_with(prefix)).map(|s| s.as_ref())
| ^^^^^^^^

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 E0373`.