-
-
Notifications
You must be signed in to change notification settings - Fork 14.5k
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
Changes from all commits
d6b0c00
8cb2709
a6964fb
e11633b
819dbb7
a161c7b
a830c43
3fa1f2f
989b63c
8116826
06311ec
a35386c
daa8aab
39621bd
ebae325
66a54cf
de7dee9
14c8827
d52465d
27dacbc
f7c4d07
c2411da
77a6156
1f968e7
3b1957a
97994e9
1e7b514
37bdf23
2f3dde6
a6ea388
92ff599
235d471
4af2661
41f7cf5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,11 +2,12 @@ use std::fmt; | |
| use std::ops::Index; | ||
|
|
||
| use rustc_data_structures::fx::{FxIndexMap, FxIndexSet}; | ||
| use rustc_hir::Mutability; | ||
| use rustc_index::bit_set::DenseBitSet; | ||
| use rustc_middle::mir::visit::{MutatingUseContext, NonUseContext, PlaceContext, Visitor}; | ||
| use rustc_middle::mir::{self, Body, Local, Location, traversal}; | ||
| use rustc_middle::span_bug; | ||
| use rustc_middle::ty::{RegionVid, TyCtxt}; | ||
| use rustc_middle::{bug, span_bug, ty}; | ||
| use rustc_mir_dataflow::move_paths::MoveData; | ||
| use tracing::debug; | ||
|
|
||
|
|
@@ -300,6 +301,71 @@ impl<'a, 'tcx> Visitor<'tcx> for GatherBorrows<'a, 'tcx> { | |
| idx | ||
| }; | ||
|
|
||
| self.local_map.entry(borrowed_place.local).or_default().insert(idx); | ||
| } else if let &mir::Rvalue::Reborrow(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!() | ||
| }; | ||
| let &ty::Adt(target_adt, assigned_args) = | ||
| assigned_place.ty(self.body, self.tcx).ty.kind() | ||
| else { | ||
| unreachable!() | ||
| }; | ||
| let borrow = if mutability == Mutability::Mut { | ||
| // Reborrow | ||
| if target_adt.did() != reborrowed_adt.did() { | ||
| bug!( | ||
| "hir-typeck passed but Reborrow involves mismatching types at {location:?}" | ||
| ) | ||
| } | ||
| 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" } | ||
| ); | ||
| }; | ||
| let region = region.as_var(); | ||
| let kind = mir::BorrowKind::Mut { kind: mir::MutBorrowKind::Default }; | ||
| BorrowData { | ||
| kind, | ||
| region, | ||
| reserve_location: location, | ||
| activation_location: TwoPhaseActivation::NotTwoPhase, | ||
| borrowed_place, | ||
| assigned_place: *assigned_place, | ||
| } | ||
| } else { | ||
| // CoerceShared | ||
| if target_adt.did() == reborrowed_adt.did() { | ||
| bug!( | ||
| "hir-typeck passed but CoerceShared involves matching types at {location:?}" | ||
| ) | ||
| } | ||
| 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" } | ||
| ); | ||
| }; | ||
|
Comment on lines
+347
to
+354
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is the same in both arms, pull it out |
||
| let region = region.as_var(); | ||
| let kind = mir::BorrowKind::Shared; | ||
| BorrowData { | ||
| kind, | ||
| region, | ||
| reserve_location: location, | ||
| activation_location: TwoPhaseActivation::NotTwoPhase, | ||
| borrowed_place, | ||
| assigned_place: *assigned_place, | ||
| } | ||
| }; | ||
| let (idx, _) = self.location_map.insert_full(location, borrow); | ||
| let idx = BorrowIndex::from(idx); | ||
|
|
||
| self.local_map.entry(borrowed_place.local).or_default().insert(idx); | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1254,6 +1254,7 @@ impl<'a, 'tcx> MirBorrowckCtxt<'a, '_, 'tcx> { | |
| let mut error_reported = false; | ||
|
|
||
| let borrows_in_scope = self.borrows_in_scope(location, state); | ||
| debug!(?borrows_in_scope, ?location); | ||
|
|
||
| each_borrow_involving_path( | ||
| self, | ||
|
|
@@ -1593,6 +1594,35 @@ impl<'a, 'tcx> MirBorrowckCtxt<'a, '_, 'tcx> { | |
| } | ||
|
|
||
| Rvalue::CopyForDeref(_) => bug!("`CopyForDeref` in borrowck"), | ||
| &Rvalue::Reborrow(mutability, place) => { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please keep all |
||
| let access_kind = ( | ||
| Deep, | ||
| if mutability == Mutability::Mut { | ||
| Write(WriteKind::MutableBorrow(BorrowKind::Mut { | ||
| kind: MutBorrowKind::Default, | ||
| })) | ||
| } else { | ||
| Read(ReadKind::Borrow(BorrowKind::Shared)) | ||
| }, | ||
| ); | ||
|
|
||
| self.access_place( | ||
| location, | ||
| (place, span), | ||
| access_kind, | ||
| LocalMutationIsAllowed::Yes, | ||
| state, | ||
| ); | ||
|
|
||
| let action = InitializationRequiringAction::Borrow; | ||
|
|
||
| self.check_if_path_or_subpath_is_moved( | ||
| location, | ||
| action, | ||
| (place.as_ref(), span), | ||
| state, | ||
| ); | ||
| } | ||
| } | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -558,6 +558,16 @@ impl<'a, 'tcx> TypeChecker<'a, 'tcx> { | |
| } | ||
|
|
||
| impl<'a, 'tcx> Visitor<'tcx> for TypeChecker<'a, 'tcx> { | ||
| fn visit_assign(&mut self, place: &Place<'tcx>, rvalue: &Rvalue<'tcx>, location: Location) { | ||
| // check rvalue is Reborrow | ||
| if let Rvalue::Reborrow(mutability, rvalue) = rvalue { | ||
| self.add_generic_reborrow_constraint(*mutability, location, place, rvalue); | ||
| } else { | ||
| // rest of the cases | ||
| self.super_assign(place, rvalue, location); | ||
| } | ||
| } | ||
|
|
||
| fn visit_span(&mut self, span: Span) { | ||
| if !span.is_dummy() { | ||
| debug!(?span); | ||
|
|
@@ -635,8 +645,11 @@ impl<'a, 'tcx> Visitor<'tcx> for TypeChecker<'a, 'tcx> { | |
| debug!(?rv_ty); | ||
| let rv_ty = self.normalize(rv_ty, location); | ||
| debug!("normalized rv_ty: {:?}", rv_ty); | ||
| if let Err(terr) = | ||
| self.sub_types(rv_ty, place_ty, location.to_locations(), category) | ||
| // Note: we've checked Reborrow/CoerceShared type matches | ||
| // separately in fn visit_assign. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This being requires is another sign that this may not be the best representation. From other use sites it seems to me like you'd want to generalize Rvalue::Ref to support more types than references
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Answered above as well, but we thought that generalising That being said... I definitely do see the appeal; it'd be nice if the compiler could view
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yea... maybe it would need to be similar to how That said, these can be merged later, it is better to do such experiments with possibly wide-ranging effects in a separate PR from the one introducing such a big thing as this PR does. |
||
| if !matches!(rv, Rvalue::Reborrow(_, _)) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we should not have a |
||
| && let Err(terr) = | ||
| self.sub_types(rv_ty, place_ty, location.to_locations(), category) | ||
| { | ||
| span_mirbug!( | ||
| self, | ||
|
|
@@ -1677,7 +1690,8 @@ impl<'a, 'tcx> Visitor<'tcx> for TypeChecker<'a, 'tcx> { | |
| | Rvalue::BinaryOp(..) | ||
| | Rvalue::RawPtr(..) | ||
| | Rvalue::ThreadLocalRef(..) | ||
| | Rvalue::Discriminant(..) => {} | ||
| | Rvalue::Discriminant(..) | ||
| | Rvalue::Reborrow(..) => {} | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -2243,7 +2257,8 @@ impl<'a, 'tcx> TypeChecker<'a, 'tcx> { | |
| | Rvalue::CopyForDeref(..) | ||
| | Rvalue::UnaryOp(..) | ||
| | Rvalue::Discriminant(..) | ||
| | Rvalue::WrapUnsafeBinder(..) => None, | ||
| | Rvalue::WrapUnsafeBinder(..) | ||
| | Rvalue::Reborrow(..) => None, | ||
|
|
||
| Rvalue::Aggregate(aggregate, _) => match **aggregate { | ||
| AggregateKind::Adt(_, _, _, user_ty, _) => user_ty, | ||
|
|
@@ -2441,6 +2456,120 @@ impl<'a, 'tcx> TypeChecker<'a, 'tcx> { | |
| } | ||
| } | ||
|
|
||
| fn add_generic_reborrow_constraint( | ||
| &mut self, | ||
| mutability: Mutability, | ||
| location: Location, | ||
| dest: &Place<'tcx>, | ||
| borrowed_place: &Place<'tcx>, | ||
| ) { | ||
| // In Polonius mode, we also push a `loan_issued_at` fact | ||
| // linking the loan to the region (in some cases, though, | ||
| // there is no loan associated with this borrow expression -- | ||
| // that occurs when we are borrowing an unsafe place, for | ||
| // example). | ||
| // if let Some(polonius_facts) = polonius_facts { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. document why there is commented out code here |
||
| // let _prof_timer = self.infcx.tcx.prof.generic_activity("polonius_fact_generation"); | ||
| // if let Some(borrow_index) = borrow_set.get_index_of(&location) { | ||
| // let region_vid = borrow_region.as_var(); | ||
| // polonius_facts.loan_issued_at.push(( | ||
| // region_vid.into(), | ||
| // borrow_index, | ||
| // location_table.mid_index(location), | ||
| // )); | ||
| // } | ||
| // } | ||
|
|
||
| // If we are reborrowing the referent of another reference, we | ||
| // need to add outlives relationships. In a case like `&mut | ||
| // *p`, where the `p` has type `&'b mut Foo`, for example, we | ||
| // need to ensure that `'b: 'a`. | ||
|
|
||
| debug!( | ||
| "add_generic_reborrow_constraint({:?}, {:?}, {:?}, {:?})", | ||
| mutability, location, dest, borrowed_place | ||
| ); | ||
|
|
||
| let tcx = self.infcx.tcx; | ||
| let def = self.body.source.def_id().expect_local(); | ||
| let upvars = tcx.closure_captures(def); | ||
| let field = | ||
| path_utils::is_upvar_field_projection(tcx, upvars, borrowed_place.as_ref(), self.body); | ||
| let category = if let Some(field) = field { | ||
| ConstraintCategory::ClosureUpvar(field) | ||
| } else { | ||
| ConstraintCategory::Boring | ||
| }; | ||
|
|
||
| let dest_ty = dest.ty(self.body, tcx).ty; | ||
| let borrowed_ty = borrowed_place.ty(self.body, tcx).ty; | ||
| let ty::Adt(_, args) = dest_ty.kind() else { bug!() }; | ||
| let [arg, ..] = ***args else { bug!() }; | ||
| let ty::GenericArgKind::Lifetime(reborrow_region) = arg.kind() else { bug!() }; | ||
| self.constraints.liveness_constraints.add_location(reborrow_region.as_var(), location); | ||
|
|
||
| if mutability.is_not() { | ||
| // FIXME: for shared reborrow we need to relate the types manually, | ||
| // field by field with CoerceShared drilling down and down and down. | ||
| // We cannot just attempt to relate T and <T as CoerceShared>::Target | ||
| // by calling relate_types. | ||
|
Comment on lines
+2514
to
+2515
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why? This is quite a bit of complexity for something that references already support and have logic for. |
||
| let ty::Adt(dest_adt, dest_args) = dest_ty.kind() else { unreachable!() }; | ||
| let ty::Adt(borrowed_adt, borrowed_args) = borrowed_ty.kind() else { unreachable!() }; | ||
| let borrowed_fields = borrowed_adt.all_fields().collect::<Vec<_>>(); | ||
| for dest_field in dest_adt.all_fields() { | ||
| let Some(borrowed_field) = | ||
| borrowed_fields.iter().find(|f| f.name == dest_field.name) | ||
| else { | ||
| continue; | ||
| }; | ||
| let dest_ty = dest_field.ty(tcx, dest_args); | ||
| let borrowed_ty = borrowed_field.ty(tcx, borrowed_args); | ||
| if let ( | ||
| ty::Ref(borrow_region, _, Mutability::Mut), | ||
| ty::Ref(ref_region, _, Mutability::Not), | ||
| ) = (borrowed_ty.kind(), dest_ty.kind()) | ||
| { | ||
| self.relate_types( | ||
| borrowed_ty.peel_refs(), | ||
| ty::Variance::Covariant, | ||
| dest_ty.peel_refs(), | ||
| location.to_locations(), | ||
| category, | ||
| ) | ||
| .unwrap(); | ||
| 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, | ||
| }); | ||
| } else { | ||
| self.relate_types( | ||
| borrowed_ty, | ||
| ty::Variance::Covariant, | ||
| dest_ty, | ||
| location.to_locations(), | ||
| category, | ||
| ) | ||
| .unwrap(); | ||
| } | ||
| } | ||
| } else { | ||
| // Exclusive reborrow | ||
| self.relate_types( | ||
| borrowed_ty, | ||
| ty::Variance::Covariant, | ||
| dest_ty, | ||
| location.to_locations(), | ||
| category, | ||
| ) | ||
| .unwrap(); | ||
| } | ||
| } | ||
|
|
||
| fn prove_aggregate_predicates( | ||
| &mut self, | ||
| aggregate_kind: &AggregateKind<'tcx>, | ||
|
|
||
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.
only return
kindfrom thisifcondition and build theBorrowDataafterwards. It's the only thing different between the two branches