From d81d961ba73db132d605ac18bf38511a8a8ab9e5 Mon Sep 17 00:00:00 2001 From: sinkuu Date: Sat, 18 Feb 2017 17:00:36 +0900 Subject: [PATCH 1/9] Lint needless take-by-value --- CHANGELOG.md | 1 + README.md | 3 +- clippy_lints/src/consts.rs | 4 +- clippy_lints/src/lib.rs | 3 + clippy_lints/src/needless_take_by_value.rs | 178 +++++++++++++++++++++ clippy_lints/src/utils/paths.rs | 2 + tests/issue-825.rs | 2 +- tests/ui/absurd-extreme-comparisons.rs | 2 +- tests/ui/box_vec.rs | 2 +- tests/ui/complex_types.rs | 2 +- tests/ui/dlist.rs | 2 +- tests/ui/drop_forget_ref.rs | 2 +- tests/ui/entry.rs | 2 +- tests/ui/eta.rs | 2 +- tests/ui/lifetimes.rs | 2 +- tests/ui/needless_take_by_value.rs | 26 +++ tests/ui/needless_take_by_value.stderr | 16 ++ tests/ui/should_assert_eq.rs | 1 + tests/ui/should_assert_eq.stderr | 16 +- tests/ui/unused_lt.rs | 2 +- 20 files changed, 249 insertions(+), 21 deletions(-) create mode 100644 clippy_lints/src/needless_take_by_value.rs create mode 100644 tests/ui/needless_take_by_value.rs create mode 100644 tests/ui/needless_take_by_value.stderr diff --git a/CHANGELOG.md b/CHANGELOG.md index 1192195cd6a5..0c829f268c3c 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 842e654678b8..a3f4dcf4a0b7 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 f6c9619dcf0a..3dbd427d0634 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 814daf7d6458..1d497b4ef43f 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 000000000000..8d2e2dfa0a75 --- /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 5edff76d9969..57d2e5a33f10 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 026bd36ee6e5..2d6c8ea384bd 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 495dd27bb2bc..d4248175aa53 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 01f1b1d09497..92304753e588 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 ef5cd68972db..64f1b4dcbdbe 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 10d0beab8344..3661b63ef536 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 911d3433e4d8..44f6f54bc8c9 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 816da06b956f..495c024f1514 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 90ffa61216fd..51fde7d85763 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 ade0deeea2ca..9d05e2bbbf92 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 000000000000..6f6f576a3c94 --- /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 000000000000..d510989c3440 --- /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 5abf35664259..4f66f7cfb5b2 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 68d287fda8d5..6cd729e4035d 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 ec44420a376e..b3639cf587cc 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() { From d2f73e7818bb000d22fa3c02b6bef68e458d8d6d Mon Sep 17 00:00:00 2001 From: sinkuu Date: Sun, 19 Feb 2017 18:06:14 +0900 Subject: [PATCH 2/9] Add explanation --- clippy_lints/src/needless_take_by_value.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/clippy_lints/src/needless_take_by_value.rs b/clippy_lints/src/needless_take_by_value.rs index 8d2e2dfa0a75..fcff4c64df3e 100644 --- a/clippy_lints/src/needless_take_by_value.rs +++ b/clippy_lints/src/needless_take_by_value.rs @@ -13,7 +13,8 @@ use std::collections::HashSet; /// **What it does:** Checks for functions taking arguments by value, but only using them by /// reference. /// -/// **Why is this bad?** +/// **Why is this bad?** In such cases, taking arguments by reference is more flexible and +/// can sometimes avoid unnecessary allocations. /// /// **Known problems:** Hopefully none. /// From 0a6bc6031aa9bca562941a33f385c81e1eeda5af Mon Sep 17 00:00:00 2001 From: sinkuu Date: Mon, 20 Feb 2017 12:50:31 +0900 Subject: [PATCH 3/9] Rename lint to needless_take_by_value And fixes false-positives for generics and `match` --- CHANGELOG.md | 2 +- README.md | 2 +- clippy_lints/src/lib.rs | 6 +- clippy_lints/src/matches.rs | 6 +- ..._by_value.rs => needless_pass_by_value.rs} | 110 +++++++++++++----- clippy_lints/src/utils/sugg.rs | 2 +- tests/ui/absurd-extreme-comparisons.rs | 2 +- tests/ui/box_vec.rs | 2 +- tests/ui/complex_types.rs | 2 +- tests/ui/dlist.rs | 2 +- tests/ui/drop_forget_ref.rs | 2 +- tests/ui/entry.rs | 2 +- tests/ui/eta.rs | 2 +- tests/ui/lifetimes.rs | 2 +- ..._by_value.rs => needless_pass_by_value.rs} | 15 ++- tests/ui/needless_pass_by_value.stderr | 43 +++++++ tests/ui/needless_take_by_value.stderr | 16 --- tests/ui/should_assert_eq.rs | 2 +- tests/ui/unused_lt.rs | 2 +- 19 files changed, 155 insertions(+), 67 deletions(-) rename clippy_lints/src/{needless_take_by_value.rs => needless_pass_by_value.rs} (51%) rename tests/ui/{needless_take_by_value.rs => needless_pass_by_value.rs} (54%) create mode 100644 tests/ui/needless_pass_by_value.stderr delete mode 100644 tests/ui/needless_take_by_value.stderr diff --git a/CHANGELOG.md b/CHANGELOG.md index 0c829f268c3c..a3b07c0ceaf9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -381,9 +381,9 @@ All notable changes to this project will be documented in this file. [`needless_bool`]: https://github.com/Manishearth/rust-clippy/wiki#needless_bool [`needless_borrow`]: https://github.com/Manishearth/rust-clippy/wiki#needless_borrow [`needless_lifetimes`]: https://github.com/Manishearth/rust-clippy/wiki#needless_lifetimes +[`needless_pass_by_value`]: https://github.com/Manishearth/rust-clippy/wiki#needless_pass_by_value [`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 a3f4dcf4a0b7..7cff4a30fe12 100644 --- a/README.md +++ b/README.md @@ -287,9 +287,9 @@ name [needless_bool](https://github.com/Manishearth/rust-clippy/wiki#needless_bool) | warn | if-statements with plain booleans in the then- and else-clause, e.g. `if p { true } else { false }` [needless_borrow](https://github.com/Manishearth/rust-clippy/wiki#needless_borrow) | warn | taking a reference that is going to be automatically dereferenced [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_pass_by_value](https://github.com/Manishearth/rust-clippy/wiki#needless_pass_by_value) | warn | functions taking arguments by value, but only using them by reference [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/lib.rs b/clippy_lints/src/lib.rs index 1d497b4ef43f..7d231ff99b50 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -107,7 +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_pass_by_value; pub mod needless_update; pub mod neg_multiply; pub mod new_without_default; @@ -300,7 +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_late_lint_pass(box needless_pass_by_value::NeedlessPassByValue); reg.register_lint_group("clippy_restrictions", vec![ arithmetic::FLOAT_ARITHMETIC, @@ -457,7 +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_pass_by_value::NEEDLESS_PASS_BY_VALUE, needless_update::NEEDLESS_UPDATE, neg_multiply::NEG_MULTIPLY, new_without_default::NEW_WITHOUT_DEFAULT, diff --git a/clippy_lints/src/matches.rs b/clippy_lints/src/matches.rs index 66b55c48455c..5a8ff442262c 100644 --- a/clippy_lints/src/matches.rs +++ b/clippy_lints/src/matches.rs @@ -395,7 +395,7 @@ fn check_match_ref_pats(cx: &LateContext, ex: &Expr, arms: &[Arm], source: Match "you don't need to add `&` to both the expression and the patterns", |db| { let inner = Sugg::hir(cx, inner, ".."); - let template = match_template(expr.span, source, inner); + let template = match_template(expr.span, source, &inner); db.span_suggestion(expr.span, "try", template); }); } else { @@ -405,7 +405,7 @@ fn check_match_ref_pats(cx: &LateContext, ex: &Expr, arms: &[Arm], source: Match "you don't need to add `&` to all patterns", |db| { let ex = Sugg::hir(cx, ex, ".."); - let template = match_template(expr.span, source, ex.deref()); + let template = match_template(expr.span, source, &ex.deref()); db.span_suggestion(expr.span, "instead of prefixing all patterns with `&`, you can dereference the expression", template); @@ -509,7 +509,7 @@ fn has_only_ref_pats(arms: &[Arm]) -> bool { mapped.map_or(false, |v| v.iter().any(|el| *el)) } -fn match_template(span: Span, source: MatchSource, expr: Sugg) -> String { +fn match_template(span: Span, source: MatchSource, expr: &Sugg) -> String { match source { MatchSource::Normal => format!("match {} {{ .. }}", expr), MatchSource::IfLetDesugar { .. } => format!("if let .. = {} {{ .. }}", expr), diff --git a/clippy_lints/src/needless_take_by_value.rs b/clippy_lints/src/needless_pass_by_value.rs similarity index 51% rename from clippy_lints/src/needless_take_by_value.rs rename to clippy_lints/src/needless_pass_by_value.rs index fcff4c64df3e..a292739302d7 100644 --- a/clippy_lints/src/needless_take_by_value.rs +++ b/clippy_lints/src/needless_pass_by_value.rs @@ -2,19 +2,21 @@ use rustc::hir::*; use rustc::hir::intravisit::FnKind; use rustc::hir::def_id::DefId; use rustc::lint::*; -use rustc::ty; +use rustc::ty::{self, TypeFoldable}; +use rustc::traits; 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 utils::{in_macro, is_self, is_copy, implements_trait, get_trait_def_id, match_type, 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. +/// **What it does:** Checks for functions taking arguments by value, but not consuming them in its +/// body. /// -/// **Why is this bad?** In such cases, taking arguments by reference is more flexible and -/// can sometimes avoid unnecessary allocations. +/// **Why is this bad?** Taking arguments by reference is more flexible and can sometimes avoid +/// unnecessary allocations. /// /// **Known problems:** Hopefully none. /// @@ -25,20 +27,20 @@ use std::collections::HashSet; /// } /// ``` declare_lint! { - pub NEEDLESS_TAKE_BY_VALUE, + pub NEEDLESS_PASS_BY_VALUE, Warn, - "taking arguments by value, but only using them by reference" + "functions taking arguments by value, but not consuming them in its body" } -pub struct NeedlessTakeByValue; +pub struct NeedlessPassByValue; -impl LintPass for NeedlessTakeByValue { +impl LintPass for NeedlessPassByValue { fn get_lints(&self) -> LintArray { - lint_array![NEEDLESS_TAKE_BY_VALUE] + lint_array![NEEDLESS_PASS_BY_VALUE] } } -impl<'a, 'tcx> LateLintPass<'a, 'tcx> for NeedlessTakeByValue { +impl<'a, 'tcx> LateLintPass<'a, 'tcx> for NeedlessPassByValue { fn check_fn( &mut self, cx: &LateContext<'a, 'tcx>, @@ -57,11 +59,18 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for NeedlessTakeByValue { return; } - // These are usually took by value and only used by reference + // These are usually passed 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"); + let preds: Vec = { + let parameter_env = ty::ParameterEnvironment::for_item(cx.tcx, node_id); + traits::elaborate_predicates(cx.tcx, parameter_env.caller_bounds.clone()) + .filter(|p| !p.is_global()) + .collect() + }; + // Collect moved variables from the function body let moved_vars = { let mut ctx = MovedVariablesCtxt::new(cx); @@ -79,13 +88,26 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for NeedlessTakeByValue { 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) { + + // Determines whether `ty` implements `Borrow` (U != ty) specifically. + // `implements_trait(.., borrow_trait, ..)` is useless + // due to the `Borrow for T` blanket impl. + let implements_borrow_trait = preds.iter() + .filter_map(|pred| if let ty::Predicate::Trait(ref poly_trait_ref) = *pred { + Some(poly_trait_ref.skip_binder()) + } else { + None + }) + .filter(|tpred| tpred.def_id() == borrow_trait && &tpred.self_ty() == ty) + .any(|tpred| &tpred.input_types().nth(1).expect("Borrow trait must have an input") != ty); + 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)), + !implements_borrow_trait, let PatKind::Binding(mode, defid, ..) = arg.pat.node, !moved_vars.contains(&defid), @@ -99,14 +121,34 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for NeedlessTakeByValue { } span_lint_and_then(cx, - NEEDLESS_TAKE_BY_VALUE, + NEEDLESS_PASS_BY_VALUE, input.span, - "this function taking a value by value, but only using them by reference", + "this argument is passed by value, but not consumed in the function body", |db| { - db.span_suggestion(input.span, - "consider taking a reference instead", - format!("&{}", snippet(cx, input.span, "_"))); - }); + if_let_chain! {[ + match_type(cx, ty, &paths::VEC), + let TyPath(QPath::Resolved(_, ref path)) = input.node, + let Some(elem_ty) = path.segments.iter() + .find(|seg| &*seg.name.as_str() == "Vec") + .map(|ps| ps.parameters.types()[0]), + ], { + let slice_ty = format!("&[{}]", snippet(cx, elem_ty.span, "_")); + db.span_suggestion(input.span, + &format!("consider changing the type to `{}`", slice_ty), + slice_ty); + return; + }} + + if match_type(cx, ty, &paths::STRING) { + db.span_suggestion(input.span, + "consider changing the type to `&str`", + "&str".to_string()); + } else { + db.span_suggestion(input.span, + "consider taking a reference instead", + format!("&{}", snippet(cx, input.span, "_"))); + } + }); }} } } @@ -125,13 +167,11 @@ impl<'a, 'tcx: 'a> MovedVariablesCtxt<'a, 'tcx> { } } - fn consume_common( - &mut self, - _consume_id: NodeId, - _consume_span: Span, - cmt: mc::cmt<'tcx>, - mode: euv::ConsumeMode - ) { + fn consume_common(&mut self, _span: Span, cmt: mc::cmt<'tcx>, mode: euv::ConsumeMode) { + /*::utils::span_lint(self.cx, + NEEDLESS_PASS_BY_VALUE, + span, + &format!("consumed here, {:?} {:?}", mode, cmt.cat));*/ if_let_chain! {[ let euv::ConsumeMode::Move(_) = mode, let mc::Categorization::Local(vid) = cmt.cat, @@ -145,14 +185,22 @@ impl<'a, 'tcx: 'a> MovedVariablesCtxt<'a, 'tcx> { } 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 consume(&mut self, _consume_id: NodeId, consume_span: Span, cmt: mc::cmt<'tcx>, mode: euv::ConsumeMode) { + self.consume_common(consume_span, cmt, mode); } - fn matched_pat(&mut self, _matched_pat: &Pat, _cmt: mc::cmt, _mode: euv::MatchMode) {} + fn matched_pat(&mut self, matched_pat: &Pat, mut cmt: mc::cmt<'tcx>, _mode: euv::MatchMode) { + if let mc::Categorization::Downcast(c, _) = cmt.cat.clone() { + cmt = c; + } + + // if let euv::MatchMode::MovingMatch = mode { + self.consume_common(matched_pat.span, cmt, euv::ConsumeMode::Move(euv::MoveReason::PatBindingMove)); + // } + } 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); + self.consume_common(consume_pat.span, cmt, mode); } fn borrow( diff --git a/clippy_lints/src/utils/sugg.rs b/clippy_lints/src/utils/sugg.rs index 2c10db158cb3..b4ae42497fe9 100644 --- a/clippy_lints/src/utils/sugg.rs +++ b/clippy_lints/src/utils/sugg.rs @@ -301,7 +301,7 @@ pub fn make_binop(op: ast::BinOpKind, lhs: &Sugg, rhs: &Sugg) -> Sugg<'static> { make_assoc(AssocOp::from_ast_binop(op), lhs, rhs) } -#[derive(PartialEq, Eq)] +#[derive(PartialEq, Eq, Clone, Copy)] /// Operator associativity. enum Associativity { /// The operator is both left-associative and right-associative. diff --git a/tests/ui/absurd-extreme-comparisons.rs b/tests/ui/absurd-extreme-comparisons.rs index d4248175aa53..3482237bd0bc 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, needless_take_by_value)] +#![allow(unused, eq_op, no_effect, unnecessary_operation, needless_pass_by_value)] fn main() { const Z: u32 = 0; diff --git a/tests/ui/box_vec.rs b/tests/ui/box_vec.rs index 92304753e588..836ac0fd37fa 100644 --- a/tests/ui/box_vec.rs +++ b/tests/ui/box_vec.rs @@ -2,7 +2,7 @@ #![plugin(clippy)] #![deny(clippy)] -#![allow(boxed_local, needless_take_by_value)] +#![allow(boxed_local, needless_pass_by_value)] #![allow(blacklisted_name)] macro_rules! boxit { diff --git a/tests/ui/complex_types.rs b/tests/ui/complex_types.rs index 64f1b4dcbdbe..f3b9e0a03af8 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, needless_take_by_value)] +#![allow(unused, needless_pass_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 3661b63ef536..2a3cefcbc1f4 100644 --- a/tests/ui/dlist.rs +++ b/tests/ui/dlist.rs @@ -4,7 +4,7 @@ #![plugin(clippy)] #![deny(clippy)] -#![allow(dead_code, needless_take_by_value)] +#![allow(dead_code, needless_pass_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 44f6f54bc8c9..97c6df8563e5 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, needless_take_by_value)] +#![allow(toplevel_ref_arg, similar_names, needless_pass_by_value)] use std::mem::{drop, forget}; diff --git a/tests/ui/entry.rs b/tests/ui/entry.rs index 495c024f1514..090221ba7a83 100644 --- a/tests/ui/entry.rs +++ b/tests/ui/entry.rs @@ -1,6 +1,6 @@ #![feature(plugin)] #![plugin(clippy)] -#![allow(unused, needless_take_by_value)] +#![allow(unused, needless_pass_by_value)] #![deny(map_entry)] diff --git a/tests/ui/eta.rs b/tests/ui/eta.rs index 51fde7d85763..d333b938a325 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, needless_take_by_value)] +#![allow(unknown_lints, unused, no_effect, redundant_closure_call, many_single_char_names, needless_pass_by_value)] #![deny(redundant_closure)] fn main() { diff --git a/tests/ui/lifetimes.rs b/tests/ui/lifetimes.rs index 9d05e2bbbf92..f953c0ad3d26 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, needless_take_by_value)] +#![allow(dead_code, needless_pass_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_pass_by_value.rs similarity index 54% rename from tests/ui/needless_take_by_value.rs rename to tests/ui/needless_pass_by_value.rs index 6f6f576a3c94..8cdc71ba4e6b 100644 --- a/tests/ui/needless_take_by_value.rs +++ b/tests/ui/needless_pass_by_value.rs @@ -1,7 +1,7 @@ #![feature(plugin)] #![plugin(clippy)] -#![deny(needless_take_by_value)] +#![deny(needless_pass_by_value)] #![allow(dead_code)] // `v` will be warned @@ -18,6 +18,19 @@ fn foo(v: Vec, w: Vec, mut x: Vec, y: Vec) -> Vec { fn consume(_: T) {} +struct Wrapper(String); + +fn bar(x: String, y: Wrapper) { + assert_eq!(x.len(), 42); + assert_eq!(y.0.len(), 42); +} + +fn test_borrow_trait, U>(t: T, u: U) { + // U implements `Borrow`, but warned correctly + println!("{}", t.borrow()); + consume(&u); +} + // ok fn test_fn i32>(f: F) { f(1); diff --git a/tests/ui/needless_pass_by_value.stderr b/tests/ui/needless_pass_by_value.stderr new file mode 100644 index 000000000000..2dec96d8f8b7 --- /dev/null +++ b/tests/ui/needless_pass_by_value.stderr @@ -0,0 +1,43 @@ +error: this argument is passed by value, but not consumed in the function body + --> $DIR/needless_pass_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_pass_by_value.rs:4:9 + | +4 | #![deny(needless_pass_by_value)] + | ^^^^^^^^^^^^^^^^^^^^^^ +help: consider changing the type to `&[T]` + | fn foo(v: &[T], w: Vec, mut x: Vec, y: Vec) -> Vec { + +error: this argument is passed by value, but not consumed in the function body + --> $DIR/needless_pass_by_value.rs:23:11 + | +23 | fn bar(x: String, y: Wrapper) { + | ^^^^^^ + | +help: consider changing the type to `&str` + | fn bar(x: &str, y: Wrapper) { + +error: this argument is passed by value, but not consumed in the function body + --> $DIR/needless_pass_by_value.rs:23:22 + | +23 | fn bar(x: String, y: Wrapper) { + | ^^^^^^^ + | +help: consider taking a reference instead + | fn bar(x: String, y: &Wrapper) { + +error: this argument is passed by value, but not consumed in the function body + --> $DIR/needless_pass_by_value.rs:28:63 + | +28 | fn test_borrow_trait, U>(t: T, u: U) { + | ^ + | +help: consider taking a reference instead + | fn test_borrow_trait, U>(t: T, u: &U) { + +error: aborting due to 4 previous errors + diff --git a/tests/ui/needless_take_by_value.stderr b/tests/ui/needless_take_by_value.stderr deleted file mode 100644 index d510989c3440..000000000000 --- a/tests/ui/needless_take_by_value.stderr +++ /dev/null @@ -1,16 +0,0 @@ -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 4f66f7cfb5b2..e6144f0027ae 100644 --- a/tests/ui/should_assert_eq.rs +++ b/tests/ui/should_assert_eq.rs @@ -1,7 +1,7 @@ #![feature(plugin)] #![plugin(clippy)] -#![allow(needless_take_by_value)] +#![allow(needless_pass_by_value)] #![deny(should_assert_eq)] #[derive(PartialEq, Eq)] diff --git a/tests/ui/unused_lt.rs b/tests/ui/unused_lt.rs index b3639cf587cc..b4462a87ab47 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, needless_take_by_value)] +#![allow(unused, dead_code, needless_lifetimes, needless_pass_by_value)] #![deny(unused_lifetimes)] fn empty() { From f1b0b774e7acac5859da808dae614a529ded8a91 Mon Sep 17 00:00:00 2001 From: sinkuu Date: Mon, 20 Feb 2017 16:45:37 +0900 Subject: [PATCH 4/9] Support non-moving usages at `match` This `match` is not moving the `String`: ```rust fn foo(x: Option) -> i32 { match x { Some(_) => 1, None => 2, } } ``` With this change, it will be linted and suggested to add `*` to deref it. ```rust fn foo(x: &Option) -> i32 { match *x { Some(_) => 1, None => 2, } } ``` --- clippy_lints/src/array_indexing.rs | 10 +- clippy_lints/src/consts.rs | 6 +- clippy_lints/src/needless_pass_by_value.rs | 101 +++++++++++++++------ tests/ui/needless_pass_by_value.rs | 25 ++++- tests/ui/needless_pass_by_value.stderr | 26 +++++- 5 files changed, 128 insertions(+), 40 deletions(-) diff --git a/clippy_lints/src/array_indexing.rs b/clippy_lints/src/array_indexing.rs index 74e9dfddc1cd..2ab70da8c734 100644 --- a/clippy_lints/src/array_indexing.rs +++ b/clippy_lints/src/array_indexing.rs @@ -83,7 +83,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for ArrayIndexing { .map(|end| constcx.eval(end, ExprTypeChecked)) .map(|v| v.ok()); - if let Some((start, end)) = to_const_range(start, end, range.limits, size) { + if let Some((start, end)) = to_const_range(&start, &end, range.limits, size) { if start > size || end > size { utils::span_lint(cx, OUT_OF_BOUNDS_INDEXING, e.span, "range is out of bounds"); } @@ -109,18 +109,18 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for ArrayIndexing { /// Returns an option containing a tuple with the start and end (exclusive) of the range. fn to_const_range( - start: Option>, - end: Option>, + start: &Option>, + end: &Option>, limits: RangeLimits, array_size: ConstInt ) -> Option<(ConstInt, ConstInt)> { - let start = match start { + let start = match *start { Some(Some(ConstVal::Integral(x))) => x, Some(_) => return None, None => ConstInt::Infer(0), }; - let end = match end { + let end = match *end { Some(Some(ConstVal::Integral(x))) => { if limits == RangeLimits::Closed { (x + ConstInt::Infer(1)).expect("such a big array is not realistic") diff --git a/clippy_lints/src/consts.rs b/clippy_lints/src/consts.rs index 3dbd427d0634..46371b938bc7 100644 --- a/clippy_lints/src/consts.rs +++ b/clippy_lints/src/consts.rs @@ -203,9 +203,9 @@ pub fn lit_to_constant(lit: &LitKind) -> Constant { } } -fn constant_not(o: Constant) -> Option { +fn constant_not(o: &Constant) -> Option { use self::Constant::*; - match o { + match *o { Bool(b) => Some(Bool(!b)), Int(value) => (!value).ok().map(Int), _ => None, @@ -271,7 +271,7 @@ impl<'c, 'cc> ConstEvalLateContext<'c, 'cc> { }, ExprUnary(op, ref operand) => { self.expr(operand).and_then(|o| match op { - UnNot => constant_not(o), + UnNot => constant_not(&o), UnNeg => constant_negate(o), UnDeref => Some(o), }) diff --git a/clippy_lints/src/needless_pass_by_value.rs b/clippy_lints/src/needless_pass_by_value.rs index a292739302d7..e4491eda0385 100644 --- a/clippy_lints/src/needless_pass_by_value.rs +++ b/clippy_lints/src/needless_pass_by_value.rs @@ -10,7 +10,7 @@ use syntax::ast::NodeId; use syntax_pos::Span; use utils::{in_macro, is_self, is_copy, implements_trait, get_trait_def_id, match_type, snippet, span_lint_and_then, paths}; -use std::collections::HashSet; +use std::collections::{HashSet, HashMap}; /// **What it does:** Checks for functions taking arguments by value, but not consuming them in its /// body. @@ -71,15 +71,15 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for NeedlessPassByValue { .collect() }; - // Collect moved variables from the function body - let moved_vars = { + // Collect moved variables and non-moving usages at `match`es from the function body + let MovedVariablesCtxt { moved_vars, non_moving_matches, .. } = { 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 + ctx }; let fn_def_id = cx.tcx.hir.local_def_id(node_id); @@ -90,8 +90,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for NeedlessPassByValue { for ((input, ty), arg) in decl.inputs.iter().zip(fn_sig.inputs()).zip(&body.arguments) { // Determines whether `ty` implements `Borrow` (U != ty) specifically. - // `implements_trait(.., borrow_trait, ..)` is useless - // due to the `Borrow for T` blanket impl. + // This is needed due to the `Borrow for T` blanket impl. let implements_borrow_trait = preds.iter() .filter_map(|pred| if let ty::Predicate::Trait(ref poly_trait_ref) = *pred { Some(poly_trait_ref.skip_binder()) @@ -99,7 +98,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for NeedlessPassByValue { None }) .filter(|tpred| tpred.def_id() == borrow_trait && &tpred.self_ty() == ty) - .any(|tpred| &tpred.input_types().nth(1).expect("Borrow trait must have an input") != ty); + .any(|tpred| &tpred.input_types().nth(1).expect("Borrow trait must have an parameter") != ty); if_let_chain! {[ !is_self(arg), @@ -148,6 +147,24 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for NeedlessPassByValue { "consider taking a reference instead", format!("&{}", snippet(cx, input.span, "_"))); } + + // For non-moving consumption at `match`es, + // suggests adding `*` to dereference the added reference. + // e.g. `match x { Some(_) => 1, None => 2 }` + // -> `match *x { .. }` + if let Some(matches) = non_moving_matches.get(&defid) { + for (i, m) in matches.iter().enumerate() { + if let ExprMatch(ref e, ..) = cx.tcx.hir.expect_expr(*m).node { + db.span_suggestion(e.span, + if i == 0 { + "...and dereference it here" + } else { + "...and here" + }, + format!("*{}", snippet(cx, e.span, ""))); + } + } + } }); }} } @@ -157,6 +174,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for NeedlessPassByValue { struct MovedVariablesCtxt<'a, 'tcx: 'a> { cx: &'a LateContext<'a, 'tcx>, moved_vars: HashSet, + non_moving_matches: HashMap>, } impl<'a, 'tcx: 'a> MovedVariablesCtxt<'a, 'tcx> { @@ -164,43 +182,61 @@ impl<'a, 'tcx: 'a> MovedVariablesCtxt<'a, 'tcx> { MovedVariablesCtxt { cx: cx, moved_vars: HashSet::new(), + non_moving_matches: HashMap::new(), } } - fn consume_common(&mut self, _span: Span, cmt: mc::cmt<'tcx>, mode: euv::ConsumeMode) { - /*::utils::span_lint(self.cx, - NEEDLESS_PASS_BY_VALUE, - span, - &format!("consumed here, {:?} {:?}", mode, cmt.cat));*/ + fn move_common(&mut self, _consume_id: NodeId, _span: Span, cmt: mc::cmt<'tcx>) { + let cmt = unwrap_downcast_or_interior(cmt); + if_let_chain! {[ - let euv::ConsumeMode::Move(_) = mode, let mc::Categorization::Local(vid) = cmt.cat, + let Some(def_id) = self.cx.tcx.hir.opt_local_def_id(vid), ], { - 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_span, cmt, mode); + fn consume(&mut self, consume_id: NodeId, consume_span: Span, cmt: mc::cmt<'tcx>, mode: euv::ConsumeMode) { + if let euv::ConsumeMode::Move(_) = mode { + self.move_common(consume_id, consume_span, cmt); + } } - fn matched_pat(&mut self, matched_pat: &Pat, mut cmt: mc::cmt<'tcx>, _mode: euv::MatchMode) { - if let mc::Categorization::Downcast(c, _) = cmt.cat.clone() { - cmt = c; - } + fn matched_pat(&mut self, matched_pat: &Pat, cmt: mc::cmt<'tcx>, mode: euv::MatchMode) { + if let euv::MatchMode::MovingMatch = mode { + self.move_common(matched_pat.id, matched_pat.span, cmt); + } else { + let cmt = unwrap_downcast_or_interior(cmt); - // if let euv::MatchMode::MovingMatch = mode { - self.consume_common(matched_pat.span, cmt, euv::ConsumeMode::Move(euv::MoveReason::PatBindingMove)); - // } + if_let_chain! {[ + let mc::Categorization::Local(vid) = cmt.cat, + let Some(def_id) = self.cx.tcx.hir.opt_local_def_id(vid), + ], { + // Find the `ExprMatch` which contains this pattern + let mut match_id = matched_pat.id; + loop { + match_id = self.cx.tcx.hir.get_parent_node(match_id); + if_let_chain! {[ + let Some(map::Node::NodeExpr(e)) = self.cx.tcx.hir.find(match_id), + let ExprMatch(..) = e.node, + ], { + break; + }} + } + + self.non_moving_matches.entry(def_id).or_insert_with(HashSet::new) + .insert(match_id); + }} + } } fn consume_pat(&mut self, consume_pat: &Pat, cmt: mc::cmt<'tcx>, mode: euv::ConsumeMode) { - self.consume_common(consume_pat.span, cmt, mode); + if let euv::ConsumeMode::Move(_) = mode { + self.move_common(consume_pat.id, consume_pat.span, cmt); + } } fn borrow( @@ -225,3 +261,16 @@ impl<'a, 'tcx: 'a> euv::Delegate<'tcx> for MovedVariablesCtxt<'a, 'tcx> { fn decl_without_init(&mut self, _id: NodeId, _span: Span) {} } + + +fn unwrap_downcast_or_interior(mut cmt: mc::cmt) -> mc::cmt { + loop { + match cmt.cat.clone() { + mc::Categorization::Downcast(c, _) | + mc::Categorization::Interior(c, _) => { + cmt = c; + }, + _ => return cmt, + } + } +} diff --git a/tests/ui/needless_pass_by_value.rs b/tests/ui/needless_pass_by_value.rs index 8cdc71ba4e6b..74554aa974b1 100644 --- a/tests/ui/needless_pass_by_value.rs +++ b/tests/ui/needless_pass_by_value.rs @@ -2,9 +2,9 @@ #![plugin(clippy)] #![deny(needless_pass_by_value)] -#![allow(dead_code)] +#![allow(dead_code, single_match)] -// `v` will be warned +// `v` should 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); @@ -25,8 +25,8 @@ fn bar(x: String, y: Wrapper) { assert_eq!(y.0.len(), 42); } +// U implements `Borrow`, but should be warned correctly fn test_borrow_trait, U>(t: T, u: U) { - // U implements `Borrow`, but warned correctly println!("{}", t.borrow()); consume(&u); } @@ -36,4 +36,23 @@ fn test_fn i32>(f: F) { f(1); } +// x should be warned, but y is ok +fn test_match(x: Option>, y: Option>) { + match x { + Some(Some(_)) => 1, // not moved + _ => 0, + }; + + match y { + Some(Some(s)) => consume(s), // moved + _ => (), + }; +} + +// x should be warned, but y is ok +fn test_destructure(x: Wrapper, y: Wrapper) { + let Wrapper(s) = y; // moved + assert_eq!(x.0.len(), s.len()); +} + fn main() {} diff --git a/tests/ui/needless_pass_by_value.stderr b/tests/ui/needless_pass_by_value.stderr index 2dec96d8f8b7..cd7cadb885d4 100644 --- a/tests/ui/needless_pass_by_value.stderr +++ b/tests/ui/needless_pass_by_value.stderr @@ -31,13 +31,33 @@ help: consider taking a reference instead | fn bar(x: String, y: &Wrapper) { error: this argument is passed by value, but not consumed in the function body - --> $DIR/needless_pass_by_value.rs:28:63 + --> $DIR/needless_pass_by_value.rs:29:63 | -28 | fn test_borrow_trait, U>(t: T, u: U) { +29 | fn test_borrow_trait, U>(t: T, u: U) { | ^ | help: consider taking a reference instead | fn test_borrow_trait, U>(t: T, u: &U) { -error: aborting due to 4 previous errors +error: this argument is passed by value, but not consumed in the function body + --> $DIR/needless_pass_by_value.rs:40:18 + | +40 | fn test_match(x: Option>, y: Option>) { + | ^^^^^^^^^^^^^^^^^^^^^^ + | +help: consider taking a reference instead + | fn test_match(x: &Option>, y: Option>) { +help: ...and dereference it here + | match *x { + +error: this argument is passed by value, but not consumed in the function body + --> $DIR/needless_pass_by_value.rs:53:24 + | +53 | fn test_destructure(x: Wrapper, y: Wrapper) { + | ^^^^^^^ + | +help: consider taking a reference instead + | fn test_destructure(x: &Wrapper, y: Wrapper) { + +error: aborting due to 6 previous errors From 263e60ce0b0e1c8199f715d6208538ddfea64b46 Mon Sep 17 00:00:00 2001 From: sinkuu Date: Mon, 20 Feb 2017 16:55:52 +0900 Subject: [PATCH 5/9] Run update_lints.py --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index 7cff4a30fe12..b6aebd15fefe 100644 --- a/README.md +++ b/README.md @@ -287,7 +287,7 @@ name [needless_bool](https://github.com/Manishearth/rust-clippy/wiki#needless_bool) | warn | if-statements with plain booleans in the then- and else-clause, e.g. `if p { true } else { false }` [needless_borrow](https://github.com/Manishearth/rust-clippy/wiki#needless_borrow) | warn | taking a reference that is going to be automatically dereferenced [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_pass_by_value](https://github.com/Manishearth/rust-clippy/wiki#needless_pass_by_value) | warn | functions taking arguments by value, but only using them by reference +[needless_pass_by_value](https://github.com/Manishearth/rust-clippy/wiki#needless_pass_by_value) | warn | functions taking arguments by value, but not consuming them in its body [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_update](https://github.com/Manishearth/rust-clippy/wiki#needless_update) | warn | using `Foo { ..base }` when there are no missing fields From 627d24c80fb998c04deba1d6169a5651e1f60ec2 Mon Sep 17 00:00:00 2001 From: sinkuu Date: Mon, 20 Feb 2017 18:18:31 +0900 Subject: [PATCH 6/9] Fix suggestion for `let ` --- clippy_lints/src/needless_pass_by_value.rs | 151 ++++++++++++--------- tests/ui/needless_pass_by_value.rs | 10 +- tests/ui/needless_pass_by_value.stderr | 17 ++- 3 files changed, 109 insertions(+), 69 deletions(-) diff --git a/clippy_lints/src/needless_pass_by_value.rs b/clippy_lints/src/needless_pass_by_value.rs index e4491eda0385..ff19700a41f4 100644 --- a/clippy_lints/src/needless_pass_by_value.rs +++ b/clippy_lints/src/needless_pass_by_value.rs @@ -8,6 +8,7 @@ use rustc::middle::expr_use_visitor as euv; use rustc::middle::mem_categorization as mc; use syntax::ast::NodeId; use syntax_pos::Span; +use syntax::errors::DiagnosticBuilder; use utils::{in_macro, is_self, is_copy, implements_trait, get_trait_def_id, match_type, snippet, span_lint_and_then, paths}; use std::collections::{HashSet, HashMap}; @@ -59,7 +60,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for NeedlessPassByValue { return; } - // These are usually passed by value and only used by reference + // Allows these to be passed by value. 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"); @@ -71,8 +72,8 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for NeedlessPassByValue { .collect() }; - // Collect moved variables and non-moving usages at `match`es from the function body - let MovedVariablesCtxt { moved_vars, non_moving_matches, .. } = { + // Collect moved variables and spans which will need dereferencings from the function body. + let MovedVariablesCtxt { moved_vars, spans_need_deref, .. } = { let mut ctx = MovedVariablesCtxt::new(cx); let infcx = cx.tcx.borrowck_fake_infer_ctxt(body.id()); { @@ -119,11 +120,8 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for NeedlessPassByValue { continue; } - span_lint_and_then(cx, - NEEDLESS_PASS_BY_VALUE, - input.span, - "this argument is passed by value, but not consumed in the function body", - |db| { + // Suggestion logic + let sugg = |db: &mut DiagnosticBuilder| { if_let_chain! {[ match_type(cx, ty, &paths::VEC), let TyPath(QPath::Resolved(_, ref path)) = input.node, @@ -148,24 +146,27 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for NeedlessPassByValue { format!("&{}", snippet(cx, input.span, "_"))); } - // For non-moving consumption at `match`es, - // suggests adding `*` to dereference the added reference. - // e.g. `match x { Some(_) => 1, None => 2 }` - // -> `match *x { .. }` - if let Some(matches) = non_moving_matches.get(&defid) { - for (i, m) in matches.iter().enumerate() { - if let ExprMatch(ref e, ..) = cx.tcx.hir.expect_expr(*m).node { - db.span_suggestion(e.span, - if i == 0 { - "...and dereference it here" - } else { - "...and here" - }, - format!("*{}", snippet(cx, e.span, ""))); - } + // Suggests adding `*` to dereference the added reference. + if let Some(spans) = spans_need_deref.get(&defid) { + let mut spans: Vec<_> = spans.iter().cloned().collect(); + spans.sort(); + for (i, span) in spans.into_iter().enumerate() { + db.span_suggestion(span, + if i == 0 { + "...and dereference it here" + } else { + "...and here" + }, + format!("*{}", snippet(cx, span, ""))); } } - }); + }; + + span_lint_and_then(cx, + NEEDLESS_PASS_BY_VALUE, + input.span, + "this argument is passed by value, but not consumed in the function body", + sugg); }} } } @@ -174,7 +175,9 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for NeedlessPassByValue { struct MovedVariablesCtxt<'a, 'tcx: 'a> { cx: &'a LateContext<'a, 'tcx>, moved_vars: HashSet, - non_moving_matches: HashMap>, + /// Spans which need to be prefixed with `*` for dereferencing the suggested additional + /// reference. + spans_need_deref: HashMap>, } impl<'a, 'tcx: 'a> MovedVariablesCtxt<'a, 'tcx> { @@ -182,7 +185,7 @@ impl<'a, 'tcx: 'a> MovedVariablesCtxt<'a, 'tcx> { MovedVariablesCtxt { cx: cx, moved_vars: HashSet::new(), - non_moving_matches: HashMap::new(), + spans_need_deref: HashMap::new(), } } @@ -196,6 +199,57 @@ impl<'a, 'tcx: 'a> MovedVariablesCtxt<'a, 'tcx> { self.moved_vars.insert(def_id); }} } + + fn non_moving_pat(&mut self, matched_pat: &Pat, cmt: mc::cmt<'tcx>) { + let cmt = unwrap_downcast_or_interior(cmt); + + if_let_chain! {[ + let mc::Categorization::Local(vid) = cmt.cat, + let Some(def_id) = self.cx.tcx.hir.opt_local_def_id(vid), + ], { + let mut id = matched_pat.id; + loop { + let parent = self.cx.tcx.hir.get_parent_node(id); + if id == parent { + // no parent + return; + } + id = parent; + + if let Some(node) = self.cx.tcx.hir.find(id) { + match node { + map::Node::NodeExpr(e) => { + // `match` and `if let` + if let ExprMatch(ref c, ..) = e.node { + self.spans_need_deref + .entry(def_id) + .or_insert_with(HashSet::new) + .insert(c.span); + } + } + + map::Node::NodeStmt(s) => { + // `let = x;` + if_let_chain! {[ + let StmtDecl(ref decl, _) = s.node, + let DeclLocal(ref local) = decl.node, + ], { + self.spans_need_deref + .entry(def_id) + .or_insert_with(HashSet::new) + .insert(local.init + .as_ref() + .map(|e| e.span) + .expect("`let` stmt without init aren't caught by match_pat")); + }} + } + + _ => {} + } + } + } + }} + } } impl<'a, 'tcx: 'a> euv::Delegate<'tcx> for MovedVariablesCtxt<'a, 'tcx> { @@ -209,27 +263,7 @@ impl<'a, 'tcx: 'a> euv::Delegate<'tcx> for MovedVariablesCtxt<'a, 'tcx> { if let euv::MatchMode::MovingMatch = mode { self.move_common(matched_pat.id, matched_pat.span, cmt); } else { - let cmt = unwrap_downcast_or_interior(cmt); - - if_let_chain! {[ - let mc::Categorization::Local(vid) = cmt.cat, - let Some(def_id) = self.cx.tcx.hir.opt_local_def_id(vid), - ], { - // Find the `ExprMatch` which contains this pattern - let mut match_id = matched_pat.id; - loop { - match_id = self.cx.tcx.hir.get_parent_node(match_id); - if_let_chain! {[ - let Some(map::Node::NodeExpr(e)) = self.cx.tcx.hir.find(match_id), - let ExprMatch(..) = e.node, - ], { - break; - }} - } - - self.non_moving_matches.entry(def_id).or_insert_with(HashSet::new) - .insert(match_id); - }} + self.non_moving_pat(matched_pat, cmt); } } @@ -241,25 +275,18 @@ impl<'a, 'tcx: 'a> euv::Delegate<'tcx> for MovedVariablesCtxt<'a, 'tcx> { 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 + _: NodeId, + _: Span, + _: mc::cmt<'tcx>, + _: &'tcx ty::Region, + _: ty::BorrowKind, + _: euv::LoanCause ) { } - fn mutate( - &mut self, - _assignment_id: NodeId, - _assignment_span: Span, - _assignee_cmt: mc::cmt<'tcx>, - _mode: euv::MutateMode - ) { - } + fn mutate(&mut self, _: NodeId, _: Span, _: mc::cmt<'tcx>, _: euv::MutateMode) {} - fn decl_without_init(&mut self, _id: NodeId, _span: Span) {} + fn decl_without_init(&mut self, _: NodeId, _: Span) {} } diff --git a/tests/ui/needless_pass_by_value.rs b/tests/ui/needless_pass_by_value.rs index 74554aa974b1..fbfd3c517f9f 100644 --- a/tests/ui/needless_pass_by_value.rs +++ b/tests/ui/needless_pass_by_value.rs @@ -2,7 +2,7 @@ #![plugin(clippy)] #![deny(needless_pass_by_value)] -#![allow(dead_code, single_match)] +#![allow(dead_code, single_match, if_let_redundant_pattern_matching, many_single_char_names)] // `v` should be warned // `w`, `x` and `y` are allowed (moved or mutated) @@ -49,10 +49,12 @@ fn test_match(x: Option>, y: Option>) { }; } -// x should be warned, but y is ok -fn test_destructure(x: Wrapper, y: Wrapper) { - let Wrapper(s) = y; // moved +// x and y should be warned, but z is ok +fn test_destructure(x: Wrapper, y: Wrapper, z: Wrapper) { + let Wrapper(s) = z; // moved + let Wrapper(ref t) = y; // not moved assert_eq!(x.0.len(), s.len()); + println!("{}", t); } fn main() {} diff --git a/tests/ui/needless_pass_by_value.stderr b/tests/ui/needless_pass_by_value.stderr index cd7cadb885d4..08df6b976ae6 100644 --- a/tests/ui/needless_pass_by_value.stderr +++ b/tests/ui/needless_pass_by_value.stderr @@ -53,11 +53,22 @@ help: ...and dereference it here error: this argument is passed by value, but not consumed in the function body --> $DIR/needless_pass_by_value.rs:53:24 | -53 | fn test_destructure(x: Wrapper, y: Wrapper) { +53 | fn test_destructure(x: Wrapper, y: Wrapper, z: Wrapper) { | ^^^^^^^ | help: consider taking a reference instead - | fn test_destructure(x: &Wrapper, y: Wrapper) { + | fn test_destructure(x: &Wrapper, y: Wrapper, z: Wrapper) { -error: aborting due to 6 previous errors +error: this argument is passed by value, but not consumed in the function body + --> $DIR/needless_pass_by_value.rs:53:36 + | +53 | fn test_destructure(x: Wrapper, y: Wrapper, z: Wrapper) { + | ^^^^^^^ + | +help: consider taking a reference instead + | fn test_destructure(x: Wrapper, y: &Wrapper, z: Wrapper) { +help: ...and dereference it here + | let Wrapper(ref t) = *y; // not moved + +error: aborting due to 7 previous errors From 3516d45d7c03bbecb67a468bf06fa217484b6ecd Mon Sep 17 00:00:00 2001 From: sinkuu Date: Tue, 21 Feb 2017 18:44:31 +0900 Subject: [PATCH 7/9] Use `multispan_sugg` --- clippy_lints/src/loops.rs | 10 +++++----- clippy_lints/src/needless_pass_by_value.rs | 23 ++++++++-------------- clippy_lints/src/utils/mod.rs | 4 ++-- tests/ui/needless_pass_by_value.rs | 2 ++ tests/ui/needless_pass_by_value.stderr | 1 + 5 files changed, 18 insertions(+), 22 deletions(-) diff --git a/clippy_lints/src/loops.rs b/clippy_lints/src/loops.rs index d7f952255fe2..5fb16cc05eb6 100644 --- a/clippy_lints/src/loops.rs +++ b/clippy_lints/src/loops.rs @@ -549,8 +549,8 @@ fn check_for_loop_range<'a, 'tcx>( |db| { multispan_sugg(db, "consider using an iterator".to_string(), - &[(pat.span, &format!("({}, )", ident.node)), - (arg.span, &format!("{}.iter().enumerate(){}{}", indexed, take, skip))]); + vec![(pat.span, format!("({}, )", ident.node)), + (arg.span, format!("{}.iter().enumerate(){}{}", indexed, take, skip))]); }); } else { let repl = if starts_at_zero && take.is_empty() { @@ -568,7 +568,7 @@ fn check_for_loop_range<'a, 'tcx>( |db| { multispan_sugg(db, "consider using an iterator".to_string(), - &[(pat.span, ""), (arg.span, &repl)]); + vec![(pat.span, "".to_string()), (arg.span, repl)]); }); } } @@ -816,8 +816,8 @@ fn check_for_loop_over_map_kv<'a, 'tcx>( let map = sugg::Sugg::hir(cx, arg, "map"); multispan_sugg(db, "use the corresponding method".into(), - &[(pat_span, &snippet(cx, new_pat_span, kind)), - (arg_span, &format!("{}.{}s{}()", map.maybe_par(), kind, mutbl))]); + vec![(pat_span, snippet(cx, new_pat_span, kind).into_owned()), + (arg_span, format!("{}.{}s{}()", map.maybe_par(), kind, mutbl))]); }); } } diff --git a/clippy_lints/src/needless_pass_by_value.rs b/clippy_lints/src/needless_pass_by_value.rs index ff19700a41f4..4666cb575efd 100644 --- a/clippy_lints/src/needless_pass_by_value.rs +++ b/clippy_lints/src/needless_pass_by_value.rs @@ -10,7 +10,7 @@ use syntax::ast::NodeId; use syntax_pos::Span; use syntax::errors::DiagnosticBuilder; use utils::{in_macro, is_self, is_copy, implements_trait, get_trait_def_id, match_type, snippet, span_lint_and_then, - paths}; + multispan_sugg, paths}; use std::collections::{HashSet, HashMap}; /// **What it does:** Checks for functions taking arguments by value, but not consuming them in its @@ -55,8 +55,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for NeedlessPassByValue { return; } - if let FnKind::ItemFn(..) = kind { - } else { + if !matches!(kind, FnKind::ItemFn(..)) { return; } @@ -146,19 +145,13 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for NeedlessPassByValue { format!("&{}", snippet(cx, input.span, "_"))); } - // Suggests adding `*` to dereference the added reference. + // Suggests adding `*` to dereference the added reference if needed. if let Some(spans) = spans_need_deref.get(&defid) { - let mut spans: Vec<_> = spans.iter().cloned().collect(); - spans.sort(); - for (i, span) in spans.into_iter().enumerate() { - db.span_suggestion(span, - if i == 0 { - "...and dereference it here" - } else { - "...and here" - }, - format!("*{}", snippet(cx, span, ""))); - } + let mut spans: Vec<_> = spans.iter().cloned() + .map(|span| (span, format!("*{}", snippet(cx, span, "")))) + .collect(); + spans.sort_by_key(|&(span, _)| span); + multispan_sugg(db, "...and dereference it here".to_string(), spans); } }; diff --git a/clippy_lints/src/utils/mod.rs b/clippy_lints/src/utils/mod.rs index ce581febfe41..44d4963a98d3 100644 --- a/clippy_lints/src/utils/mod.rs +++ b/clippy_lints/src/utils/mod.rs @@ -573,10 +573,10 @@ pub fn span_lint_and_then<'a, 'tcx: 'a, T: LintContext<'tcx>, F>( /// /// Note: in the JSON format (used by `compiletest_rs`), the help message will appear once per /// replacement. In human-readable format though, it only appears once before the whole suggestion. -pub fn multispan_sugg(db: &mut DiagnosticBuilder, help_msg: String, sugg: &[(Span, &str)]) { +pub fn multispan_sugg(db: &mut DiagnosticBuilder, help_msg: String, sugg: Vec<(Span, String)>) { let sugg = rustc_errors::RenderSpan::Suggestion(rustc_errors::CodeSuggestion { msp: MultiSpan::from_spans(sugg.iter().map(|&(span, _)| span).collect()), - substitutes: sugg.iter().map(|&(_, subs)| subs.to_owned()).collect(), + substitutes: sugg.into_iter().map(|(_, subs)| subs).collect(), }); let sub = rustc_errors::SubDiagnostic { diff --git a/tests/ui/needless_pass_by_value.rs b/tests/ui/needless_pass_by_value.rs index fbfd3c517f9f..5caf8017f96a 100644 --- a/tests/ui/needless_pass_by_value.rs +++ b/tests/ui/needless_pass_by_value.rs @@ -53,6 +53,8 @@ fn test_match(x: Option>, y: Option>) { fn test_destructure(x: Wrapper, y: Wrapper, z: Wrapper) { let Wrapper(s) = z; // moved let Wrapper(ref t) = y; // not moved + let Wrapper(_) = y; // still not moved + assert_eq!(x.0.len(), s.len()); println!("{}", t); } diff --git a/tests/ui/needless_pass_by_value.stderr b/tests/ui/needless_pass_by_value.stderr index 08df6b976ae6..ded9e0aa731d 100644 --- a/tests/ui/needless_pass_by_value.stderr +++ b/tests/ui/needless_pass_by_value.stderr @@ -69,6 +69,7 @@ help: consider taking a reference instead | fn test_destructure(x: Wrapper, y: &Wrapper, z: Wrapper) { help: ...and dereference it here | let Wrapper(ref t) = *y; // not moved + | let Wrapper(_) = *y; // still not moved error: aborting due to 7 previous errors From cb86c57c5f9c0fc5e5f736bbd57f95df204e00e1 Mon Sep 17 00:00:00 2001 From: sinkuu Date: Tue, 21 Feb 2017 19:03:50 +0900 Subject: [PATCH 8/9] Integrate suggestion spans --- clippy_lints/src/needless_pass_by_value.rs | 20 +++++++++----------- tests/ui/needless_pass_by_value.stderr | 3 +-- 2 files changed, 10 insertions(+), 13 deletions(-) diff --git a/clippy_lints/src/needless_pass_by_value.rs b/clippy_lints/src/needless_pass_by_value.rs index 4666cb575efd..bc5ecd653c1e 100644 --- a/clippy_lints/src/needless_pass_by_value.rs +++ b/clippy_lints/src/needless_pass_by_value.rs @@ -132,27 +132,25 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for NeedlessPassByValue { db.span_suggestion(input.span, &format!("consider changing the type to `{}`", slice_ty), slice_ty); - return; + return; // `Vec` and `String` cannot be destructured - no need for `*` suggestion }} if match_type(cx, ty, &paths::STRING) { db.span_suggestion(input.span, "consider changing the type to `&str`", "&str".to_string()); - } else { - db.span_suggestion(input.span, - "consider taking a reference instead", - format!("&{}", snippet(cx, input.span, "_"))); + return; } - // Suggests adding `*` to dereference the added reference if needed. - if let Some(spans) = spans_need_deref.get(&defid) { - let mut spans: Vec<_> = spans.iter().cloned() - .map(|span| (span, format!("*{}", snippet(cx, span, "")))) - .collect(); + let mut spans = vec![(input.span, format!("&{}", snippet(cx, input.span, "_")))]; + + // Suggests adding `*` to dereference the added reference. + if let Some(deref_span) = spans_need_deref.get(&defid) { + spans.extend(deref_span.iter().cloned() + .map(|span| (span, format!("*{}", snippet(cx, span, ""))))); spans.sort_by_key(|&(span, _)| span); - multispan_sugg(db, "...and dereference it here".to_string(), spans); } + multispan_sugg(db, "consider taking a reference instead".to_string(), spans); }; span_lint_and_then(cx, diff --git a/tests/ui/needless_pass_by_value.stderr b/tests/ui/needless_pass_by_value.stderr index ded9e0aa731d..88a424a9696a 100644 --- a/tests/ui/needless_pass_by_value.stderr +++ b/tests/ui/needless_pass_by_value.stderr @@ -47,7 +47,6 @@ error: this argument is passed by value, but not consumed in the function body | help: consider taking a reference instead | fn test_match(x: &Option>, y: Option>) { -help: ...and dereference it here | match *x { error: this argument is passed by value, but not consumed in the function body @@ -67,7 +66,7 @@ error: this argument is passed by value, but not consumed in the function body | help: consider taking a reference instead | fn test_destructure(x: Wrapper, y: &Wrapper, z: Wrapper) { -help: ...and dereference it here + | let Wrapper(s) = z; // moved | let Wrapper(ref t) = *y; // not moved | let Wrapper(_) = *y; // still not moved From bf21c84629278b862f2df184542aff1e3512a44f Mon Sep 17 00:00:00 2001 From: sinkuu Date: Tue, 21 Feb 2017 19:45:45 +0900 Subject: [PATCH 9/9] Ensure deref_span is empty for Vec and String --- clippy_lints/src/needless_pass_by_value.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/clippy_lints/src/needless_pass_by_value.rs b/clippy_lints/src/needless_pass_by_value.rs index bc5ecd653c1e..72399a9e6343 100644 --- a/clippy_lints/src/needless_pass_by_value.rs +++ b/clippy_lints/src/needless_pass_by_value.rs @@ -121,6 +121,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for NeedlessPassByValue { // Suggestion logic let sugg = |db: &mut DiagnosticBuilder| { + let deref_span = spans_need_deref.get(&defid); if_let_chain! {[ match_type(cx, ty, &paths::VEC), let TyPath(QPath::Resolved(_, ref path)) = input.node, @@ -132,6 +133,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for NeedlessPassByValue { db.span_suggestion(input.span, &format!("consider changing the type to `{}`", slice_ty), slice_ty); + assert!(deref_span.is_none()); return; // `Vec` and `String` cannot be destructured - no need for `*` suggestion }} @@ -139,13 +141,14 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for NeedlessPassByValue { db.span_suggestion(input.span, "consider changing the type to `&str`", "&str".to_string()); + assert!(deref_span.is_none()); return; } let mut spans = vec![(input.span, format!("&{}", snippet(cx, input.span, "_")))]; // Suggests adding `*` to dereference the added reference. - if let Some(deref_span) = spans_need_deref.get(&defid) { + if let Some(deref_span) = deref_span { spans.extend(deref_span.iter().cloned() .map(|span| (span, format!("*{}", snippet(cx, span, ""))))); spans.sort_by_key(|&(span, _)| span);