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

Fix coherence checking for impl trait in type aliases #63934

Open
wants to merge 1 commit into
base: master
from

Conversation

@Aaron1011
Copy link
Contributor

commented Aug 27, 2019

Fixes #63677

RFC 2071 (impl-trait-existential-types) does not explicitly state how type_alias_impl_trait should interact with coherence. However, there's only one choice which makes sense - coherence should look at the underlying type (i.e. the "defining" type of the impl Trait) of the type alias, just like we do for non-impl Trait type aliases.

Specifically, impl Trait type aliases that resolve to a local type should be treated like a local type with respect to coherence (e.g. impl Trait type aliases which resolve to a foreign type should be treated as a foreign type, and those that resolve to a local type should be treated as a local type).

Since neither inherent impls nor direct trait impl (i.e. impl MyType or impl MyTrait for MyType) are allowed for type aliases, this usually does not come up. Before we ever attempt to do coherence checking, we will have errored out if an impl Trait type alias was used directly in an impl clause.

However, during trait selection, we sometimes need to prove bounds like T: Sized for some type T. If T is an impl trait type alias, this requires to know the coherence behavior for impl Trait type aliases when we perform coherence checking.

Note: Since determining the underlying type of an impl Trait type alias requires us to perform body type checking, this commit causes us to type check some bodies easier than we otherwise would have. However, since this is done through a query, this shouldn't cause any problems

For completeness, I've added an additional test of the coherence-related behavior of impl Trait type aliases.

cc #63063

Fix coherence checking for impl trait in type aliases
Fixes #63677

RFC #2071 (impl-trait-existential-types) does not explicitly state how
impl trait type alises should interact with coherence. However, there's
only one choice which makes sense - coherence should look at the
underlying type (i.e. the 'defining' type of the impl trait) of the type
alias, just like we do for non-impl-trait type aliases.

Specifically, impl trait type alises which resolve to a local type
should be treated like a local type with respect to coherence (e.g.
impl trait type aliases which resolve to a forieign type should be
treated as a foreign type, and those that resolve to a local type should
be treated as a local type).

Since neither inherent impls nor direct trait impl (i.e. `impl MyType`
or `impl MyTrait for MyType`) are allowd for type aliases, this
usually does not come up. Before we ever attempt to do coherence
checking, we will have errored out if an impl trait type alias was used
directly in an 'impl' clause.

However, during trait selection, we sometimes need to prove bounds like
'T: Sized' for some type 'T'. If 'T' is an impl trait type alias, this
requires to know the coherence behavior for impl trait type aliases when
we perform coherence checking.

Note: Since determining the underlying type of an impl trait type alias
requires us to perform body type checking, this commit causes us to type
check some bodies easlier than we otherwise would have. However, since
this is done through a query, this shouldn't cause any problems

For completeness, I've added an additional test of the coherence-related
behavior of impl trait type aliases.
@rust-highfive

This comment has been minimized.

Copy link
Collaborator

commented Aug 27, 2019

r? @petrochenkov

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

@@ -0,0 +1,17 @@
// aux-build:foreign-crate.rs

This comment has been minimized.

Copy link
@Centril

Centril Aug 27, 2019

Member

This test could use some comments.

@Centril

This comment has been minimized.

Copy link
Member

commented Aug 27, 2019

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

commented Sep 4, 2019

This is another PR that requires some thought. I'm scheduling a slot on this coming Friday (Sep 6) to do that.

@nikomatsakis
Copy link
Contributor

left a comment

Spent some time thinking about this PR. I'm not sure yet if I think it's the right direction, but here are some interim notes.

// since we've already managed to successfully
// resolve all opaque types by this point
let real_ty = tcx.type_of(did);
ty_is_local_constructor(tcx, real_ty, in_crate)

This comment has been minimized.

Copy link
@nikomatsakis

nikomatsakis Sep 6, 2019

Contributor

I'm not sure if this is the only interpretation that makes sense. In fact, it's a bit surprising to me, since it means that impls can "peer into" the hidden type of an impl Trait, albeit only in limited ways. On the one hand, if the impl is within the scope of the impl Trait, this seems logical enough. On the other hand, if the impl is outside the scope of the impl Trait, it seems less clear.

As you say, the restrictions against having an opaque type in an impl somewhat prevent this from coming up -- but I guess the bottom line is that they are not complete, which is why it is coming up. I can't really come up with a strong reason why impl Opaque is disallowed by impl Foo<Opaque> is allowed -- whatever hard questions we have to answer about overlap seem to arise just as easily in both cases.

Another option would be to prohibit Opaque types more deeply (though we have to think about the interaction of that with type aliases and associated types). Even then though, similar questions could arise because of auto traits -- well, it's not quite the same I guess, since we are asking

I'd be somewhat tempted to say that Opaque is always considered non-local -- have you tried that? It would mean that we can't assume negative things about Opaque ..

Well, I realize now there is one additional complication, which is fundamental traits. If we don't do coherence in a "revealed" context, we could make some mistakes. For example, we might determine that impl Clone does not implement FnOnce(), but we don't actually know that. This would happen because we would attempt to prove impl Clone: FnOnce() and fail and interpret that failure as "does not implement".

To solve that, I guess, we could either do coherence in a revealed context (where we strongly normalize opaque types, and hence they don't exist) or we could make a smarter form of negation (more like what chalk has, which sidesteps this trap). Basically it would know that not { impl Clone: FnOnce() } is not provable. I'd have to check back in the code to see how easy this would be to add.

This comment has been minimized.

Copy link
@Aaron1011

Aaron1011 Sep 6, 2019

Author Contributor

That's a good point - I hadn't considered the impl Foo<Opaque> case.

I think it makes sense to say that opaque types are always non-local with respect to coherence. Otherwise, you could run into a situation where changing the underlying type is a breaking change (e.g. impl Foo<Opaque> no longer references any local types), which is exactly what opaque types are supposed to help prevent.

@JohnCSimon

This comment has been minimized.

Copy link

commented Sep 14, 2019

Ping from triage

@Aaron1011 What is the status of this pr?

Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.