Rollup merge of #74661 - SNCPlay42:lifetime-names-refactor, r=estebank

Refactor `region_name`: add `RegionNameHighlight`

This PR does not change any diagnostics itself, rather it enables further code changes, but I would like to get approval for the refactoring first before making use of it.

In `rustc_mir::borrow_check::diagnostics::region_name`, there is code that allows for, when giving a synthesized name like `'1` to an anonymous lifetime, pointing at e.g. the exact '`&`' that introduces the lifetime.

This PR decouples that code from the specific case of arguments, adding a new enum `RegionNameHighlight`, enabling future changes to use it in other places.

This allows:

* We could change the other `AnonRegionFrom*` variants to use `RegionNameHighlight` to precisely point at where lifetimes are introduced in other locations when they have type annotations, e.g. a closure return `|...| -> &i32`.
  * Because of how async functions are lowered this affects async functions as well, see #74072
* for #74597, we could add a second, optional `RegionNameHighlight` to the `AnonRegionFromArgument` variant that highlights a lifetime in the return type of a function when, due to elision, this is the same as the argument lifetime.
* in #74497 (comment) I noticed that a diagnostic was trying to introduce a lifetime `'2` in the opaque type `impl std::future::Future`. The code for the case of arguments has [code to handle cases like this](https://github.com/rust-lang/rust/blob/bbebe7351fcd29af1eb9a35e315369b15887ea09/src/librustc_mir/borrow_check/diagnostics/region_name.rs#L365) but not the others. This refactoring would allow the same code path to handle this.
  * It might be appropriate to add another variant of `RegionNameHighlight` to say something like `lifetime '1 appears in the opaque type impl std::future::Future`.

These are quite a few changes so I thought I would make sure the refactoring is OK before I start making changes that rely on it. :)

Author: Manishearth

src/librustc_mir/borrow_check/diagnostics/outlives_suggestion.rs

@@ -60,9 +60,7 @@ impl OutlivesSuggestionBuilder {
             // Don't give suggestions for upvars, closure return types, or other unnamable
             // regions.
             RegionNameSource::SynthesizedFreeEnvRegion(..)
-            | RegionNameSource::CannotMatchHirTy(..)
-            | RegionNameSource::MatchedHirTy(..)
-            | RegionNameSource::MatchedAdtAndSegment(..)
+            | RegionNameSource::AnonRegionFromArgument(..)
             | RegionNameSource::AnonRegionFromUpvar(..)
             | RegionNameSource::AnonRegionFromOutput(..)
             | RegionNameSource::AnonRegionFromYieldTy(..)

src/librustc_mir/borrow_check/diagnostics/region_errors.rs

@@ -19,7 +19,7 @@ use crate::borrow_check::{
     MirBorrowckCtxt,
 };
 
-use super::{OutlivesSuggestionBuilder, RegionName, RegionNameSource};
+use super::{OutlivesSuggestionBuilder, RegionName};
 
 impl ConstraintDescription for ConstraintCategory {
     fn description(&self) -> &'static str {
@@ -396,18 +396,8 @@ impl<'a, 'tcx> MirBorrowckCtxt<'a, 'tcx> {
             diag.span_label(upvar_span, "variable captured here");
         }
 
-        match self.give_region_a_name(*outlived_fr).unwrap().source {
-            RegionNameSource::NamedEarlyBoundRegion(fr_span)
-            | RegionNameSource::NamedFreeRegion(fr_span)
-            | RegionNameSource::SynthesizedFreeEnvRegion(fr_span, _)
-            | RegionNameSource::CannotMatchHirTy(fr_span, _)
-            | RegionNameSource::MatchedHirTy(fr_span)
-            | RegionNameSource::MatchedAdtAndSegment(fr_span)
-            | RegionNameSource::AnonRegionFromUpvar(fr_span, _)
-            | RegionNameSource::AnonRegionFromOutput(fr_span, _, _) => {
-                diag.span_label(fr_span, "inferred to be a `FnMut` closure");
-            }
-            _ => {}
+        if let Some(fr_span) = self.give_region_a_name(*outlived_fr).unwrap().span() {
+            diag.span_label(fr_span, "inferred to be a `FnMut` closure");
         }
 
         diag.note(

src/librustc_mir/borrow_check/diagnostics/region_name.rs

@@ -34,13 +34,8 @@ crate enum RegionNameSource {
     Static,
     /// The free region corresponding to the environment of a closure.
     SynthesizedFreeEnvRegion(Span, String),
-    /// The region name corresponds to a region where the type annotation is completely missing
-    /// from the code, e.g. in a closure arguments `|x| { ... }`, where `x` is a reference.
-    CannotMatchHirTy(Span, String),
-    /// The region name corresponds a reference that was found by traversing the type in the HIR.
-    MatchedHirTy(Span),
-    /// A region name from the generics list of a struct/enum/union.
-    MatchedAdtAndSegment(Span),
+    /// The region corresponding to an argument.
+    AnonRegionFromArgument(RegionNameHighlight),
     /// The region corresponding to a closure upvar.
     AnonRegionFromUpvar(Span, String),
     /// The region corresponding to the return type of a closure.
@@ -51,23 +46,52 @@ crate enum RegionNameSource {
     AnonRegionFromAsyncFn(Span),
 }
 
+/// Describes what to highlight to explain to the user that we're giving an anonymous region a
+/// synthesized name, and how to highlight it.
+#[derive(Debug, Clone)]
+crate enum RegionNameHighlight {
+    /// The anonymous region corresponds to a reference that was found by traversing the type in the HIR.
+    MatchedHirTy(Span),
+    /// The anonymous region corresponds to a `'_` in the generics list of a struct/enum/union.
+    MatchedAdtAndSegment(Span),
+    /// The anonymous region corresponds to a region where the type annotation is completely missing
+    /// from the code, e.g. in a closure arguments `|x| { ... }`, where `x` is a reference.
+    CannotMatchHirTy(Span, String),
+}
+
 impl RegionName {
     crate fn was_named(&self) -> bool {
         match self.source {
             RegionNameSource::NamedEarlyBoundRegion(..)
             | RegionNameSource::NamedFreeRegion(..)
             | RegionNameSource::Static => true,
             RegionNameSource::SynthesizedFreeEnvRegion(..)
-            | RegionNameSource::CannotMatchHirTy(..)
-            | RegionNameSource::MatchedHirTy(..)
-            | RegionNameSource::MatchedAdtAndSegment(..)
+            | RegionNameSource::AnonRegionFromArgument(..)
             | RegionNameSource::AnonRegionFromUpvar(..)
             | RegionNameSource::AnonRegionFromOutput(..)
             | RegionNameSource::AnonRegionFromYieldTy(..)
             | RegionNameSource::AnonRegionFromAsyncFn(..) => false,
         }
     }
 
+    crate fn span(&self) -> Option<Span> {
+        match self.source {
+            RegionNameSource::Static => None,
+            RegionNameSource::NamedEarlyBoundRegion(span)
+            | RegionNameSource::NamedFreeRegion(span)
+            | RegionNameSource::SynthesizedFreeEnvRegion(span, _)
+            | RegionNameSource::AnonRegionFromUpvar(span, _)
+            | RegionNameSource::AnonRegionFromOutput(span, _, _)
+            | RegionNameSource::AnonRegionFromYieldTy(span, _)
+            | RegionNameSource::AnonRegionFromAsyncFn(span) => Some(span),
+            RegionNameSource::AnonRegionFromArgument(ref highlight) => match *highlight {
+                RegionNameHighlight::MatchedHirTy(span)
+                | RegionNameHighlight::MatchedAdtAndSegment(span)
+                | RegionNameHighlight::CannotMatchHirTy(span, _) => Some(span),
+            },
+        }
+    }
+
     crate fn highlight_region_name(&self, diag: &mut DiagnosticBuilder<'_>) {
         match &self.source {
             RegionNameSource::NamedFreeRegion(span)
@@ -81,17 +105,22 @@ impl RegionName {
                 );
                 diag.note(&note);
             }
-            RegionNameSource::CannotMatchHirTy(span, type_name) => {
+            RegionNameSource::AnonRegionFromArgument(RegionNameHighlight::CannotMatchHirTy(
+                span,
+                type_name,
+            )) => {
                 diag.span_label(*span, format!("has type `{}`", type_name));
             }
-            RegionNameSource::MatchedHirTy(span)
+            RegionNameSource::AnonRegionFromArgument(RegionNameHighlight::MatchedHirTy(span))
             | RegionNameSource::AnonRegionFromAsyncFn(span) => {
                 diag.span_label(
                     *span,
                     format!("let's call the lifetime of this reference `{}`", self),
                 );
             }
-            RegionNameSource::MatchedAdtAndSegment(span) => {
+            RegionNameSource::AnonRegionFromArgument(
+                RegionNameHighlight::MatchedAdtAndSegment(span),
+            ) => {
                 diag.span_label(*span, format!("let's call this `{}`", self));
             }
             RegionNameSource::AnonRegionFromUpvar(span, upvar_name) => {
@@ -307,21 +336,31 @@ impl<'tcx> MirBorrowckCtxt<'_, 'tcx> {
 
         let arg_ty = self.regioncx.universal_regions().unnormalized_input_tys
             [implicit_inputs + argument_index];
-        if let Some(region_name) =
-            self.give_name_if_we_can_match_hir_ty_from_argument(fr, arg_ty, argument_index)
-        {
-            return Some(region_name);
-        }
+        let (_, span) = self.regioncx.get_argument_name_and_span_for_region(
+            &self.body,
+            &self.local_names,
+            argument_index,
+        );
 
-        self.give_name_if_we_cannot_match_hir_ty(fr, arg_ty)
+        self.get_argument_hir_ty_for_highlighting(argument_index)
+            .and_then(|arg_hir_ty| self.highlight_if_we_can_match_hir_ty(fr, arg_ty, arg_hir_ty))
+            .or_else(|| {
+                // `highlight_if_we_cannot_match_hir_ty` needs to know the number we will give to
+                // the anonymous region. If it succeeds, the `synthesize_region_name` call below
+                // will increment the counter, "reserving" the number we just used.
+                let counter = *self.next_region_name.try_borrow().unwrap();
+                self.highlight_if_we_cannot_match_hir_ty(fr, arg_ty, span, counter)
+            })
+            .map(|highlight| RegionName {
+                name: self.synthesize_region_name(),
+                source: RegionNameSource::AnonRegionFromArgument(highlight),
+            })
     }
 
-    fn give_name_if_we_can_match_hir_ty_from_argument(
+    fn get_argument_hir_ty_for_highlighting(
         &self,
-        needle_fr: RegionVid,
-        argument_ty: Ty<'tcx>,
         argument_index: usize,
-    ) -> Option<RegionName> {
+    ) -> Option<&hir::Ty<'tcx>> {
         let mir_hir_id = self.infcx.tcx.hir().as_local_hir_id(self.mir_def_id);
         let fn_decl = self.infcx.tcx.hir().fn_decl_by_hir_id(mir_hir_id)?;
         let argument_hir_ty: &hir::Ty<'_> = fn_decl.inputs.get(argument_index)?;
@@ -333,7 +372,7 @@ impl<'tcx> MirBorrowckCtxt<'_, 'tcx> {
             // (`give_name_if_anonymous_region_appears_in_arguments`).
             hir::TyKind::Infer => None,
 
-            _ => self.give_name_if_we_can_match_hir_ty(needle_fr, argument_ty, argument_hir_ty),
+            _ => Some(argument_hir_ty),
         }
     }
 
@@ -348,42 +387,28 @@ impl<'tcx> MirBorrowckCtxt<'_, 'tcx> {
     ///  |          |  has type `&'1 u32`
     ///  |          has type `&'2 u32`
     /// ```
-    fn give_name_if_we_cannot_match_hir_ty(
+    fn highlight_if_we_cannot_match_hir_ty(
         &self,
         needle_fr: RegionVid,
-        argument_ty: Ty<'tcx>,
-    ) -> Option<RegionName> {
-        let counter = *self.next_region_name.try_borrow().unwrap();
+        ty: Ty<'tcx>,
+        span: Span,
+        counter: usize,
+    ) -> Option<RegionNameHighlight> {
         let mut highlight = RegionHighlightMode::default();
         highlight.highlighting_region_vid(needle_fr, counter);
-        let type_name = self.infcx.extract_type_name(&argument_ty, Some(highlight)).0;
+        let type_name = self.infcx.extract_type_name(&ty, Some(highlight)).0;
 
         debug!(
-            "give_name_if_we_cannot_match_hir_ty: type_name={:?} needle_fr={:?}",
+            "highlight_if_we_cannot_match_hir_ty: type_name={:?} needle_fr={:?}",
             type_name, needle_fr
         );
-        let assigned_region_name = if type_name.find(&format!("'{}", counter)).is_some() {
+        if type_name.find(&format!("'{}", counter)).is_some() {
             // Only add a label if we can confirm that a region was labelled.
-            let argument_index =
-                self.regioncx.get_argument_index_for_region(self.infcx.tcx, needle_fr)?;
-            let (_, span) = self.regioncx.get_argument_name_and_span_for_region(
-                &self.body,
-                &self.local_names,
-                argument_index,
-            );
-
-            Some(RegionName {
-                // This counter value will already have been used, so this function will increment
-                // it so the next value will be used next and return the region name that would
-                // have been used.
-                name: self.synthesize_region_name(),
-                source: RegionNameSource::CannotMatchHirTy(span, type_name),
-            })
+
+            Some(RegionNameHighlight::CannotMatchHirTy(span, type_name))
         } else {
             None
-        };
-
-        assigned_region_name
+        }
     }
 
     /// Attempts to highlight the specific part of a type annotation
@@ -395,9 +420,9 @@ impl<'tcx> MirBorrowckCtxt<'_, 'tcx> {
     ///  |                - let's call the lifetime of this reference `'1`
     /// ```
     ///
-    /// the way this works is that we match up `argument_ty`, which is
+    /// the way this works is that we match up `ty`, which is
     /// a `Ty<'tcx>` (the internal form of the type) with
-    /// `argument_hir_ty`, a `hir::Ty` (the syntax of the type
+    /// `hir_ty`, a `hir::Ty` (the syntax of the type
     /// annotation). We are descending through the types stepwise,
     /// looking in to find the region `needle_fr` in the internal
     /// type. Once we find that, we can use the span of the `hir::Ty`
@@ -407,18 +432,17 @@ impl<'tcx> MirBorrowckCtxt<'_, 'tcx> {
     /// keep track of the **closest** type we've found. If we fail to
     /// find the exact `&` or `'_` to highlight, then we may fall back
     /// to highlighting that closest type instead.
-    fn give_name_if_we_can_match_hir_ty(
+    fn highlight_if_we_can_match_hir_ty(
         &self,
         needle_fr: RegionVid,
-        argument_ty: Ty<'tcx>,
-        argument_hir_ty: &hir::Ty<'_>,
-    ) -> Option<RegionName> {
-        let search_stack: &mut Vec<(Ty<'tcx>, &hir::Ty<'_>)> =
-            &mut vec![(argument_ty, argument_hir_ty)];
+        ty: Ty<'tcx>,
+        hir_ty: &hir::Ty<'_>,
+    ) -> Option<RegionNameHighlight> {
+        let search_stack: &mut Vec<(Ty<'tcx>, &hir::Ty<'_>)> = &mut vec![(ty, hir_ty)];
 
         while let Some((ty, hir_ty)) = search_stack.pop() {
             match (&ty.kind, &hir_ty.kind) {
-                // Check if the `argument_ty` is `&'X ..` where `'X`
+                // Check if the `ty` is `&'X ..` where `'X`
                 // is the region we are looking for -- if so, and we have a `&T`
                 // on the RHS, then we want to highlight the `&` like so:
                 //
@@ -429,16 +453,11 @@ impl<'tcx> MirBorrowckCtxt<'_, 'tcx> {
                     hir::TyKind::Rptr(_lifetime, referent_hir_ty),
                 ) => {
                     if region.to_region_vid() == needle_fr {
-                        let region_name = self.synthesize_region_name();
-
                         // Just grab the first character, the `&`.
                         let source_map = self.infcx.tcx.sess.source_map();
                         let ampersand_span = source_map.start_point(hir_ty.span);
 
-                        return Some(RegionName {
-                            name: region_name,
-                            source: RegionNameSource::MatchedHirTy(ampersand_span),
-                        });
+                        return Some(RegionNameHighlight::MatchedHirTy(ampersand_span));
                     }
 
                     // Otherwise, let's descend into the referent types.
@@ -458,13 +477,13 @@ impl<'tcx> MirBorrowckCtxt<'_, 'tcx> {
                         Res::Def(DefKind::TyAlias, _) => (),
                         _ => {
                             if let Some(last_segment) = path.segments.last() {
-                                if let Some(name) = self.match_adt_and_segment(
+                                if let Some(highlight) = self.match_adt_and_segment(
                                     substs,
                                     needle_fr,
                                     last_segment,
                                     search_stack,
                                 ) {
-                                    return Some(name);
+                                    return Some(highlight);
                                 }
                             }
                         }
@@ -507,7 +526,7 @@ impl<'tcx> MirBorrowckCtxt<'_, 'tcx> {
         needle_fr: RegionVid,
         last_segment: &'hir hir::PathSegment<'hir>,
         search_stack: &mut Vec<(Ty<'tcx>, &'hir hir::Ty<'hir>)>,
-    ) -> Option<RegionName> {
+    ) -> Option<RegionNameHighlight> {
         // Did the user give explicit arguments? (e.g., `Foo<..>`)
         let args = last_segment.args.as_ref()?;
         let lifetime =
@@ -517,12 +536,8 @@ impl<'tcx> MirBorrowckCtxt<'_, 'tcx> {
             | hir::LifetimeName::Error
             | hir::LifetimeName::Static
             | hir::LifetimeName::Underscore => {
-                let region_name = self.synthesize_region_name();
-                let ampersand_span = lifetime.span;
-                Some(RegionName {
-                    name: region_name,
-                    source: RegionNameSource::MatchedAdtAndSegment(ampersand_span),
-                })
+                let lifetime_span = lifetime.span;
+                Some(RegionNameHighlight::MatchedAdtAndSegment(lifetime_span))
             }
 
             hir::LifetimeName::ImplicitObjectLifetimeDefault | hir::LifetimeName::Implicit => {