Skip to content
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

Allow default type to be projected in the fully monomorphic case #42411

Closed

Conversation

sgrif
Copy link
Contributor

@sgrif sgrif commented Jun 3, 2017

While I understand the intention behind this decision, I think there are
a ton of cases that it disables which haven't been considered.
Ultimately users are more likely to run into this limitation when trying
to use a type that has a bound such as: T: SomeTrait<AssocType=SpecificValue>. The fact that this bound can fail
can end up being extremely non-local and confusing.

To put this another way, I think the effect of this ends up being the
opposite of what was intended. Simply because I allow an impl to be
specialized, I can no longer use any type which uses that impl in a
context with additional bounds beyond the original trait. Even when I
know the exact type and know that the bounds have been met.

As a simple example, let's say we wanted to change the blanket impl<T: Iterator> IntoIterator for T to be a default impl so that iterators
could implement it differently if they chose to (a bit contrived, but
maybe item is a type parameter instead of an associated type in this
case). By adding the word default, I can no longer pass any iterator
to any function which has additional bounds (e.g. T: IntoIterator, T::Iterator: ExactSizeIterator), even though I know those bounds have
been met.

With this example, it's reasonable to say "well just don't use
specialization here, that's the tradeoff". However, many of the use
cases for specialization are for places where there is no reasonable way
to write a base impl of a trait in a way that doesn't overlap with more
concrete impls. Here is a more concrete use case from Diesel:
https://is.gd/IjMCMp (simplified version https://is.gd/eImBH7)

You could also consider cases like this code (which doesn't compile for
other reasons, but gets the point accross): https://is.gd/UlniuY

Ultimately disallowing projection in the monomorphic case neuters most
uses of default type. While I understand the intention of wanting to
force it to be treated as an abstract interface, ultimately some code
has to be monomorphic, and will want to rely on the concrete value of
the type. The fact that some concrete type happens to use an impl that
was specializable by other types should not remove the ability to do
that.

While I understand the intention behind this decision, I think there are
a ton of cases that it disables which haven't been considered.
Ultimately users are more likely to run into this limitation when trying
to use a type that has a bound such as: `T:
SomeTrait<AssocType=SpecificValue>`. The fact that this bound can fail
can end up being extremely non-local and confusing.

To put this another way, I think the effect of this ends up being the
opposite of what was intended. Simply because I allow an impl to be
specialized, I can no longer use any type which uses that impl in a
context with additional bounds beyond the original trait. Even when I
know the exact type and know that the bounds have been met.

As a simple example, let's say we wanted to change the blanket `impl<T:
Iterator> IntoIterator for T` to be a `default impl` so that iterators
could implement it differently if they chose to (a bit contrived, but
maybe item is a type parameter instead of an associated type in this
case). By adding the word `default`, I can no longer pass any iterator
to any function which has additional bounds (e.g. `T: IntoIterator,
T::Iterator: ExactSizeIterator`), even though I know those bounds have
been met.

With this example, it's reasonable to say "well just don't use
specialization here, that's the tradeoff". However, many of the use
cases for specialization are for places where there is no reasonable way
to write a base impl of a trait in a way that doesn't overlap with more
concrete impls. Here is a more concrete use case from Diesel:
https://is.gd/IjMCMp (simplified version https://is.gd/eImBH7)

You could also consider cases like this code (which doesn't compile for
other reasons, but gets the point accross): https://is.gd/UlniuY

Ultimately disallowing projection in the monomorphic case neuters most
uses of `default type`. While I understand the intention of wanting to
force it to be treated as an abstract interface, ultimately *some* code
has to be monomorphic, and will want to rely on the concrete value of
the type. The fact that some concrete type happens to use an impl that
was specializable by other types should not remove the ability to do
that.
@rust-highfive
Copy link
Collaborator

r? @arielb1

(rust_highfive has picked a reviewer for you, use r? to override)

@sgrif
Copy link
Contributor Author

sgrif commented Jun 3, 2017

/cc @aturon Since you seem to have made the call on this when it was added

@arielb1
Copy link
Contributor

arielb1 commented Jun 3, 2017

@sgrif

I don't think specialization was ever intended to be used in a way that influences type checking like that - forcefully introducing the excluded middle.

@Mark-Simulacrum Mark-Simulacrum added S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). T-lang Relevant to the language team, which will review and decide on the PR/issue. I-nominated labels Jun 4, 2017
@Mark-Simulacrum
Copy link
Member

Nominating for lang team discussion, but feel free to un-nominate if that's not the right thing here.

@eddyb
Copy link
Member

eddyb commented Jun 4, 2017

@sgrif This should have to be a RFC amendment IMO to be accepted.

@sgrif
Copy link
Contributor Author

sgrif commented Jun 4, 2017

@eddyb I considered that, but it seemed like this behavior was left unspecified by the RFC so I figured I'd try here first.

@eddyb
Copy link
Member

eddyb commented Jun 4, 2017

@sgrif That seems like a serious omission either way!

@aturon
Copy link
Member

aturon commented Jun 7, 2017

This was left as an unresolved design question, yes; we wanted more practical experience. I'd prefer to wait on further changes here until we have specialization fully sorted out on Chalk (and, in particular, the story around lifetime dispatch, which may have some interactions). That should give us a much better picture of the tradeoffs here. Work on that front is ongoing.

@aturon aturon added A-specialization Area: Trait impl specialization and removed I-nominated labels Jun 7, 2017
@aturon
Copy link
Member

aturon commented Jun 7, 2017

I've tagged with A-Specialization and removed the nomination.

cc @nikomatsakis @withoutboats

@sgrif
Copy link
Contributor Author

sgrif commented Jun 8, 2017

Let me know if there's anything else I can do for this once Chalk is sorted. We have some very strong use cases for this inside of Diesel, and I actually can't come up with much use for default type (or even code that compiles) with projection being disallowed

@nikomatsakis
Copy link
Contributor

I'm still very wary of this without some form of opt-in or further thought. It effectively means that you only get one chance to decide the value of the associated type, and you can't make something specialized later unless it uses the default value, no?

@sgrif
Copy link
Contributor Author

sgrif commented Jun 11, 2017

It effectively means that you only get one chance to decide the value of the associated type, and you can't make something specialized later unless it uses the default value, no?

Do you mean in terms of code or in terms of backwards compatibility? In terms of code that is definitely not the case, as this only affects the fully monomorphic case, where we know the concrete types of everything involved and therefore know that nothing more specialized can be introduced downstream.

In terms of backwards compatibility, I'm not sure why this should be considered different than changing any other impl. By adding some new more specialized impl that changes the value of an associated type, you're essentially doing the same thing as if you had two non-overlapping impls that weren't using specialization, and changed the signature of one of them. That should be considered a major breaking change in any case, specialization or no.

@arielb1
Copy link
Contributor

arielb1 commented Jun 11, 2017

@sgrif

The "intended use" for default type is to allow specialization of struct layout, as in https://play.rust-lang.org/?gist=98b5be5cefe494ca80afb3fe044eb3a3&version=nightly&backtrace=0 - that works, but requires ugly layers of relays.

@carols10cents
Copy link
Member

carols10cents commented Jun 12, 2017

Since nominated was removed, and since there are test failures, I'm moving this back to waiting on author. It sounds like this might sit for a while though?

@carols10cents carols10cents 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-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). labels Jun 12, 2017
@arielb1
Copy link
Contributor

arielb1 commented Jun 12, 2017

@carols10cents

The team did not accept this PR yet. I'll move this to waiting-on-team.

@arielb1 arielb1 added S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 12, 2017
@nikomatsakis
Copy link
Contributor

nikomatsakis commented Jun 14, 2017

@sgrif

Do you mean in terms of code or in terms of backwards compatibility? In terms of code that is definitely not the case, as this only affects the fully monomorphic case, where we know the concrete types of everything involved and therefore know that nothing more specialized can be introduced downstream.

Let me give you an example. Suppose that I add this:

trait Auxiliary {
    type Buffer: Default;
}

// by default, the "buffer" is a `Vec<u8>`
impl<T> Auxiliary for T {
    default type Buffer = Vec<u8>;
}

Now, independently, I have a struct Foo. I can therefore project <Foo as Auxiliary>::Buffer> and get Vec<u8>.

Now, sometime later, I realize that -- in fact -- the correct type of "buffer" for Foo is Bar. I cannot add an impl like this, because there may be code relying on the current type:

impl<T> Auxiliary for Foo {
    type Buffer = Bar;
}

The fact that this applies only to monomorphic projections is no particular protection. It just means that the user experience will be uneven, because in generic code you won't be able to project when you might want to. e.g., continuing with the above example, although I can always "rely" on Foo::Buffer being Vec<u8>, if I am writing generic code using a Vec<T>, I cannot rely on Vec<T>::Buffer being Vec<u8>. This is because maybe some kinds of vectors (e.g., particular element types) have distinct Buffer values -- but, then again, maybe not! Frequently that kind of distinction is surprising; this is what leads to bad interactions like #19032, which arises from a similar "one size fits all" rule that is part of coherence.

To make things worse, consider that every method is itself a type. Granted, though, these method types are not things you can actually write, but if we allowed you to project them, you might be able to observe equality between these types. For example, should this code type check?

trait Foo {
    fn foo();
}

impl<T> Foo for T {
    default fn foo() { }
}

fn same_type<A>(a: A, b: A) { }

fn main() {
    same_type(<u8 as Foo>::foo, <u16 as Foo>::foo);
}

Today, it doesn't, which means we still have the right to specialize the Foo trait later for u8 and u16. But if we just universally allowed monomorphic projections, it ought to (as it happens, I think that even after this PR, this would still result in an error in our current implementation; but that's because we're apply a consistent rule between types and methods). Maybe it's good to apply distinct rules here, but I'm not sure, it sounds a bit fishy to me.


I want to emphasize something: I know I'm coming off across as hostile, but I'm actually quite sympathetic. I recognize that the current rule is also a "one size fits all" rule -- just one tilted in favor of future expansion. I just don't think that throwing the switch the other way is necessarily the right answer. I feel like there's a common problem here -- one which also affects coherence -- where it'd be nice if we could say with more precision what kinds of specializations may be added in the future. Maybe a similar mechanism can help with the orphan impls (after all, both are a question of "what kinds of impls may get added")...?

My preference would be to close this PR but to open up a thread on internals. I'd like to see some examples of your use cases and get a better feeling for your constraints, and I'd also like to review some of the other things that feel related to me and see if we can suss out a common thread.

Actually, @sgrif, maybe we could schedule a time to chat live about it at some point? (Others too, if they are interested.) That might be a good way to rapidly sync up at least.

@arielb1
Copy link
Contributor

arielb1 commented Jun 14, 2017

@nikomatsakis

same_type(<u8 as Foo>::foo, <u16 as Foo>::foo);

I don't think they'll have the same type anyway - the first method is [impl #1]<u8> and the second one is [impl #1]<u16>. There are probably subtler edge cases because of variance, but that's a good argument for treating method types as abstype.

@withoutboats
Copy link
Contributor

withoutboats commented Jun 14, 2017

@arielb1 @nikomatsakis I think they definitely have the same type, given that they're static methods of no arguments. Types are used to resolve the functions but once resolution is complete they will always resolve to items of the same type.

@nikomatsakis
Copy link
Contributor

@arielb1

I don't think they'll have the same type anyway

Ah, that's true, of course. Perhaps there is no danger for methods at least, which is good!

@nikomatsakis
Copy link
Contributor

@withoutboats

I think they definitely have the same type, given that they're static methods of no arguments.

Well, I think @arielb1 is correct here. That is, the type of a fn is roughly akin to a struct like this:

struct DefaultFoo<Self> {
    ...
}

The fact that they take no arguments and are static doesn't actually matter that much.

@sgrif
Copy link
Contributor Author

sgrif commented Jun 14, 2017

@nikomatsakis What are your thoughts on the second paragraph of my previous reply, which was specifically with regards to this as a backwards compatibility concern? Happy to do a live chat at some point if you'd like. :)

@nikomatsakis
Copy link
Contributor

@sgrif

In terms of backwards compatibility, I'm not sure why this should be considered different than changing any other impl.

To my mind, when you write default in an impl (as was done in the base impl), you are basically signaling that this is something that the end user cannot rely upon, because some subtypes may override it. So that makes it different from changing any other impl -- in those impls, you did not write default, and hence did not signal that this was "hidden".

I do think that there are two distinct -- but not entirely orthogonal -- things here that are being conflated in the current design: the ability to change a value in the future, and the ability for other types to specialize.

Relatedly, I also have this intuition that it is generally ok to add a new impl of some trait for your type; this intuition works with the current specialization rules, but fails if we permit monomorphic projections.

Finally, I do feel like there is another difference to changing an existing impl: with specialization, crates can setup defaults that apply to all types, including types that are unaware of the trait altogether. This means that those traits wind up with a value for that default that may or may not be the best choice for them, and there is nothing they can do to change it, once they become aware of this other trait.

In other words, without specialization, there are two choices. Either:

  • You (the trait author) make a choice on behalf of everybody else that cannot be changed; or,
  • The type author affirmatively makes a choice, from which point they are committed.

But, with specialization and monomorphic projection, there is this awkward middle ground, where the trait author makes a choice for types that exist in the universe at the time they publish their crate, but new types that are added afterwards get one chance to customize that choice (without it being a breaking change): that point where they are first published (or otherwise come to satisfy the requirements of the base impl). I do think that one use case for specialization will be to make "default" impls. Things like impl<T: Debug> MyTrait for T { ... }, and in that case we are running into exactly this scenario, right?


What are your thoughts on the second paragraph of my previous reply

See above. I'm curious what you thought about my point that monomorphic projection is likely not what you really want anyway? That is, if you have generic types in diesel, you likely want to be able to have projections for those types that resolve at type-check time, but you would not be able to.

Happy to do a live chat at some point if you'd like. :)

Sounds good. I was trying to find you on IRC but failed. =)

@shepmaster
Copy link
Member

@nikomatsakis this is your friendly ping of 2 weeks of inactivity.

@carols10cents
Copy link
Member

Aaaaand this has gone another two weeks and @nikomatsakis is now traveling :) But it sounds like he's the only person who can move this forward, so I'm moving this to review, not team.

@carols10cents carols10cents added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). labels Jul 17, 2017
@nikomatsakis
Copy link
Contributor

@carols10cents in the meantime, @sgrif and I spoke and covered the use cases in diesel. I've been wanting to write a write-up but haven't had time. I still feel that those cases would more naturally be handled by other features -- specifically, some kind of negative reasoning or closed traits -- then by this one. However, it's also worth nothing that our investigations into specialization are so far suggesting that, just to make the feature work at all, we have to go quite the opposite direction of this PR, and forbid projection during type-checking altogether if any defaults are involved. So I'm inclined to close this PR at the moment, despite my feeling that @sgrif's use cases are well-motivated and have to be addressed somehow. @aturon, thoughts?

@aturon
Copy link
Member

aturon commented Jul 17, 2017

Yes, I think we need to go ahead and close, likely for a long while. (I don't forsee revisiting this aspect of specialization for quite some time.)

Thanks, @sgrif, and I hope we can find a different way to resolve your issues more quickly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-specialization Area: Trait impl specialization S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants