unnecessary_join lint

Author: yoav-lavi

CHANGELOG.md

@@ -3508,6 +3508,7 @@ Released 2018-09-13
 [`unnecessary_filter_map`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_filter_map
 [`unnecessary_find_map`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_find_map
 [`unnecessary_fold`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_fold
+[`unnecessary_join`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_join
 [`unnecessary_lazy_evaluations`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_lazy_evaluations
 [`unnecessary_mut_passed`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_mut_passed
 [`unnecessary_operation`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_operation

clippy_lints/src/lib.register_lints.rs

@@ -334,6 +334,7 @@ store.register_lints(&[
     methods::UNNECESSARY_FILTER_MAP,
     methods::UNNECESSARY_FIND_MAP,
     methods::UNNECESSARY_FOLD,
+    methods::UNNECESSARY_JOIN,
     methods::UNNECESSARY_LAZY_EVALUATIONS,
     methods::UNNECESSARY_TO_OWNED,
     methods::UNWRAP_OR_ELSE_DEFAULT,

clippy_lints/src/lib.register_pedantic.rs

@@ -63,6 +63,7 @@ store.register_group(true, "clippy::pedantic", Some("clippy_pedantic"), vec![
     LintId::of(methods::IMPLICIT_CLONE),
     LintId::of(methods::INEFFICIENT_TO_STRING),
     LintId::of(methods::MAP_UNWRAP_OR),
+    LintId::of(methods::UNNECESSARY_JOIN),
     LintId::of(misc::FLOAT_CMP),
     LintId::of(misc::USED_UNDERSCORE_BINDING),
     LintId::of(mut_mut::MUT_MUT),

clippy_lints/src/methods/mod.rs

@@ -60,6 +60,7 @@ mod uninit_assumed_init;
 mod unnecessary_filter_map;
 mod unnecessary_fold;
 mod unnecessary_iter_cloned;
+mod unnecessary_join;
 mod unnecessary_lazy_eval;
 mod unnecessary_to_owned;
 mod unwrap_or_else_default;
@@ -2049,6 +2050,35 @@ declare_clippy_lint! {
     "unnecessary calls to `to_owned`-like functions"
 }
 
+declare_clippy_lint! {
+    /// ### What it does
+    /// Checks for use of `.collect::<Vec<String>>().join("")` on iterators.
+    ///
+    /// ### Why is this bad?
+    /// `.collect::<String>()` is more concise and usually more performant
+    ///
+    /// ### Example
+    /// ```rust
+    /// let vector = vec!["hello",  "world"];
+    /// let output = vector.iter().map(|item| item.to_uppercase()).collect::<Vec<String>>().join("");
+    /// println!("{}", output);
+    /// ```
+    /// The correct use would be:
+    /// ```rust
+    /// let vector = vec!["hello",  "world"];
+    /// let output = vector.iter().map(|item| item.to_uppercase()).collect::<String>();
+    /// println!("{}", output);
+    /// ```
+    /// ### Known problems
+    /// While `.collect::<String>()` is more performant in most cases, there are cases where
+    /// using `.collect::<String>()` over `.collect::<Vec<String>>().join("")`
+    /// will prevent loop unrolling and will result in a negative performance impact.
+    #[clippy::version = "1.61.0"]
+    pub UNNECESSARY_JOIN,
+    pedantic,
+    "using `.collect::<Vec<String>>().join(\"\")` on an iterator"
+}
+
 pub struct Methods {
     avoid_breaking_exported_api: bool,
     msrv: Option<RustcVersion>,
@@ -2134,6 +2164,7 @@ impl_lint_pass!(Methods => [
     MANUAL_SPLIT_ONCE,
     NEEDLESS_SPLITN,
     UNNECESSARY_TO_OWNED,
+    UNNECESSARY_JOIN,
 ]);
 
 /// Extracts a method call name, args, and `Span` of the method name.
@@ -2429,6 +2460,11 @@ fn check_methods<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>, msrv: Optio
             ("is_file", []) => filetype_is_file::check(cx, expr, recv),
             ("is_none", []) => check_is_some_is_none(cx, expr, recv, false),
             ("is_some", []) => check_is_some_is_none(cx, expr, recv, true),
+            ("join", [join_arg]) => {
+                if let Some(("collect", _, span)) = method_call(recv) {
+                    unnecessary_join::check(cx, expr, recv, join_arg, span);
+                }
+            },
             ("last", args @ []) | ("skip", args @ [_]) => {
                 if let Some((name2, [recv2, args2 @ ..], _span2)) = method_call(recv) {
                     if let ("cloned", []) = (name2, args2) {

clippy_lints/src/methods/unnecessary_join.rs

@@ -0,0 +1,41 @@
+use clippy_utils::{diagnostics::span_lint_and_sugg, ty::is_type_diagnostic_item};
+use rustc_ast::ast::LitKind;
+use rustc_errors::Applicability;
+use rustc_hir::{Expr, ExprKind};
+use rustc_lint::LateContext;
+use rustc_middle::ty::{Ref, Slice};
+use rustc_span::{sym, Span};
+
+use super::UNNECESSARY_JOIN;
+
+pub(super) fn check<'tcx>(
+    cx: &LateContext<'tcx>,
+    expr: &'tcx Expr<'tcx>,
+    join_self_arg: &'tcx Expr<'tcx>,
+    join_arg: &'tcx Expr<'tcx>,
+    span: Span,
+) {
+    let applicability = Applicability::MachineApplicable;
+    let collect_output_adjusted_type = cx.typeck_results().expr_ty_adjusted(join_self_arg);
+    if_chain! {
+        // the turbofish for collect is ::<Vec<String>>
+        if let Ref(_, ref_type, _) = collect_output_adjusted_type.kind();
+        if let Slice(slice) = ref_type.kind();
+        if is_type_diagnostic_item(cx, *slice, sym::String);
+        // the argument for join is ""
+        if let ExprKind::Lit(spanned) = &join_arg.kind;
+        if let LitKind::Str(symbol, _) = spanned.node;
+        if symbol.is_empty();
+        then {
+            span_lint_and_sugg(
+                cx,
+                UNNECESSARY_JOIN,
+                span.with_hi(expr.span.hi()),
+                r#"called `.collect<Vec<String>>().join("")` on an iterator"#,
+                "try using",
+                "collect::<String>()".to_owned(),
+                applicability,
+            );
+        }
+    }
+}

tests/ui/unnecessary_join.fixed

@@ -0,0 +1,35 @@
+// run-rustfix
+
+#![warn(clippy::unnecessary_join)]
+
+fn main() {
+    // should be linted
+    let vector = vec!["hello", "world"];
+    let output = vector
+        .iter()
+        .map(|item| item.to_uppercase())
+        .collect::<String>();
+    println!("{}", output);
+
+    // should be linted
+    let vector = vec!["hello", "world"];
+    let output = vector
+        .iter()
+        .map(|item| item.to_uppercase())
+        .collect::<String>();
+    println!("{}", output);
+
+    // should not be linted
+    let vector = vec!["hello", "world"];
+    let output = vector
+        .iter()
+        .map(|item| item.to_uppercase())
+        .collect::<Vec<String>>()
+        .join("\n");
+    println!("{}", output);
+
+    // should not be linted
+    let vector = vec!["hello", "world"];
+    let output = vector.iter().map(|item| item.to_uppercase()).collect::<String>();
+    println!("{}", output);
+}

tests/ui/unnecessary_join.rs

@@ -0,0 +1,37 @@
+// run-rustfix
+
+#![warn(clippy::unnecessary_join)]
+
+fn main() {
+    // should be linted
+    let vector = vec!["hello", "world"];
+    let output = vector
+        .iter()
+        .map(|item| item.to_uppercase())
+        .collect::<Vec<String>>()
+        .join("");
+    println!("{}", output);
+
+    // should be linted
+    let vector = vec!["hello", "world"];
+    let output = vector
+        .iter()
+        .map(|item| item.to_uppercase())
+        .collect::<Vec<_>>()
+        .join("");
+    println!("{}", output);
+
+    // should not be linted
+    let vector = vec!["hello", "world"];
+    let output = vector
+        .iter()
+        .map(|item| item.to_uppercase())
+        .collect::<Vec<String>>()
+        .join("\n");
+    println!("{}", output);
+
+    // should not be linted
+    let vector = vec!["hello", "world"];
+    let output = vector.iter().map(|item| item.to_uppercase()).collect::<String>();
+    println!("{}", output);
+}

tests/ui/unnecessary_join.stderr

@@ -0,0 +1,20 @@
+error: called `.collect<Vec<String>>().join("")` on an iterator
+  --> $DIR/unnecessary_join.rs:11:10
+   |
+LL |           .collect::<Vec<String>>()
+   |  __________^
+LL | |         .join("");
+   | |_________________^ help: try using: `collect::<String>()`
+   |
+   = note: `-D clippy::unnecessary-join` implied by `-D warnings`
+
+error: called `.collect<Vec<String>>().join("")` on an iterator
+  --> $DIR/unnecessary_join.rs:20:10
+   |
+LL |           .collect::<Vec<_>>()
+   |  __________^
+LL | |         .join("");
+   | |_________________^ help: try using: `collect::<String>()`
+
+error: aborting due to 2 previous errors
+