From 2dd45b73172bf4ef8f60e8ed32588d688bc27fb1 Mon Sep 17 00:00:00 2001 From: Phil Ellison Date: Sun, 19 Feb 2017 15:14:30 +0000 Subject: [PATCH 1/3] Use span of impl/trait in len_without_is_empty error message, rather than the span of the len method --- clippy_lints/src/len_zero.rs | 4 +-- tests/ui/len_zero.stderr | 53 ++++++++++++++++++++++++------------ 2 files changed, 37 insertions(+), 20 deletions(-) diff --git a/clippy_lints/src/len_zero.rs b/clippy_lints/src/len_zero.rs index 8a2f37e5aa1f..f2609dd57435 100644 --- a/clippy_lints/src/len_zero.rs +++ b/clippy_lints/src/len_zero.rs @@ -107,7 +107,7 @@ fn check_trait_items(cx: &LateContext, item: &Item, trait_items: &[TraitItemRef] if cx.access_levels.is_exported(i.id.node_id) { span_lint(cx, LEN_WITHOUT_IS_EMPTY, - i.span, + item.span, &format!("trait `{}` has a `len` method but no `is_empty` method", item.name)); } } @@ -146,7 +146,7 @@ fn check_impl_items(cx: &LateContext, item: &Item, impl_items: &[ImplItemRef]) { span_lint(cx, LEN_WITHOUT_IS_EMPTY, - i.span, + item.span, &format!("item `{}` has a public `len` method but {} `is_empty` method", ty, is_empty)); } } diff --git a/tests/ui/len_zero.stderr b/tests/ui/len_zero.stderr index 5aaaecf55cb7..430cf8575bf1 100644 --- a/tests/ui/len_zero.stderr +++ b/tests/ui/len_zero.stderr @@ -1,11 +1,13 @@ error: item `PubOne` has a public `len` method but no corresponding `is_empty` method - --> $DIR/len_zero.rs:10:5 + --> $DIR/len_zero.rs:9:1 | -10 | pub fn len(self: &Self) -> isize { - | _____^ starting here... +9 | impl PubOne { + | _^ starting here... +10 | | pub fn len(self: &Self) -> isize { 11 | | 1 12 | | } - | |_____^ ...ending here +13 | | } + | |_^ ...ending here | note: lint level defined here --> $DIR/len_zero.rs:4:9 @@ -14,28 +16,43 @@ note: lint level defined here | ^^^^^^^^^^^^^^^^^^^^ error: trait `PubTraitsToo` has a `len` method but no `is_empty` method - --> $DIR/len_zero.rs:32:5 + --> $DIR/len_zero.rs:31:1 | -32 | fn len(self: &Self) -> isize; - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +31 | pub trait PubTraitsToo { + | _^ starting here... +32 | | fn len(self: &Self) -> isize; +33 | | } + | |_^ ...ending here error: item `HasIsEmpty` has a public `len` method but a private `is_empty` method - --> $DIR/len_zero.rs:66:5 + --> $DIR/len_zero.rs:65:1 | -66 | pub fn len(self: &Self) -> isize { - | _____^ starting here... +65 | impl HasIsEmpty { + | _^ starting here... +66 | | pub fn len(self: &Self) -> isize { 67 | | 1 68 | | } - | |_____^ ...ending here +69 | | +70 | | fn is_empty(self: &Self) -> bool { +71 | | false +72 | | } +73 | | } + | |_^ ...ending here error: item `HasWrongIsEmpty` has a public `len` method but no corresponding `is_empty` method - --> $DIR/len_zero.rs:95:5 - | -95 | pub fn len(self: &Self) -> isize { - | _____^ starting here... -96 | | 1 -97 | | } - | |_____^ ...ending here + --> $DIR/len_zero.rs:94:1 + | +94 | impl HasWrongIsEmpty { + | _^ starting here... +95 | | pub fn len(self: &Self) -> isize { +96 | | 1 +97 | | } +98 | | +99 | | pub fn is_empty(self: &Self, x : u32) -> bool { +100 | | false +101 | | } +102 | | } + | |_^ ...ending here error: length comparison to zero --> $DIR/len_zero.rs:106:8 From 03967560982aa862765763cce8a738c3130d1669 Mon Sep 17 00:00:00 2001 From: Phil Ellison Date: Sun, 19 Feb 2017 15:36:17 +0000 Subject: [PATCH 2/3] Add test that adding allow attribute on impl block containing len silences len_without_is_empty. Add extra impl block to PubOne to check that this doesn't get flagged@ --- tests/ui/len_zero.rs | 24 ++++++++++++++ tests/ui/len_zero.stderr | 72 ++++++++++++++++++++-------------------- 2 files changed, 60 insertions(+), 36 deletions(-) diff --git a/tests/ui/len_zero.rs b/tests/ui/len_zero.rs index a310eeef3846..5b62949d77e4 100644 --- a/tests/ui/len_zero.rs +++ b/tests/ui/len_zero.rs @@ -12,6 +12,30 @@ impl PubOne { } } +impl PubOne { // A second impl for this struct - the error span shouldn't mention this + pub fn irrelevant(self: &Self) -> bool { + false + } +} + +// Identical to PubOne, but with an allow attribute on the impl complaining len +pub struct PubAllowed; + +#[allow(len_without_is_empty)] +impl PubAllowed { + pub fn len(self: &Self) -> isize { + 1 + } +} + +// No allow attribute on this impl block, but that doesn't matter - we only require on the +// impl containing len. +impl PubAllowed { + pub fn irrelevant(self: &Self) -> bool { + false + } +} + struct NotPubOne; impl NotPubOne { diff --git a/tests/ui/len_zero.stderr b/tests/ui/len_zero.stderr index 430cf8575bf1..9ebc5209a22e 100644 --- a/tests/ui/len_zero.stderr +++ b/tests/ui/len_zero.stderr @@ -16,48 +16,48 @@ note: lint level defined here | ^^^^^^^^^^^^^^^^^^^^ error: trait `PubTraitsToo` has a `len` method but no `is_empty` method - --> $DIR/len_zero.rs:31:1 + --> $DIR/len_zero.rs:55:1 | -31 | pub trait PubTraitsToo { +55 | pub trait PubTraitsToo { | _^ starting here... -32 | | fn len(self: &Self) -> isize; -33 | | } +56 | | fn len(self: &Self) -> isize; +57 | | } | |_^ ...ending here error: item `HasIsEmpty` has a public `len` method but a private `is_empty` method - --> $DIR/len_zero.rs:65:1 + --> $DIR/len_zero.rs:89:1 | -65 | impl HasIsEmpty { +89 | impl HasIsEmpty { | _^ starting here... -66 | | pub fn len(self: &Self) -> isize { -67 | | 1 -68 | | } -69 | | -70 | | fn is_empty(self: &Self) -> bool { -71 | | false -72 | | } -73 | | } +90 | | pub fn len(self: &Self) -> isize { +91 | | 1 +92 | | } +93 | | +94 | | fn is_empty(self: &Self) -> bool { +95 | | false +96 | | } +97 | | } | |_^ ...ending here error: item `HasWrongIsEmpty` has a public `len` method but no corresponding `is_empty` method - --> $DIR/len_zero.rs:94:1 + --> $DIR/len_zero.rs:118:1 | -94 | impl HasWrongIsEmpty { +118 | impl HasWrongIsEmpty { | _^ starting here... -95 | | pub fn len(self: &Self) -> isize { -96 | | 1 -97 | | } -98 | | -99 | | pub fn is_empty(self: &Self, x : u32) -> bool { -100 | | false -101 | | } -102 | | } +119 | | pub fn len(self: &Self) -> isize { +120 | | 1 +121 | | } +122 | | +123 | | pub fn is_empty(self: &Self, x : u32) -> bool { +124 | | false +125 | | } +126 | | } | |_^ ...ending here error: length comparison to zero - --> $DIR/len_zero.rs:106:8 + --> $DIR/len_zero.rs:130:8 | -106 | if x.len() == 0 { +130 | if x.len() == 0 { | ^^^^^^^^^^^^ | note: lint level defined here @@ -69,45 +69,45 @@ help: consider using `is_empty` | if x.is_empty() { error: length comparison to zero - --> $DIR/len_zero.rs:113:8 + --> $DIR/len_zero.rs:137:8 | -113 | if "".len() == 0 { +137 | if "".len() == 0 { | ^^^^^^^^^^^^^ | help: consider using `is_empty` | if "".is_empty() { error: length comparison to zero - --> $DIR/len_zero.rs:130:8 + --> $DIR/len_zero.rs:154:8 | -130 | if has_is_empty.len() == 0 { +154 | if has_is_empty.len() == 0 { | ^^^^^^^^^^^^^^^^^^^^^^^ | help: consider using `is_empty` | if has_is_empty.is_empty() { error: length comparison to zero - --> $DIR/len_zero.rs:136:8 + --> $DIR/len_zero.rs:160:8 | -136 | if has_is_empty.len() != 0 { +160 | if has_is_empty.len() != 0 { | ^^^^^^^^^^^^^^^^^^^^^^^ | help: consider using `is_empty` | if !has_is_empty.is_empty() { error: length comparison to zero - --> $DIR/len_zero.rs:142:8 + --> $DIR/len_zero.rs:166:8 | -142 | if has_is_empty.len() > 0 { +166 | if has_is_empty.len() > 0 { | ^^^^^^^^^^^^^^^^^^^^^^ | help: consider using `is_empty` | if !has_is_empty.is_empty() { error: length comparison to zero - --> $DIR/len_zero.rs:151:8 + --> $DIR/len_zero.rs:175:8 | -151 | if with_is_empty.len() == 0 { +175 | if with_is_empty.len() == 0 { | ^^^^^^^^^^^^^^^^^^^^^^^^ | help: consider using `is_empty` From d6a4d2cb18504a4b5d81be7d67dfac668b074cec Mon Sep 17 00:00:00 2001 From: Phil Ellison Date: Sun, 19 Feb 2017 15:39:40 +0000 Subject: [PATCH 3/3] Fix typo --- tests/ui/len_zero.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/ui/len_zero.rs b/tests/ui/len_zero.rs index 5b62949d77e4..9790f2c9fcb7 100644 --- a/tests/ui/len_zero.rs +++ b/tests/ui/len_zero.rs @@ -28,7 +28,7 @@ impl PubAllowed { } } -// No allow attribute on this impl block, but that doesn't matter - we only require on the +// No allow attribute on this impl block, but that doesn't matter - we only require one on the // impl containing len. impl PubAllowed { pub fn irrelevant(self: &Self) -> bool {