From ea665c38f1ad049935a775d19082adedede9e00e Mon Sep 17 00:00:00 2001 From: mcarton Date: Thu, 14 Jul 2016 19:31:17 +0200 Subject: [PATCH] Fix FP with `USELESS_VEC` and non-copy types --- clippy_lints/src/methods.rs | 15 ++---- clippy_lints/src/utils/mod.rs | 7 ++- clippy_lints/src/vec.rs | 87 ++++++++++++++++++++--------------- tests/compile-fail/vec.rs | 9 +++- 4 files changed, 68 insertions(+), 50 deletions(-) diff --git a/clippy_lints/src/methods.rs b/clippy_lints/src/methods.rs index f70ec4eac9f..755c325fcbc 100644 --- a/clippy_lints/src/methods.rs +++ b/clippy_lints/src/methods.rs @@ -2,7 +2,7 @@ use rustc::hir; use rustc::lint::*; use rustc::middle::const_val::ConstVal; use rustc::middle::const_qualif::ConstQualif; -use rustc::ty::subst::{Subst, TypeSpace}; +use rustc::ty::subst::TypeSpace; use rustc::ty; use rustc_const_eval::EvalHint::ExprTypeChecked; use rustc_const_eval::eval_const_expr_partial; @@ -10,9 +10,9 @@ use std::borrow::Cow; use std::fmt; use syntax::codemap::Span; use syntax::ptr::P; -use utils::{get_trait_def_id, implements_trait, in_external_macro, in_macro, match_path, match_trait_method, - match_type, method_chain_args, return_ty, same_tys, snippet, span_lint, - span_lint_and_then, span_note_and_lint, walk_ptrs_ty, walk_ptrs_ty_depth}; +use utils::{get_trait_def_id, implements_trait, in_external_macro, in_macro, is_copy, match_path, + match_trait_method, match_type, method_chain_args, return_ty, same_tys, snippet, + span_lint, span_lint_and_then, span_note_and_lint, walk_ptrs_ty, walk_ptrs_ty_depth}; use utils::MethodArgs; use utils::paths; use utils::sugg; @@ -471,7 +471,7 @@ impl LateLintPass for Pass { // check conventions w.r.t. conversion method names and predicates let ty = cx.tcx.lookup_item_type(cx.tcx.map.local_def_id(item.id)).ty; - let is_copy = is_copy(cx, ty, item); + let is_copy = is_copy(cx, ty, item.id); for &(ref conv, self_kinds) in &CONVENTIONS { if_let_chain! {[ conv.check(&name.as_str()), @@ -1163,8 +1163,3 @@ fn is_bool(ty: &hir::Ty) -> bool { false } } - -fn is_copy<'a, 'ctx>(cx: &LateContext<'a, 'ctx>, ty: ty::Ty<'ctx>, item: &hir::Item) -> bool { - let env = ty::ParameterEnvironment::for_item(cx.tcx, item.id); - !ty.subst(cx.tcx, env.free_substs).moves_by_default(cx.tcx.global_tcx(), &env, item.span) -} diff --git a/clippy_lints/src/utils/mod.rs b/clippy_lints/src/utils/mod.rs index 189801c8c0b..bcc4be745c0 100644 --- a/clippy_lints/src/utils/mod.rs +++ b/clippy_lints/src/utils/mod.rs @@ -15,7 +15,7 @@ use std::env; use std::mem; use std::str::FromStr; use syntax::ast::{self, LitKind}; -use syntax::codemap::{ExpnFormat, ExpnInfo, MultiSpan, Span}; +use syntax::codemap::{ExpnFormat, ExpnInfo, MultiSpan, Span, DUMMY_SP}; use syntax::errors::DiagnosticBuilder; use syntax::ptr::P; @@ -723,3 +723,8 @@ pub fn type_is_unsafe_function(ty: ty::Ty) -> bool { _ => false, } } + +pub fn is_copy<'a, 'ctx>(cx: &LateContext<'a, 'ctx>, ty: ty::Ty<'ctx>, env: NodeId) -> bool { + let env = ty::ParameterEnvironment::for_item(cx.tcx, env); + !ty.subst(cx.tcx, env.free_substs).moves_by_default(cx.tcx.global_tcx(), &env, DUMMY_SP) +} diff --git a/clippy_lints/src/vec.rs b/clippy_lints/src/vec.rs index 114817989d8..cc9d5a5f224 100644 --- a/clippy_lints/src/vec.rs +++ b/clippy_lints/src/vec.rs @@ -1,10 +1,10 @@ -use rustc::lint::*; -use rustc::ty::TypeVariants; use rustc::hir::*; +use rustc::lint::*; +use rustc::ty; use rustc_const_eval::EvalHint::ExprTypeChecked; use rustc_const_eval::eval_const_expr_partial; use syntax::codemap::Span; -use utils::{higher, snippet, span_lint_and_then}; +use utils::{higher, is_copy, snippet, span_lint_and_then}; /// **What it does:** This lint warns about using `&vec![..]` when using `&[..]` would be possible. /// @@ -35,50 +35,61 @@ impl LateLintPass for Pass { fn check_expr(&mut self, cx: &LateContext, expr: &Expr) { // search for `&vec![_]` expressions where the adjusted type is `&[_]` if_let_chain!{[ - let TypeVariants::TyRef(_, ref ty) = cx.tcx.expr_ty_adjusted(expr).sty, - let TypeVariants::TySlice(..) = ty.ty.sty, + let ty::TypeVariants::TyRef(_, ref ty) = cx.tcx.expr_ty_adjusted(expr).sty, + let ty::TypeVariants::TySlice(..) = ty.ty.sty, let ExprAddrOf(_, ref addressee) = expr.node, + let Some(vec_args) = higher::vec_macro(cx, addressee), ], { - check_vec_macro(cx, addressee, expr.span); + check_vec_macro(cx, &vec_args, expr.span); }} // search for `for _ in vec![…]` - if let Some((_, arg, _)) = higher::for_loop(expr) { + if_let_chain!{[ + let Some((_, arg, _)) = higher::for_loop(expr), + let Some(vec_args) = higher::vec_macro(cx, arg), + is_copy(cx, vec_type(cx.tcx.expr_ty_adjusted(arg)), cx.tcx.map.get_parent(expr.id)), + ], { // report the error around the `vec!` not inside `:` let span = cx.sess().codemap().source_callsite(arg.span); - check_vec_macro(cx, arg, span); + check_vec_macro(cx, &vec_args, span); + }} + } +} + +fn check_vec_macro(cx: &LateContext, vec_args: &higher::VecArgs, span: Span) { + let snippet = match *vec_args { + higher::VecArgs::Repeat(elem, len) => { + if eval_const_expr_partial(cx.tcx, len, ExprTypeChecked, None).is_ok() { + format!("&[{}; {}]", snippet(cx, elem.span, "elem"), snippet(cx, len.span, "len")).into() + } else { + return; + } } - } + higher::VecArgs::Vec(args) => { + if let Some(last) = args.iter().last() { + let span = Span { + lo: args[0].span.lo, + hi: last.span.hi, + expn_id: args[0].span.expn_id, + }; + + format!("&[{}]", snippet(cx, span, "..")).into() + } else { + "&[]".into() + } + } + }; + + span_lint_and_then(cx, USELESS_VEC, span, "useless use of `vec!`", |db| { + db.span_suggestion(span, "you can use a slice directly", snippet); + }); } -fn check_vec_macro(cx: &LateContext, vec: &Expr, span: Span) { - if let Some(vec_args) = higher::vec_macro(cx, vec) { - let snippet = match vec_args { - higher::VecArgs::Repeat(elem, len) => { - if eval_const_expr_partial(cx.tcx, len, ExprTypeChecked, None).is_ok() { - format!("&[{}; {}]", snippet(cx, elem.span, "elem"), snippet(cx, len.span, "len")).into() - } else { - return; - } - } - higher::VecArgs::Vec(args) => { - if let Some(last) = args.iter().last() { - let span = Span { - lo: args[0].span.lo, - hi: last.span.hi, - expn_id: args[0].span.expn_id, - }; - - format!("&[{}]", snippet(cx, span, "..")).into() - } else { - "&[]".into() - } - } - }; - - span_lint_and_then(cx, USELESS_VEC, span, "useless use of `vec!`", |db| { - db.span_suggestion(span, "you can use a slice directly", snippet); - }); +/// Return the item type of the vector (ie. the `T` in `Vec`). +fn vec_type(ty: ty::Ty) -> ty::Ty { + if let ty::TyStruct(_, substs) = ty.sty { + substs.types.get(ty::subst::ParamSpace::TypeSpace, 0) + } else { + panic!("The type of `vec!` is a not a struct?"); } } - diff --git a/tests/compile-fail/vec.rs b/tests/compile-fail/vec.rs index 92c99c20e36..7a790e62116 100644 --- a/tests/compile-fail/vec.rs +++ b/tests/compile-fail/vec.rs @@ -3,6 +3,9 @@ #![deny(useless_vec)] +#[derive(Debug)] +struct NonCopy; + fn on_slice(_: &[u8]) {} #[allow(ptr_arg)] fn on_vec(_: &Vec) {} @@ -62,6 +65,10 @@ fn main() { //~^ ERROR useless use of `vec!` //~| HELP you can use //~| SUGGESTION for a in &[1, 2, 3] { - println!("{}", a); + println!("{:?}", a); + } + + for a in vec![NonCopy, NonCopy] { + println!("{:?}", a); } }