Skip to content

Commit

Permalink
Update len_zero to handle comparisions with one
Browse files Browse the repository at this point in the history 
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
  • Loading branch information
NiekGr committed May 6, 2018
1 parent e6735dd commit e7a6b3e
Show file tree
Hide file tree
Showing 3 changed files with 158 additions and 58 deletions.
63 changes: 42 additions & 21 deletions clippy_lints/src/len_zero.rs
View file
@@ -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};
Expand Down Expand Up @@ -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
_ => (),
}
}
Expand Down Expand Up @@ -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, "_")),
);
Expand Down
73 changes: 55 additions & 18 deletions tests/ui/len_zero.rs
View file
@@ -1,6 +1,3 @@



#![warn(len_without_is_empty, len_zero)]
#![allow(dead_code, unused)]

Expand All @@ -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
}
Expand All @@ -39,15 +37,17 @@ 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
}
}

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
}
}
Expand Down Expand Up @@ -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
}
}
Expand All @@ -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!");
}

Expand All @@ -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;
Expand All @@ -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
Expand Down
80 changes: 61 additions & 19 deletions tests/ui/len_zero.stderr
View file
@@ -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`
Expand Down Expand Up @@ -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
Expand All @@ -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

0 comments on commit e7a6b3e

Please sign in to comment.