Merge pull request #215 from birkenfeld/type_improvements

Type improvements

src/methods.rs

@@ -2,7 +2,8 @@ use syntax::ast::*;
 use rustc::lint::*;
 use rustc::middle::ty;
 
-use utils::{span_lint, match_def_path, walk_ptrs_ty};
+use utils::{span_lint, match_type, walk_ptrs_ty};
+use utils::{OPTION_PATH, RESULT_PATH, STRING_PATH};
 
 #[derive(Copy,Clone)]
 pub struct MethodsPass;
@@ -16,45 +17,31 @@ declare_lint!(pub STR_TO_STRING, Warn,
 declare_lint!(pub STRING_TO_STRING, Warn,
               "calling `String.to_string()` which is a no-op");
 
-#[allow(unused_imports)]
 impl LintPass for MethodsPass {
     fn get_lints(&self) -> LintArray {
         lint_array!(OPTION_UNWRAP_USED, RESULT_UNWRAP_USED, STR_TO_STRING, STRING_TO_STRING)
     }
 
     fn check_expr(&mut self, cx: &Context, expr: &Expr) {
-        {
-            // In case stuff gets moved around
-            use core::option::Option;
-            use core::result::Result;
-            use collections::string::String;
-        }
         if let ExprMethodCall(ref ident, _, ref args) = expr.node {
-            let ref obj_ty = walk_ptrs_ty(cx.tcx.expr_ty(&*args[0])).sty;
+            let obj_ty = walk_ptrs_ty(cx.tcx.expr_ty(&*args[0]));
             if ident.node.name == "unwrap" {
-                if let ty::TyEnum(did, _) = *obj_ty {
-                    if match_def_path(cx, did.did, &["core", "option", "Option"]) {
-                        span_lint(cx, OPTION_UNWRAP_USED, expr.span,
-                                  "used unwrap() on an Option value. If you don't want \
-                                   to handle the None case gracefully, consider using
-                                   expect() to provide a better panic message");
-                    }
-                    else if match_def_path(cx, did.did, &["core", "result", "Result"]) {
-                        span_lint(cx, RESULT_UNWRAP_USED, expr.span,
-                                  "used unwrap() on a Result value. Graceful handling \
-                                   of Err values is preferred");
-                    }
+                if match_type(cx, obj_ty, &OPTION_PATH) {
+                    span_lint(cx, OPTION_UNWRAP_USED, expr.span,
+                              "used unwrap() on an Option value. If you don't want \
+                               to handle the None case gracefully, consider using \
+                               expect() to provide a better panic message");
+                } else if match_type(cx, obj_ty, &RESULT_PATH) {
+                    span_lint(cx, RESULT_UNWRAP_USED, expr.span,
+                              "used unwrap() on a Result value. Graceful handling \
+                               of Err values is preferred");
                 }
             }
             else if ident.node.name == "to_string" {
-                if let ty::TyStr = *obj_ty {
+                if obj_ty.sty == ty::TyStr {
                     span_lint(cx, STR_TO_STRING, expr.span, "`str.to_owned()` is faster");
-                }
-                else if let ty::TyStruct(did, _) = *obj_ty {
-                    if match_def_path(cx, did.did, &["collections", "string", "String"]) {
-                        span_lint(cx, STRING_TO_STRING, expr.span,
-                                  "`String.to_string()` is a no-op")
-                    }
+                } else if match_type(cx, obj_ty, &STRING_PATH) {
+                    span_lint(cx, STRING_TO_STRING, expr.span, "`String.to_string()` is a no-op");
                 }
             }
         }

src/ptr_arg.rs

@@ -4,10 +4,10 @@
 
 use rustc::lint::*;
 use syntax::ast::*;
-use syntax::codemap::Span;
+use rustc::middle::ty;
 
-use types::match_ty_unwrap;
-use utils::span_lint;
+use utils::{span_lint, match_type};
+use utils::{STRING_PATH, VEC_PATH};
 
 declare_lint! {
     pub PTR_ARG,
@@ -45,22 +45,21 @@ impl LintPass for PtrArg {
 
 fn check_fn(cx: &Context, decl: &FnDecl) {
     for arg in &decl.inputs {
-        match &arg.ty.node {
-            &TyPtr(ref p) | &TyRptr(_, ref p) =>
-                check_ptr_subtype(cx, arg.ty.span, &p.ty),
-            _ => ()
+        if arg.ty.node == TyInfer {  // "self" arguments
+            continue;
+        }
+        let ref sty = cx.tcx.pat_ty(&*arg.pat).sty;
+        if let &ty::TyRef(_, ty::TypeAndMut { ty, mutbl: MutImmutable }) = sty {
+            if match_type(cx, ty, &VEC_PATH) {
+                span_lint(cx, PTR_ARG, arg.ty.span,
+                          "writing `&Vec<_>` instead of `&[_]` involves one more reference \
+                           and cannot be used with non-Vec-based slices. Consider changing \
+                           the type to `&[...]`");
+            } else if match_type(cx, ty, &STRING_PATH) {
+                span_lint(cx, PTR_ARG, arg.ty.span,
+                          "writing `&String` instead of `&str` involves a new object \
+                           where a slice will do. Consider changing the type to `&str`");
+            }
         }
     }
 }
-
-fn check_ptr_subtype(cx: &Context, span: Span, ty: &Ty) {
-    match_ty_unwrap(ty, &["Vec"]).map_or_else(|| match_ty_unwrap(ty,
-        &["String"]).map_or((), |_| {
-            span_lint(cx, PTR_ARG, span,
-                      "writing `&String` instead of `&str` involves a new object \
-                       where a slice will do. Consider changing the type to `&str`")
-        }), |_| span_lint(cx, PTR_ARG, span,
-                          "writing `&Vec<_>` instead of \
-                           `&[_]` involves one more reference and cannot be used with \
-                           non-Vec-based slices. Consider changing the type to `&[...]`"))
-}

src/ranges.rs

@@ -1,8 +1,7 @@
 use rustc::lint::{Context, LintArray, LintPass};
-use rustc::middle::ty::TypeVariants::TyStruct;
 use syntax::ast::*;
 use syntax::codemap::Spanned;
-use utils::{match_def_path};
+use utils::match_type;
 
 declare_lint! {
     pub RANGE_STEP_BY_ZERO, Warn,
@@ -34,11 +33,9 @@ impl LintPass for StepByZero {
 fn is_range(cx: &Context, expr: &Expr) -> bool {
     // No need for walk_ptrs_ty here because step_by moves self, so it
     // can't be called on a borrowed range.
-    if let TyStruct(did, _) = cx.tcx.expr_ty(expr).sty {
-        // Note: RangeTo and RangeFull don't have step_by
-        match_def_path(cx, did.did, &["core", "ops", "Range"]) ||
-        match_def_path(cx, did.did, &["core", "ops", "RangeFrom"])
-    } else { false }
+    let ty = cx.tcx.expr_ty(expr);
+    // Note: RangeTo and RangeFull don't have step_by
+    match_type(cx, ty, &["core", "ops", "Range"]) || match_type(cx, ty, &["core", "ops", "RangeFrom"])
 }
 
 fn is_lit_zero(expr: &Expr) -> bool {

src/strings.rs

@@ -4,12 +4,12 @@
 //! 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::Spanned;
 
 use eq_op::is_exp_equal;
-use utils::{match_def_path, span_lint, walk_ptrs_ty, get_parent_expr};
+use utils::{match_type, span_lint, walk_ptrs_ty, get_parent_expr};
+use utils::STRING_PATH;
 
 declare_lint! {
     pub STRING_ADD_ASSIGN,
@@ -61,10 +61,7 @@ impl LintPass for StringAdd {
 }
 
 fn is_string(cx: &Context, e: &Expr) -> bool {
-    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 }
+    match_type(cx, walk_ptrs_ty(cx.tcx.expr_ty(e)), &STRING_PATH)
 }
 
 fn is_add(cx: &Context, src: &Expr, target: &Expr) -> bool {

src/types.rs

@@ -2,11 +2,11 @@ use rustc::lint::*;
 use syntax::ast;
 use syntax::ast::*;
 use syntax::ast_util::{is_comparison_binop, binop_to_string};
-use syntax::ptr::P;
 use rustc::middle::ty;
 use syntax::codemap::ExpnInfo;
 
-use utils::{in_macro, snippet, span_lint, span_help_and_lint, in_external_macro};
+use utils::{in_macro, match_type, snippet, span_lint, span_help_and_lint, in_external_macro};
+use utils::{LL_PATH, VEC_PATH};
 
 /// Handles all the linting of funky types
 #[allow(missing_copy_implementations)]
@@ -18,61 +18,26 @@ declare_lint!(pub LINKEDLIST, Warn,
               "usage of LinkedList, usually a vector is faster, or a more specialized data \
                structure like a RingBuf");
 
-/// Matches a type with a provided string, and returns its type parameters if successful
-pub fn match_ty_unwrap<'a>(ty: &'a Ty, segments: &[&str]) -> Option<&'a [P<Ty>]> {
-    match ty.node {
-        TyPath(_, Path {segments: ref seg, ..}) => {
-            // So ast::Path isn't the full path, just the tokens that were provided.
-            // I could muck around with the maps and find the full path
-            // however the more efficient way is to simply reverse the iterators and zip them
-            // which will compare them in reverse until one of them runs out of segments
-            if seg.iter().rev().zip(segments.iter().rev()).all(|(a,b)| a.identifier.name == b) {
-                match seg[..].last() {
-                    Some(&PathSegment {parameters: AngleBracketedParameters(ref a), ..}) => {
-                        Some(&a.types[..])
-                    }
-                    _ => None
-                }
-            } else {
-                None
-            }
-        },
-        _ => None
-    }
-}
-
-#[allow(unused_imports)]
 impl LintPass for TypePass {
     fn get_lints(&self) -> LintArray {
         lint_array!(BOX_VEC, LINKEDLIST)
     }
 
-    fn check_ty(&mut self, cx: &Context, ty: &ast::Ty) {
-        {
-            // In case stuff gets moved around
-            use std::boxed::Box;
-            use std::vec::Vec;
-        }
-        match_ty_unwrap(ty, &["std", "boxed", "Box"]).and_then(|t| t.first())
-          .and_then(|t| match_ty_unwrap(&**t, &["std", "vec", "Vec"]))
-          .map(|_| {
-            span_help_and_lint(cx, BOX_VEC, ty.span,
-                              "you seem to be trying to use `Box<Vec<T>>`. Did you mean to use `Vec<T>`?",
-                              "`Vec<T>` is already on the heap, `Box<Vec<T>>` makes an extra allocation");
-          });
-        {
-            // In case stuff gets moved around
-            use collections::linked_list::LinkedList as DL1;
-            use std::collections::linked_list::LinkedList as DL2;
-        }
-        let dlists = [vec!["std","collections","linked_list","LinkedList"],
-                      vec!["collections","linked_list","LinkedList"]];
-        for path in &dlists {
-            if match_ty_unwrap(ty, &path[..]).is_some() {
-                span_help_and_lint(cx, LINKEDLIST, ty.span,
-                                   "I see you're using a LinkedList! Perhaps you meant some other data structure?",
-                                   "a RingBuf might work");
-                return;
+    fn check_ty(&mut self, cx: &Context, ast_ty: &ast::Ty) {
+        if let Some(ty) = cx.tcx.ast_ty_to_ty_cache.borrow().get(&ast_ty.id) {
+            if let ty::TyBox(ref inner) = ty.sty {
+                if match_type(cx, inner, &VEC_PATH) {
+                    span_help_and_lint(
+                        cx, BOX_VEC, ast_ty.span,
+                        "you seem to be trying to use `Box<Vec<T>>`. Did you mean to use `Vec<T>`?",
+                        "`Vec<T>` is already on the heap, `Box<Vec<T>>` makes an extra allocation");
+                }
+            }
+            else if match_type(cx, ty, &LL_PATH) {
+                span_help_and_lint(
+                    cx, LINKEDLIST, ast_ty.span,
+                    "I see you're using a LinkedList! Perhaps you meant some other data structure?",
+                    "a RingBuf might work");
             }
         }
     }

src/utils.rs

@@ -6,6 +6,13 @@ use rustc::ast_map::Node::NodeExpr;
 use rustc::middle::ty;
 use std::borrow::Cow;
 
+// module DefPaths for certain structs/enums we check for
+pub const OPTION_PATH: [&'static str; 3] = ["core", "option", "Option"];
+pub const RESULT_PATH: [&'static str; 3] = ["core", "result", "Result"];
+pub const STRING_PATH: [&'static str; 3] = ["collections", "string", "String"];
+pub const VEC_PATH:    [&'static str; 3] = ["collections", "vec", "Vec"];
+pub const LL_PATH:     [&'static str; 3] = ["collections", "linked_list", "LinkedList"];
+
 /// returns true if the macro that expanded the crate was outside of
 /// the current crate or was a compiler plugin
 pub fn in_macro(cx: &Context, opt_info: Option<&ExpnInfo>) -> bool {
@@ -37,6 +44,18 @@ pub fn match_def_path(cx: &Context, def_id: DefId, path: &[&str]) -> bool {
         .zip(path.iter()).all(|(nm, p)| nm == p))
 }
 
+/// check if type is struct or enum type with given def path
+pub fn match_type(cx: &Context, ty: ty::Ty, path: &[&str]) -> bool {
+    match ty.sty {
+        ty::TyEnum(ref adt, _) | ty::TyStruct(ref adt, _) => {
+            match_def_path(cx, adt.did, path)
+        }
+        _ => {
+            false
+        }
+    }
+}
+
 /// match a Path against a slice of segment string literals, e.g.
 /// `match_path(path, &["std", "rt", "begin_unwind"])`
 pub fn match_path(path: &Path, segments: &[&str]) -> bool {

tests/compile-fail/dlist.rs

@@ -12,4 +12,4 @@ pub fn test(foo: LinkedList<u8>) {  //~ ERROR I see you're using a LinkedList!
 
 fn main(){
     test(LinkedList::new());
-}
+}

tests/compile-fail/ptr_arg.rs

@@ -1,20 +1,23 @@
 #![feature(plugin)]
 #![plugin(clippy)]
+#![allow(unused)]
+#![deny(ptr_arg)]
 
-#[deny(ptr_arg)]
-#[allow(unused)]
 fn do_vec(x: &Vec<i64>) { //~ERROR writing `&Vec<_>` instead of `&[_]`
     //Nothing here
 }
 
-#[deny(ptr_arg)]
-#[allow(unused)]
+fn do_vec_mut(x: &mut Vec<i64>) { // no error here
+    //Nothing here
+}
+
 fn do_str(x: &String) { //~ERROR writing `&String` instead of `&str`
     //Nothing here either
 }
 
-fn main() {
-    let x = vec![1i64, 2, 3];
-    do_vec(&x);
-    do_str(&"hello".to_owned());
+fn do_str_mut(x: &mut String) { // no error here
+    //Nothing here either
+}
+
+fn main() {
 }