Specific error for unsized dyn Trait return type

Suggest `impl Trait` when possible, and `Box<dyn Trait>` otherwise.

Author: estebank

src/librustc/traits/error_reporting.rs

@@ -1,3 +1,4 @@
+// ignore-tidy-filelength
 use super::{
     ConstEvalFailure, EvaluationResult, FulfillmentError, FulfillmentErrorCode,
     MismatchedProjectionTypes, ObjectSafetyViolation, Obligation, ObligationCause,
@@ -22,9 +23,12 @@ use crate::ty::TypeckTables;
 use crate::ty::{self, AdtKind, DefIdTree, ToPolyTraitRef, ToPredicate, Ty, TyCtxt, TypeFoldable};
 
 use rustc_data_structures::fx::{FxHashMap, FxHashSet};
-use rustc_errors::{pluralize, struct_span_err, Applicability, DiagnosticBuilder, Style};
+use rustc_errors::{
+    error_code, pluralize, struct_span_err, Applicability, DiagnosticBuilder, Style,
+};
 use rustc_hir as hir;
 use rustc_hir::def_id::{DefId, LOCAL_CRATE};
+use rustc_hir::intravisit::Visitor;
 use rustc_hir::Node;
 use rustc_span::source_map::SourceMap;
 use rustc_span::symbol::{kw, sym};
@@ -758,7 +762,7 @@ impl<'a, 'tcx> InferCtxt<'a, 'tcx> {
                                 )),
                                 Some(
                                     "the question mark operation (`?`) implicitly performs a \
-                                 conversion on the error value using the `From` trait"
+                                     conversion on the error value using the `From` trait"
                                         .to_owned(),
                                 ),
                             )
@@ -835,6 +839,10 @@ impl<'a, 'tcx> InferCtxt<'a, 'tcx> {
                         self.suggest_remove_reference(&obligation, &mut err, &trait_ref);
                         self.suggest_semicolon_removal(&obligation, &mut err, span, &trait_ref);
                         self.note_version_mismatch(&mut err, &trait_ref);
+                        if self.suggest_impl_trait(&mut err, span, &obligation, &trait_ref) {
+                            err.emit();
+                            return;
+                        }
 
                         // Try to report a help message
                         if !trait_ref.has_infer_types()
@@ -1696,6 +1704,195 @@ impl<'a, 'tcx> InferCtxt<'a, 'tcx> {
         }
     }
 
+    fn suggest_impl_trait(
+        &self,
+        err: &mut DiagnosticBuilder<'tcx>,
+        span: Span,
+        obligation: &PredicateObligation<'tcx>,
+        trait_ref: &ty::Binder<ty::TraitRef<'tcx>>,
+    ) -> bool {
+        if let ObligationCauseCode::SizedReturnType = obligation.cause.code.peel_derives() {
+        } else {
+            return false;
+        }
+
+        let hir = self.tcx.hir();
+        let parent_node = hir.get_parent_node(obligation.cause.body_id);
+        let node = hir.find(parent_node);
+        if let Some(hir::Node::Item(hir::Item {
+            kind: hir::ItemKind::Fn(sig, _, body_id), ..
+        })) = node
+        {
+            let body = hir.body(*body_id);
+            let trait_ref = self.resolve_vars_if_possible(trait_ref);
+            let ty = trait_ref.skip_binder().self_ty();
+            if let ty::Dynamic(..) = ty.kind {
+            } else {
+                // We only want to suggest `impl Trait` to `dyn Trait`s.
+                // For example, `fn foo() -> str` needs to be filtered out.
+                return false;
+            }
+            // Use `TypeVisitor` instead of the output type directly to find the span of `ty` for
+            // cases like `fn foo() -> (dyn Trait, i32) {}`.
+            // Recursively look for `TraitObject` types and if there's only one, use that span to
+            // suggest `impl Trait`.
+
+            struct ReturnsVisitor<'v>(Vec<&'v hir::Expr<'v>>);
+
+            impl<'v> Visitor<'v> for ReturnsVisitor<'v> {
+                type Map = rustc::hir::map::Map<'v>;
+
+                fn nested_visit_map(&mut self) -> hir::intravisit::NestedVisitorMap<'_, Self::Map> {
+                    hir::intravisit::NestedVisitorMap::None
+                }
+
+                fn visit_expr(&mut self, ex: &'v hir::Expr<'v>) {
+                    match ex.kind {
+                        hir::ExprKind::Ret(Some(ex)) => self.0.push(ex),
+                        _ => {}
+                    }
+                    hir::intravisit::walk_expr(self, ex);
+                }
+
+                fn visit_body(&mut self, body: &'v hir::Body<'v>) {
+                    if body.generator_kind().is_none() {
+                        if let hir::ExprKind::Block(block, None) = body.value.kind {
+                            if let Some(expr) = block.expr {
+                                self.0.push(expr);
+                            }
+                        }
+                    }
+                    hir::intravisit::walk_body(self, body);
+                }
+            }
+
+            // Visit to make sure there's a single `return` type to suggest `impl Trait`,
+            // otherwise suggest using `Box<dyn Trait>` or an enum.
+            let mut visitor = ReturnsVisitor(vec![]);
+            visitor.visit_body(&body);
+
+            let tables = self.in_progress_tables.map(|t| t.borrow()).unwrap();
+
+            if let hir::FunctionRetTy::Return(ret_ty) = sig.decl.output {
+                let mut all_returns_conform_to_trait = true;
+                let mut all_returns_have_same_type = true;
+                let mut last_ty = None;
+                if let Some(ty_ret_ty) = tables.node_type_opt(ret_ty.hir_id) {
+                    let cause = ObligationCause::misc(ret_ty.span, ret_ty.hir_id);
+                    if let ty::Dynamic(predicates, _) = &ty_ret_ty.kind {
+                        for predicate in predicates.iter() {
+                            for expr in &visitor.0 {
+                                if let Some(returned_ty) = tables.node_type_opt(expr.hir_id) {
+                                    if let Some(ty) = last_ty {
+                                        all_returns_have_same_type &= ty == returned_ty;
+                                    }
+                                    last_ty = Some(returned_ty);
+
+                                    let param_env = ty::ParamEnv::empty();
+                                    let pred = predicate.with_self_ty(self.tcx, returned_ty);
+                                    let obligation =
+                                        Obligation::new(cause.clone(), param_env, pred);
+                                    all_returns_conform_to_trait &=
+                                        self.predicate_may_hold(&obligation);
+                                }
+                            }
+                        }
+                    }
+                } else {
+                    // We still want to verify whether all the return types conform to each other.
+                    for expr in &visitor.0 {
+                        if let Some(returned_ty) = tables.node_type_opt(expr.hir_id) {
+                            if let Some(ty) = last_ty {
+                                all_returns_have_same_type &= ty == returned_ty;
+                            }
+                            last_ty = Some(returned_ty);
+                        }
+                    }
+                }
+
+                if let (true, hir::TyKind::TraitObject(..), Ok(snippet), true, Some(last_ty)) = (
+                    ret_ty.span.overlaps(span),
+                    &ret_ty.kind,
+                    self.tcx.sess.source_map().span_to_snippet(ret_ty.span),
+                    all_returns_conform_to_trait,
+                    last_ty,
+                ) {
+                    err.code = Some(error_code!(E0746));
+                    err.set_primary_message(
+                        "return type cannot have a bare trait because it must be `Sized`",
+                    );
+                    err.children.clear();
+                    let impl_trait_msg = "for information on `impl Trait`, see \
+                        <https://doc.rust-lang.org/book/ch10-02-traits.html\
+                        #returning-types-that-implement-traits>";
+                    let trait_obj_msg = "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>";
+                    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 {
+                        err.span_suggestion(
+                            ret_ty.span,
+                            &format!(
+                                "you can use the `impl Trait` feature \
+                                 in the return type because all the return paths are of type \
+                                 `{}`, which implements `dyn {}`",
+                                last_ty, trait_obj,
+                            ),
+                            format!("impl {}", trait_obj),
+                            Applicability::MaybeIncorrect,
+                        );
+                        err.note(impl_trait_msg);
+                    } else {
+                        let mut suggestions = visitor
+                            .0
+                            .iter()
+                            .map(|expr| {
+                                (
+                                    expr.span,
+                                    format!(
+                                        "Box::new({})",
+                                        self.tcx
+                                            .sess
+                                            .source_map()
+                                            .span_to_snippet(expr.span)
+                                            .unwrap()
+                                    ),
+                                )
+                            })
+                            .collect::<Vec<_>>();
+                        suggestions.push((
+                            ret_ty.span,
+                            format!("Box<{}{}>", if has_dyn { "" } else { "dyn " }, snippet),
+                        ));
+                        err.multipart_suggestion(
+                            "if the performance implications are acceptable, you can return a \
+                             trait object",
+                            suggestions,
+                            Applicability::MaybeIncorrect,
+                        );
+                        err.span_help(
+                            visitor.0.iter().map(|expr| expr.span).collect::<Vec<_>>(),
+                            &format!(
+                                "if all the returned values were of the same type you could use \
+                                 `impl {}` as the return type",
+                                trait_obj,
+                            ),
+                        );
+                        err.help(
+                            "alternatively, you can always create a new `enum` with a variant \
+                             for each returned type",
+                        );
+                        err.note(impl_trait_msg);
+                        err.note(trait_obj_msg);
+                    }
+                    return true;
+                }
+            }
+        }
+        false
+    }
+
     /// Given some node representing a fn-like thing in the HIR map,
     /// returns a span and `ArgKind` information that describes the
     /// arguments it expects. This can be supplied to

src/librustc/traits/mod.rs

@@ -1171,6 +1171,17 @@ impl<'tcx> ObligationCause<'tcx> {
     }
 }
 
+impl<'tcx> ObligationCauseCode<'tcx> {
+    pub fn peel_derives(&self) -> &Self {
+        match self {
+            BuiltinDerivedObligation(cause) | ImplDerivedObligation(cause) => {
+                cause.parent_code.peel_derives()
+            }
+            _ => self,
+        }
+    }
+}
+
 impl<'tcx, N> Vtable<'tcx, N> {
     pub fn nested_obligations(self) -> Vec<N> {
         match self {

src/librustc_error_codes/error_codes.rs

@@ -608,4 +608,5 @@ E0745: include_str!("./error_codes/E0745.md"),
     E0726, // non-explicit (not `'_`) elided lifetime in unsupported position
     E0727, // `async` generators are not yet supported
     E0739, // invalid track_caller application/syntax
+    E0746, // `dyn Trait` return type
 }

src/test/ui/error-codes/E0746.rs

@@ -0,0 +1,17 @@
+struct Struct;
+trait Trait {}
+impl Trait for Struct {}
+impl Trait for u32 {}
+
+fn foo() -> dyn Trait { Struct }
+//~^ ERROR E0746
+//~| ERROR E0308
+
+fn bar() -> dyn Trait { //~ ERROR E0746
+    if true {
+        return 0; //~ ERROR E0308
+    }
+    42 //~ ERROR E0308
+}
+
+fn main() {}

src/test/ui/error-codes/E0746.stderr

@@ -0,0 +1,62 @@
+error[E0308]: mismatched types
+  --> $DIR/E0746.rs:6:25
+   |
+LL | fn foo() -> dyn Trait { Struct }
+   |             ---------   ^^^^^^ expected trait `Trait`, found struct `Struct`
+   |             |
+   |             expected `(dyn Trait + 'static)` because of return type
+   |
+   = note: expected trait object `(dyn Trait + 'static)`
+                    found struct `Struct`
+
+error[E0746]: return type cannot have a bare trait because it must be `Sized`
+  --> $DIR/E0746.rs:6:13
+   |
+LL | fn foo() -> dyn Trait { Struct }
+   |             ^^^^^^^^^ doesn't have a size known at compile-time
+   |
+   = note: for information on `impl Trait`, see <https://doc.rust-lang.org/book/ch10-02-traits.html#returning-types-that-implement-traits>
+help: you can use the `impl Trait` feature in the return type because all the return paths are of type `Struct`, which implements `dyn Trait`
+   |
+LL | fn foo() -> impl Trait { Struct }
+   |             ^^^^^^^^^^
+
+error[E0746]: return type cannot have a bare trait because it must be `Sized`
+  --> $DIR/E0746.rs:10:13
+   |
+LL | fn bar() -> dyn Trait {
+   |             ^^^^^^^^^ doesn't have a size known at compile-time
+   |
+   = note: for information on `impl Trait`, see <https://doc.rust-lang.org/book/ch10-02-traits.html#returning-types-that-implement-traits>
+help: you can use the `impl Trait` feature in the return type because all the return paths are of type `{integer}`, which implements `dyn Trait`
+   |
+LL | fn bar() -> impl Trait {
+   |             ^^^^^^^^^^
+
+error[E0308]: mismatched types
+  --> $DIR/E0746.rs:12:16
+   |
+LL | fn bar() -> dyn Trait {
+   |             --------- expected `(dyn Trait + 'static)` because of return type
+LL |     if true {
+LL |         return 0;
+   |                ^ expected trait `Trait`, found integer
+   |
+   = note: expected trait object `(dyn Trait + 'static)`
+                      found type `{integer}`
+
+error[E0308]: mismatched types
+  --> $DIR/E0746.rs:14:5
+   |
+LL | fn bar() -> dyn Trait {
+   |             --------- expected `(dyn Trait + 'static)` because of return type
+...
+LL |     42
+   |     ^^ expected trait `Trait`, found integer
+   |
+   = note: expected trait object `(dyn Trait + 'static)`
+                      found type `{integer}`
+
+error: aborting due to 5 previous errors
+
+For more information about this error, try `rustc --explain E0308`.

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

@@ -0,0 +1,36 @@
+#![allow(bare_trait_objects)]
+struct Struct;
+trait Trait {}
+impl Trait for Struct {}
+impl Trait for u32 {}
+
+fn fuz() -> (usize, Trait) { (42, Struct) }
+//~^ ERROR E0277
+//~| ERROR E0308
+fn bar() -> (usize, dyn Trait) { (42, Struct) }
+//~^ ERROR E0277
+//~| ERROR E0308
+fn bap() -> Trait { Struct }
+//~^ ERROR E0746
+//~| ERROR E0308
+fn ban() -> dyn Trait { Struct }
+//~^ ERROR E0746
+//~| ERROR E0308
+fn bak() -> dyn Trait { unimplemented!() } //~ ERROR E0277
+// Suggest using `Box<dyn Trait>`
+fn bal() -> dyn Trait { //~ ERROR E0746
+    if true {
+        return Struct; //~ ERROR E0308
+    }
+    42 //~ ERROR E0308
+}
+
+// Suggest using `impl Trait`
+fn bat() -> dyn Trait { //~ ERROR E0746
+    if true {
+        return 0; //~ ERROR E0308
+    }
+    42 //~ ERROR E0308
+}
+
+fn main() {}