ZeroLengthPredicate: gives false warning on File.stat

[perlun]

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 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]

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]

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]

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]

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

[bbatsov]

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

[jcoyne]

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]

StringIO also.

[mikegee]

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

[alexdowad]

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

[mikegee]

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]

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]

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]

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. 🙂