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.

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

ZeroLengthPredicate: gives false warning on File.stat #2841

Closed
perlun opened this issue Feb 15, 2016 · 14 comments

Comments

Projects
None yet
7 participants
@perlun
Copy link

commented Feb 15, 2016

The ZeroLengthPredicate is a bit too aggressive in its heuristics. The code below gives a warning:

def manifest_exist(folder)
  manifest_file = File.join(folder, 'app.manifest')

  File.stat(manifest_file).size == 0
end

The warning is this:

foo.rb:4:3: C: Style/ZeroLengthPredicate: Use empty? instead of size == 0.
  File.stat(manifest_file).size == 0
  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

This is a problem since the File::Stat class doesn't even have an empty? method defined. 😄

$ irb
2.3.0 :001 > File.stat('Gemfile').empty?
NoMethodError: undefined method `empty?' for #<File::Stat:0x007f86dc902080>
    from (irb):1
    from /Users/plundberg/.rvm/rubies/ruby-2.3.0/bin/irb:11:in `<main>'

I can of course easily disable this on the line in question. This issue exists to remind us that the detection code in this cop should be made a bit smarter (if possible), to avoid treating this line as a Rubocop offense.

@alexdowad

This comment has been minimized.

Copy link
Contributor

commented Feb 15, 2016

This issue exists to remind us that the detection code in this cop should be made a bit smarter (if possible)

Thanks for the good example. Do you have any ideas how to make the cop smarter?

@perlun

This comment has been minimized.

Copy link
Author

commented Feb 15, 2016

Good question. I looked a bit at the code in https://github.com/bbatsov/rubocop/blob/master/lib/rubocop/cop/style/zero_length_predicate.rb and it's obviously a whole lot more work to do it "properly" here, i.e. trying to check types/etc.

A simple change for now would be to remove the lint for size == 0 (and size != 0) cases, and only run it for length == 0 etc. cases. I would think that would be the easy way to avoid it reporting false errors.

@Drenmi

This comment has been minimized.

Copy link
Collaborator

commented Feb 16, 2016

You can follow some of the discussion that preceded the implementation of the cop in this issue.

Because of the nature of dynamically typed, object oriented languages, and the use of duck typing in Ruby, it is impossible to avoid false positives. Attempting to do things like type checking (except for literals) would be outside the scope of what's possible with static code analysis.

If I correctly understand the intention of @bbatsov, Rubocop is meant to be tailored to the Ruby standard library, and false positives should be handled through disabling the cop, either inline for a certain piece of code, or altogether. (Feel free to correct me on this. 😄)

Within those constraints, however, the cops should obviously be as "smart" as possible, as to minimize the number of duds. 😄

@alexdowad

This comment has been minimized.

Copy link
Contributor

commented Feb 16, 2016

Within those constraints, however, the cops should obviously be as "smart" as possible, as to minimize the number of duds.

Absolutely. If these cops prod someone to make their library follow the conventions of stdlib more closely, that's not a bad thing either.

@alexdowad

This comment has been minimized.

Copy link
Contributor

commented Feb 21, 2016

So do we want to stop checking for size == 0 and only look for length == 0? @bbatsov?

@bbatsov

This comment has been minimized.

Copy link
Collaborator

commented Feb 21, 2016

I'm fine with making this configurable, but I'm not fine with checking only for length == 0.

@jcoyne

This comment has been minimized.

Copy link

commented Nov 29, 2016

I'm also hitting this with checking the size of an uploaded file (where file.tempfile is an instance of Tempfile):

file.tempfile.size == 0
@keithbro

This comment has been minimized.

Copy link

commented Nov 7, 2017

StringIO also.

@mikegee

This comment has been minimized.

Copy link
Contributor

commented Nov 12, 2017

It seems like a lot to ask, but it would be great if these classes implemented #empty?.

@alexdowad

This comment has been minimized.

Copy link
Contributor

commented Nov 15, 2017

@mikegee You can try to contribute patches to MRI Ruby. They accept PRs through GitHub now.

mikegee added a commit to mikegee/ruby that referenced this issue Nov 16, 2017

Add #empty? to Tempfile, StringIO, File::Stat
Rubocop prefers `empty?` over `length == 0` and `size == 0`, which is great for String, Array, Hash, etc. It would be nice if more classes implemented `#empty?` for consistancy.

See related discussion at rubocop-hq/rubocop#2841

mikegee added a commit to mikegee/ruby that referenced this issue Nov 16, 2017

Add #empty? to Tempfile, StringIO, File::Stat
Rubocop prefers `empty?` over `length == 0` and `size == 0`, which is great for String, Array, Hash, etc. It would be nice if more classes implemented `#empty?` for consistancy.

See related discussion at rubocop-hq/rubocop#2841

mikegee added a commit to mikegee/ruby that referenced this issue Nov 17, 2017

Add #empty? to Tempfile, StringIO, File::Stat
Rubocop prefers `empty?` over `length == 0` and `size == 0`, which is great for String, Array, Hash, etc. It would be nice if more classes implemented `#empty?` for consistancy.

See related discussion at rubocop-hq/rubocop#2841
@mikegee

This comment has been minimized.

Copy link
Contributor

commented Nov 30, 2017

I’m doing a bad job arguing for #empty? at https://bugs.ruby-lang.org/issues/14136 Does anybody else want to contribute to that discussion?

@bbatsov

This comment has been minimized.

Copy link
Collaborator

commented Sep 12, 2018

I've added a few comments myself, but I'm not holding my breath. Generally for this specific problem it seems to me we should discourage people from using File.stat to check for file emptiness.

@jcoyne

This comment has been minimized.

Copy link

commented Sep 17, 2018

Something having a 0 size doesn't necessarily imply emptiness unless the thing you have 0 of is the volume of a container. I think this is just over generalization of a solution onto too large of a problem space. This cop should be considered for removal.

Drenmi added a commit to Drenmi/rubocop that referenced this issue Sep 18, 2018

[Fix rubocop-hq#2841] Reduce Style/ZeroLengthPredicate false positives
Some collection like classes in the Ruby standard library implement
`#size` but not `#empty`. This change has the cop ignore known cases with
`Tempfile`, `StringIO`, and `File::Stat` objects to reduce false
positives.

Drenmi added a commit to Drenmi/rubocop that referenced this issue Sep 18, 2018

[Fix rubocop-hq#2841] Reduce Style/ZeroLengthPredicate false positives
Some collection like classes in the Ruby standard library implement
`#size` but not `#empty`. This change has the cop ignore known cases with
`Tempfile`, `StringIO`, and `File::Stat` objects to reduce false
positives.

Drenmi added a commit to Drenmi/rubocop that referenced this issue Sep 18, 2018

[Fix rubocop-hq#2841] Reduce Style/ZeroLengthPredicate false positives
Some collection like classes in the Ruby standard library implement
`#size` but not `#empty`. This change has the cop ignore known cases with
`Tempfile`, `StringIO`, and `File::Stat` objects to reduce false
positives.

@Drenmi Drenmi added the enhancement label Sep 18, 2018

@Drenmi

This comment has been minimized.

Copy link
Collaborator

commented Sep 18, 2018

To get somewhere with this, I created a patch to reduce known false positives that were reported in this thread. I think that's about the extent to what we can go with static analysis. For those who disagree with this cop in general, there's always the option to disable it. 🙂

@bbatsov bbatsov closed this in edc2014 Sep 18, 2018

maxh added a commit to maxh/rubocop that referenced this issue Oct 8, 2018

[Fix rubocop-hq#2841] Reduce Style/ZeroLengthPredicate false positives
Some collection like classes in the Ruby standard library implement
`#size` but not `#empty`. This change has the cop ignore known cases with
`Tempfile`, `StringIO`, and `File::Stat` objects to reduce false
positives.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.