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

Refactor supertrait handling #23026

Merged
merged 8 commits into from
Mar 5, 2015

Conversation

nikomatsakis
Copy link
Contributor

The main gist of this PR is commit 1077efb which removes the list of supertraits from the TraitDef and pulls them into a separate table, which is accessed via lookup_super_predicates. This is analogous to lookup_predicates, which gets the complete where clause. This allows us to create the TraitDef, which contains the list generics and so forth, without fully knowing the list of supertraits. This in turn allows the supertrait listing to contain references to associated types like <Self as Foo>::Item, which were previously impossible because conversion required having the TraitDef for Foo.

We do not yet support Self::Item in a supertrait listing. This doesn't work because to convert that, it attempts to expand out the full set of supertraits, which are in the process of being created. This could potentially be worked out by having the expansion of supertraits proceed in a lazy fashion, but we'd have to define shadowing rules for associated types which we don't currently have.

Along the way (in 9de9ec5) I also removed the restriction against duplicate bounds and generalized the code so that it can handle having the same supertrait multiple times with different arguments, e.g. Foo : Bar<i32> + Bar<u32>. This restriction was serving no particular purpose, since the same trait could be extended multiple times indirectly, and in the era of multidispatch it is actively harmful.

This is technically a [breaking-change] because it affects the definition of a super-trait. Anything in a where clause that looks like where Self : Foo is now considered a supertrait. Because cycles are disallowed in supertraits, that could lead to some errors. This has not been observed in any existing code.

r? @nrc

@nikomatsakis
Copy link
Contributor Author

cc @jroesch

@japaric
Copy link
Member

japaric commented Mar 4, 2015

I also removed the restriction against duplicate bounds and generalized the code so that it can handle having the same supertrait multiple times with different arguments, e.g. Foo : Bar + Bar

👍

@bors
Copy link
Contributor

bors commented Mar 4, 2015

☔ The latest upstream changes (presumably #22235) made this pull request unmergeable. Please resolve the merge conflicts.

interface, so that we can perform this query without requiring a full
trait def or set of supertraits.
us to construct trait-references and do other things without forcing a
full evaluation of the supertraits. One downside of this scheme is that
we must invoke `ensure_super_predicates` before using any construct that
might require knowing about the super-predicates.
@nrc
Copy link
Member

nrc commented Mar 5, 2015

r+ with comments addressed

@nikomatsakis
Copy link
Contributor Author

@bors r+ 9b332ff p=1

Giving higher priority because I believe this fixes a number of open associated type issues (probably more than I have tagged here).

@bors
Copy link
Contributor

bors commented Mar 5, 2015

⌛ Testing commit 9b332ff with merge eb1fc4e...

@bors
Copy link
Contributor

bors commented Mar 5, 2015

💔 Test failed - auto-linux-64-nopt-t

@Manishearth
Copy link
Member

@bors: retry

(Failed due to a force push, sorry)

@bors
Copy link
Contributor

bors commented Mar 5, 2015

bors added a commit that referenced this pull request Mar 5, 2015
…sakis

The main gist of this PR is commit 1077efb which removes the list of supertraits from the `TraitDef` and pulls them into a separate table, which is accessed via `lookup_super_predicates`. This is analogous to `lookup_predicates`, which gets the complete where clause. This allows us to create the `TraitDef`, which contains the list generics and so forth, without fully knowing the list of supertraits. This in turn allows the *supertrait listing* to contain references to associated types like `<Self as Foo>::Item`, which were previously impossible because conversion required having the `TraitDef` for `Foo`.

We do not yet support `Self::Item` in a supertrait listing. This doesn't work because to convert that, it attempts to expand out the full set of supertraits, which are in the process of being created. This could potentially be worked out by having the expansion of supertraits proceed in a lazy fashion, but we'd have to define shadowing rules for associated types which we don't currently have.

Along the way (in 9de9ec5) I also removed the restriction against duplicate bounds and generalized the code so that it can handle having the same supertrait multiple times with different arguments, e.g. `Foo : Bar<i32> + Bar<u32>`. This restriction was serving no particular purpose, since the same trait could be extended multiple times indirectly, and in the era of multidispatch it is actively harmful.

This is technically a [breaking-change] because it affects the definition of a super-trait. Anything in a where clause that looks like `where Self : Foo` is now considered a supertrait. Because cycles are disallowed in supertraits, that could lead to some errors. This has not been observed in any existing code.

r? @nrc
@bors
Copy link
Contributor

bors commented Mar 5, 2015

⌛ Testing commit 9b332ff with merge f0c74f8...

@bors bors merged commit 9b332ff into rust-lang:master Mar 5, 2015
@nikomatsakis nikomatsakis deleted the issue-20220-supertrait branch March 30, 2016 16:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants