Add lint to tell about let else pattern

Author: est31

CHANGELOG.md

@@ -3996,6 +3996,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

clippy_lints/src/lib.register_lints.rs

@@ -251,6 +251,7 @@ store.register_lints(&[
     manual_bits::MANUAL_BITS,
     manual_clamp::MANUAL_CLAMP,
     manual_instant_elapsed::MANUAL_INSTANT_ELAPSED,
+    manual_let_else::MANUAL_LET_ELSE,
     manual_non_exhaustive::MANUAL_NON_EXHAUSTIVE,
     manual_rem_euclid::MANUAL_REM_EUCLID,
     manual_retain::MANUAL_RETAIN,

clippy_lints/src/lib.register_pedantic.rs

@@ -47,6 +47,7 @@ store.register_group(true, "clippy::pedantic", Some("clippy_pedantic"), vec![
     LintId::of(macro_use::MACRO_USE_IMPORTS),
     LintId::of(manual_assert::MANUAL_ASSERT),
     LintId::of(manual_instant_elapsed::MANUAL_INSTANT_ELAPSED),
+    LintId::of(manual_let_else::MANUAL_LET_ELSE),
     LintId::of(manual_string_new::MANUAL_STRING_NEW),
     LintId::of(matches::MATCH_BOOL),
     LintId::of(matches::MATCH_ON_VEC_ITEMS),

clippy_lints/src/lib.rs

@@ -270,6 +270,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;
@@ -605,6 +606,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)));

clippy_lints/src/manual_let_else.rs

@@ -0,0 +1,165 @@
+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.60.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
+    }
+    let mut does_diverge = false;
+    for_each_expr(expr, |ex| {
+        match ex.kind {
+            ExprKind::Continue(_) | ExprKind::Break(_, _) | ExprKind::Ret(_) => {
+                does_diverge = true;
+                ControlFlow::Break(())
+            },
+            ExprKind::Call(call, _) => {
+                if is_never(cx, ex) || is_never(cx, call) {
+                    does_diverge = true;
+                    return ControlFlow::Break(());
+                }
+                ControlFlow::Continue(Descend::Yes)
+            },
+            ExprKind::MethodCall(..) => {
+                if is_never(cx, ex) {
+                    does_diverge = true;
+                    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 {
+                    does_diverge = true;
+                    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 {
+                    does_diverge = true;
+                    return ControlFlow::Break(());
+                }
+                ControlFlow::Continue(Descend::No)
+            },
+
+            // Don't continue into loops, as they are breakable,
+            // and we'd have to start checking labels.
+            ExprKind::Block(_, Some(_)) | ExprKind::Loop(..) => ControlFlow::Break(()),
+
+            // Default: descend
+            // TODO: support breakable blocks. For this we would need label support.
+            _ => ControlFlow::Continue(Descend::Yes),
+        }
+    });
+    does_diverge
+}
+
+/// 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
+}

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),

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 }

src/docs.rs

@@ -266,6 +266,7 @@ docs! {
     "manual_find_map",
     "manual_flatten",
     "manual_instant_elapsed",
+    "manual_let_else",
     "manual_map",
     "manual_memcpy",
     "manual_non_exhaustive",

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 };
+```