Merge branch 'master' of https://github.com/rust-lang-nursery/rust-clippy into range-plus-one

PUBLISH.md

@@ -2,7 +2,6 @@ Steps to publish a new Clippy version
 
 - Bump `package.version` in `./Cargo.toml` (no need to manually bump `dependencies.clippy_lints.version`).
 - Write a changelog entry.
-- If a nightly update is needed, update `min_version.txt` using `rustc -vV > min_version.txt`
 - Run `./pre_publish.sh`
 - Review and commit all changed files
 - `git push`

build.rs

@@ -1,18 +1,3 @@
-//! This build script ensures that Clippy is not compiled with an
-//! incompatible version of rust. It will panic with a descriptive
-//! error message instead.
-//!
-//! We specifially want to ensure that Clippy is only built with a
-//! rustc version that is newer or equal to the one specified in the
-//! `min_version.txt` file.
-//!
-//! `min_version.txt` is in the repo but also in the `.gitignore` to
-//! make sure that it is not updated manually by accident. Only CI
-//! should update that file.
-//!
-//! This build script was originally taken from the Rocket web framework:
-//! https://github.com/SergioBenitez/Rocket
-
 use std::env;
 
 fn main() {

clippy_lints/src/lib.rs

@@ -554,6 +554,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry<'_>, conf: &Conf) {
         loops::ITER_NEXT_LOOP,
         loops::MANUAL_MEMCPY,
         loops::MUT_RANGE_BOUND,
+        loops::NEEDLESS_COLLECT,
         loops::NEEDLESS_RANGE_LOOP,
         loops::NEVER_LOOP,
         loops::REVERSE_RANGE_LOOP,
@@ -904,6 +905,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry<'_>, conf: &Conf) {
         escape::BOXED_LOCAL,
         large_enum_variant::LARGE_ENUM_VARIANT,
         loops::MANUAL_MEMCPY,
+        loops::NEEDLESS_COLLECT,
         loops::UNUSED_COLLECT,
         methods::EXPECT_FUN_CALL,
         methods::ITER_NTH,

clippy_lints/src/loops.rs

@@ -14,10 +14,12 @@ use rustc::middle::mem_categorization::Categorization;
 use rustc::middle::mem_categorization::cmt_;
 use rustc::ty::{self, Ty};
 use rustc::ty::subst::Subst;
+use rustc_errors::Applicability;
 use std::collections::{HashMap, HashSet};
 use std::iter::{once, Iterator};
 use syntax::ast;
 use syntax::source_map::Span;
+use syntax_pos::BytePos;
 use crate::utils::{sugg, sext};
 use crate::utils::usage::mutated_variables;
 use crate::consts::{constant, Constant};
@@ -223,6 +225,27 @@ declare_clippy_lint! {
      written as a for loop"
 }
 
+/// **What it does:** Checks for functions collecting an iterator when collect
+/// is not needed.
+///
+/// **Why is this bad?** `collect` causes the allocation of a new data structure,
+/// when this allocation may not be needed.
+///
+/// **Known problems:**
+/// None
+///
+/// **Example:**
+/// ```rust
+/// let len = iterator.collect::<Vec<_>>().len();
+/// // should be
+/// let len = iterator.count();
+/// ```
+declare_clippy_lint! {
+    pub NEEDLESS_COLLECT,
+    perf,
+    "collecting an iterator when collect is not needed"
+}
+
 /// **What it does:** Checks for loops over ranges `x..y` where both `x` and `y`
 /// are constant and `x` is greater or equal to `y`, unless the range is
 /// reversed or has a negative `.step_by(_)`.
@@ -400,6 +423,7 @@ impl LintPass for Pass {
             FOR_LOOP_OVER_OPTION,
             WHILE_LET_LOOP,
             UNUSED_COLLECT,
+            NEEDLESS_COLLECT,
             REVERSE_RANGE_LOOP,
             EXPLICIT_COUNTER_LOOP,
             EMPTY_LOOP,
@@ -523,6 +547,8 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Pass {
         if let ExprKind::While(ref cond, _, _) = expr.node {
             check_infinite_loop(cx, cond, expr);
         }
+
+        check_needless_collect(expr, cx);
     }
 
     fn check_stmt(&mut self, cx: &LateContext<'a, 'tcx>, stmt: &'tcx Stmt) {
@@ -2241,3 +2267,71 @@ impl<'a, 'tcx> Visitor<'tcx> for VarCollectorVisitor<'a, 'tcx> {
         NestedVisitorMap::None
     }
 }
+
+const NEEDLESS_COLLECT_MSG: &str = "avoid using `collect()` when not needed";
+
+fn check_needless_collect<'a, 'tcx>(expr: &'tcx Expr, cx: &LateContext<'a, 'tcx>) {
+    if_chain! {
+        if let ExprKind::MethodCall(ref method, _, ref args) = expr.node;
+        if let ExprKind::MethodCall(ref chain_method, _, _) = args[0].node;
+        if chain_method.ident.name == "collect" && match_trait_method(cx, &args[0], &paths::ITERATOR);
+        if let Some(ref generic_args) = chain_method.args;
+        if let Some(GenericArg::Type(ref ty)) = generic_args.args.get(0);
+        then {
+            let ty = cx.tables.node_id_to_type(ty.hir_id);
+            if match_type(cx, ty, &paths::VEC) ||
+                match_type(cx, ty, &paths::VEC_DEQUE) ||
+                match_type(cx, ty, &paths::BTREEMAP) ||
+                match_type(cx, ty, &paths::HASHMAP) {
+                if method.ident.name == "len" {
+                    let span = shorten_needless_collect_span(expr);
+                    span_lint_and_then(cx, NEEDLESS_COLLECT, span, NEEDLESS_COLLECT_MSG, |db| {
+                        db.span_suggestion_with_applicability(
+                            span,
+                            "replace with",
+                            ".count()".to_string(),
+                            Applicability::MachineApplicable,
+                        );
+                    });
+                }
+                if method.ident.name == "is_empty" {
+                    let span = shorten_needless_collect_span(expr);
+                    span_lint_and_then(cx, NEEDLESS_COLLECT, span, NEEDLESS_COLLECT_MSG, |db| {
+                        db.span_suggestion_with_applicability(
+                            span,
+                            "replace with",
+                            ".next().is_none()".to_string(),
+                            Applicability::MachineApplicable,
+                        );
+                    });
+                }
+                if method.ident.name == "contains" {
+                    let contains_arg = snippet(cx, args[1].span, "??");
+                    let span = shorten_needless_collect_span(expr);
+                    span_lint_and_then(cx, NEEDLESS_COLLECT, span, NEEDLESS_COLLECT_MSG, |db| {
+                        db.span_suggestion_with_applicability(
+                            span,
+                            "replace with",
+                            format!(
+                                ".any(|&x| x == {})",
+                                if contains_arg.starts_with('&') { &contains_arg[1..] } else { &contains_arg }
+                            ),
+                            Applicability::MachineApplicable,
+                        );
+                    });
+                }
+            }
+        }
+    }
+}
+
+fn shorten_needless_collect_span(expr: &Expr) -> Span {
+    if_chain! {
+        if let ExprKind::MethodCall(_, _, ref args) = expr.node;
+        if let ExprKind::MethodCall(_, ref span, _) = args[0].node;
+        then {
+            return expr.span.with_lo(span.lo() - BytePos(1));
+        }
+    }
+    unreachable!()
+}

clippy_lints/src/utils/conf.rs

@@ -3,6 +3,7 @@
 #![deny(clippy::missing_docs_in_private_items)]
 
 use lazy_static::lazy_static;
+use std::default::Default;
 use std::{env, fmt, fs, io, path};
 use std::io::Read;
 use syntax::{ast, source_map};
@@ -65,7 +66,7 @@ macro_rules! define_Conf {
         mod helpers {
             use serde_derive::Deserialize;
             /// Type used to store lint configuration.
-            #[derive(Default, Deserialize)]
+            #[derive(Deserialize)]
             #[serde(rename_all="kebab-case", deny_unknown_fields)]
             pub struct Conf {
                 $(#[$doc] #[serde(default=$rust_name_str)] #[serde(with=$rust_name_str)]
@@ -146,6 +147,12 @@ define_Conf! {
     (trivial_copy_size_limit, "trivial_copy_size_limit", None => Option<u64>),
 }
 
+impl Default for Conf {
+    fn default() -> Conf {
+        toml::from_str("").expect("we never error on empty config files")
+    }
+}
+
 /// Search for the configuration file.
 pub fn lookup_conf_file() -> io::Result<Option<path::PathBuf>> {
     /// Possible filename to search for.
@@ -180,7 +187,7 @@ pub fn lookup_conf_file() -> io::Result<Option<path::PathBuf>> {
 ///
 /// Used internally for convenience
 fn default(errors: Vec<Error>) -> (Conf, Vec<Error>) {
-    (toml::from_str("").expect("we never error on empty config files"), errors)
+    (Conf::default(), errors)
 }
 
 /// Read the `toml` configuration file.

tests/ui/needless_collect.rs

@@ -0,0 +1,19 @@
+#![feature(tool_lints)]
+
+use std::collections::{HashMap, HashSet, BTreeSet};
+
+#[warn(clippy::needless_collect)]
+#[allow(unused_variables, clippy::iter_cloned_collect)]
+fn main() {
+    let sample = [1; 5];
+    let len = sample.iter().collect::<Vec<_>>().len();
+    if sample.iter().collect::<Vec<_>>().is_empty() {
+        // Empty
+    }
+    sample.iter().cloned().collect::<Vec<_>>().contains(&1);
+    sample.iter().map(|x| (x, x)).collect::<HashMap<_, _>>().len();
+    // Notice the `HashSet`--this should not be linted
+    sample.iter().collect::<HashSet<_>>().len();
+    // Neither should this
+    sample.iter().collect::<BTreeSet<_>>().len();
+}

tests/ui/needless_collect.stderr

@@ -0,0 +1,28 @@
+error: avoid using `collect()` when not needed
+ --> $DIR/needless_collect.rs:9:28
+  |
+9 |     let len = sample.iter().collect::<Vec<_>>().len();
+  |                            ^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace with: `.count()`
+  |
+  = note: `-D clippy::needless-collect` implied by `-D warnings`
+
+error: avoid using `collect()` when not needed
+  --> $DIR/needless_collect.rs:10:21
+   |
+10 |     if sample.iter().collect::<Vec<_>>().is_empty() {
+   |                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace with: `.next().is_none()`
+
+error: avoid using `collect()` when not needed
+  --> $DIR/needless_collect.rs:13:27
+   |
+13 |     sample.iter().cloned().collect::<Vec<_>>().contains(&1);
+   |                           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace with: `.any(|&x| x == 1)`
+
+error: avoid using `collect()` when not needed
+  --> $DIR/needless_collect.rs:14:34
+   |
+14 |     sample.iter().map(|x| (x, x)).collect::<HashMap<_, _>>().len();
+   |                                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace with: `.count()`
+
+error: aborting due to 4 previous errors
+