Implement coercions between &pin (mut|const) T and &(mut) T when T: Unpin#149130
Implement coercions between &pin (mut|const) T and &(mut) T when T: Unpin#149130frank-king wants to merge 2 commits intorust-lang:mainfrom
&pin (mut|const) T and &(mut) T when T: Unpin#149130Conversation
This comment has been minimized.
This comment has been minimized.
29a89ae to
92596db
Compare
|
Some changes occurred in need_type_info.rs cc @lcnr Some changes occurred in src/tools/clippy cc @rust-lang/clippy Some changes occurred in exhaustiveness checking cc @Nadrieril |
|
r? @nnethercote rustbot has assigned @nnethercote. Use |
|
I am not a good person to review this. How about... r? @lcnr |
|
r? types |
|
r? types |
|
☔ The latest upstream changes (presumably #148602) made this pull request unmergeable. Please resolve the merge conflicts. |
92596db to
500d786
Compare
This comment has been minimized.
This comment has been minimized.
|
☔ The latest upstream changes (presumably #150115) made this pull request unmergeable. Please resolve the merge conflicts. |
There was a problem hiding this comment.
Incredibly sorry about the review latency here.
Overall, the changes look not bad. That being said, there are two very mechanical changes that would be useful to split into a separate PR (or really, a separate commit is fine, but I prefer a separate PR since they stand alone well).
|
Reminder, once the PR becomes ready for a review, use |
500d786 to
2ef4a8a
Compare
This comment has been minimized.
This comment has been minimized.
|
I've split the two refactors to separated PRs:
and a post-refactor PR #151932 (remove @rustbot ready |
…r=jackh726 refactor: remove `Ty::pinned_ref` in favor of `Ty::maybe_pinned_ref` Also returns the `Region` of the reference type in `Ty::maybe_pinned_Ref`. Part of rust-lang#149130. r? jackh726
…r=jackh726 refactor: add an `enum DerefAdjustKind` in favor of `Option<OverloadedDeref>` Part of rust-lang#149130. r? jackh726
This comment has been minimized.
This comment has been minimized.
2ef4a8a to
8dd903c
Compare
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
8dd903c to
0422097
Compare
| if self.tcx.features().pin_ergonomics() | ||
| && a.pinned_ty().is_some_and(|ty| ty.is_ref()) | ||
| && let Ok(coerce) = self.commit_if_ok(|_| self.coerce_maybe_pinned_ref(a, b)) | ||
| { | ||
| return Ok(coerce); | ||
| } |
There was a problem hiding this comment.
I am sort of thinking that this code (and the lines added under the ty::Adt arm, should go in coerce_to_ref and coerce_to_pin_ref respectively - keeping this function relatively clean.
There was a problem hiding this comment.
This also would help to eliminate some span_bugs from coerce_maybe_pinned_ref
| let Some((a_ty, a_pinnedness, a_mutbl, a_region)) = a.maybe_pinned_ref() else { | ||
| span_bug!(span, "expect pinned reference or reference, found {:?}", a); | ||
| }; | ||
| let Some((_b_ty, b_pinnedness, b_mutbl, _b_region)) = b.maybe_pinned_ref() else { | ||
| span_bug!(span, "expect pinned reference or reference, found {:?}", b); | ||
| }; |
There was a problem hiding this comment.
Something feels a bit off here to me...I like have one function that covers "all the pinned ref" coercions - but simultaneously all the span_bugs here scream this function being ripe for getting misused or logic changing elsewhere but not updated here.
I sort of wonder if everything before let mut coerce = self.unify_and needs to be done in the calling functions, and necessarily bits get passed in.
There was a problem hiding this comment.
I'm not sure if I fully get your ideas. I removed the coerce_maybe_pinned_ref and split it into coerce_to_pin_ref and coerce_pin_ref_to_ref.
Theoretically, coerce_pin_ref_to_ref can be merged into coerce_to_ref, but I have no idea how to add an autoref before the autoderef (I mean, the Autoderef iterator handles &mut Pin<&mut T> to &mut T and &Pin<&T> to &T well, but for Pin<&mut T> and Pin<&T>, there need to be an autoref at the beginning). I'm also not sure if doing that would mess up the existing code.
There was a problem hiding this comment.
Skimming over, I think your changes look better, but I'm noticing there are test changes, so there are semantic changes i need to think about. Will do a proper review this week.
Use `coerce_to_pin_ref` and `coerce_pin_ref_to_ref` instead.
bc088dd to
ae0c73c
Compare
This allows the following (mutual) coercions when
T: Unpin:&T<->Pin<&T>&mut T<->Pin<&mut T>&mut T->Pin<&T>Pin<&mut T>->&TPart of Pin Ergonomics.