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 rust-lang#85843.
estebank · Nov 14, 2023 · 1afef41 · 1afef41
1 parent d97bb19
commit 1afef41
Show file tree

Hide file tree

Showing 19 changed files with 305 additions and 57 deletions.
diff --git a/compiler/rustc_resolve/src/late/diagnostics.rs b/compiler/rustc_resolve/src/late/diagnostics.rs
@@ -6,7 +6,7 @@ use crate::{Module, ModuleKind, ModuleOrUniformRoot};
 use crate::{PathResult, PathSource, Segment};
 use rustc_hir::def::Namespace::{self, *};
 
-use rustc_ast::visit::{FnCtxt, FnKind, LifetimeCtxt};
+use rustc_ast::visit::{walk_ty, FnCtxt, FnKind, LifetimeCtxt, Visitor};
 use rustc_ast::{
     self as ast, AssocItemKind, Expr, ExprKind, GenericParam, GenericParamKind, Item, ItemKind,
     MethodCall, NodeId, Path, Ty, TyKind, DUMMY_NODE_ID,
@@ -2612,6 +2612,7 @@ impl<'a: 'ast, 'ast, 'tcx> LateResolutionVisitor<'a, '_, 'ast, 'tcx> {
             .collect();
         debug!(?in_scope_lifetimes);
 
+        let mut maybe_static = false;
         debug!(?function_param_lifetimes);
         if let Some((param_lifetimes, params)) = &function_param_lifetimes {
             let elided_len = param_lifetimes.len();
@@ -2650,9 +2651,10 @@ impl<'a: 'ast, 'ast, 'tcx> LateResolutionVisitor<'a, '_, 'ast, 'tcx> {
 
             if num_params == 0 {
                 err.help(
-                    "this function's return type contains a borrowed value, \
-                 but there is no value for it to be borrowed from",
+                    "this function's return type contains a borrowed value, but there is no value \
+                     for it to be borrowed from",
                 );
+                maybe_static = true;
                 if in_scope_lifetimes.is_empty() {
                     in_scope_lifetimes = vec![(
                         Ident::with_dummy_span(kw::StaticLifetime),
@@ -2661,10 +2663,10 @@ impl<'a: 'ast, 'ast, 'tcx> LateResolutionVisitor<'a, '_, 'ast, 'tcx> {
                 }
             } else if elided_len == 0 {
                 err.help(
-                    "this function's return type contains a borrowed value with \
-                 an elided lifetime, but the lifetime cannot be derived from \
-                 the arguments",
+                    "this function's return type contains a borrowed value with an elided \
+                     lifetime, but the lifetime cannot be derived from the arguments",
                 );
+                maybe_static = true;
                 if in_scope_lifetimes.is_empty() {
                     in_scope_lifetimes = vec![(
                         Ident::with_dummy_span(kw::StaticLifetime),
@@ -2673,13 +2675,13 @@ impl<'a: 'ast, 'ast, 'tcx> LateResolutionVisitor<'a, '_, 'ast, 'tcx> {
                 }
             } else if num_params == 1 {
                 err.help(format!(
-                    "this function's return type contains a borrowed value, \
-                 but the signature does not say which {m} it is borrowed from"
+                    "this function's return type contains a borrowed value, but the signature does \
+                     not say which {m} it is borrowed from",
                 ));
             } else {
                 err.help(format!(
-                    "this function's return type contains a borrowed value, \
-                 but the signature does not say whether it is borrowed from {m}"
+                    "this function's return type contains a borrowed value, but the signature does \
+                     not say whether it is borrowed from {m}",
                 ));
             }
         }
@@ -2744,11 +2746,107 @@ impl<'a: 'ast, 'ast, 'tcx> LateResolutionVisitor<'a, '_, 'ast, 'tcx> {
                 );
             }
             1 => {
+                let post = if maybe_static {
+                    let owned = if let [lt] = &lifetime_refs[..]
+                        && lt.kind != MissingLifetimeKind::Ampersand
+                    {
+                        ", or if you will only have owned values"
+                    } else {
+                        ""
+                    };
+                    format!(
+                        ", but this is uncommon unless you're returning a borrowed value from a \
+                         `const` or a `static`{owned}",
+                    )
+                } else {
+                    String::new()
+                };
                 err.multipart_suggestion_verbose(
-                    format!("consider using the `{existing_name}` lifetime"),
+                    format!("consider using the `{existing_name}` lifetime{post}"),
                     spans_suggs,
                     Applicability::MaybeIncorrect,
                 );
+                if maybe_static {
+                    // FIXME: what follows are general suggestions, but we'd want to perform some
+                    // minimal flow analysis to provide more accurate suggestions. For example, if
+                    // we identified that the return expression references only one argument, we
+                    // would suggest borrowing only that argument, and we'd skip the prior
+                    // "use `'static`" suggestion entirely.
+                    if let [lt] = &lifetime_refs[..] && lt.kind == MissingLifetimeKind::Ampersand {
+                        let pre = if let Some((kind, _span)) =
+                            self.diagnostic_metadata.current_function
+                            && let FnKind::Fn(_, _, sig, _, _, _) = kind
+                            && !sig.decl.inputs.is_empty()
+                            && let sugg = sig
+                                .decl
+                                .inputs
+                                .iter()
+                                .filter_map(|param| {
+                                    if param.ty.span.contains(lt.span) {
+                                        // We don't want to suggest `fn elision(_: &fn() -> &i32)`
+                                        // when we have `fn elision(_: fn() -> &i32)`
+                                        None
+                                    } else if let TyKind::CVarArgs = param.ty.kind {
+                                        // Don't suggest `&...` for ffi fn with varargs
+                                        None
+                                    } else {
+                                        Some((param.ty.span.shrink_to_lo(), "&".to_string()))
+                                    }
+                                })
+                                .collect::<Vec<_>>()
+                            && !sugg.is_empty()
+                        {
+
+                            let (the, s) = if sig.decl.inputs.len() == 1 {
+                                ("the", "")
+                            } else {
+                                ("one of the", "s")
+                            };
+                            err.multipart_suggestion_verbose(
+                                format!(
+                                    "instead, you are more likely to want to change {the} \
+                                     argument{s} to be borrowed...",
+                                ),
+                                sugg,
+                                Applicability::MaybeIncorrect,
+                            );
+                            "...or alternatively,"
+                        } else {
+                            "instead, you are more likely"
+                        };
+                        let mut sugg = vec![(lt.span, String::new())];
+                        if let Some((kind, _span)) =
+                            self.diagnostic_metadata.current_function
+                            && let FnKind::Fn(_, _, sig, _, _, _) = kind
+                            && let ast::FnRetTy::Ty(ty) = &sig.decl.output
+                        {
+                            let mut lt_finder = LifetimeFinder { lifetime: lt.span, found: None };
+                            lt_finder.visit_ty(&ty);
+
+                            if let Some(ty) = lt_finder.found {
+                                if let TyKind::Path(None, Path { segments, .. }) = &ty.kind
+                                    && segments.len() == 1
+                                    && segments[0].ident.name == sym::str
+                                {
+                                    // Don't suggest `-> str`, suggest `-> String`.
+                                    sugg = vec![(lt.span.with_hi(ty.span.hi()), "String".to_string())];
+                                }
+                                if let TyKind::Slice(inner_ty) = &ty.kind {
+                                    // Don't suggest `-> [T]`, suggest `-> Vec<T>`.
+                                    sugg = vec![
+                                        (lt.span.with_hi(inner_ty.span.lo()), "Vec<".to_string()),
+                                        (ty.span.with_lo(inner_ty.span.hi()), ">".to_string()),
+                                    ];
+                                }
+                            }
+                        };
+                        err.multipart_suggestion_verbose(
+                            format!("{pre} to want to return an owned value"),
+                            sugg,
+                            Applicability::MaybeIncorrect,
+                        );
+                    }
+                }
 
                 // Record as using the suggested resolution.
                 let (_, (_, res)) = in_scope_lifetimes[0];
@@ -2778,7 +2876,7 @@ impl<'a: 'ast, 'ast, 'tcx> LateResolutionVisitor<'a, '_, 'ast, 'tcx> {
 fn mk_where_bound_predicate(
     path: &Path,
     poly_trait_ref: &ast::PolyTraitRef,
-    ty: &ast::Ty,
+    ty: &Ty,
 ) -> Option<ast::WhereBoundPredicate> {
     use rustc_span::DUMMY_SP;
     let modified_segments = {
@@ -2855,6 +2953,22 @@ pub(super) fn signal_lifetime_shadowing(sess: &Session, orig: Ident, shadower: I
     err.emit();
 }
 
+struct LifetimeFinder<'ast> {
+    lifetime: Span,
+    found: Option<&'ast Ty>,
+}
+
+impl<'ast> Visitor<'ast> for LifetimeFinder<'ast> {
+    fn visit_ty(&mut self, t: &'ast Ty) {
+        if t.span.lo() == self.lifetime.lo()
+            && let TyKind::Ref(_, mut_ty) = &t.kind
+        {
+            self.found = Some(&mut_ty.ty);
+        }
+        walk_ty(self, t)
+    }
+}
+
 /// Shadowing involving a label is only a warning for historical reasons.
 //FIXME: make this a proper lint.
 pub(super) fn signal_label_shadowing(sess: &Session, orig: Span, shadower: Ident) {

diff --git a/src/tools/tidy/src/ui_tests.rs b/src/tools/tidy/src/ui_tests.rs
@@ -11,7 +11,7 @@ use std::path::{Path, PathBuf};
 const ENTRY_LIMIT: usize = 900;
 // FIXME: The following limits should be reduced eventually.
 const ISSUES_ENTRY_LIMIT: usize = 1854;
-const ROOT_ENTRY_LIMIT: usize = 867;
+const ROOT_ENTRY_LIMIT: usize = 866;
 
 const EXPECTED_TEST_FILE_EXTENSIONS: &[&str] = &[
     "rs",     // test source files

diff --git a/tests/ui/associated-types/bound-lifetime-in-binding-only.elision.stderr b/tests/ui/associated-types/bound-lifetime-in-binding-only.elision.stderr
@@ -5,10 +5,15 @@ 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
-help: consider using the `'static` lifetime
+help: consider using the `'static` lifetime, but this is uncommon unless you're returning a borrowed value from a `const` or a `static`
    |
 LL | fn elision<T: Fn() -> &'static i32>() {
    |                        +++++++
+help: instead, you are more likely to want to return an owned value
+   |
+LL - fn elision<T: Fn() -> &i32>() {
+LL + fn elision<T: Fn() -> i32>() {
+   |
 
 error: aborting due to previous error
 

diff --git a/tests/ui/associated-types/bound-lifetime-in-return-only.elision.stderr b/tests/ui/associated-types/bound-lifetime-in-return-only.elision.stderr
@@ -5,10 +5,15 @@ LL | fn elision(_: 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
-help: consider using the `'static` lifetime
+help: consider using the `'static` lifetime, but this is uncommon unless you're returning a borrowed value from a `const` or a `static`
    |
 LL | fn elision(_: fn() -> &'static i32) {
    |                        +++++++
+help: instead, you are more likely to want to return an owned value
+   |
+LL - fn elision(_: fn() -> &i32) {
+LL + fn elision(_: fn() -> i32) {
+   |
 
 error: aborting due to previous error
 

diff --git a/tests/ui/c-variadic/variadic-ffi-6.stderr b/tests/ui/c-variadic/variadic-ffi-6.stderr
@@ -5,10 +5,19 @@ 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
+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 {
+   |
 
 error: aborting due to previous error
 

diff --git a/tests/ui/foreign-fn-return-lifetime.fixed b/tests/ui/foreign-fn-return-lifetime.fixed
diff --git a/tests/ui/foreign-fn-return-lifetime.rs b/tests/ui/foreign-fn-return-lifetime.rs
@@ -1,5 +1,3 @@
-// run-rustfix
-
 extern "C" {
     pub fn g(_: &u8) -> &u8; // OK
     pub fn f() -> &u8; //~ ERROR missing lifetime specifier

diff --git a/tests/ui/foreign-fn-return-lifetime.stderr b/tests/ui/foreign-fn-return-lifetime.stderr
@@ -1,14 +1,19 @@
 error[E0106]: missing lifetime specifier
-  --> $DIR/foreign-fn-return-lifetime.rs:5:19
+  --> $DIR/foreign-fn-return-lifetime.rs:3:19
    |
 LL |     pub fn f() -> &u8;
    |                   ^ 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
+help: consider using the `'static` lifetime, but this is uncommon unless you're returning a borrowed value from a `const` or a `static`
    |
 LL |     pub fn f() -> &'static u8;
    |                    +++++++
+help: instead, you are more likely to want to return an owned value
+   |
+LL -     pub fn f() -> &u8;
+LL +     pub fn f() -> u8;
+   |
 
 error: aborting due to previous error
 

diff --git a/tests/ui/generic-associated-types/issue-70304.stderr b/tests/ui/generic-associated-types/issue-70304.stderr
@@ -11,7 +11,7 @@ LL | fn create_doc() -> impl Document<Cursor<'_> = DocCursorImpl<'_>> {
    |                                                             ^^ 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
+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
    |
 LL | fn create_doc() -> impl Document<Cursor<'_> = DocCursorImpl<'static>> {
    |                                                             ~~~~~~~

diff --git a/tests/ui/impl-trait/impl-fn-hrtb-bounds.stderr b/tests/ui/impl-trait/impl-fn-hrtb-bounds.stderr
@@ -5,7 +5,7 @@ LL | fn d() -> impl Fn() -> (impl Debug + '_) {
    |                                      ^^ 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
+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
    |
 LL | fn d() -> impl Fn() -> (impl Debug + 'static) {
    |                                      ~~~~~~~

diff --git a/tests/ui/issues/issue-13497.stderr b/tests/ui/issues/issue-13497.stderr
@@ -5,10 +5,14 @@ LL |     &str
    |     ^ 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
+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 str
    |      +++++++
+help: instead, you are more likely to want to return an owned value
+   |
+LL |     String
+   |     ~~~~~~
 
 error: aborting due to previous error
 

diff --git a/tests/ui/lifetimes/issue-26638.stderr b/tests/ui/lifetimes/issue-26638.stderr
@@ -17,10 +17,18 @@ LL | fn parse_type_2(iter: fn(&u8)->&u8) -> &str { iter() }
    |                                        ^ 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
+help: consider using the `'static` lifetime, but this is uncommon unless you're returning a borrowed value from a `const` or a `static`
    |
 LL | fn parse_type_2(iter: fn(&u8)->&u8) -> &'static str { iter() }
    |                                         +++++++
+help: instead, you are more likely to want to change the argument to be borrowed...
+   |
+LL | fn parse_type_2(iter: &fn(&u8)->&u8) -> &str { iter() }
+   |                       +
+help: ...or alternatively, to want to return an owned value
+   |
+LL | fn parse_type_2(iter: fn(&u8)->&u8) -> String { iter() }
+   |                                        ~~~~~~
 
 error[E0106]: missing lifetime specifier
   --> $DIR/issue-26638.rs:10:22
@@ -29,10 +37,14 @@ LL | fn parse_type_3() -> &str { unimplemented!() }
    |                      ^ 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
+help: consider using the `'static` lifetime, but this is uncommon unless you're returning a borrowed value from a `const` or a `static`
    |
 LL | fn parse_type_3() -> &'static str { unimplemented!() }
    |                       +++++++
+help: instead, you are more likely to want to return an owned value
+   |
+LL | fn parse_type_3() -> String { unimplemented!() }
+   |                      ~~~~~~
 
 error[E0308]: mismatched types
   --> $DIR/issue-26638.rs:1:69