From 49e2570b77558e9215b43926d2633eef4a25f7bd Mon Sep 17 00:00:00 2001 From: Oliver Schneider Date: Wed, 11 May 2016 16:44:43 +0200 Subject: [PATCH 1/3] don't lint at the use-site of bad struct field bindings if they're shorthand fixes #899 --- src/non_expressive_names.rs | 25 ++++++++++++--------- tests/compile-fail/non_expressive_names2.rs | 14 ++++++++++++ 2 files changed, 29 insertions(+), 10 deletions(-) create mode 100644 tests/compile-fail/non_expressive_names2.rs diff --git a/src/non_expressive_names.rs b/src/non_expressive_names.rs index 9d2139fe422..e8f3858a37b 100644 --- a/src/non_expressive_names.rs +++ b/src/non_expressive_names.rs @@ -3,7 +3,7 @@ use syntax::parse::token::InternedString; use syntax::ast::*; use syntax::attr; -use syntax::visit; +use syntax::visit::{Visitor, walk_block, walk_pat, walk_expr}; use utils::{span_lint_and_then, in_macro, span_lint}; /// **What it does:** This lint warns about names that are very similar and thus confusing @@ -68,12 +68,17 @@ struct SimilarNamesLocalVisitor<'a, 'b: 'a> { struct SimilarNamesNameVisitor<'a, 'b: 'a, 'c: 'b>(&'a mut SimilarNamesLocalVisitor<'b, 'c>); -impl<'v, 'a, 'b, 'c> visit::Visitor<'v> for SimilarNamesNameVisitor<'a, 'b, 'c> { +impl<'v, 'a, 'b, 'c> Visitor<'v> for SimilarNamesNameVisitor<'a, 'b, 'c> { fn visit_pat(&mut self, pat: &'v Pat) { - if let PatKind::Ident(_, id, _) = pat.node { - self.check_name(id.span, id.node.name); + match pat.node { + PatKind::Ident(_, id, _) => self.check_name(id.span, id.node.name), + PatKind::Struct(_, ref fields, _) => for field in fields { + if !field.node.is_shorthand { + self.visit_pat(&field.node.pat); + } + }, + _ => walk_pat(self, pat), } - visit::walk_pat(self, pat); } } @@ -219,22 +224,22 @@ fn apply Fn(&'c mut Self)>(&mut self, f: F) { } } -impl<'v, 'a, 'b> visit::Visitor<'v> for SimilarNamesLocalVisitor<'a, 'b> { +impl<'v, 'a, 'b> Visitor<'v> for SimilarNamesLocalVisitor<'a, 'b> { fn visit_local(&mut self, local: &'v Local) { if let Some(ref init) = local.init { - self.apply(|this| visit::walk_expr(this, &**init)); + self.apply(|this| walk_expr(this, &**init)); } // add the pattern after the expression because the bindings aren't available yet in the init expression SimilarNamesNameVisitor(self).visit_pat(&*local.pat); } fn visit_block(&mut self, blk: &'v Block) { - self.apply(|this| visit::walk_block(this, blk)); + self.apply(|this| walk_block(this, blk)); } fn visit_arm(&mut self, arm: &'v Arm) { self.apply(|this| { // just go through the first pattern, as either all patterns bind the same bindings or rustc would have errored much earlier SimilarNamesNameVisitor(this).visit_pat(&arm.pats[0]); - this.apply(|this| visit::walk_expr(this, &arm.body)); + this.apply(|this| walk_expr(this, &arm.body)); }); } fn visit_item(&mut self, _: &'v Item) { @@ -257,7 +262,7 @@ fn check_item(&mut self, cx: &EarlyContext, item: &Item) { visit::walk_pat(&mut SimilarNamesNameVisitor(&mut visitor), &arg.pat); } // walk all other bindings - visit::walk_block(&mut visitor, blk); + walk_block(&mut visitor, blk); } } } diff --git a/tests/compile-fail/non_expressive_names2.rs b/tests/compile-fail/non_expressive_names2.rs new file mode 100644 index 00000000000..a0e5885c539 --- /dev/null +++ b/tests/compile-fail/non_expressive_names2.rs @@ -0,0 +1,14 @@ +#![feature(plugin)] +#![plugin(clippy)] +#![deny(clippy,similar_names)] +#![allow(unused)] + +struct Foo { + apple: i32, + bpple: i32, +} + +fn main() { + let Foo { apple, bpple } = unimplemented!(); + let Foo { apple: spring, bpple: sprang } = unimplemented!(); //~ ERROR: name is too similar +} From f004120495b51c925fa72156b69ebd97f011b061 Mon Sep 17 00:00:00 2001 From: Oliver Schneider Date: Wed, 11 May 2016 16:45:06 +0200 Subject: [PATCH 2/3] properly lint function argument patterns in similar_names --- src/non_expressive_names.rs | 2 +- src/shadow.rs | 14 +++++++------- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/src/non_expressive_names.rs b/src/non_expressive_names.rs index e8f3858a37b..cbb083a3e16 100644 --- a/src/non_expressive_names.rs +++ b/src/non_expressive_names.rs @@ -259,7 +259,7 @@ fn check_item(&mut self, cx: &EarlyContext, item: &Item) { }; // initialize with function arguments for arg in &decl.inputs { - visit::walk_pat(&mut SimilarNamesNameVisitor(&mut visitor), &arg.pat); + SimilarNamesNameVisitor(&mut visitor).visit_pat(&arg.pat); } // walk all other bindings walk_block(&mut visitor, blk); diff --git a/src/shadow.rs b/src/shadow.rs index 4b6439c4eb3..4639a943965 100644 --- a/src/shadow.rs +++ b/src/shadow.rs @@ -195,7 +195,7 @@ fn check_pat(cx: &LateContext, pat: &Pat, init: &Option<&Expr>, span: Span, bind } } -fn lint_shadow(cx: &LateContext, name: Name, span: Span, lspan: Span, init: &Option, prev_span: Span) +fn lint_shadow(cx: &LateContext, name: Name, span: Span, pattern_span: Span, init: &Option, prev_span: Span) where T: Deref { fn note_orig(cx: &LateContext, mut db: DiagnosticWrapper, lint: &'static Lint, span: Span) { @@ -209,15 +209,15 @@ fn note_orig(cx: &LateContext, mut db: DiagnosticWrapper, lint: &'static Lint, s SHADOW_SAME, span, &format!("{} is shadowed by itself in {}", - snippet(cx, lspan, "_"), + snippet(cx, pattern_span, "_"), snippet(cx, expr.span, ".."))); note_orig(cx, db, SHADOW_SAME, prev_span); } else if contains_self(name, expr) { let db = span_note_and_lint(cx, SHADOW_REUSE, - lspan, + pattern_span, &format!("{} is shadowed by {} which reuses the original value", - snippet(cx, lspan, "_"), + snippet(cx, pattern_span, "_"), snippet(cx, expr.span, "..")), expr.span, "initialization happens here"); @@ -225,9 +225,9 @@ fn note_orig(cx: &LateContext, mut db: DiagnosticWrapper, lint: &'static Lint, s } else { let db = span_note_and_lint(cx, SHADOW_UNRELATED, - lspan, + pattern_span, &format!("{} is shadowed by {}", - snippet(cx, lspan, "_"), + snippet(cx, pattern_span, "_"), snippet(cx, expr.span, "..")), expr.span, "initialization happens here"); @@ -238,7 +238,7 @@ fn note_orig(cx: &LateContext, mut db: DiagnosticWrapper, lint: &'static Lint, s let db = span_lint(cx, SHADOW_UNRELATED, span, - &format!("{} shadows a previous declaration", snippet(cx, lspan, "_"))); + &format!("{} shadows a previous declaration", snippet(cx, pattern_span, "_"))); note_orig(cx, db, SHADOW_UNRELATED, prev_span); } } From 0bef7b5f744b24136dc77a6478aee26e0abce33a Mon Sep 17 00:00:00 2001 From: Oliver Schneider Date: Wed, 11 May 2016 17:01:34 +0200 Subject: [PATCH 3/3] merge struct similar_name test into the general test file --- tests/compile-fail/non_expressive_names.rs | 14 ++++++++++++++ tests/compile-fail/non_expressive_names2.rs | 14 -------------- 2 files changed, 14 insertions(+), 14 deletions(-) delete mode 100644 tests/compile-fail/non_expressive_names2.rs diff --git a/tests/compile-fail/non_expressive_names.rs b/tests/compile-fail/non_expressive_names.rs index 61fe0067a27..d959507bcb2 100644 --- a/tests/compile-fail/non_expressive_names.rs +++ b/tests/compile-fail/non_expressive_names.rs @@ -11,8 +11,15 @@ //~| NOTE: lint level defined here //~| NOTE: lint level defined here //~| NOTE: lint level defined here +//~| NOTE: lint level defined here #![allow(unused)] + +struct Foo { + apple: i32, + bpple: i32, +} + fn main() { let specter: i32; let spectre: i32; @@ -90,6 +97,13 @@ fn main() { let rx_cake: i32; } +fn foo() { + let Foo { apple, bpple } = unimplemented!(); + let Foo { apple: spring, //~NOTE existing binding defined here + bpple: sprang } = unimplemented!(); //~ ERROR: name is too similar + //~^HELP for further information +} + #[derive(Clone, Debug)] enum MaybeInst { Split, diff --git a/tests/compile-fail/non_expressive_names2.rs b/tests/compile-fail/non_expressive_names2.rs deleted file mode 100644 index a0e5885c539..00000000000 --- a/tests/compile-fail/non_expressive_names2.rs +++ /dev/null @@ -1,14 +0,0 @@ -#![feature(plugin)] -#![plugin(clippy)] -#![deny(clippy,similar_names)] -#![allow(unused)] - -struct Foo { - apple: i32, - bpple: i32, -} - -fn main() { - let Foo { apple, bpple } = unimplemented!(); - let Foo { apple: spring, bpple: sprang } = unimplemented!(); //~ ERROR: name is too similar -}