Add lint to tell about let else pattern

est31 · est31 · commit b015d5efc2a5 · 2022-10-24T21:51:25.000+02:00
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -3997,6 +3997,7 @@ Released 2018-09-13
 [`manual_find_map`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_find_map
 [`manual_flatten`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_flatten
 [`manual_instant_elapsed`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_instant_elapsed
+[`manual_let_else`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_let_else
 [`manual_map`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_map
 [`manual_memcpy`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_memcpy
 [`manual_non_exhaustive`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_non_exhaustive
diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs
@@ -170,6 +170,7 @@ mod manual_async_fn;
 mod manual_bits;
 mod manual_clamp;
 mod manual_instant_elapsed;
+mod manual_let_else;
 mod manual_non_exhaustive;
 mod manual_rem_euclid;
 mod manual_retain;
@@ -603,6 +604,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         ))
     });
     store.register_late_pass(move |_| Box::new(matches::Matches::new(msrv)));
+    store.register_late_pass(move |_| Box::new(manual_let_else::ManualLetElse::new(msrv)));
     store.register_early_pass(move || Box::new(manual_non_exhaustive::ManualNonExhaustiveStruct::new(msrv)));
     store.register_late_pass(move |_| Box::new(manual_non_exhaustive::ManualNonExhaustiveEnum::new(msrv)));
     store.register_late_pass(move |_| Box::new(manual_strip::ManualStrip::new(msrv)));
diff --git a/clippy_lints/src/manual_let_else.rs b/clippy_lints/src/manual_let_else.rs
@@ -0,0 +1,160 @@
+use clippy_utils::diagnostics::span_lint;
+use clippy_utils::visitors::{for_each_expr, Descend};
+use clippy_utils::{higher, meets_msrv, msrvs, peel_blocks};
+use if_chain::if_chain;
+use rustc_hir::{Expr, ExprKind, Pat, QPath, Stmt, StmtKind};
+use rustc_lint::{LateContext, LateLintPass, LintContext};
+use rustc_middle::lint::in_external_macro;
+use rustc_semver::RustcVersion;
+use rustc_session::{declare_tool_lint, impl_lint_pass};
+use std::ops::ControlFlow;
+
+declare_clippy_lint! {
+    /// ### What it does
+    ///
+    /// Warn of cases where `let...else` could be used
+    ///
+    /// ### Why is this bad?
+    ///
+    /// `let...else` provides a standard construct for this pattern
+    /// that people can easily recognize. It's also more compact.
+    ///
+    /// ### Example
+    ///
+    /// ```rust
+    /// # let w = Some(0);
+    /// let v = if let Some(v) = w { v } else { return };
+    /// ```
+    ///
+    /// Could be written:
+    ///
+    /// ```rust
+    /// # #![feature(let_else)]
+    /// # fn main () {
+    /// # let w = Some(0);
+    /// let Some(v) = w else { return };
+    /// # }
+    /// ```
+    #[clippy::version = "1.67.0"]
+    pub MANUAL_LET_ELSE,
+    pedantic,
+    "manual implementation of a let...else statement"
+}
+
+pub struct ManualLetElse {
+    msrv: Option<RustcVersion>,
+}
+
+impl ManualLetElse {
+    #[must_use]
+    pub fn new(msrv: Option<RustcVersion>) -> Self {
+        Self { msrv }
+    }
+}
+
+impl_lint_pass!(ManualLetElse => [MANUAL_LET_ELSE]);
+
+impl<'tcx> LateLintPass<'tcx> for ManualLetElse {
+    fn check_stmt(&mut self, cx: &LateContext<'_>, stmt: &'tcx Stmt<'tcx>) {
+        if !meets_msrv(self.msrv, msrvs::LET_ELSE) {
+            return;
+        }
+
+        if in_external_macro(cx.sess(), stmt.span) {
+            return;
+        }
+
+        if_chain! {
+            if let StmtKind::Local(local) = stmt.kind;
+            if let Some(init) = local.init;
+            if let Some(higher::IfLet { let_pat, let_expr: _, if_then, if_else }) = higher::IfLet::hir(cx, init);
+            if if_then_simple_identity(let_pat, if_then);
+            if let Some(if_else) = if_else;
+            if expr_diverges(cx, if_else);
+            then {
+                span_lint(
+                    cx,
+                    MANUAL_LET_ELSE,
+                    stmt.span,
+                    "this could be rewritten as `let else`",
+                );
+            }
+        }
+    }
+
+    extract_msrv_attr!(LateContext);
+}
+
+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
+    }
+    // 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| expr_diverges(cx, arm.body));
+                if diverges {
+                    return ControlFlow::Break(());
+                }
+                ControlFlow::Continue(Descend::No)
+            },
+
+            // 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),
+        }
+    })
+    .is_some()
+}
+
+/// Checks if the passed `if_then` is a simple identity
+fn if_then_simple_identity(let_pat: &'_ Pat<'_>, if_then: &'_ Expr<'_>) -> bool {
+    // TODO support patterns with multiple bindings and tuples, like:
+    //   let (foo, bar) = if let (Some(foo), bar) = g() { (foo, bar) } else { ... }
+    if_chain! {
+        if let ExprKind::Path(QPath::Resolved(_ty, path)) = &peel_blocks(if_then).kind;
+        if let [path_seg] = path.segments;
+        then {
+            let mut pat_bindings = Vec::new();
+            let_pat.each_binding(|_ann, _hir_id, _sp, ident| {
+                pat_bindings.push(ident);
+            });
+            if let [binding] = &pat_bindings[..] {
+                return path_seg.ident == *binding;
+            }
+        }
+    }
+    false
+}
diff --git a/clippy_lints/src/utils/conf.rs b/clippy_lints/src/utils/conf.rs
@@ -213,7 +213,7 @@ define_Conf! {
     ///
     /// Suppress lints whenever the suggested change would cause breakage for other crates.
     (avoid_breaking_exported_api: bool = true),
-    /// Lint: MANUAL_SPLIT_ONCE, MANUAL_STR_REPEAT, CLONED_INSTEAD_OF_COPIED, REDUNDANT_FIELD_NAMES, REDUNDANT_STATIC_LIFETIMES, FILTER_MAP_NEXT, CHECKED_CONVERSIONS, MANUAL_RANGE_CONTAINS, USE_SELF, MEM_REPLACE_WITH_DEFAULT, MANUAL_NON_EXHAUSTIVE, OPTION_AS_REF_DEREF, MAP_UNWRAP_OR, MATCH_LIKE_MATCHES_MACRO, MANUAL_STRIP, MISSING_CONST_FOR_FN, UNNESTED_OR_PATTERNS, FROM_OVER_INTO, PTR_AS_PTR, IF_THEN_SOME_ELSE_NONE, APPROX_CONSTANT, DEPRECATED_CFG_ATTR, INDEX_REFUTABLE_SLICE, MAP_CLONE, BORROW_AS_PTR, MANUAL_BITS, ERR_EXPECT, CAST_ABS_TO_UNSIGNED, UNINLINED_FORMAT_ARGS, MANUAL_CLAMP.
+    /// Lint: MANUAL_SPLIT_ONCE, MANUAL_STR_REPEAT, CLONED_INSTEAD_OF_COPIED, REDUNDANT_FIELD_NAMES, REDUNDANT_STATIC_LIFETIMES, FILTER_MAP_NEXT, CHECKED_CONVERSIONS, MANUAL_RANGE_CONTAINS, USE_SELF, MEM_REPLACE_WITH_DEFAULT, MANUAL_NON_EXHAUSTIVE, OPTION_AS_REF_DEREF, MAP_UNWRAP_OR, MATCH_LIKE_MATCHES_MACRO, MANUAL_STRIP, MISSING_CONST_FOR_FN, UNNESTED_OR_PATTERNS, FROM_OVER_INTO, PTR_AS_PTR, IF_THEN_SOME_ELSE_NONE, APPROX_CONSTANT, DEPRECATED_CFG_ATTR, INDEX_REFUTABLE_SLICE, MAP_CLONE, BORROW_AS_PTR, MANUAL_BITS, ERR_EXPECT, CAST_ABS_TO_UNSIGNED, UNINLINED_FORMAT_ARGS, MANUAL_CLAMP, MANUAL_LET_ELSE.
     ///
     /// The minimum rust version that the project supports
     (msrv: Option<String> = None),
diff --git a/clippy_utils/src/msrvs.rs b/clippy_utils/src/msrvs.rs
@@ -12,6 +12,7 @@ macro_rules! msrv_aliases {
 
 // names may refer to stabilized feature flags or library items
 msrv_aliases! {
+    1,65,0 { LET_ELSE }
     1,62,0 { BOOL_THEN_SOME }
     1,58,0 { FORMAT_ARGS_CAPTURE }
     1,53,0 { OR_PATTERNS, MANUAL_BITS, BTREE_MAP_RETAIN, BTREE_SET_RETAIN, ARRAY_INTO_ITERATOR }
diff --git a/src/docs/manual_let_else.txt b/src/docs/manual_let_else.txt
@@ -0,0 +1,20 @@
+### What it does
+
+Warn of cases where `let...else` could be used
+
+### Why is this bad?
+
+`let...else` provides a standard construct for this pattern
+that people can easily recognize. It's also more compact.
+
+### Example
+
+```
+let v = if let Some(v) = w { v } else { return };
+```
+
+Could be written:
+
+```
+let Some(v) = w else { return };
+```
diff --git a/tests/ui/manual_let_else.rs b/tests/ui/manual_let_else.rs
@@ -0,0 +1,167 @@
+#![allow(unused_braces, unused_variables, dead_code)]
+#![allow(
+    clippy::collapsible_else_if,
+    clippy::unused_unit,
+    clippy::never_loop,
+    clippy::let_unit_value
+)]
+#![warn(clippy::manual_let_else)]
+
+fn g() -> Option<()> {
+    None
+}
+
+fn main() {}
+
+fn fire() {
+    let v = if let Some(v_some) = g() { v_some } else { return };
+    let v = if let Some(v_some) = g() {
+        v_some
+    } else {
+        return;
+    };
+
+    let v = if let Some(v) = g() {
+        // Blocks around the identity should have no impact
+        {
+            { v }
+        }
+    } else {
+        // Some computation should still make it fire
+        g();
+        return;
+    };
+
+    // continue and break diverge
+    loop {
+        let v = if let Some(v_some) = g() { v_some } else { continue };
+        let v = if let Some(v_some) = g() { v_some } else { break };
+    }
+
+    // panic also diverges
+    let v = if let Some(v_some) = g() { v_some } else { panic!() };
+
+    // abort also diverges
+    let v = if let Some(v_some) = g() {
+        v_some
+    } else {
+        std::process::abort()
+    };
+
+    // If whose two branches diverge also diverges
+    let v = if let Some(v_some) = g() {
+        v_some
+    } else {
+        if true { return } else { panic!() }
+    };
+
+    // Top level else if
+    let v = if let Some(v_some) = g() {
+        v_some
+    } else if true {
+        return;
+    } else {
+        panic!("diverge");
+    };
+
+    // All match arms diverge
+    let v = if let Some(v_some) = g() {
+        v_some
+    } else {
+        match (g(), g()) {
+            (Some(_), None) => return,
+            (None, Some(_)) => {
+                if true {
+                    return;
+                } else {
+                    panic!();
+                }
+            },
+            _ => return,
+        }
+    };
+
+    // Tuples supported for the declared variables
+    let (v, w) = if let Some(v_some) = g().map(|v| (v, 42)) {
+        v_some
+    } else {
+        return;
+    };
+}
+
+fn not_fire() {
+    let v = if let Some(v_some) = g() {
+        // Nothing returned. Should not fire.
+    } else {
+        return;
+    };
+
+    let w = 0;
+    let v = if let Some(v_some) = g() {
+        // Different variable than v_some. Should not fire.
+        w
+    } else {
+        return;
+    };
+
+    let v = if let Some(v_some) = g() {
+        // Computation in then clause. Should not fire.
+        g();
+        v_some
+    } else {
+        return;
+    };
+
+    let v = if let Some(v_some) = g() {
+        v_some
+    } else {
+        if false {
+            return;
+        }
+        // This doesn't diverge. Should not fire.
+        ()
+    };
+
+    let v = if let Some(v_some) = g() {
+        v_some
+    } else {
+        // There is one match arm that doesn't diverge. Should not fire.
+        match (g(), g()) {
+            (Some(_), None) => return,
+            (None, Some(_)) => return,
+            (Some(_), Some(_)) => (),
+            _ => return,
+        }
+    };
+
+    let v = if let Some(v_some) = g() {
+        v_some
+    } else {
+        // loop with a break statement inside does not diverge.
+        loop {
+            break;
+        }
+    };
+
+
+    let v = if let Some(v_some) = g() {
+        v_some
+    } else {
+        enum Uninhabited {}
+        fn un() -> Uninhabited {
+            panic!()
+        }
+        // Don't lint if the type is uninhabited but not !
+        un()
+    };
+
+    fn question_mark() -> Option<()> {
+        let v = if let Some(v) = g() {
+            v
+        } else {
+            // Question mark does not diverge
+            g()?
+        };
+        Some(v)
+    }
+}
diff --git a/tests/ui/manual_let_else.stderr b/tests/ui/manual_let_else.stderr
