Auto merge of #68522 - estebank:impl-trait-sugg-2, r=oli-obk

Further improve `impl Trait`/`dyn Trait` suggestions

After reading [_Returning Trait Objects_ by Bryce Fisher-Fleig](https://bryce.fisher-fleig.org/blog/returning-trait-objects/), [I noticed that](https://www.reddit.com/r/rust/comments/esueur/returning_trait_objects/ffczl4k/) #68195 had a few bugs due to not ignoring `ty::Error`.

- Account for `ty::Error`.
- Account for `if`/`else` and `match` blocks when pointing at return types and referencing their types.
- Increase the multiline suggestion output from 6 lines to 20.
This commit is contained in:
bors 2020-01-26 08:36:23 +00:00
commit 3d8778d767
5 changed files with 262 additions and 23 deletions

View File

@ -601,17 +601,24 @@ impl<'a, 'tcx> InferCtxt<'a, 'tcx> {
// Visit to make sure there's a single `return` type 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. // otherwise suggest using `Box<dyn Trait>` or an enum.
let mut visitor = ReturnsVisitor(vec![]); let mut visitor = ReturnsVisitor::default();
visitor.visit_body(&body); visitor.visit_body(&body);
let tables = self.in_progress_tables.map(|t| t.borrow()).unwrap(); 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 mut ret_types = visitor
let (last_ty, all_returns_have_same_type) = .returns
ret_types.clone().fold((None, true), |(last_ty, mut same), returned_ty| { .iter()
same &= last_ty.map_or(true, |ty| ty == returned_ty); .filter_map(|expr| tables.node_type_opt(expr.hir_id))
(Some(returned_ty), same) .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 = let all_returns_conform_to_trait =
if let Some(ty_ret_ty) = tables.node_type_opt(ret_ty.hir_id) { if let Some(ty_ret_ty) = tables.node_type_opt(ret_ty.hir_id) {
match ty_ret_ty.kind { match ty_ret_ty.kind {
@ -626,7 +633,7 @@ impl<'a, 'tcx> InferCtxt<'a, 'tcx> {
}) })
}) })
} }
_ => true, _ => false,
} }
} else { } else {
true true
@ -676,7 +683,7 @@ impl<'a, 'tcx> InferCtxt<'a, 'tcx> {
// Suggest `-> Box<dyn Trait>` and `Box::new(returned_value)`. // Suggest `-> Box<dyn Trait>` and `Box::new(returned_value)`.
// Get all the return values and collect their span and suggestion. // Get all the return values and collect their span and suggestion.
let mut suggestions = visitor let mut suggestions = visitor
.0 .returns
.iter() .iter()
.map(|expr| { .map(|expr| {
( (
@ -736,10 +743,10 @@ impl<'a, 'tcx> InferCtxt<'a, 'tcx> {
{ {
let body = hir.body(*body_id); let body = hir.body(*body_id);
// Point at all the `return`s in the function as they have failed trait bounds. // Point at all the `return`s in the function as they have failed trait bounds.
let mut visitor = ReturnsVisitor(vec![]); let mut visitor = ReturnsVisitor::default();
visitor.visit_body(&body); visitor.visit_body(&body);
let tables = self.in_progress_tables.map(|t| t.borrow()).unwrap(); 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) { if let Some(returned_ty) = tables.node_type_opt(expr.hir_id) {
let ty = self.resolve_vars_if_possible(&returned_ty); let ty = self.resolve_vars_if_possible(&returned_ty);
err.span_label(expr.span, &format!("this returned value is of type `{}`", ty)); err.span_label(expr.span, &format!("this returned value is of type `{}`", ty));
@ -1716,7 +1723,11 @@ pub fn suggest_constraining_type_param(
/// Collect all the returned expressions within the input expression. /// 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. /// Used to point at the return spans when we want to suggest some change to them.
struct ReturnsVisitor<'v>(Vec<&'v hir::Expr<'v>>); #[derive(Default)]
struct ReturnsVisitor<'v> {
returns: Vec<&'v hir::Expr<'v>>,
in_block_tail: bool,
}
impl<'v> Visitor<'v> for ReturnsVisitor<'v> { impl<'v> Visitor<'v> for ReturnsVisitor<'v> {
type Map = rustc::hir::map::Map<'v>; type Map = rustc::hir::map::Map<'v>;
@ -1726,17 +1737,41 @@ impl<'v> Visitor<'v> for ReturnsVisitor<'v> {
} }
fn visit_expr(&mut self, ex: &'v hir::Expr<'v>) { fn visit_expr(&mut self, ex: &'v hir::Expr<'v>) {
if let hir::ExprKind::Ret(Some(ex)) = ex.kind { // Visit every expression to detect `return` paths, either through the function's tail
self.0.push(ex); // expression or `return` statements. We walk all nodes to find `return` statements, but
// we only care about tail expressions when `in_block_tail` is `true`, which means that
// they're in the return path of the function body.
match ex.kind {
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>) { fn visit_body(&mut self, body: &'v hir::Body<'v>) {
assert!(!self.in_block_tail);
if body.generator_kind().is_none() { if body.generator_kind().is_none() {
if let hir::ExprKind::Block(block, None) = body.value.kind { if let hir::ExprKind::Block(block, None) = body.value.kind {
if let Some(expr) = block.expr { if block.expr.is_some() {
self.0.push(expr); self.in_block_tail = true;
} }
} }
} }

View File

@ -456,9 +456,14 @@ impl Emitter for SilentEmitter {
fn emit_diagnostic(&mut self, _: &Diagnostic) {} 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; 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 elided.
pub const MAX_SUGGESTION_HIGHLIGHT_LINES: usize = 6;
/// Maximum number of suggestions to be shown
/// ///
/// Arbitrary, but taken from trait import suggestion limit /// Arbitrary, but taken from trait import suggestion limit
pub const MAX_SUGGESTIONS: usize = 4; pub const MAX_SUGGESTIONS: usize = 4;
@ -1521,7 +1526,7 @@ impl EmitterWriter {
draw_col_separator_no_space(&mut buffer, 1, max_line_num_len + 1); draw_col_separator_no_space(&mut buffer, 1, max_line_num_len + 1);
let mut line_pos = 0; let mut line_pos = 0;
let mut lines = complete.lines(); 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) {
// Print the span column to avoid confusion // Print the span column to avoid confusion
buffer.puts( buffer.puts(
row_num, row_num,

View File

@ -22,6 +22,39 @@ fn bal() -> dyn Trait { //~ ERROR E0746
} }
42 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` // Suggest using `impl Trait`
fn bat() -> dyn Trait { //~ ERROR E0746 fn bat() -> dyn Trait { //~ ERROR E0746
@ -30,5 +63,12 @@ fn bat() -> dyn Trait { //~ ERROR E0746
} }
42 42
} }
fn bay() -> dyn Trait { //~ ERROR E0746
if true {
0
} else {
42
}
}
fn main() {} fn main() {}

View File

@ -96,7 +96,154 @@ LL | Box::new(42)
| |
error[E0746]: return type cannot have an unboxed trait object error[E0746]: return type cannot have an unboxed trait object
--> $DIR/dyn-trait-return-should-be-impl-trait.rs:27:13 --> $DIR/dyn-trait-return-should-be-impl-trait.rs:25:13
|
LL | fn bax() -> dyn Trait {
| ^^^^^^^^^ doesn't have a size known at compile-time
|
= note: 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>
= note: if all the returned values were of the same type you could use `impl Trait` as the return type
= note: for information on `impl Trait`, see <https://doc.rust-lang.org/book/ch10-02-traits.html#returning-types-that-implement-traits>
= note: you can create a new `enum` with a variant for each returned type
help: return a boxed trait object instead
|
LL | fn bax() -> Box<dyn Trait> {
LL | if true {
LL | Box::new(Struct)
LL | } else {
LL | Box::new(42)
|
error[E0308]: mismatched types
--> $DIR/dyn-trait-return-should-be-impl-trait.rs:34:16
|
LL | fn bam() -> Box<dyn Trait> {
| -------------- expected `std::boxed::Box<(dyn Trait + 'static)>` because of return type
LL | if true {
LL | return Struct;
| ^^^^^^
| |
| expected struct `std::boxed::Box`, found struct `Struct`
| help: store this in the heap by calling `Box::new`: `Box::new(Struct)`
|
= note: expected struct `std::boxed::Box<(dyn Trait + 'static)>`
found struct `Struct`
= note: for more on the distinction between the stack and the heap, read https://doc.rust-lang.org/book/ch15-01-box.html, https://doc.rust-lang.org/rust-by-example/std/box.html, and https://doc.rust-lang.org/std/boxed/index.html
error[E0308]: mismatched types
--> $DIR/dyn-trait-return-should-be-impl-trait.rs:36:5
|
LL | fn bam() -> Box<dyn Trait> {
| -------------- expected `std::boxed::Box<(dyn Trait + 'static)>` because of return type
...
LL | 42
| ^^
| |
| expected struct `std::boxed::Box`, found integer
| help: store this in the heap by calling `Box::new`: `Box::new(42)`
|
= note: expected struct `std::boxed::Box<(dyn Trait + 'static)>`
found type `{integer}`
= note: for more on the distinction between the stack and the heap, read https://doc.rust-lang.org/book/ch15-01-box.html, https://doc.rust-lang.org/rust-by-example/std/box.html, and https://doc.rust-lang.org/std/boxed/index.html
error[E0308]: mismatched types
--> $DIR/dyn-trait-return-should-be-impl-trait.rs:40:16
|
LL | fn baq() -> Box<dyn Trait> {
| -------------- expected `std::boxed::Box<(dyn Trait + 'static)>` because of return type
LL | if true {
LL | return 0;
| ^
| |
| expected struct `std::boxed::Box`, found integer
| help: store this in the heap by calling `Box::new`: `Box::new(0)`
|
= note: expected struct `std::boxed::Box<(dyn Trait + 'static)>`
found type `{integer}`
= note: for more on the distinction between the stack and the heap, read https://doc.rust-lang.org/book/ch15-01-box.html, https://doc.rust-lang.org/rust-by-example/std/box.html, and https://doc.rust-lang.org/std/boxed/index.html
error[E0308]: mismatched types
--> $DIR/dyn-trait-return-should-be-impl-trait.rs:42:5
|
LL | fn baq() -> Box<dyn Trait> {
| -------------- expected `std::boxed::Box<(dyn Trait + 'static)>` because of return type
...
LL | 42
| ^^
| |
| expected struct `std::boxed::Box`, found integer
| help: store this in the heap by calling `Box::new`: `Box::new(42)`
|
= note: expected struct `std::boxed::Box<(dyn Trait + 'static)>`
found type `{integer}`
= note: for more on the distinction between the stack and the heap, read https://doc.rust-lang.org/book/ch15-01-box.html, https://doc.rust-lang.org/rust-by-example/std/box.html, and https://doc.rust-lang.org/std/boxed/index.html
error[E0308]: mismatched types
--> $DIR/dyn-trait-return-should-be-impl-trait.rs:46:9
|
LL | fn baz() -> Box<dyn Trait> {
| -------------- expected `std::boxed::Box<(dyn Trait + 'static)>` because of return type
LL | if true {
LL | Struct
| ^^^^^^
| |
| expected struct `std::boxed::Box`, found struct `Struct`
| help: store this in the heap by calling `Box::new`: `Box::new(Struct)`
|
= note: expected struct `std::boxed::Box<(dyn Trait + 'static)>`
found struct `Struct`
= note: for more on the distinction between the stack and the heap, read https://doc.rust-lang.org/book/ch15-01-box.html, https://doc.rust-lang.org/rust-by-example/std/box.html, and https://doc.rust-lang.org/std/boxed/index.html
error[E0308]: mismatched types
--> $DIR/dyn-trait-return-should-be-impl-trait.rs:48:9
|
LL | fn baz() -> Box<dyn Trait> {
| -------------- expected `std::boxed::Box<(dyn Trait + 'static)>` because of return type
...
LL | 42
| ^^
| |
| expected struct `std::boxed::Box`, found integer
| help: store this in the heap by calling `Box::new`: `Box::new(42)`
|
= note: expected struct `std::boxed::Box<(dyn Trait + 'static)>`
found type `{integer}`
= note: for more on the distinction between the stack and the heap, read https://doc.rust-lang.org/book/ch15-01-box.html, https://doc.rust-lang.org/rust-by-example/std/box.html, and https://doc.rust-lang.org/std/boxed/index.html
error[E0308]: mismatched types
--> $DIR/dyn-trait-return-should-be-impl-trait.rs:53:9
|
LL | fn baw() -> Box<dyn Trait> {
| -------------- expected `std::boxed::Box<(dyn Trait + 'static)>` because of return type
LL | if true {
LL | 0
| ^
| |
| expected struct `std::boxed::Box`, found integer
| help: store this in the heap by calling `Box::new`: `Box::new(0)`
|
= note: expected struct `std::boxed::Box<(dyn Trait + 'static)>`
found type `{integer}`
= note: for more on the distinction between the stack and the heap, read https://doc.rust-lang.org/book/ch15-01-box.html, https://doc.rust-lang.org/rust-by-example/std/box.html, and https://doc.rust-lang.org/std/boxed/index.html
error[E0308]: mismatched types
--> $DIR/dyn-trait-return-should-be-impl-trait.rs:55:9
|
LL | fn baw() -> Box<dyn Trait> {
| -------------- expected `std::boxed::Box<(dyn Trait + 'static)>` because of return type
...
LL | 42
| ^^
| |
| expected struct `std::boxed::Box`, found integer
| help: store this in the heap by calling `Box::new`: `Box::new(42)`
|
= note: expected struct `std::boxed::Box<(dyn Trait + 'static)>`
found type `{integer}`
= note: for more on the distinction between the stack and the heap, read https://doc.rust-lang.org/book/ch15-01-box.html, https://doc.rust-lang.org/rust-by-example/std/box.html, and https://doc.rust-lang.org/std/boxed/index.html
error[E0746]: return type cannot have an unboxed trait object
--> $DIR/dyn-trait-return-should-be-impl-trait.rs:60:13
| |
LL | fn bat() -> dyn Trait { LL | fn bat() -> dyn Trait {
| ^^^^^^^^^ doesn't have a size known at compile-time | ^^^^^^^^^ doesn't have a size known at compile-time
@ -107,7 +254,19 @@ help: return `impl Trait` instead, as all return paths are of type `{integer}`,
LL | fn bat() -> impl Trait { LL | fn bat() -> impl Trait {
| ^^^^^^^^^^ | ^^^^^^^^^^
error: aborting due to 9 previous errors error[E0746]: return type cannot have an unboxed trait object
--> $DIR/dyn-trait-return-should-be-impl-trait.rs:66:13
|
LL | fn bay() -> 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`
|
LL | fn bay() -> impl Trait {
| ^^^^^^^^^^
error: aborting due to 19 previous errors
Some errors have detailed explanations: E0277, E0308, E0746. Some errors have detailed explanations: E0277, E0308, E0746.
For more information about an error, try `rustc --explain E0277`. For more information about an error, try `rustc --explain E0277`.

View File

@ -5,7 +5,7 @@ LL | fn should_ret_unit() -> impl T {
| ^^^^^^ the trait `T` is not implemented for `()` | ^^^^^^ the trait `T` is not implemented for `()`
LL | LL |
LL | panic!() LL | panic!()
| -------- this returned value is of type `()` | -------- this returned value is of type `!`
| |
= note: the return type of a function must have a statically known size = note: the return type of a function must have a statically known size
= note: this error originates in a macro outside of the current crate (in Nightly builds, run with -Z external-macro-backtrace for more info) = note: this error originates in a macro outside of the current crate (in Nightly builds, run with -Z external-macro-backtrace for more info)