diff --git a/CHANGELOG.md b/CHANGELOG.md index 25f3b5da198..008a22906fc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1665,6 +1665,7 @@ Released 2018-09-13 [`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 +[`comparison_to_empty`]: https://rust-lang.github.io/rust-clippy/master/index.html#comparison_to_empty [`copy_iterator`]: https://rust-lang.github.io/rust-clippy/master/index.html#copy_iterator [`create_dir`]: https://rust-lang.github.io/rust-clippy/master/index.html#create_dir [`crosspointer_transmute`]: https://rust-lang.github.io/rust-clippy/master/index.html#crosspointer_transmute diff --git a/clippy_lints/src/len_zero.rs b/clippy_lints/src/len_zero.rs index c9c4891bb08..8e2f03d6e4e 100644 --- a/clippy_lints/src/len_zero.rs +++ b/clippy_lints/src/len_zero.rs @@ -68,7 +68,44 @@ "traits or impls with a public `len` method but no corresponding `is_empty` method" } -declare_lint_pass!(LenZero => [LEN_ZERO, LEN_WITHOUT_IS_EMPTY]); +declare_clippy_lint! { + /// **What it does:** Checks for comparing to an empty slice such as "" or [],` + /// and suggests using `.is_empty()` where applicable. + /// + /// **Why is this bad?** Some structures can answer `.is_empty()` much faster + /// than checking for equality. So it is good to get into the habit of using + /// `.is_empty()`, and having it is cheap. + /// Besides, it makes the intent clearer than a manual comparison in some contexts. + /// + /// **Known problems:** None. + /// + /// **Example:** + /// + /// ```ignore + /// if s == "" { + /// .. + /// } + /// + /// if arr == [] { + /// .. + /// } + /// ``` + /// Use instead: + /// ```ignore + /// if s.is_empty() { + /// .. + /// } + /// + /// if arr.is_empty() { + /// .. + /// } + /// ``` + pub COMPARISON_TO_EMPTY, + style, + "checking `x == \"\"` or `x == []` (or similar) when `.is_empty()` could be used instead" +} + +declare_lint_pass!(LenZero => [LEN_ZERO, LEN_WITHOUT_IS_EMPTY, COMPARISON_TO_EMPTY]); impl<'tcx> LateLintPass<'tcx> for LenZero { fn check_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx Item<'_>) { @@ -221,6 +258,8 @@ fn check_cmp(cx: &LateContext<'_>, span: Span, method: &Expr<'_>, lit: &Expr<'_> } check_len(cx, span, method_path.ident.name, args, &lit.node, op, compare_to) + } else { + check_empty_expr(cx, span, method, lit, op) } } @@ -258,6 +297,42 @@ fn check_len( } } +fn check_empty_expr(cx: &LateContext<'_>, span: Span, lit1: &Expr<'_>, lit2: &Expr<'_>, op: &str) { + if (is_empty_array(lit2) || is_empty_string(lit2)) && has_is_empty(cx, lit1) { + let mut applicability = Applicability::MachineApplicable; + span_lint_and_sugg( + cx, + COMPARISON_TO_EMPTY, + span, + "comparison to empty slice", + &format!("using `{}is_empty` is clearer and more explicit", op), + format!( + "{}{}.is_empty()", + op, + snippet_with_applicability(cx, lit1.span, "_", &mut applicability) + ), + applicability, + ); + } +} + +fn is_empty_string(expr: &Expr<'_>) -> bool { + if let ExprKind::Lit(ref lit) = expr.kind { + if let LitKind::Str(lit, _) = lit.node { + let lit = lit.as_str(); + return lit == ""; + } + } + false +} + +fn is_empty_array(expr: &Expr<'_>) -> bool { + if let ExprKind::Array(ref arr) = expr.kind { + return arr.is_empty(); + } + false +} + /// Checks if this type has an `is_empty` method. fn has_is_empty(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool { /// Gets an `AssocItem` and return true if it matches `is_empty(self)`. diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 2d374969846..eb4d0bdc6c9 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -614,6 +614,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: &large_const_arrays::LARGE_CONST_ARRAYS, &large_enum_variant::LARGE_ENUM_VARIANT, &large_stack_arrays::LARGE_STACK_ARRAYS, + &len_zero::COMPARISON_TO_EMPTY, &len_zero::LEN_WITHOUT_IS_EMPTY, &len_zero::LEN_ZERO, &let_if_seq::USELESS_LET_IF_SEQ, @@ -1365,6 +1366,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: LintId::of(&int_plus_one::INT_PLUS_ONE), LintId::of(&large_const_arrays::LARGE_CONST_ARRAYS), LintId::of(&large_enum_variant::LARGE_ENUM_VARIANT), + LintId::of(&len_zero::COMPARISON_TO_EMPTY), LintId::of(&len_zero::LEN_WITHOUT_IS_EMPTY), LintId::of(&len_zero::LEN_ZERO), LintId::of(&let_underscore::LET_UNDERSCORE_LOCK), @@ -1591,6 +1593,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: LintId::of(&functions::RESULT_UNIT_ERR), LintId::of(&if_let_some_result::IF_LET_SOME_RESULT), LintId::of(&inherent_to_string::INHERENT_TO_STRING), + LintId::of(&len_zero::COMPARISON_TO_EMPTY), LintId::of(&len_zero::LEN_WITHOUT_IS_EMPTY), LintId::of(&len_zero::LEN_ZERO), LintId::of(&literal_representation::INCONSISTENT_DIGIT_GROUPING), diff --git a/src/lintlist/mod.rs b/src/lintlist/mod.rs index c2e63ecb581..4ecbedd71ce 100644 --- a/src/lintlist/mod.rs +++ b/src/lintlist/mod.rs @@ -298,6 +298,13 @@ deprecation: None, module: "comparison_chain", }, + Lint { + name: "comparison_to_empty", + group: "style", + desc: "checking `x == \"\"` or `x == []` (or similar) when `.is_empty()` could be used instead", + deprecation: None, + module: "len_zero", + }, Lint { name: "copy_iterator", group: "pedantic", diff --git a/tests/ui/comparison_to_empty.fixed b/tests/ui/comparison_to_empty.fixed new file mode 100644 index 00000000000..261024caca7 --- /dev/null +++ b/tests/ui/comparison_to_empty.fixed @@ -0,0 +1,23 @@ +// run-rustfix + +#![warn(clippy::comparison_to_empty)] + +fn main() { + // Disallow comparisons to empty + let s = String::new(); + let _ = s.is_empty(); + let _ = !s.is_empty(); + + let v = vec![0]; + let _ = v.is_empty(); + let _ = !v.is_empty(); + + // Allow comparisons to non-empty + let s = String::new(); + let _ = s == " "; + let _ = s != " "; + + let v = vec![0]; + let _ = v == [0]; + let _ = v != [0]; +} diff --git a/tests/ui/comparison_to_empty.rs b/tests/ui/comparison_to_empty.rs new file mode 100644 index 00000000000..98ddd974951 --- /dev/null +++ b/tests/ui/comparison_to_empty.rs @@ -0,0 +1,23 @@ +// run-rustfix + +#![warn(clippy::comparison_to_empty)] + +fn main() { + // Disallow comparisons to empty + let s = String::new(); + let _ = s == ""; + let _ = s != ""; + + let v = vec![0]; + let _ = v == []; + let _ = v != []; + + // Allow comparisons to non-empty + let s = String::new(); + let _ = s == " "; + let _ = s != " "; + + let v = vec![0]; + let _ = v == [0]; + let _ = v != [0]; +} diff --git a/tests/ui/comparison_to_empty.stderr b/tests/ui/comparison_to_empty.stderr new file mode 100644 index 00000000000..f69d6bd5255 --- /dev/null +++ b/tests/ui/comparison_to_empty.stderr @@ -0,0 +1,28 @@ +error: comparison to empty slice + --> $DIR/comparison_to_empty.rs:8:13 + | +LL | let _ = s == ""; + | ^^^^^^^ help: using `is_empty` is clearer and more explicit: `s.is_empty()` + | + = note: `-D clippy::comparison-to-empty` implied by `-D warnings` + +error: comparison to empty slice + --> $DIR/comparison_to_empty.rs:9:13 + | +LL | let _ = s != ""; + | ^^^^^^^ help: using `!is_empty` is clearer and more explicit: `!s.is_empty()` + +error: comparison to empty slice + --> $DIR/comparison_to_empty.rs:12:13 + | +LL | let _ = v == []; + | ^^^^^^^ help: using `is_empty` is clearer and more explicit: `v.is_empty()` + +error: comparison to empty slice + --> $DIR/comparison_to_empty.rs:13:13 + | +LL | let _ = v != []; + | ^^^^^^^ help: using `!is_empty` is clearer and more explicit: `!v.is_empty()` + +error: aborting due to 4 previous errors +