Swiftlint: Fix empty_count false-positives

[jhanzo]

When using a NSMapTable variable and test number of elements with count statement, Empty Count Violation is thrown whereas it should not. Indeed NSMapTable hasn't any isEmpty property.

XCode 7.3.1, iOS 9.3, swiftlint 0.11.1

[jhanzo]

A workaround would be using :

mapTable.objectEnumerator()!.allObjects.isEmpty

[masters3d]

How about

extension NSMapTable {
    var isEmpty:Bool { return count == 0 }
}

[jhanzo]

Of course, but as an other workaround. Because of Swiftlint is an integration tool, you could expect to not being required to add some extra code.

[marcelofabri]

Note that this rule is opt-in because of the false positives. You can always disable it.

There's no easy way to know a variable type without compiling the code (as far as I know) and SwiftLint does not compile any code currently.

[jpsim]

@marcelofabri is exactly right, the reason why the empty_count rule is opt-in is due to these false positives, which can't be avoided unless SwiftLint was able to resolve the type of instances on which .count is being accessed, which would be possible with a new mode we're considering for SwiftLint described in https://github.com/realm/SwiftLint/pull/829#issuecomment-263008789.

Until then, be aware of the false-positives, and don't use opt-in rules unless you're aware of their tradeoffs and find them acceptable.

[jonasman]

This is by design as the rule should be able to catch usages of count inside Array extensions for example. See #827 (comment) for more details.

Well i think we make the empty_rule useless just to support an edge case in extensions.
I had to add many disable comments to make it to work and in the end we disabled the rule all together.

[DagAgren]

From issue #1080, the rule is triggered on if count > 0 {. This does not seem like it should ever be correct. This is not accessing a property, it's using a variable. The check should require that count is preceded by a period.

(This would miss cases in subclasses that implement count, but this seems a much smaller corner case, and the ability to use count as a variable name should take priority.)

[jpsim]

This is intentional, as extensions to collection types implementing isEmpty should also prefer using it over comparing count to zero.

[jonasman]

@jpsim do you have any data on the number of extension code using isEmpty vs non extension code using it?

If the numbers are too different then it would be good to reconsider this "intentional" behaviour.

[marcelofabri]

Matching only .count could be easily implemented as a configuration too.

[jpsim]

Given that empty_count is an opt-in rule, a higher number of false positives should be tolerated if it also improves the covered surface area of the "true positives" it detects.

However, I'd be happy to accept a PR that made the rule configurable to only trigger when used as a property lookup with a period (.count). Though I'd prefer if that setting was off by default.

[DagAgren]

@jpsim Well, I'd say the covered surface area suffers when people turn the rule off due to false positives. A more focused rule is a more used rule.

[jpsim]

Please see my last comment about being open to support the mode you want if a PR is submitted.

[phoney]

Sorry about the duplicate issue https://github.com/realm/SwiftLint/issues/1241 Needless to say I think this should be fixed. Obviously multiple developers are running into this issue.

Being configurable so that it only warns on .count would be a viable workaround that I would certainly enable.

[marcelofabri]

@phoney Feel free to send a PR ☺️

[danielbyon]

Hi, my issue was a duplicate of this one, so I just created this PR to fix it: https://github.com/realm/SwiftLint/pull/1608

[fredpi]

Still no fix for this?

A working regex should trigger for .count outside the scope of anything conforming to Collection and to count (without a dot) within such a scope. This would be rather difficult if not impossible to catch with a regex, considering Collection may be implemented via an extension in a different file e. g....

[bengovernment]

So is the position of the maintainers that one shouldn't use "count" as a variable name?

[jpsim]

The position of _this_ maintainer is that the false positives do prove useful for catching more violations of this, and that we would ideally solve this by making the empty count rule configurable to avoid these false positives by default while being able to opt in to behavior that can catch more violations, at the expense of false positives.

[davisford]

It's worse than that -- if you have a variable named count, it triggers this rule which is ridiculous. It should only be triggered off a collection type, right? That's what it is complaining about, if I understand correctly: [].count == 0 vs. [].isEmpty()

[itaylorm]

I ran into this problem using PHFetchResult for Photo Kit

I was able to get the extension solution to work with a couple of needed changes

1. I had to make it a method
2. I had to add @objc on the front of the method or you get an error "Extension of a generic Objective-C class cannot access the class's generic parameters at runtime"
3. I still had to add the disable lines but at least it is only in the extension not every time I need to check results

extension PHFetchResult {

@objc func isEmpty() -> Bool {
    // swiftlint:disable empty_count
    return self.count == 0
    // swiftlint:enable empty_count
}

}

[stale[bot]]

This issue has been automatically marked as stale because it has not had any recent activity. Please comment to prevent this issue from being closed. Thank you for your contributions!