-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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
Implement associated existential types #52650
Conversation
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
}, | ||
Some(hir::map::NodeImplItem(item)) => match item.node { | ||
hir::ImplItemKind::Existential(_) => may_define_existential_type( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess one of the wacky -- but perhaps .. fine -- consequences of this is that one could do something like
impl Foo for Bar {
existential type Item;
fn method() {
fn helper() -> <Bar as Foo>::Item { .. }
}
and that helper
fn might count as a defining use?
I feel like we should only allow other members of the impl to be a defining use.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not allowing this kind of defining use would be inconsistent with how normal existential types work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see it as particularly inconsistent — depends on your POV, I suppose. It seems to me that the various "sugared forms" (e.g., impl Trait
) don't desugar directly to an existential type
"sibling item", because they have different sets of defining uses... in the case of a function, only the fn return type in question. In the case of an impl, we get to decide?
For that matter, I do wonder if we would rather have the syntax here be type Item = impl Foo
.
(cc @cramertj on this particular question)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm ok landing either way, but I would want to open an unresolved question on this point
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yea the syntax is horrible on purpose. It's the only one everyone could agree on that we do not want.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added two unresolved questions to the tracking issue. Dear god I hope they won't get lost. =)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO it's fine for helper
to be a defining use of Item
because it's "in the scope" of Item
's definition (in this case, the trait impl).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was there another discussion on syntax? Two points:
- sometimes there are no constraints: but
type Item;
is of course ambiguous - "existential" is not really the correct term (mathematically it means "there exists at least one such ..."); really we just mean "indirectly specified"
So possibly indirect type Item;
or unspec type Item;
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This implemented the placeholder syntax from the RFC. Please check out the discussion on the tracking issue and follow up RFCs. The syntax has been heavily discussed there and is currently being RFCed as type Item = impl Trait;
. So if you disagree with the proposed syntax or otherwise have opinions on this topic, bring it up on that RFC.
src/librustc/traits/project.rs
Outdated
} | ||
let substs = translate_substs(selcx.infcx(), param_env, impl_def_id, substs, assoc_ty.node); | ||
let ty = if let ty::AssociatedKind::Existential = assoc_ty.item.kind { | ||
tcx.mk_anon(assoc_ty.item.def_id, substs) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
huh, something looks fishy here -- you are using substs
as part of ty
in this case...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yea, I'm gonna add some generics tests and do this properly
} else { | ||
tcx.type_of(assoc_ty.item.def_id) | ||
}; | ||
let substs = translate_substs(selcx.infcx(), param_env, impl_def_id, substs, assoc_ty.node); | ||
Progress { | ||
ty: ty.subst(tcx, substs), |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| (Type, Existential) | ||
=> tcx.hygienic_eq(impl_item.ident, trait_item_name, trait_def_id), | ||
|
||
_ => false, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer to see this spelled out:
e.g., (Const, _) | (Method, _) | (Type, _)
etc
this way when new kinds are added, we won't overlook them
65e016b
to
01eacd8
Compare
@bors r+ |
📌 Commit 33712a8 has been approved by |
⌛ Testing commit 33712a8 with merge f1ecabf25ee6d7fc4a607e2e820013b8bf8619a1... |
💔 Test failed - status-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 retry |
⌛ Testing commit 33712a8 with merge c82d35770ac9d4b53bb93247456206c003c5e4e8... |
💔 Test failed - status-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 |
…tsakis Implement associated existential types r? @nikomatsakis no idea if these work with generic traits. I'm going home for the day 🤣
☀️ Test successful - status-appveyor, status-travis |
r? @nikomatsakis
no idea if these work with generic traits. I'm going home for the day 🤣