Change array<int...> to list<...> in PHP doc #114
Conversation
| @@ -47,7 +47,7 @@ public function once(string $eventName, callable $callBack, int $priority = 100) | |||
| * Lastly, if there are 5 event handlers for an event. The continueCallback | |||
| * will be called at most 4 times. | |||
| * | |||
| * @param array<int, mixed> $arguments | |||
| * @param list<mixed> $arguments | |||
There was a problem hiding this comment.
not sure this will work with named arguments. do we have a test-case for that?
There was a problem hiding this comment.
If I change to array<int|string, mixed> then phpstan complains:
------ ----------------------------------------------------------------------------------------------------------------------
Line lib/EmitterTrait.php (in context of class Sabre\Event\Emitter)
------ ----------------------------------------------------------------------------------------------------------------------
80 Parameter #2 $parameters of function call_user_func_array expects array<int, mixed>, array<int|string, mixed> given.
91 Parameter #2 $parameters of function call_user_func_array expects array<int, mixed>, array<int|string, mixed> given.
------ ----------------------------------------------------------------------------------------------------------------------
phpstan thinks that the PHP function call_user_func_array must be passed just a "list" array (int keys).
But https://w ww.php.net/manual/en/function.call-user-func-array.php says:
"If any keys of args are strings, those elements will be passed to callback as named arguments, with the name given by the key."
Maybe phpstan needs to be adjusted so that it knows what can be passed to call_user_func_array ?
There was a problem hiding this comment.
not sure this will work with named arguments. do we have a test-case for that?
The tests only have cases with a single unnamed argument. Named arguments only became a thing in PHP 8.0 (I think). So that will be why there are no test cases for it.
Hmmm - so maybe the phpstan thing in comment above is because I am running phpstan on PHP 7.4.
I think that the code will "just work". But annotations and test case(s) will have to apply only for PHP 8.
Should we try to add official support for named arguments now?
There was a problem hiding this comment.
I remember working on this topic in phpstan/phpstan-src#2501 months ago
its a open issue: phpstan/phpstan#5934
phil-davis
force-pushed
the
phpdoc-list
branch
from
524c804 to
0c2d959
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #114 +/- ##
=========================================
Coverage 99.21% 99.21%
Complexity 111 111
=========================================
Files 7 7
Lines 380 380
=========================================
Hits 377 377
Misses 3 3 ☔ View full report in Codecov by Sentry. |
lib/Loop/Loop.php
Outdated
| @@ -88,7 +88,7 @@ public function setInterval(callable $cb, float $timeout): array | |||
| /** | |||
| * Stops a running interval. | |||
| * | |||
| * @param array<int, mixed> $intervalId | |||
| * @param array{0:string, 1:boolean} $intervalId | |||
There was a problem hiding this comment.
Is there a way to declare that the boolean is a reference to a boolean?
There was a problem hiding this comment.
not that I am aware of.
but maybe it makes sense to use a phpstan-type so one can see that we should pass something in here, which was returned by setInterval
| typeAliases: | ||
| IntervalStatus: 'array{0:string, 1:boolean}' |
There was a problem hiding this comment.
@staabm I found that I could not use a local type alias because of:
@phpstan-typedefined type doesn't work as a return value phpstan/phpstan#9424 - local type alias cannot be the return type of a function- I couldn't get it to work in https://github.com/sabre-io/event/blob/master/lib/Loop/functions.php - that code is not a class, and
@phpstan-import-typedoes not seem to be effective for "ordinary" code that is not a class.
So I made a global type alias. That works everywhere.
But will doing that be an issue for consumers of this? They will use PHPstan to analyse their use of setInterval and clearInterval, but their phpstan.neon will not know what IntervalStatus is. They might have to add this to their phpstan.neon
There was a problem hiding this comment.
There was a problem hiding this comment.
The older issue is phpstan/phpstan#9164
There was a problem hiding this comment.
the global type aliases will not work for library consumers.
(or only if they also define this alias in their config).
additionally the global alias might confuse IDEs which don't know about this stuff, as they would try to find a same named class.
not sure this is worth it since the functions/methods involved with this type are the actual api surface of our package. I think we should stop using it until phpstan supports using the local alias properly for functions.
|
What we can do, is done in PR #116 |
Ref: comment #113 (comment)