Merge pull request #1349 from philipturnbull/extend-chars

Lint `.extend(s.chars())` (closes #792)

CHANGELOG.md

@@ -362,6 +362,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` or `String`
 [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,36 @@
     "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` or `String`.
+///
+/// **Why is this bad?** `.push_str(s)` is clearer
+///
+/// **Known problems:** None.
+///
+/// **Example:**
+/// ```rust
+/// let abc = "abc";
+/// let def = String::from("def");
+/// let mut s = String::new();
+/// s.extend(abc.chars());
+/// s.extend(def.chars());
+/// ```
+/// The correct use would be:
+/// ```rust
+/// let abc = "abc";
+/// let def = String::from("def");
+/// let mut s = String::new();
+/// s.push_str(abc);
+/// s.push_str(&def));
+/// ```
+
+declare_lint! {
+    pub STRING_EXTEND_CHARS,
+    Warn,
+    "using `x.extend(s.chars())` where s is a `&str` or `String`"
+}
+
 
 impl LintPass for Pass {
     fn get_lints(&self) -> LintArray {
@@ -514,7 +544,8 @@ fn get_lints(&self) -> LintArray {
                     FILTER_MAP,
                     ITER_NTH,
                     ITER_SKIP_NEXT,
-                    GET_UNWRAP)
+                    GET_UNWRAP,
+                    STRING_EXTEND_CHARS)
     }
 }
 
@@ -794,11 +825,7 @@ fn lint_clone_on_copy(cx: &LateContext, expr: &hir::Expr, arg: &hir::Expr, arg_t
     }
 }
 
-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) {
-        return;
-    }
+fn lint_vec_extend(cx: &LateContext, expr: &hir::Expr, args: &MethodArgs) {
     let arg_ty = cx.tcx.tables().expr_ty(&args[1]);
     if let Some(slice) = derefs_to_slice(cx, &args[1], arg_ty) {
         span_lint_and_then(cx, EXTEND_FROM_SLICE, expr.span, "use of `extend` to extend a Vec by a slice", |db| {
@@ -811,6 +838,43 @@ fn lint_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));
+        let ref_str = if self_ty.sty == ty::TyStr {
+            ""
+        } else if match_type(cx, self_ty, &paths::STRING) {
+            "&"
+        } else {
+            return;
+        };
+
+        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, "_"),
+                                ref_str,
+                                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);
+    }
+}
+
 fn lint_cstring_as_ptr(cx: &LateContext, expr: &hir::Expr, new: &hir::Expr, unwrap: &hir::Expr) {
     if_let_chain!{[
         let hir::ExprCall(ref fun, ref args) = new.node,

tests/compile-fail/methods.rs

@@ -180,6 +180,15 @@ fn skip(self, _: usize) -> 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,37 @@ fn use_extend_from_slice() {
     //~| SUGGESTION v.extend_from_slice(&["But", "this"]);
 }
 
+fn str_extend_chars() {
+    let abc = "abc";
+    let def = String::from("def");
+    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.push_str(&def);
+    s.extend(def.chars());
+    //~^ERROR calling `.extend(_.chars())`
+    //~|HELP try this
+    //~|SUGGESTION s.push_str(&def)
+
+    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