[2.x] Add basic template annotations

[WyriHaximus]

This adds basic type safety annotations for static analyzers like PHPStan and Psalm. This will cover around 80% of the use cases and a follow-up PR for all supported versions will be proposed later to get it to a 100% of close to a 100%.

By adding these annotations methods returning a promise can hint their resolving type by adding @return PromiseInterface<bool> when they for example resolve to a boolean. By doing that Psalm and PHPStan will understand that the following bit of code will not become an issue because the method's contract promised a boolean through the promise:

$promise->then(static function (bool $isEnabled) {});

However, the following will yield errors:

$promise->then(static function (string $isEnabled) {});

This PR is a requirement for reactphp/async#40

[clue]

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? 👍

[WyriHaximus]

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

[clue]

Should we have a TFulfilled and a TRejected as discussed in phpstan/phpstan#7371?

[WyriHaximus]

Playing with that locally at the moment to get the types checking in CI as you mentioned above.

[clue]

After the discussion below, perhaps we should indeed disregard my comment and focus solely on TFulfilled instead? (#227 (comment))

Sorry for the confusion!

[WyriHaximus]

After the discussion below, perhaps we should indeed disregard my comment and focus solely on TFulfilled instead? (#227 (comment))

Done!

Sorry for the confusion!

No worries, we needed that discussion 😃

[clue]

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.)

[WyriHaximus]

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.

[WyriHaximus]

@clue Updated the PR with a types directory for PHPStan type checking, and expanded the annotations a bit

[vhenzl]

I don't think the proposed typing is correct. I tried to point out some of the problems in this PHPStan playground: https://phpstan.org/r/af4e982e-7118-4572-b180-3769009a906f

In general, the problem is that the promise is declared like PromiseInterface<T, R> but then is used everywhere like PromiseInterface<T>. That doesn't work because PHPStan (unsure about Psalm) doesn't support default template types (@template R = never).

I'm not convinced that it's worth trying to type promise with two generic types, nor I'm sure that's actually possible to do it properly. Having a single template type for the fulfilled value like TypeScript has would be much easier: https://github.com/microsoft/TypeScript/blob/2ecde2718722d6643773d43390aa57c3e3199365/lib/lib.es5.d.ts#L1446-L1461

Anyway, should it be two generic templates, then I believe it should be like this (using a pseudocode):

function resolve(T $value): PromiseInterface<T, never> {}
function reject(R $reason): PromiseInterface<never, R> {}

When resolving or rejecting, the created promise will have one type based on the provided value or reason, the other is always never.

then(onFulfilled(...), onRejected(...)) => PromiseInterface<ReturnTypeOfOnFulfilled|ReturnTypeOfOnRejected, never>

then(onFulfilled(...)) => PromiseInterface<ReturnTypeOfOnFulfilled, R>

then(null, onRejected(...)) => PromiseInterface<T|ReturnTypeOfOnRejected, never>

then() => PromiseInterface<T, R>

The presence of onRejected turns a potentially rejected promise into a fulfilled promise; therefore the returned promise will be <?, never>.

If onFulfilled or onRejected is missing, the original respective T and R is propagated to the returned type.

--

For context, I got here from webonyx/graphql-php#1170 where we also discussed generics promises.

[vhenzl]

Another thing is that I believe the promise type is covariant on T (and R): webonyx/graphql-php#1170 (comment)

[WyriHaximus]

@vhenzl The main goal with this PR is to make it work well enough to make reactphp/async#40 possible and then make the rest fully work in #223. I'll take a good read of your comment and playgrounds tonight, and do a longer response. But if you have a way to reach the goal I mentioned above then please feel free to suggest it as we realised this is a bigger undertaking than just this PR. And we want to get fibers out in the short term with typing.

[clue]

I'm not convinced that it's worth trying to type promise with two generic types, nor I'm sure that's actually possible to do it properly. Having a single template type for the fulfilled value like TypeScript has would be much easier: https://github.com/microsoft/TypeScript/blob/2ecde2718722d6643773d43390aa57c3e3199365/lib/lib.es5.d.ts#L1446-L1461

@vhenzl Excellent input and indeed something we've considered before. I agree that having the TFulfilled provides most value here and whether or not TRejected is useful is still debatable, so perhaps we should indeed focus on TFulfilled only for now. Implementing full type safety is indeed somewhat harder than it may seem at first sight, so I'd rather start somewhere and build from there.

After the discussion so far, it's my understanding focusing solely on TFulfilled (as also originally suggested by @WyriHaximus prior to my feedback above) would simplify this PR significantly, probably solve like 90% of use cases and can still serve as a good starting point if we find a need to provide type safe rejection values.

[jtojnar]

No longer marked as nullable?

[WyriHaximus]

Closing this in favour of #223