From 4835372df559ff2e14edcdba409f5a6566a779bc Mon Sep 17 00:00:00 2001 From: llogiq Date: Tue, 8 Sep 2015 11:50:04 +0200 Subject: [PATCH] made shadow_unrelated allow, added previous binding span note, fixed #319 --- README.md | 2 +- src/lib.rs | 2 +- src/shadow.rs | 55 +++++++++++++++++++++++------------- tests/compile-fail/shadow.rs | 5 ++++ 4 files changed, 42 insertions(+), 22 deletions(-) diff --git a/README.md b/README.md index ca96e496bfe..fdd341efa45 100644 --- a/README.md +++ b/README.md @@ -48,7 +48,7 @@ name [result_unwrap_used](https://github.com/Manishearth/rust-clippy/wiki#result_unwrap_used) | allow | using `Result.unwrap()`, which might be better handled [shadow_reuse](https://github.com/Manishearth/rust-clippy/wiki#shadow_reuse) | allow | rebinding a name to an expression that re-uses the original value, e.g. `let x = x + 1` [shadow_same](https://github.com/Manishearth/rust-clippy/wiki#shadow_same) | allow | rebinding a name to itself, e.g. `let mut x = &mut x` -[shadow_unrelated](https://github.com/Manishearth/rust-clippy/wiki#shadow_unrelated) | warn | The name is re-bound without even using the original value +[shadow_unrelated](https://github.com/Manishearth/rust-clippy/wiki#shadow_unrelated) | allow | The name is re-bound without even using the original value [should_implement_trait](https://github.com/Manishearth/rust-clippy/wiki#should_implement_trait) | warn | defining a method that should be implementing a std trait [single_match](https://github.com/Manishearth/rust-clippy/wiki#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](https://github.com/Manishearth/rust-clippy/wiki#str_to_string) | warn | using `to_string()` on a str, which should be `to_owned()` diff --git a/src/lib.rs b/src/lib.rs index 7ce2be97358..ddaa3bf490c 100755 --- a/src/lib.rs +++ b/src/lib.rs @@ -96,6 +96,7 @@ pub fn plugin_registrar(reg: &mut Registry) { ptr_arg::PTR_ARG, shadow::SHADOW_REUSE, shadow::SHADOW_SAME, + shadow::SHADOW_UNRELATED, strings::STRING_ADD, strings::STRING_ADD_ASSIGN, types::CAST_POSSIBLE_TRUNCATION, @@ -141,7 +142,6 @@ pub fn plugin_registrar(reg: &mut Registry) { ranges::RANGE_STEP_BY_ZERO, returns::LET_AND_RETURN, returns::NEEDLESS_RETURN, - shadow::SHADOW_UNRELATED, types::BOX_VEC, types::LET_UNIT_VALUE, types::LINKEDLIST, diff --git a/src/shadow.rs b/src/shadow.rs index aaa2c91a036..ae1eeee2401 100644 --- a/src/shadow.rs +++ b/src/shadow.rs @@ -4,7 +4,7 @@ use reexport::*; use syntax::codemap::Span; use rustc_front::visit::FnKind; -use rustc::lint::{Context, LintArray, LintPass}; +use rustc::lint::{Context, Level, Lint, LintArray, LintPass}; use rustc::middle::def::Def::{DefVariant, DefStruct}; use utils::{in_external_macro, snippet, span_lint, span_note_and_lint}; @@ -14,7 +14,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_UNRELATED, Warn, +declare_lint!(pub SHADOW_UNRELATED, Allow, "The name is re-bound without even using the original value"); #[derive(Copy, Clone)] @@ -36,13 +36,13 @@ 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) + bindings.push((ident.node.name, ident.span)) } } check_block(cx, block, &mut bindings); } -fn check_block(cx: &Context, block: &Block, bindings: &mut Vec) { +fn check_block(cx: &Context, block: &Block, bindings: &mut Vec<(Name, Span)>) { let len = bindings.len(); for stmt in &block.stmts { match stmt.node { @@ -55,7 +55,7 @@ fn check_block(cx: &Context, block: &Block, bindings: &mut Vec) { bindings.truncate(len); } -fn check_decl(cx: &Context, decl: &Decl, bindings: &mut Vec) { +fn check_decl(cx: &Context, decl: &Decl, bindings: &mut Vec<(Name, Span)>) { 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; @@ -77,16 +77,23 @@ fn is_binding(cx: &Context, pat: &Pat) -> bool { } fn check_pat(cx: &Context, pat: &Pat, init: &Option<&Expr>, span: Span, - bindings: &mut Vec) { + bindings: &mut Vec<(Name, Span)>) { //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); - } else { - bindings.push(name); + let mut new_binding = true; + for tup in bindings.iter_mut() { + if tup.0 == name { + lint_shadow(cx, name, span, pat.span, init, tup.1); + tup.1 = ident.span; + new_binding = false; + break; + } + } + if new_binding { + bindings.push((name, ident.span)); } } if let Some(ref p) = *inner { check_pat(cx, p, init, span, bindings); } @@ -141,20 +148,25 @@ fn check_pat(cx: &Context, pat: &Pat, init: &Option<&Expr>, span: Span, }, PatRegion(ref inner, _) => check_pat(cx, inner, init, span, bindings), - //PatRange(P, P), //PatVec(Vec>, Option>, Vec>), _ => (), } } fn lint_shadow(cx: &Context, name: Name, span: Span, lspan: Span, init: - &Option) where T: Deref { + &Option, prev_span: Span) where T: Deref { + fn note_orig(cx: &Context, lint: &'static Lint, span: Span) { + if cx.current_level(lint) != Level::Allow { + cx.sess().span_note(span, "previous binding is here"); + } + } 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, ".."))); + note_orig(cx, SHADOW_SAME, prev_span); } else { if contains_self(name, expr) { span_note_and_lint(cx, SHADOW_REUSE, lspan, &format!( @@ -162,21 +174,24 @@ fn lint_shadow(cx: &Context, name: Name, span: Span, lspan: Span, init: snippet(cx, lspan, "_"), snippet(cx, expr.span, "..")), expr.span, "initialization happens here"); + note_orig(cx, SHADOW_REUSE, prev_span); } else { span_note_and_lint(cx, SHADOW_UNRELATED, lspan, &format!( "{} is shadowed by {}", snippet(cx, lspan, "_"), snippet(cx, expr.span, "..")), expr.span, "initialization happens here"); + note_orig(cx, SHADOW_UNRELATED, prev_span); } } } else { span_lint(cx, SHADOW_UNRELATED, span, &format!( "{} shadows a previous declaration", snippet(cx, lspan, "_"))); + note_orig(cx, SHADOW_UNRELATED, prev_span); } } -fn check_expr(cx: &Context, expr: &Expr, bindings: &mut Vec) { +fn check_expr(cx: &Context, expr: &Expr, bindings: &mut Vec<(Name, Span)>) { if in_external_macro(cx, expr.span) { return; } match expr.node { ExprUnary(_, ref e) | ExprParen(ref e) | ExprField(ref e, _) | @@ -205,20 +220,20 @@ fn check_expr(cx: &Context, expr: &Expr, bindings: &mut Vec) { 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 + //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); + bindings.truncate(len); } - if let Some(ref guard) = arm.guard { - check_expr(cx, guard, bindings); - } - check_expr(cx, &arm.body, bindings); - bindings.truncate(len); } }, _ => () } } -fn check_ty(cx: &Context, ty: &Ty, bindings: &mut Vec) { +fn check_ty(cx: &Context, ty: &Ty, bindings: &mut Vec<(Name, Span)>) { match ty.node { TyParen(ref sty) | TyObjectSum(ref sty, _) | TyVec(ref sty) => check_ty(cx, sty, bindings), diff --git a/tests/compile-fail/shadow.rs b/tests/compile-fail/shadow.rs index 80d48f84163..293d97a42fa 100755 --- a/tests/compile-fail/shadow.rs +++ b/tests/compile-fail/shadow.rs @@ -27,4 +27,9 @@ fn main() { Some(p) => p, // no error, because the p above is in its own scope None => 0, }; + + match (x, o) { + (1, Some(a)) | (a, Some(1)) => (), // no error though `a` appears twice + _ => (), + } }