Skip to content
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.

Sign up for GitHub

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

Refactor region_name: add RegionNameHighlight #74661

Merged
merged 8 commits into from
Jul 24, 2020
Merged

Refactor region_name: add RegionNameHighlight #74661

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
c276334
add RegionName::span
SNCPlay42 Jul 21, 2020
51af5af
extract RegionNameHighlight
SNCPlay42 Jul 22, 2020
601518e
change returns to RegionNameHighlight
SNCPlay42 Jul 22, 2020
723ea90
rename functions
SNCPlay42 Jul 22, 2020
b56f5b9
clean up give_name_if_anonymous_region_appears_in_arguments
SNCPlay42 Jul 22, 2020
a7450b7
decouple highlight_if_we_cannot_match_hir_ty
SNCPlay42 Jul 22, 2020
ebb4aba
move highlight_if_we_can_match_hir_ty call
SNCPlay42 Jul 22, 2020
8a776ee
rename arguments to highlight_if_we_can_match_hir_ty
SNCPlay42 Jul 22, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 1 addition & 3 deletions src/librustc_mir/borrow_check/diagnostics/outlives_suggestion.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(..)
Expand Down
16 changes: 3 additions & 13 deletions src/librustc_mir/borrow_check/diagnostics/region_errors.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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");
}
_ => {}
Comment on lines -399 to -410
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This match was missing a few variants that have spans (AnonRegionFromYieldTy/AnonRegionFromAsyncFn). I don't think that was deliberate, just that they were allowed to be absent by the underscore pattern. On the other hand I'm not sure that they would be used in this code.

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(
Expand Down
161 changes: 88 additions & 73 deletions src/librustc_mir/borrow_check/diagnostics/region_name.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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)
Expand All @@ -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) => {
Expand Down Expand Up @@ -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)
})
Comment on lines +346 to +353
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idea is that this bit (from the highlight_if_we_can_match_hir_ty call down) can be repeated to create RegionNameHighlights in other places. I would have extracted this into its own function in this PR, but I'm a bit concerned about this business with counter, and if increasing the syntactic distance from the synthesize_region_name call will make things unclear.

.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)?;
Expand All @@ -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),
}
}

Expand All @@ -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
Expand All @@ -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`
Expand All @@ -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:
//
Expand All @@ -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.
Expand All @@ -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);
}
}
}
Expand Down Expand Up @@ -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 =
Expand All @@ -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 => {
Expand Down