From 2d55381a9698694fe74438ff7ab28f9f99a45ecd Mon Sep 17 00:00:00 2001 From: llogiq Date: Wed, 12 Aug 2015 15:50:56 +0200 Subject: [PATCH 1/7] added string_add lint and fixed string_add_assign + test --- README.md | 1 + src/lib.rs | 1 + src/strings.rs | 47 ++++++++++++++++++++++++++++++++--- tests/compile-fail/strings.rs | 13 +++++++--- 4 files changed, 54 insertions(+), 8 deletions(-) diff --git a/README.md b/README.md index 0a947e12e..4644bac94 100644 --- a/README.md +++ b/README.md @@ -30,6 +30,7 @@ Lints included in this crate: - `collapsible_if`: Warns on cases where two nested `if`-expressions can be collapsed into one, e.g. `if x { if y { foo() } }` can be written as `if x && y { foo() }` - `zero_width_space`: Warns on encountering a unicode zero-width space - `string_add_assign`: Warns on `x = x + ..` where `x` is a `String` and suggests using `push_str(..)` instead. + - `string_add`: Matches `x + ..` where `x` is a `String` and where `string_add_assign` doesn't warn. Allowed by default. - `needless_return`: Warns on using `return expr;` when a simple `expr` would suffice. - `let_and_return`: Warns on doing `let x = expr; x` at the end of a function. - `option_unwrap_used`: Warns when `Option.unwrap()` is used, and suggests `.expect()`. diff --git a/src/lib.rs b/src/lib.rs index 7b47edaa4..f2872a17b 100755 --- a/src/lib.rs +++ b/src/lib.rs @@ -56,6 +56,7 @@ pub fn plugin_registrar(reg: &mut Registry) { reg.register_lint_pass(box misc::ModuloOne as LintPassObject); reg.register_lint_pass(box unicode::Unicode as LintPassObject); reg.register_lint_pass(box strings::StringAdd as LintPassObject); + reg.register_lint_pass(box strings::StringAddAssign as LintPassObject); reg.register_lint_pass(box returns::ReturnPass as LintPassObject); reg.register_lint_pass(box methods::MethodsPass as LintPassObject); diff --git a/src/strings.rs b/src/strings.rs index 97016f362..60ea23556 100644 --- a/src/strings.rs +++ b/src/strings.rs @@ -9,7 +9,7 @@ use syntax::ast::*; use syntax::codemap::{Span, Spanned}; use eq_op::is_exp_equal; use types::match_ty_unwrap; -use utils::{match_def_path, span_lint, walk_ptrs_ty}; +use utils::{match_def_path, span_lint, walk_ptrs_ty, get_parent_expr}; declare_lint! { pub STRING_ADD_ASSIGN, @@ -17,10 +17,48 @@ declare_lint! { "Warn on `x = x + ..` where x is a `String`" } -#[derive(Copy,Clone)] +declare_lint! { + pub STRING_ADD, + Allow, + "Warn on `x + ..` where x is a `String`" +} + +#[derive(Copy, Clone)] pub struct StringAdd; impl LintPass for StringAdd { + fn get_lints(&self) -> LintArray { + lint_array!(STRING_ADD) + } + + fn check_expr(&mut self, cx: &Context, e: &Expr) { + if let &ExprBinary(Spanned{ node: BiAdd, .. }, ref left, _) = &e.node { + if is_string(cx, left) { + if let Allow = cx.current_level(STRING_ADD_ASSIGN) { + // the string_add_assign is allow, so no duplicates + } else { + let parent = get_parent_expr(cx, e); + if let Some(ref p) = parent { + if let &ExprAssign(ref target, _) = &p.node { + // avoid duplicate matches + if is_exp_equal(target, left) { return; } + } + } + } + //TODO check for duplicates + span_lint(cx, STRING_ADD, e.span, + "you add something to a string. \ + Consider using `String::push_str()` instead.") + } + } + } +} + + +#[derive(Copy, Clone)] +pub struct StringAddAssign; + +impl LintPass for StringAddAssign { fn get_lints(&self) -> LintArray { lint_array!(STRING_ADD_ASSIGN) } @@ -37,8 +75,9 @@ impl LintPass for StringAdd { } fn is_string(cx: &Context, e: &Expr) -> bool { - if let TyStruct(did, _) = walk_ptrs_ty(cx.tcx.expr_ty(e)).sty { - match_def_path(cx, did.did, &["std", "string", "String"]) + let ty = walk_ptrs_ty(cx.tcx.expr_ty(e)); + if let TyStruct(did, _) = ty.sty { + match_def_path(cx, did.did, &["collections", "string", "String"]) } else { false } } diff --git a/tests/compile-fail/strings.rs b/tests/compile-fail/strings.rs index 4b6f0bc88..e898a087d 100755 --- a/tests/compile-fail/strings.rs +++ b/tests/compile-fail/strings.rs @@ -2,11 +2,16 @@ #![plugin(clippy)] #![deny(string_add_assign)] - +#![deny(string_add)] fn main() { - let x = "".to_owned(); + let mut x = "".to_owned(); - for i in (1..3) { - x = x + "."; //~ERROR + for _ in (1..3) { + x = x + "."; //~ERROR you assign the result of adding something to this string. } + + let y = "".to_owned(); + let z = y + "..."; //~ERROR you add something to a string. + + assert_eq!(&x, &z); } From f0182ca6c809279843784d9669159157eedd2954 Mon Sep 17 00:00:00 2001 From: llogiq Date: Wed, 12 Aug 2015 15:57:50 +0200 Subject: [PATCH 2/7] fixed formatting --- src/strings.rs | 48 ++++++++++++++++++++++++------------------------ 1 file changed, 24 insertions(+), 24 deletions(-) diff --git a/src/strings.rs b/src/strings.rs index 60ea23556..47af63e6d 100644 --- a/src/strings.rs +++ b/src/strings.rs @@ -18,9 +18,9 @@ declare_lint! { } declare_lint! { - pub STRING_ADD, - Allow, - "Warn on `x + ..` where x is a `String`" + pub STRING_ADD, + Allow, + "Warn on `x + ..` where x is a `String`" } #[derive(Copy, Clone)] @@ -32,26 +32,26 @@ impl LintPass for StringAdd { } fn check_expr(&mut self, cx: &Context, e: &Expr) { - if let &ExprBinary(Spanned{ node: BiAdd, .. }, ref left, _) = &e.node { - if is_string(cx, left) { - if let Allow = cx.current_level(STRING_ADD_ASSIGN) { - // the string_add_assign is allow, so no duplicates - } else { - let parent = get_parent_expr(cx, e); - if let Some(ref p) = parent { - if let &ExprAssign(ref target, _) = &p.node { - // avoid duplicate matches - if is_exp_equal(target, left) { return; } - } - } - } - //TODO check for duplicates - span_lint(cx, STRING_ADD, e.span, - "you add something to a string. \ - Consider using `String::push_str()` instead.") - } - } - } + if let &ExprBinary(Spanned{ node: BiAdd, .. }, ref left, _) = &e.node { + if is_string(cx, left) { + if let Allow = cx.current_level(STRING_ADD_ASSIGN) { + // the string_add_assign is allow, so no duplicates + } else { + let parent = get_parent_expr(cx, e); + if let Some(ref p) = parent { + if let &ExprAssign(ref target, _) = &p.node { + // avoid duplicate matches + if is_exp_equal(target, left) { return; } + } + } + } + //TODO check for duplicates + span_lint(cx, STRING_ADD, e.span, + "you add something to a string. \ + Consider using `String::push_str()` instead.") + } + } + } } @@ -75,7 +75,7 @@ impl LintPass for StringAddAssign { } fn is_string(cx: &Context, e: &Expr) -> bool { - let ty = walk_ptrs_ty(cx.tcx.expr_ty(e)); + let ty = walk_ptrs_ty(cx.tcx.expr_ty(e)); if let TyStruct(did, _) = ty.sty { match_def_path(cx, did.did, &["collections", "string", "String"]) } else { false } From e6e036ec20d2646c5682e4a7e18039b8925ce575 Mon Sep 17 00:00:00 2001 From: llogiq Date: Wed, 12 Aug 2015 16:42:42 +0200 Subject: [PATCH 3/7] pulled strings passes together, added more tests --- src/lib.rs | 1 - src/strings.rs | 18 ++------------ tests/compile-fail/strings.rs | 45 ++++++++++++++++++++++++++++++++--- 3 files changed, 44 insertions(+), 20 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index f2872a17b..7b47edaa4 100755 --- a/src/lib.rs +++ b/src/lib.rs @@ -56,7 +56,6 @@ pub fn plugin_registrar(reg: &mut Registry) { reg.register_lint_pass(box misc::ModuloOne as LintPassObject); reg.register_lint_pass(box unicode::Unicode as LintPassObject); reg.register_lint_pass(box strings::StringAdd as LintPassObject); - reg.register_lint_pass(box strings::StringAddAssign as LintPassObject); reg.register_lint_pass(box returns::ReturnPass as LintPassObject); reg.register_lint_pass(box methods::MethodsPass as LintPassObject); diff --git a/src/strings.rs b/src/strings.rs index 47af63e6d..aa0e8499f 100644 --- a/src/strings.rs +++ b/src/strings.rs @@ -28,7 +28,7 @@ pub struct StringAdd; impl LintPass for StringAdd { fn get_lints(&self) -> LintArray { - lint_array!(STRING_ADD) + lint_array!(STRING_ADD, STRING_ADD_ASSIGN) } fn check_expr(&mut self, cx: &Context, e: &Expr) { @@ -50,21 +50,7 @@ impl LintPass for StringAdd { "you add something to a string. \ Consider using `String::push_str()` instead.") } - } - } -} - - -#[derive(Copy, Clone)] -pub struct StringAddAssign; - -impl LintPass for StringAddAssign { - fn get_lints(&self) -> LintArray { - lint_array!(STRING_ADD_ASSIGN) - } - - fn check_expr(&mut self, cx: &Context, e: &Expr) { - if let &ExprAssign(ref target, ref src) = &e.node { + } else if let &ExprAssign(ref target, ref src) = &e.node { if is_string(cx, target) && is_add(src, target) { span_lint(cx, STRING_ADD_ASSIGN, e.span, "you assign the result of adding something to this string. \ diff --git a/tests/compile-fail/strings.rs b/tests/compile-fail/strings.rs index e898a087d..02ebca2fe 100755 --- a/tests/compile-fail/strings.rs +++ b/tests/compile-fail/strings.rs @@ -1,9 +1,37 @@ #![feature(plugin)] #![plugin(clippy)] -#![deny(string_add_assign)] -#![deny(string_add)] -fn main() { +#[deny(string_add)] +#[allow(string_add_assign)] +fn add_only() { // ignores assignment distinction + let mut x = "".to_owned(); + + for _ in (1..3) { + x = x + "."; //~ERROR you add something to a string. + } + + let y = "".to_owned(); + let z = y + "..."; //~ERROR you add something to a string. + + assert_eq!(&x, &z); +} + +#[deny(string_add_assign)] +fn add_assign_only() { + let mut x = "".to_owned(); + + for _ in (1..3) { + x = x + "."; //~ERROR you assign the result of adding something to this string. + } + + let y = "".to_owned(); + let z = y + "..."; + + assert_eq!(&x, &z); +} + +#[deny(string_add, string_add_assign)] +fn both() { let mut x = "".to_owned(); for _ in (1..3) { @@ -15,3 +43,14 @@ fn main() { assert_eq!(&x, &z); } + +fn main() { + add_only(); + add_assign_only(); + both(); + + // the add is only caught for String + let mut x = 1; + x = x + 1; + assert_eq!(2, x); +} From 801f01d0012d48873050c27cf9876b3fc56509c5 Mon Sep 17 00:00:00 2001 From: llogiq Date: Wed, 12 Aug 2015 16:50:43 +0200 Subject: [PATCH 4/7] added `string_add` to `clippy` lint group --- src/lib.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/lib.rs b/src/lib.rs index 7b47edaa4..46cc85e85 100755 --- a/src/lib.rs +++ b/src/lib.rs @@ -78,6 +78,7 @@ pub fn plugin_registrar(reg: &mut Registry) { collapsible_if::COLLAPSIBLE_IF, unicode::ZERO_WIDTH_SPACE, strings::STRING_ADD_ASSIGN, + strings::STRING_ADD, returns::NEEDLESS_RETURN, misc::MODULO_ONE, methods::OPTION_UNWRAP_USED, From 30a6764adb44ac93b389128aa10089546c3c65c1 Mon Sep 17 00:00:00 2001 From: Andre Bogus Date: Wed, 12 Aug 2015 21:17:21 +0200 Subject: [PATCH 5/7] grammar --- src/strings.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/strings.rs b/src/strings.rs index aa0e8499f..33db980c0 100644 --- a/src/strings.rs +++ b/src/strings.rs @@ -47,13 +47,13 @@ impl LintPass for StringAdd { } //TODO check for duplicates span_lint(cx, STRING_ADD, e.span, - "you add something to a string. \ + "you added something to a string. \ Consider using `String::push_str()` instead.") } } else if let &ExprAssign(ref target, ref src) = &e.node { if is_string(cx, target) && is_add(src, target) { span_lint(cx, STRING_ADD_ASSIGN, e.span, - "you assign the result of adding something to this string. \ + "you assigned the result of adding something to this string. \ Consider using `String::push_str()` instead.") } } From 1f8c29c6ade779f322adcc6900d2e615a583e59a Mon Sep 17 00:00:00 2001 From: Andre Bogus Date: Wed, 12 Aug 2015 21:39:42 +0200 Subject: [PATCH 6/7] fixed error messages in compile-fail test --- tests/compile-fail/strings.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/tests/compile-fail/strings.rs b/tests/compile-fail/strings.rs index 02ebca2fe..680ebb73d 100755 --- a/tests/compile-fail/strings.rs +++ b/tests/compile-fail/strings.rs @@ -7,11 +7,11 @@ fn add_only() { // ignores assignment distinction let mut x = "".to_owned(); for _ in (1..3) { - x = x + "."; //~ERROR you add something to a string. + x = x + "."; //~ERROR you added something to a string. } let y = "".to_owned(); - let z = y + "..."; //~ERROR you add something to a string. + let z = y + "..."; //~ERROR you added something to a string. assert_eq!(&x, &z); } @@ -21,7 +21,7 @@ fn add_assign_only() { let mut x = "".to_owned(); for _ in (1..3) { - x = x + "."; //~ERROR you assign the result of adding something to this string. + x = x + "."; //~ERROR you assigned the result of adding something to this string. } let y = "".to_owned(); @@ -35,11 +35,11 @@ fn both() { let mut x = "".to_owned(); for _ in (1..3) { - x = x + "."; //~ERROR you assign the result of adding something to this string. + x = x + "."; //~ERROR you assigned the result of adding something to this string. } let y = "".to_owned(); - let z = y + "..."; //~ERROR you add something to a string. + let z = y + "..."; //~ERROR you added something to a string. assert_eq!(&x, &z); } From 71b46d9ecdd4adac122199669596370b14a4bb02 Mon Sep 17 00:00:00 2001 From: llogiq Date: Thu, 13 Aug 2015 11:35:30 +0200 Subject: [PATCH 7/7] improved string_add/string_add_assign messages, Allow-by-default string_add_assign --- README.md | 2 +- src/strings.rs | 7 ++++--- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/README.md b/README.md index 4644bac94..d2e5049f8 100644 --- a/README.md +++ b/README.md @@ -29,7 +29,7 @@ Lints included in this crate: - `inline_always`: Warns on `#[inline(always)]`, because in most cases it is a bad idea - `collapsible_if`: Warns on cases where two nested `if`-expressions can be collapsed into one, e.g. `if x { if y { foo() } }` can be written as `if x && y { foo() }` - `zero_width_space`: Warns on encountering a unicode zero-width space - - `string_add_assign`: Warns on `x = x + ..` where `x` is a `String` and suggests using `push_str(..)` instead. + - `string_add_assign`: Warns on `x = x + ..` where `x` is a `String` and suggests using `push_str(..)` instead. Allowed by default. - `string_add`: Matches `x + ..` where `x` is a `String` and where `string_add_assign` doesn't warn. Allowed by default. - `needless_return`: Warns on using `return expr;` when a simple `expr` would suffice. - `let_and_return`: Warns on doing `let x = expr; x` at the end of a function. diff --git a/src/strings.rs b/src/strings.rs index 33db980c0..226b851e9 100644 --- a/src/strings.rs +++ b/src/strings.rs @@ -13,14 +13,15 @@ use utils::{match_def_path, span_lint, walk_ptrs_ty, get_parent_expr}; declare_lint! { pub STRING_ADD_ASSIGN, - Warn, - "Warn on `x = x + ..` where x is a `String`" + Allow, + "expressions of the form `x = x + ..` where x is a `String`" } declare_lint! { pub STRING_ADD, Allow, - "Warn on `x + ..` where x is a `String`" + "expressions of the form `x + ..` where x is a `String` \ + unless `string_add_assign` matches" } #[derive(Copy, Clone)]