Merge pull request #122 from Manishearth/strings

New string_add_assign lint

README.md

@@ -29,6 +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.
 
 To use, add the following lines to your Cargo.toml:
 

src/eq_op.rs

@@ -30,7 +30,7 @@ impl LintPass for EqOp {
     }
 }
 
-fn is_exp_equal(left : &Expr, right : &Expr) -> bool {
+pub fn is_exp_equal(left : &Expr, right : &Expr) -> bool {
     match (&left.node, &right.node) {
         (&ExprBinary(ref lop, ref ll, ref lr), 
                 &ExprBinary(ref rop, ref rl, ref rr)) => 
@@ -73,10 +73,9 @@ fn is_exps_equal(left : &[P<Expr>], right : &[P<Expr>]) -> bool {
 
 fn is_path_equal(left : &Path, right : &Path) -> bool {
     // The == of idents doesn't work with different contexts,
-    // we have to be explicit about hygeine
-	left.global == right.global
-    && left.segments.iter().zip(right.segments.iter())
-           .all( |(l,r)| l.identifier.name == r.identifier.name
+    // we have to be explicit about hygiene
+    left.global == right.global && over(&left.segments, &right.segments, 
+        |l, r| l.identifier.name == r.identifier.name
               && l.identifier.ctxt == r.identifier.ctxt
                && l.parameters == r.parameters)
 }

src/lib.rs

@@ -28,6 +28,7 @@ pub mod attrs;
 pub mod collapsible_if;
 pub mod unicode;
 pub mod utils;
+pub mod strings;
 
 #[plugin_registrar]
 pub fn plugin_registrar(reg: &mut Registry) {
@@ -51,6 +52,7 @@ pub fn plugin_registrar(reg: &mut Registry) {
     reg.register_lint_pass(box attrs::AttrPass as LintPassObject);
     reg.register_lint_pass(box collapsible_if::CollapsibleIf as LintPassObject);
     reg.register_lint_pass(box unicode::Unicode as LintPassObject);
+    reg.register_lint_pass(box strings::StringAdd as LintPassObject);
     
     reg.register_lint_group("clippy", vec![types::BOX_VEC, types::LINKEDLIST,
                                            misc::SINGLE_MATCH, misc::STR_TO_STRING,
@@ -70,5 +72,6 @@ pub fn plugin_registrar(reg: &mut Registry) {
                                            attrs::INLINE_ALWAYS,
                                            collapsible_if::COLLAPSIBLE_IF,
                                            unicode::ZERO_WIDTH_SPACE,
+                                           strings::STRING_ADD_ASSIGN,
                                            ]);
 }

src/strings.rs

@@ -0,0 +1,55 @@
+//! This LintPass catches both string addition and string addition + assignment
+//! 
+//! Note that since we have two lints where one subsumes the other, we try to
+//! disable the subsumed lint unless it has a higher level
+
+use rustc::lint::*;
+use rustc::middle::ty::TypeVariants::TyStruct;
+use syntax::ast::*;
+use syntax::codemap::{Span, Spanned};
+use eq_op::is_exp_equal;
+use misc::walk_ty;
+use types::match_ty_unwrap;
+use utils::{match_def_path, span_lint};
+
+declare_lint! {
+    pub STRING_ADD_ASSIGN,
+    Warn,
+    "Warn on `x = 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_ASSIGN)
+    }
+    
+    fn check_expr(&mut self, cx: &Context, e: &Expr) {
+        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. \
+                    Consider using `String::push_str(..) instead.")
+            }
+        }
+    }
+}
+
+fn is_string(cx: &Context, e: &Expr) -> bool {
+    if let TyStruct(def_id, _) = walk_ty(cx.tcx.expr_ty(e)).sty {
+        match_def_path(cx, def_id, &["std", "string", "String"])
+    } else { false }
+}
+
+fn is_add(src: &Expr, target: &Expr) -> bool {
+    match &src.node {
+        &ExprBinary(Spanned{ node: BiAdd, .. }, ref left, _) =>
+            is_exp_equal(target, left),
+        &ExprBlock(ref block) => block.stmts.is_empty() && 
+            block.expr.as_ref().map_or(false, |expr| is_add(&*expr, target)),
+        &ExprParen(ref expr) => is_add(&*expr, target),
+        _ => false
+    }
+}

src/utils.rs

@@ -41,7 +41,7 @@ pub fn match_def_path(cx: &Context, def_id: DefId, path: &[&str]) -> bool {
 /// `match_path(path, &["std", "rt", "begin_unwind"])`
 pub fn match_path(path: &Path, segments: &[&str]) -> bool {
 	path.segments.iter().rev().zip(segments.iter().rev()).all(
-		|(a,b)| a.identifier.name == b)
+		|(a,b)| &a.identifier.name.as_str() == b)
 }
 
 /// convert a span to a code snippet if available, otherwise use default, e.g.

tests/compile-fail/strings.rs

@@ -0,0 +1,12 @@
+#![feature(plugin)]
+#![plugin(clippy)]
+
+#![deny(string_add_assign)]
+
+fn main() {
+	let x = "".to_owned();
+	
+	for i in (1..3) {
+		x = x + "."; //~ERROR
+	}
+}