typeck/type_of: let wfcheck handle generics in opaque types' substs. #70272

eddyb · 2020-03-22T13:05:26Z

I was working on #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 #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 #57896, even if that PR hadn't happened.

In the process, I've moved the remaining substitution validation to wfcheck, which was already handling lifetimes, and kept only delay_span_bugs 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 #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

nikomatsakis

started reading but ran out of time. =) will return. initial comment.

nikomatsakis · 2020-03-23T15:01:58Z

src/librustc_typeck/collect/type_of.rs

-                for (idx, subst) in substs.iter().enumerate() {
-                    if let GenericArgKind::Type(ty) = subst.unpack() {
+
+                let opaque_generics = self.tcx.generics_of(self.def_id);


Pre-existing, but this code is in dire need of some comments.

To start, can we add a comment explaining what is happening here -- I think it is this:

Examine the type parameters to the opaque type used in the defining use and make sure that (a) they are only generic parameters from the enclosing scope and (b) each generic parameter is used at most once. For example, given this function:

type Foo<A, B> = impl Debug; fn f1<T, U>() -> Foo<T, U>

we would be iterating over the substs T, U and checking that they are distinct. We would error for things like -> Foo<T, T> or -> Foo<u32, U>.

wfcheck is the primary checker and has a nice comment.
This is just a tombstone insurance policy. But I can try to document it if you want.

I'd still like a comment, even if only points over to the "primary comment"

In fact, I think pointing out that this is "just" an insurance policy would be quite useful =)

nikomatsakis

r=me with nits addressed (I think just a comment)

nikomatsakis · 2020-04-03T19:35:07Z

src/test/ui/type-alias-impl-trait/generic_duplicate_param_use2.stderr

@@ -7,5 +7,23 @@ LL | |     t
 LL | | }
   | |_^

-error: aborting due to previous error
+error: concrete type differs from previous defining opaque type use


This does seem like more confusing output than the old code...

The difference is when the checks run and which "uses" (mentions, really) are considered "defining".
From the looks of the code, "defining" vs not is an unimplemented part of the feature.
At least now check::wfcheck and type_of are each more uniform and it should be easier to finish the feature, but I'd rather leave that to someone else.

nikomatsakis · 2020-04-03T19:37:50Z

src/test/ui/type-alias-impl-trait/generic_nondefining_use.stderr

@@ -4,13 +4,17 @@ error: at least one trait must be specified
 LL | type Cmp<T> = impl 'static;
   |               ^^^^^^^^^^^^

-error: defining opaque type use does not fully define opaque type: generic parameter `T` is specified as concrete type `u32`
-  --> $DIR/generic_nondefining_use.rs:10:1
+error: non-defining opaque type use in defining scope


This error message seems pretty confusing to me. But I guess the old one was pretty bad too so I don't think it needs to block this PR...

I used the error that used to be only emitted for lifetimes, also for types and consts.

I don't like either error but I didn't want to make a third variant of my own.

nikomatsakis · 2020-04-03T19:44:57Z

src/librustc_typeck/collect/type_of.rs

-                        GenericArgKind::Lifetime(lt) => !matches!(lt, ty::ReStatic),
+                        GenericArgKind::Lifetime(lt) => {
+                            matches!(lt, ty::ReEarlyBound(_) | ty::ReFree(_))
+                        }


I guess the other types just didn't happen in practice? Else it'd be nice to have a test.

These are "exported" by the MIR borrowck (cc @matthewjasper) AFAICT, and that results in late-bound lifetimes (in the fn signature, I presume) being ReFree.

This list is minimal, as in I can't remove either ReEarlyBound nor ReFree without tests failing, and adding more would cause less errors (i.e. allowing buggy situations), not more.

Actually, @matthewjasper, shouldn't ReFree turn back into ReLateBound? Although, I guess the actual concrete type is in terms of the type Foo<...> = impl Trait; generics, so it's not affected by the substitutions back at the point Foo is mentioned.

I don't think that we currently ever go from a placeholder back to a bound region/type.

…ning uses.

…bsts.

…' substs.

…parameters.

eddyb · 2020-04-04T05:31:22Z

src/librustc_typeck/collect/type_of.rs

+                // HACK(eddyb) this check shouldn't be needed, as `wfcheck`
+                // performs the same checks, in theory, but I've kept it here
+                // using `delay_span_bug`, just in case `wfcheck` slips up.


@nikomatsakis Hang on, did you see this, or not? GitHub doesn't show it on your original comment, maybe I added it afterwards?

Hmm, I haven't pushed since, maybe you left the comment on a commit before I had added the comment?

eddyb · 2020-04-04T05:57:24Z

@bors r=nikomatsakis

bors · 2020-04-04T05:57:25Z

📌 Commit 8ad149a has been approved by nikomatsakis

bors · 2020-04-04T09:16:25Z

⌛ Testing commit 8ad149a with merge 49dc2f9...

bors · 2020-04-04T12:18:13Z

☀️ Test successful - checks-azure
Approved by: nikomatsakis
Pushing 49dc2f9 to master...

rust-highfive assigned nikomatsakis Mar 22, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 22, 2020

This was referenced Mar 22, 2020

Be more permissive with required bounds on existential types #57896

Merged

Tracking issue for RFC 2515, "Permit impl Trait in type aliases" #63063

Open

ty/walk: iterate GenericArgs instead of Tys. #70164

Merged

Centril added the F-type_alias_impl_trait `#[feature(type_alias_impl_trait)]` label Mar 23, 2020

nikomatsakis reviewed Mar 23, 2020

View reviewed changes

This comment has been minimized.

Sign in to view

nikomatsakis approved these changes Apr 3, 2020

View reviewed changes

nikomatsakis added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 3, 2020

eddyb added 6 commits April 4, 2020 06:49

typeck/type_of: simplify checking of opaque types with multipler defi…

6465113

…ning uses.

typeck/type_of: don't ignore incorrect defining uses of opaque types.

a98b534

typeck/type_of: let wfcheck handle concrete types in opaque types' su…

8e9a5d9

…bsts.

typeck/type_of: let wfcheck handle duplicate generics in opaque types…

8807b00

…' substs.

tests: add tests for lifetime and const params of opaque types.

ea7999b

typeck/type_of: only early-bound and (free) late-bound lifetimes are …

8ad149a

…parameters.

eddyb commented Apr 4, 2020

View reviewed changes

eddyb force-pushed the type-of-impl-trait branch from e06d4c0 to 8ad149a Compare April 4, 2020 05:56

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 4, 2020

bors added the merged-by-bors This PR was explicitly merged by bors. label Apr 4, 2020

bors merged commit 49dc2f9 into rust-lang:master Apr 4, 2020

eddyb deleted the type-of-impl-trait branch April 4, 2020 14:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

typeck/type_of: let wfcheck handle generics in opaque types' substs. #70272

typeck/type_of: let wfcheck handle generics in opaque types' substs. #70272

eddyb commented Mar 22, 2020

nikomatsakis left a comment

nikomatsakis Mar 23, 2020

eddyb Mar 23, 2020

nikomatsakis Apr 3, 2020

nikomatsakis Apr 3, 2020

This comment has been minimized.

nikomatsakis left a comment

nikomatsakis Apr 3, 2020

eddyb Apr 3, 2020

nikomatsakis Apr 3, 2020

eddyb Apr 3, 2020

nikomatsakis Apr 3, 2020

eddyb Apr 3, 2020

eddyb Apr 5, 2020

matthewjasper Apr 5, 2020

eddyb Apr 4, 2020

eddyb Apr 4, 2020

eddyb commented Apr 4, 2020

bors commented Apr 4, 2020

bors commented Apr 4, 2020

bors commented Apr 4, 2020

typeck/type_of: let wfcheck handle generics in opaque types' substs. #70272

typeck/type_of: let wfcheck handle generics in opaque types' substs. #70272

Conversation

eddyb commented Mar 22, 2020

nikomatsakis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

nikomatsakis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eddyb commented Apr 4, 2020

bors commented Apr 4, 2020

bors commented Apr 4, 2020

bors commented Apr 4, 2020