-
Notifications
You must be signed in to change notification settings - Fork 13.3k
[WIP] Move Self: Trait
predicate to trait items instead of the trait itself
#50183
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
Changes from all commits
b914418
75884d4
32a0a76
defc7e9
f52d4de
edeb9c4
0faf7fb
9374cd2
4a98f86
1d888a4
671ede2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -30,7 +30,6 @@ use super::{Inherited, FnCtxt}; | |
/// - impl_m_span: span to use for reporting errors | ||
/// - trait_m: the method in the trait | ||
/// - impl_trait_ref: the TraitRef corresponding to the trait implementation | ||
|
||
pub fn compare_impl_method<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>, | ||
impl_m: &ty::AssociatedItem, | ||
impl_m_span: Span, | ||
|
@@ -182,7 +181,31 @@ fn compare_predicate_entailment<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>, | |
let impl_m_generics = tcx.generics_of(impl_m.def_id); | ||
let trait_m_generics = tcx.generics_of(trait_m.def_id); | ||
let impl_m_predicates = tcx.predicates_of(impl_m.def_id); | ||
let trait_m_predicates = tcx.predicates_of(trait_m.def_id); | ||
let mut trait_m_predicates = tcx.predicates_of(trait_m.def_id); | ||
|
||
// A `Self: Trait` predicate on trait items breaks selection, so filter it out. | ||
// FIXME: This is a big hack! | ||
if let Some(trait_def_id) = trait_m_predicates.parent { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What error do you get here? This feels surprising too! I would rather that -- instead of "filtering this out" -- we add something to the environment that allows it to be proven true, if needed. But it seems like the presence of this should fall out from changes elsewhere. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A full explanation is here: https://paper.dropbox.com/doc/Removing-SelfTrait-from-predicates_of-notes-neNFC7UaxCj353Du3Dd2d#:h2=Other-questions That whole doc is really just for this particular change (the title implies that it's for the whole task.) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. TL;DR: The extra predicate breaks downstream winnowing logic There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wondered if that would happen. |
||
trait_m_predicates.predicates.retain(|pred| { | ||
match pred { | ||
ty::Predicate::Trait(trait_ref) => { | ||
if trait_ref.def_id() == trait_def_id { | ||
use ty::TypeVariants::TyParam; | ||
if let TyParam(param) = trait_ref.skip_binder().self_ty().sty { | ||
if param.is_self() { | ||
debug!( | ||
"compare_impl_method: removing `{:?}` from trait_m_predicates", | ||
pred); | ||
return false; | ||
} | ||
} | ||
} | ||
true | ||
} | ||
_ => true | ||
} | ||
}); | ||
} | ||
|
||
// Check region bounds. | ||
check_region_bounds_on_impl_method(tcx, | ||
|
@@ -213,6 +236,8 @@ fn compare_predicate_entailment<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>, | |
hybrid_preds.predicates | ||
.extend(trait_m_predicates.instantiate_own(tcx, trait_to_skol_substs).predicates); | ||
|
||
debug!("compare_impl_method: impl_bounds+trait_bounds={:?}", hybrid_preds); | ||
|
||
// Construct trait parameter environment and then shift it into the skolemized viewpoint. | ||
// The key step here is to update the caller_bounds's predicates to be | ||
// the new hybrid bounds we computed. | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1321,12 +1321,17 @@ pub fn explicit_predicates_of<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>, | |
let node = tcx.hir.get(node_id); | ||
|
||
let mut is_trait = None; | ||
let mut is_trait_item = false; | ||
let mut is_default_impl_trait = None; | ||
|
||
let icx = ItemCtxt::new(tcx, def_id); | ||
let no_generics = hir::Generics::empty(); | ||
let ast_generics = match node { | ||
NodeTraitItem(item) => &item.generics, | ||
NodeTraitItem(item) => { | ||
is_trait_item = true; | ||
&item.generics | ||
} | ||
|
||
NodeImplItem(item) => &item.generics, | ||
|
||
NodeItem(item) => { | ||
|
@@ -1401,12 +1406,8 @@ pub fn explicit_predicates_of<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>, | |
// and the explicit where-clauses, but to get the full set of predicates | ||
// on a trait we need to add in the supertrait bounds and bounds found on | ||
// associated types. | ||
if let Some((trait_ref, _)) = is_trait { | ||
if let Some((_trait_ref, _)) = is_trait { | ||
predicates = tcx.super_predicates_of(def_id).predicates; | ||
|
||
// Add in a predicate that `Self:Trait` (where `Trait` is the | ||
// current trait). This is needed for builtin bounds. | ||
predicates.push(trait_ref.to_poly_trait_ref().to_predicate()); | ||
} | ||
|
||
// In default impls, we can assume that the self type implements | ||
|
@@ -1421,6 +1422,14 @@ pub fn explicit_predicates_of<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>, | |
predicates.push(trait_ref.to_poly_trait_ref().to_predicate()); | ||
} | ||
|
||
// For trait items, add `Self: Trait` predicate. | ||
if is_trait_item { | ||
let parent_id = tcx.hir.get_parent(node_id); | ||
let parent_def_id = tcx.hir.local_def_id(parent_id); | ||
debug_assert!(ty::is_trait_node(tcx, parent_id)); | ||
predicates.push(ty::TraitRef::identity(tcx, parent_def_id).to_predicate()); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This feels right! ✅ |
||
} | ||
|
||
// Collect the region predicates that were declared inline as | ||
// well. In the case of parameters declared on a fn or method, we | ||
// have to be careful to only iterate over early-bound regions. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This feels a bit ad-hoc to me. Can you remember why you added this? Also, when does this "has escaping regions" check return false? I would think .. never ? You could add a
assert!(!trait_ref.has_escaping_regions());
instead (and perhaps prove me wrong).If I understand correctly, this is saying that, to prove that
trait Foo { .. }
is well-formed, we have to prove thatSelf: Foo
? I'm not sure why that would be useful.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are indeed places where
trait_ref
has escaping regions. There is a callskip_binder()
above (look for(*)
).I believe an example that triggers this is here:
rust/src/test/compile-fail/issue-35570.rs
Lines 34 to 35 in 3eadd75
If the trait ref does have escaping regions, it would get filtered out below anyway. Except that
trait_ref.to_predicate()
invokesty::Binder::dummy
, which has the assertion you mentioned and we get a panic. Usingty::Binder::bind
is incorrect and messes up our indices.On a related note, this fn was copied and modified from the above. I should find a way to remove the repetition.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It gets used to check bounds on fn items as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm. I'll have to double check what's going on here then, I was confused.