-
Notifications
You must be signed in to change notification settings - Fork 471
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
base: 1.11.x
Are you sure you want to change the base?
Narrow down static and $this together #2972
Conversation
@@ -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.', |
There was a problem hiding this comment.
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.
9a98b7b
to
03edf69
Compare
I'm sorry, I don't fully understand this. I don't get why |
@ondrejmirtes It's not better. See #2972 (comment) |
03edf69
to
b494706
Compare
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 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
But I'm hoping that it's nevertheless an improvement over the previous implementation. |
b494706
to
1bd62a8
Compare
|
||
$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.', |
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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()); |
There was a problem hiding this comment.
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.
This pull request has been marked as ready for review. |
ddce768
to
499451b
Compare
499451b
to
db1d70a
Compare
db1d70a
to
43dab45
Compare
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:
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:
resolveTypeByName
? It returnsTypeWithClassName
, so it cannot return anIntersectionType
. Therefore, any usage withstatic
could result in the kind of bugs that this PR tries to fix.$scope->getType(new Expr\ClassConstFetch($className, 'class'))->getClassStringObjectType();
?