From 0ab7e6c598d21e419ffe6a999513d101bc78aa86 Mon Sep 17 00:00:00 2001 From: Phil Turnbull Date: Sun, 16 Oct 2016 15:13:09 -0400 Subject: [PATCH 1/5] Factor out lint_vec_extend --- clippy_lints/src/methods.rs | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/clippy_lints/src/methods.rs b/clippy_lints/src/methods.rs index a0c64c23b..73a86ff1d 100644 --- a/clippy_lints/src/methods.rs +++ b/clippy_lints/src/methods.rs @@ -794,11 +794,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 +807,13 @@ fn lint_extend(cx: &LateContext, expr: &hir::Expr, args: &MethodArgs) { } } +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); + } +} + 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, From fa78b09fa76386e41d824fe83a176c0e641105db Mon Sep 17 00:00:00 2001 From: Phil Turnbull Date: Sat, 19 Nov 2016 10:21:40 -0500 Subject: [PATCH 2/5] Add lint for `string.extend("str".chars())` fixes #792 --- CHANGELOG.md | 1 + README.md | 3 +- clippy_lints/src/lib.rs | 1 + clippy_lints/src/methods.rs | 52 ++++++++++++++++++++++++++++++++++- tests/compile-fail/methods.rs | 33 ++++++++++++++++++++++ 5 files changed, 88 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 47b91d267..c9bc5d0c9 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/README.md b/README.md index 28f3b1054..bc21bd28d 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` [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 73a86ff1d..527b3228d 100644 --- a/clippy_lints/src/methods.rs +++ b/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); } } diff --git a/tests/compile-fail/methods.rs b/tests/compile-fail/methods.rs index 9d430e7ac..658eda65f 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,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 From 73a73638c0d25318608b6aba36d9e21c6e24f89e Mon Sep 17 00:00:00 2001 From: Phil Turnbull Date: Sat, 19 Nov 2016 10:36:23 -0500 Subject: [PATCH 3/5] Add lint for `string.extend(string.chars())` fixes #792 --- README.md | 2 +- clippy_lints/src/methods.rs | 41 ++++++++++++++++++++++------------- tests/compile-fail/methods.rs | 7 ++++++ 3 files changed, 34 insertions(+), 16 deletions(-) diff --git a/README.md b/README.md index bc21bd28d..db291adef 100644 --- a/README.md +++ b/README.md @@ -327,7 +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_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/methods.rs b/clippy_lints/src/methods.rs index 527b3228d..9506c8e83 100644 --- a/clippy_lints/src/methods.rs +++ b/clippy_lints/src/methods.rs @@ -491,7 +491,7 @@ declare_lint! { } /// **What it does:** Checks for the use of `.extend(s.chars())` where s is a -/// `&str`. +/// `&str` or `String`. /// /// **Why is this bad?** `.push_str(s)` is clearer and faster /// @@ -500,20 +500,24 @@ declare_lint! { /// **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.as_str()); /// ``` declare_lint! { pub STRING_EXTEND_CHARS, Warn, - "using `x.extend(s.chars())` where s is a `&str`" + "using `x.extend(s.chars())` where s is a `&str` or `String`" } @@ -839,19 +843,26 @@ fn lint_string_extend(cx: &LateContext, expr: &hir::Expr, args: &MethodArgs) { 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, "_"))); - }); - } + let extra_suggestion = if self_ty.sty == ty::TyStr { + "" + } else if match_type(cx, self_ty, &paths::STRING) { + ".as_str()" + } 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, "_"), + snippet(cx, target.span, "_"), + extra_suggestion)); + }); } } diff --git a/tests/compile-fail/methods.rs b/tests/compile-fail/methods.rs index 658eda65f..0e30876c3 100644 --- a/tests/compile-fail/methods.rs +++ b/tests/compile-fail/methods.rs @@ -535,6 +535,7 @@ fn use_extend_from_slice() { fn str_extend_chars() { let abc = "abc"; + let def = String::from("def"); let mut s = String::new(); s.push_str(abc); @@ -549,6 +550,12 @@ fn str_extend_chars() { //~|HELP try this //~|SUGGESTION s.push_str("abc") + s.push_str(def.as_str()); + s.extend(def.chars()); + //~^ERROR calling `.extend(_.chars())` + //~|HELP try this + //~|SUGGESTION s.push_str(def.as_str()) + s.extend(abc.chars().skip(1)); s.extend("abc".chars().skip(1)); s.extend(['a', 'b', 'c'].iter()); From e9f3911899235ffdcc46c80e1d05e996c251fa15 Mon Sep 17 00:00:00 2001 From: Phil Turnbull Date: Sun, 20 Nov 2016 11:19:36 -0500 Subject: [PATCH 4/5] Suggest `&s` instead of `s.as_str()` --- clippy_lints/src/methods.rs | 10 +++++----- tests/compile-fail/methods.rs | 4 ++-- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/clippy_lints/src/methods.rs b/clippy_lints/src/methods.rs index 9506c8e83..d1192e3f2 100644 --- a/clippy_lints/src/methods.rs +++ b/clippy_lints/src/methods.rs @@ -511,7 +511,7 @@ declare_lint! { /// let def = String::from("def"); /// let mut s = String::new(); /// s.push_str(abc); -/// s.push_str(def.as_str()); +/// s.push_str(&def)); /// ``` declare_lint! { @@ -843,10 +843,10 @@ fn lint_string_extend(cx: &LateContext, expr: &hir::Expr, args: &MethodArgs) { 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 extra_suggestion = if self_ty.sty == ty::TyStr { + let ref_str = if self_ty.sty == ty::TyStr { "" } else if match_type(cx, self_ty, &paths::STRING) { - ".as_str()" + "&" } else { return; }; @@ -860,8 +860,8 @@ fn lint_string_extend(cx: &LateContext, expr: &hir::Expr, args: &MethodArgs) { db.span_suggestion(expr.span, "try this", format!("{}.push_str({}{})", snippet(cx, args[0].span, "_"), - snippet(cx, target.span, "_"), - extra_suggestion)); + ref_str, + snippet(cx, target.span, "_"))); }); } } diff --git a/tests/compile-fail/methods.rs b/tests/compile-fail/methods.rs index 0e30876c3..0add705d6 100644 --- a/tests/compile-fail/methods.rs +++ b/tests/compile-fail/methods.rs @@ -550,11 +550,11 @@ fn str_extend_chars() { //~|HELP try this //~|SUGGESTION s.push_str("abc") - s.push_str(def.as_str()); + s.push_str(&def); s.extend(def.chars()); //~^ERROR calling `.extend(_.chars())` //~|HELP try this - //~|SUGGESTION s.push_str(def.as_str()) + //~|SUGGESTION s.push_str(&def) s.extend(abc.chars().skip(1)); s.extend("abc".chars().skip(1)); From 8705f3d11cbbb2237f84a4da6c070a1fa76f8d48 Mon Sep 17 00:00:00 2001 From: Phil Turnbull Date: Sun, 20 Nov 2016 11:22:22 -0500 Subject: [PATCH 5/5] Remove mention of `.push_str(s)` being faster For the `.push_str(str.chars())` case the compiler will inline `push_str` and call `extend_from_slice` on the underlying vector, so this isn't actually faster. --- clippy_lints/src/methods.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/clippy_lints/src/methods.rs b/clippy_lints/src/methods.rs index d1192e3f2..2d6003004 100644 --- a/clippy_lints/src/methods.rs +++ b/clippy_lints/src/methods.rs @@ -493,7 +493,7 @@ declare_lint! { /// **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 and faster +/// **Why is this bad?** `.push_str(s)` is clearer /// /// **Known problems:** None. ///