Add generics for Promise.

[Warxcell]

I have typed my props with

    /**
     * @return string|\GraphQL\Executor\Promise\Promise<string>
     */

but psalm complains about it, because its missing in the Promise.

ERROR: TooManyTemplateParams - tests/Expected/QueryResolver.php:74:16 - GraphQL\Executor\Promise\Promise<Arxy\GraphQLCodegen\Tests\Expected\ObjectTest|null> has too many template params, expecting 0 (see https://psalm.dev/184)
     * @return \Arxy\GraphQLCodegen\Tests\Expected\ObjectTest|null|\GraphQL\Executor\Promise\Promise<\Arxy\GraphQLCodegen\Tests\Expected\ObjectTest|null>

[Warxcell]

If I have green light - i will proceed with adding all the generics to make psalm/phpstan happy.

[spawnia]

I generally like this change, please address the comments first and then move on to adapt the rest of the codebase.

[vhenzl]

I think this is a bit more complicated.

• There should be at least two template types - one on the Promise class, another on the then() method for onFulfilled, and probably a third for onRejected. The onFulfilled can't just return mixed.
• onRejected shouldn't assume Throwable, the rejection reason can be anything.

We have a PHPStan stub in our project that looks something like this:

/** @template T */
interface Promise
{
    /**
     * @template TResolved
     * @param (callable(T):(Promise<TResolved>|TResolved))|null $onFulfilled
     * @param (callable(mixed):mixed)|null $onRejected
     * @return Promise<TResolved>
     */
    public function then(?callable $onFulfilled = null, ?callable $onRejected = null): Promise;
}

It's rather naive, it doesn't account for the return type of onRejected – the return type of then() should be Promise<TResolved1|TResolved2>: https://phpstan.org/r/f7a355b3-8906-4b7f-88e7-e477aad17139

But if we do that (@param (callable(mixed):(Promise<TResult2>|TResult2))|null $onRejected and @return Promise<TResult1|TResult2>), PHPStan isn't happy if we don't provide $onRejected: https://phpstan.org/r/e39ddf3e-bcf1-4dce-8d1f-39181bd896a7

At this point, the game is almost over since PHPStan doesn't support default template types (@template TResult2 = never).

But not really. The recently introduced conditional types come to the rescue:

/** @template T */
interface Promise
{
    /**
     * @template TFulfilled of mixed
     * @template TRejected of mixed
     * @param (callable(T): (Promise<TFulfilled>|TFulfilled))|null $onFulfilled
     * @param (callable(mixed): (Promise<TRejected>|TRejected))|null $onRejected
     * @return Promise<(
     *   $onFulfilled is not null
     *     ? ($onRejected is not null ? TFulfilled|TRejected : TFulfilled)
     *     : ($onRejected is not null ? TRejected : T)
     * )>
     */
    public function then(?callable $onFulfilled = null, ?callable $onRejected = null): Promise;
}

https://phpstan.org/r/722e1db8-c329-4d69-a4fc-aa761ff5b5bf

(The code adjusted from phpstan/phpstan-src#1377.)

Works nicely for PHPStan but Psalm won't probably understand it.

TypeScript's Promise<T> for a reference: https://github.com/microsoft/TypeScript/blob/2ecde2718722d6643773d43390aa57c3e3199365/lib/lib.es5.d.ts#L1446-L1461

[simPod]

There's similar attempt going on here reactphp/promise#188. I think there's still a lot to be done on the SA side before we can do this.

[spawnia]

I think there's still a lot to be done on the SA side

What is SA?

Can we just use the solution proposed by @vhenzl and ignore possible errors for certain edge cases?

[simPod]

@spawnia I missed the mention of a solution for default types, that was the main blocker IIRC. With conditional types it might work actually.

(SA - static analysis)

[vhenzl]

@simPod It works: https://phpstan.org/r/722e1db8-c329-4d69-a4fc-aa761ff5b5bf

BTW, why two templates T and R in reactphp/promise#188? Doesn't make sense IMO.

[simPod]

@vhenzl I don't think the PR there is in working state right now, will have to give it some time.

[vhenzl]

One more thing to consider is the variance of the generic type.

I believe that Promise is covariant on T, not invariant.

/** @template-covariant T */
interface Promise { /* ... */ } 

Having it invariant (@template T) is unnecessarily restrictive and makes promises harder to work with and pass around.

As I understand it, Promise is a "getter" or "producer" as discussed in https://kotlinlang.org/docs/generics.html#declaration-site-variance and https://devblogs.microsoft.com/typescript/announcing-typescript-4-7/#optional-variance-annotations-for-type-parameters.

But I couldn't find any useful authoritative information about promise's variance. The best "proof" online is Promise<out T> in Kotlin: https://kotlinlang.org/api/latest/jvm/stdlib/kotlin.js/-promise/. And then this article: https://dmitripavlutin.com/typescript-covariance-contravariance/#2-covariance.

That our stub I mentioned above actually is covariant, because – well – that's what was needed to make it work. And it makes sense. If we have a field resolver with return type Promise<User|null>, we should be able to return Promise<User> from it.

[spawnia]

Hi @Warxcell, I checked out the PR and took the liberty of making a few changes.

In #1263 I was able to make the breaking change to the interface obsolete. Do you think this PR can be completed without making breaking changes?

The failing tests are fixed when reverting the code changes in ReferenceExecutor, but perhaps you can find a working solution that is more compatible with static analysis?

Without looking into it much deeper, I could not figure out how to resolve the remaining static analysis errors. If there are just a few edge cases, I would be fine with ignoring them - but I would prefer to try and really get the type definitions right so it all checks out.

[Warxcell]

Hi @Warxcell, I checked out the PR and took the liberty of making a few changes.

In #1263 I was able to make the breaking change to the interface obsolete. Do you think this PR can be completed without making breaking changes?

The failing tests are fixed when reverting the code changes in ReferenceExecutor, but perhaps you can find a working solution that is more compatible with static analysis?

Without looking into it much deeper, I could not figure out how to resolve the remaining static analysis errors. If there are just a few edge cases, I would be fine with ignoring them - but I would prefer to try and really get the type definitions right so it all checks out.

Will try to, but I have a bit of troubles around some types. Will write here if I need some help :)

[Warxcell]

Will close that for now.