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.

Sign up for GitHub

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

Jump to bottom

Trait upcasting coercion (part2) #87515

Merged
merged 5 commits into from
Aug 3, 2021
Merged

Trait upcasting coercion (part2) #87515

merged 5 commits into from
Aug 3, 2021

Conversation

crlf0710
Copy link
Member

This is the second part of trait upcasting coercion implementation.

Currently this is blocked on #86264 .

The third part might be implemented using unsafety checking

r? @bjorn3

@rust-highfive
Copy link
Collaborator

Some changes occured to rustc_codegen_cranelift

cc @bjorn3

Some changes occured to the CTFE / Miri engine

cc @rust-lang/miri

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 27, 2021
@crlf0710 crlf0710 added S-blocked Status: Blocked on something else such as an RFC or other implementation work. F-trait_upcasting `#![feature(trait_upcasting)]` and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 27, 2021
@crlf0710 crlf0710 mentioned this pull request Jul 27, 2021
Open
21 tasks
@crlf0710 crlf0710 changed the title Trait upcasting coercion part2 Trait upcasting coercion (part2) Jul 27, 2021
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@bors
Copy link
Contributor

bors commented Jul 27, 2021

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

@crlf0710
Copy link
Member Author

#86264 has landed, now this is unblocked. I'll address the review comments very soon, and add more tests.

@crlf0710 crlf0710 added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-blocked Status: Blocked on something else such as an RFC or other implementation work. labels Jul 31, 2021
@crlf0710 crlf0710 added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 31, 2021
@crlf0710 crlf0710 requested review from RalfJung and bjorn3 July 31, 2021 15:26
@crlf0710
Copy link
Member Author

Rebased, addressed the review comments and pull in some tests and document from previous work. This is ready for review again now.

bjorn3
bjorn3 requested changes Jul 31, 2021
View reviewed changes
compiler/rustc_codegen_cranelift/src/unsize.rs
.expect("unsized_info: missing principal trait for trait upcasting coercion");
let principal_b = data_b
.principal()
.expect("unsized_info: missing principal trait for trait upcasting coercion");
Copy link
Member

Choose a reason for hiding this comment

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

Isn't the principal non-existent when the trait object only consists of traits like Send, Sync or Sized?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, if the principal is non-existent, either it's not upcasting coercion, like &dyn Send+Sync to &dyn Send will be a simple case (and will take the early return code path), or it will be rejected by type-checking, like dyn Foo + Send to dyn Send will be rejected.

I added some comments within the code to clarify.

compiler/rustc_codegen_cranelift/src/unsize.rs Outdated Show resolved Hide resolved
compiler/rustc_codegen_ssa/src/base.rs Outdated Show resolved Hide resolved
@crlf0710
Copy link
Member Author

crlf0710 commented Aug 1, 2021

Thanks for the comments! Updated.

@crlf0710
Copy link
Member Author

crlf0710 commented Aug 2, 2021

@RalfJung Thanks for the review! I've addressed the comments.

bjorn3
bjorn3 approved these changes Aug 2, 2021
View reviewed changes
Copy link
Member

@bjorn3 bjorn3 left a comment

Choose a reason for hiding this comment

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

The codegen part LGTM apart from a minor nit. I will leave @RalfJung to approve the miri part. I don't see any obvious problems with the trait selection code, but given that I am not familiar with it, maybe @nikomatsakis want to take a look?

@crlf0710
Copy link
Member Author

crlf0710 commented Aug 2, 2021

Updated and addressed the comments.

For the trait selection related stuff, I think niko has been and will be on vacation in the next two weeks. if it's possible i'd prefer more detailed review of this part be defered a little, and we'll revisit it after this FIXME is solved.

@crlf0710 crlf0710 requested a review from RalfJung August 2, 2021 15:33
@RalfJung
Copy link
Member

RalfJung commented Aug 2, 2021

👍 for the Miri engine changes.

bjorn3
bjorn3 approved these changes Aug 2, 2021
View reviewed changes
Copy link
Member

@bjorn3 bjorn3 left a comment

Choose a reason for hiding this comment

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

For the trait selection related stuff, I think niko has been and will be on vacation in the next two weeks. if it's possible i'd prefer more detailed review of this part be defered a little, and we'll revisit it after this FIXME is solved.

Sure. r=me with the nit resolved.

compiler/rustc_codegen_ssa/src/base.rs Show resolved Hide resolved
@crlf0710
Copy link
Member Author

crlf0710 commented Aug 2, 2021

@bjorn3 Updated with the comment above addressed!

@bjorn3
Copy link
Member

bjorn3 commented Aug 2, 2021

@bors r+

@bors
Copy link
Contributor

bors commented Aug 2, 2021

📌 Commit 3cb625e has been approved by bjorn3

@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 Aug 2, 2021
@bors
Copy link
Contributor

bors commented Aug 3, 2021

⌛ Testing commit 3cb625e with merge c6bc102...

@bors
Copy link
Contributor

bors commented Aug 3, 2021

☀️ Test successful - checks-actions
Approved by: bjorn3
Pushing c6bc102 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Aug 3, 2021
@bors bors merged commit c6bc102 into rust-lang:master Aug 3, 2021
@rustbot rustbot added this to the 1.56.0 milestone Aug 3, 2021
@bors bors mentioned this pull request Aug 3, 2021
Closed
@tmiasko tmiasko mentioned this pull request Aug 4, 2021
Merged
bjorn3 pushed a commit to bjorn3/rust that referenced this pull request Aug 6, 2021
@bors
Auto merge of rust-lang#87515 - crlf0710:trait_upcasting_part2, r=bjorn3
0f83ac1 
Trait upcasting coercion (part2)

This is the second part of trait upcasting coercion implementation.

Currently this is blocked on rust-lang#86264 .

The third part might be implemented using unsafety checking

r? `@bjorn3`
@bjorn3 bjorn3 mentioned this pull request Aug 6, 2021
Merged
JohnTitor added a commit to JohnTitor/rust that referenced this pull request Aug 6, 2021
@JohnTitor
Rollup merge of rust-lang#87816 - bjorn3:sync_cg_clif-2021-08-06, r=b…
0de0b88 
…jorn3

Sync rustc_codegen_cranelift

05677b6 removes two assertions that should have been removed in rust-lang#87515. They are no longer correct and trigger while compiling the sysroot.

r? `@ghost`

`@rustbot` label +A-codegen +A-cranelift +T-compiler
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bjorn3 bjorn3 bjorn3 approved these changes

@RalfJung RalfJung Awaiting requested review from RalfJung

Assignees

@bjorn3 bjorn3

Labels
F-trait_upcasting `#![feature(trait_upcasting)]` merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
1.56.0
Development

Successfully merging this pull request may close these issues.
8 participants
@crlf0710 @rust-highfive @rust-log-analyzer @bors @RalfJung @bjorn3 @rustbot @alexreg