Mirrors/rust-clippy
2
0
Fork 0
mirror of https://github.com/rust-lang/rust-clippy synced 2024-11-10 15:14:29 +00:00
Code Issues Projects Releases Packages Wiki Activity

Rename lint to needless_take_by_value

Browse source 
And fixes false-positives for generics and `match`
This commit is contained in:
sinkuu 2017-02-20 12:50:31 +09:00
parent d2f73e7818
commit 0a6bc6031a
19 changed files with 155 additions and 67 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

2
CHANGELOG.md
View file

@ -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

2
README.md
View file

@ -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

6
clippy_lints/src/lib.rs
View file

@ -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,

6
clippy_lints/src/matches.rs
View file

@ -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),

110
clippy_lints/src/needless_take_by_value.rs → clippy_lints/src/needless_pass_by_value.rs
View file

@ -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<ty::Predicate> = {
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>` (U != ty) specifically.
// `implements_trait(.., borrow_trait, ..)` is useless
// due to the `Borrow<T> 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(

2
clippy_lints/src/utils/sugg.rs
View file

@ -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.

2
tests/ui/absurd-extreme-comparisons.rs
View file

@ -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;

2
tests/ui/box_vec.rs
View file

@ -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 {

2
tests/ui/complex_types.rs
View file

@ -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<Vec<Box<(u32, u32, u32, u32)>>>; // no warning here

2
tests/ui/dlist.rs
View file

@ -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;

2
tests/ui/drop_forget_ref.rs
View file

@ -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};

2
tests/ui/entry.rs
View file

@ -1,6 +1,6 @@
#![feature(plugin)]
#![plugin(clippy)]
#![allow(unused, needless_take_by_value)]
#![allow(unused, needless_pass_by_value)]
#![deny(map_entry)]

2
tests/ui/eta.rs
View file

@ -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() {

2
tests/ui/lifetimes.rs
View file

@ -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) { }

15
tests/ui/needless_take_by_value.rs → tests/ui/needless_pass_by_value.rs
View file

@ -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<T: Default>(v: Vec<T>, w: Vec<T>, mut x: Vec<T>, y: Vec<T>) -> Vec<T> {
fn consume<T>(_: 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<T: std::borrow::Borrow<str>, U>(t: T, u: U) {
// U implements `Borrow<U>`, but warned correctly
println!("{}", t.borrow());
consume(&u);
}
// ok
fn test_fn<F: Fn(i32) -> i32>(f: F) {
f(1);

43
tests/ui/needless_pass_by_value.stderr Normal file
View file

@ -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<T: Default>(v: Vec<T>, w: Vec<T>, mut x: Vec<T>, y: Vec<T>) -> Vec<T> {
| ^^^^^^
|
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<T: Default>(v: &[T], w: Vec<T>, mut x: Vec<T>, y: Vec<T>) -> Vec<T> {
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<T: std::borrow::Borrow<str>, U>(t: T, u: U) {
| ^
|
help: consider taking a reference instead
| fn test_borrow_trait<T: std::borrow::Borrow<str>, U>(t: T, u: &U) {
error: aborting due to 4 previous errors

16
tests/ui/needless_take_by_value.stderr
View file

@ -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<T: Default>(v: Vec<T>, w: Vec<T>, mut x: Vec<T>, y: Vec<T>) -> Vec<T> {
| ^^^^^^
|
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<T: Default>(v: &Vec<T>, w: Vec<T>, mut x: Vec<T>, y: Vec<T>) -> Vec<T> {
error: aborting due to previous error

2
tests/ui/should_assert_eq.rs
View file

@ -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)]

2
tests/ui/unused_lt.rs
View file

@ -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() {