Skip to content

Implement trait aliases (RFC 1733) - #55101

Merged
bors merged 10 commits into
rust-lang:masterfrom
alexreg:trait-aliases
Nov 3, 2018
Merged

Implement trait aliases (RFC 1733)#55101
bors merged 10 commits into
rust-lang:masterfrom
alexreg:trait-aliases

Conversation

@alexreg

@alexregalexreg commented Oct 15, 2018

Copy link
Copy Markdown
Contributor

Extends groundwork done in #45047, and fully implements rust-lang/rfcs#1733.

CC @durka@nikomatsakis

@rust-highfive

Copy link
Copy Markdown
Contributor

r? @zackmdavis

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

@rust-highfiverust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 15, 2018
@rust-highfive

This comment has been minimized.

@petrochenkovpetrochenkov self-assigned this Oct 15, 2018
@alexreg

Copy link
Copy Markdown
ContributorAuthor

@petrochenkov Don't even bother reviewing yet. A lot of work to go. I'll let you know when things are at a decent stage. :-) I think @nikomatsakis is going to comment with some more guidance shortly.

@zackmdaviszackmdavis removed their assignment Oct 15, 2018
Comment threadsrc/librustc/traits/select.rs Outdated
Comment threadsrc/librustc/traits/select.rs Outdated
Comment threadsrc/librustc/ty/mod.rs Outdated
Comment threadsrc/librustc/ty/mod.rs Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should just return vec!. Trait aliases do not define any associated items themselves. The associated item resolution code does have a notion of walking the superpredicates when doing lookups, I would think we should build on that.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense.

Comment threadsrc/libsyntax/feature_gate.rs Outdated
Comment threadsrc/librustc/traits/select.rs Outdated
@nikomatsakisnikomatsakis 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-review Status: Awaiting review from the assignee but also interested parties. labels Oct 16, 2018
@alexreg

alexreg commented Oct 16, 2018

Copy link
Copy Markdown
ContributorAuthor

@nikomatsakis Okay, I'm making decent headway now I think. Any advice on writing confirm_trait_alias_candidate in traits/project.rs, or the VtableTraitAlias arm for resolve_associated_item in ty/instance.rs? I'm presuming they should behave somewhat similar to for the VtableImpl case, but some of the code is very tied down to VtableImpl and VtableImplData it seems...

Comment threadsrc/librustc/traits/project.rs Outdated
Comment threadsrc/librustc/traits/select.rs Outdated
@rust-highfive

This comment has been minimized.

@alexreg

Copy link
Copy Markdown
ContributorAuthor

@nikomatsakis Great. I’ll fix that quickly, but it’s basically already working fine. Any advice on trait objects? :-)

@rust-highfive

This comment has been minimized.

@rust-highfive

This comment has been minimized.

@rust-highfive

This comment has been minimized.

@rust-highfive

This comment has been minimized.

@rust-highfive

This comment has been minimized.

@rust-highfive

This comment has been minimized.

@rust-highfive

This comment has been minimized.

@rust-highfive

This comment has been minimized.

@alexreg

Copy link
Copy Markdown
ContributorAuthor

@nikomatsakis As long as you're happy with things now, LGTM!

@nikomatsakisnikomatsakis left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Somehow I expected a bit more on trait objects, but actually I can imagine how what you have would work! Nice! My main comment is that I'd like to see more exhaustive tests. I'll leave a comment below.

Comment threadsrc/libsyntax/parse/parser.rs Outdated
@nikomatsakis

nikomatsakis commented Oct 23, 2018

Copy link
Copy Markdown
Contributor

This looks awesome. I went through the RFC and drew up a list of tests covering everything that I saw in there. Do you think you could check whether all of these scenarios are being tested, and add those that are not?

Here is the list, inlined for convenience:

Things to test:

  • impl TraitAlias for u32 { .. } -- should error I think?
  • Kinds of trait aliases:
    • trait SendSyncAlias = Send + Sync
    • trait WhereSendAlias = where Self: Send
    • trait SendEqAlias<T> = Send where T: PartialEq<Self>
    • with associated items specified (trait U32Iterator = Iterator<Item = u32>)
    • with a lifetime parameter (trait PartialEqRef<'a, T> = PartialEq<&'a T> or something like that)
  • Test (ideally) each kind of where clause in each possible position:
    • Type parameter bounds (struct Foo<T: SendSyncAlias>, impl<T: SendSyncAlias> Foo for T)
    • Where clauses (struct Foo<T> where T: SendSyncAlias)
    • Impl trait (fn foo(x: &impl SendEqAlias<u32>) { 22_u32 == *x })
  • On functions, in particular, test each kind of where-clause, e.g., but especially "special stuff":
    • fn foo<T: U32Iterator>(x: &T) -> Option<u32> { x.next() }
    • fn foo<T: SendEqAlias<u32>>(x: &T) -> bool { 22_u32 == *x }
    • fn foo<T: SendSyncAlias>() { is_send_and_sync::<T>(); } fn is_send_and_sync<T: Send + Sync>() { }
  • Trait object:
    • trait Alias = Eq, &dyn Alias -- should fail object safety check
    • &dyn Alias -- check that it works, document behavior
    • impl dyn Alias -- check that it works, document behavior
    • Test that it implements the relevant traits:
      • trait Alias = Debug; fn foo(x: &dyn Debug) { bar(x); } fn bar<T: ?Sized + Debug>(x: &T) { }
    • with associated items specified:
      • trait U32Iterator = Iterator<Item = u32>; &dyn U32Iterator
    • without associated items specified:
      • trait Alias = Iterator; &dyn Alias -- error
  • Well-formedness conditions on alias definitions:
    • trait Foo<T: Send> { } trait Bar<T> = Foo<T>; // error -- requires T: Send
    • trait Foo<'a, T: 'a> { } trait Bar<'a, T> = Foo<T: 'a>; // error -- requires T: 'a
  • Extra conditions on trait aliases:
    • given trait Foo<T> { } trait Bar<T: Display> = Foo<T>; then:
      • fn is_foo<T: Bar<U>, U>() { /* ok */ }
      • fn is_bar<T: Bar<U>, U>() { /* NOT ok */ }
      • fn is_bar2<T: Bar<U>, U: Display>() { /* ok */ }
  • Associated type constraints:
    • given trait MyIterator = Iterator:
      • fn foo<T: MyIterator<Item = u32>>() { /* ok */ }
      • fn foo<T: MyIterator<Something = u32>>() { /* not ok */ }
    • given trait Foo { type Assoc; } trait Bar { type Assoc; } trait FooBar = Foo + Bar:
  • Other situations:

@nikomatsakis

Copy link
Copy Markdown
Contributor

Also, I think it'd be ideal to put all the trait-alias tests in a directory like src/test/ui/traits/rfc1733-trait-alias or something like that.

@alexreg

Copy link
Copy Markdown
ContributorAuthor

Also, I think it'd be ideal to put all the trait-alias tests in a directory like src/test/ui/traits/rfc1733-trait-alias or something like that.

Sure. Should test names still be prefixed with trait-alias- though?

@alexreg

Copy link
Copy Markdown
ContributorAuthor

Looks good. We already test most of these, but I'll go ensure they're all there now. Thanks for making the list.

  • impl TraitAlias for u32 { .. } -- should error I think?

  • Kinds of trait aliases:

    • trait SendSyncAlias = Send + Sync
    • trait WhereSendAlias = where Self: Send
    • trait SendEqAlias<T> = Send where T: PartialEq<Self>
    • with associated items specified (trait U32Iterator = Iterator<Item = u32>)
    • with a lifetime parameter (trait PartialEqRef<'a, T> = PartialEq<&'a T> or something like that)
  • Test (ideally) each kind of where clause in each possible position:

    • Type parameter bounds (struct Foo<T: SendSyncAlias>, impl<T: SendSyncAlias> Foo for T)
    • Where clauses (struct Foo<T> where T: SendSyncAlias)
    • Impl trait (fn foo(x: &impl SendEqAlias<u32>) { 22_u32 == *x })
  • On functions, in particular, test each kind of where-clause, e.g., but especially "special stuff":

    • fn foo<T: U32Iterator>(x: &T) -> Option<u32> { x.next() }
    • fn foo<T: SendEqAlias<u32>>(x: &T) -> bool { 22_u32 == *x }
    • fn foo<T: SendSyncAlias>() { is_send_and_sync::<T>(); } fn is_send_and_sync<T: Send + Sync>() { }
  • Trait object:

    • trait Alias = Eq, &dyn Alias -- should fail object safety check

    • &dyn Alias -- check that it works, document behavior

    • impl dyn Alias -- check that it works, document behavior

    • Test that it implements the relevant traits:

      • trait Alias = Debug; fn foo(x: &dyn Debug) { bar(x); } fn bar<T: ?Sized + Debug>(x: &T) { }
    • with associated items specified:

      • trait U32Iterator = Iterator<Item = u32>; &dyn U32Iterator
    • without associated items specified:

      • trait Alias = Iterator; &dyn Alias -- error
  • Well-formedness conditions on alias definitions:

    • trait Foo<T: Send> { } trait Bar<T> = Foo<T>; // error -- requires T: Send
    • trait Foo<'a, T: 'a> { } trait Bar<'a, T> = Foo<T: 'a>; // error -- requires T: 'a
  • Extra conditions on trait aliases:

    • given trait Foo<T> { } trait Bar<T: Display> = Foo<T>; then:

      • fn is_foo<T: Bar<U>, U>() { /* ok */ }
      • fn is_bar<T: Bar<U>, U>() { /* NOT ok */ }
      • fn is_bar2<T: Bar<U>, U: Display>() { /* ok */ }
  • Associated type constraints:

    • given trait MyIterator = Iterator:

      • fn foo<T: MyIterator<Item = u32>>() { /* ok */ }
      • fn foo<T: MyIterator<Something = u32>>() { /* not ok */ }
    • given trait Foo { type Assoc; } trait Bar { type Assoc; } trait FooBar = Foo + Bar:

  • Other situations:

@bors

bors commented Nov 2, 2018

Copy link
Copy Markdown
Collaborator

⌛ Testing commit 388f9be with merge 316c35e...

@Centril

Copy link
Copy Markdown
Contributor

Seems there are some issues with the PR? (@alexreg can elaborate...)
So let's try to:

@bors r-

@bors

This comment has been minimized.

@oli-obk

Copy link
Copy Markdown
Contributor

@bors r-

@borsbors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Nov 2, 2018
@emilyalbini

Copy link
Copy Markdown
Member

@bors retry r-

@borsbors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 2, 2018
@emilyalbiniemilyalbini 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Nov 2, 2018
@oli-obk

Copy link
Copy Markdown
Contributor

@bors r=nikomatsakis

this was correct all along, some RUST_LOG debugging on @alexreg's side caused the failure they were seeing

@bors

bors commented Nov 3, 2018

Copy link
Copy Markdown
Collaborator

📌 Commit 4171685 has been approved by nikomatsakis

@borsbors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 3, 2018
@bors

bors commented Nov 3, 2018

Copy link
Copy Markdown
Collaborator

⌛ Testing commit 4171685 with merge a3f0f51...

bors added a commit that referenced this pull request Nov 3, 2018
Implement trait aliases (RFC 1733)
Extends groundwork done in #45047, and fully implements rust-lang/rfcs#1733.
CC @durka@nikomatsakis
@bors

bors commented Nov 3, 2018

Copy link
Copy Markdown
Collaborator

☀️ Test successful - status-appveyor, status-travis
Approved by: nikomatsakis
Pushing a3f0f51 to master...

@bors
bors merged commit 4171685 into rust-lang:masterNov 3, 2018
@dtolnaydtolnay mentioned this pull request Nov 7, 2018
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-trait-systemArea: Trait systemF-trait_alias`#![feature(trait_alias)]`S-waiting-on-borsStatus: Waiting on bors to run and complete tests. Bors will change the label on completion.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants

@alexreg@rust-highfive@nikomatsakis@bors@Centril@oli-obk@emilyalbini@zackmdavis@petrochenkov