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

Properly handle Pin<&mut dyn* Trait> receiver in codegen #104594

Merged
merged 3 commits into from
Nov 24, 2022
Merged

Properly handle Pin<&mut dyn* Trait> receiver in codegen #104594

merged 3 commits into from
Nov 24, 2022

Conversation

compiler-errors
Copy link
Member

@compiler-errors compiler-errors commented Nov 19, 2022 •
edited
Loading

This ensures we can actually await a dyn* Future, which seems important for async fn in dyn trait.

Also, disable dyn* trait upcasting. It's not exactly complete right now, and can cause strange ICEs for no reason -- nobody's using it either. I thought it was cute to implement when I did it, but I didn't think about how it interacts structurally with CoerceUnsized correctly.

Fixes #104794, presumably removing dyn* upcasting and its CoerceUnsized issues does the trick.

@rustbot
Copy link
Collaborator

rustbot commented Nov 19, 2022

r? @estebank

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

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Nov 19, 2022
@bors
Copy link
Contributor

bors commented Nov 22, 2022

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

estebank
estebank reviewed Nov 22, 2022
View reviewed changes
compiler/rustc_hir_typeck/src/coercion.rs Outdated
Comment on lines 764 to 765
// FIXME(dyn-star): `dyn*` upcasting is currently broken and ICEy.
/* else if !self.tcx().features().trait_upcasting {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm uncomfortable leaving this commented out.

Copy link
Member Author

Choose a reason for hiding this comment

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

Are you asking for it to be deleted instead? Because I can do that.

estebank
estebank reviewed Nov 22, 2022
View reviewed changes
compiler/rustc_trait_selection/src/traits/select/candidate_assembly.rs
@@ -782,9 +782,7 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {

match (source.kind(), target.kind()) {
// Trait+Kx+'a -> Trait+Ky+'b (upcasts).
(&ty::Dynamic(ref data_a, _, dyn_a), &ty::Dynamic(ref data_b, _, dyn_b))
if dyn_a == dyn_b =>
Copy link
Contributor

Choose a reason for hiding this comment

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

Surprised by this removal

Copy link
Contributor

Choose a reason for hiding this comment

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

I see the more specific pattern here and in the next file, but doesn't this code have to run for non dyn*?

Copy link
Member Author

Choose a reason for hiding this comment

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

Can you be more specific? Did you see the PR description that mentions "Also, disable dyn* trait upcasting"?

Because all this disables is CoerceUnsized implementations from dyn* Trait to dyn* SuperTrait, which cannot be implemented with CoerceUnsized and need to be reworked.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see. r=me after rebasing

estebank
estebank reviewed Nov 22, 2022
View reviewed changes
src/test/ui/dyn-star/upcast.rs Outdated
@@ -1,7 +1,6 @@
// run-pass
// known-bug: unknown
Copy link
Contributor

Choose a reason for hiding this comment

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

poetry

Copy link
Member Author

Choose a reason for hiding this comment

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

I can file a bug for this.

Copy link
Contributor

Choose a reason for hiding this comment

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

If you do, could you also add it to #102425?

estebank
estebank approved these changes Nov 22, 2022
View reviewed changes
Copy link
Contributor

@estebank estebank left a comment

Choose a reason for hiding this comment

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

The changes look reasonable, but might there be someone already involved in dyn* that can double check?

@compiler-errors
Copy link
Member Author

compiler-errors commented Nov 22, 2022 •
edited
Loading

but might there be someone already involved in dyn* that can double check?

r? @eholk

@rustbot rustbot assigned eholk and unassigned estebank Nov 22, 2022
eholk
eholk approved these changes Nov 23, 2022
View reviewed changes
Copy link
Contributor

@eholk eholk left a comment

Choose a reason for hiding this comment

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

Looks good to me. I added a couple small comments or questions, but I think merging as-is is fine, so r=me if you're ready.

src/test/ui/dyn-star/dispatch-on-pin-mut.rs Outdated Show resolved Hide resolved
src/test/ui/dyn-star/upcast.rs Outdated
@@ -1,7 +1,6 @@
// run-pass
// known-bug: unknown
Copy link
Contributor

Choose a reason for hiding this comment

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

If you do, could you also add it to #102425?

@compiler-errors compiler-errors mentioned this pull request Nov 24, 2022
Closed
@compiler-errors compiler-errors mentioned this pull request Nov 24, 2022
Open
@compiler-errors
Copy link
Member Author

@bors r=eholk,estebank

@bors
Copy link
Contributor

bors commented Nov 24, 2022

📌 Commit b60b76c has been approved by eholk,estebank

It is now in the queue for this repository.

@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 24, 2022
@matthiaskrgr matthiaskrgr mentioned this pull request Nov 24, 2022
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 24, 2022
@bors
Auto merge of rust-lang#104809 - matthiaskrgr:rollup-8abjdwh, r=matth…
1dda298 
…iaskrgr

Rollup of 9 pull requests

Successful merges:

 - rust-lang#103908 (Suggest `.clone()` or `ref binding` on E0382)
 - rust-lang#104517 (Throw error on failure in loading llvm-plugin)
 - rust-lang#104594 (Properly handle `Pin<&mut dyn* Trait>` receiver in codegen)
 - rust-lang#104742 (Make `deref_into_dyn_supertrait` lint the impl and not the usage)
 - rust-lang#104753 (Pass `InferCtxt` to `DropRangeVisitor` so we can resolve vars)
 - rust-lang#104771 (Add regression test for issue rust-lang#99938)
 - rust-lang#104772 (Small accessibility improvements)
 - rust-lang#104775 (Use ObligationCtxt::normalize)
 - rust-lang#104778 (:arrow_up: rust-analyzer)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit b43c2e7 into rust-lang:master Nov 24, 2022
@rustbot rustbot added this to the 1.67.0 milestone Nov 24, 2022
@compiler-errors compiler-errors deleted the dyn-star-rcvr branch August 11, 2023 19:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@estebank estebank estebank approved these changes

@eholk eholk eholk approved these changes

Assignees

@eholk eholk

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.67.0
Development

Successfully merging this pull request may close these issues.

ICE: bx.cx().is_backend_scalar_pair(cast): dyn star
5 participants
@compiler-errors @rustbot @bors @eholk @estebank