Add if_chain lints

camsteffen · camsteffen · commit 5f887d09b87d · 2021-03-31T14:06:27.000-05:00
diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs
@@ -550,6 +550,8 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         #[cfg(feature = "internal-lints")]
         &utils::internal_lints::DEFAULT_LINT,
         #[cfg(feature = "internal-lints")]
+        &utils::internal_lints::IF_CHAIN_STYLE,
+        #[cfg(feature = "internal-lints")]
         &utils::internal_lints::INTERNING_DEFINED_SYMBOL,
         #[cfg(feature = "internal-lints")]
         &utils::internal_lints::INVALID_PATHS,
@@ -1026,6 +1028,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         store.register_late_pass(|| box utils::inspector::DeepCodeInspector);
         store.register_late_pass(|| box utils::internal_lints::CollapsibleCalls);
         store.register_late_pass(|| box utils::internal_lints::CompilerLintFunctions::new());
+        store.register_late_pass(|| box utils::internal_lints::IfChainStyle);
         store.register_late_pass(|| box utils::internal_lints::InvalidPaths);
         store.register_late_pass(|| box utils::internal_lints::InterningDefinedSymbol::default());
         store.register_late_pass(|| box utils::internal_lints::LintWithoutLintPass::default());
@@ -1442,6 +1445,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
         LintId::of(&utils::internal_lints::COLLAPSIBLE_SPAN_LINT_CALLS),
         LintId::of(&utils::internal_lints::COMPILER_LINT_FUNCTIONS),
         LintId::of(&utils::internal_lints::DEFAULT_LINT),
+        LintId::of(&utils::internal_lints::IF_CHAIN_STYLE),
         LintId::of(&utils::internal_lints::INTERNING_DEFINED_SYMBOL),
         LintId::of(&utils::internal_lints::INVALID_PATHS),
         LintId::of(&utils::internal_lints::LINT_WITHOUT_LINT_PASS),
diff --git a/clippy_lints/src/utils/internal_lints.rs b/clippy_lints/src/utils/internal_lints.rs
@@ -1,8 +1,10 @@
 use crate::consts::{constant_simple, Constant};
-use clippy_utils::diagnostics::{span_lint, span_lint_and_help, span_lint_and_sugg};
+use clippy_utils::diagnostics::{span_lint, span_lint_and_help, span_lint_and_sugg, span_lint_and_then};
 use clippy_utils::source::snippet;
 use clippy_utils::ty::match_type;
-use clippy_utils::{is_expn_of, match_def_path, match_qpath, method_calls, path_to_res, paths, run_lints, SpanlessEq};
+use clippy_utils::{
+    is_else_clause, is_expn_of, match_def_path, match_qpath, method_calls, path_to_res, paths, run_lints, SpanlessEq,
+};
 use if_chain::if_chain;
 use rustc_ast::ast::{Crate as AstCrate, ItemKind, LitKind, ModKind, NodeId};
 use rustc_ast::visit::FnKind;
@@ -14,15 +16,17 @@ use rustc_hir::def_id::DefId;
 use rustc_hir::hir_id::CRATE_HIR_ID;
 use rustc_hir::intravisit::{NestedVisitorMap, Visitor};
 use rustc_hir::{
-    BinOpKind, Crate, Expr, ExprKind, HirId, Item, MutTy, Mutability, Node, Path, StmtKind, Ty, TyKind, UnOp,
+    BinOpKind, Block, Crate, Expr, ExprKind, HirId, Item, Local, MatchSource, MutTy, Mutability, Node, Path, Stmt,
+    StmtKind, Ty, TyKind, UnOp,
 };
-use rustc_lint::{EarlyContext, EarlyLintPass, LateContext, LateLintPass};
+use rustc_lint::{EarlyContext, EarlyLintPass, LateContext, LateLintPass, LintContext};
 use rustc_middle::hir::map::Map;
 use rustc_middle::mir::interpret::ConstValue;
 use rustc_middle::ty;
 use rustc_session::{declare_lint_pass, declare_tool_lint, impl_lint_pass};
-use rustc_span::source_map::{Span, Spanned};
+use rustc_span::source_map::Spanned;
 use rustc_span::symbol::{Symbol, SymbolStr};
+use rustc_span::{BytePos, Span};
 use rustc_typeck::hir_ty_to_ty;
 
 use std::borrow::{Borrow, Cow};
@@ -297,6 +301,13 @@ declare_clippy_lint! {
     "unnecessary conversion between Symbol and string"
 }
 
+declare_clippy_lint! {
+    /// Finds unidiomatic usage of `if_chain!`
+    pub IF_CHAIN_STYLE,
+    internal,
+    "non-idiomatic `if_chain!` usage"
+}
+
 declare_lint_pass!(ClippyLintsInternal => [CLIPPY_LINTS_INTERNAL]);
 
 impl EarlyLintPass for ClippyLintsInternal {
@@ -1063,3 +1074,159 @@ impl<'tcx> SymbolStrExpr<'tcx> {
         }
     }
 }
+
+declare_lint_pass!(IfChainStyle => [IF_CHAIN_STYLE]);
+
+impl<'tcx> LateLintPass<'tcx> for IfChainStyle {
+    fn check_block(&mut self, cx: &LateContext<'tcx>, block: &'tcx hir::Block<'_>) {
+        let (local, after, if_chain_span) = if_chain! {
+            if let [Stmt { kind: StmtKind::Local(local), .. }, after @ ..] = block.stmts;
+            if let Some(if_chain_span) = is_expn_of(block.span, "if_chain");
+            then { (local, after, if_chain_span) } else { return }
+        };
+        if is_first_if_chain_expr(cx, block.hir_id, if_chain_span) {
+            span_lint(
+                cx,
+                IF_CHAIN_STYLE,
+                if_chain_local_span(cx, local, if_chain_span),
+                "`let` expression should be above the `if_chain!`",
+            );
+        } else if local.span.ctxt() == block.span.ctxt() && is_if_chain_then(after, block.expr, if_chain_span) {
+            span_lint(
+                cx,
+                IF_CHAIN_STYLE,
+                if_chain_local_span(cx, local, if_chain_span),
+                "`let` expression should be inside `then { .. }`",
+            )
+        }
+    }
+
+    fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx hir::Expr<'_>) {
+        let (cond, then, els) = match expr.kind {
+            ExprKind::If(cond, then, els) => (Some(cond), then, els.is_some()),
+            ExprKind::Match(
+                _,
+                [arm, ..],
+                MatchSource::IfLetDesugar {
+                    contains_else_clause: els,
+                },
+            ) => (None, arm.body, els),
+            _ => return,
+        };
+        let then_block = match then.kind {
+            ExprKind::Block(block, _) => block,
+            _ => return,
+        };
+        let if_chain_span = is_expn_of(expr.span, "if_chain");
+        if !els {
+            check_nested_if_chains(cx, expr, then_block, if_chain_span);
+        }
+        let if_chain_span = match if_chain_span {
+            None => return,
+            Some(span) => span,
+        };
+        // check for `if a && b;`
+        if_chain! {
+            if let Some(cond) = cond;
+            if let ExprKind::Binary(op, _, _) = cond.kind;
+            if op.node == BinOpKind::And;
+            if cx.sess().source_map().is_multiline(cond.span);
+            then {
+                span_lint(cx, IF_CHAIN_STYLE, cond.span, "`if a && b;` should be `if a; if b;`");
+            }
+        }
+        if is_first_if_chain_expr(cx, expr.hir_id, if_chain_span)
+            && is_if_chain_then(then_block.stmts, then_block.expr, if_chain_span)
+        {
+            span_lint(cx, IF_CHAIN_STYLE, expr.span, "`if_chain!` only has one `if`")
+        }
+    }
+}
+
+fn check_nested_if_chains(
+    cx: &LateContext<'_>,
+    if_expr: &Expr<'_>,
+    then_block: &Block<'_>,
+    if_chain_span: Option<Span>,
+) {
+    #[rustfmt::skip]
+    let (head, tail) = match *then_block {
+        Block { stmts, expr: Some(tail), .. } => (stmts, tail),
+        Block {
+            stmts: &[
+                ref head @ ..,
+                Stmt { kind: StmtKind::Expr(tail) | StmtKind::Semi(tail), .. }
+            ],
+            ..
+        } => (head, tail),
+        _ => return,
+    };
+    if_chain! {
+        if matches!(tail.kind,
+            ExprKind::If(_, _, None)
+            | ExprKind::Match(.., MatchSource::IfLetDesugar { contains_else_clause: false }));
+        let sm = cx.sess().source_map();
+        if head
+            .iter()
+            .all(|stmt| matches!(stmt.kind, StmtKind::Local(..)) && !sm.is_multiline(stmt.span));
+        if if_chain_span.is_some() || !is_else_clause(cx.tcx, if_expr);
+        then {} else { return }
+    }
+    let (span, msg) = match (if_chain_span, is_expn_of(tail.span, "if_chain")) {
+        (None, Some(_)) => (if_expr.span, "this `if` can be part of the inner `if_chain!`"),
+        (Some(_), None) => (tail.span, "this `if` can be part of the outer `if_chain!`"),
+        (Some(a), Some(b)) if a != b => (b, "this `if_chain!` can be merged with the outer `if_chain!`"),
+        _ => return,
+    };
+    span_lint_and_then(cx, IF_CHAIN_STYLE, span, msg, |diag| {
+        let (span, msg) = match head {
+            [] => return,
+            [stmt] => (stmt.span, "this `let` statement can also be in the `if_chain!`"),
+            [a, .., b] => (
+                a.span.to(b.span),
+                "these `let` statements can also be in the `if_chain!`",
+            ),
+        };
+        diag.span_help(span, msg);
+    });
+}
+
+fn is_first_if_chain_expr(cx: &LateContext<'_>, hir_id: HirId, if_chain_span: Span) -> bool {
+    cx.tcx
+        .hir()
+        .parent_iter(hir_id)
+        .find(|(_, node)| {
+            #[rustfmt::skip]
+            !matches!(node, Node::Expr(Expr { kind: ExprKind::Block(..), .. }) | Node::Stmt(_))
+        })
+        .map_or(false, |(id, _)| {
+            is_expn_of(cx.tcx.hir().span(id), "if_chain") != Some(if_chain_span)
+        })
+}
+
+/// Checks a trailing slice of statements and expression of a `Block` to see if they are part
+/// of the `then {..}` portion of an `if_chain!`
+fn is_if_chain_then(stmts: &[Stmt<'_>], expr: Option<&Expr<'_>>, if_chain_span: Span) -> bool {
+    let span = if let [stmt, ..] = stmts {
+        stmt.span
+    } else if let Some(expr) = expr {
+        expr.span
+    } else {
+        // empty `then {}`
+        return true;
+    };
+    is_expn_of(span, "if_chain").map_or(true, |span| span != if_chain_span)
+}
+
+/// Creates a `Span` for `let x = ..;` in an `if_chain!` call.
+fn if_chain_local_span(cx: &LateContext<'_>, local: &Local<'_>, if_chain_span: Span) -> Span {
+    let mut span = local.pat.span;
+    if let Some(init) = local.init {
+        span = span.to(init.span);
+    }
+    span.adjust(if_chain_span.ctxt().outer_expn());
+    let sm = cx.sess().source_map();
+    let span = sm.span_extend_to_prev_str(span, "let", false);
+    let span = sm.span_extend_to_next_char(span, ';', false);
+    Span::new(span.lo() - BytePos(3), span.hi() + BytePos(1), span.ctxt())
+}
diff --git a/tests/ui-internal/if_chain_style.rs b/tests/ui-internal/if_chain_style.rs
@@ -0,0 +1,92 @@
+#![warn(clippy::if_chain_style)]
+#![allow(clippy::no_effect)]
+
+extern crate if_chain;
+
+use if_chain::if_chain;
+
+fn main() {
+    if true {
+        let x = "";
+        // `if_chain!` inside `if`
+        if_chain! {
+            if true;
+            if true;
+            then {}
+        }
+    }
+    if_chain! {
+        if true
+            // multi-line AND'ed conditions
+            && false;
+        if let Some(1) = Some(1);
+        // `let` before `then`
+        let x = "";
+        then {
+            ();
+        }
+    }
+    if_chain! {
+        // single `if` condition
+        if true;
+        then {
+            let x = "";
+            // nested if
+            if true {}
+        }
+    }
+    if_chain! {
+        // starts with `let ..`
+        let x = "";
+        if let Some(1) = Some(1);
+        then {
+            let x = "";
+            let x = "";
+            // nested if_chain!
+            if_chain! {
+                if true;
+                if true;
+                then {}
+            }
+        }
+    }
+}
+
+fn negative() {
+    if true {
+        ();
+        if_chain! {
+            if true;
+            if true;
+            then { (); }
+        }
+    }
+    if_chain! {
+        if true;
+        let x = "";
+        if true;
+        then { (); }
+    }
+    if_chain! {
+        if true;
+        if true;
+        then {
+            if true { 1 } else { 2 }
+        } else {
+            3
+        }
+    };
+    if true {
+        if_chain! {
+            if true;
+            if true;
+            then {}
+        }
+    } else if false {
+        if_chain! {
+            if true;
+            if false;
+            then {}
+        }
+    }
+}
diff --git a/tests/ui-internal/if_chain_style.stderr b/tests/ui-internal/if_chain_style.stderr
