nordic-dev.net/rust
nordic-dev.net/rust
2
0
Fork 0
mirror of https://github.com/rust-lang/rust.git synced 2024-12-19 20:13:47 +00:00
Code Issues Packages Projects Releases Wiki Activity

Merge pull request #513 from mcarton/entries

Browse Source 
Implement  #433
This commit is contained in:
Manish Goregaokar 2016-01-04 09:36:42 +05:30
commit fc789a675b
8 changed files with 230 additions and 83 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

3
README.md
View File

@ -6,7 +6,7 @@ A collection of lints to catch common mistakes and improve your Rust code.
[Jump to usage instructions](#usage)
##Lints
There are 90 lints included in this crate:
There are 91 lints included in this crate:
name | default | meaning
---------------------------------------------------------------------------------------------------------------|---------|------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
@ -31,6 +31,7 @@ name
[explicit_iter_loop](https://github.com/Manishearth/rust-clippy/wiki#explicit_iter_loop) | warn | for-looping over `_.iter()` or `_.iter_mut()` when `&_` or `&mut _` would do
[filter_next](https://github.com/Manishearth/rust-clippy/wiki#filter_next) | warn | using `filter(p).next()`, which is more succinctly expressed as `.find(p)`
[float_cmp](https://github.com/Manishearth/rust-clippy/wiki#float_cmp) | warn | using `==` or `!=` on float values (as floating-point operations usually involve rounding errors, it is always better to check for approximate equality within small bounds)
[hashmap_entry](https://github.com/Manishearth/rust-clippy/wiki#hashmap_entry) | warn | use of `contains_key` followed by `insert` on a `HashMap`
[identity_op](https://github.com/Manishearth/rust-clippy/wiki#identity_op) | warn | using identity operations, e.g. `x + 0` or `y / 1`
[ineffective_bit_mask](https://github.com/Manishearth/rust-clippy/wiki#ineffective_bit_mask) | warn | expressions where a bit mask will be rendered useless by a comparison, e.g. `(x | 1) > 2`
[inline_always](https://github.com/Manishearth/rust-clippy/wiki#inline_always) | warn | `#[inline(always)]` is a bad idea in most cases

71
src/eq_op.rs
View File

@ -1,10 +1,8 @@
use rustc::lint::*;
use rustc_front::hir::*;
use rustc_front::util as ast_util;
use syntax::ptr::P;
use consts::constant;
use utils::span_lint;
use utils::{is_exp_equal, span_lint};
/// **What it does:** This lint checks for equal operands to comparisons and bitwise binary operators (`&`, `|` and `^`). It is `Warn` by default.
///
@ -40,57 +38,6 @@ impl LateLintPass for EqOp {
}
}
pub fn is_exp_equal(cx: &LateContext, left : &Expr, right : &Expr) -> bool {
if let (Some(l), Some(r)) = (constant(cx, left), constant(cx, right)) {
if l == r {
return true;
}
}
match (&left.node, &right.node) {
(&ExprField(ref lfexp, ref lfident),
&ExprField(ref rfexp, ref rfident)) =>
lfident.node == rfident.node && is_exp_equal(cx, lfexp, rfexp),
(&ExprLit(ref l), &ExprLit(ref r)) => l.node == r.node,
(&ExprPath(ref lqself, ref lsubpath),
&ExprPath(ref rqself, ref rsubpath)) =>
both(lqself, rqself, is_qself_equal) &&
is_path_equal(lsubpath, rsubpath),
(&ExprTup(ref ltup), &ExprTup(ref rtup)) =>
is_exps_equal(cx, ltup, rtup),
(&ExprVec(ref l), &ExprVec(ref r)) => is_exps_equal(cx, l, r),
(&ExprCast(ref lx, ref lt), &ExprCast(ref rx, ref rt)) =>
is_exp_equal(cx, lx, rx) && is_cast_ty_equal(lt, rt),
_ => false
}
}
fn is_exps_equal(cx: &LateContext, left : &[P<Expr>], right : &[P<Expr>]) -> bool {
over(left, right, |l, r| is_exp_equal(cx, l, r))
}
fn is_path_equal(left : &Path, right : &Path) -> bool {
// The == of idents doesn't work with different contexts,
// we have to be explicit about hygiene
left.global == right.global && over(&left.segments, &right.segments,
|l, r| l.identifier.name == r.identifier.name
&& l.parameters == r.parameters)
}
fn is_qself_equal(left : &QSelf, right : &QSelf) -> bool {
left.ty.node == right.ty.node && left.position == right.position
}
fn over<X, F>(left: &[X], right: &[X], mut eq_fn: F) -> bool
where F: FnMut(&X, &X) -> bool {
left.len() == right.len() && left.iter().zip(right).all(|(x, y)|
eq_fn(x, y))
}
fn both<X, F>(l: &Option<X>, r: &Option<X>, mut eq_fn : F) -> bool
where F: FnMut(&X, &X) -> bool {
l.as_ref().map_or_else(|| r.is_none(), |x| r.as_ref().map_or(false,
|y| eq_fn(x, y)))
}
fn is_cmp_or_bit(op : &BinOp) -> bool {
match op.node {
@ -99,19 +46,3 @@ fn is_cmp_or_bit(op : &BinOp) -> bool {
_ => false
}
}
fn is_cast_ty_equal(left: &Ty, right: &Ty) -> bool {
match (&left.node, &right.node) {
(&TyVec(ref lvec), &TyVec(ref rvec)) => is_cast_ty_equal(lvec, rvec),
(&TyPtr(ref lmut), &TyPtr(ref rmut)) =>
lmut.mutbl == rmut.mutbl &&
is_cast_ty_equal(&*lmut.ty, &*rmut.ty),
(&TyRptr(_, ref lrmut), &TyRptr(_, ref rrmut)) =>
lrmut.mutbl == rrmut.mutbl &&
is_cast_ty_equal(&*lrmut.ty, &*rrmut.ty),
(&TyPath(ref lq, ref lpath), &TyPath(ref rq, ref rpath)) =>
both(lq, rq, is_qself_equal) && is_path_equal(lpath, rpath),
(&TyInfer, &TyInfer) => true,
_ => false
}
}

102
src/hashmap.rs Normal file
View File

@ -0,0 +1,102 @@
use rustc::lint::*;
use rustc_front::hir::*;
use syntax::codemap::Span;
use utils::{get_item_name, is_exp_equal, match_type, snippet, span_help_and_lint, walk_ptrs_ty};
use utils::HASHMAP_PATH;
/// **What it does:** This lint checks for uses of `contains_key` + `insert` on `HashMap`.
///
/// **Why is this bad?** Using `HashMap::entry` is more efficient.
///
/// **Known problems:** Some false negatives, eg.:
/// ```
/// let k = &key;
/// if !m.contains_key(k) { m.insert(k.clone(), v); }
/// ```
///
/// **Example:**
/// ```rust
/// if !m.contains_key(&k) { m.insert(k, v) }
/// ```
/// can be rewritten as:
/// ```rust
/// m.entry(k).or_insert(v);
/// ```
declare_lint! {
pub HASHMAP_ENTRY,
Warn,
"use of `contains_key` followed by `insert` on a `HashMap`"
}
#[derive(Copy,Clone)]
pub struct HashMapLint;
impl LintPass for HashMapLint {
fn get_lints(&self) -> LintArray {
lint_array!(HASHMAP_ENTRY)
}
}
impl LateLintPass for HashMapLint {
fn check_expr(&mut self, cx: &LateContext, expr: &Expr) {
if_let_chain! {
[
let ExprIf(ref check, ref then, _) = expr.node,
let ExprUnary(UnOp::UnNot, ref check) = check.node,
let ExprMethodCall(ref name, _, ref params) = check.node,
params.len() >= 2,
name.node.as_str() == "contains_key"
], {
let key = match params[1].node {
ExprAddrOf(_, ref key) => key,
_ => return
};
let map = &params[0];
let obj_ty = walk_ptrs_ty(cx.tcx.expr_ty(map));
if match_type(cx, obj_ty, &HASHMAP_PATH) {
let sole_expr = if then.expr.is_some() { 1 } else { 0 } + then.stmts.len() == 1;
if let Some(ref then) = then.expr {
check_for_insert(cx, expr.span, map, key, then, sole_expr);
}
for stmt in &then.stmts {
if let StmtSemi(ref stmt, _) = stmt.node {
check_for_insert(cx, expr.span, map, key, stmt, sole_expr);
}
}
}
}
}
}
}
fn check_for_insert(cx: &LateContext, span: Span, map: &Expr, key: &Expr, expr: &Expr, sole_expr: bool) {
if_let_chain! {
[
let ExprMethodCall(ref name, _, ref params) = expr.node,
params.len() == 3,
name.node.as_str() == "insert",
get_item_name(cx, map) == get_item_name(cx, &*params[0]),
is_exp_equal(cx, key, &params[1])
], {
if sole_expr {
span_help_and_lint(cx, HASHMAP_ENTRY, span,
"usage of `contains_key` followed by `insert` on `HashMap`",
&format!("Consider using `{}.entry({}).or_insert({})`",
snippet(cx, map.span, ".."),
snippet(cx, params[1].span, ".."),
snippet(cx, params[2].span, "..")));
}
else {
span_help_and_lint(cx, HASHMAP_ENTRY, span,
"usage of `contains_key` followed by `insert` on `HashMap`",
&format!("Consider using `{}.entry({})`",
snippet(cx, map.span, ".."),
snippet(cx, params[1].span, "..")));
}
}
}
}

3
src/lib.rs
View File

@ -65,6 +65,7 @@ pub mod temporary_assignment;
pub mod transmute;
pub mod cyclomatic_complexity;
pub mod escape;
pub mod hashmap;
pub mod misc_early;
pub mod array_indexing;
pub mod panic;
@ -104,6 +105,7 @@ pub fn plugin_registrar(reg: &mut Registry) {
reg.register_late_lint_pass(box types::UnitCmp);
reg.register_late_lint_pass(box loops::LoopsPass);
reg.register_late_lint_pass(box lifetimes::LifetimePass);
reg.register_late_lint_pass(box hashmap::HashMapLint);
reg.register_late_lint_pass(box ranges::StepByZero);
reg.register_late_lint_pass(box types::CastPass);
reg.register_late_lint_pass(box types::TypeComplexityPass);
@ -158,6 +160,7 @@ pub fn plugin_registrar(reg: &mut Registry) {
eq_op::EQ_OP,
escape::BOXED_LOCAL,
eta_reduction::REDUNDANT_CLOSURE,
hashmap::HASHMAP_ENTRY,
identity_op::IDENTITY_OP,
len_zero::LEN_WITHOUT_IS_EMPTY,
len_zero::LEN_ZERO,

4
src/loops.rs
View File

@ -13,7 +13,7 @@ use syntax::ast::Lit_::*;
use utils::{snippet, span_lint, get_parent_expr, match_trait_method, match_type,
in_external_macro, expr_block, span_help_and_lint, is_integer_literal,
get_enclosing_block};
use utils::{VEC_PATH, LL_PATH};
use utils::{HASHMAP_PATH, VEC_PATH, LL_PATH};
/// **What it does:** This lint checks for looping over the range of `0..len` of some collection just to get the values by index. It is `Warn` by default.
///
@ -457,7 +457,7 @@ fn is_ref_iterable_type(cx: &LateContext, e: &Expr) -> bool {
is_iterable_array(ty) ||
match_type(cx, ty, &VEC_PATH) ||
match_type(cx, ty, &LL_PATH) ||
match_type(cx, ty, &["std", "collections", "hash", "map", "HashMap"]) ||
match_type(cx, ty, &HASHMAP_PATH) ||
match_type(cx, ty, &["std", "collections", "hash", "set", "HashSet"]) ||
match_type(cx, ty, &["collections", "vec_deque", "VecDeque"]) ||
match_type(cx, ty, &["collections", "binary_heap", "BinaryHeap"]) ||

3
src/strings.rs
View File

@ -7,8 +7,7 @@ use rustc::lint::*;
use rustc_front::hir::*;
use syntax::codemap::Spanned;
use eq_op::is_exp_equal;
use utils::{match_type, span_lint, walk_ptrs_ty, get_parent_expr};
use utils::{is_exp_equal, match_type, span_lint, walk_ptrs_ty, get_parent_expr};
use utils::STRING_PATH;
/// **What it does:** This lint matches code of the form `x = x + y` (without `let`!). It is `Allow` by default.

86
src/utils.rs
View File

@ -10,6 +10,7 @@ use syntax::ast::Lit_::*;
use syntax::ast;
use syntax::errors::DiagnosticBuilder;
use syntax::ptr::P;
use consts::constant;
use rustc::session::Session;
use std::str::FromStr;
@ -18,15 +19,16 @@ use std::ops::{Deref, DerefMut};
pub type MethodArgs = HirVec<P<Expr>>;
// module DefPaths for certain structs/enums we check for
pub const OPTION_PATH: [&'static str; 3] = ["core", "option", "Option"];
pub const RESULT_PATH: [&'static str; 3] = ["core", "result", "Result"];
pub const STRING_PATH: [&'static str; 3] = ["collections", "string", "String"];
pub const VEC_PATH: [&'static str; 3] = ["collections", "vec", "Vec"];
pub const LL_PATH: [&'static str; 3] = ["collections", "linked_list", "LinkedList"];
pub const OPTION_PATH: [&'static str; 3] = ["core", "option", "Option"];
pub const RESULT_PATH: [&'static str; 3] = ["core", "result", "Result"];
pub const STRING_PATH: [&'static str; 3] = ["collections", "string", "String"];
pub const VEC_PATH: [&'static str; 3] = ["collections", "vec", "Vec"];
pub const LL_PATH: [&'static str; 3] = ["collections", "linked_list", "LinkedList"];
pub const HASHMAP_PATH: [&'static str; 5] = ["std", "collections", "hash", "map", "HashMap"];
pub const OPEN_OPTIONS_PATH: [&'static str; 3] = ["std", "fs", "OpenOptions"];
pub const MUTEX_PATH: [&'static str; 4] = ["std", "sync", "mutex", "Mutex"];
pub const CLONE_PATH: [&'static str; 2] = ["Clone", "clone"];
pub const BEGIN_UNWIND:[&'static str; 3] = ["std", "rt", "begin_unwind"];
pub const MUTEX_PATH: [&'static str; 4] = ["std", "sync", "mutex", "Mutex"];
pub const CLONE_PATH: [&'static str; 2] = ["Clone", "clone"];
pub const BEGIN_UNWIND: [&'static str; 3] = ["std", "rt", "begin_unwind"];
/// Produce a nested chain of if-lets and ifs from the patterns:
///
@ -492,3 +494,71 @@ fn parse_attrs<F: FnMut(u64)>(sess: &Session, attrs: &[ast::Attribute], name: &'
}
}
}
pub fn is_exp_equal(cx: &LateContext, left : &Expr, right : &Expr) -> bool {
if let (Some(l), Some(r)) = (constant(cx, left), constant(cx, right)) {
if l == r {
return true;
}
}
match (&left.node, &right.node) {
(&ExprField(ref lfexp, ref lfident),
&ExprField(ref rfexp, ref rfident)) =>
lfident.node == rfident.node && is_exp_equal(cx, lfexp, rfexp),
(&ExprLit(ref l), &ExprLit(ref r)) => l.node == r.node,
(&ExprPath(ref lqself, ref lsubpath),
&ExprPath(ref rqself, ref rsubpath)) =>
both(lqself, rqself, is_qself_equal) &&
is_path_equal(lsubpath, rsubpath),
(&ExprTup(ref ltup), &ExprTup(ref rtup)) =>
is_exps_equal(cx, ltup, rtup),
(&ExprVec(ref l), &ExprVec(ref r)) => is_exps_equal(cx, l, r),
(&ExprCast(ref lx, ref lt), &ExprCast(ref rx, ref rt)) =>
is_exp_equal(cx, lx, rx) && is_cast_ty_equal(lt, rt),
_ => false
}
}
fn is_exps_equal(cx: &LateContext, left : &[P<Expr>], right : &[P<Expr>]) -> bool {
over(left, right, |l, r| is_exp_equal(cx, l, r))
}
fn is_path_equal(left : &Path, right : &Path) -> bool {
// The == of idents doesn't work with different contexts,
// we have to be explicit about hygiene
left.global == right.global && over(&left.segments, &right.segments,
|l, r| l.identifier.name == r.identifier.name
&& l.parameters == r.parameters)
}
fn is_qself_equal(left : &QSelf, right : &QSelf) -> bool {
left.ty.node == right.ty.node && left.position == right.position
}
fn over<X, F>(left: &[X], right: &[X], mut eq_fn: F) -> bool
where F: FnMut(&X, &X) -> bool {
left.len() == right.len() && left.iter().zip(right).all(|(x, y)|
eq_fn(x, y))
}
fn both<X, F>(l: &Option<X>, r: &Option<X>, mut eq_fn : F) -> bool
where F: FnMut(&X, &X) -> bool {
l.as_ref().map_or_else(|| r.is_none(), |x| r.as_ref().map_or(false,
|y| eq_fn(x, y)))
}
fn is_cast_ty_equal(left: &Ty, right: &Ty) -> bool {
match (&left.node, &right.node) {
(&TyVec(ref lvec), &TyVec(ref rvec)) => is_cast_ty_equal(lvec, rvec),
(&TyPtr(ref lmut), &TyPtr(ref rmut)) =>
lmut.mutbl == rmut.mutbl &&
is_cast_ty_equal(&*lmut.ty, &*rmut.ty),
(&TyRptr(_, ref lrmut), &TyRptr(_, ref rrmut)) =>
lrmut.mutbl == rrmut.mutbl &&
is_cast_ty_equal(&*lrmut.ty, &*rrmut.ty),
(&TyPath(ref lq, ref lpath), &TyPath(ref rq, ref rpath)) =>
both(lq, rq, is_qself_equal) && is_path_equal(lpath, rpath),
(&TyInfer, &TyInfer) => true,
_ => false
}
}

41
tests/compile-fail/hashmap.rs Normal file
View File

@ -0,0 +1,41 @@
#![feature(plugin)]
#![plugin(clippy)]
#![allow(unused)]
#![deny(hashmap_entry)]
use std::collections::HashMap;
use std::hash::Hash;
fn foo() {}
fn insert_if_absent0<K: Eq + Hash, V>(m: &mut HashMap<K, V>, k: K, v: V) {
if !m.contains_key(&k) { m.insert(k, v); }
//~^ERROR: usage of `contains_key` followed by `insert` on `HashMap`
//~^^HELP: Consider using `m.entry(k).or_insert(v)`
}
fn insert_if_absent1<K: Eq + Hash, V>(m: &mut HashMap<K, V>, k: K, v: V) {
if !m.contains_key(&k) { foo(); m.insert(k, v); }
//~^ERROR: usage of `contains_key` followed by `insert` on `HashMap`
//~^^HELP: Consider using `m.entry(k)`
}
fn insert_if_absent2<K: Eq + Hash, V>(m: &mut HashMap<K, V>, k: K, v: V) {
if !m.contains_key(&k) { m.insert(k, v) } else { None };
//~^ERROR: usage of `contains_key` followed by `insert` on `HashMap`
//~^^HELP: Consider using `m.entry(k).or_insert(v)`
}
fn insert_if_absent3<K: Eq + Hash, V>(m: &mut HashMap<K, V>, k: K, v: V) {
if !m.contains_key(&k) { foo(); m.insert(k, v) } else { None };
//~^ERROR: usage of `contains_key` followed by `insert` on `HashMap`
//~^^HELP: Consider using `m.entry(k)`
}
fn insert_other_if_absent<K: Eq + Hash, V>(m: &mut HashMap<K, V>, k: K, o: K, v: V) {
if !m.contains_key(&k) { m.insert(o, v); }
}
fn main() {
}