phpstan-src
phpstan-src copied to clipboard
Narrow down static and $this together
This is an attempt to fix https://phpstan.org/r/7df004e9-cbcd-41b8-a882-363914a91c1a It also fixes the 1st part of https://github.com/phpstan/phpstan/issues/9465 Fixes https://github.com/phpstan/phpstan/issues/5987
I encountered this use-case here: https://github.com/phpstan/phpstan-src/pull/2940#issuecomment-1962383225 . IMO it's a bit of an edge-case, but it feels like phpstan should understand it.
I found one case where this change replaces one bug with another one:
In non-static contexts, the called class will be the class of the object instance. Since $this-> will try to call private methods from the same scope, using static:: may give different results. Another difference is that static:: can only refer to static properties.
https://www.php.net/manual/en/language.oop5.late-static-bindings.php https://3v4l.org/SuYOd#v8.3.4 https://phpstan.org/r/763ec51a-49b0-4ba1-a037-b9939ef3cbd4
Previously phpstan thought that static::fooPrivate() inside the ifs would still be string (private), but it should be int (public). With my changes it presumably gets affected by the same bug that affects $this in the first if, so it now thinks that it's *NEVER*.
Since it is already bugged and probably non-trivial to fix, I decided to ignore it for now.
Some open questions:
- Should something be done with
resolveTypeByName? It returnsTypeWithClassName, so it cannot return anIntersectionType. Therefore, any usage withstaticcould result in the kind of bugs that this PR tries to fix. - Should there be a shortcut method for
$scope->getType(new Expr\ClassConstFetch($className, 'class'))->getClassStringObjectType();?
I'm sorry, I don't fully understand this. I don't get why class-string<static(CheckTypeFunctionCall\MethodExistsWithTrait)> should be better than \'CheckTypeFunctionCall\\\\MethodExistsWithTrait\'.
@ondrejmirtes It's not better. See https://github.com/phpstan/phpstan-src/pull/2972#discussion_r1527450851
I added a fix for https://github.com/phpstan/phpstan/issues/5987 (because previously it was a bit more broken compared to 1.10.x). But I can't say that I'm confident that I didn't break something else as a result.
My understanding is that the closure in Closure::bind should be analyzed as if it was a method in $newScope, and it's contents was wrapped in if ($this instanceof $newThis::class) (and similarly for other possible usages). So that's what I tried to implement. But it's not really possible for me to understand all the consequences. I encountered several surprises (e.g. the phpdoc resolving), which I (hopefully) fixed. But chances are that things which are not covered by tests and issue bot, may have gotten broken.
Additionally, there are likely many bugs/inconsistencies remaining. For example https://phpstan.org/r/8a986f5e-dc04-4fdb-8ac7-aae2c820e0dc gives the following result in my branch (i.e. inconsistent static(...) and $this(...) types):
------ -----------------------------------------------------------------------------------------
22 Expected type $this(NodeScopeResolverClosureBind\Bar), actual:
NodeScopeResolverClosureBind\Bar
23 Expected type class-string<static(NodeScopeResolverClosureBind\Bar)>, actual:
class-string<NodeScopeResolverClosureBind\Bar&static(NodeScopeResolverClosureBind\Foo)>
32 Expected type $this(NodeScopeResolverClosureBind\Bar), actual:
NodeScopeResolverClosureBind\Bar
33 Expected type class-string<static(NodeScopeResolverClosureBind\Bar)>, actual:
class-string<NodeScopeResolverClosureBind\Bar&static(NodeScopeResolverClosureBind\Foo)>
------ -----------------------------------------------------------------------------------------
But I'm hoping that it's nevertheless an improvement over the previous implementation.
This pull request has been marked as ready for review.