Account for object safety when suggesting Box<dyn Trait>

estebank · estebank · commit 00e262689599 · 2020-01-16T09:49:14.000-08:00
diff --git a/src/librustc/traits/error_reporting/suggestions.rs b/src/librustc/traits/error_reporting/suggestions.rs
@@ -696,11 +696,13 @@ impl<'a, 'tcx> InferCtxt<'a, 'tcx> {
                     format!("Box<{}{}>", if has_dyn { "" } else { "dyn " }, snippet),
                 ));
                 err.multipart_suggestion(
-                        "return a trait object instead",
+                    "return a boxed trait object instead",
                     suggestions,
                     Applicability::MaybeIncorrect,
                 );
             } else {
+                // This is currently not possible to trigger because E0038 takes precedence, but
+                // leave it in for completeness in case anything changes in an earlier stage.
                 err.note(&format!(
                     "if trait `{}` was object safe, you could return a trait object",
                     trait_obj,
diff --git a/src/librustc_hir/hir.rs b/src/librustc_hir/hir.rs
@@ -377,6 +377,13 @@ pub enum GenericBound<'hir> {
 }
 
 impl GenericBound<'_> {
+    pub fn trait_def_id(&self) -> Option<DefId> {
+        match self {
+            GenericBound::Trait(data, _) => Some(data.trait_ref.trait_def_id()),
+            _ => None,
+        }
+    }
+
     pub fn span(&self) -> Span {
         match self {
             &GenericBound::Trait(ref t, ..) => t.span,
diff --git a/src/librustc_typeck/check/coercion.rs b/src/librustc_typeck/check/coercion.rs
@@ -55,6 +55,7 @@ use crate::check::{FnCtxt, Needs};
 use rustc::infer::type_variable::{TypeVariableOrigin, TypeVariableOriginKind};
 use rustc::infer::{Coercion, InferOk, InferResult};
 use rustc::session::parse::feature_err;
+use rustc::traits::object_safety_violations;
 use rustc::traits::{self, ObligationCause, ObligationCauseCode};
 use rustc::ty::adjustment::{
     Adjust, Adjustment, AllowTwoPhase, AutoBorrow, AutoBorrowMutability, PointerCast,
@@ -68,8 +69,8 @@ use rustc_error_codes::*;
 use rustc_errors::{struct_span_err, DiagnosticBuilder};
 use rustc_hir as hir;
 use rustc_hir::def_id::DefId;
-use rustc_span::{self, Span};
 use rustc_span::symbol::sym;
+use rustc_span::{self, Span};
 use rustc_target::spec::abi::Abi;
 use smallvec::{smallvec, SmallVec};
 use std::ops::Deref;
@@ -1311,7 +1312,7 @@ impl<'tcx, 'exprs, E: AsCoercionSite> CoerceMany<'tcx, 'exprs, E> {
         let mut err = fcx.report_mismatched_types(cause, expected, found, ty_err);
 
         let mut pointing_at_return_type = false;
-        let mut return_sp = None;
+        let mut fn_output = None;
 
         // Verify that this is a tail expression of a function, otherwise the
         // label pointing out the cause for the type coercion will be wrong
@@ -1348,11 +1349,11 @@ impl<'tcx, 'exprs, E: AsCoercionSite> CoerceMany<'tcx, 'exprs, E> {
                 );
             }
             if !pointing_at_return_type {
-                return_sp = Some(fn_decl.output.span()); // `impl Trait` return type
+                fn_output = Some(&fn_decl.output); // `impl Trait` return type
             }
         }
-        if let (Some(sp), Some(return_sp)) = (fcx.ret_coercion_span.borrow().as_ref(), return_sp) {
-            self.add_impl_trait_explanation(&mut err, fcx, expected, *sp, return_sp);
+        if let (Some(sp), Some(fn_output)) = (fcx.ret_coercion_span.borrow().as_ref(), fn_output) {
+            self.add_impl_trait_explanation(&mut err, fcx, expected, *sp, fn_output);
         }
         err
     }
@@ -1363,8 +1364,9 @@ impl<'tcx, 'exprs, E: AsCoercionSite> CoerceMany<'tcx, 'exprs, E> {
         fcx: &FnCtxt<'a, 'tcx>,
         expected: Ty<'tcx>,
         sp: Span,
-        return_sp: Span,
+        fn_output: &hir::FunctionRetTy<'_>,
     ) {
+        let return_sp = fn_output.span();
         err.span_label(return_sp, "expected because this return type...");
         err.span_label(
             sp,
@@ -1386,11 +1388,42 @@ impl<'tcx, 'exprs, E: AsCoercionSite> CoerceMany<'tcx, 'exprs, E> {
             .unwrap_or_else(|_| "dyn Trait".to_string());
         let mut snippet_iter = snippet.split_whitespace();
         let has_impl = snippet_iter.next().map_or(false, |s| s == "impl");
+        // Only suggest `Box<dyn Trait>` if `Trait` in `impl Trait` is object safe.
+        let mut is_object_safe = false;
+        if let hir::FunctionRetTy::Return(ty) = fn_output {
+            // Get the return type.
+            if let hir::TyKind::Def(..) = ty.kind {
+                let ty = AstConv::ast_ty_to_ty(fcx, ty);
+                // Get the `impl Trait`'s `DefId`.
+                if let ty::Opaque(def_id, _) = ty.kind {
+                    let hir_id = fcx.tcx.hir().as_local_hir_id(def_id).unwrap();
+                    // Get the `impl Trait`'s `Item` so that we can get its trait bounds and
+                    // get the `Trait`'s `DefId`.
+                    if let hir::ItemKind::OpaqueTy(hir::OpaqueTy { bounds, .. }) =
+                        fcx.tcx.hir().expect_item(hir_id).kind
+                    {
+                        // Are of this `impl Trait`'s traits object safe?
+                        is_object_safe = bounds.iter().all(|bound| {
+                            bound.trait_def_id().map_or(false, |def_id| {
+                                object_safety_violations(fcx.tcx, def_id).is_empty()
+                            })
+                        })
+                    }
+                }
+            }
+        };
         if has_impl {
-            err.help(&format!(
-                "you can instead return a trait object using `Box<dyn {}>`",
-                &snippet[5..]
-            ));
+            if is_object_safe {
+                err.help(&format!(
+                    "you can instead return a boxed trait object using `Box<dyn {}>`",
+                    &snippet[5..]
+                ));
+            } else {
+                err.help(&format!(
+                    "if the trait `{}` were object safe, you could return a boxed trait object",
+                    &snippet[5..]
+                ));
+            }
             err.note(trait_obj_msg);
         }
         err.help("alternatively, create a new `enum` with a variant for each returned type");
diff --git a/src/test/ui/impl-trait/dyn-trait-return-should-be-impl-trait.stderr b/src/test/ui/impl-trait/dyn-trait-return-should-be-impl-trait.stderr
@@ -86,7 +86,7 @@ LL | fn bal() -> dyn Trait {
    = note: if all the returned values were of the same type you could use `impl Trait` as the return type
    = note: for information on `impl Trait`, see <https://doc.rust-lang.org/book/ch10-02-traits.html#returning-types-that-implement-traits>
    = note: you can create a new `enum` with a variant for each returned type
-help: return a trait object instead
+help: return a boxed trait object instead
    |
 LL | fn bal() -> Box<dyn Trait> {
 LL |     if true {
diff --git a/src/test/ui/impl-trait/equality.stderr b/src/test/ui/impl-trait/equality.stderr
@@ -12,7 +12,7 @@ LL |     0_u32
    |
    = note: to return `impl Trait`, all returned values must be of the same type
    = 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 instead return a trait object using `Box<dyn Foo>`
+   = help: if the trait `Foo` were object safe, you could return a boxed trait object
    = 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: alternatively, create a new `enum` with a variant for each returned type
 
diff --git a/src/test/ui/impl-trait/object-unsafe-trait-in-return-position-dyn-trait.rs b/src/test/ui/impl-trait/object-unsafe-trait-in-return-position-dyn-trait.rs
@@ -0,0 +1,35 @@
+#![allow(bare_trait_objects)]
+trait NotObjectSafe {
+    fn foo() -> Self;
+}
+
+struct A;
+struct B;
+
+impl NotObjectSafe for A {
+    fn foo() -> Self {
+        A
+    }
+}
+
+impl NotObjectSafe for B {
+    fn foo() -> Self {
+        B
+    }
+}
+
+fn car() -> dyn NotObjectSafe { //~ ERROR the trait `NotObjectSafe` cannot be made into an object
+    if true {
+        return A;
+    }
+    B
+}
+
+fn cat() -> Box<dyn NotObjectSafe> { //~ ERROR the trait `NotObjectSafe` cannot be made into an
+    if true {
+        return Box::new(A);
+    }
+    Box::new(B)
+}
+
+fn main() {}
diff --git a/src/test/ui/impl-trait/object-unsafe-trait-in-return-position-dyn-trait.stderr b/src/test/ui/impl-trait/object-unsafe-trait-in-return-position-dyn-trait.stderr
@@ -0,0 +1,21 @@
+error[E0038]: the trait `NotObjectSafe` cannot be made into an object
+  --> $DIR/object-unsafe-trait-in-return-position-dyn-trait.rs:21:1
+   |
+LL |     fn foo() -> Self;
+   |        --- associated function `foo` has no `self` parameter
+...
+LL | fn car() -> dyn NotObjectSafe {
+   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `NotObjectSafe` cannot be made into an object
+
+error[E0038]: the trait `NotObjectSafe` cannot be made into an object
+  --> $DIR/object-unsafe-trait-in-return-position-dyn-trait.rs:28:1
+   |
+LL |     fn foo() -> Self;
+   |        --- associated function `foo` has no `self` parameter
+...
+LL | fn cat() -> Box<dyn NotObjectSafe> {
+   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `NotObjectSafe` cannot be made into an object
+
+error: aborting due to 2 previous errors
+
+For more information about this error, try `rustc --explain E0038`.
diff --git a/src/test/ui/impl-trait/object-unsafe-trait-in-return-position-impl-trait.rs b/src/test/ui/impl-trait/object-unsafe-trait-in-return-position-impl-trait.rs
@@ -0,0 +1,46 @@
+trait NotObjectSafe {
+    fn foo() -> Self;
+}
+
+trait ObjectSafe {
+    fn bar(&self);
+}
+
+struct A;
+struct B;
+
+impl NotObjectSafe for A {
+    fn foo() -> Self {
+        A
+    }
+}
+
+impl NotObjectSafe for B {
+    fn foo() -> Self {
+        B
+    }
+}
+
+impl ObjectSafe for A {
+    fn bar(&self) {}
+}
+
+impl ObjectSafe for B {
+    fn bar(&self) {}
+}
+
+fn can() -> impl NotObjectSafe {
+    if true {
+        return A;
+    }
+    B //~ ERROR mismatched types
+}
+
+fn cat() -> impl ObjectSafe {
+    if true {
+        return A;
+    }
+    B //~ ERROR mismatched types
+}
+
+fn main() {}
diff --git a/src/test/ui/impl-trait/object-unsafe-trait-in-return-position-impl-trait.stderr b/src/test/ui/impl-trait/object-unsafe-trait-in-return-position-impl-trait.stderr
@@ -0,0 +1,39 @@
+error[E0308]: mismatched types
+  --> $DIR/object-unsafe-trait-in-return-position-impl-trait.rs:36:5
+   |
+LL | fn can() -> impl NotObjectSafe {
+   |             ------------------ expected because this return type...
+LL |     if true {
+LL |         return A;
+   |                - ...is found to be `A` here
+LL |     }
+LL |     B
+   |     ^ expected struct `A`, found struct `B`
+   |
+   = note: to return `impl Trait`, all returned values must be of the same type
+   = note: for information on `impl Trait`, see <https://doc.rust-lang.org/book/ch10-02-traits.html#returning-types-that-implement-traits>
+   = help: if the trait `NotObjectSafe` were object safe, you could return a boxed trait object
+   = 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: alternatively, create a new `enum` with a variant for each returned type
+
+error[E0308]: mismatched types
+  --> $DIR/object-unsafe-trait-in-return-position-impl-trait.rs:43:5
+   |
+LL | fn cat() -> impl ObjectSafe {
+   |             --------------- expected because this return type...
+LL |     if true {
+LL |         return A;
+   |                - ...is found to be `A` here
+LL |     }
+LL |     B
+   |     ^ expected struct `A`, found struct `B`
+   |
+   = note: to return `impl Trait`, all returned values must be of the same type
+   = 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 instead return a boxed trait object using `Box<dyn ObjectSafe>`
+   = 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: alternatively, create a new `enum` with a variant for each returned type
+
+error: aborting due to 2 previous errors
+
+For more information about this error, try `rustc --explain E0308`.
diff --git a/src/test/ui/point-to-type-err-cause-on-impl-trait-return.stderr b/src/test/ui/point-to-type-err-cause-on-impl-trait-return.stderr
@@ -12,7 +12,7 @@ LL |     1u32
    |
    = note: to return `impl Trait`, all returned values must be of the same type
    = 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 instead return a trait object using `Box<dyn std::fmt::Display>`
+   = help: you can instead return a boxed trait object using `Box<dyn std::fmt::Display>`
    = 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: alternatively, create a new `enum` with a variant for each returned type
 
@@ -30,7 +30,7 @@ LL |         return 1u32;
    |
    = note: to return `impl Trait`, all returned values must be of the same type
    = 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 instead return a trait object using `Box<dyn std::fmt::Display>`
+   = help: you can instead return a boxed trait object using `Box<dyn std::fmt::Display>`
    = 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: alternatively, create a new `enum` with a variant for each returned type
 
@@ -48,7 +48,7 @@ LL |         1u32
    |
    = note: to return `impl Trait`, all returned values must be of the same type
    = 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 instead return a trait object using `Box<dyn std::fmt::Display>`
+   = help: you can instead return a boxed trait object using `Box<dyn std::fmt::Display>`
    = 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: alternatively, create a new `enum` with a variant for each returned type
 
@@ -78,7 +78,7 @@ LL |         _ => 1u32,
    |
    = note: to return `impl Trait`, all returned values must be of the same type
    = 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 instead return a trait object using `Box<dyn std::fmt::Display>`
+   = help: you can instead return a boxed trait object using `Box<dyn std::fmt::Display>`
    = 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: alternatively, create a new `enum` with a variant for each returned type
 
@@ -98,7 +98,7 @@ LL | |     }
    |
    = note: to return `impl Trait`, all returned values must be of the same type
    = 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 instead return a trait object using `Box<dyn std::fmt::Display>`
+   = help: you can instead return a boxed trait object using `Box<dyn std::fmt::Display>`
    = 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: alternatively, create a new `enum` with a variant for each returned type
 
@@ -116,7 +116,7 @@ LL |             1u32
    |
    = note: to return `impl Trait`, all returned values must be of the same type
    = 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 instead return a trait object using `Box<dyn std::fmt::Display>`
+   = help: you can instead return a boxed trait object using `Box<dyn std::fmt::Display>`
    = 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: alternatively, create a new `enum` with a variant for each returned type
 

Original file line number	Diff line number	Diff line change
`@@ -86,7 +86,7 @@ LL \| fn bal() -> dyn Trait {`
`86`	`86`	= note: if all the returned values were of the same type you could use `impl Trait` as the return type
`87`	`87`	= note: for information on `impl Trait`, see <https://doc.rust-lang.org/book/ch10-02-traits.html#returning-types-that-implement-traits>
`88`	`88`	= note: you can create a new `enum` with a variant for each returned type
`89`		`-help: return a trait object instead`
	`89`	`+help: return a boxed trait object instead`
`90`	`90`	`\|`
`91`	`91`	`LL \| fn bal() -> Box<dyn Trait> {`
`92`	`92`	`LL \| if true {`
Original file line number	Diff line number	Diff line change
`@@ -12,7 +12,7 @@ LL \| 0_u32`
`12`	`12`	`\|`
`13`	`13`	= note: to return `impl Trait`, all returned values must be of the same type
`14`	`14`	= note: for information on `impl Trait`, see <https://doc.rust-lang.org/book/ch10-02-traits.html#returning-types-that-implement-traits>
`15`		- = help: you can instead return a trait object using `Box<dyn Foo>`
	`15`	+ = help: if the trait `Foo` were object safe, you could return a boxed trait object
`16`	`16`	`= 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>`
`17`	`17`	= help: alternatively, create a new `enum` with a variant for each returned type
`18`	`18`