Mirrors/rust-clippy
2
0
Fork 0
mirror of https://github.com/rust-lang/rust-clippy synced 2024-11-30 00:20:36 +00:00
Code Issues Projects Releases Packages Wiki Activity

Update len_zero to handle comparisions with one

Browse source 
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
This commit is contained in:
NiekGr 2018-05-05 15:01:51 +02:00
parent e6735dd327
commit e7a6b3e613
3 changed files with 158 additions and 58 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

63
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};
@ -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, "_")),
);

73
tests/ui/len_zero.rs
View file

@ -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

80
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`
@ -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