Fix incorrect type inference for @phpstan-assert-if-true on $this with union types #4246
Conversation
|
please have a look at the issue-bot github action job summary. it seems this PR also affects other issues. please add regression tests for those which are getting fixed |
|
Thanks, @staabm. I’ll review the issue-bot GitHub Action job summary and add regression tests for the additionally affected issues. This is my first time contributing to open source (and my first PR to PHPStan), so I’ll update this PR in small batches as I add tests. If you prefer a specific location or pattern for these tests, please let me know. |
|
Welcome 🤗 Type related tests go usually into the https://github.com/phpstan/phpstan-src/tree/2.1.x/tests/PHPStan/Analyser/nsrt/ directory. Files located in this directory will automatically checked against using assertType(). |
|
Regression tests are typically added to existing test classes for rules that reported the (disappeared) errors. Alternatively covering some bugs with simple type inference tests (in |
| @@ -0,0 +1,40 @@ | |||
| <?php declare(strict_types=1); | |||
|
|
|||
| namespace Bug13358; | |||
There was a problem hiding this comment.
You can move this also into nsrt/ folder
|
|
||
| use PHPStan\Testing\TypeInferenceTestCase; | ||
|
|
||
| class AssertIfTrueOnThisTest extends TypeInferenceTestCase |
There was a problem hiding this comment.
This file can be dropped when everything else moved into nsrt/
|
@staabm The reason I want to run it individually is because I’d like to debug a specific function and check only the information related to this test case, without running everything else. Could you tell me how to do that? Thanks! |
|
This pull request has been marked as ready for review. |
|
Added regression test for #11441 (bug-11441.php) and updated the description accordingly. |
| $assertions = Assertions::createEmpty(); | ||
|
|
||
| foreach ($this->methods as $method) { | ||
| $assertions = $assertions->intersectWith($method->getAsserts()); |
There was a problem hiding this comment.
This will merge assertions from both types. Which is a wrong operation. (The method has a misleading name but it's used in IntersectionTypeMethodReflection where it's appropriate.)
In a union type what needs to be done is we need to look at assertions from all places and do some kind of intersection, figure out the common assertions.
Let's say we have Foo|Bar and we call a method that's on both types.
One method does some kind of assertion and the other method does a different assertion. The correct performed assertion on the union is none because they have nothing in common.
So we need to look at on which expression the assertion is performed (in this case $this->getParam()) and if all involved methods on the union are doing an assertion on the same expression.
Then we need to take a look at the asserted type and also take it into account only if it's the same. (It's possible that they don't have to be the same but we'd have to figure out the right operation that we can be sure about.)
closes phpstan/phpstan#13358
closes phpstan/phpstan#11441
Summary
@phpstan-assert-if-trueon$thiswith union types produced incorrect type inference.Repro
See added test:
tests/PHPStan/Rules/Methods/AssertIfTrueOnThisTest.php.Cause
Union handling for
$thisassertions wasn’t propagated intoTypeSpecifier(assert-based refinement path).Fix
Normalize/refine
$thisunion members before producingSpecifiedTypesfrom assert info.Tests
AssertIfTrueOnThisTest.php.Also fixes #11441
tests/PHPStan/Analyser/nsrt/bug-11441.php.@phpstan-assert !null $this->getParam()works correctly when called on union types (Foo|Bar), where each class defines its own method.nulltype was not removed in such cases. After the fix,int|stringis correctly inferred.