Skip to content

Update len_zero to handle comparisions with one #2730

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from May 7, 2018
Merged

Update len_zero to handle comparisions with one #2730

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
63 changes: 42 additions & 21 deletions clippy_lints/src/len_zero.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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
Open in desktop
Original file line number Diff line number Diff line change
@@ -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
Open in desktop
Original file line number Diff line number Diff line change
@@ -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