Rollup merge of #150550 - borrowck_cleanup_2, r=lcnr

Miscellaneous cleanups to borrowck related code

r? lcnr

Author: JonathanBrouwer

compiler/rustc_borrowck/src/borrow_set.rs

@@ -253,19 +253,52 @@ impl<'a, 'tcx> Visitor<'tcx> for GatherBorrows<'a, 'tcx> {
             }
 
             let region = region.as_var();
-
-            let borrow = BorrowData {
+            let borrow = |activation_location| BorrowData {
                 kind,
                 region,
                 reserve_location: location,
-                activation_location: TwoPhaseActivation::NotTwoPhase,
+                activation_location,
                 borrowed_place,
                 assigned_place: *assigned_place,
             };
-            let (idx, _) = self.location_map.insert_full(location, borrow);
-            let idx = BorrowIndex::from(idx);
 
-            self.insert_as_pending_if_two_phase(location, assigned_place, kind, idx);
+            let idx = if !kind.is_two_phase_borrow() {
+                debug!("  -> {:?}", location);
+                let (idx, _) = self
+                    .location_map
+                    .insert_full(location, borrow(TwoPhaseActivation::NotTwoPhase));
+                BorrowIndex::from(idx)
+            } else {
+                // When we encounter a 2-phase borrow statement, it will always
+                // be assigning into a temporary TEMP:
+                //
+                //    TEMP = &foo
+                //
+                // so extract `temp`.
+                let Some(temp) = assigned_place.as_local() else {
+                    span_bug!(
+                        self.body.source_info(location).span,
+                        "expected 2-phase borrow to assign to a local, not `{:?}`",
+                        assigned_place,
+                    );
+                };
+
+                // Consider the borrow not activated to start. When we find an activation, we'll update
+                // this field.
+                let (idx, _) = self
+                    .location_map
+                    .insert_full(location, borrow(TwoPhaseActivation::NotActivated));
+                let idx = BorrowIndex::from(idx);
+
+                // Insert `temp` into the list of pending activations. From
+                // now on, we'll be on the lookout for a use of it. Note that
+                // we are guaranteed that this use will come after the
+                // assignment.
+                let prev = self.pending_activations.insert(temp, idx);
+                assert_eq!(prev, None, "temporary associated with multiple two phase borrows");
+
+                idx
+            };
 
             self.local_map.entry(borrowed_place.local).or_default().insert(idx);
         }
@@ -334,62 +367,3 @@ impl<'a, 'tcx> Visitor<'tcx> for GatherBorrows<'a, 'tcx> {
         self.super_rvalue(rvalue, location)
     }
 }
-
-impl<'a, 'tcx> GatherBorrows<'a, 'tcx> {
-    /// If this is a two-phase borrow, then we will record it
-    /// as "pending" until we find the activating use.
-    fn insert_as_pending_if_two_phase(
-        &mut self,
-        start_location: Location,
-        assigned_place: &mir::Place<'tcx>,
-        kind: mir::BorrowKind,
-        borrow_index: BorrowIndex,
-    ) {
-        debug!(
-            "Borrows::insert_as_pending_if_two_phase({:?}, {:?}, {:?})",
-            start_location, assigned_place, borrow_index,
-        );
-
-        if !kind.allows_two_phase_borrow() {
-            debug!("  -> {:?}", start_location);
-            return;
-        }
-
-        // When we encounter a 2-phase borrow statement, it will always
-        // be assigning into a temporary TEMP:
-        //
-        //    TEMP = &foo
-        //
-        // so extract `temp`.
-        let Some(temp) = assigned_place.as_local() else {
-            span_bug!(
-                self.body.source_info(start_location).span,
-                "expected 2-phase borrow to assign to a local, not `{:?}`",
-                assigned_place,
-            );
-        };
-
-        // Consider the borrow not activated to start. When we find an activation, we'll update
-        // this field.
-        {
-            let borrow_data = &mut self.location_map[borrow_index.as_usize()];
-            borrow_data.activation_location = TwoPhaseActivation::NotActivated;
-        }
-
-        // Insert `temp` into the list of pending activations. From
-        // now on, we'll be on the lookout for a use of it. Note that
-        // we are guaranteed that this use will come after the
-        // assignment.
-        let old_value = self.pending_activations.insert(temp, borrow_index);
-        if let Some(old_index) = old_value {
-            span_bug!(
-                self.body.source_info(start_location).span,
-                "found already pending activation for temp: {:?} \
-                       at borrow_index: {:?} with associated data {:?}",
-                temp,
-                old_index,
-                self.location_map[old_index.as_usize()]
-            );
-        }
-    }
-}

compiler/rustc_borrowck/src/lib.rs

@@ -1301,7 +1301,7 @@ impl<'a, 'tcx> MirBorrowckCtxt<'a, '_, 'tcx> {
                 (Read(kind), BorrowKind::Mut { .. }) => {
                     // Reading from mere reservations of mutable-borrows is OK.
                     if !is_active(this.dominators(), borrow, location) {
-                        assert!(borrow.kind.allows_two_phase_borrow());
+                        assert!(borrow.kind.is_two_phase_borrow());
                         return ControlFlow::Continue(());
                     }
 
@@ -1464,7 +1464,7 @@ impl<'a, 'tcx> MirBorrowckCtxt<'a, '_, 'tcx> {
                     }
                     BorrowKind::Mut { .. } => {
                         let wk = WriteKind::MutableBorrow(bk);
-                        if bk.allows_two_phase_borrow() {
+                        if bk.is_two_phase_borrow() {
                             (Deep, Reservation(wk))
                         } else {
                             (Deep, Write(wk))

compiler/rustc_borrowck/src/polonius/legacy/loan_invalidations.rs

@@ -264,7 +264,7 @@ impl<'a, 'tcx> LoanInvalidationsGenerator<'a, 'tcx> {
                     }
                     BorrowKind::Mut { .. } => {
                         let wk = WriteKind::MutableBorrow(bk);
-                        if bk.allows_two_phase_borrow() {
+                        if bk.is_two_phase_borrow() {
                             (Deep, Reservation(wk))
                         } else {
                             (Deep, Write(wk))
@@ -384,7 +384,7 @@ impl<'a, 'tcx> LoanInvalidationsGenerator<'a, 'tcx> {
                         // Reading from mere reservations of mutable-borrows is OK.
                         if !is_active(this.dominators, borrow, location) {
                             // If the borrow isn't active yet, reads don't invalidate it
-                            assert!(borrow.kind.allows_two_phase_borrow());
+                            assert!(borrow.kind.is_two_phase_borrow());
                             return ControlFlow::Continue(());
                         }
 

compiler/rustc_borrowck/src/type_check/free_region_relations.rs

@@ -229,8 +229,9 @@ impl<'tcx> UniversalRegionRelationsBuilder<'_, 'tcx> {
         //   not ready to process them yet.
         // - Then compute the implied bounds. This will adjust
         //   the `region_bound_pairs` and so forth.
-        // - After this is done, we'll process the constraints, once
-        //   the `relations` is built.
+        // - After this is done, we'll register the constraints in
+        //   the `BorrowckInferCtxt`. Checking these constraints is
+        //   handled later by actual borrow checking.
         let mut normalized_inputs_and_output =
             Vec::with_capacity(self.universal_regions.unnormalized_input_tys.len() + 1);
         for ty in unnormalized_input_output_tys {
@@ -254,6 +255,15 @@ impl<'tcx> UniversalRegionRelationsBuilder<'_, 'tcx> {
                 constraints.push(c)
             }
 
+            // Currently `implied_outlives_bounds` will normalize the provided
+            // `Ty`, despite this it's still important to normalize the ty ourselves
+            // as normalization may introduce new region variables (#136547).
+            //
+            // If we do not add implied bounds for the type involving these new
+            // region variables then we'll wind up with the normalized form of
+            // the signature having not-wf types due to unsatisfied region
+            // constraints.
+            //
             // Note: we need this in examples like
             // ```
             // trait Foo {
@@ -262,7 +272,7 @@ impl<'tcx> UniversalRegionRelationsBuilder<'_, 'tcx> {
             // }
             // impl Foo for () {
             //   type Bar = ();
-            //   fn foo(&self) ->&() {}
+            //   fn foo(&self) -> &() {}
             // }
             // ```
             // Both &Self::Bar and &() are WF
@@ -277,6 +287,15 @@ impl<'tcx> UniversalRegionRelationsBuilder<'_, 'tcx> {
         }
 
         // Add implied bounds from impl header.
+        //
+        // We don't use `assumed_wf_types` to source the entire set of implied bounds for
+        // a few reasons:
+        // - `DefiningTy` for closure has the `&'env Self` type while `assumed_wf_types` doesn't
+        // - We compute implied bounds from the unnormalized types in the `DefiningTy` but do not
+        //   do so for types in impl headers
+        // - We must compute the normalized signature and then compute implied bounds from that
+        //   in order to connect any unconstrained region vars created during normalization to
+        //   the types of the locals corresponding to the inputs and outputs of the item. (#136547)
         if matches!(tcx.def_kind(defining_ty_def_id), DefKind::AssocFn | DefKind::AssocConst) {
             for &(ty, _) in tcx.assumed_wf_types(tcx.local_parent(defining_ty_def_id)) {
                 let result: Result<_, ErrorGuaranteed> = param_env
@@ -352,10 +371,7 @@ impl<'tcx> UniversalRegionRelationsBuilder<'_, 'tcx> {
         known_type_outlives_obligations.push(outlives);
     }
 
-    /// Update the type of a single local, which should represent
-    /// either the return type of the MIR or one of its arguments. At
-    /// the same time, compute and add any implied bounds that come
-    /// from this local.
+    /// Compute and add any implied bounds that come from a given type.
     #[instrument(level = "debug", skip(self))]
     fn add_implied_bounds(
         &mut self,

compiler/rustc_borrowck/src/universal_regions.rs

@@ -207,10 +207,10 @@ struct UniversalRegionIndices<'tcx> {
     /// `ty::Region` to the internal `RegionVid` we are using. This is
     /// used because trait matching and type-checking will feed us
     /// region constraints that reference those regions and we need to
-    /// be able to map them to our internal `RegionVid`. This is
-    /// basically equivalent to an `GenericArgs`, except that it also
-    /// contains an entry for `ReStatic` -- it might be nice to just
-    /// use an args, and then handle `ReStatic` another way.
+    /// be able to map them to our internal `RegionVid`.
+    ///
+    /// This is similar to just using `GenericArgs`, except that it contains
+    /// an entry for `'static`, and also late bound parameters in scope.
     indices: FxIndexMap<ty::Region<'tcx>, RegionVid>,
 
     /// The vid assigned to `'static`. Used only for diagnostics.

compiler/rustc_const_eval/src/interpret/step.rs

@@ -218,7 +218,7 @@ impl<'tcx, M: Machine<'tcx>> InterpCx<'tcx, M> {
                 // A fresh reference was created, make sure it gets retagged.
                 let val = M::retag_ptr_value(
                     self,
-                    if borrow_kind.allows_two_phase_borrow() {
+                    if borrow_kind.is_two_phase_borrow() {
                         mir::RetagKind::TwoPhase
                     } else {
                         mir::RetagKind::Default

compiler/rustc_middle/src/mir/statement.rs

@@ -842,7 +842,7 @@ impl BorrowKind {
 
     /// Returns whether borrows represented by this kind are allowed to be split into separate
     /// Reservation and Activation phases.
-    pub fn allows_two_phase_borrow(&self) -> bool {
+    pub fn is_two_phase_borrow(&self) -> bool {
         match *self {
             BorrowKind::Shared
             | BorrowKind::Fake(_)

compiler/rustc_trait_selection/src/traits/query/type_op/implied_outlives_bounds.rs

@@ -61,23 +61,28 @@ pub fn compute_implied_outlives_bounds_inner<'tcx>(
         "compute_implied_outlives_bounds assumes region obligations are empty before starting"
     );
 
-    let normalize_ty = |ty| -> Result<_, NoSolution> {
-        // We must normalize the type so we can compute the right outlives components.
-        // for example, if we have some constrained param type like `T: Trait<Out = U>`,
-        // and we know that `&'a T::Out` is WF, then we want to imply `U: 'a`.
-        let ty = ocx
-            .deeply_normalize(&ObligationCause::dummy_with_span(span), param_env, ty)
-            .map_err(|_| NoSolution)?;
-        Ok(ty)
-    };
+    // FIXME: This doesn't seem right. All call sites already normalize `ty`:
+    // - `Ty`s from the `DefiningTy` in Borrowck: we have to normalize in the caller
+    //      in order to get implied bounds involving any unconstrained region vars
+    //      created as part of normalizing the sig. See #136547
+    // - `Ty`s from impl headers in Borrowck and in Non-Borrowck contexts: we have
+    //      to normalize in the caller as computing implied bounds from unnormalized
+    //      types would be unsound. See #100989
+    //
+    // We must normalize the type so we can compute the right outlives components.
+    // for example, if we have some constrained param type like `T: Trait<Out = U>`,
+    // and we know that `&'a T::Out` is WF, then we want to imply `U: 'a`.
+    let normalized_ty = ocx
+        .deeply_normalize(&ObligationCause::dummy_with_span(span), param_env, ty)
+        .map_err(|_| NoSolution)?;
 
     // Sometimes when we ask what it takes for T: WF, we get back that
     // U: WF is required; in that case, we push U onto this stack and
     // process it next. Because the resulting predicates aren't always
     // guaranteed to be a subset of the original type, so we need to store the
     // WF args we've computed in a set.
     let mut checked_wf_args = rustc_data_structures::fx::FxHashSet::default();
-    let mut wf_args = vec![ty.into(), normalize_ty(ty)?.into()];
+    let mut wf_args = vec![ty.into(), normalized_ty.into()];
 
     let mut outlives_bounds: Vec<OutlivesBound<'tcx>> = vec![];
 
@@ -103,8 +108,8 @@ pub fn compute_implied_outlives_bounds_inner<'tcx>(
                 continue;
             };
             match pred {
-                // FIXME(const_generics): Make sure that `<'a, 'b, const N: &'a &'b u32>` is sound
-                // if we ever support that
+                // FIXME(generic_const_parameter_types): Make sure that `<'a, 'b, const N: &'a &'b u32>`
+                // is sound if we ever support that
                 ty::PredicateKind::Clause(ty::ClauseKind::Trait(..))
                 | ty::PredicateKind::Clause(ty::ClauseKind::HostEffect(..))
                 | ty::PredicateKind::Clause(ty::ClauseKind::ConstArgHasType(..))

compiler/rustc_traits/src/implied_outlives_bounds.rs

@@ -1,5 +1,5 @@
 //! Provider for the `implied_outlives_bounds` query.
-//! Do not call this query directory. See
+//! Do not call this query directly. See
 //! [`rustc_trait_selection::traits::query::type_op::implied_outlives_bounds`].
 
 use rustc_infer::infer::TyCtxtInferExt;