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<Name> {
} else { None }
}
-fn add(bindings: &mut Vec<Name>, pat: &Pat) {
- named(pat).map(|name| bindings.push(name));
-}
-
fn check_block(cx: &Context, block: &Block, bindings: &mut Vec<Name>) {
let len = bindings.len();
for stmt in &block.stmts {
@@ -65,41 +62,53 @@ fn check_block(cx: &Context, block: &Block, bindings: &mut Vec<Name>) {
fn check_decl(cx: &Context, decl: &Decl, bindings: &mut Vec<Name>) {
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<T>(cx: &Context, pat: &Pat, init: &Option<T>, span: Span,
+ bindings: &mut Vec<Name>) where T: Deref<Target=Expr> {
+ //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<T>(cx: &Context, name: Name, span: Span, lspan: Span, init:
+ &Option<T>) where T: Deref<Target=Expr> {
+ 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<Name>) {
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