Merge pull request #1210 from Manishearth/len_without_is_empty

Len without is empty

Author: llogiq

CHANGELOG.md

@@ -2,7 +2,11 @@
 All notable changes to this project will be documented in this file.
 
 ## 0.0.88 — ??
-* The following lints are not new but were missing from the [`clippy`] and [`clippy_pedantic`] groups: [`filter_next`], [`for_loop_over_option`], [`for_loop_over_result`], [`match_overlapping_arm`].
+* The following lints are not new but were only usable through the `clippy`
+  lint groups: [`filter_next`], [`for_loop_over_option`],
+  [`for_loop_over_result`] and [`match_overlapping_arm`]. You should now be
+  able to `#[allow/deny]` them individually and they are available directly
+  through [`cargo clippy`].
 
 ## 0.0.87 — 2016-08-31
 * Rustup to *rustc 1.13.0-nightly (eac41469d 2016-08-30)*

README.md

@@ -84,7 +84,7 @@ name
 [items_after_statements](https://github.com/Manishearth/rust-clippy/wiki#items_after_statements)                     | allow   | blocks where an item comes after a statement
 [iter_next_loop](https://github.com/Manishearth/rust-clippy/wiki#iter_next_loop)                                     | warn    | for-looping over `_.next()` which is probably not intended
 [iter_nth](https://github.com/Manishearth/rust-clippy/wiki#iter_nth)                                                 | warn    | using `.iter().nth()` on a standard library type with O(1) element access
-[len_without_is_empty](https://github.com/Manishearth/rust-clippy/wiki#len_without_is_empty)                         | warn    | traits and impls that have `.len()` but not `.is_empty()`
+[len_without_is_empty](https://github.com/Manishearth/rust-clippy/wiki#len_without_is_empty)                         | warn    | traits or impls with a public `len` method but no corresponding `is_empty` method
 [len_zero](https://github.com/Manishearth/rust-clippy/wiki#len_zero)                                                 | warn    | checking `.len() == 0` or `.len() > 0` (or similar) when `.is_empty()` could be used instead
 [let_and_return](https://github.com/Manishearth/rust-clippy/wiki#let_and_return)                                     | warn    | creating a let-binding and then immediately returning it like `let x = expr; x` at the end of a block
 [let_unit_value](https://github.com/Manishearth/rust-clippy/wiki#let_unit_value)                                     | warn    | creating a let binding to a value of unit type, which usually can't be used afterwards

clippy_lints/src/len_zero.rs

@@ -42,13 +42,13 @@ declare_lint! {
 /// **Example:**
 /// ```rust
 /// impl X {
-///     fn len(&self) -> usize { .. }
+///     pub fn len(&self) -> usize { .. }
 /// }
 /// ```
 declare_lint! {
     pub LEN_WITHOUT_IS_EMPTY,
     Warn,
-    "traits and impls that have `.len()` but not `.is_empty()`"
+    "traits or impls with a public `len` method but no corresponding `is_empty` method"
 }
 
 #[derive(Copy,Clone)]
@@ -99,13 +99,12 @@ fn check_trait_items(cx: &LateContext, item: &Item, trait_items: &[TraitItem]) {
     }
 
     if !trait_items.iter().any(|i| is_named_self(i, "is_empty")) {
-        for i in trait_items {
-            if is_named_self(i, "len") {
+        if let Some(i) = trait_items.iter().find(|i| is_named_self(i, "len")) {
+            if cx.access_levels.is_exported(i.id) {
                 span_lint(cx,
                           LEN_WITHOUT_IS_EMPTY,
                           i.span,
-                          &format!("trait `{}` has a `.len(_: &Self)` method, but no `.is_empty(_: &Self)` method. \
-                                    Consider adding one",
+                          &format!("trait `{}` has a `len` method but no `is_empty` method",
                                    item.name));
             }
         }
@@ -122,19 +121,26 @@ fn check_impl_items(cx: &LateContext, item: &Item, impl_items: &[ImplItem]) {
         }
     }
 
-    if !impl_items.iter().any(|i| is_named_self(i, "is_empty")) {
-        for i in impl_items {
-            if is_named_self(i, "len") {
-                let ty = cx.tcx.node_id_to_type(item.id);
-
-                span_lint(cx,
-                          LEN_WITHOUT_IS_EMPTY,
-                          i.span,
-                          &format!("item `{}` has a `.len(_: &Self)` method, but no `.is_empty(_: &Self)` method. \
-                                    Consider adding one",
-                                   ty));
-                return;
-            }
+    let is_empty = if let Some(is_empty) = impl_items.iter().find(|i| is_named_self(i, "is_empty")) {
+        if cx.access_levels.is_exported(is_empty.id) {
+            return;
+        } else {
+            "a private"
+        }
+    } else {
+        "no corresponding"
+    };
+
+    if let Some(i) = impl_items.iter().find(|i| is_named_self(i, "len")) {
+        if cx.access_levels.is_exported(i.id) {
+            let ty = cx.tcx.node_id_to_type(item.id);
+
+            span_lint(cx,
+                      LEN_WITHOUT_IS_EMPTY,
+                      i.span,
+                      &format!("item `{}` has a public `len` method but {} `is_empty` method",
+                               ty,
+                               is_empty));
         }
     }
 }

tests/compile-fail/len_zero.rs

@@ -1,18 +1,45 @@
 #![feature(plugin)]
 #![plugin(clippy)]
 
+#![deny(len_without_is_empty, len_zero)]
+#![allow(dead_code, unused)]
+
+pub struct PubOne;
+
+impl PubOne {
+    pub fn len(self: &Self) -> isize { //~ERROR item `PubOne` has a public `len` method but no corresponding `is_empty`
+        1
+    }
+}
+
+struct NotPubOne;
+
+impl NotPubOne {
+    pub fn len(self: &Self) -> isize { // no error, len is pub but `NotPubOne` is not exported anyway
+        1
+    }
+}
+
 struct One;
 
-#[deny(len_without_is_empty)]
 impl One {
-    fn len(self: &Self) -> isize { //~ERROR item `One` has a `.len(_: &Self)`
+    fn len(self: &Self) -> isize { // no error, len is private, see #1085
         1
     }
 }
 
-#[deny(len_without_is_empty)]
+pub trait PubTraitsToo {
+    fn len(self: &Self) -> isize; //~ERROR trait `PubTraitsToo` has a `len` method but no `is_empty`
+}
+
+impl PubTraitsToo for One {
+    fn len(self: &Self) -> isize {
+        0
+    }
+}
+
 trait TraitsToo {
-    fn len(self: &Self) -> isize; //~ERROR trait `TraitsToo` has a `.len(_:
+    fn len(self: &Self) -> isize; // no error, len is private, see #1085
 }
 
 impl TraitsToo for One {
@@ -21,11 +48,22 @@ impl TraitsToo for One {
     }
 }
 
-struct HasIsEmpty;
+struct HasPrivateIsEmpty;
+
+impl HasPrivateIsEmpty {
+    pub fn len(self: &Self) -> isize {
+        1
+    }
+
+    fn is_empty(self: &Self) -> bool {
+        false
+    }
+}
+
+pub struct HasIsEmpty;
 
-#[deny(len_without_is_empty)]
 impl HasIsEmpty {
-    fn len(self: &Self) -> isize {
+    pub fn len(self: &Self) -> isize { //~ERROR item `HasIsEmpty` has a public `len` method but a private `is_empty`
         1
     }
 
@@ -36,8 +74,7 @@ impl HasIsEmpty {
 
 struct Wither;
 
-#[deny(len_without_is_empty)]
-trait WithIsEmpty {
+pub trait WithIsEmpty {
     fn len(self: &Self) -> isize;
     fn is_empty(self: &Self) -> bool;
 }
@@ -52,21 +89,18 @@ impl WithIsEmpty for Wither {
     }
 }
 
-struct HasWrongIsEmpty;
+pub struct HasWrongIsEmpty;
 
-#[deny(len_without_is_empty)]
 impl HasWrongIsEmpty {
-    fn len(self: &Self) -> isize { //~ERROR item `HasWrongIsEmpty` has a `.len(_: &Self)`
+    pub fn len(self: &Self) -> isize { //~ERROR item `HasWrongIsEmpty` has a public `len` method but no corresponding `is_empty`
         1
     }
 
-    #[allow(dead_code, unused)]
-    fn is_empty(self: &Self, x : u32) -> bool {
+    pub fn is_empty(self: &Self, x : u32) -> bool {
         false
     }
 }
 
-#[deny(len_zero)]
 fn main() {
     let x = [1, 2];
     if x.len() == 0 {