Add lint for string.extend("str".chars())

fixes #792

Author: philipturnbull

CHANGELOG.md

@@ -359,6 +359,7 @@ All notable changes to this project will be documented in this file.
 [`str_to_string`]: https://github.com/Manishearth/rust-clippy/wiki#str_to_string
 [`string_add`]: https://github.com/Manishearth/rust-clippy/wiki#string_add
 [`string_add_assign`]: https://github.com/Manishearth/rust-clippy/wiki#string_add_assign
+[`string_extend_chars`]: https://github.com/Manishearth/rust-clippy/wiki#string_extend_chars
 [`string_lit_as_bytes`]: https://github.com/Manishearth/rust-clippy/wiki#string_lit_as_bytes
 [`string_to_string`]: https://github.com/Manishearth/rust-clippy/wiki#string_to_string
 [`stutter`]: https://github.com/Manishearth/rust-clippy/wiki#stutter

README.md

@@ -182,7 +182,7 @@ You can check out this great service at [clippy.bashy.io](https://clippy.bashy.i
 
 ## Lints
 
-There are 177 lints included in this crate:
+There are 178 lints included in this crate:
 
 name                                                                                                                   | default | triggers on
 -----------------------------------------------------------------------------------------------------------------------|---------|----------------------------------------------------------------------------------------------------------------------------------
@@ -327,6 +327,7 @@ name
 [single_match_else](https://github.com/Manishearth/rust-clippy/wiki#single_match_else)                                 | allow   | a match statement with a two arms where the second arm's pattern is a wildcard instead of `if let`
 [string_add](https://github.com/Manishearth/rust-clippy/wiki#string_add)                                               | allow   | using `x + ..` where x is a `String` instead of `push_str()`
 [string_add_assign](https://github.com/Manishearth/rust-clippy/wiki#string_add_assign)                                 | allow   | using `x = x + ..` where x is a `String` instead of `push_str()`
+[string_extend_chars](https://github.com/Manishearth/rust-clippy/wiki#string_extend_chars)                             | warn    | using `x.extend(s.chars())` where s is a `&str`
 [string_lit_as_bytes](https://github.com/Manishearth/rust-clippy/wiki#string_lit_as_bytes)                             | warn    | calling `as_bytes` on a string literal instead of using a byte string literal
 [stutter](https://github.com/Manishearth/rust-clippy/wiki#stutter)                                                     | allow   | type names prefixed/postfixed with their containing module's name
 [suspicious_assignment_formatting](https://github.com/Manishearth/rust-clippy/wiki#suspicious_assignment_formatting)   | warn    | suspicious formatting of `*=`, `-=` or `!=`

clippy_lints/src/lib.rs

@@ -391,6 +391,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry) {
         methods::SEARCH_IS_SOME,
         methods::SHOULD_IMPLEMENT_TRAIT,
         methods::SINGLE_CHAR_PATTERN,
+        methods::STRING_EXTEND_CHARS,
         methods::TEMPORARY_CSTRING_AS_PTR,
         methods::WRONG_SELF_CONVENTION,
         minmax::MIN_MAX,

clippy_lints/src/methods.rs

@@ -490,6 +490,32 @@ declare_lint! {
     "using `.get().unwrap()` or `.get_mut().unwrap()` when using `[]` would work instead"
 }
 
+/// **What it does:** Checks for the use of `.extend(s.chars())` where s is a
+/// `&str`.
+///
+/// **Why is this bad?** `.push_str(s)` is clearer and faster
+///
+/// **Known problems:** None.
+///
+/// **Example:**
+/// ```rust
+/// let abc = "abc";
+/// let mut s = String::new();
+/// s.extend(abc.chars());
+/// ```
+/// The correct use would be:
+/// ```rust
+/// let abc = "abc";
+/// let mut s = String::new();
+/// s.push_str(abc);
+/// ```
+
+declare_lint! {
+    pub STRING_EXTEND_CHARS,
+    Warn,
+    "using `x.extend(s.chars())` where s is a `&str`"
+}
+
 
 impl LintPass for Pass {
     fn get_lints(&self) -> LintArray {
@@ -514,7 +540,8 @@ impl LintPass for Pass {
                     FILTER_MAP,
                     ITER_NTH,
                     ITER_SKIP_NEXT,
-                    GET_UNWRAP)
+                    GET_UNWRAP,
+                    STRING_EXTEND_CHARS)
     }
 }
 
@@ -807,10 +834,33 @@ fn lint_vec_extend(cx: &LateContext, expr: &hir::Expr, args: &MethodArgs) {
     }
 }
 
+fn lint_string_extend(cx: &LateContext, expr: &hir::Expr, args: &MethodArgs) {
+    let arg = &args[1];
+    if let Some(arglists) = method_chain_args(arg, &["chars"]) {
+        let target = &arglists[0][0];
+        let (self_ty, _) = walk_ptrs_ty_depth(cx.tcx.tables().expr_ty(target));
+        if self_ty.sty == ty::TyStr {
+            span_lint_and_then(
+                cx,
+                STRING_EXTEND_CHARS,
+                expr.span,
+                "calling `.extend(_.chars())`",
+                |db| {
+                    db.span_suggestion(expr.span, "try this",
+                            format!("{}.push_str({})",
+                                    snippet(cx, args[0].span, "_"),
+                                    snippet(cx, target.span, "_")));
+                });
+        }
+    }
+}
+
 fn lint_extend(cx: &LateContext, expr: &hir::Expr, args: &MethodArgs) {
     let (obj_ty, _) = walk_ptrs_ty_depth(cx.tcx.tables().expr_ty(&args[0]));
     if match_type(cx, obj_ty, &paths::VEC) {
         lint_vec_extend(cx, expr, args);
+    } else if match_type(cx, obj_ty, &paths::STRING) {
+        lint_string_extend(cx, expr, args);
     }
 }
 

tests/compile-fail/methods.rs

@@ -180,6 +180,15 @@ impl IteratorFalsePositives {
     }
 }
 
+#[derive(Copy, Clone)]
+struct HasChars;
+
+impl HasChars {
+    fn chars(self) -> std::str::Chars<'static> {
+        "HasChars".chars()
+    }
+}
+
 /// Checks implementation of `FILTER_NEXT` lint
 fn filter_next() {
     let v = vec![3, 2, 1, 0, -1, -2, -3];
@@ -524,6 +533,30 @@ fn use_extend_from_slice() {
     //~| SUGGESTION v.extend_from_slice(&["But", "this"]);
 }
 
+fn str_extend_chars() {
+    let abc = "abc";
+    let mut s = String::new();
+
+    s.push_str(abc);
+    s.extend(abc.chars());
+    //~^ERROR calling `.extend(_.chars())`
+    //~|HELP try this
+    //~|SUGGESTION s.push_str(abc)
+
+    s.push_str("abc");
+    s.extend("abc".chars());
+    //~^ERROR calling `.extend(_.chars())`
+    //~|HELP try this
+    //~|SUGGESTION s.push_str("abc")
+
+    s.extend(abc.chars().skip(1));
+    s.extend("abc".chars().skip(1));
+    s.extend(['a', 'b', 'c'].iter());
+
+    let f = HasChars;
+    s.extend(f.chars());
+}
+
 fn clone_on_copy() {
     42.clone(); //~ERROR using `clone` on a `Copy` type
                 //~| HELP try removing the `clone` call