Add lint to tell about let else pattern

est31 · est31 · commit 13888649a7c3 · 2022-02-20T00:51:20.000+01:00
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -3249,6 +3249,7 @@ Released 2018-09-13
 [`manual_filter_map`]: https://rust-lang.github.io/rust-clippy/master/index.html#manual_filter_map
 [`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_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.register_lints.rs b/clippy_lints/src/lib.register_lints.rs
@@ -230,6 +230,7 @@ store.register_lints(&[
     manual_assert::MANUAL_ASSERT,
     manual_async_fn::MANUAL_ASYNC_FN,
     manual_bits::MANUAL_BITS,
+    manual_let_else::MANUAL_LET_ELSE,
     manual_map::MANUAL_MAP,
     manual_non_exhaustive::MANUAL_NON_EXHAUSTIVE,
     manual_ok_or::MANUAL_OK_OR,
diff --git a/clippy_lints/src/lib.register_restriction.rs b/clippy_lints/src/lib.register_restriction.rs
@@ -26,6 +26,7 @@ store.register_group(true, "clippy::restriction", Some("clippy_restriction"), ve
     LintId::of(integer_division::INTEGER_DIVISION),
     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(map_err_ignore::MAP_ERR_IGNORE),
     LintId::of(matches::REST_PAT_IN_FULLY_BOUND_STRUCTS),
     LintId::of(matches::WILDCARD_ENUM_MATCH_ARM),
diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs
@@ -265,6 +265,7 @@ mod main_recursion;
 mod manual_assert;
 mod manual_async_fn;
 mod manual_bits;
+mod manual_let_else;
 mod manual_map;
 mod manual_non_exhaustive;
 mod manual_ok_or;
@@ -570,6 +571,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_early_pass(move || Box::new(manual_non_exhaustive::ManualNonExhaustive::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)));
diff --git a/clippy_lints/src/manual_let_else.rs b/clippy_lints/src/manual_let_else.rs
@@ -0,0 +1,164 @@
+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` is 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
+}
diff --git a/clippy_lints/src/utils/conf.rs b/clippy_lints/src/utils/conf.rs
@@ -156,7 +156,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.
+    /// 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, 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,99,0 { LET_ELSE }
     1,53,0 { OR_PATTERNS, MANUAL_BITS }
     1,52,0 { STR_SPLIT_ONCE }
     1,51,0 { BORROW_AS_PTR }
diff --git a/tests/ui/manual_let_else.rs b/tests/ui/manual_let_else.rs
@@ -0,0 +1,150 @@
+#![allow(unused_braces, unused_variables, dead_code)]
+#![allow(clippy::collapsible_else_if, clippy::unused_unit)]
+#![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
+    } 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;
+        }
+    };
+
+    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
