psalm
psalm copied to clipboard
vimeo
Feature: RedundantCondition for unnecessary !empty checks
https://psalm.dev/r/f2049080ac
the !empty( $foo ) should give a RedundantConditionGivenDocblockType error
(there are some additional cases with !empty/empty similar)
Same for:
if ( empty( $foo ) && empty( $foo['bar'] ) ) { // unnecessary empty( $foo ) &&
if ( !empty( $foo ) && !empty( $foo['bar'] ) ) { // unnecessary !empty( $foo ) &&
if ( empty( $foo ) || empty( $foo['bar'] ) ) { // unnecessary empty( $foo ) ||
if ( !empty( $foo ) || !empty( $foo['bar'] ) ) { // unnecessary || !empty( $foo['bar'] )
if ( empty( $foo ) && !isset( $foo['bar'] ) ) { // unnecessary && !isset( $foo['bar'] )
if ( empty( $foo ) || !isset( $foo['bar'] ) ) { // unnecessary && !isset( $foo['bar'] )
I found these snippets:
https://psalm.dev/r/f2049080ac
<?php
/** @var string $foo */
if ( !empty( $foo ) && $foo === 'hello' ) {
}
Psalm output (using commit 028ac7f):
No issues!
![psalm-github-bot[bot] avatar](https://avatars.githubusercontent.com/in/48546?v=4 "Published by a pub.dev verified publisher"){kind=small}
I don't think we support these sorts of redundancy checks at all, if a check is non-redundant at the point when it happens, but it's still redundant overall we don't seem to catch it: https://psalm.dev/r/c576174634 https://psalm.dev/r/21d498354f
I'm also not sure if we should support that. At the very least, we would have to only perform that analysis when only pure functions are involved.
I found these snippets:
https://psalm.dev/r/c576174634
<?php
/** @var string $foo */;
if ($foo !== "bar" && $foo === "baz") {
}
Psalm output (using commit 028ac7f):
No issues!
https://psalm.dev/r/21d498354f
<?php
/** @var string $foo */;
if ($foo === "baz" && $foo !== "bar") {
}
Psalm output (using commit 028ac7f):
ERROR: DocblockTypeContradiction - 4:23 - 'bar' does not contain 'baz'
ERROR: RedundantConditionGivenDocblockType - 4:5 - Docblock-defined type 'baz' for $foo is never =string(bar)
It already works correctly with "isset" (in all variations): https://psalm.dev/r/fac510dec4 It only needs to be extended, so it works with empty too.
Just adding "empty" would probably cover 95% of use cases of this anyway, so maybe just go with empty (don't even need to extend it to all pure functions to keep it simple)
I found these snippets:
https://psalm.dev/r/fac510dec4
<?php
/** @var string $foo */
if ( !isset( $foo ) && $foo === 'hello' ) {
}
Psalm output (using commit 028ac7f):
ERROR: DocblockTypeContradiction - 4:6 - Cannot resolve types for $foo - docblock-defined type string does not contain null
ERROR: TypeDoesNotContainType - 4:24 - 'hello' cannot be identical to null
ERROR: TypeDoesNotContainType - 4:6 - Type null for $foo is never =string(hello)
That's different, your isset example is already contradictory even without the $foo === "hello" check (or redundant if you remove the negation). If you change it so that it's not redundant you end up with the same problem, the expression isn't considered redundant because it's only redundant when considering the entire statement, rather than considering the expressions from left to right.
I found these snippets:
https://psalm.dev/r/7e0c7aba25
<?php
/** @var string $foo */
if ( !isset( $foo ) ) {
}
Psalm output (using commit 028ac7f):
ERROR: DocblockTypeContradiction - 4:7 - Cannot resolve types for $foo - docblock-defined type string does not contain null
https://psalm.dev/r/7b68e4e3a3
<?php
/** @var string $foo */
if ( isset( $foo ) ) {
}
Psalm output (using commit 028ac7f):
ERROR: RedundantConditionGivenDocblockType - 4:6 - Docblock-defined type string for $foo is never null
https://psalm.dev/r/0bf2a870d9
<?php
/** @var string|null $foo */
if ( isset( $foo ) ) {
}
Psalm output (using commit 028ac7f):
No issues!
https://psalm.dev/r/a069aad2ee
<?php
/** @var string|null $foo */
if ( isset( $foo ) && $foo === 'hello' ) {
}
Psalm output (using commit 028ac7f):
No issues!