diff --git a/README.md b/README.md index be7154c8c62..71ca256fb9a 100644 --- a/README.md +++ b/README.md @@ -4,7 +4,7 @@ A collection of lints that give helpful tips to newbies and catch oversights. ##Lints -There are 45 lints included in this crate: +There are 48 lints included in this crate: name | default | meaning -------------------------|---------|----------------------------------------------------------------------------------------------------------------------------------------------------------------------------- @@ -44,6 +44,9 @@ ptr_arg | allow | fn arguments of the type `&Vec<...>` or `&S range_step_by_zero | warn | using Range::step_by(0), which produces an infinite iterator redundant_closure | warn | using redundant closures, i.e. `|a| foo(a)` (which can be written as just `foo`) result_unwrap_used | allow | using `Result.unwrap()`, which might be better handled +shadow_foreign | warn | The name is re-bound without even using the original value +shadow_reuse | allow | rebinding a name to an expression that re-uses the original value, e.g. `let x = x + 1` +shadow_same | allow | rebinding a name to itself, e.g. `let mut x = &mut x` single_match | warn | a match statement with a single nontrivial arm (i.e, where the other arm is `_ => {}`) is used; recommends `if let` instead str_to_string | warn | using `to_string()` on a str, which should be `to_owned()` string_add | allow | using `x + ..` where x is a `String`; suggests using `push_str()` instead diff --git a/src/lib.rs b/src/lib.rs index 863ba2624dd..e65311133d2 100755 --- a/src/lib.rs +++ b/src/lib.rs @@ -32,6 +32,7 @@ pub mod len_zero; pub mod attrs; pub mod collapsible_if; pub mod unicode; +pub mod shadow; pub mod strings; pub mod methods; pub mod returns; @@ -64,6 +65,7 @@ pub fn plugin_registrar(reg: &mut Registry) { reg.register_lint_pass(box strings::StringAdd as LintPassObject); reg.register_lint_pass(box returns::ReturnPass as LintPassObject); reg.register_lint_pass(box methods::MethodsPass as LintPassObject); + reg.register_lint_pass(box shadow::ShadowPass as LintPassObject); reg.register_lint_pass(box types::LetPass as LintPassObject); reg.register_lint_pass(box types::UnitCmp as LintPassObject); reg.register_lint_pass(box loops::LoopsPass as LintPassObject); @@ -73,6 +75,12 @@ pub fn plugin_registrar(reg: &mut Registry) { reg.register_lint_pass(box types::TypeComplexityPass as LintPassObject); reg.register_lint_pass(box matches::MatchPass as LintPassObject); + reg.register_lint_group("shadow", vec![ + shadow::SHADOW_FOREIGN, + shadow::SHADOW_REUSE, + shadow::SHADOW_SAME, + ]); + reg.register_lint_group("clippy", vec![ approx_const::APPROX_CONSTANT, attrs::INLINE_ALWAYS, @@ -106,6 +114,9 @@ pub fn plugin_registrar(reg: &mut Registry) { ranges::RANGE_STEP_BY_ZERO, returns::LET_AND_RETURN, returns::NEEDLESS_RETURN, + shadow::SHADOW_FOREIGN, + shadow::SHADOW_REUSE, + shadow::SHADOW_SAME, strings::STRING_ADD, strings::STRING_ADD_ASSIGN, types::BOX_VEC, diff --git a/src/returns.rs b/src/returns.rs index df0b93f301e..a5779984334 100644 --- a/src/returns.rs +++ b/src/returns.rs @@ -11,7 +11,7 @@ declare_lint!(pub LET_AND_RETURN, Warn, "creating a let-binding and then immediately returning it like `let x = expr; x` at \ the end of a function"); -#[derive(Copy,Clone)] +#[derive(Copy, Clone)] pub struct ReturnPass; impl ReturnPass { diff --git a/src/shadow.rs b/src/shadow.rs new file mode 100644 index 00000000000..bae05c17d7b --- /dev/null +++ b/src/shadow.rs @@ -0,0 +1,224 @@ +use syntax::ast::*; +use syntax::codemap::Span; +use syntax::visit::FnKind; + +use rustc::lint::{Context, LintArray, LintPass}; +use utils::{in_external_macro, snippet, span_lint}; + +declare_lint!(pub SHADOW_SAME, Allow, + "rebinding a name to itself, e.g. `let mut x = &mut x`"); +declare_lint!(pub SHADOW_REUSE, Allow, + "rebinding a name to an expression that re-uses the original value, e.g. \ + `let x = x + 1`"); +declare_lint!(pub SHADOW_FOREIGN, Warn, + "The name is re-bound without even using the original value"); + +#[derive(Copy, Clone)] +pub struct ShadowPass; + +impl LintPass for ShadowPass { + fn get_lints(&self) -> LintArray { + lint_array!(SHADOW_SAME, SHADOW_REUSE, SHADOW_FOREIGN) + } + + fn check_fn(&mut self, cx: &Context, _: FnKind, decl: &FnDecl, + block: &Block, _: Span, _: NodeId) { + if in_external_macro(cx, block.span) { return; } + check_fn(cx, decl, block); + } +} + +fn check_fn(cx: &Context, decl: &FnDecl, block: &Block) { + let mut bindings = Vec::new(); + for arg in &decl.inputs { + if let PatIdent(_, ident, _) = arg.pat.node { + bindings.push(ident.node.name) + } + } + check_block(cx, block, &mut bindings); +} + +fn named(pat: &Pat) -> Option { + if let PatIdent(_, ident, _) = pat.node { + Some(ident.node.name) + } else { None } +} + +fn add(bindings: &mut Vec, pat: &Pat) { + named(pat).map(|name| bindings.push(name)); +} + +fn check_block(cx: &Context, block: &Block, bindings: &mut Vec) { + let len = bindings.len(); + for stmt in &block.stmts { + match stmt.node { + StmtDecl(ref decl, _) => check_decl(cx, decl, bindings), + StmtExpr(ref e, _) | StmtSemi(ref e, _) => + check_expr(cx, e, bindings), + _ => () + } + } + if let Some(ref o) = block.expr { check_expr(cx, o, bindings); } + bindings.truncate(len); +} + +fn check_decl(cx: &Context, decl: &Decl, bindings: &mut Vec) { + if in_external_macro(cx, decl.span) { return; } + if let DeclLocal(ref local) = decl.node { + let Local{ ref pat, ref ty, ref init, id: _, span: _ } = **local; + if let &Some(ref t) = ty { check_ty(cx, t, bindings); } + named(pat).map(|name| if bindings.contains(&name) { + if let &Some(ref o) = init { + if in_external_macro(cx, o.span) { return; } + check_expr(cx, o, bindings); + bindings.push(name); + lint_shadow(cx, name, decl.span, pat.span, o); + } + }); + add(bindings, pat); + if let &Some(ref o) = init { + check_expr(cx, o, bindings) + } + } +} + +fn lint_shadow(cx: &Context, name: Name, span: Span, lspan: Span, init: &Expr) { + if is_self_shadow(name, init) { + span_lint(cx, SHADOW_SAME, span, &format!( + "{} is shadowed by itself in {}", + snippet(cx, lspan, "_"), + snippet(cx, init.span, ".."))); + } else { + if contains_self(name, init) { + span_lint(cx, SHADOW_REUSE, span, &format!( + "{} is shadowed by {} which reuses the original value", + snippet(cx, lspan, "_"), + snippet(cx, init.span, ".."))); + } else { + span_lint(cx, SHADOW_FOREIGN, span, &format!( + "{} is shadowed by {} in this declaration", + snippet(cx, lspan, "_"), + snippet(cx, init.span, ".."))); + } + } +} + +fn check_expr(cx: &Context, expr: &Expr, bindings: &mut Vec) { + if in_external_macro(cx, expr.span) { return; } + match expr.node { + ExprUnary(_, ref e) | ExprParen(ref e) | ExprField(ref e, _) | + ExprTupField(ref e, _) | ExprAddrOf(_, ref e) | ExprBox(None, ref e) + => { check_expr(cx, e, bindings) }, + ExprBox(Some(ref place), ref e) => { + check_expr(cx, place, bindings); check_expr(cx, e, bindings) } + ExprBlock(ref block) | ExprLoop(ref block, _) => + { check_block(cx, block, bindings) }, + ExprVec(ref v) | ExprTup(ref v) => + for ref e in v { check_expr(cx, e, bindings) }, + ExprIf(ref cond, ref then, ref otherwise) => { + check_expr(cx, cond, bindings); + check_block(cx, then, bindings); + if let &Some(ref o) = otherwise { check_expr(cx, o, bindings); } + }, + ExprIfLet(ref pat, ref e, ref block, ref otherwise) => { + check_expr(cx, e, bindings); + let len = bindings.len(); + add(bindings, pat); + check_block(cx, block, bindings); + if let &Some(ref o) = otherwise { check_expr(cx, o, bindings); } + bindings.truncate(len); + }, + ExprWhile(ref cond, ref block, _) => { + check_expr(cx, cond, bindings); + check_block(cx, block, bindings); + }, + ExprWhileLet(ref pat, ref e, ref block, _) | + ExprForLoop(ref pat, ref e, ref block, _) => { + check_expr(cx, e, bindings); + let len = bindings.len(); + add(bindings, pat); + check_block(cx, block, bindings); + bindings.truncate(len); + }, + _ => () + } +} + +fn check_ty(cx: &Context, ty: &Ty, bindings: &mut Vec) { + match ty.node { + TyParen(ref sty) | TyObjectSum(ref sty, _) | + TyVec(ref sty) => check_ty(cx, sty, bindings), + TyFixedLengthVec(ref fty, ref expr) => { + check_ty(cx, fty, bindings); + check_expr(cx, expr, bindings); + }, + TyPtr(MutTy{ ty: ref mty, .. }) | + TyRptr(_, MutTy{ ty: ref mty, .. }) => check_ty(cx, mty, bindings), + TyTup(ref tup) => { for ref t in tup { check_ty(cx, t, bindings) } }, + TyTypeof(ref expr) => check_expr(cx, expr, bindings), + _ => (), + } +} + +fn is_self_shadow(name: Name, expr: &Expr) -> bool { + match expr.node { + ExprBox(_, ref inner) | + ExprParen(ref inner) | + ExprAddrOf(_, ref inner) => is_self_shadow(name, inner), + ExprBlock(ref block) => block.stmts.is_empty() && block.expr.as_ref(). + map_or(false, |ref e| is_self_shadow(name, e)), + ExprUnary(op, ref inner) => (UnUniq == op || UnDeref == op) && + is_self_shadow(name, inner), + ExprPath(_, ref path) => path.segments.len() == 1 && + path.segments[0].identifier.name == name, + _ => false, + } +} + +fn contains_self(name: Name, expr: &Expr) -> bool { + match expr.node { + ExprUnary(_, ref e) | ExprParen(ref e) | ExprField(ref e, _) | + ExprTupField(ref e, _) | ExprAddrOf(_, ref e) | ExprBox(_, ref e) + => contains_self(name, e), + ExprBinary(_, ref l, ref r) => + contains_self(name, l) || contains_self(name, r), + ExprBlock(ref block) | ExprLoop(ref block, _) => + contains_block_self(name, block), + ExprCall(ref fun, ref args) => contains_self(name, fun) || + args.iter().any(|ref a| contains_self(name, a)), + ExprMethodCall(_, _, ref args) => + args.iter().any(|ref a| contains_self(name, a)), + ExprVec(ref v) | ExprTup(ref v) => + v.iter().any(|ref e| contains_self(name, e)), + ExprIf(ref cond, ref then, ref otherwise) => + contains_self(name, cond) || contains_block_self(name, then) || + otherwise.as_ref().map_or(false, |ref e| contains_self(name, e)), + ExprIfLet(_, ref e, ref block, ref otherwise) => + contains_self(name, e) || contains_block_self(name, block) || + otherwise.as_ref().map_or(false, |ref o| contains_self(name, o)), + ExprWhile(ref e, ref block, _) | + ExprWhileLet(_, ref e, ref block, _) | + ExprForLoop(_, ref e, ref block, _) => + contains_self(name, e) || contains_block_self(name, block), + ExprPath(_, ref path) => path.segments.len() == 1 && + path.segments[0].identifier.name == name, + _ => false + } +} + +fn contains_block_self(name: Name, block: &Block) -> bool { + for stmt in &block.stmts { + match stmt.node { + StmtDecl(ref decl, _) => + if let DeclLocal(ref local) = decl.node { + if let Some(ref init) = local.init { + if contains_self(name, init) { return true; } + } + }, + StmtExpr(ref e, _) | StmtSemi(ref e, _) => + if contains_self(name, e) { return true }, + _ => () + } + } + if let Some(ref e) = block.expr { contains_self(name, e) } else { false } +} diff --git a/src/utils.rs b/src/utils.rs index 5e7c63e85d9..6cb21148356 100644 --- a/src/utils.rs +++ b/src/utils.rs @@ -94,9 +94,9 @@ pub fn snippet_block<'a>(cx: &Context, span: Span, default: &'a str) -> Cow<'a, /// Trim indentation from a multiline string /// with possibility of ignoring the first line pub fn trim_multiline(s: Cow, ignore_first: bool) -> Cow { - let s = trim_multiline_inner(s, ignore_first, ' '); - let s = trim_multiline_inner(s, ignore_first, '\t'); - trim_multiline_inner(s, ignore_first, ' ') + let s_space = trim_multiline_inner(s, ignore_first, ' '); + let s_tab = trim_multiline_inner(s_space, ignore_first, '\t'); + trim_multiline_inner(s_tab, ignore_first, ' ') } fn trim_multiline_inner(s: Cow, ignore_first: bool, ch: char) -> Cow { diff --git a/tests/compile-fail/approx_const.rs b/tests/compile-fail/approx_const.rs index 799795becbd..4c289b474f7 100755 --- a/tests/compile-fail/approx_const.rs +++ b/tests/compile-fail/approx_const.rs @@ -2,7 +2,7 @@ #![plugin(clippy)] #[deny(approx_constant)] -#[allow(unused)] +#[allow(unused, shadow_foreign)] fn main() { let my_e = 2.7182; //~ERROR approximate value of `f{32, 64}::E` found let almost_e = 2.718; //~ERROR approximate value of `f{32, 64}::E` found diff --git a/tests/compile-fail/shadow.rs b/tests/compile-fail/shadow.rs new file mode 100644 index 00000000000..e3213717213 --- /dev/null +++ b/tests/compile-fail/shadow.rs @@ -0,0 +1,22 @@ +#![feature(plugin)] +#![plugin(clippy)] + +#![allow(unused_parens, unused_variables)] +#![deny(shadow)] + +fn id(x: T) -> T { x } + +fn first(x: (isize, isize)) -> isize { x.0 } + +fn main() { + let mut x = 1; + let x = &mut x; //~ERROR: x is shadowed by itself in &mut x + let x = { x }; //~ERROR: x is shadowed by itself in { x } + let x = (&*x); //~ERROR: x is shadowed by itself in (&*x) + let x = { *x + 1 }; //~ERROR: x is shadowed by { *x + 1 } which reuses + let x = id(x); //~ERROR: x is shadowed by id(x) which reuses + let x = (1, x); //~ERROR: x is shadowed by (1, x) which reuses + let x = first(x); //~ERROR: x is shadowed by first(x) which reuses + let y = 1; + let x = y; //~ERROR: x is shadowed by y in this declaration +}