Skip to content

Commit

Permalink
Rollup merge of #92191 - jackh726:issue-89352, r=nikomatsakis
Browse files Browse the repository at this point in the history
Prefer projection candidates instead of param_env candidates for Sized predicates

Fixes #89352

Also includes some drive by logging and verbose printing changes that I found useful when debugging this, but I can remove this if needed.

This is a little hacky - but imo no more than the rest of `candidate_should_be_dropped_in_favor_of`. Importantly, in a Chalk-like world, both candidates should be completely compatible.

r? ```@nikomatsakis```
  • Loading branch information
matthiaskrgr committed Jan 15, 2022
2 parents b0ec3e0 + d3aecc1 commit 6471682
Show file tree
Hide file tree
Showing 13 changed files with 84 additions and 34 deletions.
2 changes: 1 addition & 1 deletion compiler/rustc_infer/src/traits/util.rs
Original file line number Diff line number Diff line change
Expand Up @@ -152,7 +152,7 @@ impl<'tcx> Elaborator<'tcx> {
obligation.cause.clone(),
)
});
debug!("super_predicates: data={:?}", data);
debug!(?data, ?obligations, "super_predicates");

// Only keep those bounds that we haven't already seen.
// This is necessary to prevent infinite recursion in some
Expand Down
5 changes: 5 additions & 0 deletions compiler/rustc_middle/src/ty/print/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -188,6 +188,11 @@ pub trait Printer<'tcx>: Sized {
own_params.start = 1;
}

// If we're in verbose mode, then print default-equal args too
if self.tcx().sess.verbose() {
return &substs[own_params];
}

// Don't print args that are the defaults of their respective parameters.
own_params.end -= generics
.params
Expand Down
9 changes: 5 additions & 4 deletions compiler/rustc_middle/src/ty/print/pretty.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1784,10 +1784,11 @@ impl<'tcx, F: fmt::Write> Printer<'tcx> for FmtPrinter<'_, 'tcx, F> {
self = print_prefix(self)?;

// Don't print `'_` if there's no unerased regions.
let print_regions = args.iter().any(|arg| match arg.unpack() {
GenericArgKind::Lifetime(r) => *r != ty::ReErased,
_ => false,
});
let print_regions = self.tcx.sess.verbose()
|| args.iter().any(|arg| match arg.unpack() {
GenericArgKind::Lifetime(r) => *r != ty::ReErased,
_ => false,
});
let args = args.iter().cloned().filter(|arg| match arg.unpack() {
GenericArgKind::Lifetime(_) => print_regions,
_ => true,
Expand Down
9 changes: 5 additions & 4 deletions compiler/rustc_trait_selection/src/traits/project.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1225,6 +1225,10 @@ fn assemble_candidates_from_object_ty<'cx, 'tcx>(
);
}

#[tracing::instrument(
level = "debug",
skip(selcx, candidate_set, ctor, env_predicates, potentially_unnormalized_candidates)
)]
fn assemble_candidates_from_predicates<'cx, 'tcx>(
selcx: &mut SelectionContext<'cx, 'tcx>,
obligation: &ProjectionTyObligation<'tcx>,
Expand All @@ -1233,8 +1237,6 @@ fn assemble_candidates_from_predicates<'cx, 'tcx>(
env_predicates: impl Iterator<Item = ty::Predicate<'tcx>>,
potentially_unnormalized_candidates: bool,
) {
debug!(?obligation, "assemble_candidates_from_predicates");

let infcx = selcx.infcx();
for predicate in env_predicates {
debug!(?predicate);
Expand Down Expand Up @@ -1270,13 +1272,12 @@ fn assemble_candidates_from_predicates<'cx, 'tcx>(
}
}

#[tracing::instrument(level = "debug", skip(selcx, obligation, candidate_set))]
fn assemble_candidates_from_impls<'cx, 'tcx>(
selcx: &mut SelectionContext<'cx, 'tcx>,
obligation: &ProjectionTyObligation<'tcx>,
candidate_set: &mut ProjectionTyCandidateSet<'tcx>,
) {
debug!("assemble_candidates_from_impls");

// If we are resolving `<T as TraitRef<...>>::Item == Type`,
// start out by selecting the predicate `T as TraitRef<...>`:
let poly_trait_ref = ty::Binder::dummy(obligation.predicate.trait_ref(selcx.tcx()));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -173,6 +173,9 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {

let needs_infer = stack.obligation.predicate.has_infer_types_or_consts();

let sized_predicate = self.tcx().lang_items().sized_trait()
== Some(stack.obligation.predicate.skip_binder().def_id());

// If there are STILL multiple candidates, we can further
// reduce the list by dropping duplicates -- including
// resolving specializations.
Expand All @@ -181,6 +184,7 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
while i < candidates.len() {
let is_dup = (0..candidates.len()).filter(|&j| i != j).any(|j| {
self.candidate_should_be_dropped_in_favor_of(
sized_predicate,
&candidates[i],
&candidates[j],
needs_infer,
Expand Down Expand Up @@ -338,13 +342,12 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
Ok(candidates)
}

#[tracing::instrument(level = "debug", skip(self, candidates))]
fn assemble_candidates_from_projected_tys(
&mut self,
obligation: &TraitObligation<'tcx>,
candidates: &mut SelectionCandidateSet<'tcx>,
) {
debug!(?obligation, "assemble_candidates_from_projected_tys");

// Before we go into the whole placeholder thing, just
// quickly check if the self-type is a projection at all.
match obligation.predicate.skip_binder().trait_ref.self_ty().kind() {
Expand All @@ -369,12 +372,13 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
/// supplied to find out whether it is listed among them.
///
/// Never affects the inference environment.
#[tracing::instrument(level = "debug", skip(self, stack, candidates))]
fn assemble_candidates_from_caller_bounds<'o>(
&mut self,
stack: &TraitObligationStack<'o, 'tcx>,
candidates: &mut SelectionCandidateSet<'tcx>,
) -> Result<(), SelectionError<'tcx>> {
debug!(?stack.obligation, "assemble_candidates_from_caller_bounds");
debug!(?stack.obligation);

let all_bounds = stack
.obligation
Expand Down Expand Up @@ -876,14 +880,15 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
};
}

#[tracing::instrument(level = "debug", skip(self, obligation, candidates))]
fn assemble_candidates_for_trait_alias(
&mut self,
obligation: &TraitObligation<'tcx>,
candidates: &mut SelectionCandidateSet<'tcx>,
) {
// Okay to skip binder here because the tests we do below do not involve bound regions.
let self_ty = obligation.self_ty().skip_binder();
debug!(?self_ty, "assemble_candidates_for_trait_alias");
debug!(?self_ty);

let def_id = obligation.predicate.def_id();

Expand All @@ -894,21 +899,20 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {

/// Assembles the trait which are built-in to the language itself:
/// `Copy`, `Clone` and `Sized`.
#[tracing::instrument(level = "debug", skip(self, candidates))]
fn assemble_builtin_bound_candidates(
&mut self,
conditions: BuiltinImplConditions<'tcx>,
candidates: &mut SelectionCandidateSet<'tcx>,
) {
match conditions {
BuiltinImplConditions::Where(nested) => {
debug!(?nested, "builtin_bound");
candidates
.vec
.push(BuiltinCandidate { has_nested: !nested.skip_binder().is_empty() });
}
BuiltinImplConditions::None => {}
BuiltinImplConditions::Ambiguous => {
debug!("assemble_builtin_bound_candidates: ambiguous builtin");
candidates.ambiguous = true;
}
}
Expand Down
23 changes: 14 additions & 9 deletions compiler/rustc_trait_selection/src/traits/select/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -201,6 +201,7 @@ struct EvaluatedCandidate<'tcx> {
}

/// When does the builtin impl for `T: Trait` apply?
#[derive(Debug)]
enum BuiltinImplConditions<'tcx> {
/// The impl is conditional on `T1, T2, ...: Trait`.
Where(ty::Binder<'tcx, Vec<Ty<'tcx>>>),
Expand Down Expand Up @@ -344,7 +345,7 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
}
Err(e) => Err(e),
Ok(candidate) => {
debug!(?candidate);
debug!(?candidate, "confirmed");
Ok(Some(candidate))
}
}
Expand Down Expand Up @@ -1523,6 +1524,7 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
/// See the comment for "SelectionCandidate" for more details.
fn candidate_should_be_dropped_in_favor_of(
&mut self,
sized_predicate: bool,
victim: &EvaluatedCandidate<'tcx>,
other: &EvaluatedCandidate<'tcx>,
needs_infer: bool,
Expand Down Expand Up @@ -1594,6 +1596,16 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
// Drop otherwise equivalent non-const fn pointer candidates
(FnPointerCandidate { .. }, FnPointerCandidate { is_const: false }) => true,

// If obligation is a sized predicate or the where-clause bound is
// global, prefer the projection or object candidate. See issue
// #50825 and #89352.
(ObjectCandidate(_) | ProjectionCandidate(_), ParamCandidate(ref cand)) => {
sized_predicate || is_global(cand)
}
(ParamCandidate(ref cand), ObjectCandidate(_) | ProjectionCandidate(_)) => {
!(sized_predicate || is_global(cand))
}

// Global bounds from the where clause should be ignored
// here (see issue #50825). Otherwise, we have a where
// clause so don't go around looking for impls.
Expand All @@ -1609,15 +1621,8 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
| BuiltinUnsizeCandidate
| TraitUpcastingUnsizeCandidate(_)
| BuiltinCandidate { .. }
| TraitAliasCandidate(..)
| ObjectCandidate(_)
| ProjectionCandidate(_),
| TraitAliasCandidate(..),
) => !is_global(cand),
(ObjectCandidate(_) | ProjectionCandidate(_), ParamCandidate(ref cand)) => {
// Prefer these to a global where-clause bound
// (see issue #50825).
is_global(cand)
}
(
ImplCandidate(_)
| ClosureCandidate
Expand Down
4 changes: 2 additions & 2 deletions compiler/rustc_typeck/src/astconv/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -288,7 +288,7 @@ impl<'o, 'tcx> dyn AstConv<'tcx> + 'o {
/// Given the type/lifetime/const arguments provided to some path (along with
/// an implicit `Self`, if this is a trait reference), returns the complete
/// set of substitutions. This may involve applying defaulted type parameters.
/// Also returns back constraints on associated types.
/// Constraints on associated typess are created from `create_assoc_bindings_for_generic_args`.
///
/// Example:
///
Expand All @@ -302,7 +302,7 @@ impl<'o, 'tcx> dyn AstConv<'tcx> + 'o {
/// which will have been resolved to a `def_id`
/// 3. The `generic_args` contains info on the `<...>` contents. The `usize` type
/// parameters are returned in the `SubstsRef`, the associated type bindings like
/// `Output = u32` are returned in the `Vec<ConvertedBinding...>` result.
/// `Output = u32` are returned from `create_assoc_bindings_for_generic_args`.
///
/// Note that the type listing given here is *exactly* what the user provided.
///
Expand Down
2 changes: 2 additions & 0 deletions compiler/rustc_typeck/src/check/method/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -359,6 +359,8 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
let (obligation, substs) =
self.obligation_for_method(span, trait_def_id, self_ty, opt_input_types);

debug!(?obligation);

// Now we want to know if this can be matched
if !self.predicate_may_hold(&obligation) {
debug!("--> Cannot match obligation");
Expand Down
2 changes: 1 addition & 1 deletion src/test/ui/associated-types/substs-ppaux.rs
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ fn foo<'z>() where &'z (): Sized {
let x: () = <i8 as Foo<'static, 'static, u32>>::bar::<'static, char>;
//[verbose]~^ ERROR mismatched types
//[verbose]~| expected unit type `()`
//[verbose]~| found fn item `fn() {<i8 as Foo<ReStatic, ReStatic>>::bar::<ReStatic, char>}`
//[verbose]~| found fn item `fn() {<i8 as Foo<ReStatic, ReStatic, u32>>::bar::<ReStatic, char>}`
//[normal]~^^^^ ERROR mismatched types
//[normal]~| expected unit type `()`
//[normal]~| found fn item `fn() {<i8 as Foo<'static, 'static>>::bar::<'static, char>}`
Expand Down
4 changes: 2 additions & 2 deletions src/test/ui/associated-types/substs-ppaux.verbose.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -20,15 +20,15 @@ error[E0308]: mismatched types
--> $DIR/substs-ppaux.rs:25:17
|
LL | fn bar<'a, T>() where T: 'a {}
| --------------------------- fn() {<i8 as Foo<ReStatic, ReStatic>>::bar::<ReStatic, char>} defined here
| --------------------------- fn() {<i8 as Foo<ReStatic, ReStatic, u32>>::bar::<ReStatic, char>} defined here
...
LL | let x: () = <i8 as Foo<'static, 'static, u32>>::bar::<'static, char>;
| -- ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ expected `()`, found fn item
| |
| expected due to this
|
= note: expected unit type `()`
found fn item `fn() {<i8 as Foo<ReStatic, ReStatic>>::bar::<ReStatic, char>}`
found fn item `fn() {<i8 as Foo<ReStatic, ReStatic, u32>>::bar::<ReStatic, char>}`
help: use parentheses to call this function
|
LL | let x: () = <i8 as Foo<'static, 'static, u32>>::bar::<'static, char>();
Expand Down
32 changes: 32 additions & 0 deletions src/test/ui/generic-associated-types/issue-89352.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
// check-pass

#![feature(generic_associated_types)]

use std::marker::PhantomData;

pub trait GenAssoc<T> {
type Iter<'at>;
fn iter(&self) -> Self::Iter<'_>;
fn reborrow<'longt: 'shortt, 'shortt>(iter: Self::Iter<'longt>) -> Self::Iter<'shortt>;
}

pub struct Wrapper<'a, T: 'a, A: GenAssoc<T>> {
a: A::Iter<'a>,
_p: PhantomData<T>,
}

impl<'ai, T: 'ai, A: GenAssoc<T>> GenAssoc<T> for Wrapper<'ai, T, A>
where
A::Iter<'ai>: Clone,
{
type Iter<'b> = ();
fn iter<'s>(&'s self) -> Self::Iter<'s> {
let a = A::reborrow::<'ai, 's>(self.a.clone());
}

fn reborrow<'long: 'short, 'short>(iter: Self::Iter<'long>) -> Self::Iter<'short> {
()
}
}

fn main() {}
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ LL | with_signature(x, |mut y| Box::new(y.next()))
|
= note: defining type: no_region::<'_#1r, T>::{closure#0} with closure substs [
i32,
extern "rust-call" fn((std::boxed::Box<T>,)) -> std::boxed::Box<(dyn Anything + '_#2r)>,
extern "rust-call" fn((std::boxed::Box<T, std::alloc::Global>,)) -> std::boxed::Box<(dyn Anything + '_#2r), std::alloc::Global>,
(),
]
= note: number of external vids: 3
Expand Down Expand Up @@ -42,7 +42,7 @@ LL | with_signature(x, |mut y| Box::new(y.next()))
|
= note: defining type: correct_region::<'_#1r, T>::{closure#0} with closure substs [
i32,
extern "rust-call" fn((std::boxed::Box<T>,)) -> std::boxed::Box<(dyn Anything + '_#2r)>,
extern "rust-call" fn((std::boxed::Box<T, std::alloc::Global>,)) -> std::boxed::Box<(dyn Anything + '_#2r), std::alloc::Global>,
(),
]
= note: number of external vids: 3
Expand All @@ -69,7 +69,7 @@ LL | with_signature(x, |mut y| Box::new(y.next()))
|
= note: defining type: wrong_region::<'_#1r, '_#2r, T>::{closure#0} with closure substs [
i32,
extern "rust-call" fn((std::boxed::Box<T>,)) -> std::boxed::Box<(dyn Anything + '_#3r)>,
extern "rust-call" fn((std::boxed::Box<T, std::alloc::Global>,)) -> std::boxed::Box<(dyn Anything + '_#3r), std::alloc::Global>,
(),
]
= note: number of external vids: 4
Expand Down Expand Up @@ -105,7 +105,7 @@ LL | with_signature(x, |mut y| Box::new(y.next()))
|
= note: defining type: outlives_region::<'_#1r, '_#2r, T>::{closure#0} with closure substs [
i32,
extern "rust-call" fn((std::boxed::Box<T>,)) -> std::boxed::Box<(dyn Anything + '_#3r)>,
extern "rust-call" fn((std::boxed::Box<T, std::alloc::Global>,)) -> std::boxed::Box<(dyn Anything + '_#3r), std::alloc::Global>,
(),
]
= note: number of external vids: 4
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ LL | with_signature(x, |y| y)
|
= note: defining type: no_region::<'_#1r, T>::{closure#0} with closure substs [
i32,
extern "rust-call" fn((std::boxed::Box<T>,)) -> std::boxed::Box<(dyn std::fmt::Debug + '_#2r)>,
extern "rust-call" fn((std::boxed::Box<T, std::alloc::Global>,)) -> std::boxed::Box<(dyn std::fmt::Debug + '_#2r), std::alloc::Global>,
(),
]
= note: number of external vids: 3
Expand Down

0 comments on commit 6471682

Please sign in to comment.