diff --git a/CHANGELOG.md b/CHANGELOG.md index b299dbd78..14bf8ba16 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/README.md b/README.md index 28f3b1054..db291adef 100644 --- a/README.md +++ b/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 `!=` diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 07ec1f3f2..a90f121fe 100644 --- a/clippy_lints/src/lib.rs +++ b/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, diff --git a/clippy_lints/src/methods.rs b/clippy_lints/src/methods.rs index a0c64c23b..2d6003004 100644 --- a/clippy_lints/src/methods.rs +++ b/clippy_lints/src/methods.rs @@ -490,6 +490,36 @@ 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` 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 @@ impl LintPass for Pass { 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, diff --git a/tests/compile-fail/methods.rs b/tests/compile-fail/methods.rs index 9d430e7ac..0add705d6 100644 --- a/tests/compile-fail/methods.rs +++ b/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,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