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

Print nicer async/await trait errors for generators in any place in the error 'stack' #67116

Closed
Closed
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
171 changes: 130 additions & 41 deletions src/librustc/traits/error_reporting.rs
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ use crate::ty::fast_reject;
use crate::ty::fold::TypeFolder;
use crate::ty::subst::Subst;
use crate::ty::SubtypePredicate;
use crate::ty::TraitPredicate;
use crate::util::nodemap::{FxHashMap, FxHashSet};

use errors::{Applicability, DiagnosticBuilder, pluralize, Style};
Expand Down Expand Up @@ -2103,12 +2104,28 @@ impl<'a, 'tcx> InferCtxt<'a, 'tcx> {
err: &mut DiagnosticBuilder<'_>,
obligation: &PredicateObligation<'tcx>,
) {

debug!("note_obligation_cause: obligation.predicate={:?} \
obligation.cause.span={:?}", obligation.predicate, obligation.cause.span);

// First, attempt to add note to this error with an async-await-specific
// message, and fall back to regular note otherwise.
if !self.maybe_note_obligation_cause_for_async_await(err, obligation) {
self.note_obligation_cause_code(err, &obligation.predicate, &obligation.cause.code,
&mut vec![]);
if let ty::Predicate::Trait(trait_predicate) = obligation.predicate {
if self.maybe_note_obligation_cause_for_async_await(
err,
trait_predicate.skip_binder(),
&obligation.cause.code
) {
return;
}
}

self.note_obligation_cause_code(
err,
&obligation.predicate,
&obligation.cause.code,
&mut vec![]
);
}

/// Adds an async-await specific note to the diagnostic when the future does not implement
Expand Down Expand Up @@ -2156,12 +2173,17 @@ impl<'a, 'tcx> InferCtxt<'a, 'tcx> {
fn maybe_note_obligation_cause_for_async_await(
&self,
err: &mut DiagnosticBuilder<'_>,
obligation: &PredicateObligation<'tcx>,
trait_predicate: &TraitPredicate<'tcx>,
code: &ObligationCauseCode<'tcx>,
) -> bool {
debug!("maybe_note_obligation_cause_for_async_await: obligation.predicate={:?} \
obligation.cause.span={:?}", obligation.predicate, obligation.cause.span);
debug!("note_obligation_cause_for_async_await: trait_predicate={:?} \
code={:?}", trait_predicate, code);
let source_map = self.tcx.sess.source_map();

let (trait_ref, target_ty) = (trait_predicate.trait_ref, trait_predicate.self_ty());

debug!("note_obligation_cause_for_async_await: target_ty={:?}", target_ty);

// Attempt to detect an async-await error by looking at the obligation causes, looking
// for a generator to be present.
//
Expand All @@ -2187,51 +2209,99 @@ impl<'a, 'tcx> InferCtxt<'a, 'tcx> {
// The first obligation in the chain is the most useful and has the generator that captured
// the type. The last generator has information about where the bound was introduced. At
// least one generator should be present for this diagnostic to be modified.
let (mut trait_ref, mut target_ty) = match obligation.predicate {
ty::Predicate::Trait(p) =>
(Some(p.skip_binder().trait_ref), Some(p.skip_binder().self_ty())),
_ => (None, None),
};
let mut generator = None;
let mut last_generator = None;
let mut next_code = Some(&obligation.cause.code);
let mut next_code = Some(code);
let mut next_predicate = None;

// First, find an `ObligationCause` code referencing a generator.
// Skip over intermediate causes generated by `.await` lowering
while let Some(code) = next_code {
debug!("maybe_note_obligation_cause_for_async_await: code={:?}", code);
debug!("note_obligation_cause_for_async_await: code={:?}", code);
match code {
ObligationCauseCode::BuiltinDerivedObligation(derived_obligation) |
ObligationCauseCode::ImplDerivedObligation(derived_obligation) => {
let ty = derived_obligation.parent_trait_ref.self_ty();
debug!("maybe_note_obligation_cause_for_async_await: \
parent_trait_ref={:?} self_ty.kind={:?}",
derived_obligation.parent_trait_ref, ty.kind);
debug!("note_obligation_cause_for_async_await: self_ty.kind={:?}",
derived_obligation.parent_trait_ref.self_ty().kind);

next_code = Some(derived_obligation.parent_code.as_ref());
next_predicate = Some(
self.resolve_vars_if_possible(&derived_obligation.parent_trait_ref)
.to_predicate()
);

match ty.kind {
match derived_obligation.parent_trait_ref.self_ty().kind {
ty::Adt(ty::AdtDef { did, .. }, ..) if
self.tcx.is_diagnostic_item(sym::gen_future, *did) => {},
ty::Generator(did, ..) => {
generator = generator.or(Some(did));
last_generator = Some(did);
generator = Some(did);
debug!("note_obligation_cause_for_async_await: found generator {:?}",
generator);
break;
},
ty::GeneratorWitness(..) => {},
_ if generator.is_none() => {
trait_ref = Some(*derived_obligation.parent_trait_ref.skip_binder());
target_ty = Some(ty);
},
_ => {},
ty::GeneratorWitness(_) | ty::Opaque(..) => {},
_ => return false,
}

},
_ => return false,
}
};

// Now that we've found a generator, try to determine if we should
// skip over any subsequence causes.
while let Some(code) = next_code {
debug!("note_obligation_cause_for_async_await: inspecting code={:?}", code);
match code {
ObligationCauseCode::BuiltinDerivedObligation(derived_obligation) |
ObligationCauseCode::ImplDerivedObligation(derived_obligation) => {
debug!("note_obligation_cause_for_async_await: inspecting self_ty.kind={:?}",
derived_obligation.parent_trait_ref.self_ty().kind);


match derived_obligation.parent_trait_ref.self_ty().kind {
ty::Adt(ty::AdtDef { did, .. }, ..) if
self.tcx.is_diagnostic_item(sym::gen_future, *did) => {},
ty::Opaque(did, _) if
self.tcx.parent(did).map(|parent| {
self.tcx.is_diagnostic_item(sym::from_generator, parent)
}).unwrap_or(false) => {}
_ => break,
}

next_code = Some(derived_obligation.parent_code.as_ref());
next_predicate = Some(
self.resolve_vars_if_possible(&derived_obligation.parent_trait_ref)
.to_predicate()
)
},
_ => break,
_ => break
}
}

let generator_did = generator.expect("can only reach this if there was a generator");

// Finally, see if there are any generator types remaining in the 'error stack'.
// If there aren't, then treat the current generator as the 'last generator',
// which will make `note_obligation_cause_for_async_await` emit an extra
// message for it
let mut target_code = next_code;
let mut last_generator = Some(generator_did);
while let Some(code) = target_code {
debug!("note_obligation_cause_for_async_await: last_generator code={:?}", code);
if let ObligationCauseCode::BuiltinDerivedObligation(obligation) |
ObligationCauseCode::ImplDerivedObligation(obligation) = code {
if let ty::Generator(..) = obligation.parent_trait_ref.self_ty().kind {
// We found another generator, so our current generator can't be
// the last one
last_generator = None;
break;
}
target_code = Some(obligation.parent_code.as_ref());
} else {
break;
}
}

// Only continue if a generator was found.
debug!("maybe_note_obligation_cause_for_async_await: generator={:?} trait_ref={:?} \
target_ty={:?}", generator, trait_ref, target_ty);
let (generator_did, trait_ref, target_ty) = match (generator, trait_ref, target_ty) {
(Some(generator_did), Some(trait_ref), Some(target_ty)) =>
(generator_did, trait_ref, target_ty),
_ => return false,
};

let span = self.tcx.def_span(generator_did);

Expand Down Expand Up @@ -2290,7 +2360,7 @@ impl<'a, 'tcx> InferCtxt<'a, 'tcx> {
if let Some((target_span, Ok(snippet), scope_span)) = target_span {
self.note_obligation_cause_for_async_await(
err, *target_span, scope_span, snippet, generator_did, last_generator,
trait_ref, target_ty, tables, obligation, next_code,
trait_ref, target_ty, tables, &next_predicate.unwrap(), next_code,
);
true
} else {
Expand All @@ -2311,7 +2381,7 @@ impl<'a, 'tcx> InferCtxt<'a, 'tcx> {
trait_ref: ty::TraitRef<'_>,
target_ty: Ty<'tcx>,
tables: &ty::TypeckTables<'_>,
obligation: &PredicateObligation<'tcx>,
predicate: &ty::Predicate<'tcx>,
next_code: Option<&ObligationCauseCode<'tcx>>,
) {
let source_map = self.tcx.sess.source_map();
Expand Down Expand Up @@ -2342,9 +2412,8 @@ impl<'a, 'tcx> InferCtxt<'a, 'tcx> {
("`Sync`", "shared")
};

err.clear_code();
Copy link
Member

Choose a reason for hiding this comment

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

I don't have any strong feelings about undoing this change from #65345. My intent in clearing the error code was avoiding having E0277 have multiple different primary messages:

#65345 introduced two special-cased error messages:

  • "future cannot be sent between threads safely" (special case for Send)
  • "future cannot be shared between threads safely" (special case for Sync)

E0277 otherwise has the following message:

the trait bound Foo: Qux is not satisfied in impl std::future::Future

If we don't mind having E0277 be the error code for all of these cases, then we should remove this line.

cc @rust-lang/wg-diagnostics

Copy link
Contributor

Choose a reason for hiding this comment

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

I do find E0277 to be a too generic error that happens all over the place, and we have precedence of changing error codes for more specific cases (type errors are not just E0308). We should add new error codes for these cases so that we can add explanations in the index.

err.set_primary_message(
format!("future cannot be {} between threads safely", trait_verb)
&format!("future cannot be {} between threads safely", trait_verb)
);

let original_span = err.span.primary_span().unwrap();
Expand Down Expand Up @@ -2396,7 +2465,7 @@ impl<'a, 'tcx> InferCtxt<'a, 'tcx> {
debug!("note_obligation_cause_for_async_await: next_code={:?}", next_code);
self.note_obligation_cause_code(
err,
&obligation.predicate,
&predicate,
next_code.unwrap(),
&mut Vec::new(),
);
Expand All @@ -2410,6 +2479,16 @@ impl<'a, 'tcx> InferCtxt<'a, 'tcx> {
where T: fmt::Display
{
let tcx = self.tcx;


let mut handle_async_await = |trait_ref, parent_code| {
self.maybe_note_obligation_cause_for_async_await(
err,
&TraitPredicate { trait_ref },
parent_code
)
};

match *cause_code {
ObligationCauseCode::ExprAssignable |
ObligationCauseCode::MatchExpressionArm { .. } |
Expand Down Expand Up @@ -2550,6 +2629,11 @@ impl<'a, 'tcx> InferCtxt<'a, 'tcx> {
}
ObligationCauseCode::BuiltinDerivedObligation(ref data) => {
let parent_trait_ref = self.resolve_vars_if_possible(&data.parent_trait_ref);

if handle_async_await(*parent_trait_ref.skip_binder(), &data.parent_code) {
return;
}

let ty = parent_trait_ref.skip_binder().self_ty();
err.note(&format!("required because it appears within the type `{}`", ty));
obligated_types.push(ty);
Expand All @@ -2564,6 +2648,11 @@ impl<'a, 'tcx> InferCtxt<'a, 'tcx> {
}
ObligationCauseCode::ImplDerivedObligation(ref data) => {
let parent_trait_ref = self.resolve_vars_if_possible(&data.parent_trait_ref);

if handle_async_await(*parent_trait_ref.skip_binder(), &data.parent_code) {
return;
}
Copy link
Member

Choose a reason for hiding this comment

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

Could you elaborate on which cases are supported by adding handle_async_await here? I can't work out from reading the diff which cases this would catch but the existing call in note_obligation_cause wouldn't?


err.note(
&format!("required because of the requirements on the impl of `{}` for `{}`",
parent_trait_ref.print_only_trait_path(),
Expand Down
2 changes: 2 additions & 0 deletions src/libstd/future.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ pub use core::future::*;
/// better error messages (`impl Future` rather than `GenFuture<[closure.....]>`).
#[doc(hidden)]
#[unstable(feature = "gen_future", issue = "50547")]
#[cfg_attr(not(test), rustc_diagnostic_item = "from_generator")]
pub fn from_generator<T: Generator<Yield = ()>>(x: T) -> impl Future<Output = T::Return> {
GenFuture(x)
}
Expand All @@ -26,6 +27,7 @@ pub fn from_generator<T: Generator<Yield = ()>>(x: T) -> impl Future<Output = T:
#[doc(hidden)]
#[unstable(feature = "gen_future", issue = "50547")]
#[derive(Copy, Clone, Debug, Eq, PartialEq, Ord, PartialOrd, Hash)]
#[cfg_attr(not(test), rustc_diagnostic_item = "gen_future")]
struct GenFuture<T: Generator<Yield = ()>>(T);

// We rely on the fact that async/await futures are immovable in order to create
Expand Down
21 changes: 17 additions & 4 deletions src/test/ui/async-await/async-fn-nonsend.stderr
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
error: future cannot be sent between threads safely
error[E0277]: future cannot be sent between threads safely
--> $DIR/async-fn-nonsend.rs:50:5
|
LL | fn assert_send(_: impl Send) {}
Expand All @@ -18,8 +18,9 @@ LL | fut().await;
| ^^^^^^^^^^^ await occurs here, with `x` maybe used later
LL | }
| - `x` is later dropped here
= note: required because it appears within the type `impl std::future::Future`

error: future cannot be sent between threads safely
error[E0277]: future cannot be sent between threads safely
--> $DIR/async-fn-nonsend.rs:52:5
|
LL | fn assert_send(_: impl Send) {}
Expand All @@ -39,8 +40,9 @@ LL | Some(_) => fut().await,
...
LL | }
| - `non_send()` is later dropped here
= note: required because it appears within the type `impl std::future::Future`
Copy link
Contributor

Choose a reason for hiding this comment

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

the notes that just cite impl Future don't really seem to add much value


error: future cannot be sent between threads safely
error[E0277]: future cannot be sent between threads safely
--> $DIR/async-fn-nonsend.rs:54:5
|
LL | fn assert_send(_: impl Send) {}
Expand All @@ -50,6 +52,8 @@ LL | assert_send(non_sync_with_method_call());
| ^^^^^^^^^^^ future returned by `non_sync_with_method_call` is not `Send`
|
= help: the trait `std::marker::Send` is not implemented for `dyn std::fmt::Write`
= note: required because of the requirements on the impl of `std::marker::Send` for `&mut dyn std::fmt::Write`
= note: required because it appears within the type `std::fmt::Formatter<'_>`
note: future is not `Send` as this value is used across an await
--> $DIR/async-fn-nonsend.rs:43:9
|
Expand All @@ -61,8 +65,9 @@ LL | fut().await;
LL | }
LL | }
| - `f` is later dropped here
= note: required because it appears within the type `impl std::future::Future`

error: future cannot be sent between threads safely
error[E0277]: future cannot be sent between threads safely
--> $DIR/async-fn-nonsend.rs:54:5
|
LL | fn assert_send(_: impl Send) {}
Expand All @@ -72,6 +77,12 @@ LL | assert_send(non_sync_with_method_call());
| ^^^^^^^^^^^ future returned by `non_sync_with_method_call` is not `Send`
|
= help: within `std::fmt::ArgumentV1<'_>`, the trait `std::marker::Sync` is not implemented for `*mut (dyn std::ops::Fn() + 'static)`
= note: required because it appears within the type `std::marker::PhantomData<*mut (dyn std::ops::Fn() + 'static)>`
Copy link
Contributor

Choose a reason for hiding this comment

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

I have to say that I'm not sure how much value this provides either. Based on the error message, at least, I admit I can't really make heads or tails of these types. At best, it seems sort of like they're appearing in the wrong place, as they seem to be explaining something about the variable f below (at least that's the only mention of formatting that I see).

I guess I feel like in practice if I saw this error I'd just skip over all those notes and I'd probably tell users to do the same.

That said, I can imagine that in some very confusing cases notes like this could be helpful -- but I'd sort of like an example of such a case.

= note: required because it appears within the type `core::fmt::Void`
= note: required because it appears within the type `&core::fmt::Void`
= note: required because it appears within the type `std::fmt::ArgumentV1<'_>`
= note: required because of the requirements on the impl of `std::marker::Send` for `std::slice::Iter<'_, std::fmt::ArgumentV1<'_>>`
= note: required because it appears within the type `std::fmt::Formatter<'_>`
note: future is not `Send` as this value is used across an await
--> $DIR/async-fn-nonsend.rs:43:9
|
Expand All @@ -83,6 +94,8 @@ LL | fut().await;
LL | }
LL | }
| - `f` is later dropped here
= note: required because it appears within the type `impl std::future::Future`

error: aborting due to 4 previous errors

For more information about this error, try `rustc --explain E0277`.
4 changes: 3 additions & 1 deletion src/test/ui/async-await/issue-64130-1-sync.stderr
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
error: future cannot be shared between threads safely
error[E0277]: future cannot be shared between threads safely
--> $DIR/issue-64130-1-sync.rs:21:5
|
LL | fn is_sync<T: Sync>(t: T) { }
Expand All @@ -17,6 +17,8 @@ LL | baz().await;
| ^^^^^^^^^^^ await occurs here, with `x` maybe used later
LL | }
| - `x` is later dropped here
= note: required because it appears within the type `impl std::future::Future`

error: aborting due to previous error

For more information about this error, try `rustc --explain E0277`.
4 changes: 3 additions & 1 deletion src/test/ui/async-await/issue-64130-2-send.stderr
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
error: future cannot be sent between threads safely
error[E0277]: future cannot be sent between threads safely
--> $DIR/issue-64130-2-send.rs:21:5
|
LL | fn is_send<T: Send>(t: T) { }
Expand All @@ -17,6 +17,8 @@ LL | baz().await;
| ^^^^^^^^^^^ await occurs here, with `x` maybe used later
LL | }
| - `x` is later dropped here
= note: required because it appears within the type `impl std::future::Future`

error: aborting due to previous error

For more information about this error, try `rustc --explain E0277`.
1 change: 1 addition & 0 deletions src/test/ui/async-await/issue-64130-3-other.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ LL | baz().await;
| ^^^^^^^^^^^ await occurs here, with `x` maybe used later
LL | }
| - `x` is later dropped here
= note: required because it appears within the type `impl std::future::Future`

error: aborting due to previous error

Expand Down
Loading