From 00e262689599a6a753bbf7ce8786e07ed100d238 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Esteban=20K=C3=BCber?= Date: Wed, 15 Jan 2020 15:49:54 -0800 Subject: [PATCH] Account for object safety when suggesting `Box` --- .../traits/error_reporting/suggestions.rs | 4 +- src/librustc_hir/hir.rs | 7 +++ src/librustc_typeck/check/coercion.rs | 53 +++++++++++++++---- ...n-trait-return-should-be-impl-trait.stderr | 2 +- src/test/ui/impl-trait/equality.stderr | 2 +- ...safe-trait-in-return-position-dyn-trait.rs | 35 ++++++++++++ ...-trait-in-return-position-dyn-trait.stderr | 21 ++++++++ ...afe-trait-in-return-position-impl-trait.rs | 46 ++++++++++++++++ ...trait-in-return-position-impl-trait.stderr | 39 ++++++++++++++ ...type-err-cause-on-impl-trait-return.stderr | 12 ++--- 10 files changed, 202 insertions(+), 19 deletions(-) create mode 100644 src/test/ui/impl-trait/object-unsafe-trait-in-return-position-dyn-trait.rs create mode 100644 src/test/ui/impl-trait/object-unsafe-trait-in-return-position-dyn-trait.stderr create mode 100644 src/test/ui/impl-trait/object-unsafe-trait-in-return-position-impl-trait.rs create mode 100644 src/test/ui/impl-trait/object-unsafe-trait-in-return-position-impl-trait.stderr diff --git a/src/librustc/traits/error_reporting/suggestions.rs b/src/librustc/traits/error_reporting/suggestions.rs index 7c1b1041c34..c3af063d518 100644 --- 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 index 550e3654d08..5c1d600c837 100644 --- 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 { + 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 index 768e532fa3b..a32fbff7bfe 100644 --- 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` 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`", - &snippet[5..] - )); + if is_object_safe { + err.help(&format!( + "you can instead return a boxed trait object using `Box`", + &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 index ac101f8f3ce..977a7ef0e02 100644 --- 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 = 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 { LL | if true { diff --git a/src/test/ui/impl-trait/equality.stderr b/src/test/ui/impl-trait/equality.stderr index a399fadbc5d..9178358b60a 100644 --- 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 - = help: you can instead return a trait object using `Box` + = help: if the trait `Foo` were object safe, you could return a boxed trait object = note: for information on trait objects, see = 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 new file mode 100644 index 00000000000..ab3086c78b3 --- /dev/null +++ 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 { //~ 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 new file mode 100644 index 00000000000..0c8d267c134 --- /dev/null +++ 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 { + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ 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 new file mode 100644 index 00000000000..503515013b9 --- /dev/null +++ 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 new file mode 100644 index 00000000000..dd4260fbe4f --- /dev/null +++ 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 + = help: if the trait `NotObjectSafe` were object safe, you could return a boxed trait object + = note: for information on trait objects, see + = 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 + = help: you can instead return a boxed trait object using `Box` + = note: for information on trait objects, see + = 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 index 9859c73b7b1..b663cccbeef 100644 --- 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 - = help: you can instead return a trait object using `Box` + = help: you can instead return a boxed trait object using `Box` = note: for information on trait objects, see = 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 - = help: you can instead return a trait object using `Box` + = help: you can instead return a boxed trait object using `Box` = note: for information on trait objects, see = 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 - = help: you can instead return a trait object using `Box` + = help: you can instead return a boxed trait object using `Box` = note: for information on trait objects, see = 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 - = help: you can instead return a trait object using `Box` + = help: you can instead return a boxed trait object using `Box` = note: for information on trait objects, see = 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 - = help: you can instead return a trait object using `Box` + = help: you can instead return a boxed trait object using `Box` = note: for information on trait objects, see = 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 - = help: you can instead return a trait object using `Box` + = help: you can instead return a boxed trait object using `Box` = note: for information on trait objects, see = help: alternatively, create a new `enum` with a variant for each returned type