From ebe3c56c6eeca6db4a62ebb99e2ffe35f33ea4de Mon Sep 17 00:00:00 2001 From: oribenshir Date: Sat, 5 Mar 2022 12:04:32 +0200 Subject: [PATCH] Provide a better diagnostic on failure to meet send bound on futures in a foreign crate Adding diagnostic data on generators to the crate metadata and using it to provide a better diagnostic on failure to meet send bound on futures originated from a foreign crate --- compiler/rustc_metadata/src/rmeta/decoder.rs | 19 ++ .../src/rmeta/decoder/cstore_impl.rs | 1 + compiler/rustc_metadata/src/rmeta/encoder.rs | 7 +- compiler/rustc_metadata/src/rmeta/mod.rs | 2 + compiler/rustc_middle/src/query/mod.rs | 6 + compiler/rustc_middle/src/ty/context.rs | 32 +++ compiler/rustc_middle/src/ty/mod.rs | 5 +- compiler/rustc_middle/src/ty/query.rs | 10 +- .../src/traits/error_reporting/suggestions.rs | 200 ++++++++++++++---- src/test/ui/async-await/issues/issue-67893.rs | 2 +- .../ui/async-await/issues/issue-67893.stderr | 16 +- 11 files changed, 247 insertions(+), 53 deletions(-) diff --git a/compiler/rustc_metadata/src/rmeta/decoder.rs b/compiler/rustc_metadata/src/rmeta/decoder.rs index 8d0e8467404ca..fd17d3ea0d5ff 100644 --- a/compiler/rustc_metadata/src/rmeta/decoder.rs +++ b/compiler/rustc_metadata/src/rmeta/decoder.rs @@ -28,6 +28,7 @@ use rustc_middle::mir::interpret::{AllocDecodingSession, AllocDecodingState}; use rustc_middle::thir; use rustc_middle::ty::codec::TyDecoder; use rustc_middle::ty::fast_reject::SimplifiedType; +use rustc_middle::ty::GeneratorDiagnosticData; use rustc_middle::ty::{self, Ty, TyCtxt, Visibility}; use rustc_serialize::{opaque, Decodable, Decoder}; use rustc_session::cstore::{ @@ -1732,6 +1733,24 @@ impl<'a, 'tcx> CrateMetadataRef<'a> { .collect() }) } + + fn get_generator_diagnostic_data( + self, + tcx: TyCtxt<'tcx>, + id: DefIndex, + ) -> Option> { + self.root + .tables + .generator_diagnostic_data + .get(self, id) + .map(|param| param.decode((self, tcx))) + .map(|generator_data| GeneratorDiagnosticData { + generator_interior_types: generator_data.generator_interior_types, + hir_owner: generator_data.hir_owner, + nodes_types: generator_data.nodes_types, + adjustments: generator_data.adjustments, + }) + } } impl CrateMetadata { diff --git a/compiler/rustc_metadata/src/rmeta/decoder/cstore_impl.rs b/compiler/rustc_metadata/src/rmeta/decoder/cstore_impl.rs index 63bf929fb8639..ba6c4a2af77e1 100644 --- a/compiler/rustc_metadata/src/rmeta/decoder/cstore_impl.rs +++ b/compiler/rustc_metadata/src/rmeta/decoder/cstore_impl.rs @@ -246,6 +246,7 @@ provide! { <'tcx> tcx, def_id, other, cdata, crate_extern_paths => { cdata.source().paths().cloned().collect() } expn_that_defined => { cdata.get_expn_that_defined(def_id.index, tcx.sess) } + generator_diagnostic_data => { cdata.get_generator_diagnostic_data(tcx, def_id.index) } } pub(in crate::rmeta) fn provide(providers: &mut Providers) { diff --git a/compiler/rustc_metadata/src/rmeta/encoder.rs b/compiler/rustc_metadata/src/rmeta/encoder.rs index e967750aebb52..74f22e0179c2f 100644 --- a/compiler/rustc_metadata/src/rmeta/encoder.rs +++ b/compiler/rustc_metadata/src/rmeta/encoder.rs @@ -1550,16 +1550,17 @@ impl<'a, 'tcx> EncodeContext<'a, 'tcx> { fn encode_info_for_closure(&mut self, hir_id: hir::HirId) { let def_id = self.tcx.hir().local_def_id(hir_id); debug!("EncodeContext::encode_info_for_closure({:?})", def_id); - // NOTE(eddyb) `tcx.type_of(def_id)` isn't used because it's fully generic, // including on the signature, which is inferred in `typeck. - let ty = self.tcx.typeck(def_id).node_type(hir_id); - + let typeck_result: &'tcx ty::TypeckResults<'tcx> = self.tcx.typeck(def_id); + let ty = typeck_result.node_type(hir_id); match ty.kind() { ty::Generator(..) => { let data = self.tcx.generator_kind(def_id).unwrap(); + let generator_diagnostic_data = typeck_result.get_generator_diagnostic_data(); record!(self.tables.kind[def_id.to_def_id()] <- EntryKind::Generator); record!(self.tables.generator_kind[def_id.to_def_id()] <- data); + record!(self.tables.generator_diagnostic_data[def_id.to_def_id()] <- generator_diagnostic_data); } ty::Closure(..) => { diff --git a/compiler/rustc_metadata/src/rmeta/mod.rs b/compiler/rustc_metadata/src/rmeta/mod.rs index 43ccfc64e0563..e1a1589adb322 100644 --- a/compiler/rustc_metadata/src/rmeta/mod.rs +++ b/compiler/rustc_metadata/src/rmeta/mod.rs @@ -19,6 +19,7 @@ use rustc_middle::mir; use rustc_middle::thir; use rustc_middle::ty::fast_reject::SimplifiedType; use rustc_middle::ty::query::Providers; +use rustc_middle::ty::GeneratorDiagnosticData; use rustc_middle::ty::{self, ReprOptions, Ty}; use rustc_serialize::opaque::Encoder; use rustc_session::config::SymbolManglingVersion; @@ -358,6 +359,7 @@ define_tables! { def_keys: Table>, def_path_hashes: Table, proc_macro_quoted_spans: Table>, + generator_diagnostic_data: Table>>, } #[derive(Copy, Clone, MetadataEncodable, MetadataDecodable)] diff --git a/compiler/rustc_middle/src/query/mod.rs b/compiler/rustc_middle/src/query/mod.rs index f38ade1076eac..3b61d91a10fd3 100644 --- a/compiler/rustc_middle/src/query/mod.rs +++ b/compiler/rustc_middle/src/query/mod.rs @@ -1962,4 +1962,10 @@ rustc_queries! { eval_always desc { "computing the backend features for CLI flags" } } + + query generator_diagnostic_data(key: DefId) -> Option> { + storage(ArenaCacheSelector<'tcx>) + desc { |tcx| "looking up generator diagnostic data of `{}`", tcx.def_path_str(key) } + separate_provide_extern + } } diff --git a/compiler/rustc_middle/src/ty/context.rs b/compiler/rustc_middle/src/ty/context.rs index 4e6be84ad7f8f..30fe3ffa7e3c4 100644 --- a/compiler/rustc_middle/src/ty/context.rs +++ b/compiler/rustc_middle/src/ty/context.rs @@ -367,6 +367,16 @@ pub struct GeneratorInteriorTypeCause<'tcx> { pub expr: Option, } +// This type holds diagnostic information on generators and async functions across crate boundaries +// and is used to provide better error messages +#[derive(TyEncodable, TyDecodable, Clone, Debug, HashStable)] +pub struct GeneratorDiagnosticData<'tcx> { + pub generator_interior_types: ty::Binder<'tcx, Vec>>, + pub hir_owner: DefId, + pub nodes_types: ItemLocalMap>, + pub adjustments: ItemLocalMap>>, +} + #[derive(TyEncodable, TyDecodable, Debug, HashStable)] pub struct TypeckResults<'tcx> { /// The `HirId::owner` all `ItemLocalId`s in this table are relative to. @@ -623,6 +633,28 @@ impl<'tcx> TypeckResults<'tcx> { LocalTableInContextMut { hir_owner: self.hir_owner, data: &mut self.node_types } } + pub fn get_generator_diagnostic_data(&self) -> GeneratorDiagnosticData<'tcx> { + let generator_interior_type = self.generator_interior_types.map_bound_ref(|vec| { + vec.iter() + .map(|item| { + GeneratorInteriorTypeCause { + ty: item.ty, + span: item.span, + scope_span: item.scope_span, + yield_span: item.yield_span, + expr: None, //FIXME: Passing expression over crate boundaries is impossible at the moment + } + }) + .collect::>() + }); + GeneratorDiagnosticData { + generator_interior_types: generator_interior_type, + hir_owner: self.hir_owner.to_def_id(), + nodes_types: self.node_types.clone(), + adjustments: self.adjustments.clone(), + } + } + pub fn node_type(&self, id: hir::HirId) -> Ty<'tcx> { self.node_type_opt(id).unwrap_or_else(|| { bug!("node_type: no type for node `{}`", tls::with(|tcx| tcx.hir().node_to_string(id))) diff --git a/compiler/rustc_middle/src/ty/mod.rs b/compiler/rustc_middle/src/ty/mod.rs index c2accea11ba2a..a0d0e4b9c2c64 100644 --- a/compiler/rustc_middle/src/ty/mod.rs +++ b/compiler/rustc_middle/src/ty/mod.rs @@ -67,8 +67,9 @@ pub use self::consts::{ }; pub use self::context::{ tls, CanonicalUserType, CanonicalUserTypeAnnotation, CanonicalUserTypeAnnotations, - CtxtInterners, DelaySpanBugEmitted, FreeRegionInfo, GeneratorInteriorTypeCause, GlobalCtxt, - Lift, OnDiskCache, TyCtxt, TypeckResults, UserType, UserTypeAnnotationIndex, + CtxtInterners, DelaySpanBugEmitted, FreeRegionInfo, GeneratorDiagnosticData, + GeneratorInteriorTypeCause, GlobalCtxt, Lift, OnDiskCache, TyCtxt, TypeckResults, UserType, + UserTypeAnnotationIndex, }; pub use self::instance::{Instance, InstanceDef}; pub use self::list::List; diff --git a/compiler/rustc_middle/src/ty/query.rs b/compiler/rustc_middle/src/ty/query.rs index 9e48c569c253a..7629d7a8259b8 100644 --- a/compiler/rustc_middle/src/ty/query.rs +++ b/compiler/rustc_middle/src/ty/query.rs @@ -31,8 +31,11 @@ use crate::traits::{self, ImplSource}; use crate::ty::fast_reject::SimplifiedType; use crate::ty::subst::{GenericArg, SubstsRef}; use crate::ty::util::AlwaysRequiresDrop; +use crate::ty::GeneratorDiagnosticData; use crate::ty::{self, AdtSizedConstraint, CrateInherentImpls, ParamEnvAnd, Ty, TyCtxt}; +use rustc_ast as ast; use rustc_ast::expand::allocator::AllocatorKind; +use rustc_attr as attr; use rustc_data_structures::fx::{FxHashMap, FxHashSet, FxIndexMap}; use rustc_data_structures::steal::Steal; use rustc_data_structures::svh::Svh; @@ -49,13 +52,10 @@ use rustc_session::cstore::{CrateDepKind, CrateSource}; use rustc_session::cstore::{ExternCrate, ForeignModule, LinkagePreference, NativeLib}; use rustc_session::utils::NativeLibKind; use rustc_session::Limits; -use rustc_target::abi; -use rustc_target::spec::PanicStrategy; - -use rustc_ast as ast; -use rustc_attr as attr; use rustc_span::symbol::Symbol; use rustc_span::{Span, DUMMY_SP}; +use rustc_target::abi; +use rustc_target::spec::PanicStrategy; use std::ops::Deref; use std::path::PathBuf; use std::sync::Arc; diff --git a/compiler/rustc_trait_selection/src/traits/error_reporting/suggestions.rs b/compiler/rustc_trait_selection/src/traits/error_reporting/suggestions.rs index ead1f0126c465..b49ee7b9446dc 100644 --- a/compiler/rustc_trait_selection/src/traits/error_reporting/suggestions.rs +++ b/compiler/rustc_trait_selection/src/traits/error_reporting/suggestions.rs @@ -19,9 +19,11 @@ use rustc_hir::def_id::DefId; use rustc_hir::intravisit::Visitor; use rustc_hir::lang_items::LangItem; use rustc_hir::{AsyncGeneratorKind, GeneratorKind, Node}; +use rustc_middle::hir::map; use rustc_middle::ty::{ self, suggest_arbitrary_trait_bound, suggest_constraining_type_param, AdtKind, DefIdTree, - Infer, InferTy, ToPredicate, Ty, TyCtxt, TypeFoldable, + GeneratorDiagnosticData, GeneratorInteriorTypeCause, Infer, InferTy, ToPredicate, Ty, TyCtxt, + TypeFoldable, }; use rustc_middle::ty::{TypeAndMut, TypeckResults}; use rustc_session::Limit; @@ -44,6 +46,123 @@ pub enum GeneratorInteriorOrUpvar { Upvar(Span), } +// This type provides a uniform interface to retrieve data on generators, whether it originated from +// the local crate being compiled or from a foreign crate. +#[derive(Debug)] +pub enum GeneratorData<'tcx, 'a> { + Local(&'a TypeckResults<'tcx>), + Foreign(&'tcx GeneratorDiagnosticData<'tcx>), +} + +impl<'tcx, 'a> GeneratorData<'tcx, 'a> { + // Try to get information about variables captured by the generator that matches a type we are + // looking for with `ty_matches` function. We uses it to find upvar which causes a failure to + // meet an obligation + fn try_get_upvar_span( + &self, + infer_context: &InferCtxt<'a, 'tcx>, + generator_did: DefId, + ty_matches: F, + ) -> Option + where + F: Fn(ty::Binder<'tcx, Ty<'tcx>>) -> bool, + { + match self { + GeneratorData::Local(typeck_results) => { + infer_context.tcx.upvars_mentioned(generator_did).and_then(|upvars| { + upvars.iter().find_map(|(upvar_id, upvar)| { + let upvar_ty = typeck_results.node_type(*upvar_id); + let upvar_ty = infer_context.resolve_vars_if_possible(upvar_ty); + if ty_matches(ty::Binder::dummy(upvar_ty)) { + Some(GeneratorInteriorOrUpvar::Upvar(upvar.span)) + } else { + None + } + }) + }) + } + GeneratorData::Foreign(_) => None, + } + } + + // Try to get the span of a type being awaited on that matches the type we are looking with the + // `ty_matches` function. We uses it to find awaited type which causes a failure to meet an + // obligation + fn get_from_await_ty( + &self, + visitor: AwaitsVisitor, + hir: map::Map<'tcx>, + ty_matches: F, + ) -> Option + where + F: Fn(ty::Binder<'tcx, Ty<'tcx>>) -> bool, + { + match self { + GeneratorData::Local(typeck_results) => visitor + .awaits + .into_iter() + .map(|id| hir.expect_expr(id)) + .find(|await_expr| { + ty_matches(ty::Binder::dummy(typeck_results.expr_ty_adjusted(&await_expr))) + }) + .map(|expr| expr.span), + GeneratorData::Foreign(generator_diagnostic_data) => visitor + .awaits + .into_iter() + .map(|id| hir.expect_expr(id)) + .find(|await_expr| { + ty_matches(ty::Binder::dummy( + generator_diagnostic_data + .adjustments + .get(&await_expr.hir_id.local_id) + .map_or::<&[ty::adjustment::Adjustment<'tcx>], _>(&[], |a| &a[..]) + .last() + .map_or_else::, _, _>( + || { + generator_diagnostic_data + .nodes_types + .get(&await_expr.hir_id.local_id) + .cloned() + .unwrap_or_else(|| { + bug!( + "node_type: no type for node `{}`", + ty::tls::with(|tcx| tcx + .hir() + .node_to_string(await_expr.hir_id)) + ) + }) + }, + |adj| adj.target, + ), + )) + }) + .map(|expr| expr.span), + } + } + + /// Get the type, expression, span and optional scope span of all types + /// that are live across the yield of this generator + fn get_generator_interior_types( + &self, + ) -> ty::Binder<'tcx, &Vec>> { + match self { + GeneratorData::Local(typeck_result) => typeck_result.generator_interior_types.as_ref(), + GeneratorData::Foreign(generator_diagnostic_data) => { + generator_diagnostic_data.generator_interior_types.as_ref() + } + } + } + + // Used to get the source of the data, note we don't have as much information for generators + // originated from foreign crates + fn is_foreign(&self) -> bool { + match self { + GeneratorData::Local(_) => false, + GeneratorData::Foreign(_) => true, + } + } +} + // This trait is public to expose the diagnostics methods to clippy. pub trait InferCtxtExt<'tcx> { fn suggest_restricting_param_bound( @@ -152,7 +271,7 @@ pub trait InferCtxtExt<'tcx> { err: &mut Diagnostic, interior_or_upvar_span: GeneratorInteriorOrUpvar, interior_extra_info: Option<(Option, Span, Option, Option)>, - inner_generator_body: Option<&hir::Body<'tcx>>, + is_async: bool, outer_generator: Option, trait_pred: ty::TraitPredicate<'tcx>, target_ty: Ty<'tcx>, @@ -1642,6 +1761,17 @@ impl<'a, 'tcx> InferCtxtExt<'tcx> for InferCtxt<'a, 'tcx> { .map(|def_id| hir.local_def_id_to_hir_id(def_id)) .and_then(|hir_id| hir.maybe_body_owned_by(hir_id)) .map(|body_id| hir.body(body_id)); + let is_async = match generator_did.as_local() { + Some(_) => generator_body + .and_then(|body| body.generator_kind()) + .map(|generator_kind| matches!(generator_kind, hir::GeneratorKind::Async(..))) + .unwrap_or(false), + None => self + .tcx + .generator_kind(generator_did) + .map(|generator_kind| matches!(generator_kind, hir::GeneratorKind::Async(..))) + .unwrap_or(false), + }; let mut visitor = AwaitsVisitor::default(); if let Some(body) = generator_body { visitor.visit_body(body); @@ -1682,61 +1812,55 @@ impl<'a, 'tcx> InferCtxtExt<'tcx> for InferCtxt<'a, 'tcx> { // type-checking; otherwise, get them by performing a query. This is needed to avoid // cycles. If we can't use resolved types because the generator comes from another crate, // we still provide a targeted error but without all the relevant spans. - let query_typeck_results; - let typeck_results: Option<&TypeckResults<'tcx>> = match &in_progress_typeck_results { - Some(t) if t.hir_owner.to_def_id() == generator_did_root => Some(&t), + let generator_data: Option> = match &in_progress_typeck_results { + Some(t) if t.hir_owner.to_def_id() == generator_did_root => { + Some(GeneratorData::Local(&t)) + } _ if generator_did.is_local() => { - query_typeck_results = self.tcx.typeck(generator_did.expect_local()); - Some(&query_typeck_results) + Some(GeneratorData::Local(self.tcx.typeck(generator_did.expect_local()))) } - _ => None, // Do not ICE on closure typeck (#66868). + _ => self + .tcx + .generator_diagnostic_data(generator_did) + .as_ref() + .map(|generator_diag_data| GeneratorData::Foreign(generator_diag_data)), }; - if let Some(typeck_results) = typeck_results { - if let Some(upvars) = self.tcx.upvars_mentioned(generator_did) { - interior_or_upvar_span = upvars.iter().find_map(|(upvar_id, upvar)| { - let upvar_ty = typeck_results.node_type(*upvar_id); - let upvar_ty = self.resolve_vars_if_possible(upvar_ty); - if ty_matches(ty::Binder::dummy(upvar_ty)) { - Some(GeneratorInteriorOrUpvar::Upvar(upvar.span)) - } else { - None - } - }); - }; + + if let Some(generator_data) = generator_data.as_ref() { + interior_or_upvar_span = + generator_data.try_get_upvar_span(&self, generator_did, ty_matches); // The generator interior types share the same binders if let Some(cause) = - typeck_results.generator_interior_types.as_ref().skip_binder().iter().find( + generator_data.get_generator_interior_types().skip_binder().iter().find( |ty::GeneratorInteriorTypeCause { ty, .. }| { - ty_matches(typeck_results.generator_interior_types.rebind(*ty)) + ty_matches(generator_data.get_generator_interior_types().rebind(*ty)) }, ) { - // Check to see if any awaited expressions have the target type. - let from_awaited_ty = visitor - .awaits - .into_iter() - .map(|id| hir.expect_expr(id)) - .find(|await_expr| { - ty_matches(ty::Binder::dummy(typeck_results.expr_ty_adjusted(&await_expr))) - }) - .map(|expr| expr.span); + let from_awaited_ty = generator_data.get_from_await_ty(visitor, hir, ty_matches); let ty::GeneratorInteriorTypeCause { span, scope_span, yield_span, expr, .. } = cause; interior_or_upvar_span = Some(GeneratorInteriorOrUpvar::Interior(*span)); interior_extra_info = Some((*scope_span, *yield_span, *expr, from_awaited_ty)); - }; - } else { - interior_or_upvar_span = Some(GeneratorInteriorOrUpvar::Interior(span)); + } + + if interior_or_upvar_span.is_none() && generator_data.is_foreign() { + interior_or_upvar_span = Some(GeneratorInteriorOrUpvar::Interior(span)); + } } if let Some(interior_or_upvar_span) = interior_or_upvar_span { + let typeck_results = generator_data.and_then(|generator_data| match generator_data { + GeneratorData::Local(typeck_results) => Some(typeck_results), + GeneratorData::Foreign(_) => None, + }); self.note_obligation_cause_for_async_await( err, interior_or_upvar_span, interior_extra_info, - generator_body, + is_async, outer_generator, trait_ref, target_ty, @@ -1757,7 +1881,7 @@ impl<'a, 'tcx> InferCtxtExt<'tcx> for InferCtxt<'a, 'tcx> { err: &mut Diagnostic, interior_or_upvar_span: GeneratorInteriorOrUpvar, interior_extra_info: Option<(Option, Span, Option, Option)>, - inner_generator_body: Option<&hir::Body<'tcx>>, + is_async: bool, outer_generator: Option, trait_pred: ty::TraitPredicate<'tcx>, target_ty: Ty<'tcx>, @@ -1767,10 +1891,6 @@ impl<'a, 'tcx> InferCtxtExt<'tcx> for InferCtxt<'a, 'tcx> { ) { let source_map = self.tcx.sess.source_map(); - let is_async = inner_generator_body - .and_then(|body| body.generator_kind()) - .map(|generator_kind| matches!(generator_kind, hir::GeneratorKind::Async(..))) - .unwrap_or(false); let (await_or_yield, an_await_or_yield) = if is_async { ("await", "an await") } else { ("yield", "a yield") }; let future_or_generator = if is_async { "future" } else { "generator" }; diff --git a/src/test/ui/async-await/issues/issue-67893.rs b/src/test/ui/async-await/issues/issue-67893.rs index 8b53408d758e1..d73772e5fa0d9 100644 --- a/src/test/ui/async-await/issues/issue-67893.rs +++ b/src/test/ui/async-await/issues/issue-67893.rs @@ -7,5 +7,5 @@ fn g(_: impl Send) {} fn main() { g(issue_67893::run()) - //~^ ERROR generator cannot be sent between threads safely + //~^ ERROR future cannot be sent between threads safely } diff --git a/src/test/ui/async-await/issues/issue-67893.stderr b/src/test/ui/async-await/issues/issue-67893.stderr index 0aa0d5d7ccdde..316b6d06f932a 100644 --- a/src/test/ui/async-await/issues/issue-67893.stderr +++ b/src/test/ui/async-await/issues/issue-67893.stderr @@ -1,10 +1,22 @@ -error: generator cannot be sent between threads safely +error: future cannot be sent between threads safely --> $DIR/issue-67893.rs:9:7 | LL | g(issue_67893::run()) - | ^^^^^^^^^^^^^^^^^^ generator is not `Send` + | ^^^^^^^^^^^^^^^^^^ future is not `Send` | = help: within `impl Future`, the trait `Send` is not implemented for `MutexGuard<'_, ()>` +note: future is not `Send` as this value is used across an await + --> $DIR/auxiliary/issue_67893.rs:9:26 + | +LL | f(*x.lock().unwrap()).await; + | ----------------- ^^^^^^ await occurs here, with `x.lock().unwrap()` maybe used later + | | + | has type `MutexGuard<'_, ()>` which is not `Send` +note: `x.lock().unwrap()` is later dropped here + --> $DIR/auxiliary/issue_67893.rs:9:32 + | +LL | f(*x.lock().unwrap()).await; + | ^ note: required by a bound in `g` --> $DIR/issue-67893.rs:6:14 |