Rollup merge of rust-lang#98784 - compiler-errors:forgot-to-return-binding, r=estebank

Suggest returning local on "expected `ty`, found `()`" due to expr-less block

Putting this up for _initial_ review. Notably, this doesn't consider if the value has possibly been moved, or whether the type is `Copy`. It also provides a structured suggestion if there's one "preferred" binding that matches the type (i.e. one binding in the block or its parent), otherwise it just points them out if there's fewer than 4 of them.

Fixes rust-lang#98177

r? `@estebank`

Author: Dylan-DPC

compiler/rustc_typeck/src/check/fn_ctxt/checks.rs

@@ -31,9 +31,7 @@ use rustc_middle::ty::{self, DefIdTree, IsSuggestable, Ty};
 use rustc_session::Session;
 use rustc_span::symbol::Ident;
 use rustc_span::{self, Span};
-use rustc_trait_selection::traits::{
-    self, ObligationCauseCode, SelectionContext, StatementAsExpression,
-};
+use rustc_trait_selection::traits::{self, ObligationCauseCode, SelectionContext};
 
 use std::iter;
 use std::slice;
@@ -1410,7 +1408,9 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
                         &self.misc(sp),
                         &mut |err| {
                             if let Some(expected_ty) = expected.only_has_type(self) {
-                                self.consider_hint_about_removing_semicolon(blk, expected_ty, err);
+                                if !self.consider_removing_semicolon(blk, expected_ty, err) {
+                                    self.consider_returning_binding(blk, expected_ty, err);
+                                }
                                 if expected_ty == self.tcx.types.bool {
                                     // If this is caused by a missing `let` in a `while let`,
                                     // silence this redundant error, as we already emit E0070.
@@ -1478,42 +1478,6 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
         ty
     }
 
-    /// A common error is to add an extra semicolon:
-    ///
-    /// ```compile_fail,E0308
-    /// fn foo() -> usize {
-    ///     22;
-    /// }
-    /// ```
-    ///
-    /// This routine checks if the final statement in a block is an
-    /// expression with an explicit semicolon whose type is compatible
-    /// with `expected_ty`. If so, it suggests removing the semicolon.
-    fn consider_hint_about_removing_semicolon(
-        &self,
-        blk: &'tcx hir::Block<'tcx>,
-        expected_ty: Ty<'tcx>,
-        err: &mut Diagnostic,
-    ) {
-        if let Some((span_semi, boxed)) = self.could_remove_semicolon(blk, expected_ty) {
-            if let StatementAsExpression::NeedsBoxing = boxed {
-                err.span_suggestion_verbose(
-                    span_semi,
-                    "consider removing this semicolon and boxing the expression",
-                    "",
-                    Applicability::HasPlaceholders,
-                );
-            } else {
-                err.span_suggestion_short(
-                    span_semi,
-                    "remove this semicolon",
-                    "",
-                    Applicability::MachineApplicable,
-                );
-            }
-        }
-    }
-
     fn parent_item_span(&self, id: hir::HirId) -> Option<Span> {
         let node = self.tcx.hir().get_by_def_id(self.tcx.hir().get_parent_item(id));
         match node {

compiler/rustc_typeck/src/check/fn_ctxt/suggestions.rs

@@ -3,6 +3,7 @@ use crate::astconv::AstConv;
 use crate::errors::{AddReturnTypeSuggestion, ExpectedReturnTypeLabel};
 
 use rustc_ast::util::parser::ExprPrecedence;
+use rustc_data_structures::stable_set::FxHashSet;
 use rustc_errors::{Applicability, Diagnostic, MultiSpan};
 use rustc_hir as hir;
 use rustc_hir::def::{CtorOf, DefKind};
@@ -11,12 +12,12 @@ use rustc_hir::{
     Expr, ExprKind, GenericBound, Node, Path, QPath, Stmt, StmtKind, TyKind, WherePredicate,
 };
 use rustc_infer::infer::{self, TyCtxtInferExt};
-use rustc_infer::traits;
+use rustc_infer::traits::{self, StatementAsExpression};
 use rustc_middle::lint::in_external_macro;
-use rustc_middle::ty::{self, Binder, IsSuggestable, Subst, ToPredicate, Ty};
+use rustc_middle::ty::{self, Binder, IsSuggestable, Subst, ToPredicate, Ty, TypeVisitable};
 use rustc_span::symbol::sym;
 use rustc_span::Span;
-use rustc_trait_selection::traits::query::evaluate_obligation::InferCtxtExt;
+use rustc_trait_selection::traits::query::evaluate_obligation::InferCtxtExt as _;
 
 impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
     pub(in super::super) fn suggest_semicolon_at_end(&self, span: Span, err: &mut Diagnostic) {
@@ -864,4 +865,156 @@ impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
             );
         }
     }
+
+    /// A common error is to add an extra semicolon:
+    ///
+    /// ```compile_fail,E0308
+    /// fn foo() -> usize {
+    ///     22;
+    /// }
+    /// ```
+    ///
+    /// This routine checks if the final statement in a block is an
+    /// expression with an explicit semicolon whose type is compatible
+    /// with `expected_ty`. If so, it suggests removing the semicolon.
+    pub(crate) fn consider_removing_semicolon(
+        &self,
+        blk: &'tcx hir::Block<'tcx>,
+        expected_ty: Ty<'tcx>,
+        err: &mut Diagnostic,
+    ) -> bool {
+        if let Some((span_semi, boxed)) = self.could_remove_semicolon(blk, expected_ty) {
+            if let StatementAsExpression::NeedsBoxing = boxed {
+                err.span_suggestion_verbose(
+                    span_semi,
+                    "consider removing this semicolon and boxing the expression",
+                    "",
+                    Applicability::HasPlaceholders,
+                );
+            } else {
+                err.span_suggestion_short(
+                    span_semi,
+                    "remove this semicolon",
+                    "",
+                    Applicability::MachineApplicable,
+                );
+            }
+            true
+        } else {
+            false
+        }
+    }
+
+    pub(crate) fn consider_returning_binding(
+        &self,
+        blk: &'tcx hir::Block<'tcx>,
+        expected_ty: Ty<'tcx>,
+        err: &mut Diagnostic,
+    ) {
+        let mut shadowed = FxHashSet::default();
+        let mut candidate_idents = vec![];
+        let mut find_compatible_candidates = |pat: &hir::Pat<'_>| {
+            if let hir::PatKind::Binding(_, hir_id, ident, _) = &pat.kind
+                && let Some(pat_ty) = self.typeck_results.borrow().node_type_opt(*hir_id)
+            {
+                let pat_ty = self.resolve_vars_if_possible(pat_ty);
+                if self.can_coerce(pat_ty, expected_ty)
+                    && !(pat_ty, expected_ty).references_error()
+                    && shadowed.insert(ident.name)
+                {
+                    candidate_idents.push((*ident, pat_ty));
+                }
+            }
+            true
+        };
+
+        let hir = self.tcx.hir();
+        for stmt in blk.stmts.iter().rev() {
+            let StmtKind::Local(local) = &stmt.kind else { continue; };
+            local.pat.walk(&mut find_compatible_candidates);
+        }
+        match hir.find(hir.get_parent_node(blk.hir_id)) {
+            Some(hir::Node::Expr(hir::Expr { hir_id, .. })) => {
+                match hir.find(hir.get_parent_node(*hir_id)) {
+                    Some(hir::Node::Arm(hir::Arm { pat, .. })) => {
+                        pat.walk(&mut find_compatible_candidates);
+                    }
+                    Some(
+                        hir::Node::Item(hir::Item { kind: hir::ItemKind::Fn(_, _, body), .. })
+                        | hir::Node::ImplItem(hir::ImplItem {
+                            kind: hir::ImplItemKind::Fn(_, body),
+                            ..
+                        })
+                        | hir::Node::TraitItem(hir::TraitItem {
+                            kind: hir::TraitItemKind::Fn(_, hir::TraitFn::Provided(body)),
+                            ..
+                        })
+                        | hir::Node::Expr(hir::Expr {
+                            kind: hir::ExprKind::Closure(hir::Closure { body, .. }),
+                            ..
+                        }),
+                    ) => {
+                        for param in hir.body(*body).params {
+                            param.pat.walk(&mut find_compatible_candidates);
+                        }
+                    }
+                    Some(hir::Node::Expr(hir::Expr {
+                        kind:
+                            hir::ExprKind::If(
+                                hir::Expr { kind: hir::ExprKind::Let(let_), .. },
+                                then_block,
+                                _,
+                            ),
+                        ..
+                    })) if then_block.hir_id == *hir_id => {
+                        let_.pat.walk(&mut find_compatible_candidates);
+                    }
+                    _ => {}
+                }
+            }
+            _ => {}
+        }
+
+        match &candidate_idents[..] {
+            [(ident, _ty)] => {
+                let sm = self.tcx.sess.source_map();
+                if let Some(stmt) = blk.stmts.last() {
+                    let stmt_span = sm.stmt_span(stmt.span, blk.span);
+                    let sugg = if sm.is_multiline(blk.span)
+                        && let Some(spacing) = sm.indentation_before(stmt_span)
+                    {
+                        format!("\n{spacing}{ident}")
+                    } else {
+                        format!(" {ident}")
+                    };
+                    err.span_suggestion_verbose(
+                        stmt_span.shrink_to_hi(),
+                        format!("consider returning the local binding `{ident}`"),
+                        sugg,
+                        Applicability::MachineApplicable,
+                    );
+                } else {
+                    let sugg = if sm.is_multiline(blk.span)
+                        && let Some(spacing) = sm.indentation_before(blk.span.shrink_to_lo())
+                    {
+                        format!("\n{spacing}    {ident}\n{spacing}")
+                    } else {
+                        format!(" {ident} ")
+                    };
+                    let left_span = sm.span_through_char(blk.span, '{').shrink_to_hi();
+                    err.span_suggestion_verbose(
+                        sm.span_extend_while(left_span, |c| c.is_whitespace()).unwrap_or(left_span),
+                        format!("consider returning the local binding `{ident}`"),
+                        sugg,
+                        Applicability::MachineApplicable,
+                    );
+                }
+            }
+            values if (1..3).contains(&values.len()) => {
+                let spans = values.iter().map(|(ident, _)| ident.span).collect::<Vec<_>>();
+                err.span_note(spans, "consider returning one of these bindings");
+            }
+            _ => {}
+        }
+    }
 }

src/test/ui/async-await/async-block-control-flow-static-semantics.stderr

@@ -18,6 +18,14 @@ LL | |             break 0u8;
 LL | |         };
    | |_________- enclosing `async` block
 
+error[E0271]: type mismatch resolving `<impl Future<Output = u8> as Future>::Output == ()`
+  --> $DIR/async-block-control-flow-static-semantics.rs:26:39
+   |
+LL |     let _: &dyn Future<Output = ()> = &block;
+   |                                       ^^^^^^ expected `()`, found `u8`
+   |
+   = note: required for the cast from `impl Future<Output = u8>` to the object type `dyn Future<Output = ()>`
+
 error[E0308]: mismatched types
   --> $DIR/async-block-control-flow-static-semantics.rs:21:58
    |
@@ -32,7 +40,7 @@ LL | | }
    | |_^ expected `u8`, found `()`
 
 error[E0271]: type mismatch resolving `<impl Future<Output = u8> as Future>::Output == ()`
-  --> $DIR/async-block-control-flow-static-semantics.rs:26:39
+  --> $DIR/async-block-control-flow-static-semantics.rs:17:39
    |
 LL |     let _: &dyn Future<Output = ()> = &block;
    |                                       ^^^^^^ expected `()`, found `u8`
@@ -47,14 +55,6 @@ LL | fn return_targets_async_block_not_fn() -> u8 {
    |    |
    |    implicitly returns `()` as its body has no tail or `return` expression
 
-error[E0271]: type mismatch resolving `<impl Future<Output = u8> as Future>::Output == ()`
-  --> $DIR/async-block-control-flow-static-semantics.rs:17:39
-   |
-LL |     let _: &dyn Future<Output = ()> = &block;
-   |                                       ^^^^^^ expected `()`, found `u8`
-   |
-   = note: required for the cast from `impl Future<Output = u8>` to the object type `dyn Future<Output = ()>`
-
 error[E0308]: mismatched types
   --> $DIR/async-block-control-flow-static-semantics.rs:47:44
    |

src/test/ui/liveness/liveness-forgot-ret.stderr

@@ -5,6 +5,11 @@ LL | fn f(a: isize) -> isize { if god_exists(a) { return 5; }; }
    |    -              ^^^^^ expected `isize`, found `()`
    |    |
    |    implicitly returns `()` as its body has no tail or `return` expression
+   |
+help: consider returning the local binding `a`
+   |
+LL | fn f(a: isize) -> isize { if god_exists(a) { return 5; }; a }
+   |                                                           +
 
 error: aborting due to previous error
 

src/test/ui/parser/issues/issue-33413.stderr

@@ -11,6 +11,11 @@ LL |     fn f(*, a: u8) -> u8 {}
    |        -              ^^ expected `u8`, found `()`
    |        |
    |        implicitly returns `()` as its body has no tail or `return` expression
+   |
+help: consider returning the local binding `a`
+   |
+LL |     fn f(*, a: u8) -> u8 { a }
+   |                            +
 
 error: aborting due to 2 previous errors
 

src/test/ui/suggestions/return-bindings-multi.rs

@@ -0,0 +1,9 @@
+fn a(i: i32) -> i32 {
+    //~^ ERROR mismatched types
+    let j = 2i32;
+}
+
+fn b(i: i32, j: i32) -> i32 {}
+//~^ ERROR mismatched types
+
+fn main() {}

src/test/ui/suggestions/return-bindings-multi.stderr

@@ -0,0 +1,34 @@
+error[E0308]: mismatched types
+  --> $DIR/return-bindings-multi.rs:1:17
+   |
+LL | fn a(i: i32) -> i32 {
+   |    -            ^^^ expected `i32`, found `()`
+   |    |
+   |    implicitly returns `()` as its body has no tail or `return` expression
+   |
+note: consider returning one of these bindings
+  --> $DIR/return-bindings-multi.rs:1:6
+   |
+LL | fn a(i: i32) -> i32 {
+   |      ^
+LL |
+LL |     let j = 2i32;
+   |         ^
+
+error[E0308]: mismatched types
+  --> $DIR/return-bindings-multi.rs:6:25
+   |
+LL | fn b(i: i32, j: i32) -> i32 {}
+   |    -                    ^^^ expected `i32`, found `()`
+   |    |
+   |    implicitly returns `()` as its body has no tail or `return` expression
+   |
+note: consider returning one of these bindings
+  --> $DIR/return-bindings-multi.rs:6:6
+   |
+LL | fn b(i: i32, j: i32) -> i32 {}
+   |      ^       ^
+
+error: aborting due to 2 previous errors
+
+For more information about this error, try `rustc --explain E0308`.

src/test/ui/suggestions/return-bindings.fixed

@@ -0,0 +1,23 @@
+// run-rustfix
+
+#![allow(unused)]
+
+fn a(i: i32) -> i32 { i }
+//~^ ERROR mismatched types
+
+fn b(opt_str: Option<String>) {
+    let s: String = if let Some(s) = opt_str {
+        s
+    //~^ ERROR mismatched types
+    } else {
+        String::new()
+    };
+}
+
+fn c() -> Option<i32> {
+    //~^ ERROR mismatched types
+    let x = Some(1);
+    x
+}
+
+fn main() {}

src/test/ui/suggestions/return-bindings.rs

@@ -0,0 +1,21 @@
+// run-rustfix
+
+#![allow(unused)]
+
+fn a(i: i32) -> i32 {}
+//~^ ERROR mismatched types
+
+fn b(opt_str: Option<String>) {
+    let s: String = if let Some(s) = opt_str {
+        //~^ ERROR mismatched types
+    } else {
+        String::new()
+    };
+}
+
+fn c() -> Option<i32> {
+    //~^ ERROR mismatched types
+    let x = Some(1);
+}
+
+fn main() {}