Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Narrow down static and $this together #2972

Open
wants to merge 13 commits into
base: 1.10.x
Choose a base branch
from

Conversation

schlndh
Copy link
Contributor

@schlndh schlndh commented Mar 17, 2024

This is an attempt to fix https://phpstan.org/r/7df004e9-cbcd-41b8-a882-363914a91c1a
It also fixes the 1st part of phpstan/phpstan#9465
Fixes phpstan/phpstan#5987

I encountered this use-case here: #2940 (comment) . 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();?

@@ -181,30 +181,30 @@ public function testImpossibleCheckTypeFunctionCall(): void
631,
],
[
'Call to function method_exists() with \'CheckTypeFunctionCall\\\\MethodExistsWithTrait\' and \'method\' will always evaluate to true.',
'Call to function method_exists() with class-string<static(CheckTypeFunctionCall\MethodExistsWithTrait)> and \'method\' will always evaluate to true.',
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the previous message was better, but since there is already $this(...) in the messages above, I decided to ignore it.

@schlndh schlndh force-pushed the feature-narrowStaticAndThisTogether branch from 9a98b7b to 03edf69 Compare March 17, 2024 09:27
@ondrejmirtes
Copy link
Member

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\'.

@schlndh
Copy link
Contributor Author

schlndh commented Mar 20, 2024

@ondrejmirtes It's not better. See #2972 (comment)

@schlndh schlndh force-pushed the feature-narrowStaticAndThisTogether branch from 03edf69 to b494706 Compare March 23, 2024 10:00
@schlndh
Copy link
Contributor Author

schlndh commented Mar 23, 2024

I added a fix for phpstan/phpstan#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 schlndh force-pushed the feature-narrowStaticAndThisTogether branch from b494706 to 1bd62a8 Compare April 12, 2024 13:05

$this->analyse([__DIR__ . '/../Properties/data/bug-instanceof-static-vs-this-property-assign.php'], [
[
'Parameter #1 $var is passed by reference so it does not accept readonly property BugInstanceofStaticVsThisPropertyAssign\FooChild::$nativeReadonlyProp.',
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unrelated to this PR: It should probably complain about @readonly properties as well.

{
if ($this instanceof FooChild) {
$this::$prop = 5;
\PHPStan\Testing\assertType('5', $this::$prop);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally, this should work even if the write is via $this:: and the read is via static::, and vice-versa, but for now it doesn't.

if ($this::isNull($v)) \PHPStan\Testing\assertType('null', $v);
if (static::isNull($v)) \PHPStan\Testing\assertType('null', $v);

if ($this::foo() === 5) \PHPStan\Testing\assertType('5', $this::foo());
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally, this should work for any combination of $this::, static:: and $this:: (private methods might be an issue) for read/write. But for now it doesn't.

@schlndh schlndh marked this pull request as ready for review April 14, 2024 12:32
@phpstan-bot
Copy link
Collaborator

This pull request has been marked as ready for review.

@schlndh schlndh force-pushed the feature-narrowStaticAndThisTogether branch from ddce768 to 499451b Compare May 3, 2024 10:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants