Skip to content

Commit

Permalink
Permit evaluation of assoc items on Self by avoiding cycle error
Browse files Browse the repository at this point in the history
Skip the recursive evaluation of super-traits that reference associated
types on `Self` directly (without using a fully-qualified path) in order
to allow its use.

The following code would previously emit a cycle evaluation error and
will now successfuly compile:

```rust
trait Bar<T> {
    fn foo() -> T;
}
trait Foo: Bar<Self::Qux> {
    type Qux;
}
```

The following will also work:

```rust
trait Bar<T> {
    fn foo() -> T;
}
trait Baz {
    type Qux;
}
trait Foo: Bar<Self::Qux> + Baz {}
```

Fix rust-lang#35237.
  • Loading branch information
estebank committed Aug 31, 2020
1 parent 85fbf49 commit 53bacac
Show file tree
Hide file tree
Showing 11 changed files with 310 additions and 65 deletions.
32 changes: 29 additions & 3 deletions compiler/rustc_infer/src/traits/util.rs
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,10 @@ impl Extend<ty::Predicate<'tcx>> for PredicateSet<'tcx> {
pub struct Elaborator<'tcx> {
stack: Vec<PredicateObligation<'tcx>>,
visited: PredicateSet<'tcx>,
/// If this is `true` we will call `super_predicates_of_skip_self_param` instead of
/// `super_predicates_of`, which lets us get around cycle errors when super-traits reference
/// associated types on `Self`. See `super_predicates_of_skip_self_param` for more info.
evaluate_self_param: bool,
}

pub fn elaborate_trait_ref<'tcx>(
Expand Down Expand Up @@ -104,7 +108,7 @@ pub fn elaborate_obligations<'tcx>(
) -> Elaborator<'tcx> {
let mut visited = PredicateSet::new(tcx);
obligations.retain(|obligation| visited.insert(obligation.predicate));
Elaborator { stack: obligations, visited }
Elaborator { stack: obligations, visited, evaluate_self_param: true }
}

fn predicate_obligation<'tcx>(
Expand All @@ -131,9 +135,14 @@ impl Elaborator<'tcx> {
match obligation.predicate.skip_binders() {
ty::PredicateAtom::Trait(data, _) => {
// Get predicates declared on the trait.
let predicates = tcx.super_predicates_of(data.def_id());
let predicates = if self.evaluate_self_param {
tcx.super_predicates_of(data.def_id())
} else {
tcx.super_predicates_of_skip_self_param(data.def_id())
}
.predicates;

let obligations = predicates.predicates.iter().map(|&(pred, span)| {
let obligations = predicates.iter().map(|&(pred, span)| {
predicate_obligation(
pred.subst_supertrait(tcx, &ty::Binder::bind(data.trait_ref)),
Some(span),
Expand Down Expand Up @@ -271,6 +280,23 @@ pub fn supertraits<'tcx>(
elaborate_trait_ref(tcx, trait_ref).filter_to_traits()
}

/// Elaborate supertraits.
///
/// See `super_predicates_of_skip_self_param` for the behavior when `skip_self_param` is `true`.
pub fn supertraits_maybe_skip_self_param_users<'tcx>(
tcx: TyCtxt<'tcx>,
bounds: impl Iterator<Item = ty::PolyTraitRef<'tcx>>,
skip_self_param: bool,
) -> Supertraits<'tcx> {
let mut obligations: Vec<_> = bounds
.map(|trait_ref| predicate_obligation(trait_ref.without_const().to_predicate(tcx), None))
.collect();
let mut visited = PredicateSet::new(tcx);
obligations.retain(|obligation| visited.insert(obligation.predicate));
Elaborator { stack: obligations, visited, evaluate_self_param: !skip_self_param }
.filter_to_traits()
}

pub fn transitive_bounds<'tcx>(
tcx: TyCtxt<'tcx>,
bounds: impl Iterator<Item = ty::PolyTraitRef<'tcx>>,
Expand Down
7 changes: 7 additions & 0 deletions compiler/rustc_middle/src/query/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -357,6 +357,13 @@ rustc_queries! {
desc { |tcx| "computing the supertraits of `{}`", tcx.def_path_str(key) }
}

query super_predicates_of_skip_self_param(key: DefId) -> ty::GenericPredicates<'tcx> {
desc {
|tcx| "computing the supertraits of `{}` skipping super-traits referencing `Self`",
tcx.def_path_str(key)
}
}

/// To avoid cycles within the predicates of a single item we compute
/// per-type-parameter predicates for resolving `T::AssocTy`.
query type_param_predicates(key: (DefId, LocalDefId)) -> ty::GenericPredicates<'tcx> {
Expand Down
3 changes: 2 additions & 1 deletion compiler/rustc_trait_selection/src/traits/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,8 @@ pub use self::util::{
get_vtable_index_of_object_method, impl_item_is_final, predicate_for_trait_def, upcast_choices,
};
pub use self::util::{
supertrait_def_ids, supertraits, transitive_bounds, SupertraitDefIds, Supertraits,
supertrait_def_ids, supertraits, supertraits_maybe_skip_self_param_users, transitive_bounds,
SupertraitDefIds, Supertraits,
};

pub use self::chalk_fulfill::FulfillmentContext as ChalkFulfillmentContext;
Expand Down
173 changes: 136 additions & 37 deletions compiler/rustc_typeck/src/astconv/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,14 +15,14 @@ use rustc_errors::{struct_span_err, Applicability, ErrorReported, FatalError};
use rustc_hir as hir;
use rustc_hir::def::{CtorOf, DefKind, Namespace, Res};
use rustc_hir::def_id::{DefId, LocalDefId};
use rustc_hir::intravisit::{walk_generics, Visitor as _};
use rustc_hir::intravisit::{self, walk_generics, Visitor};
use rustc_hir::lang_items::LangItem;
use rustc_hir::{Constness, GenericArg, GenericArgs};
use rustc_middle::ty::subst::{self, InternalSubsts, Subst, SubstsRef};
use rustc_middle::ty::GenericParamDefKind;
use rustc_middle::ty::{self, Const, DefIdTree, Ty, TyCtxt, TypeFoldable};
use rustc_session::lint::builtin::AMBIGUOUS_ASSOCIATED_ITEMS;
use rustc_span::symbol::{Ident, Symbol};
use rustc_span::symbol::{kw, Ident, Symbol};
use rustc_span::{Span, DUMMY_SP};
use rustc_target::spec::abi;
use rustc_trait_selection::traits;
Expand Down Expand Up @@ -285,8 +285,7 @@ impl<'o, 'tcx> dyn AstConv<'tcx> + 'o {
// region with the current anon region binding (in other words,
// whatever & would get replaced with).
debug!(
"create_substs_for_ast_path(def_id={:?}, self_ty={:?}, \
generic_args={:?})",
"create_substs_for_ast_path(def_id={:?}, self_ty={:?}, generic_args={:?})",
def_id, self_ty, generic_args
);

Expand Down Expand Up @@ -316,7 +315,7 @@ impl<'o, 'tcx> dyn AstConv<'tcx> + 'o {
infer_args,
);

let is_object = self_ty.map_or(false, |ty| ty == self.tcx().types.trait_object_dummy_self);
let is_object = self_ty.map_or(false, |ty| ty == tcx.types.trait_object_dummy_self);
let default_needs_object_self = |param: &ty::GenericParamDef| {
if let GenericParamDefKind::Type { has_default, .. } = param.kind {
if is_object && has_default {
Expand Down Expand Up @@ -356,14 +355,42 @@ impl<'o, 'tcx> dyn AstConv<'tcx> + 'o {
(GenericParamDefKind::Lifetime, GenericArg::Lifetime(lt)) => {
self.ast_region_to_region(&lt, Some(param)).into()
}
(GenericParamDefKind::Type { .. }, GenericArg::Type(ty)) => {
if let (hir::TyKind::Infer, false) = (&ty.kind, self.allow_ty_infer()) {
(GenericParamDefKind::Type { .. }, GenericArg::Type(ty)) => match &ty.kind {
hir::TyKind::Infer if !self.allow_ty_infer() => {
inferred_params.push(ty.span);
tcx.ty_error().into()
} else {
self.ast_ty_to_ty(&ty).into()
}
}
hir::TyKind::Path(hir::QPath::TypeRelative(
hir::Ty {
kind:
hir::TyKind::Path(
hir::QPath::Resolved(
None,
hir::Path { res: res @ Res::SelfTy(Some(_), None), .. },
),
..,
),
..
},
segment,
)) if Some(tcx.types.self_param) == self_ty => {
// Handle `trait A: B<Self::C> { type C; }` to avoid cycle error.
// See `super_predicates_of_skip_self_param` for more information.
self.associated_path_to_ty(
ty.hir_id,
ty.span,
tcx.types.self_param,
*res,
segment,
false,
true,
)
.map(|(ty, _, _)| ty)
.unwrap_or_else(|_| tcx.ty_error())
.into()
}
_ => self.ast_ty_to_ty(&ty).into(),
},
(GenericParamDefKind::Const, GenericArg::Const(ct)) => {
ty::Const::from_opt_const_arg_anon_const(
tcx,
Expand Down Expand Up @@ -527,7 +554,10 @@ impl<'o, 'tcx> dyn AstConv<'tcx> + 'o {
) -> GenericArgCountResult {
let trait_def_id = trait_ref.trait_def_id().unwrap_or_else(|| FatalError.raise());

debug!("instantiate_poly_trait_ref({:?}, def_id={:?})", trait_ref, trait_def_id);
debug!(
"instantiate_poly_trait_ref({:?}, def_id={:?}, self_ty={:?})",
trait_ref, trait_def_id, self_ty
);

self.prohibit_generics(trait_ref.path.segments.split_last().unwrap().1);

Expand Down Expand Up @@ -704,9 +734,8 @@ impl<'o, 'tcx> dyn AstConv<'tcx> + 'o {
if tpb.path.res != Res::Def(DefKind::Trait, kind_id) {
tcx.sess.span_warn(
span,
"default bound relaxed for a type parameter, but \
this does nothing because the given bound is not \
a default; only `?Sized` is supported",
"default bound relaxed for a type parameter, but this does nothing \
because the given bound is not a default; only `?Sized` is supported",
);
}
}
Expand Down Expand Up @@ -741,15 +770,61 @@ impl<'o, 'tcx> dyn AstConv<'tcx> + 'o {
param_ty: Ty<'tcx>,
ast_bounds: &[hir::GenericBound<'_>],
bounds: &mut Bounds<'tcx>,
skip_self_param_evaluation: bool,
) {
let mut trait_bounds = Vec::new();
let mut region_bounds = Vec::new();

/// Walk each bound to see if they reference an associated type of `Self` without
/// specifying a trait (using a Resolved path `Self::Foo` instead of fully qualified path
/// `<Self as Bar>::Foo`) and if so, skip them if `skip_self_param_evaluation` is `true`.
/// This way, we avoid cycle errors and are able to resolve the associated type
/// successfully.
struct SelfFinder(bool);

impl<'v> Visitor<'v> for SelfFinder {
type Map = intravisit::ErasedMap<'v>;

fn nested_visit_map(&mut self) -> intravisit::NestedVisitorMap<Self::Map> {
intravisit::NestedVisitorMap::None
}

fn visit_ty(&mut self, t: &'v hir::Ty<'v>) {
if let hir::TyKind::Path(hir::QPath::TypeRelative(
hir::Ty {
kind:
hir::TyKind::Path(
hir::QPath::Resolved(
None,
hir::Path { res: Res::SelfTy(Some(_), None), .. },
),
..,
),
..
},
_,
)) = t.kind
{
self.0 = true;
}
intravisit::walk_ty(self, t)
}
}

let constness = self.default_constness_for_trait_bounds();
for ast_bound in ast_bounds {
match *ast_bound {
hir::GenericBound::Trait(ref b, hir::TraitBoundModifier::None) => {
trait_bounds.push((b, constness))
hir::GenericBound::Trait(ref b, modif @ hir::TraitBoundModifier::None) => {
let mut visitor = SelfFinder(false);
visitor.visit_poly_trait_ref(b, modif);

let is_self_param = match param_ty.kind {
ty::Param(param) if param.name == kw::SelfUpper => true,
_ => false,
};
if !(visitor.0 && skip_self_param_evaluation && is_self_param) {
trait_bounds.push((b, constness))
}
}
hir::GenericBound::Trait(ref b, hir::TraitBoundModifier::MaybeConst) => {
trait_bounds.push((b, Constness::NotConst))
Expand Down Expand Up @@ -794,10 +869,11 @@ impl<'o, 'tcx> dyn AstConv<'tcx> + 'o {
ast_bounds: &[hir::GenericBound<'_>],
sized_by_default: SizedByDefault,
span: Span,
skip_self_param_evaluation: bool,
) -> Bounds<'tcx> {
let mut bounds = Bounds::default();

self.add_bounds(param_ty, ast_bounds, &mut bounds);
self.add_bounds(param_ty, ast_bounds, &mut bounds, skip_self_param_evaluation);
bounds.trait_bounds.sort_by_key(|(t, _, _)| t.def_id());

bounds.implicitly_sized = if let SizedByDefault::Yes = sized_by_default {
Expand Down Expand Up @@ -969,7 +1045,7 @@ impl<'o, 'tcx> dyn AstConv<'tcx> + 'o {
// Calling `skip_binder` is okay, because `add_bounds` expects the `param_ty`
// parameter to have a skipped binder.
let param_ty = tcx.mk_projection(assoc_ty.def_id, candidate.skip_binder().substs);
self.add_bounds(param_ty, ast_bounds, bounds);
self.add_bounds(param_ty, ast_bounds, bounds, false);
}
}
Ok(())
Expand All @@ -996,7 +1072,7 @@ impl<'o, 'tcx> dyn AstConv<'tcx> + 'o {

let mut bounds = Bounds::default();
let mut potential_assoc_types = Vec::new();
let dummy_self = self.tcx().types.trait_object_dummy_self;
let dummy_self = tcx.types.trait_object_dummy_self;
for trait_bound in trait_bounds.iter().rev() {
if let GenericArgCountResult {
correct:
Expand Down Expand Up @@ -1274,15 +1350,19 @@ impl<'o, 'tcx> dyn AstConv<'tcx> + 'o {
err.emit();
}

// Search for a bound on a type parameter which includes the associated item
// given by `assoc_name`. `ty_param_def_id` is the `DefId` of the type parameter
// This function will fail if there are no suitable bounds or there is
// any ambiguity.
/// Search for a bound on a type parameter which includes the associated item
/// given by `assoc_name`. `ty_param_def_id` is the `DefId` of the type parameter
/// This function will fail if there are no suitable bounds or there is
/// any ambiguity.
///
/// See `super_predicates_of_skip_self_param` for the behavior when `skip_self_param_evaluation`
/// is `true`.
fn find_bound_for_assoc_item(
&self,
ty_param_def_id: LocalDefId,
assoc_name: Ident,
span: Span,
skip_self_param_evaluation: bool,
) -> Result<ty::PolyTraitRef<'tcx>, ErrorReported> {
let tcx = self.tcx();

Expand All @@ -1300,9 +1380,10 @@ impl<'o, 'tcx> dyn AstConv<'tcx> + 'o {
let param_name = tcx.hir().ty_param_name(param_hir_id);
self.one_bound_for_assoc_type(
|| {
traits::transitive_bounds(
traits::supertraits_maybe_skip_self_param_users(
tcx,
predicates.iter().filter_map(|(p, _)| p.to_opt_poly_trait_ref()),
skip_self_param_evaluation,
)
},
|| param_name.to_string(),
Expand Down Expand Up @@ -1432,12 +1513,16 @@ impl<'o, 'tcx> dyn AstConv<'tcx> + 'o {
Ok(bound)
}

// Create a type from a path to an associated type.
// For a path `A::B::C::D`, `qself_ty` and `qself_def` are the type and def for `A::B::C`
// and item_segment is the path segment for `D`. We return a type and a def for
// the whole path.
// Will fail except for `T::A` and `Self::A`; i.e., if `qself_ty`/`qself_def` are not a type
// parameter or `Self`.
/// Create a type from a path to an associated type.
/// For a path `A::B::C::D`, `qself_ty` and `qself_def` are the type and def for `A::B::C`
/// and `item_segment` is the path segment for `D`. We return a type and a def for
/// the whole path.
/// Will fail except for `T::A` and `Self::A`; i.e., if `qself_ty`/`qself_def` are not a type
/// parameter or `Self`.
///
/// When `dont_recurse` is `true`, and we are evaluation a path rooted in `Self`, we will skip
/// super-traits that reference `Self` to avoid cycle errors. See
/// `super_predicates_of_skip_self_param` for more information.
pub fn associated_path_to_ty(
&self,
hir_ref_id: hir::HirId,
Expand All @@ -1446,6 +1531,7 @@ impl<'o, 'tcx> dyn AstConv<'tcx> + 'o {
qself_res: Res,
assoc_segment: &hir::PathSegment<'_>,
permit_variants: bool,
dont_recurse: bool,
) -> Result<(Ty<'tcx>, DefKind, DefId), ErrorReported> {
let tcx = self.tcx();
let assoc_ident = assoc_segment.ident;
Expand Down Expand Up @@ -1494,10 +1580,15 @@ impl<'o, 'tcx> dyn AstConv<'tcx> + 'o {
|| None,
)?
}
(
&ty::Param(_),
Res::SelfTy(Some(param_did), None) | Res::Def(DefKind::TyParam, param_did),
) => self.find_bound_for_assoc_item(param_did.expect_local(), assoc_ident, span)?,
(&ty::Param(_), Res::SelfTy(Some(param_did), None)) => self.find_bound_for_assoc_item(
param_did.expect_local(),
assoc_ident,
span,
dont_recurse,
)?,
(&ty::Param(_), Res::Def(DefKind::TyParam, param_did)) => {
self.find_bound_for_assoc_item(param_did.expect_local(), assoc_ident, span, false)?
}
_ => {
if variant_resolution.is_some() {
// Variant in type position
Expand Down Expand Up @@ -2036,9 +2127,17 @@ impl<'o, 'tcx> dyn AstConv<'tcx> + 'o {
} else {
Res::Err
};
self.associated_path_to_ty(ast_ty.hir_id, ast_ty.span, ty, res, segment, false)
.map(|(ty, _, _)| ty)
.unwrap_or_else(|_| tcx.ty_error())
self.associated_path_to_ty(
ast_ty.hir_id,
ast_ty.span,
ty,
res,
segment,
false,
false,
)
.map(|(ty, _, _)| ty)
.unwrap_or_else(|_| tcx.ty_error())
}
hir::TyKind::Path(hir::QPath::LangItem(lang_item, span)) => {
let def_id = tcx.require_lang_item(lang_item, Some(span));
Expand Down
Loading

0 comments on commit 53bacac

Please sign in to comment.