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.

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

Further improve impl Trait/dyn Trait suggestions #68522

Merged
merged 6 commits into from
Jan 26, 2020
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
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
71 changes: 54 additions & 17 deletions src/librustc/traits/error_reporting/suggestions.rs
Original file line number Diff line number Diff line change
Expand Up @@ -600,17 +600,24 @@ impl<'a, 'tcx> InferCtxt<'a, 'tcx> {

// 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![]);
let mut visitor = ReturnsVisitor::new();
visitor.visit_body(&body);

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

let mut ret_types = visitor.0.iter().filter_map(|expr| tables.node_type_opt(expr.hir_id));
let (last_ty, all_returns_have_same_type) =
ret_types.clone().fold((None, true), |(last_ty, mut same), returned_ty| {
same &= last_ty.map_or(true, |ty| ty == returned_ty);
(Some(returned_ty), same)
});
let mut ret_types = visitor
.returns
.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) = ret_types.clone().fold(
(None, true),
|(last_ty, mut same): (std::option::Option<Ty<'_>>, 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)
},
);
let all_returns_conform_to_trait =
if let Some(ty_ret_ty) = tables.node_type_opt(ret_ty.hir_id) {
match ty_ret_ty.kind {
Expand All @@ -625,7 +632,7 @@ impl<'a, 'tcx> InferCtxt<'a, 'tcx> {
})
})
}
_ => true,
_ => false,
}
} else {
true
Expand Down Expand Up @@ -675,7 +682,7 @@ impl<'a, 'tcx> InferCtxt<'a, 'tcx> {
// 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
.returns
.iter()
.map(|expr| {
(
Expand Down Expand Up @@ -735,10 +742,10 @@ impl<'a, 'tcx> InferCtxt<'a, 'tcx> {
{
let body = hir.body(*body_id);
// Point at all the `return`s in the function as they have failed trait bounds.
let mut visitor = ReturnsVisitor(vec![]);
let mut visitor = ReturnsVisitor::new();
visitor.visit_body(&body);
let tables = self.in_progress_tables.map(|t| t.borrow()).unwrap();
for expr in &visitor.0 {
for expr in &visitor.returns {
if let Some(returned_ty) = tables.node_type_opt(expr.hir_id) {
let ty = self.resolve_vars_if_possible(&returned_ty);
err.span_label(expr.span, &format!("this returned value is of type `{}`", ty));
Expand Down Expand Up @@ -1689,7 +1696,16 @@ pub fn suggest_constraining_type_param(

/// 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>>);
struct ReturnsVisitor<'v> {
estebank marked this conversation as resolved.
Show resolved Hide resolved
returns: Vec<&'v hir::Expr<'v>>,
in_block_tail: bool,
}

impl ReturnsVisitor<'_> {
fn new() -> Self {
ReturnsVisitor { returns: vec![], in_block_tail: false }
}
}

impl<'v> Visitor<'v> for ReturnsVisitor<'v> {
type Map = rustc::hir::map::Map<'v>;
Expand All @@ -1699,20 +1715,41 @@ impl<'v> Visitor<'v> for ReturnsVisitor<'v> {
}

fn visit_expr(&mut self, ex: &'v hir::Expr<'v>) {
if let hir::ExprKind::Ret(Some(ex)) = ex.kind {
self.0.push(ex);
match ex.kind {
estebank marked this conversation as resolved.
Show resolved Hide resolved
hir::ExprKind::Ret(Some(ex)) => {
self.returns.push(ex);
}
hir::ExprKind::Block(block, _) if self.in_block_tail => {
self.in_block_tail = false;
for stmt in block.stmts {
hir::intravisit::walk_stmt(self, stmt);
}
self.in_block_tail = true;
if let Some(expr) = block.expr {
self.visit_expr(expr);
}
}
hir::ExprKind::Match(_, arms, _) if self.in_block_tail => {
for arm in arms {
self.visit_expr(arm.body);
}
}
// We need to walk to find `return`s in the entire body.
_ if !self.in_block_tail => hir::intravisit::walk_expr(self, ex),
_ => self.returns.push(ex),
}
hir::intravisit::walk_expr(self, ex);
}

fn visit_body(&mut self, body: &'v hir::Body<'v>) {
let prev = self.in_block_tail;
estebank marked this conversation as resolved.
Show resolved Hide resolved
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);
if block.expr.is_some() {
self.in_block_tail = true;
}
}
}
hir::intravisit::walk_body(self, body);
self.in_block_tail = prev;
estebank marked this conversation as resolved.
Show resolved Hide resolved
}
}
11 changes: 8 additions & 3 deletions src/librustc_errors/emitter.rs
Original file line number Diff line number Diff line change
Expand Up @@ -456,9 +456,14 @@ impl Emitter for SilentEmitter {
fn emit_diagnostic(&mut self, _: &Diagnostic) {}
}

/// maximum number of lines we will print for each error; arbitrary.
/// Maximum number of lines we will print for each error; arbitrary.
pub const MAX_HIGHLIGHT_LINES: usize = 6;
/// maximum number of suggestions to be shown
/// Maximum number of lines we will print for a multiline suggestion; arbitrary.
///
/// This should be replaced with a more involved mechanism to output multiline suggestions that
/// more closely mimmics the regular diagnostic output, where irrelevant code lines are ellided.
estebank marked this conversation as resolved.
Show resolved Hide resolved
pub const MAX_SUGGESTION_HIGHLIGHT_LINES: usize = 20;
/// Maximum number of suggestions to be shown
///
/// Arbitrary, but taken from trait import suggestion limit
pub const MAX_SUGGESTIONS: usize = 4;
Expand Down Expand Up @@ -1521,7 +1526,7 @@ impl EmitterWriter {
draw_col_separator_no_space(&mut buffer, 1, max_line_num_len + 1);
let mut line_pos = 0;
let mut lines = complete.lines();
for line in lines.by_ref().take(MAX_HIGHLIGHT_LINES) {
for line in lines.by_ref().take(MAX_SUGGESTION_HIGHLIGHT_LINES) {
estebank marked this conversation as resolved.
Show resolved Hide resolved
// Print the span column to avoid confusion
buffer.puts(
row_num,
Expand Down
4 changes: 2 additions & 2 deletions src/test/ui/error-codes/E0746.fixed
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,9 @@ fn foo() -> impl Trait { Struct }

fn bar() -> impl Trait { //~ ERROR E0746
if true {
return 0;
return 0u32;
}
42
42u32
}

fn main() {}
4 changes: 2 additions & 2 deletions src/test/ui/error-codes/E0746.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,9 @@ fn foo() -> dyn Trait { Struct }

fn bar() -> dyn Trait { //~ ERROR E0746
if true {
return 0;
return 0u32;
estebank marked this conversation as resolved.
Show resolved Hide resolved
}
42
42u32
}

fn main() {}
2 changes: 1 addition & 1 deletion src/test/ui/error-codes/E0746.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ LL | fn bar() -> dyn Trait {
| ^^^^^^^^^ doesn't have a size known at compile-time
|
= note: for information on `impl Trait`, see <https://doc.rust-lang.org/book/ch10-02-traits.html#returning-types-that-implement-traits>
help: return `impl Trait` instead, as all return paths are of type `{integer}`, which implements `Trait`
help: return `impl Trait` instead, as all return paths are of type `u32`, which implements `Trait`
|
LL | fn bar() -> impl Trait {
| ^^^^^^^^^^
Expand Down
40 changes: 40 additions & 0 deletions src/test/ui/impl-trait/dyn-trait-return-should-be-impl-trait.rs
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,39 @@ fn bal() -> dyn Trait { //~ ERROR E0746
}
42
}
fn bax() -> dyn Trait { //~ ERROR E0746
if true {
Struct
} else {
42
}
}
fn bam() -> Box<dyn Trait> {
if true {
return Struct; //~ ERROR mismatched types
}
42 //~ ERROR mismatched types
}
fn baq() -> Box<dyn Trait> {
if true {
return 0; //~ ERROR mismatched types
}
42 //~ ERROR mismatched types
}
fn baz() -> Box<dyn Trait> {
if true {
Struct //~ ERROR mismatched types
} else {
42 //~ ERROR mismatched types
}
}
fn baw() -> Box<dyn Trait> {
if true {
0 //~ ERROR mismatched types
} else {
42 //~ ERROR mismatched types
}
}

// Suggest using `impl Trait`
fn bat() -> dyn Trait { //~ ERROR E0746
Expand All @@ -30,5 +63,12 @@ fn bat() -> dyn Trait { //~ ERROR E0746
}
42
}
fn bay() -> dyn Trait { //~ ERROR E0746
if true {
0
} else {
42
}
}

fn main() {}
Loading