Rollup merge of #70998 - estebank:suggest-impl-trait-empty-fn, r=varkor

Suggest `-> impl Trait` and `-> Box<dyn Trait>` on fn that doesn't return

During development, a function could have a return type set that is a
bare trait object by accident. We already suggest using either a boxed
trait object or `impl Trait` if the return paths will allow it. We now
do so too when there are *no* return paths or they all resolve to `!`.
We still don't handle cases where the trait object is *not* the entirety
of the return type gracefully.

Closes #38376.

1 files changed, 92 insertions, 32 deletions

diff --git a/src/librustc_trait_selection/traits/error_reporting/suggestions.rs b/src/librustc_trait_selection/traits/error_reporting/suggestions.rs
index 331c8f3be84..e698335b2c2 100644
--- a/src/librustc_trait_selection/traits/error_reporting/suggestions.rs
+++ b/src/librustc_trait_selection/traits/error_reporting/suggestions.rs
@@ -13,7 +13,7 @@ use rustc_hir::intravisit::Visitor;
 use rustc_hir::{AsyncGeneratorKind, GeneratorKind, Node};
 use rustc_middle::ty::TypeckTables;
 use rustc_middle::ty::{
-    self, AdtKind, DefIdTree, ToPredicate, Ty, TyCtxt, TypeFoldable, WithConstness,
+    self, AdtKind, DefIdTree, Infer, InferTy, ToPredicate, Ty, TyCtxt, TypeFoldable, WithConstness,
 };
 use rustc_span::symbol::{kw, sym, Symbol};
 use rustc_span::{MultiSpan, Span, DUMMY_SP};
@@ -826,12 +826,28 @@ impl<'a, 'tcx> InferCtxtExt<'tcx> for InferCtxt<'a, 'tcx> {
             .iter()
             .filter_map(|expr| tables.node_type_opt(expr.hir_id))
             .map(|ty| self.resolve_vars_if_possible(&ty));
-        let (last_ty, all_returns_have_same_type) = ret_types.clone().fold(
-            (None, true),
-            |(last_ty, mut same): (std::option::Option<Ty<'_>>, bool), ty| {
+        let (last_ty, all_returns_have_same_type, only_never_return) = ret_types.clone().fold(
+            (None, true, true),
+            |(last_ty, mut same, only_never_return): (std::option::Option<Ty<'_>>, bool, bool),
+             ty| {
                 let ty = self.resolve_vars_if_possible(&ty);
-                same &= last_ty.map_or(true, |last_ty| last_ty == ty) && ty.kind != ty::Error;
-                (Some(ty), same)
+                same &=
+                    ty.kind != ty::Error
+                        && last_ty.map_or(true, |last_ty| {
+                            // FIXME: ideally we would use `can_coerce` here instead, but `typeck` comes
+                            // *after* in the dependency graph.
+                            match (&ty.kind, &last_ty.kind) {
+                                (Infer(InferTy::IntVar(_)), Infer(InferTy::IntVar(_)))
+                                | (Infer(InferTy::FloatVar(_)), Infer(InferTy::FloatVar(_)))
+                                | (Infer(InferTy::FreshIntTy(_)), Infer(InferTy::FreshIntTy(_)))
+                                | (
+                                    Infer(InferTy::FreshFloatTy(_)),
+                                    Infer(InferTy::FreshFloatTy(_)),
+                                ) => true,
+                                _ => ty == last_ty,
+                            }
+                        });
+                (Some(ty), same, only_never_return && matches!(ty.kind, ty::Never))
             },
         );
         let all_returns_conform_to_trait =
@@ -840,13 +856,14 @@ impl<'a, 'tcx> InferCtxtExt<'tcx> for InferCtxt<'a, 'tcx> {
                     ty::Dynamic(predicates, _) => {
                         let cause = ObligationCause::misc(ret_ty.span, ret_ty.hir_id);
                         let param_env = ty::ParamEnv::empty();
-                        ret_types.all(|returned_ty| {
-                            predicates.iter().all(|predicate| {
-                                let pred = predicate.with_self_ty(self.tcx, returned_ty);
-                                let obl = Obligation::new(cause.clone(), param_env, pred);
-                                self.predicate_may_hold(&obl)
+                        only_never_return
+                            || ret_types.all(|returned_ty| {
+                                predicates.iter().all(|predicate| {
+                                    let pred = predicate.with_self_ty(self.tcx, returned_ty);
+                                    let obl = Obligation::new(cause.clone(), param_env, pred);
+                                    self.predicate_may_hold(&obl)
+                                })
                             })
-                        })
                     }
                     _ => false,
                 }
@@ -855,21 +872,19 @@ impl<'a, 'tcx> InferCtxtExt<'tcx> for InferCtxt<'a, 'tcx> {
             };
 
         let sm = self.tcx.sess.source_map();
-        let (snippet, last_ty) =
-            if let (true, hir::TyKind::TraitObject(..), Ok(snippet), true, Some(last_ty)) = (
-                // Verify that we're dealing with a return `dyn Trait`
-                ret_ty.span.overlaps(span),
-                &ret_ty.kind,
-                sm.span_to_snippet(ret_ty.span),
-                // If any of the return types does not conform to the trait, then we can't
-                // suggest `impl Trait` nor trait objects, it is a type mismatch error.
-                all_returns_conform_to_trait,
-                last_ty,
-            ) {
-                (snippet, last_ty)
-            } else {
-                return false;
-            };
+        let snippet = if let (true, hir::TyKind::TraitObject(..), Ok(snippet), true) = (
+            // Verify that we're dealing with a return `dyn Trait`
+            ret_ty.span.overlaps(span),
+            &ret_ty.kind,
+            sm.span_to_snippet(ret_ty.span),
+            // If any of the return types does not conform to the trait, then we can't
+            // suggest `impl Trait` nor trait objects: it is a type mismatch error.
+            all_returns_conform_to_trait,
+        ) {
+            snippet
+        } else {
+            return false;
+        };
         err.code(error_code!(E0746));
         err.set_primary_message("return type cannot have an unboxed trait object");
         err.children.clear();
@@ -881,13 +896,22 @@ impl<'a, 'tcx> InferCtxtExt<'tcx> for InferCtxt<'a, 'tcx> {
             #using-trait-objects-that-allow-for-values-of-different-types>";
         let has_dyn = snippet.split_whitespace().next().map_or(false, |s| s == "dyn");
         let trait_obj = if has_dyn { &snippet[4..] } else { &snippet[..] };
-        if all_returns_have_same_type {
+        if only_never_return {
+            // No return paths, probably using `panic!()` or similar.
+            // Suggest `-> T`, `-> impl Trait`, and if `Trait` is object safe, `-> Box<dyn Trait>`.
+            suggest_trait_object_return_type_alternatives(
+                err,
+                ret_ty.span,
+                trait_obj,
+                is_object_safe,
+            );
+        } else if let (Some(last_ty), true) = (last_ty, all_returns_have_same_type) {
             // Suggest `-> impl Trait`.
             err.span_suggestion(
                 ret_ty.span,
                 &format!(
-                    "return `impl {1}` instead, as all return paths are of type `{}`, \
-                        which implements `{1}`",
+                    "use `impl {1}` as the return type, as all return paths are of type `{}`, \
+                     which implements `{1}`",
                     last_ty, trait_obj,
                 ),
                 format!("impl {}", trait_obj),
@@ -925,8 +949,8 @@ impl<'a, 'tcx> InferCtxtExt<'tcx> for InferCtxt<'a, 'tcx> {
             }
             err.note(trait_obj_msg);
             err.note(&format!(
-                "if all the returned values were of the same type you could use \
-                    `impl {}` as the return type",
+                "if all the returned values were of the same type you could use `impl {}` as the \
+                 return type",
                 trait_obj,
             ));
             err.note(impl_trait_msg);
@@ -1813,3 +1837,39 @@ impl NextTypeParamName for &[hir::GenericParam<'_>] {
             .to_string()
     }
 }
+
+fn suggest_trait_object_return_type_alternatives(
+    err: &mut DiagnosticBuilder<'tcx>,
+    ret_ty: Span,
+    trait_obj: &str,
+    is_object_safe: bool,
+) {
+    err.span_suggestion(
+        ret_ty,
+        "use some type `T` that is `T: Sized` as the return type if all return paths have the \
+            same type",
+        "T".to_string(),
+        Applicability::MaybeIncorrect,
+    );
+    err.span_suggestion(
+        ret_ty,
+        &format!(
+            "use `impl {}` as the return type if all return paths have the same type but you \
+                want to expose only the trait in the signature",
+            trait_obj,
+        ),
+        format!("impl {}", trait_obj),
+        Applicability::MaybeIncorrect,
+    );
+    if is_object_safe {
+        err.span_suggestion(
+            ret_ty,
+            &format!(
+                "use a boxed trait object if all return paths implement trait `{}`",
+                trait_obj,
+            ),
+            format!("Box<dyn {}>", trait_obj),
+            Applicability::MaybeIncorrect,
+        );
+    }
+}