Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove return type sized check hack from hir typeck #111704

Merged
merged 2 commits into from
May 23, 2023
Merged

Remove return type sized check hack from hir typeck #111704

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
795fdf7
Simplify suggestion when returning bare dyn trait
compiler-errors May 18, 2023
3db2bcf
Remove return type sized check hack from hir typeck
compiler-errors May 18, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 2 additions & 18 deletions compiler/rustc_hir_typeck/src/check.rs
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop

This file was deleted.

2 changes: 1 addition & 1 deletion tests/ui/error-codes/E0746.rs
View file
Open in desktop
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