Ensure tail expression will have a Ty for E0746

When the return type is `!Sized` we look for all the returned
expressions in the body to fetch their types and provide a reasonable
suggestion. The tail expression of the body is normally evaluated after
checking whether the return type is `Sized`. Changing the order of the
evaluation produces undesirable knock down effects, so we detect the
specific case that newcomers are likely to encounter ,returning a single
bare trait object, and only in that case we evaluate the tail
expression's type so that the suggestion will be accurate.

Author: estebank

src/librustc_error_codes/error_codes/E0746.md

@@ -2,8 +2,7 @@ Return types cannot be `dyn Trait`s as they must be `Sized`.
 
 Erroneous code example:
 
-```compile_fail,E0277
-# // FIXME: after E0746 is in beta, change the above
+```compile_fail,E0746
 trait T {
     fn bar(&self);
 }

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, count) = ret_types.clone().fold(
-            (None, true, 0),
-            |(last_ty, mut same, count): (std::option::Option<Ty<'_>>, bool, usize), 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, count + 1)
+                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)
+                                })
                             })
-                        }) || count == 0
                     }
                     _ => false,
                 }
@@ -861,7 +878,7 @@ impl<'a, 'tcx> InferCtxtExt<'tcx> for InferCtxt<'a, 'tcx> {
             &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.
+            // suggest `impl Trait` nor trait objects: it is a type mismatch error.
             all_returns_conform_to_trait,
         ) {
             snippet
@@ -879,43 +896,15 @@ 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 count == 0 {
-            // No return paths. Suggest `-> T`, `-> impl Trait`, and if `Trait` is object safe,
-            // `-> Box<dyn Trait>`.
-            err.note(
-                "currently nothing is being returned, depending on the final implementation \
-                 you could change the return type in different ways",
-            );
-            err.span_suggestion(
-                ret_ty.span,
-                "you could use some type `T` that is `T: Sized` as the return type if all return \
-                 paths will have the same type",
-                "T".to_string(),
-                Applicability::MaybeIncorrect,
-            );
-            err.span_suggestion(
+        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,
-                &format!(
-                    "you could use `impl {}` as the return type if all return paths will have the \
-                     same type but you want to expose only the trait in the signature",
-                    trait_obj,
-                ),
-                format!("impl {}", trait_obj),
-                Applicability::MaybeIncorrect,
+                trait_obj,
+                is_object_safe,
             );
-            err.note(impl_trait_msg);
-            if is_object_safe {
-                err.span_suggestion(
-                    ret_ty.span,
-                    &format!(
-                        "you could use a boxed trait object if all return paths `impl` trait `{}`",
-                        trait_obj,
-                    ),
-                    format!("Box<dyn {}>", trait_obj),
-                    Applicability::MaybeIncorrect,
-                );
-                err.note(trait_obj_msg);
-            }
         } else if let (Some(last_ty), true) = (last_ty, all_returns_have_same_type) {
             // Suggest `-> impl Trait`.
             err.span_suggestion(
@@ -1851,3 +1840,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,
+        );
+    }
+}

src/librustc_typeck/check/mod.rs

@@ -1305,7 +1305,6 @@ fn check_fn<'a, 'tcx>(
     let hir = tcx.hir();
 
     let declared_ret_ty = fn_sig.output();
-    fcx.require_type_is_sized(declared_ret_ty, decl.output.span(), traits::SizedReturnType);
     let revealed_ret_ty =
         fcx.instantiate_opaque_types_from_value(fn_id, &declared_ret_ty, decl.output.span());
     debug!("check_fn: declared_ret_ty: {}, revealed_ret_ty: {}", declared_ret_ty, revealed_ret_ty);
@@ -1374,7 +1373,25 @@ fn check_fn<'a, 'tcx>(
 
     inherited.tables.borrow_mut().liberated_fn_sigs_mut().insert(fn_id, fn_sig);
 
-    fcx.check_return_expr(&body.value);
+    if let ty::Dynamic(..) = declared_ret_ty.kind {
+        // FIXME: We need to verify that the return type is `Sized` after the return expression has
+        // been evaluated so that we have types available for all the nodes being returned, but that
+        // requires the coerced evaluated type to be stored. Moving `check_return_expr` before this
+        // causes unsized errors caused by the `declared_ret_ty` to point at the return expression,
+        // while keeping the current ordering we will ignore the tail expression's type because we
+        // don't know it yet. We can't do `check_expr_kind` while keeping `check_return_expr`
+        // because we will trigger "unreachable expression" lints unconditionally.
+        // Because of all of this, we perform a crude check to know whether the simplest `!Sized`
+        // case that a newcomer might make, returning a bare trait, and in that case we populate
+        // the tail expression's type so that the suggestion will be correct, but ignore all other
+        // possible cases.
+        fcx.check_expr(&body.value);
+        fcx.require_type_is_sized(declared_ret_ty, decl.output.span(), traits::SizedReturnType);
+        tcx.sess.delay_span_bug(decl.output.span(), "`!Sized` return type");
+    } else {
+        fcx.require_type_is_sized(declared_ret_ty, decl.output.span(), traits::SizedReturnType);
+        fcx.check_return_expr(&body.value);
+    }
 
     // We insert the deferred_generator_interiors entry after visiting the body.
     // This ensures that all nested generators appear before the entry of this generator.

src/test/ui/impl-trait/dyn-trait-return-should-be-impl-trait.rs

@@ -26,7 +26,7 @@ fn bax() -> dyn Trait { //~ ERROR E0746
     if true {
         Struct
     } else {
-        42
+        42 //~ ERROR `if` and `else` have incompatible types
     }
 }
 fn bam() -> Box<dyn Trait> {

src/test/ui/impl-trait/dyn-trait-return-should-be-impl-trait.stderr

@@ -72,18 +72,15 @@ error[E0746]: return type cannot have an unboxed trait object
 LL | fn bak() -> dyn Trait { unimplemented!() }
    |             ^^^^^^^^^ doesn't have a size known at compile-time
    |
-   = note: currently nothing is being returned, depending on the final implementation you could change the return type in different ways
-   = note: for information on `impl Trait`, see <https://doc.rust-lang.org/book/ch10-02-traits.html#returning-types-that-implement-traits>
-   = note: for information on trait objects, see <https://doc.rust-lang.org/book/ch17-02-trait-objects.html#using-trait-objects-that-allow-for-values-of-different-types>
-help: you could use some type `T` that is `T: Sized` as the return type if all return paths will have the same type
+help: use some type `T` that is `T: Sized` as the return type if all return paths have the same type
    |
 LL | fn bak() -> T { unimplemented!() }
    |             ^
-help: you could use `impl Trait` as the return type if all return paths will have the same type but you want to expose only the trait in the signature
+help: use `impl Trait` as the return type if all return paths have the same type but you want to expose only the trait in the signature
    |
 LL | fn bak() -> impl Trait { unimplemented!() }
    |             ^^^^^^^^^^
-help: you could use a boxed trait object if all return paths `impl` trait `Trait`
+help: use a boxed trait object if all return paths implement trait `Trait`
    |
 LL | fn bak() -> Box<dyn Trait> { unimplemented!() }
    |             ^^^^^^^^^^^^^^
@@ -107,6 +104,18 @@ LL |     }
 LL |     Box::new(42)
    |
 
+error[E0308]: `if` and `else` have incompatible types
+  --> $DIR/dyn-trait-return-should-be-impl-trait.rs:29:9
+   |
+LL | /     if true {
+LL | |         Struct
+   | |         ------ expected because of this
+LL | |     } else {
+LL | |         42
+   | |         ^^ expected struct `Struct`, found integer
+LL | |     }
+   | |_____- `if` and `else` have incompatible types
+
 error[E0746]: return type cannot have an unboxed trait object
   --> $DIR/dyn-trait-return-should-be-impl-trait.rs:25:13
    |
@@ -278,7 +287,7 @@ help: use `impl Trait` as the return type, as all return paths are of type `{int
 LL | fn bay() -> impl Trait {
    |             ^^^^^^^^^^
 
-error: aborting due to 19 previous errors
+error: aborting due to 20 previous errors
 
 Some errors have detailed explanations: E0277, E0308, E0746.
 For more information about an error, try `rustc --explain E0277`.

src/test/ui/issues/issue-18107.stderr

@@ -4,18 +4,15 @@ error[E0746]: return type cannot have an unboxed trait object
 LL |     dyn AbstractRenderer
    |     ^^^^^^^^^^^^^^^^^^^^ doesn't have a size known at compile-time
    |
-   = note: currently nothing is being returned, depending on the final implementation you could change the return type in different ways
-   = note: for information on `impl Trait`, see <https://doc.rust-lang.org/book/ch10-02-traits.html#returning-types-that-implement-traits>
-   = note: for information on trait objects, see <https://doc.rust-lang.org/book/ch17-02-trait-objects.html#using-trait-objects-that-allow-for-values-of-different-types>
-help: you could use some type `T` that is `T: Sized` as the return type if all return paths will have the same type
+help: use some type `T` that is `T: Sized` as the return type if all return paths have the same type
    |
 LL |     T
    |
-help: you could use `impl AbstractRenderer` as the return type if all return paths will have the same type but you want to expose only the trait in the signature
+help: use `impl AbstractRenderer` as the return type if all return paths have the same type but you want to expose only the trait in the signature
    |
 LL |     impl AbstractRenderer
    |
-help: you could use a boxed trait object if all return paths `impl` trait `AbstractRenderer`
+help: use a boxed trait object if all return paths implement trait `AbstractRenderer`
    |
 LL |     Box<dyn AbstractRenderer>
    |