-
-
Notifications
You must be signed in to change notification settings - Fork 14.4k
Experiment: Reborrow traits #151753
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
base: main
Are you sure you want to change the base?
Experiment: Reborrow traits #151753
Conversation
Signed-off-by: Xiangfei Ding <[email protected]>
This comment has been minimized.
This comment has been minimized.
e4c08f7 to
3b1957a
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Some changes occurred in cc @BoxyUwU Some changes occurred in match checking cc @Nadrieril This PR changes MIR cc @oli-obk, @RalfJung, @JakobDegen, @vakaras This PR changes rustc_public cc @oli-obk, @celinval, @ouz-a, @makai410 Some changes occurred to MIR optimizations cc @rust-lang/wg-mir-opt Some changes occurred to constck cc @fee1-dead Some changes occurred to the CTFE / Miri interpreter cc @rust-lang/miri Some changes occurred in compiler/rustc_codegen_cranelift cc @bjorn3 Some changes occurred to the CTFE machinery Some changes occurred in src/tools/clippy cc @rust-lang/clippy |
|
r? compiler |
| /// Wraps a value in an unsafe binder. | ||
| WrapUnsafeBinder(Operand<'tcx>, Ty<'tcx>), | ||
|
|
||
| /// A reborrowable type being reborrowed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please explain the runtime behavior of this operation.
The interpreter implementation seems to indicate that this is equivalent to a normal assignment of Copy($place). Why is it even a separate statement?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, my apologies, I definitely should've added more information around this: I added a link to the experiment tracking issue into the opening comment which contains some more information and links but basically: in the trivial case (#[repr(transparent)]-compatible memory layout of types and a single lifetime) Reborrow is a Copy plus extra borrow checking analysis to perform the same reborrowing operation as happens with &'a mut T, but now on some user-defined ADT struct Custom<'a>(..). CoerceShared is then the &'a mut T -> &'a T equivalent of this operation and thus also requires a user-defined target type.
This PR does not implement any non-trivial cases so in both cases the runtime effect is exactly equivalent to a Copy, but future expansions should make CoerceShared possible even if the layouts of the source and target types do not match, including the possibility for the target type to drop out some fields from the source type. (This has a requested use-case where a custom exclusive reference type has some write-time metadata which is then not necessary in a read-only reference.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a link to the experiment tracking issue
That link is broken btw.
future expansions should make CoerceShared possible even if the layouts of the source and target types do not match, including the possibility for the target type to drop out some fields from the source type.
That sounds like an operation that is too complicated to be a single MIR statement, it should instead be encoded as a sequence of simpler operations. Otherwise every MIR consumer has to re-implement all that logic, partially defeating the point of having an IR like MIR in the first place.
| } | ||
|
|
||
| adjustment::Adjust::GenericReborrow(_reborrow) => { | ||
| // Do the magic! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh we should call delegates. See the ReborrowPin arm and that is basically what we want to do.
And eventually we should be able to subsume ReborrowPin case hopefully.
| // `&mut T: DerefMut` tho, so it's kinda moot. | ||
| } | ||
| Adjust::GenericReborrow(_) => { | ||
| // No effects to enforce here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should do something here. The type would change after a generic reborrow, ie the CoerceShared case.
|
Where can I read context on what CoerceShared is/why it exists, and what the design for reborrow traits is |
| block.and(PlaceBuilder::from(temp)) | ||
| } | ||
| ExprKind::Reborrow { source: _, mutability: _, ty: _ } => { | ||
| todo!(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should generate a temporary. Therefore, we can merge this case one above, in other words build a temporary place, so that the reborrowing receives its own region "token."
I added a link to the experiment tracking issue but put it very shortly: |
|
I think I'll just |
Thank you <3 my top secret plan (designed together with tmandry) was to get a compiler review and then reroll into types to that view as well so that works well :) |
With this PR we now have basic functional Reborrow and CoerceShared traits. The current limitations are:
&mutwrappers is working (though I've not tested generic wrappers likeOption<&mut T>yet), but CoerceShared of&mutwrappers currently causes an ICE.The remaining tasks to complete before I'd consider this PR mergeable are:
Reborrow traits experiment: #145612