Merge pull request #1982 from rust-lang-nursery/bytecount

new lint: naive_bytecount

CHANGELOG.md

@@ -1,6 +1,8 @@
 # Change Log
 All notable changes to this project will be documented in this file.
 
+* New lint: [`naive_bytecount`]
+
 ## 0.0.154
 * Fix [`use_self`] triggering inside derives
 * Add support for linting an entire workspace with `cargo clippy --all`
@@ -516,6 +518,7 @@ All notable changes to this project will be documented in this file.
 [`mut_mut`]: https://github.com/rust-lang-nursery/rust-clippy/wiki#mut_mut
 [`mutex_atomic`]: https://github.com/rust-lang-nursery/rust-clippy/wiki#mutex_atomic
 [`mutex_integer`]: https://github.com/rust-lang-nursery/rust-clippy/wiki#mutex_integer
+[`naive_bytecount`]: https://github.com/rust-lang-nursery/rust-clippy/wiki#naive_bytecount
 [`needless_bool`]: https://github.com/rust-lang-nursery/rust-clippy/wiki#needless_bool
 [`needless_borrow`]: https://github.com/rust-lang-nursery/rust-clippy/wiki#needless_borrow
 [`needless_borrowed_reference`]: https://github.com/rust-lang-nursery/rust-clippy/wiki#needless_borrowed_reference

README.md

@@ -180,7 +180,7 @@ transparently:
 
 ## Lints
 
-There are 205 lints included in this crate:
+There are 206 lints included in this crate:
 
 name                                                                                                                         | default | triggers on
 -----------------------------------------------------------------------------------------------------------------------------|---------|----------------------------------------------------------------------------------------------------------------------------------
@@ -290,6 +290,7 @@ name
 [mut_mut](https://github.com/rust-lang-nursery/rust-clippy/wiki#mut_mut)                                                     | allow   | usage of double-mut refs, e.g. `&mut &mut ...`
 [mutex_atomic](https://github.com/rust-lang-nursery/rust-clippy/wiki#mutex_atomic)                                           | warn    | using a mutex where an atomic value could be used instead
 [mutex_integer](https://github.com/rust-lang-nursery/rust-clippy/wiki#mutex_integer)                                         | allow   | using a mutex for an integer type
+[naive_bytecount](https://github.com/rust-lang-nursery/rust-clippy/wiki#naive_bytecount)                                     | warn    | use of naive `<slice>.filter(|&x| x == y).count()` to count byte values
 [needless_bool](https://github.com/rust-lang-nursery/rust-clippy/wiki#needless_bool)                                         | warn    | if-statements with plain booleans in the then- and else-clause, e.g. `if p { true } else { false }`
 [needless_borrow](https://github.com/rust-lang-nursery/rust-clippy/wiki#needless_borrow)                                     | warn    | taking a reference that is going to be automatically dereferenced
 [needless_borrowed_reference](https://github.com/rust-lang-nursery/rust-clippy/wiki#needless_borrowed_reference)             | warn    | taking a needless borrowed reference

clippy_lints/src/bytecount.rs

@@ -0,0 +1,115 @@
+use rustc::hir::*;
+use rustc::lint::*;
+use rustc::ty;
+use syntax::ast::{Name, UintTy};
+use utils::{contains_name, match_type, paths, single_segment_path, snippet, span_lint_and_sugg, walk_ptrs_ty};
+
+/// **What it does:** Checks for naive byte counts
+///
+/// **Why is this bad?** The [`bytecount`](https://crates.io/crates/bytecount)
+/// crate has methods to count your bytes faster, especially for large slices.
+///
+/// **Known problems:** If you have predominantly small slices, the
+/// `bytecount::count(..)` method may actually be slower. However, if you can
+/// ensure that less than 2³²-1 matches arise, the `naive_count_32(..)` can be
+/// faster in those cases.
+///
+/// **Example:**
+///
+/// ```rust
+/// &my_data.filter(|&x| x == 0u8).count() // use bytecount::count instead
+/// ```
+declare_lint! {
+    pub NAIVE_BYTECOUNT,
+    Warn,
+    "use of naive `<slice>.filter(|&x| x == y).count()` to count byte values"
+}
+
+#[derive(Copy, Clone)]
+pub struct ByteCount;
+
+impl LintPass for ByteCount {
+    fn get_lints(&self) -> LintArray {
+        lint_array!(NAIVE_BYTECOUNT)
+    }
+}
+
+impl<'a, 'tcx> LateLintPass<'a, 'tcx> for ByteCount {
+    fn check_expr(&mut self, cx: &LateContext, expr: &Expr) {
+        if_let_chain!([
+            let ExprMethodCall(ref count, _, ref count_args) = expr.node,
+            count.name == "count",
+            count_args.len() == 1,
+            let ExprMethodCall(ref filter, _, ref filter_args) = count_args[0].node,
+            filter.name == "filter",
+            filter_args.len() == 2,
+            let ExprClosure(_, _, body_id, _) = filter_args[1].node,
+        ], {
+            let body = cx.tcx.hir.body(body_id);
+            if_let_chain!([
+                body.arguments.len() == 1,
+                let Some(argname) = get_pat_name(&body.arguments[0].pat),
+                let ExprBinary(ref op, ref l, ref r) = body.value.node,
+                op.node == BiEq,
+                match_type(cx,
+                           walk_ptrs_ty(cx.tables.expr_ty(&filter_args[0])),
+                           &paths::SLICE_ITER),
+            ], {
+                let needle = match get_path_name(l) {
+                    Some(name) if check_arg(name, argname, r) => r,
+                    _ => match get_path_name(r) {
+                        Some(name) if check_arg(name, argname, l) => l,
+                        _ => { return; }
+                    }
+                };
+                if ty::TyUint(UintTy::U8) != walk_ptrs_ty(cx.tables.expr_ty(needle)).sty {
+                    return;
+                }
+                let haystack = if let ExprMethodCall(ref path, _, ref args) =
+                        filter_args[0].node {
+                    let p = path.name;
+                    if (p == "iter" || p == "iter_mut") && args.len() == 1 {
+                        &args[0]
+                    } else {
+                        &filter_args[0]
+                    }
+                } else {
+                    &filter_args[0]
+                };
+                span_lint_and_sugg(cx,
+                                   NAIVE_BYTECOUNT,
+                                   expr.span,
+                                   "You appear to be counting bytes the naive way",
+                                   "Consider using the bytecount crate",
+                                   format!("bytecount::count({}, {})",
+                                            snippet(cx, haystack.span, ".."),
+                                            snippet(cx, needle.span, "..")));
+            });
+        });
+    }
+}
+
+fn check_arg(name: Name, arg: Name, needle: &Expr) -> bool {
+    name == arg && !contains_name(name, needle)
+}
+
+fn get_pat_name(pat: &Pat) -> Option<Name> {
+    match pat.node {
+        PatKind::Binding(_, _, ref spname, _) => Some(spname.node),
+        PatKind::Path(ref qpath) => single_segment_path(qpath).map(|ps| ps.name),
+        PatKind::Box(ref p) | PatKind::Ref(ref p, _) => get_pat_name(&*p),
+        _ => None
+    }
+}
+
+fn get_path_name(expr: &Expr) -> Option<Name> {
+    match expr.node {
+        ExprBox(ref e) | ExprAddrOf(_, ref e) | ExprUnary(UnOp::UnDeref, ref e) => get_path_name(e),
+        ExprBlock(ref b) => if b.stmts.is_empty() {
+            b.expr.as_ref().and_then(|p| get_path_name(p))
+        } else { None },
+        ExprPath(ref qpath) => single_segment_path(qpath).map(|ps| ps.name),
+        _ => None
+    }
+}
+

clippy_lints/src/lib.rs

@@ -74,6 +74,7 @@ pub mod bit_mask;
 pub mod blacklisted_name;
 pub mod block_in_if_condition;
 pub mod booleans;
+pub mod bytecount;
 pub mod collapsible_if;
 pub mod copies;
 pub mod cyclomatic_complexity;
@@ -321,6 +322,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry) {
     reg.register_late_lint_pass(box needless_pass_by_value::NeedlessPassByValue);
     reg.register_early_lint_pass(box literal_digit_grouping::LiteralDigitGrouping);
     reg.register_late_lint_pass(box use_self::UseSelf);
+    reg.register_late_lint_pass(box bytecount::ByteCount);
 
     reg.register_lint_group("clippy_restrictions", vec![
         arithmetic::FLOAT_ARITHMETIC,
@@ -388,6 +390,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry) {
         block_in_if_condition::BLOCK_IN_IF_CONDITION_EXPR,
         block_in_if_condition::BLOCK_IN_IF_CONDITION_STMT,
         booleans::LOGIC_BUG,
+        bytecount::NAIVE_BYTECOUNT,
         collapsible_if::COLLAPSIBLE_IF,
         copies::IF_SAME_THEN_ELSE,
         copies::IFS_SAME_COND,

clippy_lints/src/shadow.rs

@@ -1,10 +1,10 @@
 use reexport::*;
 use rustc::lint::*;
 use rustc::hir::*;
-use rustc::hir::intravisit::{Visitor, FnKind, NestedVisitorMap};
+use rustc::hir::intravisit::FnKind;
 use rustc::ty;
 use syntax::codemap::Span;
-use utils::{higher, in_external_macro, snippet, span_lint_and_then, iter_input_pats};
+use utils::{contains_name, higher, in_external_macro, snippet, span_lint_and_then, iter_input_pats};
 
 /// **What it does:** Checks for bindings that shadow other bindings already in
 /// scope, while just changing reference level or mutability.
@@ -261,7 +261,7 @@ fn lint_shadow<'a, 'tcx: 'a>(
                 ),
                 |db| { db.span_note(prev_span, "previous binding is here"); },
             );
-        } else if contains_self(name, expr) {
+        } else if contains_name(name, expr) {
             span_lint_and_then(
                 cx,
                 SHADOW_REUSE,
@@ -391,27 +391,3 @@ fn path_eq_name(name: Name, path: &Path) -> bool {
     !path.is_global() && path.segments.len() == 1 && path.segments[0].name.as_str() == name.as_str()
 }
 
-struct ContainsSelf {
-    name: Name,
-    result: bool,
-}
-
-impl<'tcx> Visitor<'tcx> for ContainsSelf {
-    fn visit_name(&mut self, _: Span, name: Name) {
-        if self.name == name {
-            self.result = true;
-        }
-    }
-    fn nested_visit_map<'this>(&'this mut self) -> NestedVisitorMap<'this, 'tcx> {
-        NestedVisitorMap::None
-    }
-}
-
-fn contains_self(name: Name, expr: &Expr) -> bool {
-    let mut cs = ContainsSelf {
-        name: name,
-        result: false,
-    };
-    cs.visit_expr(expr);
-    cs.result
-}

clippy_lints/src/utils/mod.rs

@@ -3,6 +3,7 @@ use rustc::hir;
 use rustc::hir::*;
 use rustc::hir::def_id::{DefId, CRATE_DEF_INDEX};
 use rustc::hir::def::Def;
+use rustc::hir::intravisit::{NestedVisitorMap, Visitor};
 use rustc::hir::map::Node;
 use rustc::lint::{LintContext, Level, LateContext, Lint};
 use rustc::session::Session;
@@ -393,6 +394,33 @@ pub fn get_item_name(cx: &LateContext, expr: &Expr) -> Option<Name> {
     }
 }
 
+struct ContainsName {
+    name: Name,
+    result: bool,
+}
+
+impl<'tcx> Visitor<'tcx> for ContainsName {
+    fn visit_name(&mut self, _: Span, name: Name) {
+        if self.name == name {
+            self.result = true;
+        }
+    }
+    fn nested_visit_map<'this>(&'this mut self) -> NestedVisitorMap<'this, 'tcx> {
+        NestedVisitorMap::None
+    }
+}
+
+/// check if an `Expr` contains a certain name
+pub fn contains_name(name: Name, expr: &Expr) -> bool {
+    let mut cn = ContainsName {
+        name: name,
+        result: false,
+    };
+    cn.visit_expr(expr);
+    cn.result
+}
+
+
 /// Convert a span to a code snippet if available, otherwise use default.
 ///
 /// # Example

clippy_lints/src/utils/paths.rs

@@ -69,6 +69,7 @@ pub const RESULT_ERR: [&'static str; 4] = ["core", "result", "Result", "Err"];
 pub const RESULT_OK: [&'static str; 4] = ["core", "result", "Result", "Ok"];
 pub const SERDE_DE_VISITOR: [&'static str; 3] = ["serde", "de", "Visitor"];
 pub const SLICE_INTO_VEC: [&'static str; 4] = ["alloc", "slice", "<impl [T]>", "into_vec"];
+pub const SLICE_ITER: [&str; 3] = ["core", "slice", "Iter"];
 pub const STRING: [&'static str; 3] = ["alloc", "string", "String"];
 pub const TO_OWNED: [&'static str; 3] = ["alloc", "borrow", "ToOwned"];
 pub const TO_STRING: [&'static str; 3] = ["alloc", "string", "ToString"];

tests/ui/bytecount.rs

@@ -0,0 +1,27 @@
+#![feature(plugin)]
+#![plugin(clippy)]
+
+#[deny(naive_bytecount)]
+fn main() {
+    let x = vec![0_u8; 16];
+
+    let _ = x.iter().filter(|&&a| a == 0).count(); // naive byte count
+
+    let _ = (&x[..]).iter().filter(|&a| *a == 0).count(); // naive byte count
+
+    let _ = x.iter().filter(|a| **a > 0).count(); // not an equality count, OK.
+
+    let _ = x.iter().map(|a| a + 1).filter(|&a| a < 15).count(); // not a slice
+
+    let b = 0;
+
+    let _ = x.iter().filter(|_| b > 0).count(); // woah there
+
+    let _ = x.iter().filter(|_a| b == b + 1).count(); // nothing to see here, move along
+
+    let _ = x.iter().filter(|a| b + 1 == **a).count(); // naive byte count
+
+    let y = vec![0_u16; 3];
+
+    let _ = y.iter().filter(|&&a| a == 0).count(); // naive count, but not bytes
+}

tests/ui/bytecount.stderr

@@ -0,0 +1,26 @@
+error: You appear to be counting bytes the naive way
+ --> $DIR/bytecount.rs:8:13
+  |
+8 |     let _ = x.iter().filter(|&&a| a == 0).count(); // naive byte count
+  |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: Consider using the bytecount crate: `bytecount::count(x, 0)`
+  |
+note: lint level defined here
+ --> $DIR/bytecount.rs:4:8
+  |
+4 | #[deny(naive_bytecount)]
+  |        ^^^^^^^^^^^^^^^
+
+error: You appear to be counting bytes the naive way
+  --> $DIR/bytecount.rs:10:13
+   |
+10 |     let _ = (&x[..]).iter().filter(|&a| *a == 0).count(); // naive byte count
+   |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: Consider using the bytecount crate: `bytecount::count((&x[..]), 0)`
+
+error: You appear to be counting bytes the naive way
+  --> $DIR/bytecount.rs:22:13
+   |
+22 |     let _ = x.iter().filter(|a| b + 1 == **a).count(); // naive byte count
+   |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: Consider using the bytecount crate: `bytecount::count(x, b + 1)`
+
+error: aborting due to 3 previous errors
+