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.

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

Be more permissive with required bounds on existential types #57896

Merged
merged 10 commits into from
Feb 19, 2019
5 changes: 5 additions & 0 deletions src/librustc/ich/impls_ty.rs
Original file line number Diff line number Diff line change
Expand Up @@ -233,6 +233,11 @@ impl_stable_hash_for!(struct ty::FnSig<'tcx> {
abi
});

impl_stable_hash_for!(struct ty::ResolvedOpaqueTy<'tcx> {
concrete_type,
substs
});

impl<'a, 'gcx, T> HashStable<StableHashingContext<'a>> for ty::Binder<T>
where T: HashStable<StableHashingContext<'a>>
{
Expand Down
2 changes: 1 addition & 1 deletion src/librustc/infer/opaque_types/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ pub struct OpaqueTypeDecl<'tcx> {
///
/// winds up desugared to:
///
/// abstract type Foo<'x, T>: Trait<'x>
/// abstract type Foo<'x, X>: Trait<'x>
/// fn foo<'a, 'b, T>() -> Foo<'a, T>
///
/// then `substs` would be `['a, T]`.
Expand Down
13 changes: 12 additions & 1 deletion src/librustc/ty/context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -315,6 +315,17 @@ impl<'a, V> LocalTableInContextMut<'a, V> {
}
}

/// All information necessary to validate and reveal an `impl Trait` or `existential Type`
#[derive(RustcEncodable, RustcDecodable, Debug)]
pub struct ResolvedOpaqueTy<'tcx> {
/// The revealed type as seen by this function.
pub concrete_type: Ty<'tcx>,
/// Generic parameters on the opaque type as passed by this function.
/// For `existential type Foo<A, B>; fn foo<T, U>() -> Foo<T, U> { .. }` this is `[T, U]`, not
/// `[A, B]`
pub substs: &'tcx Substs<'tcx>,
Comment on lines +323 to +326
Copy link
Member

Choose a reason for hiding this comment

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

Hang on, isn't substs checking handled by wfcheck?

/// Checks "defining uses" of opaque `impl Trait` types to ensure that they meet the restrictions
/// laid for "higher-order pattern unification".
/// This ensures that inference is tractable.
/// In particular, definitions of opaque types can only use other generics as arguments,
/// and they cannot repeat an argument. Example:
///
/// ```rust
/// type Foo<A, B> = impl Bar<A, B>;
///
/// // Okay -- `Foo` is applied to two distinct, generic types.
/// fn a<T, U>() -> Foo<T, U> { .. }
///
/// // Not okay -- `Foo` is applied to `T` twice.
/// fn b<T>() -> Foo<T, T> { .. }
///
/// // Not okay -- `Foo` is applied to a non-generic type.
/// fn b<T>() -> Foo<T, u32> { .. }
/// ```
///
fn check_opaque_types<'fcx, 'tcx>(

Wouldn't that make ResolvedOpaqueTy and most of this PR entirely redundant, if params were checked there?

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, I can't remove ResolvedOpaqueTy as it seems MIR borrowck needs the Substs, and I'd rather not touch that code if I can help it.

}

#[derive(RustcEncodable, RustcDecodable, Debug)]
pub struct TypeckTables<'tcx> {
/// The HirId::owner all ItemLocalIds in this table are relative to.
Expand Down Expand Up @@ -417,7 +428,7 @@ pub struct TypeckTables<'tcx> {

/// All the existential types that are restricted to concrete types
/// by this function
pub concrete_existential_types: FxHashMap<DefId, Ty<'tcx>>,
pub concrete_existential_types: FxHashMap<DefId, ResolvedOpaqueTy<'tcx>>,

/// Given the closure ID this map provides the list of UpvarIDs used by it.
/// The upvarID contains the HIR node ID and it also contains the full path
Expand Down
2 changes: 1 addition & 1 deletion src/librustc/ty/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ pub use self::context::{TyCtxt, FreeRegionInfo, GlobalArenas, AllArenas, tls, ke
pub use self::context::{Lift, TypeckTables, CtxtInterners};
pub use self::context::{
UserTypeAnnotationIndex, UserType, CanonicalUserType,
CanonicalUserTypeAnnotation, CanonicalUserTypeAnnotations,
CanonicalUserTypeAnnotation, CanonicalUserTypeAnnotations, ResolvedOpaqueTy,
};

pub use self::instance::{Instance, InstanceDef};
Expand Down
3 changes: 0 additions & 3 deletions src/librustc_typeck/check/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1385,10 +1385,7 @@ pub fn check_item_type<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>, it: &'tcx hir::Ite
}
hir::ItemKind::Existential(..) => {
let def_id = tcx.hir().local_def_id(it.id);
let pty_ty = tcx.type_of(def_id);
let generics = tcx.generics_of(def_id);

check_bounds_are_used(tcx, &generics, pty_ty);
let substs = Substs::identity_for_item(tcx, def_id);
check_opaque(tcx, def_id, substs, it.span);
}
Expand Down
14 changes: 11 additions & 3 deletions src/librustc_typeck/check/writeback.rs
Original file line number Diff line number Diff line change
Expand Up @@ -561,21 +561,29 @@ impl<'cx, 'gcx, 'tcx> WritebackCx<'cx, 'gcx, 'tcx> {
if def_id == defin_ty_def_id {
// Concrete type resolved to the existential type itself
// Force a cycle error
// FIXME(oli-obk): we could just not insert it into `concrete_existential_types`
// which simply would make this use not a defining use.
self.tcx().at(span).type_of(defin_ty_def_id);
}
}

let new = ty::ResolvedOpaqueTy {
concrete_type: definition_ty,
substs: self.tcx().lift_to_global(&opaque_defn.substs).unwrap(),
};

let old = self.tables
.concrete_existential_types
.insert(def_id, definition_ty);
.insert(def_id, new);
if let Some(old) = old {
if old != definition_ty {
if old.concrete_type != definition_ty || old.substs != opaque_defn.substs {
span_bug!(
span,
"visit_opaque_types tried to write \
different types for the same existential type: {:?}, {:?}, {:?}",
different types for the same existential type: {:?}, {:?}, {:?}, {:?}",
def_id,
definition_ty,
opaque_defn,
old,
);
}
Expand Down
112 changes: 101 additions & 11 deletions src/librustc_typeck/collect.rs
Original file line number Diff line number Diff line change
Expand Up @@ -23,9 +23,10 @@ use middle::resolve_lifetime as rl;
use middle::weak_lang_items;
use rustc::mir::mono::Linkage;
use rustc::ty::query::Providers;
use rustc::ty::subst::Substs;
use rustc::ty::subst::{Subst, Substs};
use rustc::ty::util::Discr;
use rustc::ty::util::IntTypeExt;
use rustc::ty::subst::UnpackedKind;
use rustc::ty::{self, AdtKind, ToPolyTraitRef, Ty, TyCtxt};
use rustc::ty::{ReprOptions, ToPredicate};
use rustc::util::captures::Captures;
Expand Down Expand Up @@ -1193,7 +1194,7 @@ fn type_of<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>, def_id: DefId) -> Ty<'tcx> {
tcx.typeck_tables_of(owner)
.concrete_existential_types
.get(&def_id)
.cloned()
.map(|opaque| opaque.concrete_type)
.unwrap_or_else(|| {
// This can occur if some error in the
// owner fn prevented us from populating
Expand Down Expand Up @@ -1325,7 +1326,13 @@ fn find_existential_constraints<'a, 'tcx>(
struct ConstraintLocator<'a, 'tcx: 'a> {
tcx: TyCtxt<'a, 'tcx, 'tcx>,
def_id: DefId,
found: Option<(Span, ty::Ty<'tcx>)>,
// First found type span, actual type, mapping from the existential type's generic
// parameters to the concrete type's generic parameters
//
// The mapping is an index for each use site of a generic parameter in the concrete type
//
// The indices index into the generic parameters on the existential type.
found: Option<(Span, ty::Ty<'tcx>, Vec<usize>)>,
}

impl<'a, 'tcx> ConstraintLocator<'a, 'tcx> {
Expand All @@ -1340,23 +1347,106 @@ fn find_existential_constraints<'a, 'tcx>(
.tcx
.typeck_tables_of(def_id)
.concrete_existential_types
.get(&self.def_id)
.cloned();
if let Some(ty) = ty {
.get(&self.def_id);
if let Some(ty::ResolvedOpaqueTy { concrete_type, substs }) = ty {
// FIXME(oli-obk): trace the actual span from inference to improve errors
let span = self.tcx.def_span(def_id);
if let Some((prev_span, prev_ty)) = self.found {
if ty != prev_ty {
// used to quickly look up the position of a generic parameter
let mut index_map: FxHashMap<ty::ParamTy, usize> = FxHashMap::default();
// skip binder is ok, since we only use this to find generic parameters and their
// positions.
for (idx, subst) in substs.iter().enumerate() {
if let UnpackedKind::Type(ty) = subst.unpack() {
if let ty::Param(p) = ty.sty {
if index_map.insert(p, idx).is_some() {
// there was already an entry for `p`, meaning a generic parameter
// was used twice
self.tcx.sess.span_err(
span,
&format!("defining existential type use restricts existential \
type by using the generic parameter `{}` twice", p.name),
);
return;
}
} else {
self.tcx.sess.delay_span_bug(
span,
&format!(
"non-defining exist ty use in defining scope: {:?}, {:?}",
concrete_type, substs,
),
);
}
}
}
// compute the index within the existential type for each generic parameter used in
// the concrete type
let indices = concrete_type
.subst(self.tcx, substs)
.walk()
.filter_map(|t| match &t.sty {
ty::Param(p) => Some(*index_map.get(p).unwrap()),
_ => None,
}).collect();
Comment on lines +1384 to +1390
Copy link
Member

@eddyb eddyb Mar 22, 2020

Choose a reason for hiding this comment

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

Trying to understand what this does, we have these facts:

  1. concrete_type, in the "generics domain" of the opaque type
  2. concrete_type.subst(self.tcx, substs), in the "generics domain" of the definer
  3. substs are required to be a 1:1 mapping from the opaque type to the definer
    • this implies index_map is the exact inverse of substs

All that means indices could be computed so:

                let indices = concrete_type
                    .walk()
                    .filter_map(|t| match &t.sty {
                        ty::Param(p) => Some(p.index),
                        _ => None,
                    }).collect();

If indices have to agree between definers, that means concrete_types are fully equal.

Am I missing something? This entire PR, other than checking substs, looks like a noop.

I'll attempt a revert of the parts that look like a noop, and generalize to handle const generics (if this is even the right place to do so).

Copy link
Member

Choose a reason for hiding this comment

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

Opened #70272.

let is_param = |ty: ty::Ty| match ty.sty {
ty::Param(_) => true,
_ => false,
};
if !substs.types().all(is_param) {
self.tcx.sess.span_err(
span,
"defining existential type use does not fully define existential type",
);
} else if let Some((prev_span, prev_ty, ref prev_indices)) = self.found {
let mut ty = concrete_type.walk().fuse();
let mut p_ty = prev_ty.walk().fuse();
let iter_eq = (&mut ty).zip(&mut p_ty).all(|(t, p)| match (&t.sty, &p.sty) {
// type parameters are equal to any other type parameter for the purpose of
// concrete type equality, as it is possible to obtain the same type just
// by passing matching parameters to a function.
(ty::Param(_), ty::Param(_)) => true,
_ => t == p,
Comment on lines +1404 to +1408
Copy link
Member

Choose a reason for hiding this comment

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

I'm suspicious of this entire approach. Only the outermost type could be a differing ty::Params, anything containing ty::Param would automatically hit t == p being false.

There are two ways to compare types deeply, and they're:

  1. exact equality (which, with the right substitutions, can be reached for types coming from different contexts)
  2. unification (ty::relate or full-on inference, which uses the former) - this is how e.g. impl matching works in the current trait system

"Walking" a type is a not a good way to achieve almost anything, the main legitimate usecase I've found is gathering leaves of some kind (e.g. ty::Infer(_)) or searching for a specific type.

});
if !iter_eq || ty.next().is_some() || p_ty.next().is_some() {
// found different concrete types for the existential type
let mut err = self.tcx.sess.struct_span_err(
span,
"defining existential type use differs from previous",
"concrete type differs from previous defining existential type use",
);
err.span_label(
span,
format!("expected `{}`, got `{}`", prev_ty, concrete_type),
);
err.span_note(prev_span, "previous use here");
err.emit();
} else if indices != *prev_indices {
// found "same" concrete types, but the generic parameter order differs
let mut err = self.tcx.sess.struct_span_err(
span,
"concrete type's generic parameters differ from previous defining use",
);
use std::fmt::Write;
let mut s = String::new();
write!(s, "expected [").unwrap();
let list = |s: &mut String, indices: &Vec<usize>| {
let mut indices = indices.iter().cloned();
if let Some(first) = indices.next() {
write!(s, "`{}`", substs[first]).unwrap();
for i in indices {
write!(s, ", `{}`", substs[i]).unwrap();
}
}
};
list(&mut s, prev_indices);
write!(s, "], got [").unwrap();
list(&mut s, &indices);
write!(s, "]").unwrap();
err.span_label(span, s);
err.span_note(prev_span, "previous use here");
err.emit();
}
} else {
self.found = Some((span, ty));
self.found = Some((span, concrete_type, indices));
}
}
}
Expand Down Expand Up @@ -1415,7 +1505,7 @@ fn find_existential_constraints<'a, 'tcx>(
}

match locator.found {
Some((_, ty)) => ty,
Some((_, ty, _)) => ty,
None => {
let span = tcx.def_span(def_id);
tcx.sess.span_err(span, "could not find defining uses");
Expand Down
3 changes: 2 additions & 1 deletion src/test/ui/existential_types/bound_reduction2.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,11 +8,12 @@ trait TraitWithAssoc {
}

existential type Foo<V>: Trait<V>;
//~^ ERROR could not find defining uses

trait Trait<U> {}

impl<W> Trait<W> for () {}

fn foo_desugared<T: TraitWithAssoc>(_: T) -> Foo<T::Assoc> { //~ ERROR non-defining
fn foo_desugared<T: TraitWithAssoc>(_: T) -> Foo<T::Assoc> { //~ ERROR does not fully define
()
}
16 changes: 8 additions & 8 deletions src/test/ui/existential_types/bound_reduction2.stderr
Original file line number Diff line number Diff line change
@@ -1,16 +1,16 @@
error: non-defining existential type use in defining scope
--> $DIR/bound_reduction2.rs:16:1
error: defining existential type use does not fully define existential type
--> $DIR/bound_reduction2.rs:17:1
|
LL | / fn foo_desugared<T: TraitWithAssoc>(_: T) -> Foo<T::Assoc> { //~ ERROR non-defining
LL | / fn foo_desugared<T: TraitWithAssoc>(_: T) -> Foo<T::Assoc> { //~ ERROR does not fully define
LL | | ()
LL | | }
| |_^
|
note: used non-generic type <T as TraitWithAssoc>::Assoc for generic parameter
--> $DIR/bound_reduction2.rs:10:22

error: could not find defining uses
--> $DIR/bound_reduction2.rs:10:1
|
LL | existential type Foo<V>: Trait<V>;
| ^
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

error: aborting due to previous error
error: aborting due to 2 previous errors

2 changes: 1 addition & 1 deletion src/test/ui/existential_types/different_defining_uses.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,6 @@ fn foo() -> Foo {
""
}

fn bar() -> Foo { //~ ERROR defining existential type use differs from previous
fn bar() -> Foo { //~ ERROR concrete type differs from previous
42i32
}
6 changes: 3 additions & 3 deletions src/test/ui/existential_types/different_defining_uses.stderr
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
error: defining existential type use differs from previous
error: concrete type differs from previous defining existential type use
--> $DIR/different_defining_uses.rs:12:1
|
LL | / fn bar() -> Foo { //~ ERROR defining existential type use differs from previous
LL | / fn bar() -> Foo { //~ ERROR concrete type differs from previous
LL | | 42i32
LL | | }
| |_^
| |_^ expected `&'static str`, got `i32`
|
note: previous use here
--> $DIR/different_defining_uses.rs:8:1
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,10 +9,10 @@ fn foo() -> Foo {
""
}

fn bar() -> Foo { //~ ERROR defining existential type use differs from previous
fn bar() -> Foo { //~ ERROR concrete type differs from previous
panic!()
}

fn boo() -> Foo { //~ ERROR defining existential type use differs from previous
fn boo() -> Foo { //~ ERROR concrete type differs from previous
loop {}
}
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
error: defining existential type use differs from previous
error: concrete type differs from previous defining existential type use
--> $DIR/different_defining_uses_never_type.rs:12:1
|
LL | / fn bar() -> Foo { //~ ERROR defining existential type use differs from previous
LL | / fn bar() -> Foo { //~ ERROR concrete type differs from previous
LL | | panic!()
LL | | }
| |_^
| |_^ expected `&'static str`, got `()`
|
note: previous use here
--> $DIR/different_defining_uses_never_type.rs:8:1
Expand All @@ -14,13 +14,13 @@ LL | | ""
LL | | }
| |_^

error: defining existential type use differs from previous
error: concrete type differs from previous defining existential type use
--> $DIR/different_defining_uses_never_type.rs:16:1
|
LL | / fn boo() -> Foo { //~ ERROR defining existential type use differs from previous
LL | / fn boo() -> Foo { //~ ERROR concrete type differs from previous
LL | | loop {}
LL | | }
| |_^
| |_^ expected `&'static str`, got `()`
|
note: previous use here
--> $DIR/different_defining_uses_never_type.rs:8:1
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,6 @@ fn my_iter<T>(t: T) -> MyIter<T> {
std::iter::once(t)
}

fn my_iter2<T>(t: T) -> MyIter<T> { //~ ERROR defining existential type use differs from previous
fn my_iter2<T>(t: T) -> MyIter<T> { //~ ERROR concrete type differs from previous
Some(t).into_iter()
}
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
error: defining existential type use differs from previous
error: concrete type differs from previous defining existential type use
--> $DIR/generic_different_defining_uses.rs:11:1
|
LL | / fn my_iter2<T>(t: T) -> MyIter<T> { //~ ERROR defining existential type use differs from previous
LL | / fn my_iter2<T>(t: T) -> MyIter<T> { //~ ERROR concrete type differs from previous
LL | | Some(t).into_iter()
LL | | }
| |_^
| |_^ expected `std::iter::Once<T>`, got `std::option::IntoIter<T>`
|
note: previous use here
--> $DIR/generic_different_defining_uses.rs:7:1
Expand Down
Loading