Add lint to tell about let else pattern

Author: est31

CHANGELOG.md

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

@@ -244,6 +244,7 @@ store.register_lints(&[
     manual_async_fn::MANUAL_ASYNC_FN,
     manual_bits::MANUAL_BITS,
     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_restriction.rs

@@ -30,6 +30,7 @@ store.register_group(true, "clippy::restriction", Some("clippy_restriction"), ve
     LintId::of(large_include_file::LARGE_INCLUDE_FILE),
     LintId::of(let_underscore::LET_UNDERSCORE_MUST_USE),
     LintId::of(literal_representation::DECIMAL_LITERAL_REPRESENTATION),
+    LintId::of(manual_let_else::MANUAL_LET_ELSE),
     LintId::of(matches::REST_PAT_IN_FULLY_BOUND_STRUCTS),
     LintId::of(matches::TRY_ERR),
     LintId::of(matches::WILDCARD_ENUM_MATCH_ARM),

clippy_lints/src/lib.rs

@@ -269,6 +269,7 @@ mod manual_assert;
 mod manual_async_fn;
 mod manual_bits;
 mod manual_instant_elapsed;
+mod manual_let_else;
 mod manual_non_exhaustive;
 mod manual_rem_euclid;
 mod manual_retain;
@@ -610,6 +611,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
     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)));
+    store.register_late_pass(move || Box::new(manual_let_else::ManualLetElse::new(msrv)));
     store.register_early_pass(move || Box::new(redundant_static_lifetimes::RedundantStaticLifetimes::new(msrv)));
     store.register_early_pass(move || Box::new(redundant_field_names::RedundantFieldNames::new(msrv)));
     store.register_late_pass(move || Box::new(checked_conversions::CheckedConversions::new(msrv)));

clippy_lints/src/manual_let_else.rs

@@ -0,0 +1,165 @@
+use clippy_utils::diagnostics::span_lint;
+use clippy_utils::visitors::expr_visitor_no_bodies;
+use clippy_utils::{higher, meets_msrv, msrvs, peel_blocks};
+use if_chain::if_chain;
+use rustc_hir::intravisit::Visitor;
+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};
+
+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,
+    restriction,
+    "TODO"
+}
+
+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.as_ref(), &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;
+    expr_visitor_no_bodies(|ex| {
+        match ex.kind {
+            ExprKind::Continue(_) | ExprKind::Break(_, _) | ExprKind::Ret(_) => {
+                does_diverge = true;
+                false
+            },
+            ExprKind::Call(call, _) => {
+                if is_never(cx, ex) || is_never(cx, call) {
+                    does_diverge = true;
+                    return false;
+                }
+                true
+            },
+            ExprKind::MethodCall(..) => {
+                if is_never(cx, ex) {
+                    does_diverge = true;
+                    return false;
+                }
+                true
+            },
+            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;
+                }
+                false
+            },
+            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;
+                }
+                false
+            },
+
+            // Don't continue into loops, as they are breakable,
+            // and we'd have to start checking labels.
+            ExprKind::Loop(..) => false,
+
+            // Default: descend
+            // TODO: support breakable blocks once they are stable.
+            // For this we'd need to add label support.
+            _ => true,
+        }
+    })
+    .visit_expr(expr);
+    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.
+    /// 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, MANUAL_LET_ELSE.
     ///
     /// The minimum rust version that the project supports
     (msrv: Option<String> = None),

clippy_utils/src/msrvs.rs

@@ -12,9 +12,9 @@ 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,53,0 { OR_PATTERNS, MANUAL_BITS, BTREE_MAP_RETAIN, BTREE_SET_RETAIN, ARRAY_INTO_ITERATOR }
-    1,52,0 { STR_SPLIT_ONCE, REM_EUCLID_CONST }
     1,51,0 { BORROW_AS_PTR, UNSIGNED_ABS }
     1,50,0 { BOOL_THEN }
     1,47,0 { TAU }

src/docs.rs

@@ -259,6 +259,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 };
+```