phpstan-src icon indicating copy to clipboard operation
phpstan-src copied to clipboard

Narrow down static and $this together

Open schlndh opened this issue 1 year ago • 4 comments

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 returns TypeWithClassName, so it cannot return an IntersectionType. Therefore, any usage with static could 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();?

schlndh avatar Mar 17 '24 09:03 schlndh

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 avatar Mar 20 '24 08:03 ondrejmirtes

@ondrejmirtes It's not better. See https://github.com/phpstan/phpstan-src/pull/2972#discussion_r1527450851

schlndh avatar Mar 20 '24 14:03 schlndh

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.

schlndh avatar Mar 23 '24 10:03 schlndh

This pull request has been marked as ready for review.

phpstan-bot avatar Apr 14 '24 12:04 phpstan-bot