Skip to content

len_without_is_empty in private types #1085

New issue

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

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

Already on GitHub? # to your account

Closed
FraGag opened this issue Jul 10, 2016 · 4 comments
Closed

len_without_is_empty in private types #1085

FraGag opened this issue Jul 10, 2016 · 4 comments
Labels
C-bug Category: Clippy is not doing the correct thing T-middle Type: Probably requires verifiying types

Comments

@FraGag
Copy link

FraGag commented Jul 10, 2016

The len_without_is_empty lint fires on private types, but adding an is_empty method as suggested can lead to the is_empty method raising the dead_code lint.

For example, this code gives a len_without_is_empty warning on Foo.len():

struct Foo;

impl Foo {
    fn len(&self) -> usize { // warning: item `Foo` has a `.len(_: &Self)` method, but no `.is_empty(_: &Self)` method. Consider adding one, #[warn(len_without_is_empty)] on by default
        unimplemented!()
    }
}

fn main() {
    println!("{}", Foo.len());
}

If the code is modified to add an is_empty method, then Foo.is_empty raises a dead_code warning.

struct Foo;

impl Foo {
    fn is_empty(&self) -> bool { // warning: method is never used: `is_empty`, #[warn(dead_code)] on by default
        unimplemented!()
    }

    fn len(&self) -> usize {
        unimplemented!()
    }
}

fn main() {
    println!("{}", Foo.len());
}

Therefore, if the programmer doesn't need an is_empty method, he or she is faced with a dilemma: either silence the len_without_is_empty lint, or add the is_empty method and silence the dead_code lint. We could avoid this dilemma by not raising the len_without_is_empty lint in the first place if the is_empty would raise a dead_code warning; instinctively, I would check if the type is private (i.e. not exported from the crate), but I don't know if that's enough.

Also, I noticed that the len_zero lint doesn't fire if there's no is_empty method on the type. By applying the suggestion above, the len_without_is_empty would not fire and the len() == 0 would not raise any warnings. Perhaps the len_zero lint could be adjusted to also fire when the len_without_is_empty lint was silenced, or the len_without_is_empty lint should fire only if one of the patterns len_zero looks for is found and the type doesn't have an is_empty method yet. The idea is to suggest adding the is_empty method only when it's needed. :)

@mcarton mcarton added C-bug Category: Clippy is not doing the correct thing T-middle Type: Probably requires verifiying types labels Jul 10, 2016
@mcarton
Copy link
Member

mcarton commented Jul 10, 2016

That's a very complete issue report, thanks!

@killercup
Copy link
Member

Huh, I'm kinda surprised there isn't a trait Countable (or something like that) in std.

Closest thing I could find is std::iter::ExactSizeIterator – which has len but no is_empty 😅

@mcarton
Copy link
Member

mcarton commented Jul 11, 2016

There's an issue for it: rust-lang/rust#34357.

@mcarton
Copy link
Member

mcarton commented Aug 29, 2016

Closest thing I could find is std::iter::ExactSizeIterator – which has len but no is_empty 😅

It does now

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
C-bug Category: Clippy is not doing the correct thing T-middle Type: Probably requires verifiying types
Projects
None yet
Development

No branches or pull requests

3 participants