Skip to content

made shadow_unrelated allow, added previous binding span note, #321

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Sep 8, 2015
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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()`
Expand Down
2 changes: 1 addition & 1 deletion src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand Down
55 changes: 35 additions & 20 deletions src/shadow.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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};
Expand All @@ -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)]
Expand All @@ -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<Name>) {
fn check_block(cx: &Context, block: &Block, bindings: &mut Vec<(Name, Span)>) {
let len = bindings.len();
for stmt in &block.stmts {
match stmt.node {
Expand All @@ -55,7 +55,7 @@ fn check_block(cx: &Context, block: &Block, bindings: &mut Vec<Name>) {
bindings.truncate(len);
}

fn check_decl(cx: &Context, decl: &Decl, bindings: &mut Vec<Name>) {
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;
Expand All @@ -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<Name>) {
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); }
Expand Down Expand Up @@ -141,42 +148,50 @@ fn check_pat(cx: &Context, pat: &Pat, init: &Option<&Expr>, span: Span,
},
PatRegion(ref inner, _) =>
check_pat(cx, inner, init, span, bindings),
//PatRange(P<Expr>, P<Expr>),
//PatVec(Vec<P<Pat>>, Option<P<Pat>>, Vec<P<Pat>>),
_ => (),
}
}

fn lint_shadow<T>(cx: &Context, name: Name, span: Span, lspan: Span, init:
&Option<T>) where T: Deref<Target=Expr> {
&Option<T>, prev_span: Span) where T: Deref<Target=Expr> {
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!(
"{} is shadowed by {} which reuses the original value",
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<Name>) {
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, _) |
Expand Down Expand Up @@ -205,20 +220,20 @@ fn check_expr(cx: &Context, expr: &Expr, bindings: &mut Vec<Name>) {
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);
//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);
}
check_expr(cx, &arm.body, bindings);
bindings.truncate(len);
}
},
_ => ()
}
}

fn check_ty(cx: &Context, ty: &Ty, bindings: &mut Vec<Name>) {
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),
Expand Down
5 changes: 5 additions & 0 deletions tests/compile-fail/shadow.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
_ => (),
}
}