Fix chaining nullsafe operator

[olsavmic]

Resolves: phpstan/phpstan#5868

[olsavmic]

Default required in order to not break rules in phpstan-strict-rules and others

[ondrejmirtes]

Hi, can you please explain to me how is it possible that this fixes the bug? In current dev-master, we first call NullsafeOperatorHelper::getNullsafeShortcircuitedExpr($node->var) and RuleLevelHelper then gets the type:

phpstan-src/src/Rules/RuleLevelHelper.php

Line 141 in 95754d1

$type = $scope->getType($var);

In your version you basically do the same thing but getNullsafeShortcircuitedExpr is called inside findTypeToCheck. Maybe I'm tired, but I don't see the difference.

[olsavmic]

True, it's hard to grasp, I tried to explain it in phpstan/phpstan#5868 (comment)

The problem is that running NullsafeOperatorHelper::getNullsafeShortcircuitedExpr on $expr modifies the expression in such a way that it does not match the one saved in $scope->moreSpecificTypes.

There is a difference in how $method?->get()?->get() and $method?->get()->get().
One relies only on the moreSpecificTypes whereas the other needs the NullsafeOperatorHelper as it does not have the moreSpecificTypes information available.

So the previous version for expression $method?->get()?->getExisting()->get() worked like this:

• $method?->get() returns ObjectType|NULL, $var == $method which is ObjectType
• ($method?->get())?->getExisting() returns ObjectType|NULL, $var == $method?->get() which is ObjectType|NULL

Current fix works like this:

• $method?->get() returns ObjectType (thanks to $scope->moreSpecificType), $var == $method which is ObjectType
• ($method?->get())?->getExisting() returns ObjectType|NULL, $var == $method?->get() which is ObjectType (due to $scope->moreSpecificType)
• ($method?->get()?->getExisting())->get() return ObjectType|NULL, $var == $method?->get()?->getExisting() and this is where $scope->moreSpecificType fails, for regular MethodCall (and not NullsafeMethodCall) and NullsafeOperatorHelper is required.

I'll try to come up with real examples to make it more clear.

[ondrejmirtes]

Who saves it to moreSpecificTypes?

The fix I'm thinking of is that NullsafeOperatorHelper would have two public methods: getNullsafeShortcircuitedExpr and a new one with a different name, just so that we avoid boolean parameters.

People would still call NullsafeOperatorHelper in their specific rules when passing $var into RuleLevelHelper. Where you're passing false in your PR, the old getNullsafeShortcircuitedExpr method would be called.

Where you're passing true, the new method would be called. What it would do is it'd inspect the last part of the chain, exchanged it for MethodCall and PropertyFetch, but it would note in the node's attributes that this used to be a nullsafe node.

RuleLevelHelper would inspect $var for this attribute and it it would find it, it would remove NullType from the var type.

WDYT?

[olsavmic]

NodeScopeResolver::2025, I don't really understand the logic here though yet, I just came to this conclusion by step-by-step debugging.

I don't like that we have to deal with "shallow non-nullability" on two levels (in NodeScopeResolver && specific rules). I'd say it should be possible to modify nested scope similarly to how it's done for NullsafeMethodCalls for MethodCall->var as well 🤔

Regarding your proposed solution, in the case of passing false, the expression does not have to be modified I think. Yes, I think we can do that if you're OK with saving extra node attribute.

[ondrejmirtes]

Yes, please try it. Right now it looks like that it's only luck that it works so I'd like something more explicit :)

[olsavmic]

@ondrejmirtes Check it now please, I did not do it the "attribute-way" because the decision is based on the top-level expression and not the $expr->var (or other, which is passed to the NullsafeOperatorHelper).

I'd prefer to come up with a different name for the NullsafeOperatorHelper methods though 🤔 Any ideas?

[ondrejmirtes]

Thank you, this is great!