Skip to content

Commit

Permalink
Rollup merge of #111704 - compiler-errors:sized-return-cleanup, r=oli…
Browse files Browse the repository at this point in the history 
…-obk

Remove return type sized check hack from hir typeck

Remove a bunch of special-cased suggestions when someone returns `-> dyn Trait` that checks for type equality, etc.

This was a pretty complex piece of code that also relied on a hack in hir typeck (see changes to `compiler/rustc_hir_typeck/src/check.rs`), and I'm not convinced that it's necessary to maintain, when all we really need to tell the user is that they should return `-> impl Trait` or `-> Box<dyn Trait>`, depending on their specific use-case.

This is necessary because we may need to move the "return type is sized" check from hir typeck to wfcheck, which does not have access to typeck results. This is a prerequisite for that, and I'm fairly confident that the diagnostics "regressions" here are not a big deal.
  • Loading branch information
@Dylan-DPC
Dylan-DPC authored May 23, 2023
2 parents 00185be + 3db2bcf commit 4b26b80
Show file tree
Hide file tree
Showing 15 changed files with 182 additions and 455 deletions.
20 changes: 2 additions & 18 deletions compiler/rustc_hir_typeck/src/check.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -103,24 +103,8 @@ pub(super) fn check_fn<'a, 'tcx>(

fcx.typeck_results.borrow_mut().liberated_fn_sigs_mut().insert(fn_id, fn_sig);

if let ty::Dynamic(_, _, ty::Dyn) = 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);
} else {
fcx.require_type_is_sized(declared_ret_ty, decl.output.span(), traits::SizedReturnType);
fcx.check_return_expr(&body.value, false);
}
fcx.require_type_is_sized(declared_ret_ty, decl.output.span(), traits::SizedReturnType);
fcx.check_return_expr(&body.value, false);

// We insert the deferred_generator_interiors entry after visiting the body.
// This ensures that all nested generators appear before the entry of this generator.
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_trait_selection/src/traits/error_reporting/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -885,7 +885,7 @@ impl<'tcx> TypeErrCtxtExt<'tcx> for TypeErrCtxt<'_, 'tcx> {
return;
}

if self.suggest_impl_trait(&mut err, span, &obligation, trait_predicate) {
if self.suggest_impl_trait(&mut err, &obligation, trait_predicate) {
err.emit();
return;
}
Expand Down
277 changes: 48 additions & 229 deletions compiler/rustc_trait_selection/src/traits/error_reporting/suggestions.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,9 +30,9 @@ use rustc_middle::hir::map;
use rustc_middle::ty::error::TypeError::{self, Sorts};
use rustc_middle::ty::{
self, suggest_arbitrary_trait_bound, suggest_constraining_type_param, AdtKind,
GeneratorDiagnosticData, GeneratorInteriorTypeCause, Infer, InferTy, InternalSubsts,
IsSuggestable, ToPredicate, Ty, TyCtxt, TypeAndMut, TypeFoldable, TypeFolder,
TypeSuperFoldable, TypeVisitableExt, TypeckResults,
GeneratorDiagnosticData, GeneratorInteriorTypeCause, InferTy, InternalSubsts, IsSuggestable,
ToPredicate, Ty, TyCtxt, TypeAndMut, TypeFoldable, TypeFolder, TypeSuperFoldable,
TypeVisitableExt, TypeckResults,
};
use rustc_span::def_id::LocalDefId;
use rustc_span::symbol::{sym, Ident, Symbol};
Expand Down Expand Up @@ -261,7 +261,6 @@ pub trait TypeErrCtxtExt<'tcx> {
fn suggest_impl_trait(
&self,
err: &mut Diagnostic,
span: Span,
obligation: &PredicateObligation<'tcx>,
trait_pred: ty::PolyTraitPredicate<'tcx>,
) -> bool;
Expand Down Expand Up @@ -1792,215 +1791,66 @@ impl<'tcx> TypeErrCtxtExt<'tcx> for TypeErrCtxt<'_, 'tcx> {
fn suggest_impl_trait(
&self,
err: &mut Diagnostic,
span: Span,
obligation: &PredicateObligation<'tcx>,
trait_pred: ty::PolyTraitPredicate<'tcx>,
) -> bool {
match obligation.cause.code().peel_derives() {
// Only suggest `impl Trait` if the return type is unsized because it is `dyn Trait`.
ObligationCauseCode::SizedReturnType => {}
_ => return false,
}

let hir = self.tcx.hir();
let fn_hir_id = hir.local_def_id_to_hir_id(obligation.cause.body_id);
let node = hir.find_by_def_id(obligation.cause.body_id);
let Some(hir::Node::Item(hir::Item {
kind: hir::ItemKind::Fn(sig, _, body_id),
..
})) = node
else {
let ObligationCauseCode::SizedReturnType = obligation.cause.code() else {
return false;
};
let body = hir.body(*body_id);
let trait_pred = self.resolve_vars_if_possible(trait_pred);
let ty = trait_pred.skip_binder().self_ty();
let is_object_safe = match ty.kind() {
ty::Dynamic(predicates, _, ty::Dyn) => {
// If the `dyn Trait` is not object safe, do not suggest `Box<dyn Trait>`.
predicates
.principal_def_id()
.map_or(true, |def_id| self.tcx.check_is_object_safe(def_id))
}
// We only want to suggest `impl Trait` to `dyn Trait`s.
// For example, `fn foo() -> str` needs to be filtered out.
_ => return false,
};

let hir::FnRetTy::Return(ret_ty) = sig.decl.output else {
let ty::Dynamic(_, _, ty::Dyn) = trait_pred.self_ty().skip_binder().kind() else {
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`.

// 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::default();
visitor.visit_body(&body);

let typeck_results = self.typeck_results.as_ref().unwrap();
let Some(liberated_sig) = typeck_results.liberated_fn_sigs().get(fn_hir_id).copied() else { return false; };

let ret_types = visitor
.returns
.iter()
.filter_map(|expr| Some((expr.span, typeck_results.node_type_opt(expr.hir_id)?)))
.map(|(expr_span, ty)| (expr_span, self.resolve_vars_if_possible(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 &=
!matches!(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 mut spans_and_needs_box = vec![];

match liberated_sig.output().kind() {
ty::Dynamic(predicates, _, ty::Dyn) => {
let cause = ObligationCause::misc(ret_ty.span, obligation.cause.body_id);
let param_env = ty::ParamEnv::empty();

if !only_never_return {
for (expr_span, return_ty) in ret_types {
let self_ty_satisfies_dyn_predicates = |self_ty| {
predicates.iter().all(|predicate| {
let pred = predicate.with_self_ty(self.tcx, self_ty);
let obl = Obligation::new(self.tcx, cause.clone(), param_env, pred);
self.predicate_may_hold(&obl)
})
};

if let ty::Adt(def, substs) = return_ty.kind()
&& def.is_box()
&& self_ty_satisfies_dyn_predicates(substs.type_at(0))
{
spans_and_needs_box.push((expr_span, false));
} else if self_ty_satisfies_dyn_predicates(return_ty) {
spans_and_needs_box.push((expr_span, true));
} else {
return false;
}
}
}
}
_ => return false,
};

let sm = self.tcx.sess.source_map();
if !ret_ty.span.overlaps(span) {
return false;
}
let snippet = if let hir::TyKind::TraitObject(..) = ret_ty.kind {
if let Ok(snippet) = sm.span_to_snippet(ret_ty.span) {
snippet
} else {
return false;
}
} else {
// Substitute the type, so we can print a fixup given `type Alias = dyn Trait`
let name = liberated_sig.output().to_string();
let name =
name.strip_prefix('(').and_then(|name| name.strip_suffix(')')).unwrap_or(&name);
if !name.starts_with("dyn ") {
return false;
}
name.to_owned()
};

err.code(error_code!(E0746));
err.set_primary_message("return type cannot have an unboxed trait object");
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 only_never_return {
// No return paths, probably using `panic!()` or similar.
// Suggest `-> impl Trait`, and if `Trait` is object safe, `-> Box<dyn Trait>`.
suggest_trait_object_return_type_alternatives(
err,
ret_ty.span,
trait_obj,
is_object_safe,
);
} else if let (Some(last_ty), true) = (last_ty, all_returns_have_same_type) {
// Suggest `-> impl Trait`.

let span = obligation.cause.span;
if let Ok(snip) = self.tcx.sess.source_map().span_to_snippet(span)
&& snip.starts_with("dyn ")
{
err.span_suggestion(
ret_ty.span,
format!(
"use `impl {1}` as the return type, as all return paths are of type `{}`, \
which implements `{1}`",
last_ty, trait_obj,
),
format!("impl {}", trait_obj),
Applicability::MachineApplicable,
span.with_hi(span.lo() + BytePos(4)),
"return an `impl Trait` instead of a `dyn Trait`, \
if all returned values are the same type",
"impl ",
Applicability::MaybeIncorrect,
);
err.note(impl_trait_msg);
} else {
if is_object_safe {
// Suggest `-> Box<dyn Trait>` and `Box::new(returned_value)`.
err.multipart_suggestion(
"return a boxed trait object instead",
vec![
(ret_ty.span.shrink_to_lo(), "Box<".to_string()),
(span.shrink_to_hi(), ">".to_string()),
],
Applicability::MaybeIncorrect,
);
for (span, needs_box) in spans_and_needs_box {
if needs_box {
err.multipart_suggestion(
"... and box this value",
vec![
(span.shrink_to_lo(), "Box::new(".to_string()),
(span.shrink_to_hi(), ")".to_string()),
],
Applicability::MaybeIncorrect,
);
}
}
}

let body = self.tcx.hir().body(self.tcx.hir().body_owned_by(obligation.cause.body_id));

let mut visitor = ReturnsVisitor::default();
visitor.visit_body(&body);

let mut sugg =
vec![(span.shrink_to_lo(), "Box<".to_string()), (span.shrink_to_hi(), ">".to_string())];
sugg.extend(visitor.returns.into_iter().flat_map(|expr| {
let span = expr.span.find_ancestor_in_same_ctxt(obligation.cause.span).unwrap_or(expr.span);
if !span.can_be_used_for_suggestions() {
vec![]
} else if let hir::ExprKind::Call(path, ..) = expr.kind
&& let hir::ExprKind::Path(hir::QPath::TypeRelative(ty, method)) = path.kind
&& method.ident.name == sym::new
&& let hir::TyKind::Path(hir::QPath::Resolved(.., box_path)) = ty.kind
&& box_path.res.opt_def_id().is_some_and(|def_id| Some(def_id) == self.tcx.lang_items().owned_box())
{
// Don't box `Box::new`
vec![]
} 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 `{}` were object-safe, you could return a trait object",
trait_obj,
));
vec![
(span.shrink_to_lo(), "Box::new(".to_string()),
(span.shrink_to_hi(), ")".to_string()),
]
}
err.note(trait_obj_msg);
err.note(format!(
"if all the returned values were of the same type you could use `impl {}` as the \
return type",
trait_obj,
));
err.note(impl_trait_msg);
err.note("you can create a new `enum` with a variant for each returned type");
}
}));

err.multipart_suggestion(
"box the return type, and wrap all of the returned values in `Box::new`",
sugg,
Applicability::MaybeIncorrect,
);

true
}

Expand Down Expand Up @@ -4139,37 +3989,6 @@ impl NextTypeParamName for &[hir::GenericParam<'_>] {
}
}

fn suggest_trait_object_return_type_alternatives(
err: &mut Diagnostic,
ret_ty: Span,
trait_obj: &str,
is_object_safe: bool,
) {
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.multipart_suggestion(
format!(
"use a boxed trait object if all return paths implement trait `{}`",
trait_obj,
),
vec![
(ret_ty.shrink_to_lo(), "Box<".to_string()),
(ret_ty.shrink_to_hi(), ">".to_string()),
],
Applicability::MaybeIncorrect,
);
}
}

/// Collect the spans that we see the generic param `param_did`
struct ReplaceImplTraitVisitor<'a> {
ty_spans: &'a mut Vec<Span>,
Expand Down
18 changes: 0 additions & 18 deletions tests/ui/error-codes/E0746.fixed
View file

This file was deleted.

2 changes: 1 addition & 1 deletion tests/ui/error-codes/E0746.rs
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
// run-rustfix
#![allow(dead_code)]

struct Struct;
trait Trait {}
impl Trait for Struct {}
Expand Down
Loading

0 comments on commit 4b26b80

Please sign in to comment.