Fix check_expr and add check_local

rudyardrichter · rudyardrichter · commit ff3228c8aaa8 · 2018-06-10T15:11:13.000-05:00
diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs
@@ -419,6 +419,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry) {
     reg.register_early_lint_pass(box multiple_crate_versions::Pass);
     reg.register_late_lint_pass(box map_unit_fn::Pass);
     reg.register_late_lint_pass(box infallible_destructuring_match::Pass);
+    reg.register_late_lint_pass(box use_shared_from_slice::Pass);
     reg.register_late_lint_pass(box inherent_impl::Pass::default());
 
 
diff --git a/clippy_lints/src/types.rs b/clippy_lints/src/types.rs
@@ -12,8 +12,8 @@ use std::borrow::Cow;
 use syntax::ast::{FloatTy, IntTy, UintTy};
 use syntax::codemap::Span;
 use syntax::errors::DiagnosticBuilder;
-use crate::utils::{comparisons, differing_macro_contexts, higher, in_constant, in_external_macro, in_macro, last_path_segment, match_def_path, match_path,
-            match_type, multispan_sugg, opt_def_id, same_tys, snippet, snippet_opt, span_help_and_lint, span_lint,
+use crate::utils::{comparisons, differing_macro_contexts, higher, in_constant, in_external_macro, in_macro, match_def_path, match_path,
+            match_type, multispan_sugg, opt_def_id, same_tys, snippet, snippet_opt, span_help_and_lint, span_lint, match_type_parameter,
             span_lint_and_sugg, span_lint_and_then, clip, unsext, sext, int_bits};
 use crate::utils::paths;
 use crate::consts::{constant, Constant};
@@ -176,23 +176,6 @@ fn check_fn_decl(cx: &LateContext, decl: &FnDecl) {
     }
 }
 
-/// Check if `qpath` has last segment with type parameter matching `path`
-fn match_type_parameter(cx: &LateContext, qpath: &QPath, path: &[&str]) -> bool {
-    let last = last_path_segment(qpath);
-    if_chain! {
-        if let Some(ref params) = last.parameters;
-        if !params.parenthesized;
-        if let Some(ty) = params.types.get(0);
-        if let TyPath(ref qpath) = ty.node;
-        if let Some(did) = opt_def_id(cx.tables.qpath_def(qpath, cx.tcx.hir.node_to_hir_id(ty.id)));
-        if match_def_path(cx.tcx, did, path);
-        then {
-            return true;
-        }
-    }
-    false
-}
-
 /// Recursively check for `TypePass` lints in the given type. Stop at the first
 /// lint found.
 ///
diff --git a/clippy_lints/src/use_shared_from_slice.rs b/clippy_lints/src/use_shared_from_slice.rs
@@ -1,14 +1,15 @@
-use rustc::hir::Expr;
-use rustc::lint::{LateContext, LateLintPass, LintArray, LintContext, LintPass};
+use rustc::hir;
+use rustc::hir::{Expr, Local};
+use rustc::lint::{LateContext, LateLintPass, LintArray, LintPass};
 use rustc::ty;
-use utils::{match_type, span_lint_and_sugg, walk_ptrs_ty};
-use utils::paths;
+use crate::utils::{match_qpath, match_type, match_type_parameter, snippet, span_lint_and_sugg, walk_ptrs_ty, get_type_parameter};
+use crate::utils::paths;
 
 /// **What it does:**
 /// Checks for usage of `Rc<String>` or `Rc<Vec<T>>`.
 ///
 /// **Why is this bad?**
-/// Using a `Rc<str>` or `Rc<[T]>` is more efficient and easy to construct with
+/// Using `Rc<str>` or `Rc<[T]>` is more efficient and easy to construct with
 /// `into()`.
 ///
 /// **Known problems:**
@@ -23,15 +24,15 @@ use utils::paths;
 /// let bad_ref: Rc<Vec<usize>> = Rc::new(vec!(1, 2, 3));
 ///
 /// // Good
-/// let good_ref: Rc<[usize]> = Rc::new(vec!(1, 2, 3).into());
+/// let good_ref: Rc<[usize]> = vec!(1, 2, 3).into();
 /// ```
 declare_clippy_lint! {
     pub USE_SHARED_FROM_SLICE,
     nursery,
-    "use `into()` to construct `Rc` from slice"
+    "constructing reference-counted type from `Vec` or `String`"
 }
 
-#[derive(Copy, Clone, Debug)]
+#[derive(Copy, Clone)]
 pub struct Pass;
 
 impl LintPass for Pass {
@@ -40,31 +41,99 @@ impl LintPass for Pass {
     }
 }
 
-impl <'a, 'tcx> LateLintPass<'a, 'tcx> for Pass {
-    fn check_expr(&mut self, cx: &LateContext<'a, 'tcx>, expr: &'tcx Expr) {
-        let expr_ty = walk_ptrs_ty(cx.tables.expr_ty(expr));
+/// If the given `expr` is constructing an `Rc` or `Arc` containing a `Vec` or
+/// `String`, output a suggestion to fix accordingly.
+fn check_rc_construction<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, expr: &'tcx Expr) {
+    let expr_ty = walk_ptrs_ty(cx.tables.expr_ty(expr));
+
+    // Check for expressions with the type `Rc<Vec<T>>`.
+    if_chain! {
+        // Check if this expression is constructing an `Rc` or `Arc`.
+        let is_rc = match_type(cx, expr_ty, &paths::RC);
+        let is_arc = match_type(cx, expr_ty, &paths::ARC);
+        if is_rc || is_arc;
+
+        // Check if the `Rc` or `Arc` is constructed with `Vec` or `String`.
+        if let ty::TyAdt(_, subst) = expr_ty.sty;
+        let arg_type = subst.type_at(0);
+        let arg_is_vec = match_type(cx, arg_type, &paths::VEC);
+        let arg_is_string = match_type(cx, arg_type, &paths::STRING);
+        if arg_is_vec || arg_is_string;
+
+        // Get the argument, to use for writing out the lint message.
+        if let hir::ExprCall(_, ref args) = expr.node;
+        if let Some(arg) = args.get(0);
 
-        // Check for expressions with the type `Rc<Vec<T>>`.
-        if_chain! {
-            if let ty::TyAdt(_, subst) = expr_ty.sty;
-            if match_type(cx, expr_ty, &paths::RC);
-            if match_type(cx, subst.type_at(1), &paths::VEC);
-            then {
-                cx.sess().note_without_error(&format!("{:?}", subst));
-                span_lint_and_sugg(
-                    cx,
-                    USE_SHARED_FROM_SLICE,
-                    expr.span,
-                    "constructing reference-counted type from vec",
-                    "consider using `into()`",
-                    format!("{}", "TODO"),
-                );
+        then {
+            if arg_is_vec {
+                let msg = "avoid constructing reference-counted type from Vec; convert from slice instead";
+                let help = "use";
+                let argument = snippet(cx, arg.span.source_callsite(), "..");
+                let sugg = format!("{}.into()", argument);
+                span_lint_and_sugg(cx, USE_SHARED_FROM_SLICE, expr.span, &msg, help, sugg);
+            } else if arg_is_string {
+                let msg = "avoid constructing reference-counted type from String; convert from slice instead";
+                let help = "use";
+                let argument = snippet(cx, arg.span.source_callsite(), "..");
+                let sugg = format!("{}.as_str().into()", argument);
+                span_lint_and_sugg(cx, USE_SHARED_FROM_SLICE, expr.span, &msg, help, sugg);
             }
         }
+    }
+}
+
+/// Check a type declaration to lint, such as in
+///
+///     let x: Rc<String> = Rc::new(some_string)
+///
+/// If `ty`, the declared type, is an `Rc` or `Arc` containing a `Vec` or
+/// `String` then output a suggestion to change it.
+fn check_rc_type<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, ty: &hir::Ty) {
+    match ty.node {
+        hir::TyPath(ref qpath) => {
+            let matches_rc = match_qpath(qpath, &paths::RC);
+            let matches_arc = match_qpath(qpath, &paths::ARC);
+            if matches_rc || matches_arc {
+                let has_vec = match_type_parameter(cx, qpath, &paths::VEC);
+                let has_string = match_type_parameter(cx, qpath, &paths::STRING);
+                // Keep the type for making suggestions later.
+                let constructor = if matches_arc { "Arc" } else { "Rc" };
+                if_chain! {
+                    if has_vec;
+                    // In the case we have something like `Rc<Vec<usize>>`, get the inner parameter
+                    // type out from the parameter type of the `Rc`; so in this example, get the
+                    // type `usize`. Use this to suggest using the type `Rc<[usize]>` instead.
+                    let mut vec_ty = get_type_parameter(qpath).expect("");
+                    if let hir::TyPath(ref vec_qpath) = vec_ty.node;
+                    if let Some(param_ty) = get_type_parameter(&vec_qpath);
+                    then {
+                        let msg = "use slice instead of `Vec` in reference-counted type";
+                        let help = "use";
+                        let sugg = format!("{}<[{}]>", constructor, snippet(cx, param_ty.span.source_callsite(), ".."));
+                        span_lint_and_sugg(cx, USE_SHARED_FROM_SLICE, ty.span, msg, help, sugg);
+                    }
+                }
+                if has_string {
+                    //ty.node = TyPath(hir::Resolved(None, P()))
+                    let msg = "use slice instead of `String` in reference-counted type";
+                    let help = "use";
+                    let sugg = format!("{}<str>", constructor);
+                    span_lint_and_sugg(cx, USE_SHARED_FROM_SLICE, ty.span, msg, help, sugg);
+                }
+            }
+        },
+        _ => {},
+    }
+}
 
-        // TODO
-        // Check for expressions with the type `Rc<String>`.
-        // Check for expressions with the type `Arc<String>`.
-        // Check for expressions with the type `Arc<Vec<T>>`.
+impl <'a, 'tcx> LateLintPass<'a, 'tcx> for Pass {
+    fn check_expr(&mut self, cx: &LateContext<'a, 'tcx>, expr: &'tcx Expr) {
+        check_rc_construction(cx, expr);
+    }
+
+    fn check_local(&mut self, cx: &LateContext<'a, 'tcx>, local: &Local) {
+        if let Some(ref ty) = local.ty {
+            check_rc_type(cx, ty);
+        }
     }
 }
diff --git a/clippy_lints/src/utils/mod.rs b/clippy_lints/src/utils/mod.rs
@@ -149,6 +149,45 @@ pub fn match_type(cx: &LateContext, ty: Ty, path: &[&str]) -> bool {
     }
 }
 
+/// Return the type parameter from a path.
+///
+/// # Examples
+/// ```rust,ignore
+/// // let path be a QPath representing `Vec<usize>`
+/// get_type_parameter(path)
+/// // => type(usize)
+/// ```
+pub fn get_type_parameter<'a>(qpath: &'a QPath) -> Option<&'a hir::Ty> {
+    let last = last_path_segment(qpath);
+    if_chain! {
+        if let Some(ref params) = last.parameters;
+        if !params.parenthesized;
+        if let Some(ty) = params.types.get(0);
+        then {
+            Some(ty)
+        } else {
+            None
+        }
+    }
+}
+
+/// Check if `qpath` has last segment with type parameter matching `path`
+pub fn match_type_parameter(cx: &LateContext, qpath: &QPath, path: &[&str]) -> bool {
+    let last = last_path_segment(qpath);
+    if_chain! {
+        if let Some(ref params) = last.parameters;
+        if !params.parenthesized;
+        if let Some(ty) = params.types.get(0);
+        if let TyPath(ref qpath) = ty.node;
+        if let Some(did) = opt_def_id(cx.tables.qpath_def(qpath, cx.tcx.hir.node_to_hir_id(ty.id)));
+        if match_def_path(cx.tcx, did, path);
+        then {
+            return true;
+        }
+    }
+    false
+}
+
 /// Check if the method call given in `expr` belongs to given type.
 pub fn match_impl_method(cx: &LateContext, expr: &Expr, path: &[&str]) -> bool {
     let method_call = cx.tables.type_dependent_defs()[expr.hir_id];
diff --git a/tests/ui/use_shared_from_slice.rs b/tests/ui/use_shared_from_slice.rs
@@ -1,7 +1,35 @@
 use std::rc::Rc;
+use std::rc;
+use std::sync::Arc;
 
+#[warn(clippy, use_shared_from_slice)]
+#[allow(unused_variables)]
 fn main() {
-    let _bad_ref: Rc<Vec<usize>> = Rc::new(vec!(1, 2, 3));
+    // Test constructing `Rc` directly from `vec!` macro.
+    let bad_rc_vec_0: Rc<Vec<usize>> = Rc::new(vec!(1, 2, 3));
+    let bad_rc_vec_00: rc::Rc<Vec<usize>> = rc::Rc::new(vec!(1, 2, 3));
+    // Test constructing `Rc` from `Vec` variable.
+    let example_vec: Vec<usize> = vec!(4, 5, 6);
+    let bad_rc_vec_1: Rc<Vec<usize>> = Rc::new(example_vec);
+    // Test constructing `Rc` with a `String`.
+    let bad_rc_string_0: Rc<String> = Rc::new("test".to_string());
+    // Test constructing `Rc` with a `String` variable.
+    let example_string: String = "test".to_string();
+    let bad_rc_string_1: Rc<String> = Rc::new(example_string);
 
-    let _good_ref: Rc<[usize]> = vec!(1, 2, 3).into();
+    // Test constructing `Arc` from `vec!` macro.
+    let bad_arc_vec_0: Arc<Vec<usize>> = Arc::new(vec!(1, 2, 3));
+    // Test constructing `Arc` from `Vec` variable.
+    let example_vec: Vec<usize> = vec!(4, 5, 6);
+    let bad_arc_vec_1: Arc<Vec<usize>> = Arc::new(example_vec);
+    // Test constructing `Arc` with a `String`.
+    let bad_arc_string_0: Arc<String> = Arc::new("test".to_string());
+    // Test constructing `Arc` with a `String` variable.
+    let example_string: String = "test".to_string();
+    let bad_arc_string_0: Arc<String> = Arc::new(example_string);
+
+    // Test that using `.into()` doesn't cause suggestions.
+    let good_rc_0: Rc<[usize]> = vec!(1, 2, 3).into();
+    let example_vec: Vec<usize> = vec!(4, 5, 6);
+    let good_rc_1: Rc<[usize]> = example_vec.into();
 }
diff --git a/tests/ui/use_shared_from_slice.stderr b/tests/ui/use_shared_from_slice.stderr
