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 checking of auto trait bounds in trait objects. #45772

Merged
merged 2 commits into from
Nov 11, 2017

Conversation

leoyvens
Copy link
Contributor

@leoyvens leoyvens commented Nov 5, 2017

Any auto trait is allowed in trait object bounds. Fix duplicate check of type and lifetime parameter count, which we were emitting twice.

Note: This was the last use of Send in the compiler, meaning after a new stage0 we could remove the send lang item.

Any auto trait is allowed in trait object bounds.
Fix duplicate check of type and lifetime parameter count.
@rust-highfive
Copy link
Collaborator

r? @nikomatsakis

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

@kennytm kennytm added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-lang Relevant to the language team, which will review and decide on the PR/issue. labels Nov 5, 2017
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.

Code looks nice.

} else {
false
}
Def::Trait(trait_did) if tcx.trait_is_auto(trait_did) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

In principle we could easily extend this to arbitrary marker traits, I suppose?

@nikomatsakis
Copy link
Contributor

nikomatsakis commented Nov 6, 2017

Note: This was the last use of Send in the compiler, meaning after a new stage0 we could remove the send lang item.

Could we do it now, and just use #[cfg_attr(stage0, lang)]?

Oh, I guess that's annoying, for same reason as before. Forgive me, old habits die hard. =)

@nikomatsakis
Copy link
Contributor

OK -- thought this over. I don't think extending to arbitrary marker traits is something we can do without an RFC. But auto traits should be ok, since defining new auto traits is still unstable.

@nikomatsakis
Copy link
Contributor

Well, there are a few auto traits that are stable and are not Send nor Sync (e.g. UnwindSafe). I'm going to ask @rust-lang/lang team for an FCP here, just to play it safe.

@nikomatsakis
Copy link
Contributor

@bors fcp merge

I'd like to propose that we merge this PR. It has the effect of enabling trait objects with arbitrary auto traits composed together. So, whereas before you could only do dyn (Trait + Send), you can now do dyn (Trait + Send + UnwindSafe), for example. I think this makes sense logically, and it fits with the oft-expressed intention to permit arbitrary object-safe traits to be composed (e.g., dyn (Foo + Bar)).

I don't feel an RFC is needed for this particular PR. However, to go further and support composition of arbitrary object-safe traits, I would like to have an RFC. This is not because I expect us to be against the basic idea, but just because there are various approaches we might take (e.g., I favor multiple vtables, but a case can be made for a single composed vtable) and I think it would benefit from a proper design cycle.

The change as implemented is "insta-stable". If we wanted, we could also make arbitrary extensions to the capabilities here, but require a feature-gate to go beyond Send/Sync. This might actually be better, so feel free to object if you think it would be.

@leoyvens
Copy link
Contributor Author

leoyvens commented Nov 6, 2017

@nikomatsakis I think you meant to invoke @rfcbot.

@withoutboats
Copy link
Contributor

@rfcbot fcp merge

@rfcbot
Copy link

rfcbot commented Nov 6, 2017

Team member @withoutboats has proposed to merge this. The next step is review by the rest of the tagged teams:

No concerns currently listed.

Once these reviewers reach consensus, this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added the proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. label Nov 6, 2017
@eddyb
Copy link
Member

eddyb commented Nov 6, 2017

Oh, I guess that's annoying, for same reason as before. Forgive me, old habits die hard. =)

Wait, what's the explanation here?

@leoyvens
Copy link
Contributor Author

leoyvens commented Nov 6, 2017

@eddyb If we do it I imagine that when the stage0 is updated the build would start failing because the new stage0 dosen't know the send lang item.

@eddyb
Copy link
Member

eddyb commented Nov 6, 2017

@leodasvacas But that's standard procedure - whoever updates the bootstrap compiler removes all the cfg{,_attr}(stage0) annotations.

@leoyvens
Copy link
Contributor Author

leoyvens commented Nov 6, 2017

@eddyb Ah, I was ignorant on the process here. I'll do it then.

@cramertj
Copy link
Member

cramertj commented Nov 6, 2017

@rfcbot reviewed

@kennytm kennytm added S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 7, 2017
It's completely unused.
@leoyvens
Copy link
Contributor Author

leoyvens commented Nov 7, 2017

Removed the send lang item.

@eddyb eddyb added the relnotes Marks issues that should be documented in the release notes of the next release. label Nov 7, 2017
@rfcbot
Copy link

rfcbot commented Nov 9, 2017

🔔 This is now entering its final comment period, as per the review above. 🔔

@rfcbot rfcbot added final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. and removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. labels Nov 9, 2017
@nikomatsakis
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Nov 9, 2017

📌 Commit 7995f87 has been approved by nikomatsakis

@nikomatsakis
Copy link
Contributor

@bors r-

@nikomatsakis
Copy link
Contributor

Guess I should show some decorum and wait a bit for FCP.

@nikomatsakis
Copy link
Contributor

@bors r+

I'm impatient. We can always back it out if something dramatic comes up.

@bors
Copy link
Contributor

bors commented Nov 10, 2017

📌 Commit 7995f87 has been approved by nikomatsakis

@kennytm kennytm 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-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). labels Nov 10, 2017
@bors
Copy link
Contributor

bors commented Nov 11, 2017

⌛ Testing commit 7995f87 with merge e33731286e7e57b67375218bb3024caa93d4a530...

@bors
Copy link
Contributor

bors commented Nov 11, 2017

💔 Test failed - status-travis

@kennytm
Copy link
Member

kennytm commented Nov 11, 2017

@bors retry

[01:13:14] test process::tests::test_process_output_fail_to_start has been running for over 60 seconds


No output has been received in the last 30m0s, this potentially indicates a stalled build or something wrong with the build itself.
Check the details on how to adjust your build configuration on: https://docs.travis-ci.com/user/common-build-problems/#Build-times-out-because-no-output-was-received

The build has been terminated

@bors
Copy link
Contributor

bors commented Nov 11, 2017

⌛ Testing commit 7995f87 with merge e528c7e...

bors added a commit that referenced this pull request Nov 11, 2017
…r=nikomatsakis

Fix checking of auto trait bounds in trait objects.

Any auto trait is allowed in trait object bounds. Fix duplicate check of type and lifetime parameter count, which we were [emitting twice](https://play.rust-lang.org/?gist=37dbbdbbec62dec423bb8f6d92f137cc&version=stable).

Note: This was the last use of `Send` in the compiler, meaning after a new `stage0` we could remove the `send` lang item.
@bors
Copy link
Contributor

bors commented Nov 11, 2017

💔 Test failed - status-travis

@kennytm
Copy link
Member

kennytm commented Nov 11, 2017

@bors retry

dist-arm-linux 3-hour timed out?

@bors
Copy link
Contributor

bors commented Nov 11, 2017

⌛ Testing commit 7995f87 with merge 69ee5a8...

bors added a commit that referenced this pull request Nov 11, 2017
…r=nikomatsakis

Fix checking of auto trait bounds in trait objects.

Any auto trait is allowed in trait object bounds. Fix duplicate check of type and lifetime parameter count, which we were [emitting twice](https://play.rust-lang.org/?gist=37dbbdbbec62dec423bb8f6d92f137cc&version=stable).

Note: This was the last use of `Send` in the compiler, meaning after a new `stage0` we could remove the `send` lang item.
@bors
Copy link
Contributor

bors commented Nov 11, 2017

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

@bors bors merged commit 7995f87 into rust-lang:master Nov 11, 2017
@leoyvens
Copy link
Contributor Author

@gbutler69 It was removed because it was no longer in use. Keeping lang items to a minimal is good.

@leoyvens leoyvens deleted the fix-auto-bounds-in-trait-objects branch November 17, 2017 18:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. relnotes Marks issues that should be documented in the release notes of the next release. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.