From e53625706106e0227656ddd2fa4d7df54ae2b90e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Esteban=20K=C3=BCber?= Date: Sat, 11 Apr 2020 11:31:54 -0700 Subject: [PATCH] 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. --- src/librustc_error_codes/error_codes/E0746.md | 3 +- .../traits/error_reporting/suggestions.rs | 121 +++++++++++------- src/librustc_typeck/check/mod.rs | 21 ++- .../dyn-trait-return-should-be-impl-trait.rs | 2 +- ...n-trait-return-should-be-impl-trait.stderr | 23 +++- src/test/ui/issues/issue-18107.stderr | 9 +- 6 files changed, 113 insertions(+), 66 deletions(-) diff --git a/src/librustc_error_codes/error_codes/E0746.md b/src/librustc_error_codes/error_codes/E0746.md index 16b2722f0eac2..305667e58f8fb 100644 --- a/src/librustc_error_codes/error_codes/E0746.md +++ b/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); } diff --git a/src/librustc_trait_selection/traits/error_reporting/suggestions.rs b/src/librustc_trait_selection/traits/error_reporting/suggestions.rs index abebbe9e903dc..74c852046a3f7 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, count) = ret_types.clone().fold( - (None, true, 0), - |(last_ty, mut same, count): (std::option::Option>, 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>, 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`. - 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`. + 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", 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", trait_obj), + Applicability::MaybeIncorrect, + ); + } +} diff --git a/src/librustc_typeck/check/mod.rs b/src/librustc_typeck/check/mod.rs index 1be8d258dcb18..65bcb19b20a89 100644 --- a/src/librustc_typeck/check/mod.rs +++ b/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. diff --git a/src/test/ui/impl-trait/dyn-trait-return-should-be-impl-trait.rs b/src/test/ui/impl-trait/dyn-trait-return-should-be-impl-trait.rs index af368203de021..cbf1daabe2b4e 100644 --- a/src/test/ui/impl-trait/dyn-trait-return-should-be-impl-trait.rs +++ b/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 { 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 a01bfc4a81f47..c55dbd7d2fafe 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 @@ -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 - = note: for information on trait objects, see -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 { 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`. diff --git a/src/test/ui/issues/issue-18107.stderr b/src/test/ui/issues/issue-18107.stderr index 08785ffa73a9f..1eb6822b8a11a 100644 --- a/src/test/ui/issues/issue-18107.stderr +++ b/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 - = note: for information on trait objects, see -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 |