Remove note about iteration count in coerce

and replace it with a simple note suggesting returning a value.

The type mismatch error was never due to how many times
the loop iterates. It is more because of the peculiar structure
of what the for loop desugars to. So the note talking about
iteration count didn't make sense

Author: gurry

Diff for: compiler/rustc_hir_typeck/src/coercion.rs

@@ -36,11 +36,9 @@
 //! ```
 
 use crate::FnCtxt;
-use rustc_errors::{codes::*, struct_span_code_err, Applicability, Diag, MultiSpan};
+use rustc_errors::{codes::*, struct_span_code_err, Applicability, Diag};
 use rustc_hir as hir;
 use rustc_hir::def_id::DefId;
-use rustc_hir::intravisit::{self, Visitor};
-use rustc_hir::Expr;
 use rustc_hir_analysis::hir_ty_lowering::HirTyLowerer;
 use rustc_infer::infer::type_variable::{TypeVariableOrigin, TypeVariableOriginKind};
 use rustc_infer::infer::{Coercion, DefineOpaqueTypes, InferOk, InferResult};
@@ -93,22 +91,6 @@ impl<'a, 'tcx> Deref for Coerce<'a, 'tcx> {
 
 type CoerceResult<'tcx> = InferResult<'tcx, (Vec<Adjustment<'tcx>>, Ty<'tcx>)>;
 
-struct CollectRetsVisitor<'tcx> {
-    ret_exprs: Vec<&'tcx hir::Expr<'tcx>>,
-}
-
-impl<'tcx> Visitor<'tcx> for CollectRetsVisitor<'tcx> {
-    fn visit_expr(&mut self, expr: &'tcx Expr<'tcx>) {
-        match expr.kind {
-            hir::ExprKind::Ret(_) => self.ret_exprs.push(expr),
-            // `return` in closures does not return from the outer function
-            hir::ExprKind::Closure(_) => return,
-            _ => {}
-        }
-        intravisit::walk_expr(self, expr);
-    }
-}
-
 /// Coercing a mutable reference to an immutable works, while
 /// coercing `&T` to `&mut T` should be forbidden.
 fn coerce_mutbls<'tcx>(
@@ -1591,7 +1573,6 @@ impl<'tcx, 'exprs, E: AsCoercionSite> CoerceMany<'tcx, 'exprs, E> {
 
                 let mut err;
                 let mut unsized_return = false;
-                let mut visitor = CollectRetsVisitor { ret_exprs: vec![] };
                 match *cause.code() {
                     ObligationCauseCode::ReturnNoExpression => {
                         err = struct_span_code_err!(
@@ -1617,11 +1598,6 @@ impl<'tcx, 'exprs, E: AsCoercionSite> CoerceMany<'tcx, 'exprs, E> {
                         if !fcx.tcx.features().unsized_locals {
                             unsized_return = self.is_return_ty_definitely_unsized(fcx);
                         }
-                        if let Some(expression) = expression
-                            && let hir::ExprKind::Loop(loop_blk, ..) = expression.kind
-                        {
-                            intravisit::walk_block(&mut visitor, loop_blk);
-                        }
                     }
                     ObligationCauseCode::ReturnValue(id) => {
                         err = self.report_return_mismatched_types(
@@ -1659,15 +1635,8 @@ impl<'tcx, 'exprs, E: AsCoercionSite> CoerceMany<'tcx, 'exprs, E> {
                         None,
                         Some(coercion_error),
                     );
-                    if visitor.ret_exprs.len() > 0 {
-                        self.note_unreachable_loop_return(
-                            &mut err,
-                            fcx.tcx,
-                            &expr,
-                            &visitor.ret_exprs,
-                            expected,
-                        );
-                    }
+
+                    self.note_unreachable_loop_return(&mut err, fcx.tcx, &expr, expected);
                 }
 
                 let reported = err.emit_unless(unsized_return);
@@ -1682,102 +1651,61 @@ impl<'tcx, 'exprs, E: AsCoercionSite> CoerceMany<'tcx, 'exprs, E> {
         err: &mut Diag<'_>,
         tcx: TyCtxt<'tcx>,
         expr: &hir::Expr<'tcx>,
-        ret_exprs: &Vec<&'tcx hir::Expr<'tcx>>,
         ty: Ty<'tcx>,
     ) {
-        let hir::ExprKind::Loop(_, _, _, loop_span) = expr.kind else {
+        let hir::ExprKind::Loop(_, _, _, _) = expr.kind else {
             return;
         };
-        let mut span: MultiSpan = vec![loop_span].into();
-        span.push_span_label(loop_span, "this might have zero elements to iterate on");
-        const MAXITER: usize = 3;
-        let iter = ret_exprs.iter().take(MAXITER);
-        for ret_expr in iter {
-            span.push_span_label(
-                ret_expr.span,
-                "if the loop doesn't execute, this value would never get returned",
-            );
-        }
-        err.span_note(
-            span,
-            "the function expects a value to always be returned, but loops might run zero times",
-        );
-        if MAXITER < ret_exprs.len() {
-            err.note(format!(
-                "if the loop doesn't execute, {} other values would never get returned",
-                ret_exprs.len() - MAXITER
-            ));
-        }
+
         let hir = tcx.hir();
-        let item = hir.get_parent_item(expr.hir_id);
-        let ret_msg = "return a value for the case when the loop has zero elements to iterate on";
-        let ret_ty_msg =
-            "otherwise consider changing the return type to account for that possibility";
-        let node = tcx.hir_node(item.into());
-        if let Some(body_id) = node.body_id()
-            && let Some(sig) = node.fn_sig()
-            && let hir::ExprKind::Block(block, _) = hir.body(body_id).value.kind
-            && !ty.is_never()
-        {
-            let indentation = if let None = block.expr
-                && let [.., last] = &block.stmts
-            {
-                tcx.sess.source_map().indentation_before(last.span).unwrap_or_else(String::new)
-            } else if let Some(expr) = block.expr {
-                tcx.sess.source_map().indentation_before(expr.span).unwrap_or_else(String::new)
+        let body_def_id = hir.enclosing_body_owner(expr.hir_id);
+        let body_id = hir.body_owned_by(body_def_id);
+        let body = hir.body(body_id);
+
+        let span = match body.value.kind {
+            // Regular block as in a function body
+            hir::ExprKind::Block(block, _) => {
+                if let None = block.expr
+                    && let [.., last] = &block.stmts
+                {
+                    Some(last.span)
+                } else if let Some(expr) = block.expr {
+                    Some(expr.span)
+                } else {
+                    None
+                }
+            }
+            // Anon const body (there's no enclosing block in this case)
+            hir::ExprKind::DropTemps(expr) => Some(expr.span),
+            _ => None,
+        };
+
+        if let Some(span) = span {
+            let (msg, suggestion) = if ty.is_never() {
+                (
+                    "consider adding a diverging expression here",
+                    "`loop {}` or `panic!(\"...\")`".to_string(),
+                )
             } else {
-                String::new()
+                ("consider returning a value here", format!("`{ty}` value"))
             };
-            if let None = block.expr
-                && let [.., last] = &block.stmts
-            {
-                err.span_suggestion_verbose(
-                    last.span.shrink_to_hi(),
-                    ret_msg,
-                    format!("\n{indentation}/* `{ty}` value */"),
-                    Applicability::MaybeIncorrect,
-                );
-            } else if let Some(expr) = block.expr {
-                err.span_suggestion_verbose(
-                    expr.span.shrink_to_hi(),
-                    ret_msg,
-                    format!("\n{indentation}/* `{ty}` value */"),
-                    Applicability::MaybeIncorrect,
-                );
-            }
-            let mut sugg = match sig.decl.output {
-                hir::FnRetTy::DefaultReturn(span) => {
-                    vec![(span, " -> Option<()>".to_string())]
-                }
-                hir::FnRetTy::Return(ty) => {
-                    vec![
-                        (ty.span.shrink_to_lo(), "Option<".to_string()),
-                        (ty.span.shrink_to_hi(), ">".to_string()),
-                    ]
-                }
+
+            let src_map = tcx.sess.source_map();
+            let suggestion = if src_map.is_multiline(expr.span) {
+                let indentation = src_map.indentation_before(span).unwrap_or_else(String::new);
+                format!("\n{indentation}/* {suggestion} */")
+            } else {
+                // If the entire expr is on a single line
+                // put the suggestion also on the same line
+                format!(" /* {suggestion} */")
             };
-            for ret_expr in ret_exprs {
-                match ret_expr.kind {
-                    hir::ExprKind::Ret(Some(expr)) => {
-                        sugg.push((expr.span.shrink_to_lo(), "Some(".to_string()));
-                        sugg.push((expr.span.shrink_to_hi(), ")".to_string()));
-                    }
-                    hir::ExprKind::Ret(None) => {
-                        sugg.push((ret_expr.span.shrink_to_hi(), " Some(())".to_string()));
-                    }
-                    _ => {}
-                }
-            }
-            if let None = block.expr
-                && let [.., last] = &block.stmts
-            {
-                sugg.push((last.span.shrink_to_hi(), format!("\n{indentation}None")));
-            } else if let Some(expr) = block.expr {
-                sugg.push((expr.span.shrink_to_hi(), format!("\n{indentation}None")));
-            }
-            err.multipart_suggestion(ret_ty_msg, sugg, Applicability::MaybeIncorrect);
-        } else {
-            err.help(format!("{ret_msg}, {ret_ty_msg}"));
+
+            err.span_suggestion_verbose(
+                span.shrink_to_hi(),
+                msg,
+                suggestion,
+                Applicability::MaybeIncorrect,
+            );
         }
     }
 

Diff for: tests/ui/coercion/coerce-loop-issue-122561.rs

@@ -0,0 +1,94 @@
+// Regression test for #122561
+
+fn for_infinite() -> bool {
+    for i in 0.. {
+    //~^ ERROR mismatched types
+        return false;
+    }
+}
+
+fn for_finite() -> String {
+    for i in 0..5 {
+    //~^ ERROR mismatched types
+        return String::from("test");
+    }
+}
+
+fn for_never_type() -> ! {
+    for i in 0..0 {
+    //~^ ERROR mismatched types
+    }
+}
+
+// Entire function on a single line.
+// Tests that we format the suggestion
+// correctly in this case
+fn for_single_line() -> bool { for i in 0.. { return false; } }
+//~^ ERROR mismatched types
+
+// Loop in an anon const in function args
+// Tests that we:
+// a. deal properly with this complex case
+// b. format the suggestion correctly so
+//    that it's readable
+fn for_in_arg(a: &[(); for x in 0..2 {}]) -> bool {
+//~^ ERROR `for` is not allowed in a `const`
+//~| ERROR mismatched types
+    true
+}
+
+fn while_true() -> bool {
+    while true {
+    //~^ ERROR mismatched types
+    //~| WARN denote infinite loops with `loop { ... }` [while_true]
+        return true;
+    }
+}
+
+fn while_false() -> bool {
+    while false {
+    //~^ ERROR mismatched types
+        return true;
+    }
+}
+
+fn while_true_never_type() -> ! {
+    while true {
+    //~^ ERROR mismatched types
+    //~| WARN denote infinite loops with `loop { ... }` [while_true]
+    }
+}
+
+// No type mismatch error in this case
+fn loop_() -> bool {
+    loop {
+        return true;
+    }
+}
+
+const C: i32 = {
+    for i in 0.. {
+    //~^ ERROR `for` is not allowed in a `const`
+    //~| ERROR mismatched types
+    }
+};
+
+fn main() {
+    let _ = [10; {
+        for i in 0..5 {
+        //~^ ERROR `for` is not allowed in a `const`
+        //~| ERROR mismatched types
+        }
+    }];
+
+    let _ = [10; {
+        while false {
+        //~^ ERROR mismatched types
+        }
+    }];
+
+
+    let _ = |a: &[(); for x in 0..2 {}]| {};
+    //~^ ERROR `for` is not allowed in a `const`
+    //~| ERROR mismatched types
+}