From 13f7edc030631c0e76e06afe7055a1c9ab2d7e1b Mon Sep 17 00:00:00 2001 From: lcnr Date: Mon, 10 Jan 2022 13:32:09 +0100 Subject: [PATCH] don't simplify projections --- compiler/rustc_middle/src/ty/fast_reject.rs | 41 +++++++++---------- .../hr-associated-type-bound-object.stderr | 4 ++ src/test/ui/issues/issue-38821.stderr | 6 +-- 3 files changed, 25 insertions(+), 26 deletions(-) diff --git a/compiler/rustc_middle/src/ty/fast_reject.rs b/compiler/rustc_middle/src/ty/fast_reject.rs index daf9156a15f34..295c4d548052d 100644 --- a/compiler/rustc_middle/src/ty/fast_reject.rs +++ b/compiler/rustc_middle/src/ty/fast_reject.rs @@ -60,29 +60,23 @@ pub enum StripReferences { No, } -/// Tries to simplify a type by only returning the outermost injective¹ layer, if one exists. +/// Tries to simplify a type by only returning the outermost layer of an inductive type, if one exists. /// /// The idea is to get something simple that we can use to quickly decide if two types could unify, /// for example during method lookup. /// -/// A special case here are parameters and projections. Projections can be normalized to -/// a different type, meaning that `::Assoc` and `u8` can be unified, even though -/// their outermost layer is different while parameters like `T` of impls are later replaced -/// with an inference variable, which then also allows unification with other types. +/// We have to be careful with parameters and projections as we might want them to unify with other types. +/// Projections can be normalized to a different type, meaning that `::Assoc` +/// and `u8` can be unified, even though their outermost layer is different while parameters like `T` of +/// impls are later replaced with an inference variable, which then also allows unification with other types. /// -/// When using `SimplifyParams::Yes`, we still return a simplified type for params and projections², -/// the reasoning for this can be seen at the places doing this. +/// When using `SimplifyParams::Yes`, we still return a simplified type for params. +/// This is used during candidate selection when looking for impls satisfying `T: Trait` +/// but must not be used on candidates, as the `T` in `impl Trait for T` can unify +/// with any other type. /// /// For diagnostics we strip references with `StripReferences::Yes`. This is currently the best /// way to skip some unhelpful suggestions. -/// -/// ¹ meaning that if two outermost layers are different, then the whole types are also different. -/// ² FIXME(@lcnr): this seems like it can actually end up being unsound with the way it's used during -/// candidate selection. We do not consider non blanket impls for `<_ as Trait>::Assoc` even -/// though `_` can be inferred to a concrete type later at which point a concrete impl -/// could actually apply. After experimenting for about an hour I wasn't able to cause any issues -/// this way so I am not going to change this until we actually find an issue as I am really -/// interesting in getting an actual test for this. pub fn simplify_type( tcx: TyCtxt<'_>, ty: Ty<'_>, @@ -124,18 +118,23 @@ pub fn simplify_type( ty::Never => Some(NeverSimplifiedType), ty::Tuple(ref tys) => Some(TupleSimplifiedType(tys.len())), ty::FnPtr(ref f) => Some(FunctionSimplifiedType(f.skip_binder().inputs().len())), - ty::Projection(_) | ty::Param(_) => { + ty::Param(_) => { if can_simplify_params == SimplifyParams::Yes { - // In normalized types, projections don't unify with - // anything. when lazy normalization happens, this - // will change. It would still be nice to have a way - // to deal with known-not-to-unify-with-anything - // projections (e.g., the likes of <__S as Encoder>::Error). Some(ParameterSimplifiedType) } else { None } } + ty::Projection(_) => { + // Returning a simplified for projections can end up being unsound as it would + // stop us from considering not consider non blanket impls for `<_ as Trait>::Assoc` even + // though `_` can be inferred to a concrete type later at which point a concrete impl + // could actually apply. + // + // We could return `Some` here in cases where the input type is fully normalized, but + // I (@lcnr) don't know how to guarantee that. + None + } ty::Opaque(def_id, _) => Some(OpaqueSimplifiedType(def_id)), ty::Foreign(def_id) => Some(ForeignSimplifiedType(def_id)), ty::Placeholder(..) | ty::Bound(..) | ty::Infer(_) | ty::Error(_) => None, diff --git a/src/test/ui/associated-types/hr-associated-type-bound-object.stderr b/src/test/ui/associated-types/hr-associated-type-bound-object.stderr index 354f5ae459729..7db9f501b22d6 100644 --- a/src/test/ui/associated-types/hr-associated-type-bound-object.stderr +++ b/src/test/ui/associated-types/hr-associated-type-bound-object.stderr @@ -5,7 +5,11 @@ LL | fn f<'a, T: X<'a> + ?Sized>(x: &>::U) { | ^^^^^ the trait `for<'b> Clone` is not implemented for `>::U` | = help: the following implementations were found: + <&T as Clone> + <*const T as Clone> + <*mut T as Clone> + and 1087 others note: required by a bound in `X` --> $DIR/hr-associated-type-bound-object.rs:3:33 | diff --git a/src/test/ui/issues/issue-38821.stderr b/src/test/ui/issues/issue-38821.stderr index cdf1f0dfc5361..a3bcbada8edca 100644 --- a/src/test/ui/issues/issue-38821.stderr +++ b/src/test/ui/issues/issue-38821.stderr @@ -4,11 +4,7 @@ error[E0277]: the trait bound `::SqlType: NotNull` is not sat LL | #[derive(Debug, Copy, Clone)] | ^^^^ the trait `NotNull` is not implemented for `::SqlType` | -note: required because of the requirements on the impl of `IntoNullable` for `::SqlType` - --> $DIR/issue-38821.rs:9:18 - | -LL | impl IntoNullable for T { - | ^^^^^^^^^^^^ ^ + = note: required because of the requirements on the impl of `IntoNullable` for `::SqlType` = note: this error originates in the derive macro `Copy` (in Nightly builds, run with -Z macro-backtrace for more info) help: consider further restricting the associated type |