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.

5 files changed, 262 insertions, 23 deletions

diff --git a/src/librustc/traits/error_reporting/suggestions.rs b/src/librustc/traits/error_reporting/suggestions.rs
index aa5e9f65d2b..72629c6a3cf 100644
--- a/src/librustc/traits/error_reporting/suggestions.rs
+++ b/src/librustc/traits/error_reporting/suggestions.rs
@@ -601,17 +601,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::default();
         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 {
@@ -626,7 +633,7 @@ impl<'a, 'tcx> InferCtxt<'a, 'tcx> {
                             })
                         })
                     }
-                    _ => true,
+                    _ => false,
                 }
             } else {
                 true
@@ -676,7 +683,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| {
                         (
@@ -736,10 +743,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::default();
             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));
@@ -1716,7 +1723,11 @@ 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>>);
+#[derive(Default)]
+struct ReturnsVisitor<'v> {
+    returns: Vec<&'v hir::Expr<'v>>,
+    in_block_tail: bool,
+}
 
 impl<'v> Visitor<'v> for ReturnsVisitor<'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>) {
-        if let hir::ExprKind::Ret(Some(ex)) = ex.kind {
-            self.0.push(ex);
+        // Visit every expression to detect `return` paths, either through the function's tail
+        // 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>) {
+        assert!(!self.in_block_tail);
         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;
                 }
             }
         }
diff --git a/src/librustc_errors/emitter.rs b/src/librustc_errors/emitter.rs
index b0e0cb611af..7218730538a 100644
--- a/src/librustc_errors/emitter.rs
+++ b/src/librustc_errors/emitter.rs
@@ -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 elided.
+pub const MAX_SUGGESTION_HIGHLIGHT_LINES: usize = 6;
+/// Maximum number of suggestions to be shown
 ///
 /// Arbitrary, but taken from trait import suggestion limit
 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);
             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) {
                 // Print the span column to avoid confusion
                 buffer.puts(
                     row_num,
diff --git a/src/test/ui/impl-trait/dyn-trait-return-should-be-impl-trait.rs b/src/test/ui/impl-trait/dyn-trait-return-should-be-impl-trait.rs
index b70a51dc825..08bab5734fd 100644
--- a/src/test/ui/impl-trait/dyn-trait-return-should-be-impl-trait.rs
+++ b/src/test/ui/impl-trait/dyn-trait-return-should-be-impl-trait.rs
@@ -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
@@ -30,5 +63,12 @@ fn bat() -> dyn Trait { //~ ERROR E0746
     }
     42
 }
+fn bay() -> dyn Trait { //~ ERROR E0746
+    if true {
+        0
+    } else {
+        42
+    }
+}
 
 fn main() {}
diff --git a/src/test/ui/impl-trait/dyn-trait-return-should-be-impl-trait.stderr b/src/test/ui/impl-trait/dyn-trait-return-should-be-impl-trait.stderr
index 977a7ef0e02..664a897c593 100644
--- a/src/test/ui/impl-trait/dyn-trait-return-should-be-impl-trait.stderr
+++ b/src/test/ui/impl-trait/dyn-trait-return-should-be-impl-trait.stderr
@@ -96,7 +96,154 @@ LL |     Box::new(42)
    |
 
 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 {
    |             ^^^^^^^^^ 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 {
    |             ^^^^^^^^^^
 
-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.
 For more information about an error, try `rustc --explain E0277`.
diff --git a/src/test/ui/never_type/feature-gate-never_type_fallback.stderr b/src/test/ui/never_type/feature-gate-never_type_fallback.stderr
index 77288f1bada..08e16f46936 100644
--- a/src/test/ui/never_type/feature-gate-never_type_fallback.stderr
+++ b/src/test/ui/never_type/feature-gate-never_type_fallback.stderr
@@ -5,7 +5,7 @@ LL | fn should_ret_unit() -> impl T {
    |                         ^^^^^^ the trait `T` is not implemented for `()`
 LL |
 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: this error originates in a macro outside of the current crate (in Nightly builds, run with -Z external-macro-backtrace for more info)