Support field-wise CoerceShared reborrows#157101
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.
|
🔨 5 commits were squashed into b8e47e8. |
rust-bors
Bot
force-pushed
the
fix-reborrow-coerceshared-field-relations
branch
from
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
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.
|
Unclosed quote in argument. Run |
|
@bors squash msg="Support field-wise CoerceShared reborrows" |
This comment has been minimized.
This comment has been minimized.
|
🔨 7 commits were squashed into 8987e4a. |
rust-bors
Bot
force-pushed
the
fix-reborrow-coerceshared-field-relations
branch
from
62ec49c to
8987e4a
Compare
rustbot
added
S-waiting-on-review
|
r? @folkertdev rustbot has assigned @folkertdev. Use Why was this reviewer chosen?The reviewer was selected based on:
|
|
|
|
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 |
Oh I'm Sorry I did not think much. I inferred from #156955 that you might be the right person to look specifically at the CTFE/Miri part, since that PR also touched reborrow const-eval/interpreter code and you reviewed it. But I understand now that this might be a bit to much to review on top of the work you already have.. Once again sorry, have a nice day |
|
Looks like this is associated with https://rust-lang.github.io/rust-project-goals/2026/reborrow-traits.html ? |
Hi Ralf, sorry, I always thought the tracking issue was enough. Thanks for the correction, I won't make the mistake again! |
|
Fair, the tracking issue should in principle suffice. But not everyone always clicks through all the links. ;) |
|
Hello @P8L1 and thank you for taking interest in the reborrow work. With per-field reborrowing the proposed design will break away from a few fundamental but important assumptions and intended use cases of this language feature. A design document would certainly bring @aapoalas and me onto the same page and help us understand why the extension would be desirable. Let us chat on Zulip as well if you need any help. |
Thanks, that makes sense. I wrote up a PR-specific design note here please review it and share your thoughts so this PR can proceed! |
There was a problem hiding this comment.
Borrow-set gathering records MIR Reborrow loans.
This file now treats malformed generic reborrow shapes as delayed compiler bugs instead of hard panics.
| } else if let &mir::Rvalue::Reborrow(target, mutability, borrowed_place) = rvalue { | ||
| let borrowed_place_ty = borrowed_place.ty(self.body, self.tcx).ty; | ||
| let &ty::Adt(reborrowed_adt, _reborrowed_args) = borrowed_place_ty.kind() else { | ||
| unreachable!() | ||
| self.tcx.dcx().span_delayed_bug( | ||
| self.body.source_info(location).span, | ||
| format!("generic reborrow source is not an ADT: {borrowed_place_ty:?}"), | ||
| ); | ||
| return; | ||
| }; | ||
| let &ty::Adt(target_adt, assigned_args) = target.kind() else { | ||
| self.tcx.dcx().span_delayed_bug( | ||
| self.body.source_info(location).span, | ||
| format!("generic reborrow target is not an ADT: {target:?}"), | ||
| ); | ||
| return; | ||
| }; | ||
| let &ty::Adt(target_adt, assigned_args) = target.kind() else { unreachable!() }; | ||
| let Some(ty::GenericArgKind::Lifetime(region)) = assigned_args.get(0).map(|r| r.kind()) | ||
| else { | ||
| bug!( | ||
| "hir-typeck passed but {} does not have a lifetime argument", | ||
| if mutability == Mutability::Mut { "Reborrow" } else { "CoerceShared" } | ||
| self.tcx.dcx().span_delayed_bug( | ||
| self.body.source_info(location).span, | ||
| format!( | ||
| "hir-typeck passed but {} does not have a lifetime argument", | ||
| if mutability == Mutability::Mut { "Reborrow" } else { "CoerceShared" } | ||
| ), | ||
| ); | ||
| return; |
There was a problem hiding this comment.
This replaces unreachable!/bug! assumptions with delayed bugs and early returns.
The borrow-set visitor can now avoid constructing a loan from invalid generic reborrow MIR.
There was a problem hiding this comment.
Borrowck type checking is where generic reborrow MIR becomes region and type constraints.
The main change is replacing ad hoc one-field CoerceShared handling with recursive field-wise constraints.
| let ty::Adt(_, dest_args) = dest_ty.kind() else { | ||
| span_mirbug!( | ||
| self, | ||
| borrowed_place, | ||
| "generic reborrow target is not an ADT: {dest_ty:?}" | ||
| ); | ||
| return; | ||
| }; | ||
| let Some(ty::GenericArgKind::Lifetime(dest_region)) = dest_args.get(0).map(|r| r.kind()) | ||
| else { | ||
| span_mirbug!( | ||
| self, | ||
| borrowed_place, | ||
| "generic reborrow target does not have a lifetime argument: {dest_ty:?}" | ||
| ); | ||
| return; | ||
| }; |
There was a problem hiding this comment.
This turns shape assumptions on the reborrow target into MIR bugs with early returns.
Borrowck still records liveness for the target lifetime, but only after proving the first generic arg is a lifetime.
| // CoerceShared relates distinct ADTs field-wise. Impl validation guarantees that | ||
| // every target field has the corresponding source field and that each field relation | ||
| // is Copy-compatible, recursively CoerceShared-compatible, or `&mut T` to `&T`. | ||
| // The loan itself is still issued for `borrowed_place` as a whole, so source-only | ||
| // fields remain protected for the inferred target lifetime. Under the current | ||
| // single-lifetime impl model, that is the source lifetime. | ||
| if self | ||
| .add_generic_shared_reborrow_constraints( | ||
| location, | ||
| borrowed_place, | ||
| borrowed_ty, | ||
| dest_ty, | ||
| category, | ||
| ) | ||
| .is_err() | ||
| { | ||
| return; |
There was a problem hiding this comment.
The previous local name-based loop is replaced by a recursive helper call.
The key invariant is that validation, borrowck, and materialization all consume the same canonical field pairs.
|
This PR currently does not correctly handle #![feature(reborrow, decl_macro)]
use std::marker::{CoerceShared, Reborrow};
macro my_macro($field:ident) {
pub struct MyMut<'a> {
$field: &'a i32,
field: &'a i64,
}
#[derive(Clone, Copy)]
pub struct MyRef<'a> {
$field: &'a i32,
field: &'a i64,
}
impl Reborrow for MyMut<'_> {}
impl<'a> CoerceShared<MyRef<'a>> for MyMut<'a> {}
}
my_macro!(field); |
| // Exclusive reborrow | ||
| self.relate_types( | ||
| if let Err(terr) = self.relate_types( | ||
| borrowed_ty, | ||
| ty::Variance::Covariant, | ||
| dest_ty, | ||
| location.to_locations(), | ||
| category, | ||
| ) | ||
| .unwrap(); | ||
| ) { | ||
| span_mirbug!( | ||
| self, | ||
| borrowed_place, | ||
| "bad generic exclusive reborrow relation ({:?}: {:?}): {:?}", | ||
| borrowed_ty, | ||
| dest_ty, | ||
| terr | ||
| ); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Exclusive reborrows keep the ordinary same-type relation but now report a MIR bug instead of unwrapping.
This is defensive behavior for malformed MIR and does not change the success path.
| } | ||
| } | ||
|
|
||
| fn add_generic_shared_reborrow_constraints( |
There was a problem hiding this comment.
This helper recursively walks the validated CoerceShared field relation.
It normalizes field types in borrowck's inference context before deciding whether to relate leaves or recurse.
| if let Err(terr) = self.relate_types_structurally_relating_aliases( | ||
| borrowed_ty.peel_refs(), | ||
| ty::Variance::Covariant, | ||
| dest_ty.peel_refs(), | ||
| location.to_locations(), | ||
| category, | ||
| ) { | ||
| span_mirbug!( | ||
| self, | ||
| borrowed_place, | ||
| "bad generic shared reborrow field relation ({:?}: {:?}): {:?}", | ||
| borrowed_ty, | ||
| dest_ty, | ||
| terr | ||
| ); | ||
| return Err(()); | ||
| } | ||
|
|
||
| self.constraints.outlives_constraints.push(OutlivesConstraint { | ||
| sup: ref_region.as_var(), | ||
| sub: borrow_region.as_var(), | ||
| locations: location.to_locations(), | ||
| span: location.to_locations().span(self.body), | ||
| category, | ||
| variance_info: ty::VarianceDiagInfo::default(), | ||
| from_closure: false, | ||
| }); | ||
| return Ok(()); | ||
| } | ||
|
|
||
| match (borrowed_ty.kind(), dest_ty.kind()) { | ||
| (ty::Adt(borrowed_adt, borrowed_args), ty::Adt(dest_adt, dest_args)) | ||
| if borrowed_adt.did() != dest_adt.did() => | ||
| { |
There was a problem hiding this comment.
The &mut T to &T leaf relates pointee types structurally and then emits the target-outlives-source constraint.
This mirrors built-in shared reborrowing while allowing aliases and projections in the pointee type.
| let field_pairs = match reborrow::coerce_shared_field_pairs( | ||
| tcx, | ||
| *borrowed_adt, | ||
| *borrowed_args, | ||
| *dest_adt, | ||
| *dest_args, | ||
| ) { | ||
| Ok(field_pairs) => field_pairs, | ||
| Err(CoerceSharedFieldPairError::FieldStyleMismatch) => { | ||
| span_mirbug!( | ||
| self, | ||
| borrowed_place, | ||
| "generic shared reborrow has unsupported field structure: \ | ||
| {borrowed_ty:?} -> {dest_ty:?}" | ||
| ); | ||
| return Err(()); | ||
| } | ||
| Err(CoerceSharedFieldPairError::MissingSourceField { .. }) => { | ||
| span_mirbug!( | ||
| self, | ||
| borrowed_place, | ||
| "generic shared reborrow is missing a source field: \ | ||
| {borrowed_ty:?} -> {dest_ty:?}" | ||
| ); | ||
| return Err(()); | ||
| } | ||
| }; | ||
|
|
||
| for field_pair in field_pairs { | ||
| self.add_generic_shared_reborrow_constraints( | ||
| location, | ||
| borrowed_place, | ||
| field_pair.source.ty, | ||
| field_pair.target.ty, | ||
| category, | ||
| )?; | ||
| } | ||
|
|
||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
Distinct ADTs are expanded through canonical CoerceShared field pairs, then each field pair is related recursively.
This is the borrowck side of nested field-wise reborrows such as OuterMut to OuterRef.
| _ => { | ||
| if let Err(terr) = self.relate_types_structurally_relating_aliases( | ||
| borrowed_ty, | ||
| ty::Variance::Covariant, | ||
| dest_ty, | ||
| location.to_locations(), | ||
| category, | ||
| ) { | ||
| span_mirbug!( | ||
| self, | ||
| borrowed_place, | ||
| "bad generic shared reborrow field relation ({:?}: {:?}): {:?}", | ||
| borrowed_ty, | ||
| dest_ty, | ||
| terr | ||
| ); | ||
| Err(()) | ||
| } else { | ||
| Ok(()) | ||
| } | ||
| } |
There was a problem hiding this comment.
The fallback relates non-recursive leaves structurally, including aliases, using the same variance as the old field loop.
This covers copied scalar fields and equal field types after normalization.
There was a problem hiding this comment.
HIR type checking creates the generic reborrow coercion adjustment before MIR borrowck sees it. This file adds a structural shape guard so only field-corresponding CoerceShared impls produce reborrow MIR.
| }; | ||
| use rustc_middle::ty::error::TypeError; | ||
| use rustc_middle::ty::{self, Ty, TyCtxt, TypeVisitableExt, Unnormalized}; | ||
| use rustc_middle::ty::{self, Ty, TyCtxt, TypeVisitableExt, Unnormalized, reborrow}; |
There was a problem hiding this comment.
HIR typeck imports the shared reborrow helper for a structural shape guard before creating a reborrow adjustment. This keeps obviously malformed CoerceShared shapes out of MIR.
| } | ||
|
|
||
| /// Applies generic exclusive reborrowing on type implementing `Reborrow`. | ||
| /// Applies generic shared reborrowing on types implementing `CoerceShared`. |
There was a problem hiding this comment.
The method comment is corrected to describe CoerceShared shared reborrowing, matching the code path below. The new shape guard rejects non-structs, missing lifetimes, and missing field correspondence before registering the trait obligation.
| // This is only a shape guard for producing well-formed MIR adjustments. The field-type | ||
| // relation remains owned by the `CoerceShared` obligation and builtin impl validation. | ||
| if !self.coerce_shared_reborrow_structurally_well_formed(a, b) { | ||
| return Err(TypeError::Mismatch); | ||
| } |
There was a problem hiding this comment.
This helper checks only ADT struct shape, exactly-one-lifetime presence, and successful canonical field pairing. It does not inspect per-field type compatibility beyond what the pair helper needs for field metadata.
| } | ||
| } | ||
|
|
||
| fn coerce_shared_reborrow_structurally_well_formed(&self, a: Ty<'tcx>, b: Ty<'tcx>) -> bool { |
There was a problem hiding this comment.
This helper is the HIR typeck shape gate for producing ExprKind::Reborrow adjustments. It accepts only struct ADTs with one lifetime argument each and successful canonical field pairing.
Ok im going to fix this now then finish comments |
There was a problem hiding this comment.
This new helper module is the canonical home for Reborrow and CoerceShared field metadata: it extracts participating ADT fields, preserves projection indices and spans, filters PhantomData, and computes source-to-target field pairs.
| use crate::ty::{self, Ty, TyCtxt}; | ||
|
|
||
| #[derive(Clone, Copy, Debug)] | ||
| pub struct ReborrowField<'tcx> { |
There was a problem hiding this comment.
This struct is the data contract for one participating field. index preserves the real ADT field index for MIR projection, ident is needed for hygiene-aware named-field matching, name is kept for diagnostics, ty is the instantiated field type, and span lets validation point at the field that caused the problem.
The split between ident and name matters: name is useful for user-facing output, but matching named fields must not be reduced to raw textual-name equality.
| } | ||
|
|
||
| #[derive(Clone, Copy, Debug)] | ||
| pub struct CoerceSharedFieldPair<'tcx> { |
There was a problem hiding this comment.
CoerceSharedFieldPair is the normalized relation later compiler phases operate on: each target field has a specific source field. The error enum keeps correspondence failures field-local, distinguishing mismatched struct styles from a target field that has no valid source-side data field.
| } | ||
|
|
||
| /// Returns the instantiated non-`PhantomData` fields that participate in generic reborrows. | ||
| pub fn reborrow_data_fields<'tcx>( |
There was a problem hiding this comment.
This function extracts the instantiated non-PhantomData fields that actually participate in field-wise reborrowing. It deliberately preserves the original FieldIdx, because matching ignores marker fields but MIR / CTFE / codegen still need to project from the real ADT layout.
| if source_variant.ctor_kind() != target_variant.ctor_kind() { | ||
| return Err(CoerceSharedFieldPairError::FieldStyleMismatch); | ||
| } | ||
|
|
||
| let by_position = matches!(target_variant.ctor_kind(), Some(CtorKind::Fn)); |
There was a problem hiding this comment.
This is the boundary between named-field and tuple-field correspondence. CoerceShared does not allow named structs and tuple structs to be matched against each other, because the meaning of “corresponding field” is different in the two styles.
| let target_fields = reborrow_data_fields(tcx, target_def, target_args); | ||
|
|
||
| if by_position { | ||
| target_fields |
There was a problem hiding this comment.
Tuple structs match by filtered data-field ordinal, not by original field index. This lets PhantomData be ignored for correspondence while still preserving each field’s original FieldIdx inside the returned pair for later projection.
| if by_position { | ||
| target_fields | ||
| .into_iter() | ||
| .zip(source_fields.into_iter().map(Some).chain(std::iter::repeat(None))) |
There was a problem hiding this comment.
The repeat(None) tail makes the target-driven failure mode explicit: every target data field must have a corresponding source data field. Extra source fields do not matter for constructing the shared target view, but missing source fields are rejected.
| .find(|source| { | ||
| tcx.hygienic_eq(target.ident, source.ident, source_variant.def_id) | ||
| }) |
There was a problem hiding this comment.
This is the hygiene-sensitive part of named-field correspondence. Two fields can print with the same textual name under decl_macro while still being distinct identifiers, so this must use rustc’s hygienic field identity rather than raw Symbol equality. (The decl macro hygiene bug was caused by me just comparing names)
| tcx.hygienic_eq(target.ident, source.ident, source_variant.def_id) | ||
| }) | ||
| .ok_or(CoerceSharedFieldPairError::MissingSourceField { target })?; | ||
|
|
There was a problem hiding this comment.
The named-field branch is target-driven: each non-PhantomData target field must resolve to a hygienically corresponding source field. If no source field matches, the helper reports the target field as missing instead of silently omitting it or falling back to position.
|
The #![feature(reborrow)]
use std::marker::{CoerceShared, Reborrow};
trait Trait {
type Assoc;
}
impl Trait for i32 {
type Assoc = i64;
}
struct MyMut<'a> {
x: &'a (),
y: i64,
}
#[derive(Copy, Clone)]
struct MyRef<'a> {
x: &'a (),
y: <i32 as Trait>::Assoc,
}
impl Reborrow for MyMut<'_> {}
impl<'a> CoerceShared<MyRef<'a>> for MyMut<'a> {} |
|
The // Compiles without errors
#![feature(reborrow)]
use std::marker::{CoerceShared, Reborrow};
struct MyMut<'a> {
x: &'static (),
y: &'a (),
}
#[derive(Copy, Clone)]
struct MyRef<'a> {
x: &'a (),
y: &'static (),
}
impl Reborrow for MyMut<'_> {}
impl<'a> CoerceShared<MyRef<'a>> for MyMut<'a> {} |
|
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 |
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 |
10 participants
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