diff --git a/README.md b/README.md index be7154c8c62..66411e432c9 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_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` +shadow_unrelated | warn | The name is re-bound without even using the original value 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..33788190fd3 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_REUSE, + shadow::SHADOW_SAME, + shadow::SHADOW_UNRELATED, + ]); + 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_REUSE, + shadow::SHADOW_SAME, + shadow::SHADOW_UNRELATED, strings::STRING_ADD, strings::STRING_ADD_ASSIGN, types::BOX_VEC, diff --git a/src/returns.rs b/src/returns.rs index 301072f7912..889688cb0c7 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..2c16d9d78f4 --- /dev/null +++ b/src/shadow.rs @@ -0,0 +1,282 @@ +use std::ops::Deref; +use syntax::ast::*; +use syntax::codemap::Span; +use syntax::visit::FnKind; + +use rustc::lint::{Context, LintArray, LintPass}; +use rustc::middle::def::Def::{DefVariant, DefStruct}; + +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_UNRELATED, 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_UNRELATED) + } + + 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 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) } + if let &Some(ref o) = init { check_expr(cx, o, bindings) } + check_pat(cx, pat, init, span, bindings); + } +} + +fn is_binding(cx: &Context, pat: &Pat) -> bool { + match cx.tcx.def_map.borrow().get(&pat.id).map(|d| d.full_def()) { + Some(DefVariant(..)) | Some(DefStruct(..)) => false, + _ => true + } +} + +fn check_pat(cx: &Context, pat: &Pat, init: &Option, span: Span, + bindings: &mut Vec) where T: Deref { + //TODO: match more stuff / destructuring + match pat.node { + PatIdent(_, ref ident, ref inner) => { + let name = ident.node.name; + if is_binding(cx, pat) { + if bindings.contains(&name) { + lint_shadow(cx, name, span, pat.span, init); + } + bindings.push(name); + } + if let Some(ref p) = *inner { check_pat(cx, p, init, span, bindings); } + }, + //PatEnum(Path, Option>>), + //PatQPath(QSelf, Path), + //PatStruct(Path, Vec>, bool), + //PatTup(Vec>), + PatBox(ref inner) => { + if let Some(ref initp) = *init { + match initp.node { + ExprBox(_, ref inner_init) => + check_pat(cx, inner, &Some(&**inner_init), span, bindings), + //TODO: ExprCall on Box::new + _ => check_pat(cx, inner, init, span, bindings), + } + } else { + check_pat(cx, inner, init, span, bindings); + } + }, + //PatRegion(P, Mutability), + //PatRange(P, P), + //PatVec(Vec>, Option>, Vec>), + _ => (), + } +} + +fn lint_shadow(cx: &Context, name: Name, span: Span, lspan: Span, init: + &Option) where T: Deref { + if let &Some(ref expr) = init { + if is_self_shadow(name, expr) { + span_lint(cx, SHADOW_SAME, span, &format!( + "{} is shadowed by itself in {}", + snippet(cx, lspan, "_"), + snippet(cx, expr.span, ".."))); + } else { + if contains_self(name, expr) { + span_lint(cx, SHADOW_REUSE, span, &format!( + "{} is shadowed by {} which reuses the original value", + snippet(cx, lspan, "_"), + snippet(cx, expr.span, ".."))); + } else { + span_lint(cx, SHADOW_UNRELATED, span, &format!( + "{} is shadowed by {} in this declaration", + snippet(cx, lspan, "_"), + snippet(cx, expr.span, ".."))); + } + } + } else { + span_lint(cx, SHADOW_UNRELATED, span, &format!( + "{} is shadowed in this declaration", snippet(cx, lspan, "_"))); + } +} + +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) }, + //ExprCall + //ExprMethodCall + 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); } + }, + ExprWhile(ref cond, ref block, _) => { + check_expr(cx, cond, bindings); + check_block(cx, block, bindings); + }, + ExprMatch(ref init, ref arms, _) => { + check_expr(cx, init, bindings); + for ref arm in arms { + for ref pat in &arm.pats { + check_pat(cx, &pat, &Some(&**init), pat.span, bindings); + //TODO: This is ugly, but needed to get the right type + } + if let Some(ref guard) = arm.guard { + check_expr(cx, guard, bindings); + } + check_expr(cx, &arm.body, bindings); + } + }, + _ => () + } +} + +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_eq_name(name, path), + _ => false, + } +} + +fn path_eq_name(name: Name, path: &Path) -> bool { + !path.global && path.segments.len() == 1 && + path.segments[0].identifier.name == name +} + +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)), + ExprWhile(ref e, ref block, _) => + contains_self(name, e) || contains_block_self(name, block), + ExprMatch(ref e, ref arms, _) => + arms.iter().any(|ref arm| arm.pats.iter().any(|ref pat| + contains_pat_self(name, pat))) || contains_self(name, e), + ExprPath(_, ref path) => path_eq_name(name, path), + _ => 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 { + //TODO: We don't currently handle the case where the name + //is shadowed wiithin the block; this means code including this + //degenerate pattern will get the wrong warning. + 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 } +} + +fn contains_pat_self(name: Name, pat: &Pat) -> bool { + match pat.node { + PatIdent(_, ref ident, ref inner) => name == ident.node.name || + inner.as_ref().map_or(false, |ref p| contains_pat_self(name, p)), + PatEnum(_, ref opats) => opats.as_ref().map_or(false, + |pats| pats.iter().any(|p| contains_pat_self(name, p))), + PatQPath(_, ref path) => path_eq_name(name, path), + PatStruct(_, ref fieldpats, _) => fieldpats.iter().any( + |ref fp| contains_pat_self(name, &fp.node.pat)), + PatTup(ref ps) => ps.iter().any(|ref p| contains_pat_self(name, p)), + PatBox(ref p) | + PatRegion(ref p, _) => contains_pat_self(name, p), + PatRange(ref from, ref until) => + contains_self(name, from) || contains_self(name, until), + PatVec(ref pre, ref opt, ref post) => + pre.iter().any(|ref p| contains_pat_self(name, p)) || + opt.as_ref().map_or(false, |ref p| contains_pat_self(name, p)) || + post.iter().any(|ref p| contains_pat_self(name, p)), + _ => false, + } +} diff --git a/src/utils.rs b/src/utils.rs index 394204bedfc..c71d61f81e7 100644 --- a/src/utils.rs +++ b/src/utils.rs @@ -93,9 +93,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..a75cd0bf3f2 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_unrelated)] 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 +}