Implement existential types #52024

oli-obk · 2018-07-03T17:46:51Z

(not for associated types yet)

cramertj · 2018-07-03T18:09:42Z

src/librustc_typeck/collect.rs

+                let span = self.tcx.def_span(def_id);
+                if let Some((prev_span, prev_ty)) = self.found {
+                    if ty != prev_ty {
+                        // found different concrete types for the existential type


This won't work for generic existential types because they might be applied with different parameters, right? Is the plan to relax this constraint in the future?

My brain broke when I tried to think about that topic, so I left the problem for tomorrow (literally, not far future). As long as there is only a single defining use, you can use generics already in this PR.

cramertj · 2018-07-03T18:17:52Z

src/test/run-pass/existential_type.rs

+// generics
+existential type Cmp<T>: std::cmp::PartialEq<T> + std::fmt::Debug;
+
+fn cmp() -> Cmp<u32> {


This should be an error according to the RFC. In order for type parameters to be applied correctly (and fully generally) there needs to be at least one definition site that leaves the parameters generic / separate.

e.g.

existential type Cmp<A, B>; struct Bar<A, B>(A, B); fn foo() -> Cmp<u32, u32> { Bar(5u32, 6u32) }

What is the definition of Cmp? It might seem like it is type Cmp<A, B> = Bar<A, B>;, but type Cmp<A, B> = Cmp<B, A>; might also be a valid choice.

In this example (fn cmp() -> Cmp<u32>) the mapping could be type Cmp<A> = A;, but it could also be type Cmp<A> = <A as TraitThatAlwaysOutputsU32>::u32;, right?

Makes sense. This case shouldn't be hard to catch (instead of being an error, this case would be a non-defining use, right?)

@oli-obk Yes, although it still has to be unified with whatever the "fully defining" usage is to make sure that it can work.

cramertj · 2018-07-03T18:21:52Z

This is awesome!

oli-obk · 2018-07-04T15:46:04Z

The following cases do not error yet, but should

existential type Cmp<T>: std::cmp::PartialEq<T>;
//~^ ERROR could not find defining uses

// not a defining use, because it doesn't define *all* possible generics
fn cmp() -> Cmp<u32> {
    5u32 //~ mismatched types
}

existential type Cmp2<T>: std::cmp::PartialEq<T>;
//~^ ERROR could not find defining uses

// not a defining use, because it doesn't define *all* possible generics
// it only defines `Cmp2` for `T: SomeBounds`
fn cmp2<T: std::cmp::PartialEq<T>>(t: T) -> Cmp2<T> {
    t
}

alexreg · 2018-07-05T15:06:55Z

Are you going to follow up with associated types in a separate PR, @oli-obk?

oli-obk · 2018-07-05T15:13:50Z

Yea, I want to get the "simple" case working first. I'm slowly getting the hang of generics.

alexreg · 2018-07-09T00:20:24Z

@oli-obk Waiting for review before you fix the above errors? :-)

oli-obk · 2018-07-09T07:54:17Z

I'm working on it

oli-obk · 2018-07-16T08:53:23Z

So uhm, in other news

trait Bar {}
struct Dummy;
impl Bar for Dummy {}

trait Foo {
    type Assoc: Bar;
    fn foo() -> Self::Assoc;
    fn bar() -> Self::Assoc;
}

existential type Helper: Bar;

impl Foo for i32 {
    type Assoc = Helper;
    fn foo() -> Helper {
        Dummy
    }
    fn bar() -> Helper {
        Dummy
    }
}

fn main() {}

works out of the box

nikomatsakis

Sorry for being slow. This is so awesome. Left a bunch of minor comments below.

nikomatsakis · 2018-07-16T13:27:35Z

src/librustc/infer/anon_types/mod.rs

@@ -691,10 +691,22 @@ impl<'a, 'gcx, 'tcx> Instantiator<'a, 'gcx, 'tcx> {
                    // ```
                    if let Some(anon_node_id) = tcx.hir.as_local_node_id(def_id) {
                        let anon_parent_def_id = match tcx.hir.expect_item(anon_node_id).node {


This code feels kind of crazy convoluted now. In particular, two of the arms propagate back a def-id to be compared with self.parent_def_id, but one of them just does the comparison itself. How about this instead:

let in_definition_scope = match tcx.hir.expect_item(anon_node_id).node { hir::ItemExistential(hir::ExistTy { impl_trait_fn: Some(parent), .. }) => self.parent_def_id == parent, hir::ItemExistential(hir::ExistTy { impl_trait_fn: None, .. }) => may_define_existential_type(...), _ => { let anon_parent_node_id = tcx.hir.get_parent(anon_node_id); self.parent_def_id == tcx.hir.local_def_id(anon_parent_node_id) } }; if in_definition_scope { return self.fold_anon_ty(ty, def_id, substs); }

nikomatsakis · 2018-07-16T13:36:17Z

src/librustc/infer/anon_types/mod.rs

@@ -778,3 +806,25 @@ impl<'a, 'gcx, 'tcx> Instantiator<'a, 'gcx, 'tcx> {
        ty_var
    }
 }
+
+/// Whether `anon_node_id` is a sibling or a child of a sibling of `def_id`
+pub fn may_define_existential_type(


Personally I would have appreciated a longer comment with a quick code example here. e.g

pub mod foo { pub mod bar { pub existential type Baz; fn f1() -> Baz { .. } } fn f2() -> bar::Baz { .. } }

Here, def_id will be the def_id of the existential type Baz. anon_node_id is the node-id of the reference to Baz -- so either the return type of the function f1 or f2. We will return true if the reference is within the same module as the existential type -- so true for f1, false for f2.

nikomatsakis · 2018-07-16T14:16:57Z

src/librustc/ty/mod.rs

@@ -2857,8 +2863,15 @@ fn param_env<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>,

    // The param_env of an existential type is its parent's param_env


Can we update this comment? This is specific to impl Trait

nikomatsakis · 2018-07-16T14:17:36Z

src/librustc_save_analysis/dump_visitor.rs

+                // 'def' and the name here should be a ref to the def in the
+                // trait.
+                for bound in bounds.iter() {
+                    if let ast::GenericBound::Trait(ref trait_ref, _) = *bound {


Nit: why not get rid of the * and the ref =)

we should make a clippy lint for that

nikomatsakis · 2018-07-16T14:20:58Z

src/librustc_typeck/check/wfcheck.rs

@@ -209,6 +210,10 @@ fn check_associated_item<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>,
                    fcx.register_wf_obligation(ty, span, code.clone());
                }
            }
+            ty::AssociatedKind::Existential => {


Hmm. I... don't think this concept even makes sense, does it? I would rather expect that a existential type in an impl is more like -> impl Trait in a function -- it desugars to the pair of an existential type that is scoped to the impl and defined within. On the trait side, this is just a plain associated type.

yea, I was just following the RFC, but all of the associated existential code is stubbed out anyway. I'll remove it from this PR entirely

yes; my point is that even if we allow the syntax I wouldn't necessarily expect it to show up at the ty level (though perhaps it will wind up being necessary for...reasons)

nikomatsakis · 2018-07-16T14:33:10Z

src/test/ui/existential_type2.rs

+existential type Underconstrained<T: std::fmt::Debug>: 'static;
+//~^ ERROR `U` doesn't implement `std::fmt::Debug`
+
+// not a defining use, because it doesn't define *all* possible generics


The span of the error here is pretty odd, no? I think we talked about this -- I was expecting to see errors arise from the WF checks on this function. I guess perhaps the WF definition for an existential type is not quite right...

Yep, this code doesn't look right:

rust/src/librustc/ty/wf.rs

Lines 363 to 367 in 3d5753f

ty::TyAnon(..) => {

// all of the requirements on type parameters

// should've been checked by the instantiation

// of whatever returned this exact `impl Trait`.

}

We probably want to rewrite this to basically do the same as structs/enums:

rust/src/librustc/ty/wf.rs

Lines 292 to 296 in 3d5753f

ty::TyAdt(def, substs) => {

// WfNominalType

let obligations = self.nominal_obligations(def.did, substs);

self.out.extend(obligations);

}

We might want to only do that for the case of explicit existential type declarations -- though I imagine it will be fine either way.

Yea we need to skip it for impl Trait, otherwise https://github.com/rust-lang/rust/blob/master/src/test/ui/nll/ty-outlives/impl-trait-outlives.rs passes without errors

Also

existential type WrongGeneric<T>: 'static; //~^ ERROR the parameter type `T` may not live long enough fn wrong_generic<T>(t: T) -> WrongGeneric<T> { t }

stops being an error with NLL if I do that change.

nikomatsakis · 2018-07-16T14:33:37Z

src/test/ui/existential_type.rs

+fn main() {}
+
+// two definitions with different types
+existential type Foo: std::fmt::Debug;


PS I am resisting complaining about this syntax so much 😛

nikomatsakis · 2018-07-16T14:34:05Z

src/test/ui/existential_type.rs

+    42i32
+}
+
+// declared but never defined


I think I would rather see these as distinct tests (with suggestive filenames like src/test/ui/existential_type/declared_but_never_defined.rs). These "mega tests" files always make it kind of hard for me to sort of which parts belong to which test.

nikomatsakis · 2018-07-16T14:34:49Z

src/test/ui/existential_type.rs

@@ -0,0 +1,95 @@
+// Copyright 2018 The Rust Project Developers. See the COPYRIGHT


please put these tests into src/test/ui/existential_type/XXX.rs files

nikomatsakis · 2018-07-16T14:35:56Z

src/test/ui/existential_type.rs

+}
+
+fn bomp() -> boo::Boo {
+    "" //~ ERROR mismatched types


What would happen if this were to be loop { } or panic!()... no error?

Added tests.

Non-defining uses are allowed to be !, but with defining uses you get a type mismatch error. If you wrote if foo { loop {} } else { "" } then it would work again.

nikomatsakis · 2018-07-17T15:00:11Z

OK, so @oli-obk and I were talking about some of the suboptimal errors -- these are a symptom of the current setup, where we check for consistency of the inferred values for an existential type during the "collect" query. The problem is that this invokes the "check fn body" queries which never wind up running the "wfcheck" query on those fns. If we force the "check fn body" query to invoke "wfcheck" (which it really ought to do), then I see the errors I expect. We decided to try and do this more generally in a follow-up.

Therefore, r=me @oli-obk post rebase.

Diff:

diff --git a/src/librustc_typeck/check/mod.rs b/src/librustc_typeck/check/mod.rs
index 3e81262424..3a81891624 100644
--- a/src/librustc_typeck/check/mod.rs
+++ b/src/librustc_typeck/check/mod.rs
@@ -826,6 +826,13 @@ fn typeck_tables_of<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>,
     let id = tcx.hir.as_local_node_id(def_id).unwrap();
     let span = tcx.hir.span(id);

+    // For the base function, first run the well-formedness check.
+    // This is a hack -- there should just be a `check_well_formed`
+    // query that works for any def-id.
+    if let Node::NodeItem(_) = tcx.hir.get(id) {
+        ty::query::queries::check_item_well_formed::ensure(tcx, def_id);
+    }
+
     // Figure out what primary body this item has.
     let (body_id, fn_decl) = primary_body_of(tcx, id).unwrap_or_else(|| {
         span_bug!(span, "can't type-check body of {:?}", def_id);

oli-obk · 2018-07-19T11:36:49Z

@bors r=nikomatsakis

bors · 2018-07-19T11:36:50Z

📌 Commit 5df1fb70ed154af05d0a12dc403221a4a67cd324 has been approved by nikomatsakis

bors · 2018-07-19T12:50:12Z

⌛ Testing commit 5df1fb70ed154af05d0a12dc403221a4a67cd324 with merge d9e9ed5ea3e112013f51097b35aa1a4bc7777343...

bors · 2018-07-19T13:46:10Z

💔 Test failed - status-travis

oli-obk · 2018-07-19T17:17:30Z

@bors r=nikomatsakis

Finally figured out how to run this locally. Passes now

bors · 2018-07-19T17:17:31Z

📌 Commit 9017f79 has been approved by nikomatsakis

bors · 2018-07-19T21:14:08Z

⌛ Testing commit 9017f79 with merge c7cba3d...

@nikomatsakis

Implement existential types (not for associated types yet) r? @nikomatsakis cc @Centril @varkor @alexreg

bors · 2018-07-19T23:24:44Z

☀️ Test successful - status-appveyor, status-travis
Approved by: nikomatsakis
Pushing c7cba3d to master...

rust-highfive · 2018-07-19T23:26:21Z

📣 Toolstate changed by #52024!

Tested on commit c7cba3d.
Direct link to PR: #52024

💔 clippy-driver on windows: test-pass → build-fail (cc @Manishearth @llogiq @mcarton @oli-obk, @rust-lang/infra).
💔 clippy-driver on linux: test-pass → build-fail (cc @Manishearth @llogiq @mcarton @oli-obk, @rust-lang/infra).
💔 rls on windows: test-pass → build-fail (cc @nrc, @rust-lang/infra).
💔 rls on linux: test-pass → build-fail (cc @nrc, @rust-lang/infra).

@Manishearth

Tested on commit rust-lang/rust@c7cba3d. Direct link to PR: <rust-lang/rust#52024> 💔 clippy-driver on windows: test-pass → build-fail (cc @Manishearth @llogiq @mcarton @oli-obk, @rust-lang/infra). 💔 clippy-driver on linux: test-pass → build-fail (cc @Manishearth @llogiq @mcarton @oli-obk, @rust-lang/infra). 💔 rls on windows: test-pass → build-fail (cc @nrc, @rust-lang/infra). 💔 rls on linux: test-pass → build-fail (cc @nrc, @rust-lang/infra).

kennytm · 2018-07-19T23:35:36Z

This breakage is inconveniently timed...

[01:02:31] error[E0004]: non-exhaustive patterns: `AssociatedExistential(_)` not covered
[01:02:31]    --> tools/clippy/clippy_lints/src/utils/mod.rs:964:11
[01:02:31]     |
[01:02:31] 964 |     match def {
[01:02:31]     |           ^^^ pattern `AssociatedExistential(_)` not covered
[01:02:31]
[01:02:31] error[E0004]: non-exhaustive patterns: `Existential(_)` not covered
[01:02:31]   --> tools/clippy/clippy_lints/src/utils/inspector.rs:66:15
[01:02:31]    |
[01:02:31] 66 |         match item.node {
[01:02:31]    |               ^^^^^^^^^ pattern `Existential(_)` not covered
[01:02:31]
[01:02:31] error[E0004]: non-exhaustive patterns: `Existential(_)` not covered
[01:02:31]    --> tools/clippy/clippy_lints/src/missing_doc.rs:176:26
[01:02:31]     |
[01:02:31] 176 |         let desc = match impl_item.node {
[01:02:31]     |                          ^^^^^^^^^^^^^^ pattern `Existential(_)` not covered
[01:02:31]
[01:02:31] error[E0004]: non-exhaustive patterns: `Existential(_)` not covered
[01:02:31]    --> tools/clippy/clippy_lints/src/missing_inline.rs:165:26
[01:02:31]     |
[01:02:31] 165 |         let desc = match impl_item.node {
[01:02:31]     |                          ^^^^^^^^^^^^^^ pattern `Existential(_)` not covered
[01:02:31]
[01:02:31] error: aborting due to 4 previous errors
[01:02:31]
[01:02:31] For more information about this error, try `rustc --explain E0004`.
[01:02:31] error: Could not compile `clippy_lints`.

alexreg · 2018-07-20T03:08:22Z

@kennytm Fixed. rust-lang/rust-clippy#2939

rust-highfive assigned nikomatsakis Jul 3, 2018

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 3, 2018

oli-obk force-pushed the existential_parse branch 2 times, most recently from cf9eb1a to 39c0d89 Compare July 3, 2018 18:05

cramertj reviewed Jul 3, 2018

View reviewed changes

oli-obk force-pushed the existential_parse branch from 39c0d89 to bfe56cf Compare July 3, 2018 18:39