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

Jump to bottom

simplify_type: don't simplify projections #92721

Closed
wants to merge 1 commit into from
Closed

simplify_type: don't simplify projections #92721

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
41 changes: 20 additions & 21 deletions compiler/rustc_middle/src/ty/fast_reject.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't really know what "inductive" type means here, and I expect other readers won't either =)

///
/// 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 `<T as Trait>::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 `<T as Trait>::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.
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if a better name for this would be TreatParamsAsPlaceholders-- I've been trying to push the "placeholder" terminology for "universally bound free variable".

Copy link
Contributor

Choose a reason for hiding this comment

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

Or something like

enum TreatParams {
    AsPlaceholderTypes,
    AsBoundTypes,
}

/// 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<T> 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<'_>,
Expand Down Expand Up @@ -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
Comment on lines +129 to +136
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// 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
// Returning a simplified for projections can end up being unsound. Consider this example:
//
// ```rust
// trait Foo { }
// impl Foo for u32 { }
//
// trait Bar { type Assoc; }
// impl Bar for f32 { type Assoc = u32; }
// impl Bar for f64 { type Assoc = u64; }
// ```
// If we are matching `<_ as Bar>::Assoc` against `Foo`, we might rule out the impl even though `_` could be inferred to `f32` and hence it might match.
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,
Expand Down
4 changes: 4 additions & 0 deletions src/test/ui/associated-types/hr-associated-type-bound-object.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,11 @@ LL | fn f<'a, T: X<'a> + ?Sized>(x: &<T as X<'a>>::U) {
| ^^^^^ the trait `for<'b> Clone` is not implemented for `<T as X<'b>>::U`
|
= help: the following implementations were found:
<! as Clone>
<&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
|
Expand Down
6 changes: 1 addition & 5 deletions src/test/ui/issues/issue-38821.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,7 @@ error[E0277]: the trait bound `<Col as Expression>::SqlType: NotNull` is not sat
LL | #[derive(Debug, Copy, Clone)]
| ^^^^ the trait `NotNull` is not implemented for `<Col as Expression>::SqlType`
|
note: required because of the requirements on the impl of `IntoNullable` for `<Col as Expression>::SqlType`
--> $DIR/issue-38821.rs:9:18
|
LL | impl<T: NotNull> IntoNullable for T {
| ^^^^^^^^^^^^ ^
= note: required because of the requirements on the impl of `IntoNullable` for `<Col as Expression>::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
|
Expand Down