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

Diagnose anonymous lifetimes errors more uniformly between async and regular fns #97023

Merged
merged 5 commits into from
Jun 2, 2022
Merged

Diagnose anonymous lifetimes errors more uniformly between async and regular fns #97023

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
8da2707
Stop pretty-printing anonymous lifetimes.
cjgillot May 12, 2022
da175c7
Make lifetime errors more precise in the presence of `Fresh` lifetimes.
cjgillot May 12, 2022
7b86c6f
Fortify check for number of generic parameters.
cjgillot May 12, 2022
3162b33
Handle anonymous lifetimes properly in diagnostics.
cjgillot May 13, 2022
0cf79d7
Rebase fallout.
cjgillot May 29, 2022
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
10 changes: 6 additions & 4 deletions compiler/rustc_borrowck/src/diagnostics/region_name.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -567,15 +567,17 @@ impl<'tcx> MirBorrowckCtxt<'_, 'tcx> {
let lifetime =
self.try_match_adt_and_generic_args(substs, needle_fr, args, search_stack)?;
match lifetime.name {
hir::LifetimeName::Param(_)
hir::LifetimeName::Param(hir::ParamName::Plain(_) | hir::ParamName::Error)
| hir::LifetimeName::Error
| hir::LifetimeName::Static
| hir::LifetimeName::Underscore => {
| hir::LifetimeName::Static => {
let lifetime_span = lifetime.span;
Some(RegionNameHighlight::MatchedAdtAndSegment(lifetime_span))
}

hir::LifetimeName::ImplicitObjectLifetimeDefault | hir::LifetimeName::Implicit => {
cjgillot marked this conversation as resolved.
Show resolved Hide resolved
hir::LifetimeName::Param(hir::ParamName::Fresh(_))
| hir::LifetimeName::ImplicitObjectLifetimeDefault
| hir::LifetimeName::Implicit
| hir::LifetimeName::Underscore => {
// In this case, the user left off the lifetime; so
// they wrote something like:
//
Expand Down
11 changes: 11 additions & 0 deletions compiler/rustc_hir/src/hir.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,6 +131,17 @@ impl LifetimeName {
}
}

pub fn is_anonymous(&self) -> bool {
match *self {
LifetimeName::ImplicitObjectLifetimeDefault
| LifetimeName::Implicit
| LifetimeName::Underscore
| LifetimeName::Param(ParamName::Fresh(_))
| LifetimeName::Error => true,
LifetimeName::Static | LifetimeName::Param(_) => false,
}
}

pub fn is_elided(&self) -> bool {
match self {
LifetimeName::ImplicitObjectLifetimeDefault
Expand Down
62 changes: 36 additions & 26 deletions compiler/rustc_infer/src/infer/error_reporting/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ use rustc_middle::ty::{
subst::{GenericArgKind, Subst, SubstsRef},
Binder, EarlyBinder, List, Region, Ty, TyCtxt, TypeFoldable,
};
use rustc_span::{sym, BytePos, DesugaringKind, Pos, Span};
use rustc_span::{sym, symbol::kw, BytePos, DesugaringKind, Pos, Span};
use rustc_target::spec::abi;
use std::ops::ControlFlow;
use std::{cmp, fmt, iter};
Expand Down Expand Up @@ -161,35 +161,45 @@ fn msg_span_from_early_bound_and_free_regions<'tcx>(
{
sp = param.span;
}
(format!("the lifetime `{}` as defined here", br.name), sp)
let text = if br.has_name() {
format!("the lifetime `{}` as defined here", br.name)
} else {
format!("the anonymous lifetime as defined here")
};
(text, sp)
}
ty::ReFree(ty::FreeRegion {
bound_region: ty::BoundRegionKind::BrNamed(_, name), ..
}) => {
let mut sp = sm.guess_head_span(tcx.def_span(scope));
if let Some(param) =
tcx.hir().get_generics(scope).and_then(|generics| generics.get_named(name))
ty::ReFree(ref fr) => {
if !fr.bound_region.is_named()
&& let Some((ty, _)) = find_anon_type(tcx, region, &fr.bound_region)
{
sp = param.span;
}
(format!("the lifetime `{}` as defined here", name), sp)
}
ty::ReFree(ref fr) => match fr.bound_region {
ty::BrAnon(idx) => {
if let Some((ty, _)) = find_anon_type(tcx, region, &fr.bound_region) {
("the anonymous lifetime defined here".to_string(), ty.span)
} else {
(
("the anonymous lifetime defined here".to_string(), ty.span)
} else {
match fr.bound_region {
ty::BoundRegionKind::BrNamed(_, name) => {
let mut sp = sm.guess_head_span(tcx.def_span(scope));
if let Some(param) =
tcx.hir().get_generics(scope).and_then(|generics| generics.get_named(name))
{
sp = param.span;
}
let text = if name == kw::UnderscoreLifetime {
format!("the anonymous lifetime as defined here")
} else {
format!("the lifetime `{}` as defined here", name)
};
(text, sp)
}
ty::BrAnon(idx) => (
format!("the anonymous lifetime #{} defined here", idx + 1),
tcx.def_span(scope),
)
tcx.def_span(scope)
),
_ => (
format!("the lifetime `{}` as defined here", region),
sm.guess_head_span(tcx.def_span(scope)),
),
}
}
_ => (
format!("the lifetime `{}` as defined here", region),
sm.guess_head_span(tcx.def_span(scope)),
),
},
}
_ => bug!(),
}
}
Expand Down Expand Up @@ -2545,7 +2555,7 @@ impl<'a, 'tcx> InferCtxt<'a, 'tcx> {
ty::ReEarlyBound(ty::EarlyBoundRegion { name, .. })
| ty::ReFree(ty::FreeRegion { bound_region: ty::BrNamed(_, name), .. }),
_,
) => {
) if name != kw::UnderscoreLifetime => {
// Does the required lifetime have a nice name we can print?
let mut err = struct_span_err!(
self.tcx.sess,
Expand Down
50 changes: 28 additions & 22 deletions compiler/rustc_infer/src/infer/error_reporting/nice_region_error/different_lifetimes.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ use rustc_errors::{struct_span_err, Applicability, Diagnostic, ErrorGuaranteed};
use rustc_hir as hir;
use rustc_hir::{GenericParamKind, Ty};
use rustc_middle::ty::Region;
use rustc_span::symbol::kw;

impl<'a, 'tcx> NiceRegionError<'a, 'tcx> {
/// Print the error message for lifetime errors when both the concerned regions are anonymous.
Expand Down Expand Up @@ -169,7 +170,7 @@ pub fn suggest_adding_lifetime_params<'tcx>(
return false;
};

if !lifetime_sub.name.is_elided() || !lifetime_sup.name.is_elided() {
if !lifetime_sub.name.is_anonymous() || !lifetime_sup.name.is_anonymous() {
return false;
};

Expand All @@ -188,32 +189,37 @@ pub fn suggest_adding_lifetime_params<'tcx>(
_ => return false,
};

let (suggestion_param_name, introduce_new) = generics
let suggestion_param_name = generics
.params
.iter()
.find(|p| matches!(p.kind, GenericParamKind::Lifetime { .. }))
.and_then(|p| tcx.sess.source_map().span_to_snippet(p.span).ok())
.map(|name| (name, false))
.unwrap_or_else(|| ("'a".to_string(), true));

let mut suggestions = vec![
if let hir::LifetimeName::Underscore = lifetime_sub.name {
(lifetime_sub.span, suggestion_param_name.clone())
.filter(|p| matches!(p.kind, GenericParamKind::Lifetime { .. }))
.map(|p| p.name.ident().name)
.find(|i| *i != kw::UnderscoreLifetime);
let introduce_new = suggestion_param_name.is_none();
let suggestion_param_name =
suggestion_param_name.map(|n| n.to_string()).unwrap_or_else(|| "'a".to_owned());

debug!(?lifetime_sup.span);
debug!(?lifetime_sub.span);
let make_suggestion = |span: rustc_span::Span| {
if span.is_empty() {
(span, format!("{}, ", suggestion_param_name))
} else if let Ok("&") = tcx.sess.source_map().span_to_snippet(span).as_deref() {
(span.shrink_to_hi(), format!("{} ", suggestion_param_name))
} else {
(lifetime_sub.span.shrink_to_hi(), suggestion_param_name.clone() + " ")
},
if let hir::LifetimeName::Underscore = lifetime_sup.name {
(lifetime_sup.span, suggestion_param_name.clone())
} else {
(lifetime_sup.span.shrink_to_hi(), suggestion_param_name.clone() + " ")
},
];
(span, suggestion_param_name.clone())
}
};
let mut suggestions =
vec![make_suggestion(lifetime_sub.span), make_suggestion(lifetime_sup.span)];

if introduce_new {
let new_param_suggestion = match &generics.params {
[] => (generics.span, format!("<{}>", suggestion_param_name)),
[first, ..] => (first.span.shrink_to_lo(), format!("{}, ", suggestion_param_name)),
};
let new_param_suggestion =
if let Some(first) = generics.params.iter().find(|p| !p.name.ident().span.is_empty()) {
(first.span.shrink_to_lo(), format!("{}, ", suggestion_param_name))
} else {
(generics.span, format!("<{}>", suggestion_param_name))
};

suggestions.push(new_param_suggestion);
}
Expand Down
3 changes: 2 additions & 1 deletion compiler/rustc_infer/src/infer/error_reporting/nice_region_error/named_anon_conflict.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ use crate::infer::error_reporting::nice_region_error::find_anon_type::find_anon_
use crate::infer::error_reporting::nice_region_error::NiceRegionError;
use rustc_errors::{struct_span_err, Applicability, DiagnosticBuilder, ErrorGuaranteed};
use rustc_middle::ty;
use rustc_span::symbol::kw;

impl<'a, 'tcx> NiceRegionError<'a, 'tcx> {
/// When given a `ConcreteFailure` for a function with parameters containing a named region and
Expand Down Expand Up @@ -67,7 +68,7 @@ impl<'a, 'tcx> NiceRegionError<'a, 'tcx> {
let is_impl_item = region_info.is_impl_item;

match br {
ty::BrAnon(_) => {}
ty::BrNamed(_, kw::UnderscoreLifetime) | ty::BrAnon(_) => {}
_ => {
/* not an anonymous region */
debug!("try_report_named_anon_conflict: not an anonymous region");
Expand Down
71 changes: 28 additions & 43 deletions compiler/rustc_middle/src/ty/print/pretty.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2177,61 +2177,47 @@ impl<'tcx> FmtPrinter<'_, 'tcx> {
define_scoped_cx!(self);

let mut region_index = self.region_index;
let mut next_name = |this: &Self| loop {
let name = name_by_region_index(region_index);
region_index += 1;
if !this.used_region_names.contains(&name) {
break name;
}
};

// If we want to print verbosely, then print *all* binders, even if they
// aren't named. Eventually, we might just want this as the default, but
// this is not *quite* right and changes the ordering of some output
// anyways.
let (new_value, map) = if self.tcx().sess.verbose() {
// anon index + 1 (BrEnv takes 0) -> name
let mut region_map: BTreeMap<u32, Symbol> = BTreeMap::default();
let mut region_map: FxHashMap<_, _> = Default::default();
Copy link
Member

Choose a reason for hiding this comment

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

This code is a bit sus to me. This relies on us not having duplicate BoundRegionKinds in binders. This is how the compiler behaves today (or should barring bugs), but there isn't nothing to say that we can't (you could, for example, imagine that we drop the anon index).

This makes more sense as just a Vec where each index is just the BoundRegionKind that corresponds to the binder (or just some placeholder value for non-regions) and then use BoundRegion.var.as_usize() as an index into that vec, which is exactly how bound variables work.

(I'm making these changes currently for the bug below).

let bound_vars = value.bound_vars();
for var in bound_vars {
let ty::BoundVariableKind::Region(var) = var else { continue };
match var {
ty::BoundVariableKind::Region(ty::BrNamed(_, name)) => {
ty::BrAnon(_) | ty::BrEnv => {
start_or_continue(&mut self, "for<", ", ");
let name = next_name(&self);
do_continue(&mut self, name);
region_map.insert(var, ty::BrNamed(CRATE_DEF_ID.to_def_id(), name));
}
ty::BoundVariableKind::Region(ty::BrAnon(i)) => {
ty::BrNamed(def_id, kw::UnderscoreLifetime) => {
start_or_continue(&mut self, "for<", ", ");
let name = loop {
let name = name_by_region_index(region_index);
region_index += 1;
if !self.used_region_names.contains(&name) {
break name;
}
};
let name = next_name(&self);
do_continue(&mut self, name);
region_map.insert(i + 1, name);
region_map.insert(var, ty::BrNamed(def_id, name));
}
ty::BoundVariableKind::Region(ty::BrEnv) => {
ty::BrNamed(_, name) => {
start_or_continue(&mut self, "for<", ", ");
let name = loop {
let name = name_by_region_index(region_index);
region_index += 1;
if !self.used_region_names.contains(&name) {
break name;
}
};
do_continue(&mut self, name);
region_map.insert(0, name);
}
_ => continue,
}
}
start_or_continue(&mut self, "", "> ");

self.tcx.replace_late_bound_regions(value.clone(), |br| {
let kind = match br.kind {
ty::BrNamed(_, _) => br.kind,
ty::BrAnon(i) => {
let name = region_map[&(i + 1)];
ty::BrNamed(CRATE_DEF_ID.to_def_id(), name)
}
ty::BrEnv => {
let name = region_map[&0];
ty::BrNamed(CRATE_DEF_ID.to_def_id(), name)
}
};
let kind = region_map[&br.kind];
Comment on lines -2224 to +2220
Copy link
Member

Choose a reason for hiding this comment

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

This broke cases under -Zverbose where we try to print a named bound region. Above, we don't insert them in the region_map but here we try to lookup the variable anyways.

self.tcx.mk_region(ty::ReLateBound(
ty::INNERMOST,
ty::BoundRegion { var: br.var, kind },
Expand All @@ -2242,21 +2228,20 @@ impl<'tcx> FmtPrinter<'_, 'tcx> {
let mut name = |br: ty::BoundRegion| {
start_or_continue(&mut self, "for<", ", ");
let kind = match br.kind {
ty::BrNamed(_, name) => {
do_continue(&mut self, name);
br.kind
}
ty::BrAnon(_) | ty::BrEnv => {
let name = loop {
let name = name_by_region_index(region_index);
region_index += 1;
if !self.used_region_names.contains(&name) {
break name;
}
};
let name = next_name(&self);
do_continue(&mut self, name);
ty::BrNamed(CRATE_DEF_ID.to_def_id(), name)
}
ty::BrNamed(def_id, kw::UnderscoreLifetime) => {
let name = next_name(&self);
do_continue(&mut self, name);
ty::BrNamed(def_id, name)
}
ty::BrNamed(_, name) => {
do_continue(&mut self, name);
br.kind
}
};
tcx.mk_region(ty::ReLateBound(ty::INNERMOST, ty::BoundRegion { var: br.var, kind }))
};
Expand Down
Loading