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.

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

Tweak 'static suggestion code #71235

Merged
merged 6 commits into from
Apr 24, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
5 changes: 4 additions & 1 deletion src/librustc_ast_lowering/path.rs
Original file line number Diff line number Diff line change
Expand Up @@ -273,7 +273,10 @@ impl<'a, 'hir> LoweringContext<'a, 'hir> {
.next();
if !generic_args.parenthesized && !has_lifetimes {
generic_args.args = self
.elided_path_lifetimes(path_span, expected_lifetimes)
.elided_path_lifetimes(
first_generic_span.map(|s| s.shrink_to_lo()).unwrap_or(segment.ident.span),
expected_lifetimes,
)
.map(GenericArg::Lifetime)
.chain(generic_args.args.into_iter())
.collect();
Expand Down
198 changes: 111 additions & 87 deletions src/librustc_resolve/late/diagnostics.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1034,101 +1034,125 @@ impl<'tcx> LifetimeContext<'_, 'tcx> {
lifetime_names: &FxHashSet<ast::Ident>,
params: &[ElisionFailureInfo],
) {
if count > 1 {
err.span_label(span, format!("expected {} lifetime parameters", count));
} else {
let snippet = self.tcx.sess.source_map().span_to_snippet(span).ok();
let suggest_existing = |err: &mut DiagnosticBuilder<'_>, sugg| {
err.span_suggestion(
span,
"consider using the named lifetime",
sugg,
Applicability::MaybeIncorrect,
);
};
let suggest_new = |err: &mut DiagnosticBuilder<'_>, sugg: &str| {
err.span_label(span, "expected named lifetime parameter");
let snippet = self.tcx.sess.source_map().span_to_snippet(span).ok();

for missing in self.missing_named_lifetime_spots.iter().rev() {
let mut introduce_suggestion = vec![];
let msg;
let should_break;
introduce_suggestion.push(match missing {
MissingLifetimeSpot::Generics(generics) => {
msg = "consider introducing a named lifetime parameter".to_string();
should_break = true;
if let Some(param) = generics.params.iter().find(|p| match p.kind {
hir::GenericParamKind::Type {
synthetic: Some(hir::SyntheticTyParamKind::ImplTrait),
..
} => false,
_ => true,
}) {
(param.span.shrink_to_lo(), "'a, ".to_string())
} else {
(generics.span, "<'a>".to_string())
}
}
MissingLifetimeSpot::HigherRanked { span, span_type } => {
msg = format!(
"consider making the {} lifetime-generic with a new `'a` lifetime",
span_type.descr(),
);
should_break = false;
err.note(
"for more information on higher-ranked polymorphism, visit \
https://doc.rust-lang.org/nomicon/hrtb.html",
);
(*span, span_type.suggestion("'a"))
}
});
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!("&'a {}", &snippet[1..])));
} else if snippet.starts_with("&'_ ") {
introduce_suggestion
.push((param.span, format!("&'a {}", &snippet[4..])));
}
err.span_label(
span,
&format!(
"expected {} lifetime parameter{}",
if count == 1 { "named".to_string() } else { count.to_string() },
pluralize!(count)
),
);

let suggest_existing = |err: &mut DiagnosticBuilder<'_>, sugg| {
err.span_suggestion_verbose(
span,
&format!("consider using the `{}` lifetime", lifetime_names.iter().next().unwrap()),
sugg,
Applicability::MaybeIncorrect,
);
};
let suggest_new = |err: &mut DiagnosticBuilder<'_>, sugg: &str| {
for missing in self.missing_named_lifetime_spots.iter().rev() {
let mut introduce_suggestion = vec![];
let msg;
let should_break;
introduce_suggestion.push(match missing {
MissingLifetimeSpot::Generics(generics) => {
msg = "consider introducing a named lifetime parameter".to_string();
should_break = true;
if let Some(param) = generics.params.iter().find(|p| match p.kind {
hir::GenericParamKind::Type {
synthetic: Some(hir::SyntheticTyParamKind::ImplTrait),
..
} => false,
_ => true,
}) {
(param.span.shrink_to_lo(), "'a, ".to_string())
} else {
(generics.span, "<'a>".to_string())
}
}
introduce_suggestion.push((span, sugg.to_string()));
err.multipart_suggestion(
&msg,
introduce_suggestion,
Applicability::MaybeIncorrect,
);
if should_break {
break;
MissingLifetimeSpot::HigherRanked { span, span_type } => {
msg = format!(
"consider making the {} lifetime-generic with a new `'a` lifetime",
span_type.descr(),
);
should_break = false;
err.note(
"for more information on higher-ranked polymorphism, visit \
https://doc.rust-lang.org/nomicon/hrtb.html",
);
(*span, span_type.suggestion("'a"))
}
});
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!("&'a {}", &snippet[1..])));
} else if snippet.starts_with("&'_ ") {
introduce_suggestion
.push((param.span, format!("&'a {}", &snippet[4..])));
}
}
}
};

match (lifetime_names.len(), lifetime_names.iter().next(), snippet.as_deref()) {
(1, Some(name), Some("&")) => {
suggest_existing(err, format!("&{} ", name));
}
(1, Some(name), Some("'_")) => {
suggest_existing(err, name.to_string());
}
(1, Some(name), Some(snippet)) if !snippet.ends_with('>') => {
suggest_existing(err, format!("{}<{}>", snippet, name));
}
(0, _, Some("&")) => {
suggest_new(err, "&'a ");
}
(0, _, Some("'_")) => {
suggest_new(err, "'a");
}
(0, _, Some(snippet)) if !snippet.ends_with('>') => {
suggest_new(err, &format!("{}<'a>", snippet));
introduce_suggestion.push((span, sugg.to_string()));
err.multipart_suggestion(&msg, introduce_suggestion, Applicability::MaybeIncorrect);
if should_break {
break;
}
_ => {
err.span_label(span, "expected lifetime parameter");
}
};

match (lifetime_names.len(), lifetime_names.iter().next(), snippet.as_deref()) {
(1, Some(name), Some("&")) => {
suggest_existing(err, format!("&{} ", name));
}
(1, Some(name), Some("'_")) => {
suggest_existing(err, name.to_string());
}
(1, Some(name), Some("")) => {
suggest_existing(err, format!("{}, ", name).repeat(count));
}
(1, Some(name), Some(snippet)) if !snippet.ends_with('>') => {
suggest_existing(
err,
format!(
"{}<{}>",
snippet,
std::iter::repeat(name.to_string())
.take(count)
.collect::<Vec<_>>()
.join(", ")
),
);
}
(0, _, Some("&")) if count == 1 => {
suggest_new(err, "&'a ");
}
(0, _, Some("'_")) if count == 1 => {
suggest_new(err, "'a");
}
(0, _, Some(snippet)) if !snippet.ends_with('>') && count == 1 => {
suggest_new(err, &format!("{}<'a>", snippet));
}
(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");
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.
err.span_suggestion_verbose(
span,
"consider using one of the available lifetimes here",
"'lifetime, ".repeat(count),
Applicability::HasPlaceholders,
);
}
}
_ => {}
}
}
}
55 changes: 15 additions & 40 deletions src/librustc_resolve/late/lifetimes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2388,52 +2388,28 @@ impl<'a, 'tcx> LifetimeContext<'a, 'tcx> {
};

let mut err = self.report_missing_lifetime_specifiers(span, lifetime_refs.len());
let mut add_label = true;

if let Some(params) = error {
if lifetime_refs.len() == 1 {
add_label = add_label && self.report_elision_failure(&mut err, params, span);
// If there's no lifetime available, suggest `'static`.
if self.report_elision_failure(&mut err, params) && lifetime_names.is_empty() {
lifetime_names.insert(ast::Ident::from_str("'static"));
}
}
if add_label {
self.add_missing_lifetime_specifiers_label(
&mut err,
span,
lifetime_refs.len(),
&lifetime_names,
error.map(|p| &p[..]).unwrap_or(&[]),
);
}

self.add_missing_lifetime_specifiers_label(
&mut err,
span,
lifetime_refs.len(),
&lifetime_names,
error.map(|p| &p[..]).unwrap_or(&[]),
);
err.emit();
}

fn suggest_lifetime(&self, db: &mut DiagnosticBuilder<'_>, span: Span, msg: &str) -> bool {
match self.tcx.sess.source_map().span_to_snippet(span) {
Ok(ref snippet) => {
let (sugg, applicability) = if snippet == "&" {
("&'static ".to_owned(), Applicability::MachineApplicable)
} else if snippet == "'_" {
("'static".to_owned(), Applicability::MachineApplicable)
} else {
(format!("{} + 'static", snippet), Applicability::MaybeIncorrect)
};
db.span_suggestion(span, msg, sugg, applicability);
false
}
Err(_) => {
db.help(msg);
true
}
}
}

fn report_elision_failure(
&mut self,
db: &mut DiagnosticBuilder<'_>,
params: &[ElisionFailureInfo],
span: Span,
) -> bool {
) -> bool /* add `'static` lifetime to lifetime list */ {
let mut m = String::new();
let len = params.len();

Expand Down Expand Up @@ -2482,29 +2458,28 @@ impl<'a, 'tcx> LifetimeContext<'a, 'tcx> {
"this function's return type contains a borrowed value, \
but there is no value for it to be borrowed from",
);
self.suggest_lifetime(db, span, "consider giving it a 'static lifetime")
true
} else if elided_len == 0 {
db.help(
"this function's return type contains a borrowed value with \
an elided lifetime, but the lifetime cannot be derived from \
the arguments",
);
let msg = "consider giving it an explicit bounded or 'static lifetime";
self.suggest_lifetime(db, span, msg)
true
} else if elided_len == 1 {
db.help(&format!(
"this function's return type contains a borrowed value, \
but the signature does not say which {} it is borrowed from",
m
));
true
false
} else {
db.help(&format!(
"this function's return type contains a borrowed value, \
but the signature does not say whether it is borrowed from {}",
m
));
true
false
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems that the semantics of the return value of report_elision_failure have changed. What does the bool indicate here? Would you mind documenting it?

}
}

Expand Down
3 changes: 2 additions & 1 deletion src/librustc_span/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -657,7 +657,8 @@ impl MultiSpan {
MultiSpan { primary_spans: vec![primary_span], span_labels: vec![] }
}

pub fn from_spans(vec: Vec<Span>) -> MultiSpan {
pub fn from_spans(mut vec: Vec<Span>) -> MultiSpan {
vec.sort();
MultiSpan { primary_spans: vec, span_labels: vec![] }
}

Expand Down
13 changes: 2 additions & 11 deletions src/librustc_typeck/astconv.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1017,18 +1017,8 @@ impl<'o, 'tcx> dyn AstConv<'tcx> + 'o {

self.prohibit_generics(trait_ref.path.segments.split_last().unwrap().1);

let path_span = if let [segment] = &trait_ref.path.segments[..] {
// FIXME: `trait_ref.path.span` can point to a full path with multiple
// segments, even though `trait_ref.path.segments` is of length `1`. Work
// around that bug here, even though it should be fixed elsewhere.
// This would otherwise cause an invalid suggestion. For an example, look at
// `src/test/ui/issues/issue-28344.rs`.
segment.ident.span
} else {
trait_ref.path.span
};
Copy link
Contributor

Choose a reason for hiding this comment

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

Out of curiosity, was this fixed elsewhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Partly. I think this used to affect multiple tests but the only case that appears now is at https://github.com/rust-lang/rust/pull/71235/files#diff-1bd3f65ee2d72d0225a5db83a8391165.

let (substs, assoc_bindings, arg_count_correct) = self.create_substs_for_ast_trait_ref(
path_span,
trait_ref.path.span,
trait_def_id,
self_ty,
trait_ref.path.segments.last().unwrap(),
Expand Down Expand Up @@ -1729,6 +1719,7 @@ impl<'o, 'tcx> dyn AstConv<'tcx> + 'o {
self.ast_region_to_region(lifetime, None)
} else {
self.re_infer(None, span).unwrap_or_else(|| {
// FIXME: these can be redundant with E0106, but not always.
struct_span_err!(
tcx.sess,
span,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,13 @@ error[E0106]: missing lifetime specifier
--> $DIR/bound-lifetime-in-binding-only.rs:52:23
|
LL | fn elision<T: Fn() -> &i32>() {
| ^ help: consider giving it a 'static lifetime: `&'static`
| ^ 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
help: consider using the `'static` lifetime
|
LL | fn elision<T: Fn() -> &'static i32>() {
| ^^^^^^^^

error: aborting due to previous error

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,13 @@ error[E0106]: missing lifetime specifier
--> $DIR/bound-lifetime-in-return-only.rs:34:23
|
LL | fn elision(_: fn() -> &i32) {
| ^ help: consider giving it a 'static lifetime: `&'static`
| ^ 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
help: consider using the `'static` lifetime
|
LL | fn elision(_: fn() -> &'static i32) {
| ^^^^^^^^

error: aborting due to previous error

Expand Down
Loading