Allow undefined property fetch of ObjectWithoutClassType in Coalesce

[rajyan]

fixes phpstan/phpstan#3659
fixes phpstan/phpstan#6026
fixes phpstan/phpstan#6809
fixes phpstan/phpstan#4559

This PR relaxes the condition of UndefinedExpressionAllowed for Coalesce slightly, to allow undefined property fetch of ObjectWithoutClassType in Coalesce.

This can be useful in cases like

class HelloWorld
{
    public function test(string $data): void
    {
        $obj = json_decode($data);
        if (!is_object($obj)) {
            return;
        }
        $val = $obj->prop ?? "test";
    }
}

which currently reports an error https://phpstan.org/r/fbf356da-5a24-43a3-a5a2-1638afa9dd99

As seen in https://github.com/rajyan/phpstan-src/blob/c6b6aa702554e5ff14591bae130f842ebd51357e/tests/PHPStan/Rules/Properties/data/dynamic-properties.php#L7-L18 the current behavior for dynamic properties in Coalesce is kept as is.

Changed to fix by making coalesce and isset consistent.

[rajyan]

Failing tests are unrelated to this pull request.

I believe it’s related to 58d7df3

[ondrejmirtes]

This feels very hacky. What about asking that Type::hasProperty() is not no()?

[rajyan]

Thank you for your comment.

What about asking that Type::hasProperty() is not no()?

I think checking by hasProperty would make allowing dynamic properties.

What this PR is trying is to solve is to allow undefined property fetches with object while leaving the current strict behavior for dynamic property, those are almost same except the class so it inevitably becomes hacky...

$obj = returnsObj();
$val = $obj->prop ?? "foo"; // obj maybe hasProperty

$this->dynamicProp ??  "foo"; // $this maybe hasProperty if object class is not final
$this->dynamicPropTypo ??  "foo"; // want to disallow dynamic properties to catch these typoes

The difference is, you have no prop information anyway for object, while you can check for props with class objects.

Maybe object shape feature phpstan/phpstan#6892 is a better way to go for phpstan/phpstan#6026, but this still cant solve phpstan/phpstan#3659 nor use cases with json_decode

[ondrejmirtes]

Alright, I changed my mind about this, we really need to clean this up. I'm fine with any changes as long as they remove false positives and do not introduce new errors.

We need to make isset() and ?? consistent with each other. By default isset($foo->nonexistentProp) and $foo->nonexistentProp must be kept silent because it's valid code and the core PHPStan must not be opinionated by default. I usually go by this philosophy but the isset/?? difference is currently an exception which I want to fix.

But I see two possible opportunities here:

• This PHP RFC for 8.2 https://wiki.php.net/rfc/deprecate_dynamic_properties disallows dynamic properties. So on this PHP version going forward, we should report an error for both isset($foo->nonexistentProp) and $foo->nonexistentProp for classes of $foo without #[AllowDynamicProperties]. We can implement this after PHP 8.2 feature freeze.
• There should be a stricter behaviour option that would be closer to the current behaviour in a stable version. Essentially it would mean that $foo->prop ?? 'bar' is not an alias for isset($foo->prop) but rather for $foo->prop !== null.

Currently I'd also say that this piece of code in ObjectType::hasProperty() is wrong:

		if ($classReflection->isFinal()) {
			return TrinaryLogic::createNo();
		}

Because the possibility of property existence isn't about subclasses, it's about dynamic properties which can be assigned to final classes too. The only situation where ObjectType::hasProperty() should say no is on PHP 8.2 without #[AllowDynamicProperties].

[rajyan]

Okay, thanks for your decision. I'll fix the incompatibility with isset and coalesce and add some regression tests from the related issues.

[rajyan]

We need a little more fix to solve issues like phpstan/phpstan#3171 (there are some couple more related issues).
I'll send a fix with a separate pull request.

Also, I have an idea with

This PHP RFC for 8.2 https://wiki.php.net/rfc/deprecate_dynamic_properties disallows dynamic properties. So on this PHP version going forward, we should report an error for both isset($foo->nonexistentProp) and $foo->nonexistentProp for classes of $foo without #[AllowDynamicProperties]. We can implement this after PHP 8.2 feature freeze.

using allowedUndefinedExpression so I'll create a pull request for that too.

[ondrejmirtes]

Thank you very much! Can you please add a bool option to NodeScopeResolver to optionally restore the original behaviour? With ?? not allowing undefined stuff.