From ed96583677a955e4dcd935e35031e434ac9ea5e5 Mon Sep 17 00:00:00 2001 From: llogiq Date: Mon, 25 Jan 2016 14:02:47 +0100 Subject: [PATCH 1/5] extend_from_slice lint --- README.md | 3 ++- src/lib.rs | 1 + src/methods.rs | 46 +++++++++++++++++++++++++++++++++-- tests/compile-fail/methods.rs | 7 ++++++ 4 files changed, 54 insertions(+), 3 deletions(-) diff --git a/README.md b/README.md index 83ff6daa9f58..290684d3bad1 100644 --- a/README.md +++ b/README.md @@ -6,7 +6,7 @@ A collection of lints to catch common mistakes and improve your Rust code. [Jump to usage instructions](#usage) ##Lints -There are 98 lints included in this crate: +There are 99 lints included in this crate: name | default | meaning ---------------------------------------------------------------------------------------------------------------|---------|------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ @@ -33,6 +33,7 @@ name [expl_impl_clone_on_copy](https://github.com/Manishearth/rust-clippy/wiki#expl_impl_clone_on_copy) | warn | implementing `Clone` explicitly on `Copy` types [explicit_counter_loop](https://github.com/Manishearth/rust-clippy/wiki#explicit_counter_loop) | warn | for-looping with an explicit counter when `_.enumerate()` would do [explicit_iter_loop](https://github.com/Manishearth/rust-clippy/wiki#explicit_iter_loop) | warn | for-looping over `_.iter()` or `_.iter_mut()` when `&_` or `&mut _` would do +[extend_from_slice](https://github.com/Manishearth/rust-clippy/wiki#extend_from_slice) | warn | `.extend_from_slice(_)` is a faster way to extend a Vec by a slice [filter_next](https://github.com/Manishearth/rust-clippy/wiki#filter_next) | warn | using `filter(p).next()`, which is more succinctly expressed as `.find(p)` [float_cmp](https://github.com/Manishearth/rust-clippy/wiki#float_cmp) | warn | using `==` or `!=` on float values (as floating-point operations usually involve rounding errors, it is always better to check for approximate equality within small bounds) [identity_op](https://github.com/Manishearth/rust-clippy/wiki#identity_op) | warn | using identity operations, e.g. `x + 0` or `y / 1` diff --git a/src/lib.rs b/src/lib.rs index c43c01268fea..f44191b562de 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -199,6 +199,7 @@ pub fn plugin_registrar(reg: &mut Registry) { matches::MATCH_REF_PATS, matches::SINGLE_MATCH, methods::CHARS_NEXT_CMP, + methods::EXTEND_FROM_SLICE, methods::FILTER_NEXT, methods::OK_EXPECT, methods::OPTION_MAP_UNWRAP_OR, diff --git a/src/methods.rs b/src/methods.rs index 6338961b56b3..0f09d67485a3 100644 --- a/src/methods.rs +++ b/src/methods.rs @@ -14,7 +14,7 @@ }; use utils::{ BTREEMAP_ENTRY_PATH, DEFAULT_TRAIT_PATH, HASHMAP_ENTRY_PATH, OPTION_PATH, RESULT_PATH, - STRING_PATH + STRING_PATH, VEC_PATH, }; use utils::MethodArgs; use rustc::middle::cstore::CrateStore; @@ -212,9 +212,20 @@ declare_lint!(pub OR_FUN_CALL, Warn, "using any `*or` method when the `*or_else` would do"); +/// **What it does:** This lint `Warn`s on using `.extend(s)` on a `vec` to extend the vec by a slice. +/// +/// **Why is this bad?** Since Rust 1.6, the `extend_from_slice(_)` method is stable and at least for now faster. +/// +/// **Known problems:** None. +/// +/// **Example:** `my_vec.extend(&xs)` +declare_lint!(pub EXTEND_FROM_SLICE, Warn, + "`.extend_from_slice(_)` is a faster way to extend a Vec by a slice"); + impl LintPass for MethodsPass { fn get_lints(&self) -> LintArray { - lint_array!(OPTION_UNWRAP_USED, + lint_array!(EXTEND_FROM_SLICE, + OPTION_UNWRAP_USED, RESULT_UNWRAP_USED, STR_TO_STRING, STRING_TO_STRING, @@ -256,6 +267,8 @@ fn check_expr(&mut self, cx: &LateContext, expr: &Expr) { lint_search_is_some(cx, expr, "position", arglists[0], arglists[1]); } else if let Some(arglists) = method_chain_args(expr, &["rposition", "is_some"]) { lint_search_is_some(cx, expr, "rposition", arglists[0], arglists[1]); + } else if let Some(arglists) = method_chain_args(expr, &["extend"]) { + lint_extend(cx, expr, arglists[0]); } lint_or_fun_call(cx, expr, &name.node.as_str(), &args); @@ -427,6 +440,35 @@ fn check_general_case( } } +fn lint_extend(cx: &LateContext, expr: &Expr, args: &MethodArgs) { + let (obj_ty, _) = walk_ptrs_ty_depth(cx.tcx.expr_ty(&args[0])); + let arg_ty = cx.tcx.expr_ty(&args[1]); + + if !match_type(cx, obj_ty, &VEC_PATH) { + return; // for your Vecs only + } + + if derefs_to_slice(&arg_ty) { + span_lint(cx, EXTEND_FROM_SLICE, expr.span, + &format!("use of `extend` to extend a Vec by a slice")) + .span_suggestion(expr.span, "try this", + format!("{}.extend_from_slice({})", + snippet(cx, args[0].span, "_"), + snippet(cx, args[1].span, "_"))); + } +} + +fn derefs_to_slice(ty: &ty::Ty) -> bool { + match ty.sty { + ty::TySlice(_) | + ty::TyStr => true, + ty::TyBox(ref inner) => derefs_to_slice(inner), + ty::TyArray(_, size) => size < 32, + ty::TyRef(_, ty::TypeAndMut { ty: ref t, .. }) => derefs_to_slice(t), + _ => false + } +} + #[allow(ptr_arg)] // Type of MethodArgs is potentially a Vec /// lint use of `unwrap()` for `Option`s and `Result`s diff --git a/tests/compile-fail/methods.rs b/tests/compile-fail/methods.rs index 535e8cc4a26c..64b31b755971 100644 --- a/tests/compile-fail/methods.rs +++ b/tests/compile-fail/methods.rs @@ -304,3 +304,10 @@ fn starts_with() { //~| HELP like this //~| SUGGESTION !"".starts_with(' ') } + +fn use_extend_from_slice() { + let mut v : Vec<&'static str> = vec![]; + v.extend(&["Hello", "World"]); //~ERROR use of `extend` + + +} From 2d97f916ebdcb53f22b65e8142382c9624079297 Mon Sep 17 00:00:00 2001 From: llogiq Date: Mon, 25 Jan 2016 19:46:56 +0100 Subject: [PATCH 2/5] added more test, now works with vecs and iter --- src/methods.rs | 16 ++++++++++------ tests/compile-fail/methods.rs | 7 +++++-- 2 files changed, 15 insertions(+), 8 deletions(-) diff --git a/src/methods.rs b/src/methods.rs index 0f09d67485a3..5377dd1bc521 100644 --- a/src/methods.rs +++ b/src/methods.rs @@ -448,7 +448,7 @@ fn lint_extend(cx: &LateContext, expr: &Expr, args: &MethodArgs) { return; // for your Vecs only } - if derefs_to_slice(&arg_ty) { + if derefs_to_slice(cx, &args[1], &arg_ty) { span_lint(cx, EXTEND_FROM_SLICE, expr.span, &format!("use of `extend` to extend a Vec by a slice")) .span_suggestion(expr.span, "try this", @@ -458,13 +458,17 @@ fn lint_extend(cx: &LateContext, expr: &Expr, args: &MethodArgs) { } } -fn derefs_to_slice(ty: &ty::Ty) -> bool { +fn derefs_to_slice(cx: &LateContext, expr: &Expr, ty: &ty::Ty) -> bool { + if let ExprMethodCall(name, _, ref args) = expr.node { + return &name.node.as_str() == &"iter" && + derefs_to_slice(cx, &args[0], &cx.tcx.expr_ty(&args[0])) + } match ty.sty { - ty::TySlice(_) | - ty::TyStr => true, - ty::TyBox(ref inner) => derefs_to_slice(inner), + ty::TyStruct(..) => match_type(cx, ty, &VEC_PATH), + ty::TySlice(_) => true, ty::TyArray(_, size) => size < 32, - ty::TyRef(_, ty::TypeAndMut { ty: ref t, .. }) => derefs_to_slice(t), + ty::TyRef(_, ty::TypeAndMut { ty: ref inner, .. }) | + ty::TyBox(ref inner) => derefs_to_slice(cx, expr, inner), _ => false } } diff --git a/tests/compile-fail/methods.rs b/tests/compile-fail/methods.rs index 64b31b755971..752427847f31 100644 --- a/tests/compile-fail/methods.rs +++ b/tests/compile-fail/methods.rs @@ -308,6 +308,9 @@ fn starts_with() { fn use_extend_from_slice() { let mut v : Vec<&'static str> = vec![]; v.extend(&["Hello", "World"]); //~ERROR use of `extend` - - + v.extend(vec!["Some", "more"]); //~ERROR use of `extend` + v.extend(vec!["And", "even", "more"].iter()); //~ERROR use of `extend` + let o : Option<&'static str> = None; + v.extend(o); + v.extend(Some("Bye")); } From d152e5c683165b9ba43de1e65aadd08f137d999d Mon Sep 17 00:00:00 2001 From: llogiq Date: Tue, 26 Jan 2016 23:51:06 +0100 Subject: [PATCH 3/5] fixed argument check --- src/methods.rs | 22 ++++++++++++++-------- tests/compile-fail/methods.rs | 3 ++- 2 files changed, 16 insertions(+), 9 deletions(-) diff --git a/src/methods.rs b/src/methods.rs index 5377dd1bc521..f1021c839cd4 100644 --- a/src/methods.rs +++ b/src/methods.rs @@ -442,12 +442,10 @@ fn check_general_case( fn lint_extend(cx: &LateContext, expr: &Expr, args: &MethodArgs) { let (obj_ty, _) = walk_ptrs_ty_depth(cx.tcx.expr_ty(&args[0])); - let arg_ty = cx.tcx.expr_ty(&args[1]); - if !match_type(cx, obj_ty, &VEC_PATH) { - return; // for your Vecs only + return; } - + let arg_ty = cx.tcx.expr_ty(&args[1]); if derefs_to_slice(cx, &args[1], &arg_ty) { span_lint(cx, EXTEND_FROM_SLICE, expr.span, &format!("use of `extend` to extend a Vec by a slice")) @@ -459,16 +457,24 @@ fn lint_extend(cx: &LateContext, expr: &Expr, args: &MethodArgs) { } fn derefs_to_slice(cx: &LateContext, expr: &Expr, ty: &ty::Ty) -> bool { + fn may_slice(cx: &LateContext, expr: &Expr, ty: &ty::Ty) -> bool { + match ty.sty { + ty::TySlice(_) => true, + ty::TyStruct(..) => match_type(cx, ty, &VEC_PATH), + ty::TyArray(_, size) => size < 32, + ty::TyRef(_, ty::TypeAndMut { ty: ref inner, .. }) | + ty::TyBox(ref inner) => may_slice(cx, expr, inner), + _ => false + } + } if let ExprMethodCall(name, _, ref args) = expr.node { return &name.node.as_str() == &"iter" && - derefs_to_slice(cx, &args[0], &cx.tcx.expr_ty(&args[0])) + may_slice(cx, &args[0], &cx.tcx.expr_ty(&args[0])) } match ty.sty { - ty::TyStruct(..) => match_type(cx, ty, &VEC_PATH), ty::TySlice(_) => true, - ty::TyArray(_, size) => size < 32, ty::TyRef(_, ty::TypeAndMut { ty: ref inner, .. }) | - ty::TyBox(ref inner) => derefs_to_slice(cx, expr, inner), + ty::TyBox(ref inner) => may_slice(cx, expr, inner), _ => false } } diff --git a/tests/compile-fail/methods.rs b/tests/compile-fail/methods.rs index 752427847f31..3156a38cdebb 100644 --- a/tests/compile-fail/methods.rs +++ b/tests/compile-fail/methods.rs @@ -308,9 +308,10 @@ fn starts_with() { fn use_extend_from_slice() { let mut v : Vec<&'static str> = vec![]; v.extend(&["Hello", "World"]); //~ERROR use of `extend` - v.extend(vec!["Some", "more"]); //~ERROR use of `extend` + v.extend(&vec!["Some", "more"]); //~ERROR use of `extend` v.extend(vec!["And", "even", "more"].iter()); //~ERROR use of `extend` let o : Option<&'static str> = None; v.extend(o); v.extend(Some("Bye")); + v.extend(vec!["Not", "like", "this"]); } From 5d5e50d67edd071aa82035435d522577bd03b2d8 Mon Sep 17 00:00:00 2001 From: llogiq Date: Wed, 27 Jan 2016 14:51:30 +0100 Subject: [PATCH 4/5] fixed suggestion for iter case --- src/methods.rs | 36 +++++++++++++++++++++-------------- tests/compile-fail/methods.rs | 1 + 2 files changed, 23 insertions(+), 14 deletions(-) diff --git a/src/methods.rs b/src/methods.rs index f1021c839cd4..a5f27440f941 100644 --- a/src/methods.rs +++ b/src/methods.rs @@ -446,36 +446,44 @@ fn lint_extend(cx: &LateContext, expr: &Expr, args: &MethodArgs) { return; } let arg_ty = cx.tcx.expr_ty(&args[1]); - if derefs_to_slice(cx, &args[1], &arg_ty) { + if let Some((span, r)) = derefs_to_slice(cx, &args[1], &arg_ty) { span_lint(cx, EXTEND_FROM_SLICE, expr.span, &format!("use of `extend` to extend a Vec by a slice")) .span_suggestion(expr.span, "try this", - format!("{}.extend_from_slice({})", + format!("{}.extend_from_slice({}{})", snippet(cx, args[0].span, "_"), - snippet(cx, args[1].span, "_"))); + r, snippet(cx, span, "_"))); } } -fn derefs_to_slice(cx: &LateContext, expr: &Expr, ty: &ty::Ty) -> bool { - fn may_slice(cx: &LateContext, expr: &Expr, ty: &ty::Ty) -> bool { +fn derefs_to_slice(cx: &LateContext, expr: &Expr, ty: &ty::Ty) + -> Option<(Span, &'static str)> { + fn may_slice(cx: &LateContext, ty: &ty::Ty) -> bool { match ty.sty { ty::TySlice(_) => true, ty::TyStruct(..) => match_type(cx, ty, &VEC_PATH), ty::TyArray(_, size) => size < 32, ty::TyRef(_, ty::TypeAndMut { ty: ref inner, .. }) | - ty::TyBox(ref inner) => may_slice(cx, expr, inner), + ty::TyBox(ref inner) => may_slice(cx, inner), _ => false } } if let ExprMethodCall(name, _, ref args) = expr.node { - return &name.node.as_str() == &"iter" && - may_slice(cx, &args[0], &cx.tcx.expr_ty(&args[0])) - } - match ty.sty { - ty::TySlice(_) => true, - ty::TyRef(_, ty::TypeAndMut { ty: ref inner, .. }) | - ty::TyBox(ref inner) => may_slice(cx, expr, inner), - _ => false + if &name.node.as_str() == &"iter" && + may_slice(cx, &cx.tcx.expr_ty(&args[0])) { + Some((args[0].span, "&")) + } else { + None + } + } else { + match ty.sty { + ty::TySlice(_) => Some((expr.span, "")), + ty::TyRef(_, ty::TypeAndMut { ty: ref inner, .. }) | + ty::TyBox(ref inner) => if may_slice(cx, inner) { + Some((expr.span, "")) + } else { None }, + _ => None + } } } diff --git a/tests/compile-fail/methods.rs b/tests/compile-fail/methods.rs index 3156a38cdebb..5172aa9e9df1 100644 --- a/tests/compile-fail/methods.rs +++ b/tests/compile-fail/methods.rs @@ -314,4 +314,5 @@ fn use_extend_from_slice() { v.extend(o); v.extend(Some("Bye")); v.extend(vec!["Not", "like", "this"]); + v.extend(["Nor", "this"].iter()); } From a1ac3125de6ae7cb0ffffd845df28b3ba3872a19 Mon Sep 17 00:00:00 2001 From: llogiq Date: Wed, 27 Jan 2016 20:13:15 +0100 Subject: [PATCH 5/5] fixed and extended tests --- tests/compile-fail/methods.rs | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/tests/compile-fail/methods.rs b/tests/compile-fail/methods.rs index 5172aa9e9df1..c72e602ac2bf 100644 --- a/tests/compile-fail/methods.rs +++ b/tests/compile-fail/methods.rs @@ -308,11 +308,18 @@ fn starts_with() { fn use_extend_from_slice() { let mut v : Vec<&'static str> = vec![]; v.extend(&["Hello", "World"]); //~ERROR use of `extend` - v.extend(&vec!["Some", "more"]); //~ERROR use of `extend` + v.extend(&vec!["Some", "more"]); + //~^ERROR use of `extend` + //~| HELP try this + //~| SUGGESTION v.extend_from_slice(&vec!["Some", "more"]); + v.extend(vec!["And", "even", "more"].iter()); //~ERROR use of `extend` let o : Option<&'static str> = None; v.extend(o); v.extend(Some("Bye")); v.extend(vec!["Not", "like", "this"]); - v.extend(["Nor", "this"].iter()); + v.extend(["But", "this"].iter()); + //~^ERROR use of `extend + //~| HELP try this + //~| SUGGESTION v.extend_from_slice(&["But", "this"]); }