From 209e6981a3ec67ddc8d94cb46d876550948f6238 Mon Sep 17 00:00:00 2001 From: llogiq Date: Fri, 21 Aug 2015 17:11:34 +0200 Subject: [PATCH 1/7] shadowing detection --- README.md | 5 +- src/lib.rs | 11 ++ src/returns.rs | 2 +- src/shadow.rs | 224 +++++++++++++++++++++++++++++ src/utils.rs | 6 +- tests/compile-fail/approx_const.rs | 2 +- tests/compile-fail/shadow.rs | 22 +++ 7 files changed, 266 insertions(+), 6 deletions(-) create mode 100644 src/shadow.rs create mode 100644 tests/compile-fail/shadow.rs diff --git a/README.md b/README.md index be7154c8c629..71ca256fb9a4 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 863ba2624ddd..e65311133d29 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 df0b93f301ea..a5779984334c 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 000000000000..bae05c17d7bc --- /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 5e7c63e85d9b..6cb211483569 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 799795becbd6..4c289b474f72 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 000000000000..e32137172131 --- /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 +} From 5225feceaa345e55bdd3b45007555f6d477faf55 Mon Sep 17 00:00:00 2001 From: llogiq Date: Fri, 21 Aug 2015 17:11:34 +0200 Subject: [PATCH 2/7] 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 71ca256fb9a4..66411e432c91 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 e65311133d29..33788190fd3f 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 df8e35d98fbc..40043be109a6 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 bae05c17d7bc..bbd146f77a5a 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 4c289b474f72..a75cd0bf3f26 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 From 88047a0953d8032db9021a38e60d5c088d8318b0 Mon Sep 17 00:00:00 2001 From: Georg Brandl Date: Tue, 25 Aug 2015 13:26:20 +0200 Subject: [PATCH 3/7] collapsible_if: remove extraneous note output This was probably a debug addition. --- src/collapsible_if.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/src/collapsible_if.rs b/src/collapsible_if.rs index 0b6dfc19e6b5..7d654b43f2f1 100644 --- a/src/collapsible_if.rs +++ b/src/collapsible_if.rs @@ -48,7 +48,6 @@ fn check_expr_expd(cx: &Context, e: &Expr, info: Option<&ExpnInfo>) { if e.span.expn_id != sp.expn_id { return; } - cx.sess().note(&format!("{:?} -- {:?}", e.span, sp)); span_help_and_lint(cx, COLLAPSIBLE_IF, e.span, "this if statement can be collapsed", &format!("try\nif {} && {} {}", From b13d318f48f181292e8efbdd4fe2d0353e1f51d2 Mon Sep 17 00:00:00 2001 From: Georg Brandl Date: Tue, 25 Aug 2015 14:41:35 +0200 Subject: [PATCH 4/7] all: remove unneeded deref and/or ref operations --- src/attrs.rs | 4 ++-- src/collapsible_if.rs | 2 +- src/consts.rs | 6 +++--- src/eta_reduction.rs | 2 +- src/len_zero.rs | 2 +- src/lifetimes.rs | 6 +++--- src/loops.rs | 6 +++--- src/matches.rs | 10 +++++----- src/methods.rs | 2 +- src/misc.rs | 2 +- src/needless_bool.rs | 4 ++-- src/ptr_arg.rs | 2 +- src/returns.rs | 4 ++-- src/strings.rs | 4 ++-- src/types.rs | 10 +++++----- src/utils.rs | 4 ---- 16 files changed, 33 insertions(+), 37 deletions(-) diff --git a/src/attrs.rs b/src/attrs.rs index 3e451ac5edaf..ad021f28a4d6 100644 --- a/src/attrs.rs +++ b/src/attrs.rs @@ -71,14 +71,14 @@ fn is_relevant_block(block: &Block) -> bool { _ => () } } - block.expr.as_ref().map_or(false, |e| is_relevant_expr(&*e)) + block.expr.as_ref().map_or(false, |e| is_relevant_expr(e)) } fn is_relevant_expr(expr: &Expr) -> bool { match expr.node { ExprBlock(ref block) => is_relevant_block(block), ExprRet(Some(ref e)) | ExprParen(ref e) => - is_relevant_expr(&*e), + is_relevant_expr(e), ExprRet(None) | ExprBreak(_) | ExprMac(_) => false, ExprCall(ref path_expr, _) => { if let ExprPath(_, ref path) = path_expr.node { diff --git a/src/collapsible_if.rs b/src/collapsible_if.rs index 7d654b43f2f1..e0b25b7283bf 100644 --- a/src/collapsible_if.rs +++ b/src/collapsible_if.rs @@ -79,7 +79,7 @@ fn single_stmt_of_block(block: &Block) -> Option<&Expr> { } else { None } } else { if block.stmts.is_empty() { - if let Some(ref p) = block.expr { Some(&*p) } else { None } + if let Some(ref p) = block.expr { Some(p) } else { None } } else { None } } } diff --git a/src/consts.rs b/src/consts.rs index e54ac77b599f..1a828317fc2d 100644 --- a/src/consts.rs +++ b/src/consts.rs @@ -222,7 +222,7 @@ fn neg_float_str(s: String) -> String { if s.starts_with('-') { s[1..].to_owned() } else { - format!("-{}", &*s) + format!("-{}", s) } } @@ -299,7 +299,7 @@ impl<'c, 'cc> ConstEvalContext<'c, 'cc> { ExprPath(_, _) => self.fetch_path(e), ExprBlock(ref block) => self.block(block), ExprIf(ref cond, ref then, ref otherwise) => - self.ifthenelse(&*cond, &*then, &*otherwise), + self.ifthenelse(cond, then, otherwise), ExprLit(ref lit) => Some(lit_to_constant(&lit.node)), ExprVec(ref vec) => self.multi(vec).map(ConstantVec), ExprTup(ref tup) => self.multi(tup).map(ConstantTuple), @@ -362,7 +362,7 @@ impl<'c, 'cc> ConstEvalContext<'c, 'cc> { if b { self.block(then) } else { - otherwise.as_ref().and_then(|ref expr| self.expr(expr)) + otherwise.as_ref().and_then(|expr| self.expr(expr)) } } else { None } } diff --git a/src/eta_reduction.rs b/src/eta_reduction.rs index 25e967b07e54..481512abc627 100644 --- a/src/eta_reduction.rs +++ b/src/eta_reduction.rs @@ -22,7 +22,7 @@ impl LintPass for EtaPass { ExprCall(_, ref args) | ExprMethodCall(_, _, ref args) => { for arg in args { - check_closure(cx, &*arg) + check_closure(cx, arg) } }, _ => (), diff --git a/src/len_zero.rs b/src/len_zero.rs index 5eaa0256402d..ca3ce51bf7cc 100644 --- a/src/len_zero.rs +++ b/src/len_zero.rs @@ -102,7 +102,7 @@ fn check_len_zero(cx: &Context, span: Span, method: &SpannedIdent, args: &[P], lit: &Lit, op: &str) { if let Spanned{node: LitInt(0, _), ..} = *lit { if method.node.name == "len" && args.len() == 1 && - has_is_empty(cx, &*args[0]) { + has_is_empty(cx, &args[0]) { span_lint(cx, LEN_ZERO, span, &format!( "consider replacing the len comparison with `{}{}.is_empty()`", op, snippet(cx, args[0].span, "_"))) diff --git a/src/lifetimes.rs b/src/lifetimes.rs index 9d07df4a3ed2..660d68535bd5 100644 --- a/src/lifetimes.rs +++ b/src/lifetimes.rs @@ -26,14 +26,14 @@ impl LintPass for LifetimePass { fn check_impl_item(&mut self, cx: &Context, item: &ImplItem) { if let MethodImplItem(ref sig, _) = item.node { - check_fn_inner(cx, &*sig.decl, Some(&sig.explicit_self), + check_fn_inner(cx, &sig.decl, Some(&sig.explicit_self), &sig.generics.lifetimes, item.span); } } fn check_trait_item(&mut self, cx: &Context, item: &TraitItem) { if let MethodTraitItem(ref sig, _) = item.node { - check_fn_inner(cx, &*sig.decl, Some(&sig.explicit_self), + check_fn_inner(cx, &sig.decl, Some(&sig.explicit_self), &sig.generics.lifetimes, item.span); } } @@ -92,7 +92,7 @@ fn could_use_elision(func: &FnDecl, slf: Option<&ExplicitSelf>, } // extract lifetimes in input argument types for arg in &func.inputs { - walk_ty(&mut input_visitor, &*arg.ty); + walk_ty(&mut input_visitor, &arg.ty); } // extract lifetimes in output type if let Return(ref ty) = func.output { diff --git a/src/loops.rs b/src/loops.rs index 5f18439eafe4..ca8d3990fc5d 100644 --- a/src/loops.rs +++ b/src/loops.rs @@ -95,9 +95,9 @@ fn recover_for_loop(expr: &Expr) -> Option<(&Pat, &Expr, &Expr)> { let PatEnum(_, Some(ref somepats)) = innerarms[0].pats[0].node, somepats.len() == 1 ], { - return Some((&*somepats[0], - &*iterargs[0], - &*innerarms[0].body)); + return Some((&somepats[0], + &iterargs[0], + &innerarms[0].body)); } } None diff --git a/src/matches.rs b/src/matches.rs index 002da07f50b7..d1c74daf2cd1 100644 --- a/src/matches.rs +++ b/src/matches.rs @@ -34,7 +34,7 @@ impl LintPass for MatchPass { // when an enum is extended, so we don't consider these cases arms[1].pats[0].node == PatWild(PatWildSingle) && // finally, we don't want any content in the second arm (unit or empty block) - is_unit_expr(&*arms[1].body) + is_unit_expr(&arms[1].body) { let body_code = snippet_block(cx, arms[0].body.span, ".."); let body_code = if let ExprBlock(_) = arms[0].body.node { @@ -46,10 +46,10 @@ impl LintPass for MatchPass { "you seem to be trying to use match for \ destructuring a single pattern. Did you mean to \ use `if let`?", - &*format!("try\nif let {} = {} {}", - snippet(cx, arms[0].pats[0].span, ".."), - snippet(cx, ex.span, ".."), - body_code) + &format!("try\nif let {} = {} {}", + snippet(cx, arms[0].pats[0].span, ".."), + snippet(cx, ex.span, ".."), + body_code) ); } diff --git a/src/methods.rs b/src/methods.rs index 40043be109a6..07693e11d996 100644 --- a/src/methods.rs +++ b/src/methods.rs @@ -24,7 +24,7 @@ impl LintPass for MethodsPass { fn check_expr(&mut self, cx: &Context, expr: &Expr) { if let ExprMethodCall(ref ident, _, ref args) = expr.node { - let obj_ty = walk_ptrs_ty(cx.tcx.expr_ty(&*args[0])); + let obj_ty = walk_ptrs_ty(cx.tcx.expr_ty(&args[0])); if ident.node.name == "unwrap" { if match_type(cx, obj_ty, &OPTION_PATH) { span_lint(cx, OPTION_UNWRAP_USED, expr.span, diff --git a/src/misc.rs b/src/misc.rs index 81b03db5e143..2290af38bb57 100644 --- a/src/misc.rs +++ b/src/misc.rs @@ -203,7 +203,7 @@ fn check_to_owned(cx: &Context, expr: &Expr, other_span: Span) { fn is_str_arg(cx: &Context, args: &[P]) -> bool { args.len() == 1 && if let ty::TyStr = - walk_ptrs_ty(cx.tcx.expr_ty(&*args[0])).sty { true } else { false } + walk_ptrs_ty(cx.tcx.expr_ty(&args[0])).sty { true } else { false } } declare_lint!(pub MODULO_ONE, Warn, "taking a number modulo 1, which always returns 0"); diff --git a/src/needless_bool.rs b/src/needless_bool.rs index 7671d63a35d4..0fe52c44189c 100644 --- a/src/needless_bool.rs +++ b/src/needless_bool.rs @@ -5,7 +5,7 @@ use rustc::lint::*; use syntax::ast::*; -use utils::{de_p, span_lint, snippet}; +use utils::{span_lint, snippet}; declare_lint! { pub NEEDLESS_BOOL, @@ -55,7 +55,7 @@ impl LintPass for NeedlessBool { fn fetch_bool_block(block: &Block) -> Option { if block.stmts.is_empty() { - block.expr.as_ref().map(de_p).and_then(fetch_bool_expr) + block.expr.as_ref().and_then(|e| fetch_bool_expr(e)) } else { None } } diff --git a/src/ptr_arg.rs b/src/ptr_arg.rs index 2d09fcbcca95..bcbd8dad68a9 100644 --- a/src/ptr_arg.rs +++ b/src/ptr_arg.rs @@ -45,7 +45,7 @@ impl LintPass for PtrArg { fn check_fn(cx: &Context, decl: &FnDecl) { for arg in &decl.inputs { - if let Some(pat_ty) = cx.tcx.pat_ty_opt(&*arg.pat) { + if let Some(pat_ty) = cx.tcx.pat_ty_opt(&arg.pat) { if let ty::TyRef(_, ty::TypeAndMut { ty, mutbl: MutImmutable }) = pat_ty.sty { if match_type(cx, ty, &VEC_PATH) { span_lint(cx, PTR_ARG, arg.ty.span, diff --git a/src/returns.rs b/src/returns.rs index a5779984334c..889688cb0c70 100644 --- a/src/returns.rs +++ b/src/returns.rs @@ -50,7 +50,7 @@ impl ReturnPass { // a match expr, check all arms ExprMatch(_, ref arms, _) => { for arm in arms { - self.check_final_expr(cx, &*arm.body); + self.check_final_expr(cx, &arm.body); } } _ => { } @@ -76,7 +76,7 @@ impl ReturnPass { let PatIdent(_, Spanned { node: id, .. }, _) = local.pat.node, let Some(ref retexpr) = block.expr, let ExprPath(_, ref path) = retexpr.node, - match_path(path, &[&*id.name.as_str()]) + match_path(path, &[&id.name.as_str()]) ], { self.emit_let_lint(cx, retexpr.span, initexpr.span); } diff --git a/src/strings.rs b/src/strings.rs index b24ea345244a..d03f4d53c606 100644 --- a/src/strings.rs +++ b/src/strings.rs @@ -70,8 +70,8 @@ fn is_add(cx: &Context, src: &Expr, target: &Expr) -> bool { is_exp_equal(cx, target, left), ExprBlock(ref block) => block.stmts.is_empty() && block.expr.as_ref().map_or(false, - |expr| is_add(cx, &*expr, target)), - ExprParen(ref expr) => is_add(cx, &*expr, target), + |expr| is_add(cx, expr, target)), + ExprParen(ref expr) => is_add(cx, expr, target), _ => false } } diff --git a/src/types.rs b/src/types.rs index 4e9dd133ac8e..7479a65b6ee5 100644 --- a/src/types.rs +++ b/src/types.rs @@ -55,7 +55,7 @@ declare_lint!(pub LET_UNIT_VALUE, Warn, fn check_let_unit(cx: &Context, decl: &Decl, info: Option<&ExpnInfo>) { if in_macro(cx, info) { return; } if let DeclLocal(ref local) = decl.node { - let bindtype = &cx.tcx.pat_ty(&*local.pat).sty; + let bindtype = &cx.tcx.pat_ty(&local.pat).sty; if *bindtype == ty::TyTuple(vec![]) { span_lint(cx, LET_UNIT_VALUE, decl.span, &format!( "this let-binding has unit value. Consider omitting `let {} =`", @@ -210,7 +210,7 @@ impl LintPass for CastPass { fn check_expr(&mut self, cx: &Context, expr: &Expr) { if let ExprCast(ref ex, _) = expr.node { - let (cast_from, cast_to) = (cx.tcx.expr_ty(&*ex), cx.tcx.expr_ty(expr)); + let (cast_from, cast_to) = (cx.tcx.expr_ty(ex), cx.tcx.expr_ty(expr)); if cast_from.is_numeric() && cast_to.is_numeric() && !in_external_macro(cx, expr.span) { match (cast_from.is_integral(), cast_to.is_integral()) { (true, false) => { @@ -263,14 +263,14 @@ impl LintPass for TypeComplexityPass { } fn check_struct_field(&mut self, cx: &Context, field: &StructField) { - check_type(cx, &*field.node.ty); + check_type(cx, &field.node.ty); } fn check_variant(&mut self, cx: &Context, var: &Variant, _: &Generics) { // StructVariant is covered by check_struct_field if let TupleVariantKind(ref args) = var.node.kind { for arg in args { - check_type(cx, &*arg.ty); + check_type(cx, &arg.ty); } } } @@ -312,7 +312,7 @@ impl LintPass for TypeComplexityPass { fn check_fndecl(cx: &Context, decl: &FnDecl) { for arg in &decl.inputs { - check_type(cx, &*arg.ty); + check_type(cx, &arg.ty); } if let Return(ref ty) = decl.output { check_type(cx, ty); diff --git a/src/utils.rs b/src/utils.rs index 6cb211483569..c71d61f81e71 100644 --- a/src/utils.rs +++ b/src/utils.rs @@ -1,7 +1,6 @@ use rustc::lint::*; use syntax::ast::*; use syntax::codemap::{ExpnInfo, Span}; -use syntax::ptr::P; use rustc::ast_map::Node::NodeExpr; use rustc::middle::ty; use std::borrow::Cow; @@ -130,9 +129,6 @@ pub fn get_parent_expr<'c>(cx: &'c Context, e: &Expr) -> Option<&'c Expr> { if let NodeExpr(parent) = node { Some(parent) } else { None } ) } -/// dereference a P and return a ref on the result -pub fn de_p(p: &P) -> &T { &*p } - #[cfg(not(feature="structured_logging"))] pub fn span_lint(cx: &Context, lint: &'static Lint, sp: Span, msg: &str) { cx.span_lint(lint, sp, msg); From bd22521af2313bc3e68b228077373a51bc3dda23 Mon Sep 17 00:00:00 2001 From: llogiq Date: Fri, 21 Aug 2015 17:11:34 +0200 Subject: [PATCH 5/7] shadowing detection --- src/shadow.rs | 67 +++++++++++++++++++++++++++++++++------------------ 1 file changed, 43 insertions(+), 24 deletions(-) diff --git a/src/shadow.rs b/src/shadow.rs index bbd146f77a5a..1c09bffd9e62 100644 --- a/src/shadow.rs +++ b/src/shadow.rs @@ -39,12 +39,6 @@ fn check_fn(cx: &Context, decl: &FnDecl, block: &Block) { check_block(cx, block, &mut bindings); } -fn named(pat: &Pat) -> Option { - if let PatIdent(_, ident, _) = pat.node { - Some(ident.node.name) - } else { None } -} - fn check_block(cx: &Context, block: &Block, bindings: &mut Vec) { let len = bindings.len(); for stmt in &block.stmts { @@ -64,25 +58,46 @@ fn check_decl(cx: &Context, decl: &Decl, bindings: &mut Vec) { 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) } - check_pat(cx, pat, init, span, bindings); if let &Some(ref o) = init { check_expr(cx, o, bindings) } + check_pat(cx, pat, init, span, bindings); } } 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); + match pat.node { + PatIdent(_, ref ident, ref inner) => { + let name = ident.node.name; + if pat_is_binding(&cx.tcx.def_map, pat) { + if bindings.contains(&name) { + lint_shadow(cx, name, span, pat.span, init); + } + bindings.push(name); } - } - 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: @@ -122,6 +137,8 @@ fn check_expr(cx: &Context, expr: &Expr, bindings: &mut Vec) { 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) => { @@ -133,17 +150,19 @@ fn check_expr(cx: &Context, expr: &Expr, bindings: &mut Vec) { check_expr(cx, cond, bindings); check_block(cx, block, bindings); }, - ExprMatch(ref init, ref arms, _) => + 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 - 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); - }, + check_expr(cx, &arm.body, bindings); + } + }, _ => () } } @@ -179,7 +198,8 @@ fn is_self_shadow(name: Name, expr: &Expr) -> bool { } fn path_eq_name(name: Name, path: &Path) -> bool { - path.segments.len() == 1 && path.segments[0].identifier.name == name + !path.global && path.segments.len() == 1 && + path.segments[0].identifier.name == name } fn contains_self(name: Name, expr: &Expr) -> bool { @@ -205,8 +225,7 @@ fn contains_self(name: Name, expr: &Expr) -> bool { 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, + ExprPath(_, ref path) => path_eq_name(name, path), _ => false } } From 974ceefc1e18221b1c01961a34d8866750d7690c Mon Sep 17 00:00:00 2001 From: llogiq Date: Fri, 21 Aug 2015 17:11:34 +0200 Subject: [PATCH 6/7] shadowing detection --- src/shadow.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/shadow.rs b/src/shadow.rs index 1c09bffd9e62..c4f636ae1123 100644 --- a/src/shadow.rs +++ b/src/shadow.rs @@ -4,6 +4,8 @@ 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, From 9012d8f1975829a1440fde6f6fd6158a239595c4 Mon Sep 17 00:00:00 2001 From: llogiq Date: Tue, 25 Aug 2015 20:11:03 +0200 Subject: [PATCH 7/7] fixed false positives on structs/enum variants --- src/shadow.rs | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/src/shadow.rs b/src/shadow.rs index c4f636ae1123..2c16d9d78f42 100644 --- a/src/shadow.rs +++ b/src/shadow.rs @@ -65,13 +65,20 @@ fn check_decl(cx: &Context, decl: &Decl, bindings: &mut Vec) { } } +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 pat_is_binding(&cx.tcx.def_map, pat) { + if is_binding(cx, pat) { if bindings.contains(&name) { lint_shadow(cx, name, span, pat.span, init); }