Auto merge of rust-lang#10332 - samueltardieu:issue-10296, r=Alexendoo

manual_let_else: let/else is not divergent by default

The divergent `else` block of a `let`/`else` statement does not make the `let/else` statement itself divergent.

Fixes rust-lang#10296

changelog: [`manual_let_else`]: do not consider `let`/`else` to be divergent by default

Author: bors

clippy_lints/src/manual_let_else.rs

@@ -4,11 +4,12 @@ use clippy_utils::msrvs::{self, Msrv};
 use clippy_utils::peel_blocks;
 use clippy_utils::source::snippet_with_context;
 use clippy_utils::ty::is_type_diagnostic_item;
-use clippy_utils::visitors::{for_each_expr, Descend};
+use clippy_utils::visitors::{Descend, Visitable};
 use if_chain::if_chain;
 use rustc_data_structures::fx::FxHashSet;
 use rustc_errors::Applicability;
-use rustc_hir::{Expr, ExprKind, MatchSource, Pat, PatKind, QPath, Stmt, StmtKind};
+use rustc_hir::intravisit::{walk_expr, Visitor};
+use rustc_hir::{Expr, ExprKind, HirId, ItemId, Local, MatchSource, Pat, PatKind, QPath, Stmt, StmtKind, Ty};
 use rustc_lint::{LateContext, LateLintPass, LintContext};
 use rustc_middle::lint::in_external_macro;
 use rustc_session::{declare_tool_lint, impl_lint_pass};
@@ -162,61 +163,102 @@ fn emit_manual_let_else(cx: &LateContext<'_>, span: Span, expr: &Expr<'_>, pat:
     );
 }
 
-fn expr_diverges(cx: &LateContext<'_>, expr: &'_ Expr<'_>) -> bool {
-    fn is_never(cx: &LateContext<'_>, expr: &'_ Expr<'_>) -> bool {
-        if let Some(ty) = cx.typeck_results().expr_ty_opt(expr) {
-            return ty.is_never();
-        }
-        false
+/// Check whether an expression is divergent. May give false negatives.
+fn expr_diverges(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool {
+    struct V<'cx, 'tcx> {
+        cx: &'cx LateContext<'tcx>,
+        res: ControlFlow<(), Descend>,
     }
-    // We can't just call is_never on expr and be done, because the type system
-    // sometimes coerces the ! type to something different before we can get
-    // our hands on it. So instead, we do a manual search. We do fall back to
-    // is_never in some places when there is no better alternative.
-    for_each_expr(expr, |ex| {
-        match ex.kind {
-            ExprKind::Continue(_) | ExprKind::Break(_, _) | ExprKind::Ret(_) => ControlFlow::Break(()),
-            ExprKind::Call(call, _) => {
-                if is_never(cx, ex) || is_never(cx, call) {
-                    return ControlFlow::Break(());
-                }
-                ControlFlow::Continue(Descend::Yes)
-            },
-            ExprKind::MethodCall(..) => {
-                if is_never(cx, ex) {
-                    return ControlFlow::Break(());
-                }
-                ControlFlow::Continue(Descend::Yes)
-            },
-            ExprKind::If(if_expr, if_then, if_else) => {
-                let else_diverges = if_else.map_or(false, |ex| expr_diverges(cx, ex));
-                let diverges = expr_diverges(cx, if_expr) || (else_diverges && expr_diverges(cx, if_then));
-                if diverges {
-                    return ControlFlow::Break(());
-                }
-                ControlFlow::Continue(Descend::No)
-            },
-            ExprKind::Match(match_expr, match_arms, _) => {
-                let diverges = expr_diverges(cx, match_expr)
-                    || match_arms.iter().all(|arm| {
-                        let guard_diverges = arm.guard.as_ref().map_or(false, |g| expr_diverges(cx, g.body()));
-                        guard_diverges || expr_diverges(cx, arm.body)
-                    });
-                if diverges {
-                    return ControlFlow::Break(());
+    impl<'tcx> Visitor<'tcx> for V<'_, '_> {
+        fn visit_expr(&mut self, e: &'tcx Expr<'tcx>) {
+            fn is_never(cx: &LateContext<'_>, expr: &'_ Expr<'_>) -> bool {
+                if let Some(ty) = cx.typeck_results().expr_ty_opt(expr) {
+                    return ty.is_never();
                 }
-                ControlFlow::Continue(Descend::No)
-            },
+                false
+            }
+
+            if self.res.is_break() {
+                return;
+            }
 
-            // Don't continue into loops or labeled blocks, as they are breakable,
-            // and we'd have to start checking labels.
-            ExprKind::Block(_, Some(_)) | ExprKind::Loop(..) => ControlFlow::Continue(Descend::No),
+            // We can't just call is_never on expr and be done, because the type system
+            // sometimes coerces the ! type to something different before we can get
+            // our hands on it. So instead, we do a manual search. We do fall back to
+            // is_never in some places when there is no better alternative.
+            self.res = match e.kind {
+                ExprKind::Continue(_) | ExprKind::Break(_, _) | ExprKind::Ret(_) => ControlFlow::Break(()),
+                ExprKind::Call(call, _) => {
+                    if is_never(self.cx, e) || is_never(self.cx, call) {
+                        ControlFlow::Break(())
+                    } else {
+                        ControlFlow::Continue(Descend::Yes)
+                    }
+                },
+                ExprKind::MethodCall(..) => {
+                    if is_never(self.cx, e) {
+                        ControlFlow::Break(())
+                    } else {
+                        ControlFlow::Continue(Descend::Yes)
+                    }
+                },
+                ExprKind::If(if_expr, if_then, if_else) => {
+                    let else_diverges = if_else.map_or(false, |ex| expr_diverges(self.cx, ex));
+                    let diverges =
+                        expr_diverges(self.cx, if_expr) || (else_diverges && expr_diverges(self.cx, if_then));
+                    if diverges {
+                        ControlFlow::Break(())
+                    } else {
+                        ControlFlow::Continue(Descend::No)
+                    }
+                },
+                ExprKind::Match(match_expr, match_arms, _) => {
+                    let diverges = expr_diverges(self.cx, match_expr)
+                        || match_arms.iter().all(|arm| {
+                            let guard_diverges = arm.guard.as_ref().map_or(false, |g| expr_diverges(self.cx, g.body()));
+                            guard_diverges || expr_diverges(self.cx, arm.body)
+                        });
+                    if diverges {
+                        ControlFlow::Break(())
+                    } else {
+                        ControlFlow::Continue(Descend::No)
+                    }
+                },
 
-            // Default: descend
-            _ => ControlFlow::Continue(Descend::Yes),
+                // Don't continue into loops or labeled blocks, as they are breakable,
+                // and we'd have to start checking labels.
+                ExprKind::Block(_, Some(_)) | ExprKind::Loop(..) => ControlFlow::Continue(Descend::No),
+
+                // Default: descend
+                _ => ControlFlow::Continue(Descend::Yes),
+            };
+            if let ControlFlow::Continue(Descend::Yes) = self.res {
+                walk_expr(self, e);
+            }
+        }
+
+        fn visit_local(&mut self, local: &'tcx Local<'_>) {
+            // Don't visit the else block of a let/else statement as it will not make
+            // the statement divergent even though the else block is divergent.
+            if let Some(init) = local.init {
+                self.visit_expr(init);
+            }
         }
-    })
-    .is_some()
+
+        // Avoid unnecessary `walk_*` calls.
+        fn visit_ty(&mut self, _: &'tcx Ty<'tcx>) {}
+        fn visit_pat(&mut self, _: &'tcx Pat<'tcx>) {}
+        fn visit_qpath(&mut self, _: &'tcx QPath<'tcx>, _: HirId, _: Span) {}
+        // Avoid monomorphising all `visit_*` functions.
+        fn visit_nested_item(&mut self, _: ItemId) {}
+    }
+
+    let mut v = V {
+        cx,
+        res: ControlFlow::Continue(Descend::Yes),
+    };
+    expr.visit(&mut v);
+    v.res.is_break()
 }
 
 fn pat_allowed_for_else(cx: &LateContext<'_>, pat: &'_ Pat<'_>, check_types: bool) -> bool {

tests/ui/manual_let_else.rs

@@ -248,4 +248,15 @@ fn not_fire() {
         Some(value) => value,
         _ => macro_call!(),
     };
+
+    // Issue 10296
+    // The let/else block in the else part is not divergent despite the presence of return
+    let _x = if let Some(x) = Some(1) {
+        x
+    } else {
+        let Some(_z) = Some(3) else {
+            return
+        };
+        1
+    };
 }