Be more permissive with required bounds on existential types #57896
Conversation
rust-highfive
added
the
S-waiting-on-review
This comment has been minimized.
This comment has been minimized.
|
So... I was looking at the various examples and found one reason not to just outright treat any generic parameter equal to any other generic parameter, as long as all the other types match up. My considerations basically boil down to: if fn foo<T>() -> Foo<T, T> { ... }
fn bar<T, U>() -> Foo<T, U> { ... }
fn mep<U, T>() -> Foo<T, U> { ... }all are defining uses as long as the concrete type they return has the same structure. Then the following concrete return types are ok:
So if we chose the tuple variant: fn foo<T: Copy>(t: T) -> Foo<T, T> { (t, t) }
fn bar<T, U>(t: T, u: U) -> Foo<T, U> { (t, u) }
fn mep<U, T>(u: U, t: T) -> Foo<T, U> { (t, u) }Means that, assuming we could reveal existential types, the following would compile: let _: (&str, &str) = foo("foo"); // Foo<&str, &str>
let _: (&str, i32) = bar("", 42); // Foo<&str, i32>
let _: (&str, i32) = mep(42, ""); // Foo<&str, i32>
let _: (i32, &str) = bar(42, ""); // Foo<i32, &str>
let _: (i32, &str) = mep("", 42); // Foo<i32, &str>Now if we swapped the fn foo<T: Copy>(t: T) -> Foo<T, T> { (t, t) }
fn bar<T, U>(t: T, u: U) -> Foo<T, U> { (t, u) }
fn mep<U, T>(u: U, t: T) -> Foo<T, U> { (u, t) }We'd get let _: (&str, &str) = foo("foo"); // Foo<&str, &str>
let _: (&str, i32) = bar("", 42); // Foo<&str, i32>
let _: (i32, &str) = mep(42, ""); // Foo<&str, i32>
let _: (i32, &str) = bar(42, ""); // Foo<i32, &str>
let _: (&str, i32) = mep("", 42); // Foo<i32, &str>Which would mean that Which means we need to take the generic parameters from the existential type and not the function parameters, because the latter are irrelevant. For simplicity (and forward compat to a more lenient approach), I will make it so that fn foo<T>() -> Foo<T, T> { ... }
fn bar<T, U>() -> Foo<T, U> { ... }and fn foo<T>() -> Foo<T, T> { ... }
fn mep<U, T>() -> Foo<T, U> { ... }are forbidden, but fn bar<T, U>() -> Foo<T, U> { ... }
fn mep<U, T>() -> Foo<T, U> { ... }and fn bar<T, U>() -> Foo<T, U> { ... }
fn mep<T, U>() -> Foo<U, T> { ... }are allowed as long as there is an injective mapping from |
oli-obk
force-pushed
the
permissive_existence
branch
from
24e93ee to
a4cda21
Compare
|
r? @cramertj This PR is ready now |
This comment has been minimized.
This comment has been minimized.
oli-obk
force-pushed
the
permissive_existence
branch
from
a4cda21 to
f2241f6
Compare
|
Talked at the all hands-- I'm still of the opinion that we can't allow defining uses which don't keep both type parameters as fully universal: existential type X<A, B>;
fn foo<A>(a: A) -> X<A, u32> {
(a, 5u32)
}This could force the concrete type to be X<A,B> = (A, B), or it could be X<A, B> = (A, u32), we don't know which one it is (it could also be something like X<A, B> = (A, <A as SomeTrait>::Output)) |
|
So... we've come full circle. We are now again requiring defining uses to use all the generic parameters of the existential type in the concrete type. |
|
I'm a bit confused by the latest error. It's fine to not use the generic parameter in the concrete type, but each generic parameter has to be left fully variant in the type signature of the function that includes the defining use. The example you added unambiguously (afaict) does not use type |
… a generic existential type
oli-obk
force-pushed
the
permissive_existence
branch
from
f000382 to
eb98d31
Compare
|
Yea that was a brain-afk moment for me. I cleaned it up and it makes much more sense now (and still fixes the original issue that caused this PR) Right now we are erroring on all potential defining uses that end up not being a defining use. As a next step (separate from this PR), we can instead collect all the potential sites and add them as notes to the "no defining use" error. If a single defining use site exists, we don't need to emit an error for the potential use sites (except if their concrete types conflict with the defining use sites) |
|
Nice, this looks good to me! Thanks for the fixes :) @bors r+ |
|
📌 Commit eb98d31 has been approved by |
|
💔 Test failed - checks-travis |
bors
added
S-waiting-on-review
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
@bors retry timout |
bors
added
S-waiting-on-bors
|
💔 Test failed - checks-travis |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
bors
added
S-waiting-on-review
|
@bors retry 3 hour timeout 🤔 (caused by rebuilding docker images) |
bors
added
S-waiting-on-bors
|
☀️ Test successful - checks-travis, status-appveyor |
| // 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, |
There was a problem hiding this comment.
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:
- exact equality (which, with the right substitutions, can be reached for types coming from different contexts)
- unification (
ty::relateor full-on inference, which uses the former) - this is how e.g.implmatching 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.
| 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(); |
There was a problem hiding this comment.
Trying to understand what this does, we have these facts:
concrete_type, in the "generics domain" of the opaque typeconcrete_type.subst(self.tcx, substs), in the "generics domain" of the definersubstsare required to be a 1:1 mapping from the opaque type to the definer- this implies
index_mapis the exact inverse ofsubsts
- this implies
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).
| /// 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>, |
There was a problem hiding this comment.
Hang on, isn't substs checking handled by wfcheck?
rust/src/librustc_typeck/check/wfcheck.rs
Lines 818 to 837 in 5574b1d
Wouldn't that make ResolvedOpaqueTy and most of this PR entirely redundant, if params were checked there?
There was a problem hiding this comment.
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.
typeck/type_of: let wfcheck handle generics in opaque types' substs. I was working on rust-lang#70164, and `type_of`'s handling of opaque types seemed to be, by far, the trickiest use of `Ty::walk`, but I believe it wasn't doing anything (see rust-lang#57896 (comment) - I suspect, based on glancing at the PR discussion, that an early attempt was kept in, despite becoming just an overcomplicated way to do exactly the same as the previous simple type equality check). I would've loved to remove `ResolvedOpaqueTy` (keep the `Ty` and lose the `Substs`), but it looks like the MIR borrowck part of the process needs that now, so it would've been added anyway since rust-lang#57896, even if that PR hadn't happened. <hr/> In the process, I've moved the remaining substitution validation to `wfcheck`, which was already handling lifetimes, and kept only `delay_span_bug`s in `type_of`, as an insurance policy. I've added tests for lifetime and const cases, they seem to be checked correctly now. (and more uniform than they were in rust-lang#63063 (comment)) However, the quality of the errors is maybe a bit worse, and they don't trigger when there are other errors (not sure if this is due to compilation stop points or something more specific to one opaque type). r? @nikomatsakis cc @matthewjasper @oli-obk @Aaron1011
fixes #54184
r? @pnkfelix