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

[2.x] Add basic template annotations #227

Closed
wants to merge 1 commit into from
Closed

[2.x] Add basic template annotations #227

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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: 2 additions & 0 deletions .gitattributes
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
/.gitattributes export-ignore
/.github/ export-ignore
/.gitignore export-ignore
/phpstan.neon.dist export-ignore
/phpunit.xml.dist export-ignore
/phpunit.xml.legacy export-ignore
/tests/ export-ignore
/types/ export-ignore
15 changes: 15 additions & 0 deletions .github/workflows/ci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,3 +45,18 @@ jobs:
- run: composer self-update --2.2 # downgrade Composer for HHVM
- run: hhvm $(which composer) install
- run: hhvm vendor/bin/phpunit

PHPStan:
name: PHPStan
runs-on: ubuntu-20.04
strategy:
matrix:
php:
- 8.1
steps:
- uses: actions/checkout@v3
- uses: shivammathur/setup-php@v2
with:
php-version: ${{ matrix.php }}
- run: composer require phpstan/phpstan
- run: vendor/bin/phpstan
4 changes: 4 additions & 0 deletions phpstan.neon.dist
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
parameters:
paths:
- types
level: max
8 changes: 6 additions & 2 deletions src/PromiseInterface.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,9 @@

namespace React\Promise;

/**
* @template T
*/
interface PromiseInterface
{
/**
Expand Down Expand Up @@ -32,10 +35,11 @@ interface PromiseInterface
* than once.
* 3. `$onProgress` (deprecated) may be called multiple times.
*
* @param callable|null $onFulfilled
* @template TReturn of mixed
* @param callable(T): TReturn $onFulfilled
Copy link

Choose a reason for hiding this comment

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

No longer marked as nullable?

* @param callable|null $onRejected
* @param callable|null $onProgress This argument is deprecated and should not be used anymore.
* @return PromiseInterface
* @return (TReturn is PromiseInterface ? TReturn : PromiseInterface<TReturn>)
Copy link
Member

Choose a reason for hiding this comment

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

How should we handle cases such as this?

$in = resolve(mt_rand(0, 100));
$out = $in->then(function (int $n): float {
    if ($n === 0) {
        throw new \UnexpectedValueException();
    }
    return 1 / $n;
});

I see you did some work regarding type assertions in phpstan/phpstan#7371, perhaps this is something we should include as part of this PR? 👍

Copy link
Member Author

Choose a reason for hiding this comment

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

Could do that, but then just the basics, and not the more complicated cases 👍 .

*/
public function then(callable $onFulfilled = null, callable $onRejected = null, callable $onProgress = null);
Copy link
Member

Choose a reason for hiding this comment

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

I see this currently only affects the then() method, should we also add this for the methods defined in ExtendedPromiseInterface? (Despite being removed in Promise v3 via #75.)

Copy link
Member Author

Choose a reason for hiding this comment

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

This is purposely only covering then and not the other methods. In #223 I want to expand that to other methods. But the primary goal is to get this ready for reactphp/async#40 and the minimum we need for that is support only on the then method. If we can add more methods before that release of that I'm all up for it. But if we don't I propose we leave it out.

}
5 changes: 3 additions & 2 deletions src/functions.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,9 @@
*
* If `$promiseOrValue` is a promise, it will be returned as is.
*
* @param mixed $promiseOrValue
* @return PromiseInterface
* @template T
* @param T $promiseOrValue
* @return PromiseInterface<T>
*/
function resolve($promiseOrValue = null)
{
Expand Down
9 changes: 9 additions & 0 deletions types/PromiseInterface.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
<?php

use function PHPStan\Testing\assertType;
use function React\Promise\resolve;

$passThroughBoolFn = static fn (bool $bool): bool => $bool;

assertType('React\Promise\PromiseInterface<bool>', resolve(true));
assertType('React\Promise\PromiseInterface<bool>', resolve(true)->then($passThroughBoolFn));