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.

Sign up for GitHub

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

Change array<int...> to list<...> in PHP doc #114

Closed
wants to merge 3 commits into from
Closed

Change array<int...> to list<...> in PHP doc #114

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0c2d959
Change array<int...> to list<...> in PHP doc
phil-davis Nov 15, 2023
e24e559
Declare intervalId more specifically
phil-davis Nov 15, 2023
c778e0a
Add global type alias IntervalStatus
phil-davis Nov 19, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion lib/EmitterInterface.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

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

not sure this will work with named arguments. do we have a test-case for that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

I remember working on this topic in phpstan/phpstan-src#2501 months ago

its a open issue: phpstan/phpstan#5934

* @param callable():bool|null $continueCallBack
*/
public function emit(string $eventName, array $arguments = [], callable $continueCallBack = null): bool;
Expand Down
2 changes: 1 addition & 1 deletion lib/EmitterTrait.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -172,7 +172,7 @@ public function removeAllListeners(string $eventName = null): void
/**
* The list of listeners.
*
* @var array<string, array{0:boolean, 1:array<int, mixed>, 2:array<int, callable>}>
* @var array<string, array{0:boolean, 1:list<mixed>, 2:list<callable>}>
*/
protected array $listeners = [];
}
6 changes: 3 additions & 3 deletions lib/Loop/Loop.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ public function setTimeout(callable $cb, float $timeout): void
* The value this function returns can be used to stop the interval with
* clearInterval.
*
* @return array<int, mixed>
* @return IntervalStatus
*/
public function setInterval(callable $cb, float $timeout): array
{
Expand All @@ -88,7 +88,7 @@ public function setInterval(callable $cb, float $timeout): array
/**
* Stops a running interval.
*
* @param array<int, mixed> $intervalId
* @param IntervalStatus $intervalId
*/
public function clearInterval(array $intervalId): void
{
Expand Down Expand Up @@ -302,7 +302,7 @@ protected function runStreams(?float $timeout): void
/**
* A list of timers, added by setTimeout.
*
* @var array<int, array{0:float, 1:callable}>
* @var list<array{0:float, 1:callable}>
*/
protected array $timers = [];

Expand Down
4 changes: 2 additions & 2 deletions lib/Loop/functions.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ function setTimeout(callable $cb, float $timeout): void
* The value this function returns can be used to stop the interval with
* clearInterval.
*
* @return array<int, mixed>
* @return IntervalStatus
*/
function setInterval(callable $cb, float $timeout): array
{
Expand All @@ -28,7 +28,7 @@ function setInterval(callable $cb, float $timeout): array
/**
* Stops a running interval.
*
* @param array<int, mixed> $intervalId
* @param IntervalStatus $intervalId
*/
function clearInterval(array $intervalId): void
{
Expand Down
2 changes: 1 addition & 1 deletion lib/Promise.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -202,7 +202,7 @@ public function wait()
* A list of subscribers. Subscribers are the callbacks that want us to let
* them know if the callback was fulfilled or rejected.
*
* @var array<int, array{0:Promise<TReturn>, 1:callable|null, 2:callable|null}>
* @var list<array{0:Promise<TReturn>, 1:callable|null, 2:callable|null}>
*/
protected array $subscribers = [];

Expand Down
6 changes: 3 additions & 3 deletions lib/WildcardEmitterTrait.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -216,14 +216,14 @@ public function removeAllListeners(string $eventName = null): void
/**
* The list of listeners.
*
* @var array<string, array<int, array{0:int, 1:callable}>>
* @var array<string, list<array{0:int, 1:callable}>>
*/
protected array $listeners = [];

/**
* The list of "wildcard listeners".
*
* @var array<string, array<int, array{0:int, 1:callable}>>
* @var array<string, list<array{0:int, 1:callable}>>
*/
protected array $wildcardListeners = [];

Expand All @@ -233,7 +233,7 @@ public function removeAllListeners(string $eventName = null): void
*
* If the list of listeners changes though, the index clears.
*
* @var array<string, array<int, callable>>
* @var array<string, list<callable>>
*/
protected array $listenerIndex = [];
}
2 changes: 2 additions & 0 deletions phpstan.neon
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
parameters:
level: 9
phpVersion: 70430 # PHP 7.4.30
typeAliases:
IntervalStatus: 'array{0:string, 1:boolean}'
Comment on lines +4 to +5
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@staabm I found that I could not use a local type alias because of:

  1. @phpstan-type defined type doesn't work as a return value phpstan/phpstan#9424 - local type alias cannot be the return type of a function
  2. 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-type does 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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

phpstan/phpstan#10154

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The older issue is phpstan/phpstan#9164

Copy link
Member

@staabm staabm Nov 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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.

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 made PR #116 that has the code from here, without the last commit. Please review that. Then I can close this PR #114 - leaving this discussion.

ignoreErrors:
-
message: "#^If condition is always true.$#"
Expand Down