assertType not picking up templates as expected

[WyriHaximus]

While working on reactphp/promise#223 to add templates for type safety to react/promise not everything was picked up as expected by assertType. I'm pretty sure fixing a few things will resolve most if not all errors currently coming up.

Will respond to this discussion with each error I'm seeing once by one. The PR I linked is where I'll push the changes made after fixing once as this spawns multiple files.

[WyriHaximus]

The FulfileldPromise implements the PromiseInterface. But when asserting the following:

assertType('React\Promise\PromiseInterface<bool>', new FulfilledPromise(true));

The following error pops up:

Expected type React\Promise\PromiseInterface<bool>, actual: React\Promise\FulfilledPromise<bool>  

[ondrejmirtes]

That’s expected. assertType() asserts the exact type so that you can verify the type inference is working. It’s not “acceptTypes”, for example literal integer 1 isn’t going to be satisfied with “int”, only with “1”.

[WyriHaximus]

Ok thanks, I'll be scrapping this assertion then.

[herndlm]

I was thinking the same as Ondrej described but kept thinking about how this could still be asserted. Theoretically, an expression that resolves to a constant boolean could be asserted too, right? How high would this score on the hackyness scale? xD
https://phpstan.org/r/8a1e6d5c-8418-45b0-be79-f93bc83fa74b

UPDATE: but in your case with generics it's utterly useless I guess hehe

[WyriHaximus]

@herndlm Yes :D! And for this one I get it, it's fine. It would be a really nice to have, but I have bigger fish to fry in that file :D.

[VincentLanglet]

Would it be useful to have a PHPStan\Testing\acceptType() then ?

[WyriHaximus]

This assertion works fine:

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

However the moment we put an empty then() between the resolve and the then() taking a function, it errors out. Effectively both lines do the same thing:

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

The following error pops up:

Parameter #1 $onFulfilled of method React\Promise\PromiseInterface<mixed>::then() expects (callable(mixed): bool)|null, Closure(bool): bool given. 

[rvanvelzen]

This should be doable with a conditional return type, but while writing it I noticed that it reported some incorrect errors. See phpstan/phpstan-src#1377 for more details.

[WyriHaximus]

@rvanvelzen Thank you, I'll test those changes out tomorrow 👍.

[WyriHaximus]

The fix you PRed yesterday helps @rvanvelzen. But it might also contain a critical flaw in how promises work. If I interpret this conditional return statement correctly it looks at which argument is passed or not, but not at which of those callables is called:

     /**
     * @return PromiseInterface<(
     *   $onFulfilled is not null
     *     ? ($onRejected is not null ? TFulfilled|TRejected : TFulfilled)
     *     : ($onRejected is not null ? TRejected : F)
     * )>
     */

The way promises work both can be called depending on the outcome. For example during the following assertion $passThroughThrowable is passed but never called even though $onFulfilled is passed a null:

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

And yields the following errors:

Parameter #1 $onFulfilled of method React\Promise\PromiseInterface<React\Promise\PromiseInterface,mixed>::then() expects (callable(React\Promise\PromiseInterface): bool)|null, Closure(bool): bool given.
Parameter #2 $onRejected of method React\Promise\PromiseInterface<bool,mixed>::then() expects (callable(mixed): React\Promise\PromiseInterface)|null, Closure(Throwable): React\Promise\PromiseInterface given. 

Had a discussion yesterday with @clue and @SimonFrings on this. And we're not sure how return types would look like when a promise can resolve into a bool or reject with a RuntimeException. Would that be PromiseInterface<bool, RuntimeException> or PromiseInterface<bool|RuntimeException>. Both follow a slightly different path, but only one of them can come out of the promise.

[rvanvelzen]

I forgot to realize that the callbacks can return a promise as well, so the types need some fixing:

     * @param (callable(T): (PromiseInterface<TFulfilled>|TFulfilled))|null $onFulfilled
     * @param (callable(mixed): (PromiseInterface<TRejected>|TRejected))|null $onRejected

For the rest it's hard to say without looking at the bigger picture in how the return values are consumed. I can take a look at reactphp/promise#223 later this week to help out if you'd like.

[clue]

Had a discussion yesterday with @clue and @SimonFrings on this. And we're not sure how return types would look like when a promise can resolve into a bool or reject with a RuntimeException. Would that be PromiseInterface<bool, RuntimeException> or PromiseInterface<bool|RuntimeException>. Both follow a slightly different path, but only one of them can come out of the promise.

I think the obvious approach would be to take a look at how an await() function (here with a single fulfillment type only for brevity) should be typed:

/**
 * @template T
 * @param PromiseInterface<T>
 * @return T
 * @throws \Throwable
 */
function await(PromiseInterface $promise): mixed { /* … */ }

If we want to say this function returns a value of the type the promise fulfilled with, then this implies we must not mix in any rejection values into the same type. In other words, we do not expect the await() function to return a bool or RuntimeException. Instead, we expect it to either return a bool or throw a RuntimeException.

To me, this makes it clear we should have two independent template types for fulfillment values and rejection values. I think we all agree that having better typing for fulfillment values is the most crucial feature here and probably what we should focus on as a first step. Whether specifying the rejection type has value or not, I'm not entirely sure. The rejection type is always a Throwable or any Exception subclass (as of Promise v3 at least). There might be use cases where specifying a certain rejection type is expected could be useful, like saying a promise can reject only with a RuntimeException rather than the generic Throwable in your above example.

/**
 * @template TFulfilled
 * @template TRejected
 * @param PromiseInterface<TFulfilled, TRejected>
 * @return TFulfilled
 * @throws TRejected
 */
function await(PromiseInterface $promise): mixed { }

[clue]

Likewise, if want to apply two types for TFulfilled and TRejected, our resolve() and reject() functions could be typed like this:

/**
 * @template T of mixed
 * @param T $value
 * @return ($value is PromiseInterface ? T : PromiseInterface<T,void>)
 * @throws void
 */
function resolve(mixed $value): PromiseInterface { /* … */ }

/**
 * @template TRejected of \Throwable
 * @param TRejected $reason
 * @return PromiseInterface<void,TRejected>
 * @throws void
 */
function reject(\Throwable $reason): PromiseInterface { /* … */ }

If we want to go even deeper, we should probably also specify TFulfilled can be of typed mixed except it can not be of type PromiseInterface.

[WyriHaximus]

For the rest it's hard to say without looking at the bigger picture in how the return values are consumed. I can take a look at reactphp/promise#223 later this week to help out if you'd like.

That would be very much appreciated! This is also why I posted the link to the related PR and started with a file listing all expected out comes.

And as @clue mentions the next step is to do the same for react/async, but that should be fairly straight forward once react/promise has been done.

If we want to go even deeper, we should probably also specify TFulfilled can be of typed mixed except it can not be of type PromiseInterface.

As per our chat earlier this week, as long as we can 90% of the use cases everything else is a welcome bonus.

[WyriHaximus]

Been trying to wrap my head around the fact that I can't seem to change anything in this error list:

 ------ ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- 
  Line   Promises.php                                                                                                                                                                                   
 ------ ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- 
  22     Parameter #1 $promisesOrValues of function React\Promise\all expects array<bool|int|React\Promise\PromiseInterface<bool|int>>, array{React\Promise\PromiseInterface<bool>,                     
         React\Promise\PromiseInterface<int>} given.                                                                                                                                                    
  23     Expected type React\Promise\PromiseInterface<array<bool|int>>, actual: React\Promise\PromiseInterface<array<bool>>                                                                             
  23     Parameter #1 $promisesOrValues of function React\Promise\all expects array<bool|React\Promise\PromiseInterface<bool>>, array{React\Promise\PromiseInterface<bool>, int<1, max>} given.         
  24     Expected type React\Promise\PromiseInterface<array<bool|int>>, actual: React\Promise\PromiseInterface<array<int>>                                                                              
  24     Parameter #1 $promisesOrValues of function React\Promise\all expects array<int|React\Promise\PromiseInterface<int>>, array{true, React\Promise\PromiseInterface<int>} given.                   
  30     Parameter #1 $onFulfilled of method React\Promise\PromiseInterface<mixed,mixed>::then() expects (callable(mixed): bool|React\Promise\PromiseInterface<bool>)|null, Closure(bool): bool given.  
  30     Parameter #2 $onRejected of method React\Promise\PromiseInterface<bool,mixed>::then() expects (callable(mixed): mixed)|null, Closure(Throwable): React\Promise\PromiseInterface given.         
  31     Parameter #1 $onFulfilled of method React\Promise\PromiseInterface<mixed,mixed>::then() expects (callable(mixed): bool|React\Promise\PromiseInterface<bool>)|null, Closure(bool): bool given.  
  31     Parameter #2 $onRejected of method React\Promise\PromiseInterface<bool,mixed>::then() expects (callable(mixed): mixed)|null, Closure(Throwable): React\Promise\PromiseInterface given.         
  32     Parameter #1 $onFulfilled of method React\Promise\PromiseInterface<mixed,mixed>::then() expects (callable(mixed): bool|React\Promise\PromiseInterface<bool>)|null, Closure(bool): bool given.  
  32     Parameter #2 $onRejected of method React\Promise\PromiseInterface<bool,mixed>::then() expects (callable(mixed): mixed)|null, Closure(Throwable): React\Promise\PromiseInterface given.         
  34     Parameter #1 $onFulfilled of method React\Promise\FulfilledPromise<bool>::then() expects (callable(mixed): bool|React\Promise\PromiseInterface<bool>)|null, Closure(bool): bool given.         
 ------ ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- 

It seems I cannot find a way to tell it that the Closure(bool): bool is perfectly fine for (callable(mixed): bool|React\Promise\PromiseInterface<bool>)|null. Even with the suggestions @rvanvelzen did.

[WyriHaximus]

Sorry, it's been a while. But managed to several things solved. Only have one problem to solve at this point and that is functions like race, all, and any don't support having a PromiseInterface<bool> and an int passed in them as an array. I've extracted this playground from the work: https://phpstan.org/r/12494500-5d34-471d-ba92-cf9d3d639f37 However it's not fully working as it's missing a function body and that won't fit in the playground. The full changeset can be viewed here: https://github.com/reactphp/promise/pull/223/files

[WyriHaximus]

@ondrejmirtes @rvanvelzen Sorry for pinging if you saw this, but any insights would be welcome.