Skip to content
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.

Sign up for GitHub

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

Use span of impl/trait in len_without_is_empty error message, rather … #1559

Merged
merged 3 commits into from
Feb 21, 2017
Merged

Use span of impl/trait in len_without_is_empty error message, rather … #1559

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2dd45b7
Use span of impl/trait in len_without_is_empty error message, rather …
theotherphil Feb 19, 2017
0396756
Add test that adding allow attribute on impl block containing len sil…
theotherphil Feb 19, 2017
d6a4d2c
Fix typo
theotherphil Feb 19, 2017
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
4 changes: 2 additions & 2 deletions clippy_lints/src/len_zero.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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));
}
}
Expand Down Expand Up @@ -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));
}
}
Expand Down
24 changes: 24 additions & 0 deletions tests/ui/len_zero.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 one on the
// impl containing len.
impl PubAllowed {
pub fn irrelevant(self: &Self) -> bool {
false
}
}

struct NotPubOne;

impl NotPubOne {
Expand Down
81 changes: 49 additions & 32 deletions tests/ui/len_zero.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes me wonder what happens when there is more than one impl for a type.

Can you also had a test that checks that this actually fixes #1532?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. The current lint has a false positive if len and is_empty are defined in separate impl blocks.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've pushed a change which:

  • adds an extra, irrelevant impl block to PubOne to check that we this doesn't generate any errors
  • adds a PubAllowed struct equivalent to PubOne but with an allow attribution on the relevant impl.

Are you happy to merge this as is, or do you want to wait for a fix to the impls-in-different-blocks issue?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you happy to merge this as is, or do you want to wait for a fix to the impls-in-different-blocks issue?

That's a different issue. I reported it as #1562

|
note: lint level defined here
--> $DIR/len_zero.rs:4:9
Expand All @@ -14,33 +16,48 @@ 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:55:1
|
32 | fn len(self: &Self) -> isize;
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
55 | pub trait PubTraitsToo {
| _^ starting here...
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:66:5
--> $DIR/len_zero.rs:89:1
|
66 | pub fn len(self: &Self) -> isize {
| _____^ starting here...
67 | | 1
68 | | }
| |_____^ ...ending here
89 | impl HasIsEmpty {
| _^ starting here...
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:95:5
|
95 | pub fn len(self: &Self) -> isize {
| _____^ starting here...
96 | | 1
97 | | }
| |_____^ ...ending here
--> $DIR/len_zero.rs:118:1
|
118 | impl HasWrongIsEmpty {
| _^ starting here...
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
Expand All @@ -52,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`
Expand Down