Implement existential types by oli-obk · Pull Request #52024 · rust-lang/rust (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation70 Commits16 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
stepancheg reacted with thumbs up emoji matthewjasper, cramertj, LifeIsStrange, Centril, sinkuu, fanzier, wesleywiser, lqd, killercup, clarfonthey, and 10 more reacted with hooray emoji cramertj, Centril, varkor, alexreg, lqd, seanmonstar, killercup, Pratyush, and 95th reacted with heart emoji
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense. This case shouldn't be hard to catch (instead of being an error, this case would be a non-defining use, right?)
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@oli-obk Yes, although it still has to be unified with whatever the "fully defining" usage is to make sure that it can work.
This comment has been minimized.
The following cases do not error yet, but should
existential type Cmp: std::cmp::PartialEq; //~^ ERROR could not find defining uses
// not a defining use, because it doesn't define all possible generics fn cmp() -> Cmp { 5u32 //~ mismatched types }
existential type Cmp2: std::cmp::PartialEq; //~^ 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) -> Cmp2 {
t
}
This comment has been minimized.
Are you going to follow up with associated types in a separate PR, @oli-obk?
Yea, I want to get the "simple" case working first. I'm slowly getting the hang of generics.
This comment has been minimized.
@oli-obk Waiting for review before you fix the above errors? :-)
This comment has been minimized.
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
This comment has been minimized.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for being slow. This is so awesome. Left a bunch of minor comments below.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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); }
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we update this comment? This is specific to impl Trait
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: why not get rid of the * and the ref =)
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should make a clippy lint for that
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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...
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, this code doesn't look right:
| 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:
| 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also
existential type WrongGeneric: 'static;
//~^ ERROR the parameter type T may not live long enough
fn wrong_generic(t: T) -> WrongGeneric { t }
stops being an error with NLL if I do that change.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PS I am resisting complaining about this syntax so much 😛
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please put these tests into src/test/ui/existential_type/XXX.rs files
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What would happen if this were to be loop { } or panic!()... no error?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
This comment has been minimized.
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);
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
📌 Commit 5df1fb70ed154af05d0a12dc403221a4a67cd324 has been approved by nikomatsakis
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
⌛ Testing commit 5df1fb70ed154af05d0a12dc403221a4a67cd324 with merge d9e9ed5ea3e112013f51097b35aa1a4bc7777343...
This comment has been minimized.
Status: Awaiting review from the assignee but also interested parties.
and removed S-waiting-on-bors
Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
labels
@bors r=nikomatsakis
Finally figured out how to run this locally. Passes now
📌 Commit 9017f79 has been approved by nikomatsakis
Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
and removed S-waiting-on-review
Status: Awaiting review from the assignee but also interested parties.
labels
bors added a commit that referenced this pull request
rust-highfive added a commit to rust-lang-nursery/rust-toolstate that referenced this pull request
This was referenced
Jul 19, 2018
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`.
Labels
Status: Waiting on bors to run and complete tests. Bors will change the label on completion.