diff --git a/CHANGELOG.md b/CHANGELOG.md index 56160aa587a3..5bcdf5192dc6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -850,6 +850,7 @@ All notable changes to this project will be documented in this file. [`unused_label`]: https://rust-lang-nursery.github.io/rust-clippy/master/index.html#unused_label [`use_debug`]: https://rust-lang-nursery.github.io/rust-clippy/master/index.html#use_debug [`use_self`]: https://rust-lang-nursery.github.io/rust-clippy/master/index.html#use_self +[`use_shared_from_slice`]: https://rust-lang-nursery.github.io/rust-clippy/master/index.html#use_shared_from_slice [`used_underscore_binding`]: https://rust-lang-nursery.github.io/rust-clippy/master/index.html#used_underscore_binding [`useless_asref`]: https://rust-lang-nursery.github.io/rust-clippy/master/index.html#useless_asref [`useless_attribute`]: https://rust-lang-nursery.github.io/rust-clippy/master/index.html#useless_attribute diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 64dd7196128e..93b808d427f2 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -202,6 +202,7 @@ pub mod unused_io_amount; pub mod unused_label; pub mod unwrap; pub mod use_self; +pub mod use_shared_from_slice; pub mod vec; pub mod write; pub mod zero_div_zero; @@ -420,6 +421,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()); reg.register_late_lint_pass(box neg_cmp_op_on_partial_ord::NoNegCompOpForPartialOrd); reg.register_late_lint_pass(box unwrap::Pass); @@ -925,6 +927,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry) { needless_borrow::NEEDLESS_BORROW, ranges::RANGE_PLUS_ONE, unwrap::UNNECESSARY_UNWRAP, + use_shared_from_slice::USE_SHARED_FROM_SLICE, ]); } diff --git a/clippy_lints/src/types.rs b/clippy_lints/src/types.rs index a71d47e40854..b9e5a3b82113 100644 --- 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 new file mode 100644 index 000000000000..b41eb3723431 --- /dev/null +++ b/clippy_lints/src/use_shared_from_slice.rs @@ -0,0 +1,135 @@ +use rustc::hir; +use rustc::hir::{Expr, Local}; +use rustc::lint::{LateContext, LateLintPass, LintArray, LintPass}; +use rustc::ty; +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` or `Rc>`. +/// +/// **Why is this bad?** +/// Using `Rc` or `Rc<[T]>` is more efficient and easy to construct with +/// `into()`. +/// +/// **Known problems:** +/// None. +/// +/// **Example:** +/// +/// ```rust +/// use std::rc::Rc; +/// +/// // Bad +/// let bad_ref: Rc> = Rc::new(vec!(1, 2, 3)); +/// +/// // Good +/// let good_ref: Rc<[usize]> = vec!(1, 2, 3).into(); +/// ``` +declare_clippy_lint! { + pub USE_SHARED_FROM_SLICE, + nursery, + "constructing reference-counted type from `Vec` or `String`" +} + +#[derive(Copy, Clone)] +pub struct Pass; + +impl LintPass for Pass { + fn get_lints(&self) -> LintArray { + lint_array!(USE_SHARED_FROM_SLICE) + } +} + +/// 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>`. + 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); + + 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 = 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) { + if let hir::TyPath(ref qpath) = ty.node { + 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>`, 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 { + let msg = "use slice instead of `String` in reference-counted type"; + let help = "use"; + let sugg = format!("{}", constructor); + span_lint_and_sugg(cx, USE_SHARED_FROM_SLICE, ty.span, msg, help, sugg); + } + } + } +} + +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 index 6c3f0c25a3ec..bd4cf1c70a33 100644 --- a/clippy_lints/src/utils/mod.rs +++ b/clippy_lints/src/utils/mod.rs @@ -150,6 +150,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` +/// 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 new file mode 100644 index 000000000000..917b88c61115 --- /dev/null +++ b/tests/ui/use_shared_from_slice.rs @@ -0,0 +1,35 @@ +use std::rc::Rc; +use std::rc; +use std::sync::Arc; + +#[warn(clippy, use_shared_from_slice)] +#[allow(unused_variables)] +fn main() { + // Test constructing `Rc` directly from `vec!` macro. + let bad_rc_vec_0: Rc> = Rc::new(vec!(1, 2, 3)); + let bad_rc_vec_00: rc::Rc> = rc::Rc::new(vec!(1, 2, 3)); + // Test constructing `Rc` from `Vec` variable. + let example_vec: Vec = vec!(4, 5, 6); + let bad_rc_vec_1: Rc> = Rc::new(example_vec); + // Test constructing `Rc` with a `String`. + let bad_rc_string_0: Rc = 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 = Rc::new(example_string); + + // Test constructing `Arc` from `vec!` macro. + let bad_arc_vec_0: Arc> = Arc::new(vec!(1, 2, 3)); + // Test constructing `Arc` from `Vec` variable. + let example_vec: Vec = vec!(4, 5, 6); + let bad_arc_vec_1: Arc> = Arc::new(example_vec); + // Test constructing `Arc` with a `String`. + let bad_arc_string_0: Arc = 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 = 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 = 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 new file mode 100644 index 000000000000..4cf89eafff07 --- /dev/null +++ b/tests/ui/use_shared_from_slice.stderr @@ -0,0 +1,112 @@ +error: use slice instead of `Vec` in reference-counted type + --> $DIR/use_shared_from_slice.rs:9:23 + | +9 | let bad_rc_vec_0: Rc> = Rc::new(vec!(1, 2, 3)); + | ^^^^^^^^^^^^^^ help: use: `Rc<[usize]>` + | + = note: `-D use-shared-from-slice` implied by `-D warnings` + +error: avoid constructing reference-counted type from Vec; convert from slice instead + --> $DIR/use_shared_from_slice.rs:9:40 + | +9 | let bad_rc_vec_0: Rc> = Rc::new(vec!(1, 2, 3)); + | ^^^^^^^^^^^^^^^^^^^^^^ help: use: `vec!(1, 2, 3).into()` + +error: use slice instead of `Vec` in reference-counted type + --> $DIR/use_shared_from_slice.rs:10:24 + | +10 | let bad_rc_vec_00: rc::Rc> = rc::Rc::new(vec!(1, 2, 3)); + | ^^^^^^^^^^^^^^^^^^ help: use: `Rc<[usize]>` + +error: avoid constructing reference-counted type from Vec; convert from slice instead + --> $DIR/use_shared_from_slice.rs:10:45 + | +10 | let bad_rc_vec_00: rc::Rc> = rc::Rc::new(vec!(1, 2, 3)); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use: `vec!(1, 2, 3).into()` + +error: use slice instead of `Vec` in reference-counted type + --> $DIR/use_shared_from_slice.rs:13:23 + | +13 | let bad_rc_vec_1: Rc> = Rc::new(example_vec); + | ^^^^^^^^^^^^^^ help: use: `Rc<[usize]>` + +error: avoid constructing reference-counted type from Vec; convert from slice instead + --> $DIR/use_shared_from_slice.rs:13:40 + | +13 | let bad_rc_vec_1: Rc> = Rc::new(example_vec); + | ^^^^^^^^^^^^^^^^^^^^ help: use: `example_vec.into()` + +error: use slice instead of `String` in reference-counted type + --> $DIR/use_shared_from_slice.rs:15:26 + | +15 | let bad_rc_string_0: Rc = Rc::new("test".to_string()); + | ^^^^^^^^^^ help: use: `Rc` + +error: avoid constructing reference-counted type from String; convert from slice instead + --> $DIR/use_shared_from_slice.rs:15:39 + | +15 | let bad_rc_string_0: Rc = Rc::new("test".to_string()); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use: `"test".to_string().as_str().into()` + +error: use slice instead of `String` in reference-counted type + --> $DIR/use_shared_from_slice.rs:18:26 + | +18 | let bad_rc_string_1: Rc = Rc::new(example_string); + | ^^^^^^^^^^ help: use: `Rc` + +error: avoid constructing reference-counted type from String; convert from slice instead + --> $DIR/use_shared_from_slice.rs:18:39 + | +18 | let bad_rc_string_1: Rc = Rc::new(example_string); + | ^^^^^^^^^^^^^^^^^^^^^^^ help: use: `example_string.as_str().into()` + +error: use slice instead of `Vec` in reference-counted type + --> $DIR/use_shared_from_slice.rs:21:24 + | +21 | let bad_arc_vec_0: Arc> = Arc::new(vec!(1, 2, 3)); + | ^^^^^^^^^^^^^^^ help: use: `Arc<[usize]>` + +error: avoid constructing reference-counted type from Vec; convert from slice instead + --> $DIR/use_shared_from_slice.rs:21:42 + | +21 | let bad_arc_vec_0: Arc> = Arc::new(vec!(1, 2, 3)); + | ^^^^^^^^^^^^^^^^^^^^^^^ help: use: `vec!(1, 2, 3).into()` + +error: use slice instead of `Vec` in reference-counted type + --> $DIR/use_shared_from_slice.rs:24:24 + | +24 | let bad_arc_vec_1: Arc> = Arc::new(example_vec); + | ^^^^^^^^^^^^^^^ help: use: `Arc<[usize]>` + +error: avoid constructing reference-counted type from Vec; convert from slice instead + --> $DIR/use_shared_from_slice.rs:24:42 + | +24 | let bad_arc_vec_1: Arc> = Arc::new(example_vec); + | ^^^^^^^^^^^^^^^^^^^^^ help: use: `example_vec.into()` + +error: use slice instead of `String` in reference-counted type + --> $DIR/use_shared_from_slice.rs:26:27 + | +26 | let bad_arc_string_0: Arc = Arc::new("test".to_string()); + | ^^^^^^^^^^^ help: use: `Arc` + +error: avoid constructing reference-counted type from String; convert from slice instead + --> $DIR/use_shared_from_slice.rs:26:41 + | +26 | let bad_arc_string_0: Arc = Arc::new("test".to_string()); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use: `"test".to_string().as_str().into()` + +error: use slice instead of `String` in reference-counted type + --> $DIR/use_shared_from_slice.rs:29:27 + | +29 | let bad_arc_string_0: Arc = Arc::new(example_string); + | ^^^^^^^^^^^ help: use: `Arc` + +error: avoid constructing reference-counted type from String; convert from slice instead + --> $DIR/use_shared_from_slice.rs:29:41 + | +29 | let bad_arc_string_0: Arc = Arc::new(example_string); + | ^^^^^^^^^^^^^^^^^^^^^^^^ help: use: `example_string.as_str().into()` + +error: aborting due to 18 previous errors +