diff --git a/CHANGELOG.md b/CHANGELOG.md index 1192195cd..0c829f268 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -383,6 +383,7 @@ All notable changes to this project will be documented in this file. [`needless_lifetimes`]: https://github.com/Manishearth/rust-clippy/wiki#needless_lifetimes [`needless_range_loop`]: https://github.com/Manishearth/rust-clippy/wiki#needless_range_loop [`needless_return`]: https://github.com/Manishearth/rust-clippy/wiki#needless_return +[`needless_take_by_value`]: https://github.com/Manishearth/rust-clippy/wiki#needless_take_by_value [`needless_update`]: https://github.com/Manishearth/rust-clippy/wiki#needless_update [`neg_multiply`]: https://github.com/Manishearth/rust-clippy/wiki#neg_multiply [`never_loop`]: https://github.com/Manishearth/rust-clippy/wiki#never_loop diff --git a/README.md b/README.md index 842e65467..a3f4dcf4a 100644 --- a/README.md +++ b/README.md @@ -180,7 +180,7 @@ transparently: ## Lints -There are 191 lints included in this crate: +There are 192 lints included in this crate: name | default | triggers on -----------------------------------------------------------------------------------------------------------------------|---------|---------------------------------------------------------------------------------------------------------------------------------- @@ -289,6 +289,7 @@ name [needless_lifetimes](https://github.com/Manishearth/rust-clippy/wiki#needless_lifetimes) | warn | using explicit lifetimes for references in function arguments when elision rules would allow omitting them [needless_range_loop](https://github.com/Manishearth/rust-clippy/wiki#needless_range_loop) | warn | for-looping over a range of indices where an iterator over items would do [needless_return](https://github.com/Manishearth/rust-clippy/wiki#needless_return) | warn | using a return statement like `return expr;` where an expression would suffice +[needless_take_by_value](https://github.com/Manishearth/rust-clippy/wiki#needless_take_by_value) | warn | taking arguments by value, but only using them by reference [needless_update](https://github.com/Manishearth/rust-clippy/wiki#needless_update) | warn | using `Foo { ..base }` when there are no missing fields [neg_multiply](https://github.com/Manishearth/rust-clippy/wiki#neg_multiply) | warn | multiplying integers with -1 [never_loop](https://github.com/Manishearth/rust-clippy/wiki#never_loop) | warn | any loop with an unconditional `break` statement diff --git a/clippy_lints/src/consts.rs b/clippy_lints/src/consts.rs index f6c9619dc..3dbd427d0 100644 --- a/clippy_lints/src/consts.rs +++ b/clippy_lints/src/consts.rs @@ -216,12 +216,12 @@ fn constant_negate(o: Constant) -> Option { use self::Constant::*; match o { Int(value) => (-value).ok().map(Int), - Float(is, ty) => Some(Float(neg_float_str(is), ty)), + Float(is, ty) => Some(Float(neg_float_str(&is), ty)), _ => None, } } -fn neg_float_str(s: String) -> String { +fn neg_float_str(s: &str) -> String { if s.starts_with('-') { s[1..].to_owned() } else { diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 814daf7d6..1d497b4ef 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -107,6 +107,7 @@ pub mod mut_reference; pub mod mutex_atomic; pub mod needless_bool; pub mod needless_borrow; +pub mod needless_take_by_value; pub mod needless_update; pub mod neg_multiply; pub mod new_without_default; @@ -299,6 +300,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry) { reg.register_late_lint_pass(box unused_io_amount::UnusedIoAmount); reg.register_late_lint_pass(box large_enum_variant::LargeEnumVariant::new(conf.enum_variant_size_threshold)); reg.register_late_lint_pass(box should_assert_eq::ShouldAssertEq); + reg.register_late_lint_pass(box needless_take_by_value::NeedlessTakeByValue); reg.register_lint_group("clippy_restrictions", vec![ arithmetic::FLOAT_ARITHMETIC, @@ -455,6 +457,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry) { needless_bool::BOOL_COMPARISON, needless_bool::NEEDLESS_BOOL, needless_borrow::NEEDLESS_BORROW, + needless_take_by_value::NEEDLESS_TAKE_BY_VALUE, needless_update::NEEDLESS_UPDATE, neg_multiply::NEG_MULTIPLY, new_without_default::NEW_WITHOUT_DEFAULT, diff --git a/clippy_lints/src/needless_take_by_value.rs b/clippy_lints/src/needless_take_by_value.rs new file mode 100644 index 000000000..8d2e2dfa0 --- /dev/null +++ b/clippy_lints/src/needless_take_by_value.rs @@ -0,0 +1,178 @@ +use rustc::hir::*; +use rustc::hir::intravisit::FnKind; +use rustc::hir::def_id::DefId; +use rustc::lint::*; +use rustc::ty; +use rustc::middle::expr_use_visitor as euv; +use rustc::middle::mem_categorization as mc; +use syntax::ast::NodeId; +use syntax_pos::Span; +use utils::{in_macro, is_self, is_copy, implements_trait, get_trait_def_id, snippet, span_lint_and_then, paths}; +use std::collections::HashSet; + +/// **What it does:** Checks for functions taking arguments by value, but only using them by +/// reference. +/// +/// **Why is this bad?** +/// +/// **Known problems:** Hopefully none. +/// +/// **Example:** +/// ```rust +/// fn foo(v: Vec) { +/// assert_eq!(v.len(), 42); +/// } +/// ``` +declare_lint! { + pub NEEDLESS_TAKE_BY_VALUE, + Warn, + "taking arguments by value, but only using them by reference" +} + +pub struct NeedlessTakeByValue; + +impl LintPass for NeedlessTakeByValue { + fn get_lints(&self) -> LintArray { + lint_array![NEEDLESS_TAKE_BY_VALUE] + } +} + +impl<'a, 'tcx> LateLintPass<'a, 'tcx> for NeedlessTakeByValue { + fn check_fn( + &mut self, + cx: &LateContext<'a, 'tcx>, + kind: FnKind<'tcx>, + decl: &'tcx FnDecl, + body: &'tcx Body, + span: Span, + node_id: NodeId + ) { + if in_macro(cx, span) { + return; + } + + if let FnKind::ItemFn(..) = kind { + } else { + return; + } + + // These are usually took by value and only used by reference + let fn_trait = cx.tcx.lang_items.fn_trait().expect("failed to find `Fn` trait"); + let asref_trait = get_trait_def_id(cx, &paths::ASREF_TRAIT).expect("failed to find `AsRef` trait"); + let borrow_trait = get_trait_def_id(cx, &paths::BORROW_TRAIT).expect("failed to find `Borrow` trait"); + + // Collect moved variables from the function body + let moved_vars = { + let mut ctx = MovedVariablesCtxt::new(cx); + let infcx = cx.tcx.borrowck_fake_infer_ctxt(body.id()); + { + let mut v = euv::ExprUseVisitor::new(&mut ctx, &infcx); + v.consume_body(body); + } + ctx.moved_vars + }; + + let fn_def_id = cx.tcx.hir.local_def_id(node_id); + let param_env = ty::ParameterEnvironment::for_item(cx.tcx, node_id); + let fn_sig = cx.tcx.item_type(fn_def_id).fn_sig(); + let fn_sig = cx.tcx.liberate_late_bound_regions(param_env.free_id_outlive, fn_sig); + + for ((input, ty), arg) in decl.inputs.iter().zip(fn_sig.inputs()).zip(&body.arguments) { + if_let_chain! {[ + !is_self(arg), + !ty.is_mutable_pointer(), + !is_copy(cx, ty, node_id), + !implements_trait(cx, ty, fn_trait, &[], Some(node_id)), + !implements_trait(cx, ty, asref_trait, &[], Some(node_id)), + !implements_trait(cx, ty, borrow_trait, &[], Some(node_id)), + + let PatKind::Binding(mode, defid, ..) = arg.pat.node, + !moved_vars.contains(&defid), + ], { + // Note: `toplevel_ref_arg` warns if `BindByRef` + let m = match mode { + BindingMode::BindByRef(m) | BindingMode::BindByValue(m) => m, + }; + if m == Mutability::MutMutable { + continue; + } + + span_lint_and_then(cx, + NEEDLESS_TAKE_BY_VALUE, + input.span, + "this function taking a value by value, but only using them by reference", + |db| { + db.span_suggestion(input.span, + "consider taking a reference instead", + format!("&{}", snippet(cx, input.span, "_"))); + }); + }} + } + } +} + +struct MovedVariablesCtxt<'a, 'tcx: 'a> { + cx: &'a LateContext<'a, 'tcx>, + moved_vars: HashSet, +} + +impl<'a, 'tcx: 'a> MovedVariablesCtxt<'a, 'tcx> { + fn new(cx: &'a LateContext<'a, 'tcx>) -> Self { + MovedVariablesCtxt { + cx: cx, + moved_vars: HashSet::new(), + } + } + + fn consume_common( + &mut self, + _consume_id: NodeId, + _consume_span: Span, + cmt: mc::cmt<'tcx>, + mode: euv::ConsumeMode + ) { + if_let_chain! {[ + let euv::ConsumeMode::Move(_) = mode, + let mc::Categorization::Local(vid) = cmt.cat, + ], { + if let Some(def_id) = self.cx.tcx.hir.opt_local_def_id(vid) { + self.moved_vars.insert(def_id); + } + }} + + } +} + +impl<'a, 'tcx: 'a> euv::Delegate<'tcx> for MovedVariablesCtxt<'a, 'tcx> { + fn consume(&mut self, consume_id: NodeId, consume_span: Span, cmt: mc::cmt<'tcx>, mode: euv::ConsumeMode) { + self.consume_common(consume_id, consume_span, cmt, mode); + } + + fn matched_pat(&mut self, _matched_pat: &Pat, _cmt: mc::cmt, _mode: euv::MatchMode) {} + + fn consume_pat(&mut self, consume_pat: &Pat, cmt: mc::cmt<'tcx>, mode: euv::ConsumeMode) { + self.consume_common(consume_pat.id, consume_pat.span, cmt, mode); + } + + fn borrow( + &mut self, + _borrow_id: NodeId, + _borrow_span: Span, + _cmt: mc::cmt<'tcx>, + _loan_region: &'tcx ty::Region, + _bk: ty::BorrowKind, + _loan_cause: euv::LoanCause + ) { + } + + fn mutate( + &mut self, + _assignment_id: NodeId, + _assignment_span: Span, + _assignee_cmt: mc::cmt<'tcx>, + _mode: euv::MutateMode + ) { + } + + fn decl_without_init(&mut self, _id: NodeId, _span: Span) {} +} diff --git a/clippy_lints/src/utils/paths.rs b/clippy_lints/src/utils/paths.rs index 5edff76d9..57d2e5a33 100644 --- a/clippy_lints/src/utils/paths.rs +++ b/clippy_lints/src/utils/paths.rs @@ -1,7 +1,9 @@ //! This module contains paths to types and functions Clippy needs to know about. +pub const ASREF_TRAIT: [&'static str; 3] = ["core", "convert", "AsRef"]; pub const BEGIN_PANIC: [&'static str; 3] = ["std", "panicking", "begin_panic"]; pub const BINARY_HEAP: [&'static str; 3] = ["collections", "binary_heap", "BinaryHeap"]; +pub const BORROW_TRAIT: [&'static str; 3] = ["core", "borrow", "Borrow"]; pub const BOX: [&'static str; 3] = ["std", "boxed", "Box"]; pub const BOX_NEW: [&'static str; 4] = ["std", "boxed", "Box", "new"]; pub const BTREEMAP: [&'static str; 4] = ["collections", "btree", "map", "BTreeMap"]; diff --git a/tests/issue-825.rs b/tests/issue-825.rs index 026bd36ee..2d6c8ea38 100644 --- a/tests/issue-825.rs +++ b/tests/issue-825.rs @@ -4,7 +4,7 @@ #![allow(warnings)] // this should compile in a reasonable amount of time -fn rust_type_id(name: String) { +fn rust_type_id(name: &str) { if "bool" == &name[..] || "uint" == &name[..] || "u8" == &name[..] || "u16" == &name[..] || "u32" == &name[..] || "f32" == &name[..] || "f64" == &name[..] || "i8" == &name[..] || "i16" == &name[..] || "i32" == &name[..] || diff --git a/tests/ui/absurd-extreme-comparisons.rs b/tests/ui/absurd-extreme-comparisons.rs index 495dd27bb..d4248175a 100644 --- a/tests/ui/absurd-extreme-comparisons.rs +++ b/tests/ui/absurd-extreme-comparisons.rs @@ -2,7 +2,7 @@ #![plugin(clippy)] #![deny(absurd_extreme_comparisons)] -#![allow(unused, eq_op, no_effect, unnecessary_operation)] +#![allow(unused, eq_op, no_effect, unnecessary_operation, needless_take_by_value)] fn main() { const Z: u32 = 0; diff --git a/tests/ui/box_vec.rs b/tests/ui/box_vec.rs index 01f1b1d09..92304753e 100644 --- a/tests/ui/box_vec.rs +++ b/tests/ui/box_vec.rs @@ -2,7 +2,7 @@ #![plugin(clippy)] #![deny(clippy)] -#![allow(boxed_local)] +#![allow(boxed_local, needless_take_by_value)] #![allow(blacklisted_name)] macro_rules! boxit { diff --git a/tests/ui/complex_types.rs b/tests/ui/complex_types.rs index ef5cd6897..64f1b4dcb 100644 --- a/tests/ui/complex_types.rs +++ b/tests/ui/complex_types.rs @@ -1,7 +1,7 @@ #![feature(plugin)] #![plugin(clippy)] #![deny(clippy)] -#![allow(unused)] +#![allow(unused, needless_take_by_value)] #![feature(associated_consts, associated_type_defaults)] type Alias = Vec>>; // no warning here diff --git a/tests/ui/dlist.rs b/tests/ui/dlist.rs index 10d0beab8..3661b63ef 100644 --- a/tests/ui/dlist.rs +++ b/tests/ui/dlist.rs @@ -4,7 +4,7 @@ #![plugin(clippy)] #![deny(clippy)] -#![allow(dead_code)] +#![allow(dead_code, needless_take_by_value)] extern crate collections; use collections::linked_list::LinkedList; diff --git a/tests/ui/drop_forget_ref.rs b/tests/ui/drop_forget_ref.rs index 911d3433e..44f6f54bc 100644 --- a/tests/ui/drop_forget_ref.rs +++ b/tests/ui/drop_forget_ref.rs @@ -2,7 +2,7 @@ #![plugin(clippy)] #![deny(drop_ref, forget_ref)] -#![allow(toplevel_ref_arg, similar_names)] +#![allow(toplevel_ref_arg, similar_names, needless_take_by_value)] use std::mem::{drop, forget}; diff --git a/tests/ui/entry.rs b/tests/ui/entry.rs index 816da06b9..495c024f1 100644 --- a/tests/ui/entry.rs +++ b/tests/ui/entry.rs @@ -1,6 +1,6 @@ #![feature(plugin)] #![plugin(clippy)] -#![allow(unused)] +#![allow(unused, needless_take_by_value)] #![deny(map_entry)] diff --git a/tests/ui/eta.rs b/tests/ui/eta.rs index 90ffa6121..51fde7d85 100644 --- a/tests/ui/eta.rs +++ b/tests/ui/eta.rs @@ -1,6 +1,6 @@ #![feature(plugin)] #![plugin(clippy)] -#![allow(unknown_lints, unused, no_effect, redundant_closure_call, many_single_char_names)] +#![allow(unknown_lints, unused, no_effect, redundant_closure_call, many_single_char_names, needless_take_by_value)] #![deny(redundant_closure)] fn main() { diff --git a/tests/ui/lifetimes.rs b/tests/ui/lifetimes.rs index ade0deeea..9d05e2bbb 100644 --- a/tests/ui/lifetimes.rs +++ b/tests/ui/lifetimes.rs @@ -2,7 +2,7 @@ #![plugin(clippy)] #![deny(needless_lifetimes, unused_lifetimes)] -#![allow(dead_code)] +#![allow(dead_code, needless_take_by_value)] fn distinct_lifetimes<'a, 'b>(_x: &'a u8, _y: &'b u8, _z: u8) { } diff --git a/tests/ui/needless_take_by_value.rs b/tests/ui/needless_take_by_value.rs new file mode 100644 index 000000000..6f6f576a3 --- /dev/null +++ b/tests/ui/needless_take_by_value.rs @@ -0,0 +1,26 @@ +#![feature(plugin)] +#![plugin(clippy)] + +#![deny(needless_take_by_value)] +#![allow(dead_code)] + +// `v` will be warned +// `w`, `x` and `y` are allowed (moved or mutated) +fn foo(v: Vec, w: Vec, mut x: Vec, y: Vec) -> Vec { + assert_eq!(v.len(), 42); + + consume(w); + + x.push(T::default()); + + y +} + +fn consume(_: T) {} + +// ok +fn test_fn i32>(f: F) { + f(1); +} + +fn main() {} diff --git a/tests/ui/needless_take_by_value.stderr b/tests/ui/needless_take_by_value.stderr new file mode 100644 index 000000000..d510989c3 --- /dev/null +++ b/tests/ui/needless_take_by_value.stderr @@ -0,0 +1,16 @@ +error: this function taking a value by value, but only using them by reference + --> $DIR/needless_take_by_value.rs:9:23 + | +9 | fn foo(v: Vec, w: Vec, mut x: Vec, y: Vec) -> Vec { + | ^^^^^^ + | +note: lint level defined here + --> $DIR/needless_take_by_value.rs:4:9 + | +4 | #![deny(needless_take_by_value)] + | ^^^^^^^^^^^^^^^^^^^^^^ +help: consider taking a reference instead + | fn foo(v: &Vec, w: Vec, mut x: Vec, y: Vec) -> Vec { + +error: aborting due to previous error + diff --git a/tests/ui/should_assert_eq.rs b/tests/ui/should_assert_eq.rs index 5abf35664..4f66f7cfb 100644 --- a/tests/ui/should_assert_eq.rs +++ b/tests/ui/should_assert_eq.rs @@ -1,6 +1,7 @@ #![feature(plugin)] #![plugin(clippy)] +#![allow(needless_take_by_value)] #![deny(should_assert_eq)] #[derive(PartialEq, Eq)] diff --git a/tests/ui/should_assert_eq.stderr b/tests/ui/should_assert_eq.stderr index 68d287fda..6cd729e40 100644 --- a/tests/ui/should_assert_eq.stderr +++ b/tests/ui/should_assert_eq.stderr @@ -1,28 +1,28 @@ error: use `assert_eq` for better reporting - --> $DIR/should_assert_eq.rs:13:5 + --> $DIR/should_assert_eq.rs:14:5 | -13 | assert!(1 == 2); +14 | assert!(1 == 2); | ^^^^^^^^^^^^^^^^ | note: lint level defined here - --> $DIR/should_assert_eq.rs:4:9 + --> $DIR/should_assert_eq.rs:5:9 | -4 | #![deny(should_assert_eq)] +5 | #![deny(should_assert_eq)] | ^^^^^^^^^^^^^^^^ = note: this error originates in a macro outside of the current crate error: use `assert_eq` for better reporting - --> $DIR/should_assert_eq.rs:14:5 + --> $DIR/should_assert_eq.rs:15:5 | -14 | assert!(Debug(1) == Debug(2)); +15 | assert!(Debug(1) == Debug(2)); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | = note: this error originates in a macro outside of the current crate error: use `assert_eq` for better reporting - --> $DIR/should_assert_eq.rs:21:5 + --> $DIR/should_assert_eq.rs:22:5 | -21 | assert!(x == y); +22 | assert!(x == y); | ^^^^^^^^^^^^^^^^ | = note: this error originates in a macro outside of the current crate diff --git a/tests/ui/unused_lt.rs b/tests/ui/unused_lt.rs index ec44420a3..b3639cf58 100644 --- a/tests/ui/unused_lt.rs +++ b/tests/ui/unused_lt.rs @@ -1,6 +1,6 @@ #![feature(plugin)] #![plugin(clippy)] -#![allow(unused, dead_code, needless_lifetimes)] +#![allow(unused, dead_code, needless_lifetimes, needless_take_by_value)] #![deny(unused_lifetimes)] fn empty() {