trait alias infrastructure #45047
trait alias infrastructure #45047
Conversation
|
r? @eddyb (rust_highfive has picked a reviewer for you, use r? to override) |
| @@ -391,6 +391,9 @@ impl<'a> Resolver<'a> { | |||
| self.define(parent, ident, TypeNS, (module, vis, sp, expansion)); | |||
| self.current_module = module; | |||
| } | |||
| ItemKind::TraitAlias(..) => { | |||
| // TODO: do we need to do anything here? | |||
There was a problem hiding this comment.
Yes, the same thing as in ItemKind::Ty (only with Def::TraitAlias).
There was a problem hiding this comment.
You'll also have to add a new variant to PathSource for contexts permitting trait alias paths (they are slightly different than contexts permitting trait paths.)
There was a problem hiding this comment.
Why would the contexts be different? Because impl Alias {} won't be allowed?
There was a problem hiding this comment.
Yes.
So you'll automatically get "expected a trait, found a trait alias" errors on impl Alias {}.
There was a problem hiding this comment.
This already happens with no modifications to PathSource.
There was a problem hiding this comment.
Oh but maybe this is needed to check for impl Alias for Struct {}?
src/libsyntax/parse/parser.rs
Outdated
| } else { | ||
| self.expect(&token::Eq)?; | ||
| let bounds = self.parse_ty_param_bounds()?; | ||
| self.expect(&token::Semi)?; |
src/libsyntax/ast.rs
Outdated
| /// Trait alias | ||
| /// | ||
| /// E.g. `trait Foo = Bar + Quux;` | ||
| TraitAlias(Generics, TyParamBounds), |
There was a problem hiding this comment.
I need to reread the RFC, but IIRC there should be two sets of predicates - one for the alias itself (LHS) and one for the alias substitution result.
I.e.
trait Alias<T: Clone> = where T: Default;
=>
trait Alias<T> whereLHS T: Clone = whereRHS T: Default;(But maybe they are equivalent?)
There was a problem hiding this comment.
I mean Alias<NoClone> is an immediate error, but Alias<CloneNoDefault> is a valid always-false predicate.
There was a problem hiding this comment.
I think they are equivalent - both are added as predicates.
There was a problem hiding this comment.
This should be related to #21903 somehow.
There was a problem hiding this comment.
The RFC only mentions whereRHS.
There was a problem hiding this comment.
@durka
Could you prohibit bounds on trait alias' generic parameters in ast_validation for now?
trait TraitAlias<T: Bound /* <-- error, this one represents "whereLHS" */>
There was a problem hiding this comment.
Added an error for this and also trait Alias<T = ()>. cfail
TLDR is "look how |
|
☔ The latest upstream changes (presumably #45046) made this pull request unmergeable. Please resolve the merge conflicts. |
shepmaster
added
the
S-waiting-on-author
|
I'm in way over my head here. I stopped I can't see where the error is coming from, since trait selection keeps saying no errors and the log looks similar to successful fulfillments, to my eye. Where should I look next? |
| self.print_name(item.name)?; | ||
| self.print_generics(generics)?; | ||
| let mut real_bounds = Vec::with_capacity(bounds.len()); | ||
| // FIXME(durka) this seems to be some quite outdated syntax |
There was a problem hiding this comment.
yeah, you used to have to say trait Foo for ?Sized, but now that's the default (and you have to write trait Foo: Sized)
There was a problem hiding this comment.
I would assume that TraitBoundModifier::Maybe in this context is illegal, right? i.e., trait Foo = ?Sized is kind of nonsense.
src/librustc_typeck/astconv.rs
Outdated
| @@ -1446,7 +1447,15 @@ impl<'a, 'gcx, 'tcx> Bounds<'tcx> { | |||
| } | |||
|
|
|||
| for bound_trait_ref in &self.trait_bounds { | |||
| vec.push(bound_trait_ref.to_predicate()); | |||
| if let Some(node_id) = tcx.hir.as_local_node_id(bound_trait_ref.def_id()) { | |||
| if let hir::ItemTraitAlias(..) = tcx.hir.expect_item(node_id).node { | |||
There was a problem hiding this comment.
Hmm. This doesn't seem like the right place to be doing these sorts of tests. By the time we get to ty::TraitRef, aliases should already be expanded, I think -- or, alternatively, we should do the expansion during trait resolution. The latter seems better and more like the strategy we want long term (not sure if that's the direction you've been going, still reading into the PR). Either way, this bit of code should't have to change.
src/librustc_typeck/collect.rs
Outdated
| @@ -668,7 +668,7 @@ fn adt_def<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>, | |||
| /// Ensures that the super-predicates of the trait with def-id | |||
| /// trait_def_id are converted and stored. This also ensures that | |||
| /// the transitive super-predicates are converted; | |||
| fn super_predicates_of<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>, | |||
| pub fn super_predicates_of<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>, | |||
There was a problem hiding this comment.
Why is this public now? It's a provider so I think it should not have to be, we should be using the query instead.
There was a problem hiding this comment.
It's public because I needed it in the hack that I added to astconv, which you correctly pointed out just above is the wrong place 😛
|
Question: Is the overall strategy here to treat a "trait alias"
If so, I approve. =) This is how I had hoped to do it, though for some reason I thought we might need to do some kind of "syntactic expansion" first. But now I don't see a reason we have to go through that step. |
src/librustc_typeck/collect.rs
Outdated
| if items.is_some() { | ||
| // 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()); |
There was a problem hiding this comment.
I feel like we should be able to treat traits and trait aliases the same here if we set things up right (i.e., either both or neither would get this extra predicate). But it's not a big deal I don't think.
There was a problem hiding this comment.
Yeah, this is the part that works if you say "what are the preds of TraitAlias" (i.e. it adds only the super predicates and not TraitAlias itself) but falls over if you ask "what are the preds of <some item that refers to TraitAlias>", which may indicate it's too early or late to be distinguishing traits from aliases.
I think that this comment is exactly your problem here, though it's possible I misunderstand. I think that hunk should basically be reverted. Instead, we want to extend the There are ways I can see to do this. One way would be that, in HIR lowering, we would basically generate a synthetic impl like Some questions though:
The other way would be to extend trait selection to treat "trait aliases" as a kind of native concept. This isn't too much work, we probably have to add a new vtable variant like pub enum Vtable<'tcx, N> {
...
VtableAlias(VtableAliasData),
...
}
pub struct VtableAliasData<N> {
pub alias_def_id: DefId,
pub substs: &'tcx Substs<'tcx>,
pub nested: Vec<N>,
}and then extend this logic here to add an "alias candidate" for aliases and things. This promises to be straightforward but a lot of boilerplate. Personally, I'm inclined to go with the "lower at HIR construction time" route myself. What do you all think? Am I overlooking something here or does this all make sense. =) |
|
What you asked about was indeed my plan, and I figured I would get to the trait object subtleties once I got everything to stop ICEing all the time 😛. About the HIR desugaring: hmm, but I claimed that sugar was accurate early in the RFC thread, I was corrected. The reasons I was wrong were that (1) it wouldn't allow you to impl a trait alias (which we decided not to allow in the end, but could be relitigated in the future), and (2) it means that |
Also the supertraits point that I raised earlier =)
While true, I think this isn't quite a blocker. In particular, if we do decide to permit that, I feel like it would mostly mean that we want to adjust the desugaring further -- i.e., maybe we desugar that impl by unrolling the trait alias one step (in fact, that's what I would probably expect us to do), but leaving the "magic, implicit" impl for the alias in place. (Really, I want to rework trait selection so that everything winds up getting lowered internally to simpler primitives anyway, which would kind of render this discussion semi-moot, but we're not there yet.)
This is an interesting point, but I think orthogonal from the question of whether to desugar into a "synthetic HIR impl" vs customizing trait selection. It might argue for another approach entirely, though I suspect it can be made to work. Basically, the problem here is that I think we can correct this in two ways:
I favor the first idea. =) |
|
First idea sounds good to me! Type errors already see through type aliases, so the error message doesn't seem like a blocker to me. But, does it present a problem with more complex aliases? For example, |
|
@nikomatsakis So, basically I should revert most of my changes to |
First off, I would say it's a good idea to produce a first PR that basically just makes it an error to use trait aliases in an object type. In any case, I imagine that after we expand the trait alias we'll have to examine the traits and make sure that they fit what our current set of object types can support. You can already type things (e.g., |
|
I've read the previous messages, but I still don't understand why trait aliases can't be expanded in astconv/collect in the same exact way like type aliases are expanded (doesn't matter where, in trait objects or not in trait objects). |
|
Where are type aliases expanded? |
|
|
Triage ping to keep this on your radar @durka! |
|
It's still on my radar!
…On Mon, Oct 30, 2017 at 11:44 AM, Carol (Nichols || Goulding) < ***@***.***> wrote:
Triage ping to keep this on your radar @durka <https://github.com/durka>!
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#45047 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAC3n0_rwIsg_NFQZpoNIOA26kMDmmC-ks5sxe7dgaJpZM4PvOn1>
.
|
They could, but I'd prefer to avoid that if we can. For one thing, it does not offer an easy path to handling where clauses. I actually hope to refactor so that type aliases are also expanded in the trait system eventually, as a kind of poor man's associated type. |
|
@durka how's it going? Still have time to work on this? |
|
Not until next week unfortunately :(
…On Nov 10, 2017 12:26, "Niko Matsakis" ***@***.***> wrote:
@durka <https://github.com/durka> how's it going? Still have time to work
on this?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#45047 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AAC3n57NjyZFGbMooYwL0biASUpnl0ttks5s1Ic0gaJpZM4PvOn1>
.
|
|
@durka Yeah clippy is an unrelated error. You may ignore it for now. |
|
@bors r=petrochenkov |
|
📌 Commit 834674f has been approved by |
kennytm
added
S-waiting-on-bors
trait alias infrastructure This will be an implementation of trait aliases (RFC 1733, #41517). Progress so far: - [x] Feature gate - [x] Add to parser - [x] `where` clauses - [x] prohibit LHS type parameter bounds via AST validation #45047 (comment) - [x] Add to AST and HIR - [x] make a separate PathSource for trait alias contexts #45047 (comment) - [x] Stub out enough of typeck and resolve to just barely not ICE Postponed: - [ ] Actually implement the alias part - [ ] #21903 - [ ] #24010 I need some pointers on where to start with that last one. The test currently does this: ``` error[E0283]: type annotations required: cannot resolve `_: CD` --> src/test/run-pass/trait-alias.rs:34:16 | 34 | let both = foo(); | ^^^ | = note: required by `foo` ```
|
☀️ Test successful - status-appveyor, status-travis |
Implement trait aliases (RFC 1733) Extends groundwork done in #45047, and fully implements rust-lang/rfcs#1733. CC @durka @nikomatsakis
Implement trait aliases (RFC 1733) Extends groundwork done in #45047, and fully implements rust-lang/rfcs#1733. CC @durka @nikomatsakis
Remove outdated syntax from trait alias pretty printing
Given the following program:
```rust
#![feature(trait_alias)]
trait A = ?Sized;
fn main() {}
```
Old output of `rustc +nightly ./t.rs -Zunpretty=normal`:
```rust
#![feature(trait_alias)]
trait A for ? Sized ;
fn main() {}
```
New output of `rustc +a ./t.rs -Zunpretty=normal`:
```rust
#![feature(trait_alias)]
trait A = ?Sized;
fn main() {}
```
cc `@durka` (you've written the `FIXME` in rust-lang#45047, see rust-lang#45047 (comment))
This will be an implementation of trait aliases (RFC 1733, #41517).
Progress so far:
whereclausesPostponed:
I need some pointers on where to start with that last one. The test currently does this: