Skip to content

Commit 13f7edc

Browse files
committed
don't simplify projections
1 parent d63a8d9 commit 13f7edc

File tree

3 files changed

+25
-26
lines changed

3 files changed

+25
-26
lines changed

compiler/rustc_middle/src/ty/fast_reject.rs

+20-21
Original file line numberDiff line numberDiff line change
@@ -60,29 +60,23 @@ pub enum StripReferences {
6060
No,
6161
}
6262

63-
/// Tries to simplify a type by only returning the outermost injective¹ layer, if one exists.
63+
/// Tries to simplify a type by only returning the outermost layer of an inductive type, if one exists.
6464
///
6565
/// The idea is to get something simple that we can use to quickly decide if two types could unify,
6666
/// for example during method lookup.
6767
///
68-
/// A special case here are parameters and projections. Projections can be normalized to
69-
/// a different type, meaning that `<T as Trait>::Assoc` and `u8` can be unified, even though
70-
/// their outermost layer is different while parameters like `T` of impls are later replaced
71-
/// with an inference variable, which then also allows unification with other types.
68+
/// We have to be careful with parameters and projections as we might want them to unify with other types.
69+
/// Projections can be normalized to a different type, meaning that `<T as Trait>::Assoc`
70+
/// and `u8` can be unified, even though their outermost layer is different while parameters like `T` of
71+
/// impls are later replaced with an inference variable, which then also allows unification with other types.
7272
///
73-
/// When using `SimplifyParams::Yes`, we still return a simplified type for params and projections²,
74-
/// the reasoning for this can be seen at the places doing this.
73+
/// When using `SimplifyParams::Yes`, we still return a simplified type for params.
74+
/// This is used during candidate selection when looking for impls satisfying `T: Trait`
75+
/// but must not be used on candidates, as the `T` in `impl<T> Trait for T` can unify
76+
/// with any other type.
7577
///
7678
/// For diagnostics we strip references with `StripReferences::Yes`. This is currently the best
7779
/// way to skip some unhelpful suggestions.
78-
///
79-
/// ¹ meaning that if two outermost layers are different, then the whole types are also different.
80-
/// ² FIXME(@lcnr): this seems like it can actually end up being unsound with the way it's used during
81-
/// candidate selection. We do not consider non blanket impls for `<_ as Trait>::Assoc` even
82-
/// though `_` can be inferred to a concrete type later at which point a concrete impl
83-
/// could actually apply. After experimenting for about an hour I wasn't able to cause any issues
84-
/// this way so I am not going to change this until we actually find an issue as I am really
85-
/// interesting in getting an actual test for this.
8680
pub fn simplify_type(
8781
tcx: TyCtxt<'_>,
8882
ty: Ty<'_>,
@@ -124,18 +118,23 @@ pub fn simplify_type(
124118
ty::Never => Some(NeverSimplifiedType),
125119
ty::Tuple(ref tys) => Some(TupleSimplifiedType(tys.len())),
126120
ty::FnPtr(ref f) => Some(FunctionSimplifiedType(f.skip_binder().inputs().len())),
127-
ty::Projection(_) | ty::Param(_) => {
121+
ty::Param(_) => {
128122
if can_simplify_params == SimplifyParams::Yes {
129-
// In normalized types, projections don't unify with
130-
// anything. when lazy normalization happens, this
131-
// will change. It would still be nice to have a way
132-
// to deal with known-not-to-unify-with-anything
133-
// projections (e.g., the likes of <__S as Encoder>::Error).
134123
Some(ParameterSimplifiedType)
135124
} else {
136125
None
137126
}
138127
}
128+
ty::Projection(_) => {
129+
// Returning a simplified for projections can end up being unsound as it would
130+
// stop us from considering not consider non blanket impls for `<_ as Trait>::Assoc` even
131+
// though `_` can be inferred to a concrete type later at which point a concrete impl
132+
// could actually apply.
133+
//
134+
// We could return `Some` here in cases where the input type is fully normalized, but
135+
// I (@lcnr) don't know how to guarantee that.
136+
None
137+
}
139138
ty::Opaque(def_id, _) => Some(OpaqueSimplifiedType(def_id)),
140139
ty::Foreign(def_id) => Some(ForeignSimplifiedType(def_id)),
141140
ty::Placeholder(..) | ty::Bound(..) | ty::Infer(_) | ty::Error(_) => None,

src/test/ui/associated-types/hr-associated-type-bound-object.stderr

+4
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,11 @@ LL | fn f<'a, T: X<'a> + ?Sized>(x: &<T as X<'a>>::U) {
55
| ^^^^^ the trait `for<'b> Clone` is not implemented for `<T as X<'b>>::U`
66
|
77
= help: the following implementations were found:
8+
<! as Clone>
89
<&T as Clone>
10+
<*const T as Clone>
11+
<*mut T as Clone>
12+
and 1087 others
913
note: required by a bound in `X`
1014
--> $DIR/hr-associated-type-bound-object.rs:3:33
1115
|

src/test/ui/issues/issue-38821.stderr

+1-5
Original file line numberDiff line numberDiff line change
@@ -4,11 +4,7 @@ error[E0277]: the trait bound `<Col as Expression>::SqlType: NotNull` is not sat
44
LL | #[derive(Debug, Copy, Clone)]
55
| ^^^^ the trait `NotNull` is not implemented for `<Col as Expression>::SqlType`
66
|
7-
note: required because of the requirements on the impl of `IntoNullable` for `<Col as Expression>::SqlType`
8-
--> $DIR/issue-38821.rs:9:18
9-
|
10-
LL | impl<T: NotNull> IntoNullable for T {
11-
| ^^^^^^^^^^^^ ^
7+
= note: required because of the requirements on the impl of `IntoNullable` for `<Col as Expression>::SqlType`
128
= note: this error originates in the derive macro `Copy` (in Nightly builds, run with -Z macro-backtrace for more info)
139
help: consider further restricting the associated type
1410
|

0 commit comments

Comments
 (0)