Hi community ,
To be honest, I am not 100% sure if I am posting this at the correct place.
- What language is this for?
- Which rule?
- PHP static code analysis: "empty()" should be used to test for emptiness (or Pull Request that introduces that rule)
- Why do you believe it’s a false-positive/false-negative?
- Not a false-positive but rather a confusing (maybe misleading and incomplete) description.
- Are you using
- SonarCloud
- SonarQube - 10.0.0.68432
-
SonarLint - Jetbrains Intellij/PHPStorm (plugin version 8.2.0.68615)
- in connected mode with SonarQube or SonarCloud?
Why do I think this rule is confusing/misleading?
count
works with arrays and countable objects. Using count
throws a TypeError
if called with any other type as parameter. Replacing count
with empty
would remove such implicit fail-safe - empty
accepts Strings, too.
After reading the PR changes it is not clear to me whether this rule only applies to variables that has been clearly identified as array or if it would also raise issues for countable objects. If the latter, empty
would be no valid replacement as it returns false
for countable objects.
Countable objects code sample
<?php
class CountMe implements Countable
{
protected int $myCount = 0;
public function count(): int
{
return $this->myCount;
}
}
$countable = new CountMe();
var_dump(count($countable) === 0); // returns 0
var_dump(empty($countable)); // returns false
Suggested changes (discussion very welcome)
In case rule RSPEC-1155
only applies to arrays, it should be mentioned in the rule description. Maybe even referencing the very similar rule RSPEC-3981
. Would it also be more type safe to check the emptiness of an array by comparing with an empty array instead of empty
? If the variable does not hold an array, there should already be other checks conditions.
<?php
// code that provides $variableUnderTest
// ...
if ($variableUnderTest === []) {
// handle empty array case
}
Best regards,
Steven