Support field-wise CoerceShared reborrows - #157101
Closed
P8L1 wants to merge 3 commits into
Closed
Conversation
rustbot
added
S-waiting-on-author
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment has been minimized.
This comment has been minimized.
Contributor
|
🔨 5 commits were squashed into b8e47e8. |
rust-bors
Bot
force-pushed
the
fix-reborrow-coerceshared-field-relations
branch
from
May 29, 2026 09:36
9713677 to
b8e47e8
Compare
This comment has been minimized.
This comment has been minimized.
P8L1
force-pushed
the
fix-reborrow-coerceshared-field-relations
branch
from
May 31, 2026 17:45
b8e47e8 to
83d9f22
Compare
This comment has been minimized.
This comment has been minimized.
This comment was marked as outdated.
This comment was marked as outdated.
Contributor
|
Unclosed quote in argument. Run |
Contributor
Author
|
@bors squash msg="Support field-wise CoerceShared reborrows" |
This comment has been minimized.
This comment has been minimized.
Contributor
|
🔨 7 commits were squashed into 8987e4a. |
rust-bors
Bot
force-pushed
the
fix-reborrow-coerceshared-field-relations
branch
from
June 1, 2026 06:26
62ec49c to
8987e4a
Compare
P8L1
marked this pull request as ready for review
June 1, 2026 06:28
Collaborator
rustbot
added
S-waiting-on-review
Collaborator
|
r? @folkertdev rustbot has assigned @folkertdev. Use Why was this reviewer chosen?The reviewer was selected based on:
|
Collaborator
|
|
Member
|
Why are you asking for my review here? I have nothing to do with this feature and don't have capacity to take on a new feature, especially not a massive PR like this. @rustbot reroll |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
Contributor
Author
|
Ok I would like to start off by saying sorry for all the pings. I have recently received valid criticism about the reviewability of this PR. Thus, I took my self-review notes, summarised them and posted them as review comments here. This was done to make it easier for reviewers to wrap their heads around the changes. I have also updated the Design Note to reflect the new decl_macro hygiene changes |
Contributor
Author
Thanks, both of these were bugs in the impl validation rather than intentional behavior. I pushed a fix in Validate CoerceShared field relations after normalization. CoerceShared field validation now structurally normalizes the field types before accepting equality or the built-in That fixes the associated-type case because |
Contributor
Author
|
Once again, I updated the design note to reflect the new "Validate CoerceShared field relations after normalization" changes |
P8L1
commented
Jun 2, 2026
| if !errors.is_empty() { Err(errors) } else { Ok(()) } | ||
| } | ||
|
|
||
| pub(crate) fn coerce_shared_info<'tcx>( |
Contributor
Author
There was a problem hiding this comment.
Validates a builtin CoerceShared impl. Source and target shapes that later reborrow phases cannot interpret are rejected here. Field-type legality is delegated to canonical field-pair validation below.
|
|
||
| // FIXME(#155345): This should return `Unnormalized` | ||
| fn collect_struct_data_fields<'tcx>( | ||
| fn validate_reborrow_field_access( |
Contributor
Author
There was a problem hiding this comment.
Rejects fields that later reborrow lowering cannot legally project. Field-wise validation must be able to name every participating field, so private, otherwise inaccessible, and non-exhaustive fields are rejected before canonical field-pair validation.
| Ok(()) | ||
| } | ||
|
|
||
| fn validate_coerce_shared_fields<'tcx>( |
Contributor
Author
There was a problem hiding this comment.
Builds and validates canonical CoerceShared source-to-target field pairs. Pair construction defines the correspondence used by validation and later phases: named fields use hygienic identity, while tuple fields use the ordinal among non-PhantomData data fields.
| Ok(()) | ||
| } | ||
|
|
||
| fn validate_coerce_shared_field<'tcx>( |
Contributor
Author
There was a problem hiding this comment.
Validates one corresponding CoerceShared field relation. The accepted cases are deliberately narrow: a builtin &mut T to &T shared-reference leaf, a copy-compatible equal field type, or a recursive SourceField: CoerceShared<TargetField> obligation. Successful paths must discharge generated obligations and region constraints before accepting.
| ocx.resolve_regions_and_report_errors(impl_did, param_env, []) | ||
| } | ||
|
|
||
| enum FieldRelation { |
Contributor
Author
There was a problem hiding this comment.
Field relation to prove after normalization in the impl-validation context. This keeps equality separate from the builtin shared-reference leaf. The MutRefToSharedRef case is not type equality: it normalizes both fields, strips exactly one &mut/& layer, and records the required lifetime constraint before relating the referents.
| } | ||
|
|
||
| enum FieldRelation { | ||
| Equal, |
Contributor
Author
There was a problem hiding this comment.
Require normalized source and target field types to be equal.
|
|
||
| enum FieldRelation { | ||
| Equal, | ||
| MutRefToSharedRef, |
Contributor
Author
There was a problem hiding this comment.
Require &mut T to relate to &T after adding the lifetime constraint.
| MutRefToSharedRef, | ||
| } | ||
|
|
||
| fn field_tys_satisfy_relation_after_normalization_and_resolution<'tcx>( |
Contributor
Author
There was a problem hiding this comment.
Normalizes, relates, and resolves a field-type relation as a single check. Alias and projection types are structurally normalized in the impl-validation context. Generated obligations are evaluated with ambiguity as failure, the normalized types are structurally related, and region constraints must resolve before this returns true.
| && ocx.resolve_regions(impl_did, param_env, []).is_empty() | ||
| } | ||
|
|
||
| fn assert_field_type_is_copy<'tcx>( |
Contributor
Author
There was a problem hiding this comment.
Verifies the copy-compatible leaf case after field equality is established. The equal-type field path may only accept a leaf when that field type is Copy, and the generated copy obligations and regions must also resolve.
Contributor
|
The following code causes an ICE with this PR: #![feature(reborrow)]
use std::marker::{CoerceShared, Reborrow};
pub trait Trait {
type Assoc;
}
impl<T> Trait for T {
type Assoc = T;
}
#[expect(dead_code)]
pub struct MyMut<'a, T>(&'a (), T);
#[expect(dead_code)]
#[derive(Copy, Clone)]
struct MyRef<'a, T>(&'a (), <T as Trait>::Assoc);
impl<T: Copy> Reborrow for MyMut<'_, T> {}
impl<'a, T: Copy> CoerceShared<MyRef<'a, T>> for MyMut<'a, T> {}
pub fn foo<T: Trait + Copy>(x: MyMut<'_, T>) {
let _: MyRef<'_, T> = x;
}Error outputI think that rust was unable to figure out that |
aapoalas
requested changes
Jun 4, 2026
Contributor
There was a problem hiding this comment.
Hey, first: thank you for the work you've done.
However, after some discussion with Xiang we've come to the conclusion that unfortunately this direction this PR is walking us is indeed a bit wrong. Here's what we'd want to do:
- Perform all well-formedness checking in the
coherence/builtin.rs: there shouldn't be any need to perform any checks relating to the correctness of the traits or field types in THIR building, MIR building, or the borrow checker. - Similarly to now, the THIR will emit a single reborrow adjustment that is used in borrow checking.
- An MIR pass will be added that lowers the single Rvalue::Reborrow into field-wise actions. This goes into
fn run_runtime_lowering_passes.
This gets us "both flavours of reborrowing": simple reborrowing in the MIR for borrow checking, and field-by-field reborrowing in the lowered MIR for code generation. Actual code generation backends shouldn't have to do all that much work for reborrowing to work at this point.
If you want to help, here's how:
- The tests you're adding here are good; adding them in a separate PR would be great. It's okay if some or all of them produce ICEs in that PR, there's a
tests/ui/crashesfolder for that and we can add in atests/ui/crashes/reborrowfolder specifically for those. - The work you've done on the well-formedness check (
coherence/builtin.rs) deserves a separate PR. We can then build on top of that to improve the checking to get to the desired state of the well-formedness check containing all the check logic. - A distant third is the MIR pass itself: this we'll be very deeply involved in so collaborating may be harder, but as we get into building the pass I can ping you or send an email and let you know if there is anything in particular you could help with.
On the communication front, I recommend using your own voice and words in the future: you're doing yourself a disfavour by using an LLM to translate and generate text for you. You do not need to communicate in perfect English - it's perfectly okay to communicate at the level that you're capable of, and nothing more or less.
Cheers.
rustbot
added
S-waiting-on-author
Collaborator
|
Reminder, once the PR becomes ready for a review, use |
aapoalas
reviewed
Jun 4, 2026
Contributor
There was a problem hiding this comment.
note: Seeing this file change is a sign that this PR puts us on the wrong path: there shouldn't really be any codegen-specific handling of reborrowing, or if there is then it should just be an extra way to generate a copy.
aapoalas
reviewed
Jun 4, 2026
| let ty::Adt(dest_adt, dest_args) = dest_ty.kind() else { bug!() }; | ||
| let [dest_arg, ..] = ***dest_args else { bug!() }; | ||
| let ty::GenericArgKind::Lifetime(dest_region) = dest_arg.kind() else { bug!() }; | ||
| let ty::Adt(_, dest_args) = dest_ty.kind() else { |
Contributor
There was a problem hiding this comment.
note: These changes are questionable. These paths should be unreachable, so why are we adding extra dead code?
This was referenced Jun 5, 2026
Contributor
Author
Hi, thanks so much. I appreciate it! I am unfortunately a bit caught up with work at the moment. Thus, I dont think I will be doing more work soon. I will email you if I get a breather and can resume work on Reborrow. I also think that the above approach sounds great. I have split this PR into #157490 and #157489 I am, however, uncertain if it is the exact split you want. Just say if I have to remove things |
rustbot
removed
the
S-waiting-on-author
GuillaumeGomez
added a commit
to GuillaumeGomez/rust
that referenced
this pull request
Jun 12, 2026
…rrow-tests, r=aapoalas Add field-wise CoerceShared reborrow tests Title. Split from rust-lang#157101 r? @aapoalas
JonathanBrouwer
added a commit
to JonathanBrouwer/rust
that referenced
this pull request
Jun 12, 2026
…rrow-tests, r=aapoalas Add field-wise CoerceShared reborrow tests Title. Split from rust-lang#157101 r? @aapoalas
JonathanBrouwer
added a commit
to JonathanBrouwer/rust
that referenced
this pull request
Jun 12, 2026
…rrow-tests, r=aapoalas Add field-wise CoerceShared reborrow tests Title. Split from rust-lang#157101 r? @aapoalas
JonathanBrouwer
added a commit
to JonathanBrouwer/rust
that referenced
this pull request
Jun 12, 2026
…rrow-tests, r=aapoalas Add field-wise CoerceShared reborrow tests Title. Split from rust-lang#157101 r? @aapoalas
JonathanBrouwer
added a commit
to JonathanBrouwer/rust
that referenced
this pull request
Jun 12, 2026
…rrow-tests, r=aapoalas Add field-wise CoerceShared reborrow tests Title. Split from rust-lang#157101 r? @aapoalas
rust-timer
added a commit
that referenced
this pull request
Jun 13, 2026
pull Bot
pushed a commit
to xtqqczze/rust-lang-miri
that referenced
this pull request
Jun 13, 2026
…s, r=aapoalas Add field-wise CoerceShared reborrow tests Title. Split from rust-lang/rust#157101 r? @aapoalas
JonathanBrouwer
added a commit
to JonathanBrouwer/rust
that referenced
this pull request
Aug 5, 2026
…r=oli-obk Add CoerceShared field-wise reborrow WF checks This PR attempts to add a well-formedness check for CoerceShared. Split out of rust-lang#157101 r? @aapoalas
rust-timer
added a commit
that referenced
this pull request
Aug 6, 2026
github-actions Bot
pushed a commit
to rust-lang/stdarch
that referenced
this pull request
Aug 6, 2026
Add CoerceShared field-wise reborrow WF checks This PR attempts to add a well-formedness check for CoerceShared. Split out of rust-lang/rust#157101 r? @aapoalas
pull Bot
pushed a commit
to LeeeeeeM/miri
that referenced
this pull request
Aug 6, 2026
Add CoerceShared field-wise reborrow WF checks This PR attempts to add a well-formedness check for CoerceShared. Split out of rust-lang/rust#157101 r? @aapoalas
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
10 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
View all comments
This PR extends generic shared reborrows so that
CoerceSharedvalidates and lowers source-to-target structs field by field, rather than relying on a same-layout, transmute-like copy.The core change is a shared
rustc_middle::ty::reborrowhelper that computes the field correspondence used byCoerceSharedvalidation, borrow checking, const-eval, and codegen.The correspondence rules are:
PhantomDatadata fields by name.PhantomDatadata fields by position.PhantomDatafields are ignored.CoerceSharedimpl validation now checks each corresponding field relation. A field relation is accepted when the field is:CoerceShared-compatible, orThe validation also rejects mismatched field styles, missing source fields, mismatched reborrow lifetimes, and inaccessible fields with targeted diagnostics.
Borrowck now adds shared generic reborrow constraints recursively through the validated field relations, while still issuing the loan for the original source place as a whole. This ensures source-only fields remain protected for the inferred target lifetime when the target omits fields from the source.
Const-eval and codegen now lower shared generic reborrows recursively into the target fields instead of treating the operation as a transmute-like copy. This covers nested
CoerceSharedfields and layout-changing source/target pairs.Fixes
Fixes #156566.
Fixes #156309.
Fixes #156315.
PR scope
I did not split these into smaller PRs because the issues mostly share the same root cause:
CoerceSharedneeded a field-by-field validation and lowering model. So when making this, I just decided to write it in a way that will fix the issues anyway.Tracking
#145612
https://rust-lang.github.io/rust-project-goals/2025h2/autoreborrow-traits.html
CC. @aapoalas, @dingxiangfei2009
r? @RalfJung