-
Notifications
You must be signed in to change notification settings - Fork 13k
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
#37653 support default impl
for specialization
#45404
Conversation
☔ The latest upstream changes (presumably #45381) made this pull request unmergeable. Please resolve the merge conflicts. |
Hmm. My impression was that a |
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.
Wow, I'm excited to see someone taking this on. I think this is going in the right direction, but a few things need adjusting for sure. I'll have to ponder a bit about how to proceed.
src/librustc/traits/select.rs
Outdated
// applicable to the trait predicate or the cause of the predicate is an | ||
// `ObjectCastObligation` | ||
if self.tcx().impl_is_default(def_id) && | ||
!self.tcx().default_impl_implement_all_methods(def_id){ |
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.
As I wrote in a separate comment, I don't think that default impls should be considered impls -- this means we don't even need to make candidates for them, I think.
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.
The RFC doesn't mention it. It was just my deduction to let them act as regular impls if they are full implemented. I'll remove it.
Concerning the candiates, do you mean moving this check when creating candiates (select.rs#L1110) instead of doing it as a filter at the end of the selection process?
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.
Right, so, one option might be to filter the impls out in this loop, but really I suspect that we just don't want them to appear in the result of for_each_relevant_impl
at all.
In general, I've been trying to think through the idea of doing a "desugaring" step to start. In other words, I'm not happy with how our trait selection code works directly on traits and impls, and I'd prefer for it to work more like chalk -- first lowering traits/impls to some kind of predicate, and then applying those.
I don't necessarily want to block support for default on making that transition, though, but in general we probably want to do a bit of refactoring to better isolate the "things we use to do trait selection" from the "actual impls" that the user wrote.
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.
Ok, I'm working on it
src/librustc/ty/instance.rs
Outdated
@@ -219,6 +224,13 @@ fn resolve_associated_item<'a, 'tcx>( | |||
traits::VtableImpl(impl_data) => { | |||
let (def_id, substs) = traits::find_associated_item( | |||
tcx, trait_item, rcvr_substs, &impl_data); | |||
|
|||
check_unimplemented_trait_item(tcx, |
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 doesn't seem like the right place to be doing this check. This function runs after type-checking has completed, so this error should be reported earlier. Under what scenario did this code execute before?
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.
At first I thought that this error should be raised in typeck but I didn't found there where an impl method call is evaluated against a set of possible impls.
I mean, given this code
trait Foo {
fn foo_one(&self) -> &'static str;
fn foo_two(&self) -> &'static str;
}
struct MyStruct;
default impl<T> Foo for T {
fn foo_one(&self) -> &'static str {
"generic"
}
}
default impl<T: Clone> Foo for T {
fn foo_one(&self) -> &'static str {
"generic Clone"
}
}
If i call MyStruct.foo_one()
the compiler select the method contained in the first impl.
The error should be raised in case this impl doesn't contain an imlementation of foo_one
.
Since I didn't found the selection process during typeck I've choosen to implement it after it occurs inside the resolve_associated_item
function
Do you think I should move all inside typeck?
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.
right, so what should happen in this case is that you get an error, because MyStruct
does not implement Foo
. This is sort of related to the prior discussion -- having a default impl
just means that you have some defaults that are applied to other impls.
What I would expect is that if the user added a impl Foo for MyStruct
, then we would get an error there saying that no version of foo_two
can be found.
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.
Ok, Got it. So there is no need to check for an unimplemented trait item. I'll remove it and include some tests on the use cases you provided.
src/librustc/ty/mod.rs
Outdated
pub struct TraitPredicate<'tcx> { | ||
pub trait_ref: TraitRef<'tcx> | ||
pub trait_ref: TraitRef<'tcx>, | ||
pub default_impl_check: DefaultImplCheck |
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.
What is the function of this field?
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 field is like a flag used during Trait Resolution.
When setted to Yes
, the ImplCandidate
resolved from an Obligation
that contain the TraitPredicate
are checked to see if they refer to a default impl
.
An Err(Unimplemented)
is returned in that case because a default impl
doesn't implement a Trait
.
For example, given this code
trait Foo {
fn foo_one(&self) -> &'static str;
fn foo_two(&self) -> &'static str;
}
default impl<T> Foo for T {
fn foo_one(&self) -> &'static str {
"generic"
}
}
struct MyStruct;
- a call like
MyStruct.foo_one()
should pass the trait resolution since theTraitPredicate(MyStruct as Foo)
is marked asNo
- a call like
foo(MyStruct)
with foo likefn foo<T: Foo>(x: T) -> &'static str
should fail the trait resolution since theTraitPredicate(MyStruct as Foo)
is marked asYes
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.
OK, I don't think we should need this field once we've aligned things correctly.
@giannicic I wonder -- maybe we could schedule a time to chat about this change in more detail (i.e., a video chat -- or perhaps just over IRC/gitter)? I'll have to try to map out how I think it would best be implemented, I've not thought that much about it. |
Sure @nikomatsakis Edit Probably is to early for you :) |
@giannicic sorry I missed you the other day. I'm at the Rust Belt Rust conference right now so my schedule is sort of wacky, but we could probalby schedule a time to "synchronize" -- actually your time zone may be helpful, since I could get up early. Perhaps Thursday (tomorrow), say at 11:00 GMT? (Which I believe is 7:00 US Eastern Time, which is my local time zone.) |
@nikomatsakis Ok, see you tomorrow. |
@nikomatsakis What do you think about? Should I delete them or try to let them work by adding some additional check inside the |
As discussed, those tests may not be valid, since having a I was thinking: before we implement |
@nikomatsakis Concerning the new task, sure I'd like to do it! |
d6cf351
to
87e7f35
Compare
@nikomatsakis I'm reading this comment as "we shouldn't do this PR until we figure out these other bits" — is that accurate? If so, shall we close this PR until we figure out those bits? |
Sorry for radio silence. I'm not quite sure how to proceed here, to be quite honest. Part of the problem is that I am focusing ~100% on getting NLL and a few other major work items over the finish line, and it's hard for me to focus right now on thinking through what default impls would require to get implemented. That said, @giannicic, you've done some good work here, and I hate to leave you hanging! Here is what I propose:
|
Sure, Niko! You can rely on me ;) Concerning the "Restrictions around lifetime dispatch" I'll file an issue with what is my understanding of the things to do so, you can review it as you find some time. Let me know about this PR as well. |
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.
The surviving tests seem pretty reasonable, and the changes in this PR at least seem basically correct to me! Sorry for the long delay. I did have one concern though (see below).
src/librustc_typeck/check/wfcheck.rs
Outdated
@@ -383,8 +383,18 @@ impl<'a, 'gcx> CheckTypeWellFormedVisitor<'a, 'gcx> { | |||
fcx.body_id, | |||
&trait_ref, | |||
ast_trait_ref.path.span); | |||
|
|||
// not registering predicates associcated with a `default 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.
OK, so, I think what this is doing is trying to skip the logic that requires a trait's where-clauses to be satisfied for it to be implemented. Which seems .. maybe right, maybe not. But what I don't know is why it is only skipping trait
obligations, that seems definitely odd.
For example:
trait Foo<'a, T: Eq + 'a> { }
default impl<U> Foo<'static, U> for () { }
This will error because U: 'static
is not known to hold, but will not error about the fact that U: Eq
is not known to hold. Neither is obviously right to me. I suspect we should just not skip any checks here. Do you know what motivated you to do so?
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.
Yes, I skip registering default impl
obligations as a conseguence of not emitting default impl
s inside for_each_relevat_impl
At first I've just skipped all kind of obligations but I noticed that, when running the full test suite, some tests failed.
Instead when skipping only trait obligations the tests run correctly.
If I remove the default
keyword in your example the error given is
the trait bound `U: std::cmp::Eq` is not satisfied`
If I understand you correctly this should be the error that I should get if i put the default
keyword again so, I'm finding out which is the obligation involved in this check in order
to extract a general rule to follow when registering these predicates.
Perhaps I should skip only the TraitPredicate
corresponding to the TraitRef
of the impl. I'm doing some checks and let you know
Thanks
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.
At first I've just skipped all kind of obligations but I noticed that, when running the full test suite, some tests failed.
Do you mean you just always skipped all kinds of obligations? Or only when you skipped all obligations for default
impls? If the latter, can you tell me which tests failed?
Still, yes, I think we should be skipping no obligations here, and checking the same conditions on default
impls as non-default impls.
I'm finding out which is the obligation involved in this check in order
to extract a general rule to follow when registering these predicates.
I don't understand this sentence, sorry :(
87e7f35
to
efd8feb
Compare
@nikomatsakis , I think I've found a different solution, Hope it works for you :) Basically I've introduced a new |
ping @nikomatsakis -- looks like this is back in your court! |
ping^2 @nikomatsakis -- looks like this is back in your court! |
src/librustc/infer/mod.rs
Outdated
@@ -181,6 +181,9 @@ pub struct InferCtxt<'a, 'gcx: 'a+'tcx, 'tcx: 'a> { | |||
// obligations within. This is expected to be done 'late enough' | |||
// that all type inference variables have been bound and so forth. | |||
region_obligations: RefCell<Vec<(ast::NodeId, RegionObligation<'tcx>)>>, | |||
|
|||
// true if trait selection in this context should emit `default impl` candiates | |||
pub emit_defaul_impl_candidates: Cell<bool>, |
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.
Sorry again for the delay. This flag doesn't feel right. It seems like it's true when we are checking the default impl itself, is that right? Can you give an example of the errors you were getting that you wanted to solve via this flag?
My guess is that the right way to solve this would be by adding something into the parameter environment for a default impl; for example, an assumption that Self: Trait<..>
, rather like we have the trait definition itself, which is -- after all -- a kind of default 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.
Oh, I see you left a pretty detailed comment saying why you added it. =) That sounds about like what I was expecting -- but I don't think the flag is the right way to go. We would rather approach this through the parameter environment, I think.
Heads up from triage, @giannicic — you've got some pending feedback to address. |
@shepmaster |
a `default impl` need not include all items from the trait a `default impl` alone does not mean that a type implements the trait
a default impl should never be considered as implementing the trait on its own -- regardless of whether it contains all items or not
not skipping any wfchecks on default impls
efd8feb
to
2f22a92
Compare
@nikomatsakis |
@giannicic cool! will look asap |
src/librustc/traits/select.rs
Outdated
// Check if default impls should be emitted. | ||
// default impls are emitted if the param_env is refered to a default impl. | ||
// The param_env should contain a Self: Trait<..> predicate in those cases | ||
let self_trait_is_present:Vec<&ty::Predicate<'tcx>> = |
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.
Hmm, I don't think we should have to modify select.rs at all. Instead, what I would expect is for us to modify the set of predicates associated with default impls, analogously to what we do with traits. This would be part of the predicates_of
query -- for example, in traits, we insert Self: Trait<...>
right here:
rust/src/librustc_typeck/collect.rs
Lines 1444 to 1446 in 4d2d3fc
// Add in a predicate that `Self:Trait` (where `Trait` is the | |
// current trait). This is needed for builtin bounds. | |
predicates.push(trait_ref.to_poly_trait_ref().to_predicate()); |
I would think that we would want to do the same code in the case of a default impl
. In that case, I don't think we should have to modify select.rs
at all. The required assumptions will be found in the environment.
@nikomatsakis |
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.
Looking good! Left a few comments! Thanks for seeing this through @giannicic, I know it's been a long road.
@@ -579,15 +579,15 @@ impl<'a, I, T: 'a> FusedIterator for Cloned<I> | |||
{} | |||
|
|||
#[doc(hidden)] | |||
default unsafe impl<'a, I, T: 'a> TrustedRandomAccess for Cloned<I> | |||
unsafe impl<'a, I, T: 'a> TrustedRandomAccess for Cloned<I> |
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.
Indeed, I think default
here was incorrect.
cc @rust-lang/libs -- TrustedRandomAccess
was meant to be implemented for Cloned<I>
, right?
(i.e., a default impl is not itself an impl, it just supplies defaults to other impls)
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.
Yeah I think this looks fine by me!
src/librustc_typeck/collect.rs
Outdated
@@ -1446,6 +1452,10 @@ fn explicit_predicates_of<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>, | |||
predicates.push(trait_ref.to_poly_trait_ref().to_predicate()); | |||
} | |||
|
|||
if let Some(trait_ref) = is_default_impl_trait { |
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.
Can you add a comment here? Something like this:
// In default impls, we can assume that the self type implements
// the trait. So in:
//
// default impl Foo for Bar { .. }
//
// we add a default where clause `Foo: Bar`. We do a similar thing for traits
// (see below). Recall that a default impl is not itself an impl, but rather a
// set of defaults that can be incorporated into another impl.
} | ||
|
||
impl Foo for MyStruct {} | ||
//~^ ERROR not all trait items implemented, missing: `foo_two` [E0046] |
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.
👍
struct MyStruct; | ||
|
||
default impl<T> Foo for T { | ||
fn foo_one(&self) -> &'static str { |
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.
👍
// except according to those terms. | ||
|
||
#![feature(specialization)] | ||
|
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.
It'd be nice to have comments on these tests with what they are testing:
Tests that (a) default impls do not have to supply all items and (b) a default impl does not count as an impl (in this case, an incomplete default impl).
// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your | ||
// option. This file may not be copied, modified, or distributed | ||
// except according to those terms. | ||
|
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.
It'd be nice to have comments on these tests with what they are testing:
Tests that (a) default impls do not have to supply all items but (b) regular impls do.
@@ -10,23 +10,9 @@ | |||
|
|||
#![feature(specialization)] | |||
|
|||
// Regression test for ICE when combining specialized associated types and type | |||
// aliases | |||
trait Foo<'a, T: Eq + 'a> { } |
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.
Comment:
Tests that a default impl still has to have a WF trait ref.
@@ -8,30 +8,27 @@ | |||
// option. This file may not be copied, modified, or distributed | |||
// except according to those terms. | |||
|
|||
// Test that non-method associated functions can be specialized | |||
|
|||
#![feature(specialization)] | |||
|
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.
Comment:
Tests that we can combine a default impl that supplies one method with a full impl that supplies the other, and they can invoke one another.
|
||
default impl<T> Foo for T { | ||
fn foo_one(&self) -> &'static str { | ||
"generic" |
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.
Maybe nice to have this invoke a foo_three
that the regular impl supplies, just to complete the "calling back and forth" cycle.
// $i32 &str $Vec<T> | ||
// /\ | ||
// / \ | ||
// Vec<i32> $Vec<i64> |
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.
What's up with these tests? Were they just not quite correct? I like that they are testing more advanced scenarios, but it seems fine to remove them from now and revisit them later. I've not looked closely at them mind you. =)
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 test has been created by me in a previous PR by just coping the files that was in the specialization dir. As discussed here #45404 (comment), i think that it's wrong because it's on the assumption that a default impl is also a regular impl so you've removed it.
482cb70
to
220bb22
Compare
@nikomatsakis |
@bors r+ |
📌 Commit 220bb22 has been approved by |
#37653 support `default impl` for specialization this commit implements the second part of the `default impl` feature: > - a `default impl` need not include all items from the trait > - a `default impl` alone does not mean that a type implements the trait The first point allows rustc to compile and run something like this: ``` trait Foo { fn foo_one(&self) -> &'static str; fn foo_two(&self) -> &'static str; } default impl<T> Foo for T { fn foo_one(&self) -> &'static str { "generic" } } struct MyStruct; fn main() { assert!(MyStruct.foo_one() == "generic"); } ``` but it shows a proper error if trying to call `MyStruct.foo_two()` The second point allows a `default impl` to be considered as not implementing the `Trait` if it doesn't implement all the trait items. The tests provided (in the compile-fail section) should cover all the possible trait resolutions. Let me know if some tests is missed. See [referenced ](#37653) issue for further info r? @nikomatsakis
☀️ Test successful - status-appveyor, status-travis |
BTW, @giannicic and @qnighy -- check out this internals thread announcing the traits working group, I thought you folks might be interested. |
this commit implements the second part of the
default impl
feature:The first point allows rustc to compile and run something like this:
but it shows a proper error if trying to call
MyStruct.foo_two()
The second point allows a
default impl
to be considered as not implementing theTrait
if it doesn't implement all the trait items.The tests provided (in the compile-fail section) should cover all the possible trait resolutions.
Let me know if some tests is missed.
See referenced issue for further info
r? @nikomatsakis