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.

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

[WIP][2.x] Add template annotations #223

Draft
wants to merge 1 commit into
base: 2.x
Choose a base branch
from

Conversation

WyriHaximus
Copy link
Member

No description provided.

@WyriHaximus WyriHaximus added this to the v2.10.0 milestone May 19, 2022
@WyriHaximus WyriHaximus force-pushed the 2.x-add-template-annotations branch 4 times, most recently from b15cc8d to 82b32eb Compare June 2, 2022 07:38
@WyriHaximus WyriHaximus force-pushed the 2.x-add-template-annotations branch 2 times, most recently from 9ab066b to b87eda7 Compare June 9, 2022 14:16
@WyriHaximus
Copy link
Member Author

FYI just filed #227 and #228

Both are a getting support out there and then improve on it kind of PR.

@zmitic
Copy link

zmitic commented Jan 6, 2023

Sorry for barging but I am really interested in this PR and would like to remove my own stubs. Should I wait for merge or use PR branch instead?

@WyriHaximus
Copy link
Member Author

@zmitic Honestly same, will have a look at this again during the weekend and hopefully, this got resolved in recent PHPStan updates. But currently support for mixed scalar and scalar through PromiseInterface is blocking this

@WyriHaximus
Copy link
Member Author

Opened phpstan/phpstan#8707 to hopefully get the last few functions fully in

WyriHaximus added a commit to WyriHaximus-labs/http that referenced this pull request Jan 21, 2023
The fact that a promise can also be rejected with a Throwable and/or Exception is implied and there is no need to also define that here.

Refs: reactphp/promise#223
WyriHaximus added a commit to WyriHaximus-labs/promise-stream that referenced this pull request Jan 21, 2023
The fact that a promise can also be rejected with a Throwable and/or Exception is implied and there is no need to also define that here.

Refs: reactphp/promise#223
WyriHaximus added a commit to WyriHaximus-secret-labs/promise-timer that referenced this pull request Jan 21, 2023
The fact that a promise can also be rejected with a Throwable and/or Exception is implied and there is no need to also define that here.

Refs: reactphp/promise#223
WyriHaximus added a commit to WyriHaximus-labs/dns that referenced this pull request Jan 21, 2023
The fact that a promise can also be rejected with a Throwable and/or Exception is implied and there is no need to also define that here.

Refs: reactphp/promise#223
WyriHaximus added a commit to WyriHaximus-labs/dns that referenced this pull request Jan 24, 2023
The fact that a promise can also be rejected with a Throwable and/or Exception is implied and there is no need to also define that here.

Refs: reactphp/promise#223
WyriHaximus added a commit to WyriHaximus-labs/promise-stream that referenced this pull request Jan 24, 2023
The fact that a promise can also be rejected with a Throwable and/or Exception is implied and there is no need to also define that here.

Refs: reactphp/promise#223
WyriHaximus added a commit to WyriHaximus-secret-labs/promise-timer that referenced this pull request Jan 24, 2023
In the previous PR (reactphp#63) I missed incorrect usage in the readme, this PR addresses that.

The fact that a promise can also be rejected with a Throwable and/or Exception is implied and there is no need to also define that here.

Refs: reactphp/promise#223
WyriHaximus added a commit to WyriHaximus-labs/async that referenced this pull request Jan 25, 2023
The fact that a promise can also be rejected with a Throwable and/or Exception is implied and there is no need to also define that here.

Refs: reactphp/promise#223
WyriHaximus added a commit to WyriHaximus-labs/async that referenced this pull request Jan 25, 2023
The fact that a promise can also be rejected with a Throwable and/or Exception is implied and there is no need to also define that here.

Refs: reactphp/promise#223
WyriHaximus added a commit to WyriHaximus-labs/async that referenced this pull request Jan 25, 2023
The fact that a promise can also be rejected with a Throwable and/or Exception is implied and there is no need to also define that here.

Refs: reactphp/promise#223
WyriHaximus added a commit to WyriHaximus-labs/promise-stream that referenced this pull request Jan 25, 2023
The fact that a promise can also be rejected with a Throwable and/or Exception is implied and there is no need to also define that here.

Refs: reactphp/promise#223
composer.json Outdated Show resolved Hide resolved
types/Promises.php Outdated Show resolved Hide resolved
@@ -2,9 +2,17 @@

namespace React\Promise;

/**
* @template-implements PromiseInterface
* @template-covariant T
Copy link

Choose a reason for hiding this comment

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

It would be nice to also have a type parameter for rejection reason. But I have tried doing that for guzzlehttp/promisees and it was a pain.

Copy link
Member Author

Choose a reason for hiding this comment

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

Even if it's a pain, does it work? Guess, based on no PR associated with those changes it doesn't?

One of the issues is literally this: guzzle/promises@master...jtojnar:guzzle-promises:generic-types-2#diff-08c91937f94e1c62ad95bd704218d97cff992fb41a9b71b7fa9519442f8d67dbR33 that checks what is passed in to handle both scenarios not what is coming out of the previous promise.

That is one of the reasons we decided not to bother with rejections (for now).

Copy link

Choose a reason for hiding this comment

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

I made a PR but gave up because the various handler propagations were too difficult to reason about and I ran out of time I allocated for the experiment.

assertType('React\Promise\PromiseInterface<array<bool|int>>', all([true, time()]));
assertType('React\Promise\PromiseInterface<array<bool|int>>', all([resolve(true), resolve(time())]));
assertType('React\Promise\PromiseInterface<array<bool|float>>', all([resolve(true), hrtime()]));
assertType('React\Promise\PromiseInterface<array<bool|int>>', all([true, resolve(time())]));
Copy link

Choose a reason for hiding this comment

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

Perhaps it would be fine to comment these out for now. Most of the time people will be using homogeneous lists anyway. And if someone needs to use a heterogeneous one, they can always add it to ignore list in phpstan.neon.

Copy link
Member Author

Choose a reason for hiding this comment

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

Lines 46 and 47 work fine, lines 48 and 49 don't. If the bug in PHPStan doesn't get fixed before this PR will be merged we'll comment 48 and 49 out. As that's not a very common situation. 46 and maybe 47 I have a lot in applications where I don't directly control the typing and accept multiple. An example is the GitHub client I've been generating having a Repository and a SimpleRepository. Both work for where I use them and it gets either depending on where something originates from. So something returning an array with both is perfectly possible.

@@ -13,8 +13,10 @@
*
* If `$promiseOrValue` is a promise, it will be returned as is.
*
* @param mixed $promiseOrValue
* @return PromiseInterface
Copy link

Choose a reason for hiding this comment

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

Looks like this actually returns ExtendedPromiseInterface.

Copy link
Member Author

Choose a reason for hiding this comment

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

Technically yes, but that isn't the base contract we're using. This is also why in v3 ExtendedPromiseInterface has been merged into PromiseInterface.

@WyriHaximus WyriHaximus modified the milestones: v2.10.0, v2.11.0 May 2, 2023
lucasnetau pushed a commit to lucasnetau/react-dns that referenced this pull request Jun 30, 2023
The fact that a promise can also be rejected with a Throwable and/or Exception is implied and there is no need to also define that here.

Refs: reactphp/promise#223
@SimonFrings SimonFrings removed this from the v2.11.0 milestone Nov 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants