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.

Sign up for GitHub

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

Permit evaluation of assoc items on Self by avoiding cycle error #74130

Closed
wants to merge 1 commit into from
Closed

Permit evaluation of assoc items on Self by avoiding cycle error #74130

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
32 changes: 29 additions & 3 deletions compiler/rustc_infer/src/traits/util.rs
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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);
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be better to avoid the visitor if !skip_self_param_evaluation or !is_self_param, right?

I would also maybe find that if below a little easier to read if it were restructured.


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,
Copy link
Contributor

Choose a reason for hiding this comment

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

Pre-existing, sort of, but could we introduce struct PermitVariants(bool) and struct DontRecurse(bool) or something? I found it hard to read calls like self.associated_path_to_ty(..., true, false) etc and remember what those booleans were meant to represent.

) -> 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