-
Notifications
You must be signed in to change notification settings - Fork 540
POC: Use SimultaneousTypeTraverser in RuleLevelHelper::transformAcceptedType #4210
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
base: 2.1.x
Are you sure you want to change the base?
Conversation
266d55e
to
16685d2
Compare
The output from issue-bot is yummy here! https://github.com/phpstan/phpstan-src/actions/runs/16773630245 Please add regression tests for those. I feel like we could improve I feel that even when there's This needs extensive tests of course. |
Linking phpstan/phpstan#9096 |
Sure, but shouldn't we fix the actual tests first ?
Yeah, but I dunno how to improve the implementation of the existing traverseSimultaneously, it's not easy :/ |
16685d2
to
994ee8d
Compare
I feel like 6597ef6#r167671380 is helping us. And maybe same should be done for IntersectionType |
Yeah, I'm thinking about how to implement it. |
Right now I have this failing test: diff --git a/tests/PHPStan/Type/SimultaneousTypeTraverserTest.php b/tests/PHPStan/Type/SimultaneousTypeTraverserTest.php
index 76dc038873..750a7a56bd 100644
--- a/tests/PHPStan/Type/SimultaneousTypeTraverserTest.php
+++ b/tests/PHPStan/Type/SimultaneousTypeTraverserTest.php
@@ -125,6 +125,13 @@ class SimultaneousTypeTraverserTest extends PHPStanTestCase
$chooseScalarSubtype,
"'aaa'|Foo",
];
+
+ yield [
+ 'list<string|null>',
+ 'array<string>',
+ $chooseScalarSubtype,
+ 'list<string>',
+ ];
}
/** |
Alright here's something 0693e5a But I don't think it works well for constant arrays for example or other situations, but some use cases could be solved now. |
994ee8d
to
69bbbe0
Compare
Isn't it weird to only have the case
|
Well it might not even be needed, depends on how the callback is written. You can send me some failing tests with expectations. |
Current status, 2 errors:
First one is maybe an Intersection type with |
Static analysis is failing because TypeTraverserInstanceofVisitor doesn't support SimultaneousTypeTraverser (not sure it should).
The only current test failing is
but I feel like we could reproduce more errors because...
UnionType/Intersection types doesn't work well with the SimultaneousTypeTraverser because
A&B&C
andA
we're not traversing because second type is not an IntersectionA&B&C
andA&C
we're not traversing because types count are differentThe main issue would be, when traversing
A&B&C
andA&C
, to match types in order to