Skip to content

Commit

Permalink
async/await: improve obligation errors
Browse files Browse the repository at this point in the history
This commit improves obligation errors for async/await:

```
note: future does not implement `std::marker::Send` because this value is used across an
      await
  --> $DIR/issue-64130-non-send-future-diags.rs:15:5
   |
LL |     let g = x.lock().unwrap();
   |         - has type `std::sync::MutexGuard<'_, u32>`
LL |     baz().await;
   |     ^^^^^^^^^^^ await occurs here, with `g` maybe used later
LL | }
   | - `g` is later dropped here
```

Signed-off-by: David Wood <david@davidtw.co>
  • Loading branch information
davidtwco committed Sep 30, 2019
1 parent d046ffd commit 04fa9b1
Show file tree
Hide file tree
Showing 9 changed files with 281 additions and 28 deletions.
177 changes: 165 additions & 12 deletions src/librustc/traits/error_reporting.rs
Expand Up @@ -24,7 +24,7 @@ use crate::hir::def_id::DefId;
use crate::infer::{self, InferCtxt};
use crate::infer::type_variable::{TypeVariableOrigin, TypeVariableOriginKind};
use crate::session::DiagnosticMessageId;
use crate::ty::{self, AdtKind, ToPredicate, ToPolyTraitRef, Ty, TyCtxt, TypeFoldable};
use crate::ty::{self, AdtKind, DefIdTree, ToPredicate, ToPolyTraitRef, Ty, TyCtxt, TypeFoldable};
use crate::ty::GenericParamDefKind;
use crate::ty::error::ExpectedFound;
use crate::ty::fast_reject;
Expand All @@ -37,7 +37,7 @@ use errors::{Applicability, DiagnosticBuilder, pluralise};
use std::fmt;
use syntax::ast;
use syntax::symbol::{sym, kw};
use syntax_pos::{DUMMY_SP, Span, ExpnKind};
use syntax_pos::{DUMMY_SP, Span, ExpnKind, MultiSpan};

impl<'a, 'tcx> InferCtxt<'a, 'tcx> {
pub fn report_fulfillment_errors(
Expand Down Expand Up @@ -550,7 +550,8 @@ impl<'a, 'tcx> InferCtxt<'a, 'tcx> {
self.suggest_new_overflow_limit(&mut err);
}

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

err.emit();
self.tcx.sess.abort_if_errors();
Expand Down Expand Up @@ -940,7 +941,9 @@ impl<'a, 'tcx> InferCtxt<'a, 'tcx> {
bug!("overflow should be handled before the `report_selection_error` path");
}
};

self.note_obligation_cause(&mut err, obligation);

err.emit();
}

Expand Down Expand Up @@ -1604,15 +1607,165 @@ impl<'a, 'tcx> InferCtxt<'a, 'tcx> {
})
}

fn note_obligation_cause<T>(&self,
err: &mut DiagnosticBuilder<'_>,
obligation: &Obligation<'tcx, T>)
where T: fmt::Display
{
self.note_obligation_cause_code(err,
&obligation.predicate,
&obligation.cause.code,
&mut vec![]);
fn note_obligation_cause(
&self,
err: &mut DiagnosticBuilder<'_>,
obligation: &PredicateObligation<'tcx>,
) {
// First, attempt to add note to this error with an async-await-specific
// message, and fall back to regular note otherwise.
if !self.note_obligation_cause_for_async_await(err, obligation) {
self.note_obligation_cause_code(err, &obligation.predicate, &obligation.cause.code,
&mut vec![]);
}
}

/// Adds an async-await specific note to the diagnostic:
///
/// ```ignore (diagnostic)
/// note: future does not implement `std::marker::Send` because this value is used across an
/// await
/// --> $DIR/issue-64130-non-send-future-diags.rs:15:5
/// |
/// LL | let g = x.lock().unwrap();
/// | - has type `std::sync::MutexGuard<'_, u32>`
/// LL | baz().await;
/// | ^^^^^^^^^^^ await occurs here, with `g` maybe used later
/// LL | }
/// | - `g` is later dropped here
/// ```
///
/// Returns `true` if an async-await specific note was added to the diagnostic.
fn note_obligation_cause_for_async_await(
&self,
err: &mut DiagnosticBuilder<'_>,
obligation: &PredicateObligation<'tcx>,
) -> bool {
debug!("note_obligation_cause_for_async_await: obligation.predicate={:?} \
obligation.cause.span={:?}", obligation.predicate, obligation.cause.span);
let source_map = self.tcx.sess.source_map();

// Look into the obligation predicate to determine the type in the generator which meant
// that the predicate was not satisifed.
let (trait_ref, target_ty) = match obligation.predicate {
ty::Predicate::Trait(trait_predicate) =>
(trait_predicate.skip_binder().trait_ref, trait_predicate.skip_binder().self_ty()),
_ => return false,
};
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 only generators, generator witnesses, opaque types or `std::future::GenFuture` to
// be present.
//
// When a future does not implement a trait because of a captured type in one of the
// generators somewhere in the call stack, then the result is a chain of obligations.
// Given a `async fn` A that calls a `async fn` B which captures a non-send type and that
// future is passed as an argument to a function C which requires a `Send` type, then the
// chain looks something like this:
//
// - `BuiltinDerivedObligation` with a generator witness (B)
// - `BuiltinDerivedObligation` with a generator (B)
// - `BuiltinDerivedObligation` with `std::future::GenFuture` (B)
// - `BuiltinDerivedObligation` with `impl std::future::Future` (B)
// - `BuiltinDerivedObligation` with `impl std::future::Future` (B)
// - `BuiltinDerivedObligation` with a generator witness (A)
// - `BuiltinDerivedObligation` with a generator (A)
// - `BuiltinDerivedObligation` with `std::future::GenFuture` (A)
// - `BuiltinDerivedObligation` with `impl std::future::Future` (A)
// - `BuiltinDerivedObligation` with `impl std::future::Future` (A)
// - `BindingObligation` with `impl_send (Send requirement)
//
// The first obligations in the chain can be used to get the details of the type that is
// captured but the entire chain must be inspected to detect this case.
let mut generator = None;
let mut next_code = Some(&obligation.cause.code);
while let Some(code) = next_code {
debug!("note_obligation_cause_for_async_await: code={:?}", code);
match code {
ObligationCauseCode::BuiltinDerivedObligation(derived_obligation) |
ObligationCauseCode::ImplDerivedObligation(derived_obligation) => {
debug!("note_obligation_cause_for_async_await: 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::Generator(did, ..) => generator = generator.or(Some(did)),
ty::GeneratorWitness(_) | ty::Opaque(..) => {},
_ => return false,
}

next_code = Some(derived_obligation.parent_code.as_ref());
},
ObligationCauseCode::ItemObligation(_) | ObligationCauseCode::BindingObligation(..)
if generator.is_some() => break,
_ => return false,
}
}

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

// Only continue to add a note if the generator is from an `async` function.
let parent_node = self.tcx.parent(generator_did)
.and_then(|parent_did| self.tcx.hir().get_if_local(parent_did));
debug!("note_obligation_cause_for_async_await: parent_node={:?}", parent_node);
if let Some(hir::Node::Item(hir::Item {
kind: hir::ItemKind::Fn(_, header, _, _),
..
})) = parent_node {
debug!("note_obligation_cause_for_async_await: header={:?}", header);
if header.asyncness != hir::IsAsync::Async {
return false;
}
}

let span = self.tcx.def_span(generator_did);
let tables = self.tcx.typeck_tables_of(generator_did);
debug!("note_obligation_cause_for_async_await: generator_did={:?} span={:?} ",
generator_did, span);

// Look for a type inside the generator interior that matches the target type to get
// a span.
let target_span = tables.generator_interior_types.iter()
.find(|ty::GeneratorInteriorTypeCause { ty, .. }| ty::TyS::same_type(*ty, target_ty))
.map(|ty::GeneratorInteriorTypeCause { span, scope_span, .. }|
(span, source_map.span_to_snippet(*span), scope_span));
if let Some((target_span, Ok(snippet), scope_span)) = target_span {
// Look at the last interior type to get a span for the `.await`.
let await_span = tables.generator_interior_types.iter().map(|i| i.span).last().unwrap();
let mut span = MultiSpan::from_span(await_span);
span.push_span_label(
await_span, format!("await occurs here, with `{}` maybe used later", snippet));

span.push_span_label(*target_span, format!("has type `{}`", target_ty));

// If available, use the scope span to annotate the drop location.
if let Some(scope_span) = scope_span {
span.push_span_label(
source_map.end_point(*scope_span),
format!("`{}` is later dropped here", snippet),
);
}

err.span_note(span, &format!(
"future does not implement `{}` as this value is used across an await",
trait_ref,
));

// Add a note for the item obligation that remains - normally a note pointing to the
// bound that introduced the obligation (e.g. `T: Send`).
debug!("note_obligation_cause_for_async_await: next_code={:?}", next_code);
self.note_obligation_cause_code(
err,
&obligation.predicate,
next_code.unwrap(),
&mut Vec::new(),
);

true
} else {
false
}
}

fn note_obligation_cause_code<T>(&self,
Expand Down
35 changes: 35 additions & 0 deletions src/librustc/ty/context.rs
Expand Up @@ -288,6 +288,34 @@ pub struct ResolvedOpaqueTy<'tcx> {
pub substs: SubstsRef<'tcx>,
}

/// Whenever a value may be live across a generator yield, the type of that value winds up in the
/// `GeneratorInteriorTypeCause` struct. This struct adds additional information about such
/// captured types that can be useful for diagnostics. In particular, it stores the span that
/// caused a given type to be recorded, along with the scope that enclosed the value (which can
/// be used to find the await that the value is live across).
///
/// For example:
///
/// ```ignore (pseudo-Rust)
/// async move {
/// let x: T = ...;
/// foo.await
/// ...
/// }
/// ```
///
/// Here, we would store the type `T`, the span of the value `x`, and the "scope-span" for
/// the scope that contains `x`.
#[derive(RustcEncodable, RustcDecodable, Clone, Debug, Eq, Hash, HashStable, PartialEq)]
pub struct GeneratorInteriorTypeCause<'tcx> {
/// Type of the captured binding.
pub ty: Ty<'tcx>,
/// Span of the binding that was captured.
pub span: Span,
/// Span of the scope of the captured binding.
pub scope_span: Option<Span>,
}

#[derive(RustcEncodable, RustcDecodable, Debug)]
pub struct TypeckTables<'tcx> {
/// The HirId::owner all ItemLocalIds in this table are relative to.
Expand Down Expand Up @@ -397,6 +425,10 @@ pub struct TypeckTables<'tcx> {
/// leading to the member of the struct or tuple that is used instead of the
/// entire variable.
pub upvar_list: ty::UpvarListMap,

/// Stores the type, span and optional scope span of all types
/// that are live across the yield of this generator (if a generator).
pub generator_interior_types: Vec<GeneratorInteriorTypeCause<'tcx>>,
}

impl<'tcx> TypeckTables<'tcx> {
Expand All @@ -422,6 +454,7 @@ impl<'tcx> TypeckTables<'tcx> {
free_region_map: Default::default(),
concrete_opaque_types: Default::default(),
upvar_list: Default::default(),
generator_interior_types: Default::default(),
}
}

Expand Down Expand Up @@ -729,6 +762,7 @@ impl<'a, 'tcx> HashStable<StableHashingContext<'a>> for TypeckTables<'tcx> {
ref free_region_map,
ref concrete_opaque_types,
ref upvar_list,
ref generator_interior_types,

} = *self;

Expand Down Expand Up @@ -773,6 +807,7 @@ impl<'a, 'tcx> HashStable<StableHashingContext<'a>> for TypeckTables<'tcx> {
free_region_map.hash_stable(hcx, hasher);
concrete_opaque_types.hash_stable(hcx, hasher);
upvar_list.hash_stable(hcx, hasher);
generator_interior_types.hash_stable(hcx, hasher);
})
}
}
Expand Down
2 changes: 1 addition & 1 deletion src/librustc/ty/mod.rs
Expand Up @@ -75,7 +75,7 @@ pub use self::binding::BindingMode;
pub use self::binding::BindingMode::*;

pub use self::context::{TyCtxt, FreeRegionInfo, AllArenas, tls, keep_local};
pub use self::context::{Lift, TypeckTables, CtxtInterners, GlobalCtxt};
pub use self::context::{Lift, GeneratorInteriorTypeCause, TypeckTables, CtxtInterners, GlobalCtxt};
pub use self::context::{
UserTypeAnnotationIndex, UserType, CanonicalUserType,
CanonicalUserTypeAnnotation, CanonicalUserTypeAnnotations, ResolvedOpaqueTy,
Expand Down
15 changes: 12 additions & 3 deletions src/librustc_typeck/check/generator_interior.rs
Expand Up @@ -14,7 +14,7 @@ use crate::util::nodemap::FxHashMap;

struct InteriorVisitor<'a, 'tcx> {
fcx: &'a FnCtxt<'a, 'tcx>,
types: FxHashMap<Ty<'tcx>, usize>,
types: FxHashMap<ty::GeneratorInteriorTypeCause<'tcx>, usize>,
region_scope_tree: &'tcx region::ScopeTree,
expr_count: usize,
kind: hir::GeneratorKind,
Expand Down Expand Up @@ -83,7 +83,12 @@ impl<'a, 'tcx> InteriorVisitor<'a, 'tcx> {
} else {
// Map the type to the number of types added before it
let entries = self.types.len();
self.types.entry(&ty).or_insert(entries);
let scope_span = scope.map(|s| s.span(self.fcx.tcx, self.region_scope_tree));
self.types.entry(ty::GeneratorInteriorTypeCause {
span: source_span,
ty: &ty,
scope_span
}).or_insert(entries);
}
} else {
debug!("no type in expr = {:?}, count = {:?}, span = {:?}",
Expand Down Expand Up @@ -118,8 +123,12 @@ pub fn resolve_interior<'a, 'tcx>(
// Sort types by insertion order
types.sort_by_key(|t| t.1);

// Store the generator types and spans into the tables for this generator.
let interior_types = types.iter().cloned().map(|t| t.0).collect::<Vec<_>>();
visitor.fcx.inh.tables.borrow_mut().generator_interior_types = interior_types;

// Extract type components
let type_list = fcx.tcx.mk_type_list(types.into_iter().map(|t| t.0));
let type_list = fcx.tcx.mk_type_list(types.into_iter().map(|t| (t.0).ty));

// The types in the generator interior contain lifetimes local to the generator itself,
// which should not be exposed outside of the generator. Therefore, we replace these
Expand Down
7 changes: 7 additions & 0 deletions src/librustc_typeck/check/writeback.rs
Expand Up @@ -58,6 +58,7 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
wbcx.visit_free_region_map();
wbcx.visit_user_provided_tys();
wbcx.visit_user_provided_sigs();
wbcx.visit_generator_interior_types();

let used_trait_imports = mem::replace(
&mut self.tables.borrow_mut().used_trait_imports,
Expand Down Expand Up @@ -430,6 +431,12 @@ impl<'cx, 'tcx> WritebackCx<'cx, 'tcx> {
}
}

fn visit_generator_interior_types(&mut self) {
let fcx_tables = self.fcx.tables.borrow();
debug_assert_eq!(fcx_tables.local_id_root, self.tables.local_id_root);
self.tables.generator_interior_types = fcx_tables.generator_interior_types.clone();
}

fn visit_opaque_types(&mut self, span: Span) {
for (&def_id, opaque_defn) in self.fcx.opaque_types.borrow().iter() {
let hir_id = self.tcx().hir().as_local_hir_id(def_id).unwrap();
Expand Down
1 change: 1 addition & 0 deletions src/libstd/future.rs
Expand Up @@ -26,6 +26,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

0 comments on commit 04fa9b1

Please sign in to comment.