Skip to content

Commit

Permalink
review comments
Browse files Browse the repository at this point in the history
  • Loading branch information
@estebank
estebank committed Jan 16, 2020
1 parent c305ac3 commit d7a6212
Show file tree
Hide file tree
Showing 8 changed files with 204 additions and 216 deletions.
32 changes: 0 additions & 32 deletions src/librustc/traits/error_reporting/mod.rs
View file
Expand Up @@ -25,7 +25,6 @@ use rustc_data_structures::fx::{FxHashMap, FxHashSet};
use rustc_errors::{struct_span_err, Applicability, DiagnosticBuilder};
use rustc_hir as hir;
use rustc_hir::def_id::{DefId, LOCAL_CRATE};
use rustc_hir::intravisit::Visitor;
use rustc_span::source_map::SourceMap;
use rustc_span::{ExpnKind, Span, DUMMY_SP};
use std::fmt;
Expand Down Expand Up @@ -1411,34 +1410,3 @@ pub fn suggest_constraining_type_param(
}
false
}

/// Collect all the returned expressions within the input expression.
/// Used to point at the return spans when we want to suggest some change to them.
struct ReturnsVisitor<'v>(Vec<&'v hir::Expr<'v>>);

impl<'v> Visitor<'v> for ReturnsVisitor<'v> {
type Map = rustc::hir::map::Map<'v>;

fn nested_visit_map(&mut self) -> hir::intravisit::NestedVisitorMap<'_, Self::Map> {
hir::intravisit::NestedVisitorMap::None
}

fn visit_expr(&mut self, ex: &'v hir::Expr<'v>) {
match ex.kind {
hir::ExprKind::Ret(Some(ex)) => self.0.push(ex),
_ => {}
}
hir::intravisit::walk_expr(self, ex);
}

fn visit_body(&mut self, body: &'v hir::Body<'v>) {
if body.generator_kind().is_none() {
if let hir::ExprKind::Block(block, None) = body.value.kind {
if let Some(expr) = block.expr {
self.0.push(expr);
}
}
}
hir::intravisit::walk_body(self, body);
}
}
270 changes: 137 additions & 133 deletions src/librustc/traits/error_reporting/suggestions.rs
View file
Expand Up @@ -563,157 +563,159 @@ impl<'a, 'tcx> InferCtxt<'a, 'tcx> {
let hir = self.tcx.hir();
let parent_node = hir.get_parent_node(obligation.cause.body_id);
let node = hir.find(parent_node);
if let Some(hir::Node::Item(hir::Item {
kind: hir::ItemKind::Fn(sig, _, body_id), ..
let (sig, body_id) = if let Some(hir::Node::Item(hir::Item {
kind: hir::ItemKind::Fn(sig, _, body_id),
..
})) = node
{
let body = hir.body(*body_id);
let trait_ref = self.resolve_vars_if_possible(trait_ref);
let ty = trait_ref.skip_binder().self_ty();
let is_object_safe;
match ty.kind {
ty::Dynamic(predicates, _) => {
// The `dyn Trait` is not object safe, do not suggest `Box<dyn Trait>`.
is_object_safe = predicates.principal_def_id().map_or(true, |def_id| {
!object_safety_violations(self.tcx, def_id).is_empty()
})
}
// We only want to suggest `impl Trait` to `dyn Trait`s.
// For example, `fn foo() -> str` needs to be filtered out.
_ => return false,
(sig, body_id)
} else {
return false;
};
let body = hir.body(*body_id);
let trait_ref = self.resolve_vars_if_possible(trait_ref);
let ty = trait_ref.skip_binder().self_ty();
let is_object_safe = match ty.kind {
ty::Dynamic(predicates, _) => {
// If the `dyn Trait` is not object safe, do not suggest `Box<dyn Trait>`.
predicates
.principal_def_id()
.map_or(true, |def_id| object_safety_violations(self.tcx, def_id).is_empty())
}
// We only want to suggest `impl Trait` to `dyn Trait`s.
// For example, `fn foo() -> str` needs to be filtered out.
_ => return false,
};

let ret_ty = if let hir::FunctionRetTy::Return(ret_ty) = sig.decl.output {
ret_ty
} 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(vec![]);
visitor.visit_body(&body);

let tables = self.in_progress_tables.map(|t| t.borrow()).unwrap();
let ret_ty = if let hir::FunctionRetTy::Return(ret_ty) = sig.decl.output {
ret_ty
} else {
return false;
};

let mut all_returns_conform_to_trait = true;
let mut all_returns_have_same_type = true;
let mut last_ty = None;
if let Some(ty_ret_ty) = tables.node_type_opt(ret_ty.hir_id) {
let cause = ObligationCause::misc(ret_ty.span, ret_ty.hir_id);
let param_env = ty::ParamEnv::empty();
if let ty::Dynamic(predicates, _) = &ty_ret_ty.kind {
for expr in &visitor.0 {
if let Some(returned_ty) = tables.node_type_opt(expr.hir_id) {
all_returns_have_same_type &=
Some(returned_ty) == last_ty || last_ty.is_none();
last_ty = Some(returned_ty);
for predicate in predicates.iter() {
let pred = predicate.with_self_ty(self.tcx, returned_ty);
let obl = Obligation::new(cause.clone(), param_env, pred);
all_returns_conform_to_trait &= self.predicate_may_hold(&obl);
}
// 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(vec![]);
visitor.visit_body(&body);

let tables = self.in_progress_tables.map(|t| t.borrow()).unwrap();

let mut all_returns_conform_to_trait = true;
let mut all_returns_have_same_type = true;
let mut last_ty = None;
if let Some(ty_ret_ty) = tables.node_type_opt(ret_ty.hir_id) {
let cause = ObligationCause::misc(ret_ty.span, ret_ty.hir_id);
let param_env = ty::ParamEnv::empty();
if let ty::Dynamic(predicates, _) = &ty_ret_ty.kind {
for expr in &visitor.0 {
if let Some(returned_ty) = tables.node_type_opt(expr.hir_id) {
all_returns_have_same_type &=
Some(returned_ty) == last_ty || last_ty.is_none();
last_ty = Some(returned_ty);
for predicate in predicates.iter() {
let pred = predicate.with_self_ty(self.tcx, returned_ty);
let obl = Obligation::new(cause.clone(), param_env, pred);
all_returns_conform_to_trait &= self.predicate_may_hold(&obl);
}
}
}
} else {
// We still want to verify whether all the return types conform to each other.
for expr in &visitor.0 {
let returned_ty = tables.node_type_opt(expr.hir_id);
all_returns_have_same_type &= last_ty == returned_ty || last_ty.is_none();
last_ty = returned_ty;
}
}
} else {
// We still want to verify whether all the return types conform to each other.
for expr in &visitor.0 {
let returned_ty = tables.node_type_opt(expr.hir_id);
all_returns_have_same_type &= last_ty == returned_ty || last_ty.is_none();
last_ty = returned_ty;
}
}

let (snippet, last_ty) =
if let (true, hir::TyKind::TraitObject(..), Ok(snippet), true, Some(last_ty)) = (
// Verify that we're dealing with a return `dyn Trait`
ret_ty.span.overlaps(span),
&ret_ty.kind,
self.tcx.sess.source_map().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.
all_returns_conform_to_trait,
last_ty,
) {
(snippet, last_ty)
} else {
return false;
};
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 all_returns_have_same_type {
// Suggest `-> impl Trait`.
err.span_suggestion(
let (snippet, last_ty) =
if let (true, hir::TyKind::TraitObject(..), Ok(snippet), true, Some(last_ty)) = (
// Verify that we're dealing with a return `dyn Trait`
ret_ty.span.overlaps(span),
&ret_ty.kind,
self.tcx.sess.source_map().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.
all_returns_conform_to_trait,
last_ty,
) {
(snippet, last_ty)
} else {
return false;
};
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 all_returns_have_same_type {
// Suggest `-> impl Trait`.
err.span_suggestion(
ret_ty.span,
&format!(
"return `impl {1}` instead, as all return paths are of type `{}`, \
which implements `{1}`",
last_ty, trait_obj,
),
format!("impl {}", trait_obj),
Applicability::MachineApplicable,
);
err.note(impl_trait_msg);
} else {
if is_object_safe {
// Suggest `-> Box<dyn Trait>` and `Box::new(returned_value)`.
// Get all the return values and collect their span and suggestion.
let mut suggestions = visitor
.0
.iter()
.map(|expr| {
(
expr.span,
format!(
"Box::new({})",
self.tcx.sess.source_map().span_to_snippet(expr.span).unwrap()
),
)
})
.collect::<Vec<_>>();
// Add the suggestion for the return type.
suggestions.push((
ret_ty.span,
&format!(
"return `impl {1}` instead, as all return paths are of type `{}`, \
which implements `{1}`",
last_ty, trait_obj,
),
format!("impl {}", trait_obj),
Applicability::MachineApplicable,
format!("Box<{}{}>", if has_dyn { "" } else { "dyn " }, snippet),
));
err.multipart_suggestion(
"return a trait object instead",
suggestions,
Applicability::MaybeIncorrect,
);
err.note(impl_trait_msg);
} else {
if is_object_safe {
// Suggest `-> Box<dyn Trait>` and `Box::new(returned_value)`.
// Get all the return values and collect their span and suggestion.
let mut suggestions = visitor
.0
.iter()
.map(|expr| {
(
expr.span,
format!(
"Box::new({})",
self.tcx.sess.source_map().span_to_snippet(expr.span).unwrap()
),
)
})
.collect::<Vec<_>>();
// Add the suggestion for the return type.
suggestions.push((
ret_ty.span,
format!("Box<{}{}>", if has_dyn { "" } else { "dyn " }, snippet),
));
err.multipart_suggestion(
"return a trait object instead",
suggestions,
Applicability::MaybeIncorrect,
);
} else {
err.note(&format!(
"if trait `{}` was object safe, you could return a trait object",
trait_obj,
));
}
err.note(&format!(
"if all the returned values were of the same type you could use \
`impl {}` as the return type",
"if trait `{}` was object safe, you could return a trait object",
trait_obj,
));
err.note(impl_trait_msg);
err.note(trait_obj_msg);
err.note("you can create a new `enum` with a variant for each returned type");
}
return true;
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");
}
false
true
}

crate fn point_at_returns_when_relevant(
Expand Down Expand Up @@ -1686,6 +1688,8 @@ pub fn suggest_constraining_type_param(
false
}

/// Collect all the returned expressions within the input expression.
/// Used to point at the return spans when we want to suggest some change to them.
struct ReturnsVisitor<'v>(Vec<&'v hir::Expr<'v>>);

impl<'v> Visitor<'v> for ReturnsVisitor<'v> {
Expand Down
2 changes: 1 addition & 1 deletion src/librustc/traits/mod.rs
View file
Expand Up @@ -1171,7 +1171,7 @@ impl<'tcx> ObligationCause<'tcx> {
}
}

impl<'tcx> ObligationCauseCode<'tcx> {
impl ObligationCauseCode<'_> {
// Return the base obligation, ignoring derived obligations.
pub fn peel_derives(&self) -> &Self {
let mut base_cause = self;
Expand Down
10 changes: 5 additions & 5 deletions src/librustc_error_codes/error_codes/E0746.md
View file
Expand Up @@ -12,8 +12,8 @@ impl T for S {
fn bar(&self) {}
}
// Having the trait `T` as return type is invalid because bare traits do not
// have a statically known size:
// Having the trait `T` as return type is invalid because
// bare trait objects do not have a statically known size:
fn foo() -> dyn T {
S(42)
}
Expand All @@ -32,15 +32,15 @@ If there is a single type involved, you can use [`impl Trait`]:
# fn bar(&self) {}
# }
// The compiler will select `S(usize)` as the materialized return type of this
// function, but callers will only be able to access associated items from `T`.
// function, but callers will only know that the return type implements `T`.
fn foo() -> impl T {
S(42)
}
```

If there are multiple types involved, the only way you care to interact with
them is through the trait's interface and having to rely on dynamic dispatch is
acceptable, then you can use [trait objects] with `Box`, or other container
them is through the trait's interface, and having to rely on dynamic dispatch
is acceptable, then you can use [trait objects] with `Box`, or other container
types like `Rc` or `Arc`:

```
Expand Down

0 comments on commit d7a6212

Please sign in to comment.