From e7a6b3e613ba7adf2b3e3bceec66846598fa7374 Mon Sep 17 00:00:00 2001 From: NiekGr Date: Sat, 5 May 2018 15:01:51 +0200 Subject: [PATCH] Update len_zero to handle comparisions with one I have added test cases for comparisons with zero and one. While implementing handling of one, incorrect handlings of zero were also fixed. fixes rust-lang-nursery/rust-clippy/#2554 --- clippy_lints/src/len_zero.rs | 63 ++++++++++++++++++---------- tests/ui/len_zero.rs | 73 ++++++++++++++++++++++++-------- tests/ui/len_zero.stderr | 80 +++++++++++++++++++++++++++--------- 3 files changed, 158 insertions(+), 58 deletions(-) diff --git a/clippy_lints/src/len_zero.rs b/clippy_lints/src/len_zero.rs index df3239ee1c7..09b3da1e99c 100644 --- a/clippy_lints/src/len_zero.rs +++ b/clippy_lints/src/len_zero.rs @@ -1,7 +1,7 @@ -use rustc::lint::*; use rustc::hir::def_id::DefId; -use rustc::ty; use rustc::hir::*; +use rustc::lint::*; +use rustc::ty; use std::collections::HashSet; use syntax::ast::{Lit, LitKind, Name}; use syntax::codemap::{Span, Spanned}; @@ -81,8 +81,24 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for LenZero { if let ExprBinary(Spanned { node: cmp, .. }, ref left, ref right) = expr.node { match cmp { - BiEq => check_cmp(cx, expr.span, left, right, ""), - BiGt | BiNe => check_cmp(cx, expr.span, left, right, "!"), + BiEq => { + check_cmp(cx, expr.span, left, right, "", 0); // len == 0 + check_cmp(cx, expr.span, right, left, "", 0); // 0 == len + }, + BiNe => { + check_cmp(cx, expr.span, left, right, "!", 0); // len != 0 + check_cmp(cx, expr.span, right, left, "!", 0); // 0 != len + }, + BiGt => { + check_cmp(cx, expr.span, left, right, "!", 0); // len > 0 + check_cmp(cx, expr.span, right, left, "", 1); // 1 > len + }, + BiLt => { + check_cmp(cx, expr.span, left, right, "", 1); // len < 1 + check_cmp(cx, expr.span, right, left, "!", 0); // 0 < len + }, + BiGe => check_cmp(cx, expr.span, left, right, "!", 1), // len <= 1 + BiLe => check_cmp(cx, expr.span, right, left, "!", 1), // 1 >= len _ => (), } } @@ -168,40 +184,45 @@ fn check_impl_items(cx: &LateContext, item: &Item, impl_items: &[ImplItemRef]) { cx, LEN_WITHOUT_IS_EMPTY, item.span, - &format!("item `{}` has a public `len` method but {} `is_empty` method", ty, is_empty), + &format!( + "item `{}` has a public `len` method but {} `is_empty` method", + ty, is_empty + ), ); } } } -fn check_cmp(cx: &LateContext, span: Span, left: &Expr, right: &Expr, op: &str) { - // check if we are in an is_empty() method - if let Some(name) = get_item_name(cx, left) { - if name == "is_empty" { - return; +fn check_cmp(cx: &LateContext, span: Span, method: &Expr, lit: &Expr, op: &str, compare_to: u32) { + if let (&ExprMethodCall(ref method_path, _, ref args), &ExprLit(ref lit)) = (&method.node, &lit.node) { + // check if we are in an is_empty() method + if let Some(name) = get_item_name(cx, method) { + if name == "is_empty" { + return; + } } - } - match (&left.node, &right.node) { - (&ExprLit(ref lit), &ExprMethodCall(ref method_path, _, ref args)) | - (&ExprMethodCall(ref method_path, _, ref args), &ExprLit(ref lit)) => { - check_len_zero(cx, span, method_path.name, args, lit, op) - }, - _ => (), + + check_len(cx, span, method_path.name, args, lit, op, compare_to) } } -fn check_len_zero(cx: &LateContext, span: Span, name: Name, args: &[Expr], lit: &Lit, op: &str) { +fn check_len(cx: &LateContext, span: Span, method_name: Name, args: &[Expr], lit: &Lit, op: &str, compare_to: u32) { if let Spanned { - node: LitKind::Int(0, _), + node: LitKind::Int(lit, _), .. } = *lit { - if name == "len" && args.len() == 1 && has_is_empty(cx, &args[0]) { + // check if length is compared to the specified number + if lit != u128::from(compare_to) { + return; + } + + if method_name == "len" && args.len() == 1 && has_is_empty(cx, &args[0]) { span_lint_and_sugg( cx, LEN_ZERO, span, - "length comparison to zero", + &format!("length comparison to {}", if compare_to == 0 { "zero" } else { "one" }), "using `is_empty` is more concise", format!("{}{}.is_empty()", op, snippet(cx, args[0].span, "_")), ); diff --git a/tests/ui/len_zero.rs b/tests/ui/len_zero.rs index aba1dd3055a..2e71c2761fa 100644 --- a/tests/ui/len_zero.rs +++ b/tests/ui/len_zero.rs @@ -1,6 +1,3 @@ - - - #![warn(len_without_is_empty, len_zero)] #![allow(dead_code, unused)] @@ -12,7 +9,8 @@ impl PubOne { } } -impl PubOne { // A second impl for this struct - the error span shouldn't mention this +impl PubOne { + // A second impl for this struct - the error span shouldn't mention this pub fn irrelevant(self: &Self) -> bool { false } @@ -39,7 +37,8 @@ impl PubAllowed { struct NotPubOne; impl NotPubOne { - pub fn len(self: &Self) -> isize { // no error, len is pub but `NotPubOne` is not exported anyway + pub fn len(self: &Self) -> isize { + // no error, len is pub but `NotPubOne` is not exported anyway 1 } } @@ -47,7 +46,8 @@ impl NotPubOne { struct One; impl One { - fn len(self: &Self) -> isize { // no error, len is private, see #1085 + fn len(self: &Self) -> isize { + // no error, len is private, see #1085 1 } } @@ -120,7 +120,7 @@ impl HasWrongIsEmpty { 1 } - pub fn is_empty(self: &Self, x : u32) -> bool { + pub fn is_empty(self: &Self, x: u32) -> bool { false } } @@ -129,28 +129,28 @@ pub trait Empty { fn is_empty(&self) -> bool; } -pub trait InheritingEmpty: Empty { //must not trigger LEN_WITHOUT_IS_EMPTY +pub trait InheritingEmpty: Empty { + //must not trigger LEN_WITHOUT_IS_EMPTY fn len(&self) -> isize; } - - fn main() { let x = [1, 2]; if x.len() == 0 { println!("This should not happen!"); } - if "".len() == 0 { - } + if "".len() == 0 {} let y = One; - if y.len() == 0 { //no error because One does not have .is_empty() + if y.len() == 0 { + //no error because One does not have .is_empty() println!("This should not happen either!"); } - let z : &TraitsToo = &y; - if z.len() > 0 { //no error, because TraitsToo has no .is_empty() method + let z: &TraitsToo = &y; + if z.len() > 0 { + //no error, because TraitsToo has no .is_empty() method println!("Nor should this!"); } @@ -164,6 +164,43 @@ fn main() { if has_is_empty.len() > 0 { println!("Or this!"); } + if has_is_empty.len() < 1 { + println!("Or this!"); + } + if has_is_empty.len() >= 1 { + println!("Or this!"); + } + if has_is_empty.len() > 1 { + // no error + println!("This can happen."); + } + if has_is_empty.len() <= 1 { + // no error + println!("This can happen."); + } + if 0 == has_is_empty.len() { + println!("Or this!"); + } + if 0 != has_is_empty.len() { + println!("Or this!"); + } + if 0 < has_is_empty.len() { + println!("Or this!"); + } + if 1 <= has_is_empty.len() { + println!("Or this!"); + } + if 1 > has_is_empty.len() { + println!("Or this!"); + } + if 1 < has_is_empty.len() { + // no error + println!("This can happen."); + } + if 1 >= has_is_empty.len() { + // no error + println!("This can happen."); + } assert!(!has_is_empty.is_empty()); let with_is_empty: &WithIsEmpty = &Wither; @@ -173,14 +210,14 @@ fn main() { assert!(!with_is_empty.is_empty()); let has_wrong_is_empty = HasWrongIsEmpty; - if has_wrong_is_empty.len() == 0 { //no error as HasWrongIsEmpty does not have .is_empty() + if has_wrong_is_empty.len() == 0 { + //no error as HasWrongIsEmpty does not have .is_empty() println!("Or this!"); } } fn test_slice(b: &[u8]) { - if b.len() != 0 { - } + if b.len() != 0 {} } // this used to ICE diff --git a/tests/ui/len_zero.stderr b/tests/ui/len_zero.stderr index 6e3cf1b3ca1..a04185bc63f 100644 --- a/tests/ui/len_zero.stderr +++ b/tests/ui/len_zero.stderr @@ -1,11 +1,11 @@ error: item `PubOne` has a public `len` method but no corresponding `is_empty` method - --> $DIR/len_zero.rs:9:1 + --> $DIR/len_zero.rs:6:1 | -9 | / impl PubOne { -10 | | pub fn len(self: &Self) -> isize { -11 | | 1 -12 | | } -13 | | } +6 | / impl PubOne { +7 | | pub fn len(self: &Self) -> isize { +8 | | 1 +9 | | } +10 | | } | |_^ | = note: `-D len-without-is-empty` implied by `-D warnings` @@ -43,17 +43,17 @@ error: item `HasWrongIsEmpty` has a public `len` method but no corresponding `is | |_^ error: length comparison to zero - --> $DIR/len_zero.rs:140:8 + --> $DIR/len_zero.rs:139:8 | -140 | if x.len() == 0 { +139 | if x.len() == 0 { | ^^^^^^^^^^^^ help: using `is_empty` is more concise: `x.is_empty()` | = note: `-D len-zero` implied by `-D warnings` error: length comparison to zero - --> $DIR/len_zero.rs:144:8 + --> $DIR/len_zero.rs:143:8 | -144 | if "".len() == 0 { +143 | if "".len() == 0 {} | ^^^^^^^^^^^^^ help: using `is_empty` is more concise: `"".is_empty()` error: length comparison to zero @@ -74,25 +74,67 @@ error: length comparison to zero 164 | if has_is_empty.len() > 0 { | ^^^^^^^^^^^^^^^^^^^^^^ help: using `is_empty` is more concise: `!has_is_empty.is_empty()` -error: length comparison to zero +error: length comparison to one + --> $DIR/len_zero.rs:167:8 + | +167 | if has_is_empty.len() < 1 { + | ^^^^^^^^^^^^^^^^^^^^^^ help: using `is_empty` is more concise: `has_is_empty.is_empty()` + +error: length comparison to one --> $DIR/len_zero.rs:170:8 | -170 | if with_is_empty.len() == 0 { +170 | if has_is_empty.len() >= 1 { + | ^^^^^^^^^^^^^^^^^^^^^^^ help: using `is_empty` is more concise: `!has_is_empty.is_empty()` + +error: length comparison to zero + --> $DIR/len_zero.rs:181:8 + | +181 | if 0 == has_is_empty.len() { + | ^^^^^^^^^^^^^^^^^^^^^^^ help: using `is_empty` is more concise: `has_is_empty.is_empty()` + +error: length comparison to zero + --> $DIR/len_zero.rs:184:8 + | +184 | if 0 != has_is_empty.len() { + | ^^^^^^^^^^^^^^^^^^^^^^^ help: using `is_empty` is more concise: `!has_is_empty.is_empty()` + +error: length comparison to zero + --> $DIR/len_zero.rs:187:8 + | +187 | if 0 < has_is_empty.len() { + | ^^^^^^^^^^^^^^^^^^^^^^ help: using `is_empty` is more concise: `!has_is_empty.is_empty()` + +error: length comparison to one + --> $DIR/len_zero.rs:190:8 + | +190 | if 1 <= has_is_empty.len() { + | ^^^^^^^^^^^^^^^^^^^^^^^ help: using `is_empty` is more concise: `!has_is_empty.is_empty()` + +error: length comparison to one + --> $DIR/len_zero.rs:193:8 + | +193 | if 1 > has_is_empty.len() { + | ^^^^^^^^^^^^^^^^^^^^^^ help: using `is_empty` is more concise: `has_is_empty.is_empty()` + +error: length comparison to zero + --> $DIR/len_zero.rs:207:8 + | +207 | if with_is_empty.len() == 0 { | ^^^^^^^^^^^^^^^^^^^^^^^^ help: using `is_empty` is more concise: `with_is_empty.is_empty()` error: length comparison to zero - --> $DIR/len_zero.rs:182:8 + --> $DIR/len_zero.rs:220:8 | -182 | if b.len() != 0 { +220 | if b.len() != 0 {} | ^^^^^^^^^^^^ help: using `is_empty` is more concise: `!b.is_empty()` error: trait `DependsOnFoo` has a `len` method but no (possibly inherited) `is_empty` method - --> $DIR/len_zero.rs:189:1 + --> $DIR/len_zero.rs:226:1 | -189 | / pub trait DependsOnFoo: Foo { -190 | | fn len(&mut self) -> usize; -191 | | } +226 | / pub trait DependsOnFoo: Foo { +227 | | fn len(&mut self) -> usize; +228 | | } | |_^ -error: aborting due to 12 previous errors +error: aborting due to 19 previous errors