From db205c82a4125446d47296c3d3463c81efb0dd3d Mon Sep 17 00:00:00 2001 From: mcarton Date: Tue, 2 Feb 2016 22:35:01 +0100 Subject: [PATCH] Add a lint about using `clone` on `Copy` types --- README.md | 3 ++- src/lib.rs | 1 + src/loops.rs | 2 +- src/methods.rs | 36 ++++++++++++++++++++++++++++----- src/misc_early.rs | 2 +- tests/compile-fail/map_clone.rs | 2 +- tests/compile-fail/methods.rs | 15 ++++++++++++-- 7 files changed, 50 insertions(+), 11 deletions(-) diff --git a/README.md b/README.md index 0c5b12df8ae..d7086a6dac8 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 107 lints included in this crate: +There are 108 lints included in this crate: name | default | meaning ---------------------------------------------------------------------------------------------------------------|---------|------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ @@ -22,6 +22,7 @@ name [cast_sign_loss](https://github.com/Manishearth/rust-clippy/wiki#cast_sign_loss) | allow | casts from signed types to unsigned types, e.g `x as u32` where `x: i32` [char_lit_as_u8](https://github.com/Manishearth/rust-clippy/wiki#char_lit_as_u8) | warn | Casting a character literal to u8 [chars_next_cmp](https://github.com/Manishearth/rust-clippy/wiki#chars_next_cmp) | warn | using `.chars().next()` to check if a string starts with a char +[clone_on_copy](https://github.com/Manishearth/rust-clippy/wiki#clone_on_copy) | warn | using `clone` on a `Copy` type [cmp_nan](https://github.com/Manishearth/rust-clippy/wiki#cmp_nan) | deny | comparisons to NAN (which will always return false, which is probably not intended) [cmp_owned](https://github.com/Manishearth/rust-clippy/wiki#cmp_owned) | warn | creating owned instances for comparing with others, e.g. `x == "foo".to_string()` [collapsible_if](https://github.com/Manishearth/rust-clippy/wiki#collapsible_if) | warn | two nested `if`-expressions can be collapsed into one, e.g. `if x { if y { foo() } }` can be written as `if x && y { foo() }` and an `else { if .. } expression can be collapsed to `else if` diff --git a/src/lib.rs b/src/lib.rs index e7302ea5829..36954284184 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -214,6 +214,7 @@ pub fn plugin_registrar(reg: &mut Registry) { matches::MATCH_REF_PATS, matches::SINGLE_MATCH, methods::CHARS_NEXT_CMP, + methods::CLONE_ON_COPY, methods::EXTEND_FROM_SLICE, methods::FILTER_NEXT, methods::OK_EXPECT, diff --git a/src/loops.rs b/src/loops.rs index 20c75e08d23..49e073aacd0 100644 --- a/src/loops.rs +++ b/src/loops.rs @@ -475,7 +475,7 @@ fn check_for_loop_explicit_counter(cx: &LateContext, arg: &Expr, body: &Expr, ex let mut visitor2 = InitializeVisitor { cx: cx, end_expr: expr, - var_id: id.clone(), + var_id: *id, state: VarState::IncrOnce, name: None, depth: 0, diff --git a/src/methods.rs b/src/methods.rs index 5768a6e8f85..0584d39330d 100644 --- a/src/methods.rs +++ b/src/methods.rs @@ -219,6 +219,17 @@ declare_lint!(pub OR_FUN_CALL, Warn, declare_lint!(pub EXTEND_FROM_SLICE, Warn, "`.extend_from_slice(_)` is a faster way to extend a Vec by a slice"); +/// **What it does:** This lint warns on using `.clone()` on a `Copy` type. +/// +/// **Why is this bad?** The only reason `Copy` types implement `Clone` is for generics, not for +/// using the `clone` method on a concrete type. +/// +/// **Known problems:** None. +/// +/// **Example:** `42u64.clone()` +declare_lint!(pub CLONE_ON_COPY, Warn, + "using `clone` on a `Copy` type"); + impl LintPass for MethodsPass { fn get_lints(&self) -> LintArray { lint_array!(EXTEND_FROM_SLICE, @@ -233,7 +244,8 @@ impl LintPass for MethodsPass { OPTION_MAP_UNWRAP_OR, OPTION_MAP_UNWRAP_OR_ELSE, OR_FUN_CALL, - CHARS_NEXT_CMP) + CHARS_NEXT_CMP, + CLONE_ON_COPY) } } @@ -269,6 +281,7 @@ impl LateLintPass for MethodsPass { } lint_or_fun_call(cx, expr, &name.node.as_str(), &args); + lint_clone_on_copy(cx, expr, &name.node.as_str(), &args); } ExprBinary(op, ref lhs, ref rhs) if op.node == BiEq || op.node == BiNe => { if !lint_chars_next(cx, expr, lhs, rhs, op.node == BiEq) { @@ -439,6 +452,19 @@ fn lint_or_fun_call(cx: &LateContext, expr: &Expr, name: &str, args: &[P]) } } +/// Checks for the `CLONE_ON_COPY` lint. +fn lint_clone_on_copy(cx: &LateContext, expr: &Expr, name: &str, args: &[P]) { + if args.len() == 1 && name == "clone" { + let ty = cx.tcx.expr_ty(expr); + let parent = cx.tcx.map.get_parent(expr.id); + let parameter_environment = ty::ParameterEnvironment::for_item(cx.tcx, parent); + + if !ty.moves_by_default(¶meter_environment, expr.span) { + span_lint(cx, CLONE_ON_COPY, expr.span, "using `clone` on a `Copy` type"); + } + } +} + fn lint_extend(cx: &LateContext, expr: &Expr, args: &MethodArgs) { let (obj_ty, _) = walk_ptrs_ty_depth(cx.tcx.expr_ty(&args[0])); if !match_type(cx, obj_ty, &VEC_PATH) { @@ -701,7 +727,7 @@ fn lint_chars_next(cx: &LateContext, expr: &Expr, chain: &Expr, other: &Expr, eq false } -// Given a `Result` type, return its error type (`E`) +/// Given a `Result` type, return its error type (`E`). fn get_error_type<'a>(cx: &LateContext, ty: ty::Ty<'a>) -> Option> { if !match_type(cx, ty, &RESULT_PATH) { return None; @@ -714,9 +740,9 @@ fn get_error_type<'a>(cx: &LateContext, ty: ty::Ty<'a>) -> Option> { None } -// This checks whether a given type is known to implement Debug. It's -// conservative, i.e. it should not return false positives, but will return -// false negatives. +/// This checks whether a given type is known to implement Debug. It's +/// conservative, i.e. it should not return false positives, but will return +/// false negatives. fn has_debug_impl<'a, 'b>(ty: ty::Ty<'a>, cx: &LateContext<'b, 'a>) -> bool { let no_ref_ty = walk_ptrs_ty(ty); let debug = match cx.tcx.lang_items.debug_trait() { diff --git a/src/misc_early.rs b/src/misc_early.rs index a90c901df8f..1573aff2a4d 100644 --- a/src/misc_early.rs +++ b/src/misc_early.rs @@ -110,7 +110,7 @@ impl EarlyLintPass for MiscEarly { arg_name[1..].to_owned())); } } else { - registered_names.insert(arg_name, arg.pat.span.clone()); + registered_names.insert(arg_name, arg.pat.span); } } } diff --git a/tests/compile-fail/map_clone.rs b/tests/compile-fail/map_clone.rs index 2e60300afda..bd630211f19 100644 --- a/tests/compile-fail/map_clone.rs +++ b/tests/compile-fail/map_clone.rs @@ -3,7 +3,7 @@ #![plugin(clippy)] #![deny(map_clone)] -#![allow(unused)] +#![allow(clone_on_copy, unused)] use std::ops::Deref; diff --git a/tests/compile-fail/methods.rs b/tests/compile-fail/methods.rs index c72e602ac2b..f998a83e831 100644 --- a/tests/compile-fail/methods.rs +++ b/tests/compile-fail/methods.rs @@ -312,14 +312,25 @@ fn use_extend_from_slice() { //~^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(["But", "this"].iter()); + v.extend(["But", "this"].iter()); //~^ERROR use of `extend //~| HELP try this //~| SUGGESTION v.extend_from_slice(&["But", "this"]); } + +fn clone_on_copy() { + 42.clone(); //~ERROR using `clone` on a `Copy` type + vec![1].clone(); // ok, not a Copy type + Some(vec![1]).clone(); // ok, not a Copy type +} + +fn clone_on_copy_generic(t: T) { + t.clone(); //~ERROR using `clone` on a `Copy` type + Some(t).clone(); //~ERROR using `clone` on a `Copy` type +}