From 5225feceaa345e55bdd3b45007555f6d477faf55 Mon Sep 17 00:00:00 2001 From: llogiq Date: Fri, 21 Aug 2015 17:11:34 +0200 Subject: [PATCH] shadowing detection --- README.md | 2 +- src/lib.rs | 4 +- src/methods.rs | 3 +- src/shadow.rs | 152 +++++++++++++++++------------ tests/compile-fail/approx_const.rs | 2 +- 5 files changed, 97 insertions(+), 66 deletions(-) diff --git a/README.md b/README.md index 71ca256fb9a..66411e432c9 100644 --- a/README.md +++ b/README.md @@ -44,9 +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` +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 e65311133d2..33788190fd3 100755 --- a/src/lib.rs +++ b/src/lib.rs @@ -76,9 +76,9 @@ pub fn plugin_registrar(reg: &mut Registry) { reg.register_lint_pass(box matches::MatchPass as LintPassObject); reg.register_lint_group("shadow", vec![ - shadow::SHADOW_FOREIGN, shadow::SHADOW_REUSE, shadow::SHADOW_SAME, + shadow::SHADOW_UNRELATED, ]); reg.register_lint_group("clippy", vec![ @@ -114,9 +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, + shadow::SHADOW_UNRELATED, strings::STRING_ADD, strings::STRING_ADD_ASSIGN, types::BOX_VEC, diff --git a/src/methods.rs b/src/methods.rs index df8e35d98fb..40043be109a 100644 --- a/src/methods.rs +++ b/src/methods.rs @@ -41,7 +41,8 @@ impl LintPass for MethodsPass { if obj_ty.sty == ty::TyStr { span_lint(cx, STR_TO_STRING, expr.span, "`str.to_owned()` is faster"); } else if match_type(cx, obj_ty, &STRING_PATH) { - span_lint(cx, STRING_TO_STRING, expr.span, "`String.to_string()` is a no-op"); + span_lint(cx, STRING_TO_STRING, expr.span, "`String.to_string()` is a no-op; use \ + `clone()` to make a copy"); } } } diff --git a/src/shadow.rs b/src/shadow.rs index bae05c17d7b..bbd146f77a5 100644 --- a/src/shadow.rs +++ b/src/shadow.rs @@ -1,3 +1,4 @@ +use std::ops::Deref; use syntax::ast::*; use syntax::codemap::Span; use syntax::visit::FnKind; @@ -10,7 +11,7 @@ declare_lint!(pub SHADOW_SAME, Allow, 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, +declare_lint!(pub SHADOW_UNRELATED, Warn, "The name is re-bound without even using the original value"); #[derive(Copy, Clone)] @@ -18,7 +19,7 @@ pub struct ShadowPass; impl LintPass for ShadowPass { fn get_lints(&self) -> LintArray { - lint_array!(SHADOW_SAME, SHADOW_REUSE, SHADOW_FOREIGN) + lint_array!(SHADOW_SAME, SHADOW_REUSE, SHADOW_UNRELATED) } fn check_fn(&mut self, cx: &Context, _: FnKind, decl: &FnDecl, @@ -44,10 +45,6 @@ fn named(pat: &Pat) -> Option { } 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 { @@ -65,41 +62,53 @@ fn check_block(cx: &Context, block: &Block, bindings: &mut Vec) { 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) - } + let Local{ ref pat, ref ty, ref init, id: _, span } = **local; + if let &Some(ref t) = ty { check_ty(cx, t, bindings) } + check_pat(cx, pat, init, span, bindings); + 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_pat(cx: &Context, pat: &Pat, init: &Option, span: Span, + bindings: &mut Vec) where T: Deref { + //TODO: match more stuff / destructuring + named(pat).map(|name| { + if let &Some(ref o) = init { + if !in_external_macro(cx, o.span) { + check_expr(cx, o, bindings); + } } + if bindings.contains(&name) { + lint_shadow(cx, name, span, pat.span, init); + } + bindings.push(name); + }); +} + +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, "_"))); } } @@ -120,26 +129,21 @@ fn check_expr(cx: &Context, expr: &Expr, bindings: &mut Vec) { 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); - }, + ExprMatch(ref init, ref arms, _) => + for ref arm in arms { + for ref pat in &arm.pats { + //TODO: This is ugly, but needed to get the right type + check_pat(cx, pat, &Some(&**init), pat.span, bindings); + } + if let Some(ref guard) = arm.guard { + check_expr(cx, guard, bindings); + } + check_expr(cx, &*arm.body, bindings); + }, _ => () } } @@ -169,12 +173,15 @@ fn is_self_shadow(name: Name, expr: &Expr) -> bool { 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, + ExprPath(_, ref path) => path_eq_name(name, path), _ => false, } } +fn path_eq_name(name: Name, path: &Path) -> bool { + 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, _) | @@ -193,13 +200,11 @@ fn contains_self(name: Name, expr: &Expr) -> bool { 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, _) => + 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.segments.len() == 1 && path.segments[0].identifier.name == name, _ => false @@ -211,6 +216,9 @@ fn contains_block_self(name: Name, block: &Block) -> bool { 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; } } @@ -222,3 +230,25 @@ fn contains_block_self(name: Name, block: &Block) -> bool { } 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/tests/compile-fail/approx_const.rs b/tests/compile-fail/approx_const.rs index 4c289b474f7..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, shadow_foreign)] +#[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