mikros/rust
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Merge pull request #884 from oli-obk/needless_ref2

Browse Source 
Add a `needless_borrow` lint
This commit is contained in:
Martin Carton 2016-05-09 13:15:33 +02:00
commit 488199d88f
23 changed files with 120 additions and 37 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

1
CHANGELOG.md
View File

@ -149,6 +149,7 @@ All notable changes to this project will be documented in this file.
[`mutex_atomic`]: https://github.com/Manishearth/rust-clippy/wiki#mutex_atomic
[`mutex_integer`]: https://github.com/Manishearth/rust-clippy/wiki#mutex_integer
[`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_range_loop`]: https://github.com/Manishearth/rust-clippy/wiki#needless_range_loop
[`needless_return`]: https://github.com/Manishearth/rust-clippy/wiki#needless_return

3
README.md
View File

@ -17,7 +17,7 @@ Table of contents:
## Lints
There are 146 lints included in this crate:
There are 147 lints included in this crate:
name | default | meaning
---------------------------------------------------------------------------------------------------------------------|---------|------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
@ -97,6 +97,7 @@ name
[mutex_atomic](https://github.com/Manishearth/rust-clippy/wiki#mutex_atomic) | warn | using a Mutex where an atomic value could be used instead
[mutex_integer](https://github.com/Manishearth/rust-clippy/wiki#mutex_integer) | allow | using a Mutex for an integer type
[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_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

2
src/array_indexing.rs
View File

@ -67,7 +67,7 @@ fn check_expr(&mut self, cx: &LateContext, e: &Expr) {
let size = ConstInt::Infer(size as u64);
// Index is a constant uint
let const_index = eval_const_expr_partial(cx.tcx, &index, ExprTypeChecked, None);
let const_index = eval_const_expr_partial(cx.tcx, index, ExprTypeChecked, None);
if let Ok(ConstVal::Integral(const_index)) = const_index {
if size <= const_index {
utils::span_lint(cx, OUT_OF_BOUNDS_INDEXING, e.span, "const index is out of bounds");

2
src/block_in_if_condition.rs
View File

@ -59,7 +59,7 @@ fn visit_expr(&mut self, expr: &'v Expr) {
}
};
if complex {
self.found_block = Some(&expr);
self.found_block = Some(expr);
return;
}
}

4
src/booleans.rs
View File

@ -221,7 +221,7 @@ fn recurse(brackets: bool, cx: &LateContext, suggestion: &Bool, terminals: &[&Ex
s.push('(');
}
}
s.push_str(&snip(&terminals[n as usize]));
s.push_str(&snip(terminals[n as usize]));
if brackets {
if let ExprBinary(..) = terminals[n as usize].node {
s.push(')');
@ -319,7 +319,7 @@ fn bool_expr(&self, e: &Expr) {
}
let mut improvements = Vec::new();
'simplified: for suggestion in &simplified {
let simplified_stats = terminal_stats(&suggestion);
let simplified_stats = terminal_stats(suggestion);
let mut improvement = false;
for i in 0..32 {
// ignore any "simplifications" that end up requiring a terminal more often than in the original expression

4
src/consts.rs
View File

@ -164,14 +164,14 @@ fn partial_cmp(&self, other: &Constant) -> Option<Ordering> {
}
}
(&Constant::Bool(ref l), &Constant::Bool(ref r)) => Some(l.cmp(r)),
(&Constant::Vec(ref l), &Constant::Vec(ref r)) => l.partial_cmp(&r),
(&Constant::Tuple(ref l), &Constant::Tuple(ref r)) |
(&Constant::Vec(ref l), &Constant::Vec(ref r)) => l.partial_cmp(r),
(&Constant::Repeat(ref lv, ref ls), &Constant::Repeat(ref rv, ref rs)) => {
match lv.partial_cmp(rv) {
Some(Equal) => Some(ls.cmp(rs)),
x => x,
}
}
(&Constant::Tuple(ref l), &Constant::Tuple(ref r)) => l.partial_cmp(r),
_ => None, //TODO: Are there any useful inter-type orderings?
}
}

6
src/copies.rs
View File

@ -142,7 +142,7 @@ fn lint_match_arms(cx: &LateContext, expr: &Expr) {
};
if let ExprMatch(_, ref arms, MatchSource::Normal) = expr.node {
if let Some((i, j)) = search_same(&arms, hash, eq) {
if let Some((i, j)) = search_same(arms, hash, eq) {
span_note_and_lint(cx,
MATCH_SAME_ARMS,
j.body.span,
@ -256,8 +256,8 @@ fn search_same<T, Hash, Eq>(exprs: &[T], hash: Hash, eq: Eq) -> Option<(&T, &T)>
match map.entry(hash(expr)) {
Entry::Occupied(o) => {
for o in o.get() {
if eq(&o, expr) {
return Some((&o, expr));
if eq(o, expr) {
return Some((o, expr));
}
}
}

2
src/cyclomatic_complexity.rs
View File

@ -58,7 +58,7 @@ fn check<'a, 'tcx>(&mut self, cx: &'a LateContext<'a, 'tcx>, block: &Block, span
divergence: 0,
short_circuits: 0,
returns: 0,
tcx: &cx.tcx,
tcx: cx.tcx,
};
helper.visit_block(block);
let CCHelper { match_arms, divergence, short_circuits, returns, .. } = helper;

8
src/enum_variants.rs
View File

@ -68,9 +68,9 @@ fn check_item(&mut self, cx: &EarlyContext, item: &Item) {
for var in &def.variants {
let name = var2str(var);
let pre_match = partial_match(&pre, &name);
let pre_match = partial_match(pre, &name);
pre = &pre[..pre_match];
let pre_camel = camel_case_until(&pre);
let pre_camel = camel_case_until(pre);
pre = &pre[..pre_camel];
while let Some((next, last)) = name[pre.len()..].chars().zip(pre.chars().rev()).next() {
if next.is_lowercase() {
@ -82,10 +82,10 @@ fn check_item(&mut self, cx: &EarlyContext, item: &Item) {
}
}
let post_match = partial_rmatch(&post, &name);
let post_match = partial_rmatch(post, &name);
let post_end = post.len() - post_match;
post = &post[post_end..];
let post_camel = camel_case_from(&post);
let post_camel = camel_case_from(post);
post = &post[post_camel..];
}
let (what, value) = match (pre.is_empty(), post.is_empty()) {

2
src/len_zero.rs
View File

@ -184,7 +184,7 @@ fn has_is_empty_impl(cx: &LateContext, id: &DefId) -> bool {
})
}
let ty = &walk_ptrs_ty(&cx.tcx.expr_ty(expr));
let ty = &walk_ptrs_ty(cx.tcx.expr_ty(expr));
match ty.sty {
ty::TyTrait(_) => {
cx.tcx

5
src/lib.rs
View File

@ -96,6 +96,7 @@ macro_rules! declare_restriction_lint {
pub mod mut_reference;
pub mod mutex_atomic;
pub mod needless_bool;
pub mod needless_borrow;
pub mod needless_update;
pub mod neg_multiply;
pub mod new_without_default;
@ -140,7 +141,7 @@ pub fn plugin_registrar(reg: &mut Registry) {
("clippy.toml", false)
};
let (conf, errors) = utils::conf::read_conf(&file_name, must_exist);
let (conf, errors) = utils::conf::read_conf(file_name, must_exist);
// all conf errors are non-fatal, we just use the default conf in case of error
for error in errors {
@ -210,6 +211,7 @@ pub fn plugin_registrar(reg: &mut Registry) {
reg.register_late_lint_pass(box zero_div_zero::ZeroDivZeroPass);
reg.register_late_lint_pass(box mutex_atomic::MutexAtomic);
reg.register_late_lint_pass(box needless_update::NeedlessUpdatePass);
reg.register_late_lint_pass(box needless_borrow::NeedlessBorrow);
reg.register_late_lint_pass(box no_effect::NoEffectPass);
reg.register_late_lint_pass(box map_clone::MapClonePass);
reg.register_late_lint_pass(box temporary_assignment::TemporaryAssignmentPass);
@ -364,6 +366,7 @@ pub fn plugin_registrar(reg: &mut Registry) {
mutex_atomic::MUTEX_ATOMIC,
needless_bool::BOOL_COMPARISON,
needless_bool::NEEDLESS_BOOL,
needless_borrow::NEEDLESS_BORROW,
needless_update::NEEDLESS_UPDATE,
neg_multiply::NEG_MULTIPLY,
new_without_default::NEW_WITHOUT_DEFAULT,

4
src/lifetimes.rs
View File

@ -46,7 +46,7 @@ fn get_lints(&self) -> LintArray {
impl LateLintPass for LifetimePass {
fn check_item(&mut self, cx: &LateContext, item: &Item) {
if let ItemFn(ref decl, _, _, _, ref generics, _) = item.node {
check_fn_inner(cx, decl, None, &generics, item.span);
check_fn_inner(cx, decl, None, generics, item.span);
}
}
@ -102,7 +102,7 @@ fn check_fn_inner(cx: &LateContext, decl: &FnDecl, slf: Option<&ExplicitSelf>, g
span,
"explicit lifetimes given in parameter types where they could be elided");
}
report_extra_lifetimes(cx, decl, &generics, slf);
report_extra_lifetimes(cx, decl, generics, slf);
}
fn could_use_elision<'a, T: Iterator<Item = &'a Lifetime>>(cx: &LateContext, func: &FnDecl, slf: Option<&ExplicitSelf>,

10
src/loops.rs
View File

@ -328,7 +328,7 @@ fn check_for_loop(cx: &LateContext, pat: &Pat, arg: &Expr, body: &Expr, expr: &E
/// Check for looping over a range and then indexing a sequence with it.
/// The iteratee must be a range literal.
fn check_for_loop_range(cx: &LateContext, pat: &Pat, arg: &Expr, body: &Expr, expr: &Expr) {
if let Some(UnsugaredRange { start: Some(ref start), ref end, .. }) = unsugar_range(&arg) {
if let Some(UnsugaredRange { start: Some(ref start), ref end, .. }) = unsugar_range(arg) {
// the var must be a single name
if let PatKind::Ident(_, ref ident, _) = pat.node {
let mut visitor = VarVisitor {
@ -363,7 +363,7 @@ fn check_for_loop_range(cx: &LateContext, pat: &Pat, arg: &Expr, body: &Expr, ex
};
let take: Cow<_> = if let Some(ref end) = *end {
if is_len_call(&end, &indexed) {
if is_len_call(end, &indexed) {
"".into()
} else {
format!(".take({})", snippet(cx, end.span, "..")).into()
@ -422,10 +422,10 @@ fn is_len_call(expr: &Expr, var: &Name) -> bool {
fn check_for_loop_reverse_range(cx: &LateContext, arg: &Expr, expr: &Expr) {
// if this for loop is iterating over a two-sided range...
if let Some(UnsugaredRange { start: Some(ref start), end: Some(ref end), limits }) = unsugar_range(&arg) {
if let Some(UnsugaredRange { start: Some(ref start), end: Some(ref end), limits }) = unsugar_range(arg) {
// ...and both sides are compile-time constant integers...
if let Ok(start_idx) = eval_const_expr_partial(&cx.tcx, start, ExprTypeChecked, None) {
if let Ok(end_idx) = eval_const_expr_partial(&cx.tcx, end, ExprTypeChecked, None) {
if let Ok(start_idx) = eval_const_expr_partial(cx.tcx, start, ExprTypeChecked, None) {
if let Ok(end_idx) = eval_const_expr_partial(cx.tcx, end, ExprTypeChecked, None) {
// ...and the start index is greater than the end index,
// this loop will never run. This is often confusing for developers
// who think that this will iterate from the larger value to the

6
src/matches.rs
View File

@ -461,8 +461,8 @@ fn cmp(&self, other: &Self) -> Ordering {
let mut values = Vec::with_capacity(2 * ranges.len());
for r in ranges {
values.push(Kind::Start(r.node.0, &r));
values.push(Kind::End(r.node.1, &r));
values.push(Kind::Start(r.node.0, r));
values.push(Kind::End(r.node.1, r));
}
values.sort();
@ -475,7 +475,7 @@ fn cmp(&self, other: &Self) -> Ordering {
}
}
(&Kind::End(a, _), &Kind::Start(b, _)) if a != b => (),
_ => return Some((&a.range(), &b.range())),
_ => return Some((a.range(), b.range())),
}
}

4
src/methods.rs
View File

@ -365,7 +365,7 @@ fn check_expr(&mut self, cx: &LateContext, expr: &Expr) {
lint_cstring_as_ptr(cx, expr, &arglists[0][0], &arglists[1][0]);
}
lint_or_fun_call(cx, expr, &name.node.as_str(), &args);
lint_or_fun_call(cx, expr, &name.node.as_str(), args);
let self_ty = cx.tcx.expr_ty_adjusted(&args[0]);
if args.len() == 1 && name.node.as_str() == "clone" {
@ -420,7 +420,7 @@ fn check_item(&mut self, cx: &LateContext, item: &Item) {
// check conventions w.r.t. conversion method names and predicates
let ty = cx.tcx.lookup_item_type(cx.tcx.map.local_def_id(item.id)).ty;
let is_copy = is_copy(cx, &ty, &item);
let is_copy = is_copy(cx, ty, item);
for &(ref conv, self_kinds) in &CONVENTIONS {
if conv.check(&name.as_str()) &&
!self_kinds.iter().any(|k| k.matches(&sig.explicit_self.node, is_copy)) {

2
src/misc.rs
View File

@ -424,7 +424,7 @@ fn is_used(cx: &LateContext, expr: &Expr) -> bool {
match parent.node {
ExprAssign(_, ref rhs) |
ExprAssignOp(_, _, ref rhs) => **rhs == *expr,
_ => is_used(cx, &parent),
_ => is_used(cx, parent),
}
} else {
true

4
src/mut_reference.rs
View File

@ -39,13 +39,13 @@ fn check_expr(&mut self, cx: &LateContext, e: &Expr) {
If this happened, the compiler would have \
aborted the compilation long ago");
if let ExprPath(_, ref path) = fn_expr.node {
check_arguments(cx, &arguments, function_type, &path.to_string());
check_arguments(cx, arguments, function_type, &path.to_string());
}
}
ExprMethodCall(ref name, _, ref arguments) => {
let method_call = MethodCall::expr(e.id);
let method_type = borrowed_table.method_map.get(&method_call).expect("This should never happen.");
check_arguments(cx, &arguments, method_type.ty, &name.node.as_str())
check_arguments(cx, arguments, method_type.ty, &name.node.as_str())
}
_ => (),
}

50
src/needless_borrow.rs Normal file
View File

@ -0,0 +1,50 @@
//! Checks for needless address of operations (`&`)
//!
//! This lint is **warn** by default
use rustc::lint::*;
use rustc::hir::*;
use rustc::ty::TyRef;
use utils::{span_lint, in_macro};
/// **What it does:** This lint checks for address of operations (`&`) that are going to be dereferenced immediately by the compiler
///
/// **Why is this bad?** Suggests that the receiver of the expression borrows the expression
///
/// **Known problems:**
///
/// **Example:** `let x: &i32 = &&&&&&5;`
declare_lint! {
pub NEEDLESS_BORROW,
Warn,
"taking a reference that is going to be automatically dereferenced"
}
#[derive(Copy,Clone)]
pub struct NeedlessBorrow;
impl LintPass for NeedlessBorrow {
fn get_lints(&self) -> LintArray {
lint_array!(NEEDLESS_BORROW)
}
}
impl LateLintPass for NeedlessBorrow {
fn check_expr(&mut self, cx: &LateContext, e: &Expr) {
if in_macro(cx, e.span) {
return;
}
if let ExprAddrOf(MutImmutable, ref inner) = e.node {
if let TyRef(..) = cx.tcx.expr_ty(inner).sty {
let ty = cx.tcx.expr_ty(e);
let adj_ty = cx.tcx.expr_ty_adjusted(e);
if ty != adj_ty {
span_lint(cx,
NEEDLESS_BORROW,
e.span,
"this expression borrows a reference that is immediately dereferenced by the compiler");
}
}
}
}
}

2
src/non_expressive_names.rs
View File

@ -249,7 +249,7 @@ fn check_item(&mut self, cx: &EarlyContext, item: &Item) {
let mut visitor = SimilarNamesLocalVisitor {
names: Vec::new(),
cx: cx,
lint: &self,
lint: self,
single_char_names: Vec::new(),
};
// initialize with function arguments

2
src/shadow.rs
View File

@ -278,7 +278,7 @@ fn check_expr(cx: &LateContext, expr: &Expr, bindings: &mut Vec<(Name, Span)>) {
let len = bindings.len();
for ref arm in arms {
for ref pat in &arm.pats {
check_pat(cx, &pat, &Some(&**init), pat.span, bindings);
check_pat(cx, pat, &Some(&**init), pat.span, bindings);
// This is ugly, but needed to get the right type
if let Some(ref guard) = arm.guard {
check_expr(cx, guard, bindings);

6
src/types.rs
View File

@ -910,7 +910,7 @@ fn upcast_comparison_bounds_err(cx: &LateContext, span: &Span, rel: comparisons:
if let Some(norm_rhs_val) = node_as_const_fullint(cx, rhs) {
if rel == Rel::Eq || rel == Rel::Ne {
if norm_rhs_val < lb || norm_rhs_val > ub {
err_upcast_comparison(cx, &span, lhs, rel == Rel::Ne);
err_upcast_comparison(cx, span, lhs, rel == Rel::Ne);
}
} else if match rel {
Rel::Lt => {
@ -929,7 +929,7 @@ fn upcast_comparison_bounds_err(cx: &LateContext, span: &Span, rel: comparisons:
}
Rel::Eq | Rel::Ne => unreachable!(),
} {
err_upcast_comparison(cx, &span, lhs, true)
err_upcast_comparison(cx, span, lhs, true)
} else if match rel {
Rel::Lt => {
if invert {
@ -947,7 +947,7 @@ fn upcast_comparison_bounds_err(cx: &LateContext, span: &Span, rel: comparisons:
}
Rel::Eq | Rel::Ne => unreachable!(),
} {
err_upcast_comparison(cx, &span, lhs, false)
err_upcast_comparison(cx, span, lhs, false)
}
}
}

1
tests/compile-fail/eta.rs
View File

@ -18,6 +18,7 @@ fn main() {
//~| SUGGESTION let c = Some(1u8).map({1+2; foo});
let d = Some(1u8).map(|a| foo((|b| foo2(b))(a))); //is adjusted?
all(&[1, 2, 3], &&2, |x, y| below(x, y)); //is adjusted
//~^ WARN needless_borrow
unsafe {
Some(1u8).map(|a| unsafe_fn(a)); // unsafe fn
}

27
tests/compile-fail/needless_borrow.rs Normal file
View File

@ -0,0 +1,27 @@
#![feature(plugin)]
#![plugin(clippy)]
fn x(y: &i32) -> i32 {
*y
}
#[deny(clippy)]
#[allow(unused_variables)]
fn main() {
let a = 5;
let b = x(&a);
let c = x(&&a); //~ ERROR: needless_borrow
let s = &String::from("hi");
let s_ident = f(&s); // should not error, because `&String` implements Copy, but `String` does not
let g_val = g(&Vec::new()); // should not error, because `&Vec<T>` derefs to `&[T]`
let vec = Vec::new();
let vec_val = g(&vec); // should not error, because `&Vec<T>` derefs to `&[T]`
}
fn f<T:Copy>(y: &T) -> T {
*y
}
fn g(y: &[u8]) -> u8 {
y[0]
}