rectorphp / type-perfect

Next level type declaration check PHPStan rules
https://getrector.com/blog/introducing-type-perfect-for-extra-safety
MIT License
74 stars 4 forks source link

Add flag to disable "Use instanceof instead of isset() on object" rule (`NoIssetOnObjectRule`) #26

Closed ZebulanStanphill closed 2 months ago

ZebulanStanphill commented 2 months ago

I have a lot of code that resembles this:

function myFunc(): Foo|null {
    // ...
}

function useFoo(Foo $foo) {
    // ...
}

$value = myFunc();
if (isset($value)) {
    useFoo($value);
}

NoIssetOnObjectRule suggests switching the isset() condition to the following:

if ($foo instanceof Foo) {

However, when taking static analysis into account, this might actually open the door to subtle bugs later on, since the "absent value" type (null, in this case) is lumped in with potentially unknown new value types. For example, if myFunc() was updated to begin returning Foo|Bar|null, the new possibility of Bar would automatically be treated the same as null. This could be desirable, or it could be a subtle bug, where we thought we were handling all non-empty values, but in fact were silently ignoring all except the one we already knew to be possible.

In contrast, the traditional isset() check allows all non-null values through, which means that, in the case of Bar becoming a possible value, it would automatically be let through, and any resulting type mismatches in the code that uses $value would be caught by the static analyzer.

For this reason, I would prefer to disable NoIssetOnObjectRule in my project, since although it would theoretically improve the type-safety of my code in its current state, it may also prove less safe in the long run due to functions expanding their signature over time.

TomasVotruba commented 2 months ago

Ideally, use ignore to skip this rule. It has the same effect.