From: James Wang Date: Tue, 24 Sep 2019 21:55:05 +0000 (-0500) Subject: Add a new lint for comparison chains X-Git-Url: https://git.lizzy.rs/?a=commitdiff_plain;h=52408f5b7d0392b008d647c5414e45dad4c4f5fd;p=rust.git Add a new lint for comparison chains --- diff --git a/CHANGELOG.md b/CHANGELOG.md index 3e2b441518d..ca0eb65ca3d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -962,6 +962,7 @@ Released 2018-09-13 [`cmp_owned`]: https://rust-lang.github.io/rust-clippy/master/index.html#cmp_owned [`cognitive_complexity`]: https://rust-lang.github.io/rust-clippy/master/index.html#cognitive_complexity [`collapsible_if`]: https://rust-lang.github.io/rust-clippy/master/index.html#collapsible_if +[`comparison_chain`]: https://rust-lang.github.io/rust-clippy/master/index.html#comparison_chain [`copy_iterator`]: https://rust-lang.github.io/rust-clippy/master/index.html#copy_iterator [`crosspointer_transmute`]: https://rust-lang.github.io/rust-clippy/master/index.html#crosspointer_transmute [`dbg_macro`]: https://rust-lang.github.io/rust-clippy/master/index.html#dbg_macro diff --git a/README.md b/README.md index 915396b901c..f944e716e9a 100644 --- a/README.md +++ b/README.md @@ -6,7 +6,7 @@ A collection of lints to catch common mistakes and improve your [Rust](https://github.com/rust-lang/rust) code. -[There are 316 lints included in this crate!](https://rust-lang.github.io/rust-clippy/master/index.html) +[There are 317 lints included in this crate!](https://rust-lang.github.io/rust-clippy/master/index.html) We have a bunch of lint categories to allow you to choose how much Clippy is supposed to ~~annoy~~ help you: diff --git a/clippy_lints/src/comparison_chain.rs b/clippy_lints/src/comparison_chain.rs new file mode 100644 index 00000000000..bac6ba55d58 --- /dev/null +++ b/clippy_lints/src/comparison_chain.rs @@ -0,0 +1,107 @@ +use crate::utils::{if_sequence, parent_node_is_if_expr, span_help_and_lint, SpanlessEq}; +use rustc::hir::*; +use rustc::lint::{LateContext, LateLintPass, LintArray, LintPass}; +use rustc::{declare_lint_pass, declare_tool_lint}; + +declare_clippy_lint! { + /// **What it does:** Checks comparison chains written with `if` that can be + /// rewritten with `match` and `cmp`. + /// + /// **Why is this bad?** `if` is not guaranteed to be exhaustive and conditionals can get + /// repetitive + /// + /// **Known problems:** None. + /// + /// **Example:** + /// ```rust,ignore + /// # fn a() {} + /// # fn b() {} + /// # fn c() {} + /// fn f(x: u8, y: u8) { + /// if x > y { + /// a() + /// } else if x < y { + /// b() + /// } else { + /// c() + /// } + /// } + /// ``` + /// + /// Could be written: + /// + /// ```rust,ignore + /// use std::cmp::Ordering; + /// # fn a() {} + /// # fn b() {} + /// # fn c() {} + /// fn f(x: u8, y: u8) { + /// match x.cmp(y) { + /// Ordering::Greater => a(), + /// Ordering::Less => b(), + /// Ordering::Equal => c() + /// } + /// } + /// ``` + pub COMPARISON_CHAIN, + style, + "`if`s that can be rewritten with `match` and `cmp`" +} + +declare_lint_pass!(ComparisonChain => [COMPARISON_CHAIN]); + +impl<'a, 'tcx> LateLintPass<'a, 'tcx> for ComparisonChain { + fn check_expr(&mut self, cx: &LateContext<'a, 'tcx>, expr: &'tcx Expr) { + if expr.span.from_expansion() { + return; + } + + // We only care about the top-most `if` in the chain + if parent_node_is_if_expr(expr, cx) { + return; + } + + // Check that there exists at least one explicit else condition + let (conds, _) = if_sequence(expr); + if conds.len() < 2 { + return; + } + + for cond in conds.windows(2) { + if let ( + &ExprKind::Binary(ref kind1, ref lhs1, ref rhs1), + &ExprKind::Binary(ref kind2, ref lhs2, ref rhs2), + ) = (&cond[0].node, &cond[1].node) + { + if !kind_is_cmp(kind1.node) || !kind_is_cmp(kind2.node) { + return; + } + + // Check that both sets of operands are equal + let mut spanless_eq = SpanlessEq::new(cx); + if (!spanless_eq.eq_expr(lhs1, lhs2) || !spanless_eq.eq_expr(rhs1, rhs2)) + && (!spanless_eq.eq_expr(lhs1, rhs2) || !spanless_eq.eq_expr(rhs1, lhs2)) + { + return; + } + } else { + // We only care about comparison chains + return; + } + } + span_help_and_lint( + cx, + COMPARISON_CHAIN, + expr.span, + "`if` chain can be rewritten with `match`", + "Consider rewriting the `if` chain to use `cmp` and `match`.", + ) + } +} + +fn kind_is_cmp(kind: BinOpKind) -> bool { + match kind { + BinOpKind::Lt | BinOpKind::Gt | BinOpKind::Eq => true, + _ => false, + } +} diff --git a/clippy_lints/src/copies.rs b/clippy_lints/src/copies.rs index b01ce7eeb77..38654c753bf 100644 --- a/clippy_lints/src/copies.rs +++ b/clippy_lints/src/copies.rs @@ -1,11 +1,11 @@ -use crate::utils::{get_parent_expr, higher, same_tys, snippet, span_lint_and_then, span_note_and_lint}; +use crate::utils::{get_parent_expr, higher, if_sequence, same_tys, snippet, span_lint_and_then, span_note_and_lint}; use crate::utils::{SpanlessEq, SpanlessHash}; use rustc::hir::*; use rustc::lint::{LateContext, LateLintPass, LintArray, LintPass}; use rustc::ty::Ty; use rustc::{declare_lint_pass, declare_tool_lint}; use rustc_data_structures::fx::FxHashMap; -use smallvec::SmallVec; +use std::cmp::Ordering; use std::collections::hash_map::Entry; use std::hash::BuildHasherDefault; use syntax::symbol::Symbol; @@ -241,39 +241,6 @@ fn same_bindings<'tcx>( } } -/// Returns the list of condition expressions and the list of blocks in a -/// sequence of `if/else`. -/// E.g., this returns `([a, b], [c, d, e])` for the expression -/// `if a { c } else if b { d } else { e }`. -fn if_sequence(mut expr: &Expr) -> (SmallVec<[&Expr; 1]>, SmallVec<[&Block; 1]>) { - let mut conds = SmallVec::new(); - let mut blocks: SmallVec<[&Block; 1]> = SmallVec::new(); - - while let Some((ref cond, ref then_expr, ref else_expr)) = higher::if_block(&expr) { - conds.push(&**cond); - if let ExprKind::Block(ref block, _) = then_expr.node { - blocks.push(block); - } else { - panic!("ExprKind::If node is not an ExprKind::Block"); - } - - if let Some(ref else_expr) = *else_expr { - expr = else_expr; - } else { - break; - } - } - - // final `else {..}` - if !blocks.is_empty() { - if let ExprKind::Block(ref block, _) = expr.node { - blocks.push(&**block); - } - } - - (conds, blocks) -} - /// Returns the list of bindings in a pattern. fn bindings<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, pat: &Pat) -> FxHashMap> { fn bindings_impl<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, pat: &Pat, map: &mut FxHashMap>) { @@ -338,16 +305,15 @@ fn search_common_cases<'a, T, Eq>(exprs: &'a [T], eq: &Eq) -> Option<(&'a T, &'a where Eq: Fn(&T, &T) -> bool, { - if exprs.len() < 2 { - None - } else if exprs.len() == 2 { - if eq(&exprs[0], &exprs[1]) { - Some((&exprs[0], &exprs[1])) - } else { - None - } - } else { - None + match exprs.len().cmp(&2) { + Ordering::Greater | Ordering::Less => None, + Ordering::Equal => { + if eq(&exprs[0], &exprs[1]) { + Some((&exprs[0], &exprs[1])) + } else { + None + } + }, } } diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 2caa065b937..ed47b6a1857 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -159,6 +159,7 @@ macro_rules! declare_clippy_lint { pub mod checked_conversions; pub mod cognitive_complexity; pub mod collapsible_if; +pub mod comparison_chain; pub mod copies; pub mod copy_iterator; pub mod dbg_macro; @@ -600,6 +601,7 @@ pub fn register_plugins(reg: &mut rustc_driver::plugin::Registry<'_>, conf: &Con reg.register_late_lint_pass(box integer_division::IntegerDivision); reg.register_late_lint_pass(box inherent_to_string::InherentToString); reg.register_late_lint_pass(box trait_bounds::TraitBounds); + reg.register_late_lint_pass(box comparison_chain::ComparisonChain); reg.register_lint_group("clippy::restriction", Some("clippy_restriction"), vec![ arithmetic::FLOAT_ARITHMETIC, @@ -706,6 +708,7 @@ pub fn register_plugins(reg: &mut rustc_driver::plugin::Registry<'_>, conf: &Con bytecount::NAIVE_BYTECOUNT, cognitive_complexity::COGNITIVE_COMPLEXITY, collapsible_if::COLLAPSIBLE_IF, + comparison_chain::COMPARISON_CHAIN, copies::IFS_SAME_COND, copies::IF_SAME_THEN_ELSE, derive::DERIVE_HASH_XOR_EQ, @@ -932,6 +935,7 @@ pub fn register_plugins(reg: &mut rustc_driver::plugin::Registry<'_>, conf: &Con block_in_if_condition::BLOCK_IN_IF_CONDITION_EXPR, block_in_if_condition::BLOCK_IN_IF_CONDITION_STMT, collapsible_if::COLLAPSIBLE_IF, + comparison_chain::COMPARISON_CHAIN, doc::MISSING_SAFETY_DOC, enum_variants::ENUM_VARIANT_NAMES, enum_variants::MODULE_INCEPTION, diff --git a/clippy_lints/src/needless_bool.rs b/clippy_lints/src/needless_bool.rs index b761457f64c..d19939be6c0 100644 --- a/clippy_lints/src/needless_bool.rs +++ b/clippy_lints/src/needless_bool.rs @@ -3,7 +3,7 @@ //! This lint is **warn** by default use crate::utils::sugg::Sugg; -use crate::utils::{higher, span_lint, span_lint_and_sugg}; +use crate::utils::{higher, parent_node_is_if_expr, span_lint, span_lint_and_sugg}; use rustc::hir::*; use rustc::lint::{LateContext, LateLintPass, LintArray, LintPass}; use rustc::{declare_lint_pass, declare_tool_lint}; @@ -118,17 +118,6 @@ fn check_expr(&mut self, cx: &LateContext<'a, 'tcx>, e: &'tcx Expr) { } } -fn parent_node_is_if_expr<'a, 'b>(expr: &Expr, cx: &LateContext<'a, 'b>) -> bool { - let parent_id = cx.tcx.hir().get_parent_node(expr.hir_id); - let parent_node = cx.tcx.hir().get(parent_id); - - match parent_node { - rustc::hir::Node::Expr(e) => higher::if_block(&e).is_some(), - rustc::hir::Node::Arm(e) => higher::if_block(&e.body).is_some(), - _ => false, - } -} - declare_lint_pass!(BoolComparison => [BOOL_COMPARISON]); impl<'a, 'tcx> LateLintPass<'a, 'tcx> for BoolComparison { diff --git a/clippy_lints/src/non_expressive_names.rs b/clippy_lints/src/non_expressive_names.rs index 88bf52b1e8d..ff06eaca465 100644 --- a/clippy_lints/src/non_expressive_names.rs +++ b/clippy_lints/src/non_expressive_names.rs @@ -1,6 +1,7 @@ use crate::utils::{span_lint, span_lint_and_then}; use rustc::lint::{EarlyContext, EarlyLintPass, LintArray, LintPass}; use rustc::{declare_tool_lint, impl_lint_pass}; +use std::cmp::Ordering; use syntax::ast::*; use syntax::attr; use syntax::source_map::Span; @@ -206,63 +207,67 @@ fn check_ident(&mut self, ident: Ident) { continue; } let mut split_at = None; - if existing_name.len > count { - if existing_name.len - count != 1 || levenstein_not_1(&interned_name, &existing_name.interned) { - continue; - } - } else if existing_name.len < count { - if count - existing_name.len != 1 || levenstein_not_1(&existing_name.interned, &interned_name) { - continue; - } - } else { - let mut interned_chars = interned_name.chars(); - let mut existing_chars = existing_name.interned.chars(); - let first_i = interned_chars.next().expect("we know we have at least one char"); - let first_e = existing_chars.next().expect("we know we have at least one char"); - let eq_or_numeric = |(a, b): (char, char)| a == b || a.is_numeric() && b.is_numeric(); + match existing_name.len.cmp(&count) { + Ordering::Greater => { + if existing_name.len - count != 1 || levenstein_not_1(&interned_name, &existing_name.interned) { + continue; + } + }, + Ordering::Less => { + if count - existing_name.len != 1 || levenstein_not_1(&existing_name.interned, &interned_name) { + continue; + } + }, + Ordering::Equal => { + let mut interned_chars = interned_name.chars(); + let mut existing_chars = existing_name.interned.chars(); + let first_i = interned_chars.next().expect("we know we have at least one char"); + let first_e = existing_chars.next().expect("we know we have at least one char"); + let eq_or_numeric = |(a, b): (char, char)| a == b || a.is_numeric() && b.is_numeric(); - if eq_or_numeric((first_i, first_e)) { - let last_i = interned_chars.next_back().expect("we know we have at least two chars"); - let last_e = existing_chars.next_back().expect("we know we have at least two chars"); - if eq_or_numeric((last_i, last_e)) { - if interned_chars - .zip(existing_chars) - .filter(|&ie| !eq_or_numeric(ie)) - .count() - != 1 - { - continue; + if eq_or_numeric((first_i, first_e)) { + let last_i = interned_chars.next_back().expect("we know we have at least two chars"); + let last_e = existing_chars.next_back().expect("we know we have at least two chars"); + if eq_or_numeric((last_i, last_e)) { + if interned_chars + .zip(existing_chars) + .filter(|&ie| !eq_or_numeric(ie)) + .count() + != 1 + { + continue; + } + } else { + let second_last_i = interned_chars + .next_back() + .expect("we know we have at least three chars"); + let second_last_e = existing_chars + .next_back() + .expect("we know we have at least three chars"); + if !eq_or_numeric((second_last_i, second_last_e)) + || second_last_i == '_' + || !interned_chars.zip(existing_chars).all(eq_or_numeric) + { + // allowed similarity foo_x, foo_y + // or too many chars differ (foo_x, boo_y) or (foox, booy) + continue; + } + split_at = interned_name.char_indices().rev().next().map(|(i, _)| i); } } else { - let second_last_i = interned_chars - .next_back() - .expect("we know we have at least three chars"); - let second_last_e = existing_chars - .next_back() - .expect("we know we have at least three chars"); - if !eq_or_numeric((second_last_i, second_last_e)) - || second_last_i == '_' + let second_i = interned_chars.next().expect("we know we have at least two chars"); + let second_e = existing_chars.next().expect("we know we have at least two chars"); + if !eq_or_numeric((second_i, second_e)) + || second_i == '_' || !interned_chars.zip(existing_chars).all(eq_or_numeric) { - // allowed similarity foo_x, foo_y - // or too many chars differ (foo_x, boo_y) or (foox, booy) + // allowed similarity x_foo, y_foo + // or too many chars differ (x_foo, y_boo) or (xfoo, yboo) continue; } - split_at = interned_name.char_indices().rev().next().map(|(i, _)| i); + split_at = interned_name.chars().next().map(char::len_utf8); } - } else { - let second_i = interned_chars.next().expect("we know we have at least two chars"); - let second_e = existing_chars.next().expect("we know we have at least two chars"); - if !eq_or_numeric((second_i, second_e)) - || second_i == '_' - || !interned_chars.zip(existing_chars).all(eq_or_numeric) - { - // allowed similarity x_foo, y_foo - // or too many chars differ (x_foo, y_boo) or (xfoo, yboo) - continue; - } - split_at = interned_name.chars().next().map(char::len_utf8); - } + }, } span_lint_and_then( self.0.cx, diff --git a/clippy_lints/src/utils/mod.rs b/clippy_lints/src/utils/mod.rs index 9165f8d74d7..49d9de35e18 100644 --- a/clippy_lints/src/utils/mod.rs +++ b/clippy_lints/src/utils/mod.rs @@ -1168,3 +1168,47 @@ pub fn match_def_path<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, did: DefId, syms: &[ let path = cx.get_def_path(did); path.len() == syms.len() && path.into_iter().zip(syms.iter()).all(|(a, &b)| a.as_str() == b) } + +/// Returns the list of condition expressions and the list of blocks in a +/// sequence of `if/else`. +/// E.g., this returns `([a, b], [c, d, e])` for the expression +/// `if a { c } else if b { d } else { e }`. +pub fn if_sequence(mut expr: &Expr) -> (SmallVec<[&Expr; 1]>, SmallVec<[&Block; 1]>) { + let mut conds = SmallVec::new(); + let mut blocks: SmallVec<[&Block; 1]> = SmallVec::new(); + + while let Some((ref cond, ref then_expr, ref else_expr)) = higher::if_block(&expr) { + conds.push(&**cond); + if let ExprKind::Block(ref block, _) = then_expr.node { + blocks.push(block); + } else { + panic!("ExprKind::If node is not an ExprKind::Block"); + } + + if let Some(ref else_expr) = *else_expr { + expr = else_expr; + } else { + break; + } + } + + // final `else {..}` + if !blocks.is_empty() { + if let ExprKind::Block(ref block, _) = expr.node { + blocks.push(&**block); + } + } + + (conds, blocks) +} + +pub fn parent_node_is_if_expr<'a, 'b>(expr: &Expr, cx: &LateContext<'a, 'b>) -> bool { + let parent_id = cx.tcx.hir().get_parent_node(expr.hir_id); + let parent_node = cx.tcx.hir().get(parent_id); + + match parent_node { + rustc::hir::Node::Expr(e) => higher::if_block(&e).is_some(), + rustc::hir::Node::Arm(e) => higher::if_block(&e.body).is_some(), + _ => false, + } +} diff --git a/src/lintlist/mod.rs b/src/lintlist/mod.rs index e6bcbfadf4f..1b838b17f7f 100644 --- a/src/lintlist/mod.rs +++ b/src/lintlist/mod.rs @@ -6,7 +6,7 @@ pub use lint::LINT_LEVELS; // begin lint list, do not remove this comment, it’s used in `update_lints` -pub const ALL_LINTS: [Lint; 316] = [ +pub const ALL_LINTS: [Lint; 317] = [ Lint { name: "absurd_extreme_comparisons", group: "correctness", @@ -252,6 +252,13 @@ deprecation: None, module: "collapsible_if", }, + Lint { + name: "comparison_chain", + group: "style", + desc: "`if`s that can be rewritten with `match` and `cmp`", + deprecation: None, + module: "comparison_chain", + }, Lint { name: "copy_iterator", group: "pedantic", diff --git a/tests/ui/comparison_chain.rs b/tests/ui/comparison_chain.rs new file mode 100644 index 00000000000..b697413b6e0 --- /dev/null +++ b/tests/ui/comparison_chain.rs @@ -0,0 +1,79 @@ +#![allow(dead_code)] +#![warn(clippy::comparison_chain)] + +fn a() {} +fn b() {} +fn c() {} + +fn f(x: u8, y: u8, z: u8) { + // Ignored: Only one branch + if x > y { + a() + } + + if x > y { + a() + } else if x < y { + b() + } + + // Ignored: Only one explicit conditional + if x > y { + a() + } else { + b() + } + + if x > y { + a() + } else if x < y { + b() + } else { + c() + } + + if x > y { + a() + } else if y > x { + b() + } else { + c() + } + + if x > 1 { + a() + } else if x < 1 { + b() + } else if x == 1 { + c() + } + + // Ignored: Binop args are not equivalent + if x > 1 { + a() + } else if y > 1 { + b() + } else { + c() + } + + // Ignored: Binop args are not equivalent + if x > y { + a() + } else if x > z { + b() + } else if y > z { + c() + } + + // Ignored: Not binary comparisons + if true { + a() + } else if false { + b() + } else { + c() + } +} + +fn main() {} diff --git a/tests/ui/comparison_chain.stderr b/tests/ui/comparison_chain.stderr new file mode 100644 index 00000000000..575181dd719 --- /dev/null +++ b/tests/ui/comparison_chain.stderr @@ -0,0 +1,57 @@ +error: `if` chain can be rewritten with `match` + --> $DIR/comparison_chain.rs:14:5 + | +LL | / if x > y { +LL | | a() +LL | | } else if x < y { +LL | | b() +LL | | } + | |_____^ + | + = note: `-D clippy::comparison-chain` implied by `-D warnings` + = help: Consider rewriting the `if` chain to use `cmp` and `match`. + +error: `if` chain can be rewritten with `match` + --> $DIR/comparison_chain.rs:27:5 + | +LL | / if x > y { +LL | | a() +LL | | } else if x < y { +LL | | b() +LL | | } else { +LL | | c() +LL | | } + | |_____^ + | + = help: Consider rewriting the `if` chain to use `cmp` and `match`. + +error: `if` chain can be rewritten with `match` + --> $DIR/comparison_chain.rs:35:5 + | +LL | / if x > y { +LL | | a() +LL | | } else if y > x { +LL | | b() +LL | | } else { +LL | | c() +LL | | } + | |_____^ + | + = help: Consider rewriting the `if` chain to use `cmp` and `match`. + +error: `if` chain can be rewritten with `match` + --> $DIR/comparison_chain.rs:43:5 + | +LL | / if x > 1 { +LL | | a() +LL | | } else if x < 1 { +LL | | b() +LL | | } else if x == 1 { +LL | | c() +LL | | } + | |_____^ + | + = help: Consider rewriting the `if` chain to use `cmp` and `match`. + +error: aborting due to 4 previous errors + diff --git a/tests/ui/crashes/if_same_then_else.rs b/tests/ui/crashes/if_same_then_else.rs index b2a4d541f59..4ef992b05e7 100644 --- a/tests/ui/crashes/if_same_then_else.rs +++ b/tests/ui/crashes/if_same_then_else.rs @@ -1,5 +1,6 @@ // run-pass +#![allow(clippy::comparison_chain)] #![deny(clippy::if_same_then_else)] /// Test for https://github.com/rust-lang/rust-clippy/issues/2426 diff --git a/tests/ui/ifs_same_cond.rs b/tests/ui/ifs_same_cond.rs index b67e730b937..80e9839ff40 100644 --- a/tests/ui/ifs_same_cond.rs +++ b/tests/ui/ifs_same_cond.rs @@ -1,5 +1,5 @@ #![warn(clippy::ifs_same_cond)] -#![allow(clippy::if_same_then_else)] // all empty blocks +#![allow(clippy::if_same_then_else, clippy::comparison_chain)] // all empty blocks fn ifs_same_cond() { let a = 0;