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

Various fixes to trait alias feature #55994

Closed
wants to merge 12 commits into from

Conversation

alexreg
Copy link
Contributor

@alexreg alexreg commented Nov 16, 2018

  1. Disallows multiple regular traits in trait objects when trait aliases used, ensuring code like the following fails to compile.

    #![feature(trait_alias)]
    
    trait Foo = std::io::Read + std::io::Write;
    
    fn main() {
        let foo: Box<dyn Foo>;
    }
  2. Implemented cross-crate usage of trait aliases. They know get properly encoded and decoded in crate metadata. Added test.

  3. Introduces the DUPLICATE_AUTO_TRAITS_IN_BOUNDS lint (warn-by-default).

  4. Added test for issue ICE with trait aliases and use items #56488. (Appears to be false positive.)

Issue fixes

CC @nikomatsakis @scalexm

@rust-highfive
Copy link
Collaborator

r? @nikomatsakis

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

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

This comment has been minimized.

@alexreg alexreg changed the title [WIP] Disallows multiple regular traits in trait objects when trait aliases used [WIP] Disallow multiple regular traits in trait objects when trait aliases used Nov 16, 2018
@alexreg
Copy link
Contributor Author

alexreg commented Nov 16, 2018

I still want to improve error messages a bit, but this is nearly ready. Feel free to review.

@rust-highfive

This comment has been minimized.

@alexreg alexreg changed the title [WIP] Disallow multiple regular traits in trait objects when trait aliases used Disallow multiple regular traits in trait objects when trait aliases used Nov 17, 2018
@alexreg
Copy link
Contributor Author

alexreg commented Nov 17, 2018

Ready for review and (hopefully) merge now, I think.

@rust-highfive

This comment has been minimized.

Copy link
Contributor

@nikomatsakis nikomatsakis left a comment

Choose a reason for hiding this comment

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

Looks great! Left one nit (on the test)

src/librustc/traits/util.rs Show resolved Hide resolved
src/test/ui/error-codes/E0225.rs Show resolved Hide resolved
@nikomatsakis
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Nov 20, 2018

📌 Commit c180d7ce09bba0ab7c97365e2a1d1c33face2618 has been approved by nikomatsakis

@bors bors 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-review Status: Awaiting review from the assignee but also interested parties. labels Nov 20, 2018
@bors
Copy link
Contributor

bors commented Nov 23, 2018

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

@bors bors 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 23, 2018
@alexreg
Copy link
Contributor Author

alexreg commented Nov 24, 2018

What on Earth... this didn't merge yet?!

@rust-highfive

This comment has been minimized.

@Mark-Simulacrum
Copy link
Member

@bors r=nikomatsakis

@bors
Copy link
Contributor

bors commented Nov 25, 2018

📌 Commit 4fb40da has been approved by nikomatsakis

@bors bors 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 25, 2018
@alexreg alexreg force-pushed the fix-trait-alias-1 branch from 174915c to d56ef2d Compare January 3, 2019 04:50
@rust-highfive

This comment has been minimized.

@alexreg alexreg force-pushed the fix-trait-alias-1 branch from d56ef2d to 14e8c6a Compare January 3, 2019 15:11
@alexreg alexreg force-pushed the fix-trait-alias-1 branch from 14e8c6a to d246b2c Compare January 3, 2019 17:15
@rust-lang rust-lang deleted a comment from rust-highfive Jan 3, 2019
@alexreg
Copy link
Contributor Author

alexreg commented Jan 3, 2019

@nikomatsakis Okay, finally reading for r+ I think! Linting has been removed; that will be addressed in a separated PR.

src/librustc/traits/util.rs Show resolved Hide resolved
src/librustc/traits/util.rs Show resolved Hide resolved
src/librustc_typeck/astconv.rs Show resolved Hide resolved
@bors
Copy link
Contributor

bors commented Jan 5, 2019

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

@stokhos
Copy link

stokhos commented Jan 14, 2019

Ping from triage @alexreg : Have you been able to make any progress on this PR?

@alexreg
Copy link
Contributor Author

alexreg commented Jan 14, 2019

@stokhos 2 and 4 have been factored out into a separate PR by @nikomatsakis (and merged), while he is still working on 1, because it's slightly non-trivial. 3 is postponed for now.

Perhaps we should close this PR?

@nikomatsakis
Copy link
Contributor

Let's go ahead and close this PR, the relevant commits are in other branches by now.

Centril added a commit to Centril/rust that referenced this pull request Jan 19, 2019
…nikomatsakis

make trait-aliases work across crates

This is rebase of a small part of @alexreg's PR rust-lang#55994. It focuses just on the changes that integrate trait aliases properly into crate metadata, excluding the stylistic edits and the trait objects.

The stylistic edits I also rebased and can open a separate PR.

The trait object stuff I found challenging and decided it basically needed to be reimplemented. For now I've excluded it.

Since this is really @alexreg's work (I really just rebased) I am going to make it r=me once it is working.

Fixes rust-lang#56488.
Fixes rust-lang#57023.
bors added a commit that referenced this pull request May 20, 2019
…=oli-obk

Ban multi-trait objects via trait aliases

Obviously, multi-trait objects are not normally supported, so they should not be supported via trait aliases.

This has been factored out from the previous PR #55994 (see point 1).

r? @Centril

CC @nikomatsakis
bors added a commit that referenced this pull request May 22, 2019
…=oli-obk

Ban multi-trait objects via trait aliases

Obviously, multi-trait objects are not normally supported, so they should not be supported via trait aliases.

This has been factored out from the previous PR #55994 (see point 1).

r? @Centril

CC @nikomatsakis

------------------

### RELNOTES:

We now allow `dyn Send + fmt::Debug` with equivalent semantics to `dyn fmt::Debug + Send`.
That is, the order of the mentioned traits does not matter wrt. principal/not-principal traits.
This is a small change that might deserve a mention in the blog post because it is a language change but most likely not.

See https://github.com/rust-lang/rust/blob/ce2ee305f9165c037ecddddb5792588a15ff6c37/src/test/ui/traits/wf-trait-object-reverse-order.rs.

// @Centril
@alexreg alexreg added A-trait-system Area: Trait system F-trait_alias `#![feature(trait_alias)]` labels Nov 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-trait-system Area: Trait system F-trait_alias `#![feature(trait_alias)]` S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
10 participants