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

Fix suggestion to use lifetime in type and in assoc const #75372

Merged
merged 5 commits into from
Aug 13, 2020
Merged

Fix suggestion to use lifetime in type and in assoc const #75372

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
0f7205f
Fix suggestion to use lifetime in type
estebank Aug 10, 2020
7956b1c
Assoc `const`s don't have generics
estebank Aug 10, 2020
b9585fd
When suggesting `for` lts, consider existing lifetime names
estebank Aug 10, 2020
becd479
review comment: simplify code by using slice pat
estebank Aug 11, 2020
6a3deb0
Suggest using `'static` in assoc consts and suggest when multiple lts…
estebank Aug 11, 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
161 changes: 137 additions & 24 deletions src/librustc_resolve/late/diagnostics.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,8 @@ use rustc_hir::def_id::{DefId, CRATE_DEF_INDEX, LOCAL_CRATE};
use rustc_hir::PrimTy;
use rustc_session::config::nightly_options;
use rustc_span::hygiene::MacroKind;
use rustc_span::symbol::{kw, sym, Ident};
use rustc_span::{BytePos, Span};
use rustc_span::symbol::{kw, sym, Ident, Symbol};
use rustc_span::{BytePos, Span, DUMMY_SP};

use log::debug;

Expand All @@ -33,6 +33,7 @@ enum AssocSuggestion {
crate enum MissingLifetimeSpot<'tcx> {
Generics(&'tcx hir::Generics<'tcx>),
HigherRanked { span: Span, span_type: ForLifetimeSpanType },
Static,
}

crate enum ForLifetimeSpanType {
Expand Down Expand Up @@ -1186,6 +1187,7 @@ impl<'tcx> LifetimeContext<'_, 'tcx> {
https://doc.rust-lang.org/nomicon/hrtb.html",
);
}
_ => {}
}
}
if nightly_options::is_nightly_build()
Expand Down Expand Up @@ -1244,7 +1246,8 @@ impl<'tcx> LifetimeContext<'_, 'tcx> {
err: &mut DiagnosticBuilder<'_>,
span: Span,
count: usize,
lifetime_names: &FxHashSet<Ident>,
lifetime_names: &FxHashSet<Symbol>,
lifetime_spans: Vec<Span>,
params: &[ElisionFailureInfo],
) {
let snippet = self.tcx.sess.source_map().span_to_snippet(span).ok();
Expand All @@ -1258,11 +1261,60 @@ impl<'tcx> LifetimeContext<'_, 'tcx> {
),
);

let suggest_existing = |err: &mut DiagnosticBuilder<'_>, sugg| {
let suggest_existing = |err: &mut DiagnosticBuilder<'_>,
name: &str,
formatter: &dyn Fn(&str) -> String| {
if let Some(MissingLifetimeSpot::HigherRanked { span: for_span, span_type }) =
self.missing_named_lifetime_spots.iter().rev().next()
{
// When we have `struct S<'a>(&'a dyn Fn(&X) -> &X);` we want to not only suggest
// using `'a`, but also introduce the concept of HRLTs by suggesting
// `struct S<'a>(&'a dyn for<'b> Fn(&X) -> &'b X);`. (#72404)
let mut introduce_suggestion = vec![];

let a_to_z_repeat_n = |n| {
(b'a'..=b'z').map(move |c| {
let mut s = '\''.to_string();
s.extend(std::iter::repeat(char::from(c)).take(n));
s
})
};

// If all single char lifetime names are present, we wrap around and double the chars.
let lt_name = (1..)
.flat_map(a_to_z_repeat_n)
.find(|lt| !lifetime_names.contains(&Symbol::intern(&lt)))
.unwrap();
let msg = format!(
"consider making the {} lifetime-generic with a new `{}` lifetime",
span_type.descr(),
lt_name,
);
err.note(
"for more information on higher-ranked polymorphism, visit \
https://doc.rust-lang.org/nomicon/hrtb.html",
);
let for_sugg = span_type.suggestion(&lt_name);
for param in params {
if let Ok(snippet) = self.tcx.sess.source_map().span_to_snippet(param.span) {
if snippet.starts_with('&') && !snippet.starts_with("&'") {
introduce_suggestion
.push((param.span, format!("&{} {}", lt_name, &snippet[1..])));
} else if snippet.starts_with("&'_ ") {
introduce_suggestion
.push((param.span, format!("&{} {}", lt_name, &snippet[4..])));
}
}
}
introduce_suggestion.push((*for_span, for_sugg.to_string()));
introduce_suggestion.push((span, formatter(&lt_name)));
err.multipart_suggestion(&msg, introduce_suggestion, Applicability::MaybeIncorrect);
}

err.span_suggestion_verbose(
span,
&format!("consider using the `{}` lifetime", lifetime_names.iter().next().unwrap()),
sugg,
formatter(name),
Applicability::MaybeIncorrect,
);
};
Expand All @@ -1273,6 +1325,15 @@ impl<'tcx> LifetimeContext<'_, 'tcx> {
let should_break;
introduce_suggestion.push(match missing {
MissingLifetimeSpot::Generics(generics) => {
if generics.span == DUMMY_SP {
// Account for malformed generics in the HIR. This shouldn't happen,
// but if we make a mistake elsewhere, mainly by keeping something in
// `missing_named_lifetime_spots` that we shouldn't, like associated
// `const`s or making a mistake in the AST lowering we would provide
// non-sensical suggestions. Guard against that by skipping these.
// (#74264)
continue;
}
msg = "consider introducing a named lifetime parameter".to_string();
should_break = true;
if let Some(param) = generics.params.iter().find(|p| match p.kind {
Expand All @@ -1299,6 +1360,42 @@ impl<'tcx> LifetimeContext<'_, 'tcx> {
);
(*span, span_type.suggestion("'a"))
}
MissingLifetimeSpot::Static => {
let (span, sugg) = match snippet.as_deref() {
Some("&") => (span.shrink_to_hi(), "'static ".to_owned()),
Some("'_") => (span, "'static".to_owned()),
Some(snippet) if !snippet.ends_with('>') => {
if snippet == "" {
(
span,
std::iter::repeat("'static")
.take(count)
.collect::<Vec<_>>()
.join(", "),
)
} else {
(
span.shrink_to_hi(),
format!(
"<{}>",
std::iter::repeat("'static")
.take(count)
.collect::<Vec<_>>()
.join(", ")
),
)
}
}
_ => continue,
};
err.span_suggestion_verbose(
span,
"consider using the `'static` lifetime",
sugg.to_string(),
Applicability::MaybeIncorrect,
);
continue;
}
});
for param in params {
if let Ok(snippet) = self.tcx.sess.source_map().span_to_snippet(param.span) {
Expand All @@ -1319,41 +1416,57 @@ impl<'tcx> LifetimeContext<'_, 'tcx> {
}
};

match (lifetime_names.len(), lifetime_names.iter().next(), snippet.as_deref()) {
(1, Some(name), Some("&")) => {
suggest_existing(err, format!("&{} ", name));
let lifetime_names: Vec<_> = lifetime_names.into_iter().collect();
match (&lifetime_names[..], snippet.as_deref()) {
([name], Some("&")) => {
suggest_existing(err, &name.as_str()[..], &|name| format!("&{} ", name));
}
(1, Some(name), Some("'_")) => {
suggest_existing(err, name.to_string());
([name], Some("'_")) => {
suggest_existing(err, &name.as_str()[..], &|n| n.to_string());
}
(1, Some(name), Some("")) => {
suggest_existing(err, format!("{}, ", name).repeat(count));
([name], Some("")) => {
suggest_existing(err, &name.as_str()[..], &|n| format!("{}, ", n).repeat(count));
lcnr marked this conversation as resolved.
Show resolved Hide resolved
}
(1, Some(name), Some(snippet)) if !snippet.ends_with('>') => {
suggest_existing(
err,
([name], Some(snippet)) if !snippet.ends_with('>') => {
let f = |name: &str| {
format!(
"{}<{}>",
snippet,
std::iter::repeat(name.to_string())
.take(count)
.collect::<Vec<_>>()
.join(", ")
),
);
)
};
suggest_existing(err, &name.as_str()[..], &f);
}
(0, _, Some("&")) if count == 1 => {
([], Some("&")) if count == 1 => {
suggest_new(err, "&'a ");
}
(0, _, Some("'_")) if count == 1 => {
([], Some("'_")) if count == 1 => {
suggest_new(err, "'a");
}
(0, _, Some(snippet)) if !snippet.ends_with('>') && count == 1 => {
suggest_new(err, &format!("{}<'a>", snippet));
([], Some(snippet)) if !snippet.ends_with('>') => {
if snippet == "" {
// This happens when we have `type Bar<'a> = Foo<T>` where we point at the space
// before `T`. We will suggest `type Bar<'a> = Foo<'a, T>`.
suggest_new(
err,
&std::iter::repeat("'a, ").take(count).collect::<Vec<_>>().join(""),
);
} else {
suggest_new(
err,
&format!(
"{}<{}>",
snippet,
std::iter::repeat("'a").take(count).collect::<Vec<_>>().join(", ")
),
);
}
}
(n, ..) if n > 1 => {
let spans: Vec<Span> = lifetime_names.iter().map(|lt| lt.span).collect();
err.span_note(spans, "these named lifetimes are available to use");
(lts, ..) if lts.len() > 1 => {
err.span_note(lifetime_spans, "these named lifetimes are available to use");
if Some("") == snippet.as_deref() {
// This happens when we have `Foo<T>` where we point at the space before `T`,
// but this can be confusing so we give a suggestion with placeholders.
Expand Down
47 changes: 35 additions & 12 deletions src/librustc_resolve/late/lifetimes.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -711,18 +711,20 @@ impl<'a, 'tcx> Visitor<'tcx> for LifetimeContext<'a, 'tcx> {

fn visit_trait_item(&mut self, trait_item: &'tcx hir::TraitItem<'tcx>) {
use self::hir::TraitItemKind::*;
self.missing_named_lifetime_spots.push((&trait_item.generics).into());
match trait_item.kind {
Fn(ref sig, _) => {
self.missing_named_lifetime_spots.push((&trait_item.generics).into());
let tcx = self.tcx;
self.visit_early_late(
Some(tcx.hir().get_parent_item(trait_item.hir_id)),
&sig.decl,
&trait_item.generics,
|this| intravisit::walk_trait_item(this, trait_item),
);
self.missing_named_lifetime_spots.pop();
}
Type(bounds, ref ty) => {
self.missing_named_lifetime_spots.push((&trait_item.generics).into());
let generics = &trait_item.generics;
let mut index = self.next_early_index();
debug!("visit_ty: index = {}", index);
Expand Down Expand Up @@ -757,31 +759,35 @@ impl<'a, 'tcx> Visitor<'tcx> for LifetimeContext<'a, 'tcx> {
this.visit_ty(ty);
}
});
self.missing_named_lifetime_spots.pop();
}
Const(_, _) => {
// Only methods and types support generics.
assert!(trait_item.generics.params.is_empty());
self.missing_named_lifetime_spots.push(MissingLifetimeSpot::Static);
intravisit::walk_trait_item(self, trait_item);
self.missing_named_lifetime_spots.pop();
}
}
self.missing_named_lifetime_spots.pop();
}

fn visit_impl_item(&mut self, impl_item: &'tcx hir::ImplItem<'tcx>) {
use self::hir::ImplItemKind::*;
self.missing_named_lifetime_spots.push((&impl_item.generics).into());
match impl_item.kind {
Fn(ref sig, _) => {
self.missing_named_lifetime_spots.push((&impl_item.generics).into());
let tcx = self.tcx;
self.visit_early_late(
Some(tcx.hir().get_parent_item(impl_item.hir_id)),
&sig.decl,
&impl_item.generics,
|this| intravisit::walk_impl_item(this, impl_item),
)
);
self.missing_named_lifetime_spots.pop();
}
TyAlias(ref ty) => {
let generics = &impl_item.generics;
self.missing_named_lifetime_spots.push(generics.into());
let mut index = self.next_early_index();
let mut non_lifetime_count = 0;
debug!("visit_ty: index = {}", index);
Expand Down Expand Up @@ -810,14 +816,16 @@ impl<'a, 'tcx> Visitor<'tcx> for LifetimeContext<'a, 'tcx> {
this.visit_generics(generics);
this.visit_ty(ty);
});
self.missing_named_lifetime_spots.pop();
}
Const(_, _) => {
// Only methods and types support generics.
assert!(impl_item.generics.params.is_empty());
self.missing_named_lifetime_spots.push(MissingLifetimeSpot::Static);
intravisit::walk_impl_item(self, impl_item);
self.missing_named_lifetime_spots.pop();
}
}
self.missing_named_lifetime_spots.pop();
}

fn visit_lifetime(&mut self, lifetime_ref: &'tcx hir::Lifetime) {
Expand Down Expand Up @@ -2315,6 +2323,7 @@ impl<'a, 'tcx> LifetimeContext<'a, 'tcx> {
let mut late_depth = 0;
let mut scope = self.scope;
let mut lifetime_names = FxHashSet::default();
let mut lifetime_spans = vec![];
let error = loop {
match *scope {
// Do not assign any resolution, it will be inferred.
Expand All @@ -2326,7 +2335,8 @@ impl<'a, 'tcx> LifetimeContext<'a, 'tcx> {
// collect named lifetimes for suggestions
for name in lifetimes.keys() {
if let hir::ParamName::Plain(name) = name {
lifetime_names.insert(*name);
lifetime_names.insert(name.name);
lifetime_spans.push(name.span);
}
}
late_depth += 1;
Expand All @@ -2344,12 +2354,24 @@ impl<'a, 'tcx> LifetimeContext<'a, 'tcx> {
}
Elide::Exact(l) => l.shifted(late_depth),
Elide::Error(ref e) => {
if let Scope::Binder { ref lifetimes, .. } = s {
// collect named lifetimes for suggestions
for name in lifetimes.keys() {
if let hir::ParamName::Plain(name) = name {
lifetime_names.insert(*name);
let mut scope = s;
loop {
match scope {
Scope::Binder { ref lifetimes, s, .. } => {
// Collect named lifetimes for suggestions.
for name in lifetimes.keys() {
if let hir::ParamName::Plain(name) = name {
lifetime_names.insert(name.name);
lifetime_spans.push(name.span);
}
}
scope = s;
}
Scope::ObjectLifetimeDefault { ref s, .. }
| Scope::Elision { ref s, .. } => {
scope = s;
}
_ => break,
}
}
break Some(e);
Expand All @@ -2373,14 +2395,15 @@ impl<'a, 'tcx> LifetimeContext<'a, 'tcx> {
if let Some(params) = error {
// If there's no lifetime available, suggest `'static`.
if self.report_elision_failure(&mut err, params) && lifetime_names.is_empty() {
lifetime_names.insert(Ident::with_dummy_span(kw::StaticLifetime));
lifetime_names.insert(kw::StaticLifetime);
}
}
self.add_missing_lifetime_specifiers_label(
&mut err,
span,
lifetime_refs.len(),
&lifetime_names,
lifetime_spans,
error.map(|p| &p[..]).unwrap_or(&[]),
);
err.emit();
Expand Down
5 changes: 5 additions & 0 deletions src/test/ui/associated-types/bound-lifetime-in-binding-only.elision.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,11 @@ LL | fn elision<T: Fn() -> &i32>() {
| ^ expected named lifetime parameter
|
= help: this function's return type contains a borrowed value, but there is no value for it to be borrowed from
= note: for more information on higher-ranked polymorphism, visit https://doc.rust-lang.org/nomicon/hrtb.html
help: consider making the bound lifetime-generic with a new `'a` lifetime
|
LL | fn elision<T: for<'a> Fn() -> &'a i32>() {
| ^^^^^^^ ^^^
help: consider using the `'static` lifetime
|
LL | fn elision<T: Fn() -> &'static i32>() {
Expand Down
Loading