Skip to content

Commit b7a23bc

Browse files
estebankestebank
committed
On borrow return type, suggest borrowing from arg or owned return type
When we encounter a function with a return type that has an anonymous lifetime with no argument to borrow from, besides suggesting the `'static` lifetime we now also suggest changing the arguments to be borrows or changing the return type to be an owned type. ``` error[E0106]: missing lifetime specifier --> $DIR/variadic-ffi-6.rs:7:6 | LL | ) -> &usize { | ^ 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, but this is uncommon unless you're returning a borrowed value from a `const` or a `static` | LL | ) -> &'static usize { | +++++++ help: instead, you are more likely to want to change one of the arguments to be borrowed... | LL | x: &usize, | + help: ...or alternatively, to want to return an owned value | LL - ) -> &usize { LL + ) -> usize { | ``` Fix #85843.
1 parent 3a85a5c commit b7a23bc

19 files changed

+305
-57
lines changed

‎compiler/rustc_resolve/src/late/diagnostics.rs‎

Lines changed: 126 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@ use crate::{PathResult, PathSource, Segment};
88
use rustc_hir::def::Namespace::{self, *};
99

1010
use rustc_ast::ptr::P;
11-
use rustc_ast::visit::{FnCtxt, FnKind, LifetimeCtxt};
11+
use rustc_ast::visit::{walk_ty, FnCtxt, FnKind, LifetimeCtxt, Visitor};
1212
use rustc_ast::{
1313
self as ast, AssocItemKind, Expr, ExprKind, GenericParam, GenericParamKind, Item, ItemKind,
1414
MethodCall, NodeId, Path, Ty, TyKind, DUMMY_NODE_ID,
@@ -2811,6 +2811,7 @@ impl<'a: 'ast, 'ast, 'tcx> LateResolutionVisitor<'a, '_, 'ast, 'tcx> {
28112811
.collect();
28122812
debug!(?in_scope_lifetimes);
28132813

2814+
let mut maybe_static = false;
28142815
debug!(?function_param_lifetimes);
28152816
if let Some((param_lifetimes, params)) = &function_param_lifetimes {
28162817
let elided_len = param_lifetimes.len();
@@ -2849,9 +2850,10 @@ impl<'a: 'ast, 'ast, 'tcx> LateResolutionVisitor<'a, '_, 'ast, 'tcx> {
28492850

28502851
if num_params == 0 {
28512852
err.help(
2852-
"this function's return type contains a borrowed value, \
2853-
but there is no value for it to be borrowed from",
2853+
"this function's return type contains a borrowed value, but there is no value \
2854+
for it to be borrowed from",
28542855
);
2856+
maybe_static = true;
28552857
if in_scope_lifetimes.is_empty() {
28562858
in_scope_lifetimes = vec![(
28572859
Ident::with_dummy_span(kw::StaticLifetime),
@@ -2860,10 +2862,10 @@ impl<'a: 'ast, 'ast, 'tcx> LateResolutionVisitor<'a, '_, 'ast, 'tcx> {
28602862
}
28612863
} else if elided_len == 0 {
28622864
err.help(
2863-
"this function's return type contains a borrowed value with \
2864-
an elided lifetime, but the lifetime cannot be derived from \
2865-
the arguments",
2865+
"this function's return type contains a borrowed value with an elided \
2866+
lifetime, but the lifetime cannot be derived from the arguments",
28662867
);
2868+
maybe_static = true;
28672869
if in_scope_lifetimes.is_empty() {
28682870
in_scope_lifetimes = vec![(
28692871
Ident::with_dummy_span(kw::StaticLifetime),
@@ -2872,13 +2874,13 @@ impl<'a: 'ast, 'ast, 'tcx> LateResolutionVisitor<'a, '_, 'ast, 'tcx> {
28722874
}
28732875
} else if num_params == 1 {
28742876
err.help(format!(
2875-
"this function's return type contains a borrowed value, \
2876-
but the signature does not say which {m} it is borrowed from"
2877+
"this function's return type contains a borrowed value, but the signature does \
2878+
not say which {m} it is borrowed from",
28772879
));
28782880
} else {
28792881
err.help(format!(
2880-
"this function's return type contains a borrowed value, \
2881-
but the signature does not say whether it is borrowed from {m}"
2882+
"this function's return type contains a borrowed value, but the signature does \
2883+
not say whether it is borrowed from {m}",
28822884
));
28832885
}
28842886
}
@@ -2943,11 +2945,107 @@ impl<'a: 'ast, 'ast, 'tcx> LateResolutionVisitor<'a, '_, 'ast, 'tcx> {
29432945
);
29442946
}
29452947
1 => {
2948+
let post = if maybe_static {
2949+
let owned = if let [lt] = &lifetime_refs[..]
2950+
&& lt.kind != MissingLifetimeKind::Ampersand
2951+
{
2952+
", or if you will only have owned values"
2953+
} else {
2954+
""
2955+
};
2956+
format!(
2957+
", but this is uncommon unless you're returning a borrowed value from a \
2958+
`const` or a `static`{owned}",
2959+
)
2960+
} else {
2961+
String::new()
2962+
};
29462963
err.multipart_suggestion_verbose(
2947-
format!("consider using the `{existing_name}` lifetime"),
2964+
format!("consider using the `{existing_name}` lifetime{post}"),
29482965
spans_suggs,
29492966
Applicability::MaybeIncorrect,
29502967
);
2968+
if maybe_static {
2969+
// FIXME: what follows are general suggestions, but we'd want to perform some
2970+
// minimal flow analysis to provide more accurate suggestions. For example, if
2971+
// we identified that the return expression references only one argument, we
2972+
// would suggest borrowing only that argument, and we'd skip the prior
2973+
// "use `'static`" suggestion entirely.
2974+
if let [lt] = &lifetime_refs[..] && lt.kind == MissingLifetimeKind::Ampersand {
2975+
let pre = if let Some((kind, _span)) =
2976+
self.diagnostic_metadata.current_function
2977+
&& let FnKind::Fn(_, _, sig, _, _, _) = kind
2978+
&& !sig.decl.inputs.is_empty()
2979+
&& let sugg = sig
2980+
.decl
2981+
.inputs
2982+
.iter()
2983+
.filter_map(|param| {
2984+
if param.ty.span.contains(lt.span) {
2985+
// We don't want to suggest `fn elision(_: &fn() -> &i32)`
2986+
// when we have `fn elision(_: fn() -> &i32)`
2987+
None
2988+
} else if let TyKind::CVarArgs = param.ty.kind {
2989+
// Don't suggest `&...` for ffi fn with varargs
2990+
None
2991+
} else {
2992+
Some((param.ty.span.shrink_to_lo(), "&".to_string()))
2993+
}
2994+
})
2995+
.collect::<Vec<_>>()
2996+
&& !sugg.is_empty()
2997+
{
2998+
2999+
let (the, s) = if sig.decl.inputs.len() == 1 {
3000+
("the", "")
3001+
} else {
3002+
("one of the", "s")
3003+
};
3004+
err.multipart_suggestion_verbose(
3005+
format!(
3006+
"instead, you are more likely to want to change {the} \
3007+
argument{s} to be borrowed...",
3008+
),
3009+
sugg,
3010+
Applicability::MaybeIncorrect,
3011+
);
3012+
"...or alternatively,"
3013+
} else {
3014+
"instead, you are more likely"
3015+
};
3016+
let mut sugg = vec![(lt.span, String::new())];
3017+
if let Some((kind, _span)) =
3018+
self.diagnostic_metadata.current_function
3019+
&& let FnKind::Fn(_, _, sig, _, _, _) = kind
3020+
&& let ast::FnRetTy::Ty(ty) = &sig.decl.output
3021+
{
3022+
let mut lt_finder = LifetimeFinder { lifetime: lt.span, found: None };
3023+
lt_finder.visit_ty(&ty);
3024+
3025+
if let Some(ty) = lt_finder.found {
3026+
if let TyKind::Path(None, Path { segments, .. }) = &ty.kind
3027+
&& segments.len() == 1
3028+
&& segments[0].ident.name == sym::str
3029+
{
3030+
// Don't suggest `-> str`, suggest `-> String`.
3031+
sugg = vec![(lt.span.with_hi(ty.span.hi()), "String".to_string())];
3032+
}
3033+
if let TyKind::Slice(inner_ty) = &ty.kind {
3034+
// Don't suggest `-> [T]`, suggest `-> Vec<T>`.
3035+
sugg = vec![
3036+
(lt.span.with_hi(inner_ty.span.lo()), "Vec<".to_string()),
3037+
(ty.span.with_lo(inner_ty.span.hi()), ">".to_string()),
3038+
];
3039+
}
3040+
}
3041+
};
3042+
err.multipart_suggestion_verbose(
3043+
format!("{pre} to want to return an owned value"),
3044+
sugg,
3045+
Applicability::MaybeIncorrect,
3046+
);
3047+
}
3048+
}
29513049

29523050
// Record as using the suggested resolution.
29533051
let (_, (_, res)) = in_scope_lifetimes[0];
@@ -2977,7 +3075,7 @@ impl<'a: 'ast, 'ast, 'tcx> LateResolutionVisitor<'a, '_, 'ast, 'tcx> {
29773075
fn mk_where_bound_predicate(
29783076
path: &Path,
29793077
poly_trait_ref: &ast::PolyTraitRef,
2980-
ty: &ast::Ty,
3078+
ty: &Ty,
29813079
) -> Option<ast::WhereBoundPredicate> {
29823080
use rustc_span::DUMMY_SP;
29833081
let modified_segments = {
@@ -3054,6 +3152,22 @@ pub(super) fn signal_lifetime_shadowing(sess: &Session, orig: Ident, shadower: I
30543152
err.emit();
30553153
}
30563154

3155+
struct LifetimeFinder<'ast> {
3156+
lifetime: Span,
3157+
found: Option<&'ast Ty>,
3158+
}
3159+
3160+
impl<'ast> Visitor<'ast> for LifetimeFinder<'ast> {
3161+
fn visit_ty(&mut self, t: &'ast Ty) {
3162+
if t.span.lo() == self.lifetime.lo()
3163+
&& let TyKind::Ref(_, mut_ty) = &t.kind
3164+
{
3165+
self.found = Some(&mut_ty.ty);
3166+
}
3167+
walk_ty(self, t)
3168+
}
3169+
}
3170+
30573171
/// Shadowing involving a label is only a warning for historical reasons.
30583172
//FIXME: make this a proper lint.
30593173
pub(super) fn signal_label_shadowing(sess: &Session, orig: Span, shadower: Ident) {

‎src/tools/tidy/src/ui_tests.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,7 @@ use std::path::{Path, PathBuf};
1111
const ENTRY_LIMIT: usize = 900;
1212
// FIXME: The following limits should be reduced eventually.
1313
const ISSUES_ENTRY_LIMIT: usize = 1852;
14-
const ROOT_ENTRY_LIMIT: usize = 867;
14+
const ROOT_ENTRY_LIMIT: usize = 866;
1515

1616
const EXPECTED_TEST_FILE_EXTENSIONS: &[&str] = &[
1717
"rs", // test source files

‎tests/ui/associated-types/bound-lifetime-in-binding-only.elision.stderr‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,10 +5,15 @@ LL | fn elision<T: Fn() -> &i32>() {
55
| ^ expected named lifetime parameter
66
|
77
= help: this function's return type contains a borrowed value, but there is no value for it to be borrowed from
8-
help: consider using the `'static` lifetime
8+
help: consider using the `'static` lifetime, but this is uncommon unless you're returning a borrowed value from a `const` or a `static`
99
|
1010
LL | fn elision<T: Fn() -> &'static i32>() {
1111
| +++++++
12+
help: instead, you are more likely to want to return an owned value
13+
|
14+
LL - fn elision<T: Fn() -> &i32>() {
15+
LL + fn elision<T: Fn() -> i32>() {
16+
|
1217

1318
error: aborting due to previous error
1419

‎tests/ui/associated-types/bound-lifetime-in-return-only.elision.stderr‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,10 +5,15 @@ LL | fn elision(_: fn() -> &i32) {
55
| ^ expected named lifetime parameter
66
|
77
= help: this function's return type contains a borrowed value, but there is no value for it to be borrowed from
8-
help: consider using the `'static` lifetime
8+
help: consider using the `'static` lifetime, but this is uncommon unless you're returning a borrowed value from a `const` or a `static`
99
|
1010
LL | fn elision(_: fn() -> &'static i32) {
1111
| +++++++
12+
help: instead, you are more likely to want to return an owned value
13+
|
14+
LL - fn elision(_: fn() -> &i32) {
15+
LL + fn elision(_: fn() -> i32) {
16+
|
1217

1318
error: aborting due to previous error
1419

‎tests/ui/c-variadic/variadic-ffi-6.stderr‎

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,10 +5,19 @@ LL | ) -> &usize {
55
| ^ expected named lifetime parameter
66
|
77
= help: this function's return type contains a borrowed value, but there is no value for it to be borrowed from
8-
help: consider using the `'static` lifetime
8+
help: consider using the `'static` lifetime, but this is uncommon unless you're returning a borrowed value from a `const` or a `static`
99
|
1010
LL | ) -> &'static usize {
1111
| +++++++
12+
help: instead, you are more likely to want to change one of the arguments to be borrowed...
13+
|
14+
LL | x: &usize,
15+
| +
16+
help: ...or alternatively, to want to return an owned value
17+
|
18+
LL - ) -> &usize {
19+
LL + ) -> usize {
20+
|
1221

1322
error: aborting due to previous error
1423

‎tests/ui/foreign-fn-return-lifetime.fixed‎

Lines changed: 0 additions & 8 deletions
This file was deleted.

‎tests/ui/foreign-fn-return-lifetime.rs‎

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,3 @@
1-
// run-rustfix
2-
31
extern "C" {
42
pub fn g(_: &u8) -> &u8; // OK
53
pub fn f() -> &u8; //~ ERROR missing lifetime specifier

‎tests/ui/foreign-fn-return-lifetime.stderr‎

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,14 +1,19 @@
11
error[E0106]: missing lifetime specifier
2-
--> $DIR/foreign-fn-return-lifetime.rs:5:19
2+
--> $DIR/foreign-fn-return-lifetime.rs:3:19
33
|
44
LL | pub fn f() -> &u8;
55
| ^ expected named lifetime parameter
66
|
77
= help: this function's return type contains a borrowed value, but there is no value for it to be borrowed from
8-
help: consider using the `'static` lifetime
8+
help: consider using the `'static` lifetime, but this is uncommon unless you're returning a borrowed value from a `const` or a `static`
99
|
1010
LL | pub fn f() -> &'static u8;
1111
| +++++++
12+
help: instead, you are more likely to want to return an owned value
13+
|
14+
LL - pub fn f() -> &u8;
15+
LL + pub fn f() -> u8;
16+
|
1217

1318
error: aborting due to previous error
1419

‎tests/ui/generic-associated-types/issue-70304.stderr‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,7 @@ LL | fn create_doc() -> impl Document<Cursor<'_> = DocCursorImpl<'_>> {
1111
| ^^ expected named lifetime parameter
1212
|
1313
= help: this function's return type contains a borrowed value, but there is no value for it to be borrowed from
14-
help: consider using the `'static` lifetime
14+
help: consider using the `'static` lifetime, but this is uncommon unless you're returning a borrowed value from a `const` or a `static`, or if you will only have owned values
1515
|
1616
LL | fn create_doc() -> impl Document<Cursor<'_> = DocCursorImpl<'static>> {
1717
| ~~~~~~~

‎tests/ui/impl-trait/impl-fn-hrtb-bounds.stderr‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@ LL | fn d() -> impl Fn() -> (impl Debug + '_) {
55
| ^^ expected named lifetime parameter
66
|
77
= help: this function's return type contains a borrowed value, but there is no value for it to be borrowed from
8-
help: consider using the `'static` lifetime
8+
help: consider using the `'static` lifetime, but this is uncommon unless you're returning a borrowed value from a `const` or a `static`, or if you will only have owned values
99
|
1010
LL | fn d() -> impl Fn() -> (impl Debug + 'static) {
1111
| ~~~~~~~

0 commit comments

Comments
 (0)