Add more generic phpdocs #306

dantleech · 2020-03-28T08:30:52Z

This allows:

interface WatcherProcess
{
    public function stop(): void;

    /**
     * @return Promise<?ModifiedFile>
     */
    public function wait(): Promise;
}

dantleech · 2020-03-28T08:31:18Z

lib/Promise.php

@@ -23,7 +24,7 @@ interface Promise
     * Note: You shouldn't implement this interface yourself. Instead, provide a method that returns a promise for the
     * operation you're implementing. Objects other than pure placeholders implementing it are a very bad idea.
     *
-     * @param callable(\Throwable|null, TValue|null) $onResolved The first argument shall
+     * @param callable(\Throwable|null, TValue|null): void $onResolved The first argument shall


Phpstan wouldn't parse this without :void (which is in the Psalm stub)

enumag

Please add @template for the call function too. It's one of the most frequently used ones so I think it deserves it.
https://github.com/vimeo/psalm/blob/master/src/Psalm/Internal/Stubs/Amp.php#L15-L24

dantleech · 2020-03-28T08:41:57Z

Please add @template for the call function too

I tried this, but at least with PHPStan it gives an error, and I'm not sure how to add the type information:

   /**
     * @template TValue
     *
     * @param callable():(\Generator<mixed, mixed, mixed, TValue>|TValue) $gen
     *
     * @return Promise<TValue>
     */
    function call(callable $callback, ...$args): Promise

 ------ ---------------------------------------------------------------- 
  Line   SystemDetector/CommandDetector.php                                     
 ------ ---------------------------------------------------------------- 
  16     Unable to resolve the template type TValue in call to function  
         Amp\call                                                        
 ------ ----------------------------------------------------------------

enumag · 2020-03-28T08:44:37Z

lib/functions.php

     *
-     * @return callable(mixed ...$args): \Amp\Promise
+     * @param callable():\Generator<mixed, mixed, mixed, TValue> $callback


$callback is passed to call() which accepts many different types as return types from the callback, not just Generator.
See https://github.com/amphp/amp/blob/master/lib/functions.php#L65-L77

I've updated to

* @param callable(mixed ...$args): (\Generator<mixed, mixed, mixed, TValue>|\Amp\Promise<TValue>|mixed) $callback

If that makes sense.

It would be better to use TValue instead of the last mixed but I'm guessing that runs into the same issue you mentioned above, right?

Second mixed of the generator should possibly be Promise<mixed>. Afaik yielding anything else than a Promise would cause an error.

Actually, that works. However not sure about the ReactPromise? Should we add that too?

* @param callable(mixed ...$args): (\Generator<mixed, mixed, mixed, TValue>|\Amp\Promise<TValue>|\React\Promise\PromiseInterface<TValue>|TValue) $callback

I guess... Personally I'm kinda against using React promises in Amphp code and would always call adapt() anyway but it works so I guess it's correct to add it here.

By the way. technically the return value of the Generator is Promise<TValue>|ReactPromise<TValue>|TValue.

cool, updated to use local names

enumag · 2020-03-28T08:47:31Z

I tried this, but at least with PHPStan it gives an error, and I'm not sure how to add the type information:

I see... yeah that complicates things. Maybe we should report it to PHPStan then.

lib/Promise.php

enumag · 2020-03-28T13:31:19Z

Pretty sure we don't.

…

On Sat, Mar 28, 2020, 14:05 Niklas Keller ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In lib/Promise.php <#306 (comment)>: > @@ -5,6 +5,7 @@ /** * Representation of the future value of an asynchronous operation. * + * @template TValue Why do we need both? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#306 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAEDWRRLH4QSTLS7HLH5DSTRJXY2BANCNFSM4LVPZCJA> .

kelunik · 2020-03-28T14:05:33Z

I've added quite a lot of annotations I had locally already, so there are some conflicts now, sorry about that.

dantleech · 2020-03-28T14:29:12Z

lib/functions.php

@@ -14,8 +14,7 @@
     *
     * @template TReturn
     *
-     * @param callable(mixed ...$args):(\Generator<mixed,Promise|ReactPromise|array<array-key, Promise|ReactPromise>,mixed,Promise<TReturn>|ReactPromise|TReturn>|Promise<TReturn>|ReactPromise|TReturn)
-     *     $callback
+     * @param callable(mixed ...$args):(\Generator<mixed,Promise|ReactPromise|array<array-key, Promise|ReactPromise>,mixed,Promise<TReturn>|ReactPromise|TReturn>|Promise<TReturn>|ReactPromise|TReturn) $callback


The phpstan docblock parser didn't like $callback being ona new line for some reason.

Could you please open an phpstan issue for that? Phpstorm automatically formats it like that for me.

Hmm, it works now it seems.

PhpStorm doesn't understand this syntax at all so I'm not surprised it breaks on that.

@ondrejmirtes: PhpStorm doesn't break, it just reformats too long lines like that.

@kelunik Yeah but it won't be able to understand the method accepts callable. Maybe in this case yes, because the type starts with callable, but definitely not in some more complex cases.

dantleech · 2020-03-28T14:39:27Z

Updated most of it seems to be implemented now anyway, although it does fix a parsing issue with Phpstan. The new call / asyncCall annotations don't work with Phpstan for the reason given above - I'd report a bug but will need to think about it quite hard 👀

dantleech · 2020-03-28T18:13:58Z

ok - so I think everything is covered by your changes and the original stuff in this PR can be discarded.

I've updated the PR to replace @psalm-param with @param as these seem to work with Phpstan.

ondrejmirtes · 2020-03-28T18:16:11Z

I wouldn't use these in @param because you'll break IDE autocompletion. Better to use analyser-prefixed tags like @phpstan-param and @psalm-param.

dantleech · 2020-03-28T18:25:57Z

Then, I'll close this. Thanks for implementing the types @kelunik

@ondrejmirtes I would create an issue for what seems like a Phpstan bug above , but I'm not quite sure yet how to reproduce, hopefully can do it later.

dantleech · 2020-03-28T18:35:13Z

I just tried running Psalm with the lastest annotations in master:

            while (null !== $chunk = yield $stream->read()) {
                foreach (str_split($chunk) as $char) {

gives:

INFO: InvalidArgument - src/Parser/LineParser.php:18:36 - Argument 1 of str_split expects string, Amp\Promise<null
|string> provided (see https://psalm.dev/004)                                                                     
                foreach (str_split($chunk) as $char) {

(after adding the psalm-return to ProcessInputStream:

     * @psalm-return Promise<string|null>
     */
    public function read(): Promise

🤔

enumag · 2020-03-28T18:38:43Z

Even with the annotations both Psalm and PHPStan are very unlikely to recognize that
yield Promise<TValue> will produce TValue.

ondrejmirtes · 2020-03-28T18:40:11Z

I havent checked the actual annotations but Generator stubs in PHPStan/Psalm support this in TSend.

kelunik · 2020-03-28T18:56:45Z

@ondrejmirtes They just support one type per generator, but not a type based on the yielded value.

ondrejmirtes · 2020-03-28T19:03:59Z

@kelunik I don't understand. See this example: https://phpstan.org/r/0eaa8d12-2237-489a-9165-eb243d637329

What do you think wouldn't work in regards to Amp?

enumag · 2020-03-28T19:10:57Z

@ondrejmirtes With Amphp the Generators are usually immediately called using the \Amp\call() function. They do not have an annotation with a TSend type. Also they often yield promises for different things. See the code example here.

$http->request("https://example.com/") returns a Promise<Response> which means that $response instanceof Response. Then $response->getBody(); returns a Promise<string> so $body is a string.

enumag · 2020-03-28T19:14:42Z

In other words this code:

/**
 * @return Promise<string>
 */
function getExampleCom(HttpClient $http): Promise {
    return Amp\call(function () use ($http) {
        $response = yield $http->request("https://example.com/");
        $body = yield $response->getBody();
        return $body;
    });
}

would look like this with async-await:

async function getExampleCom(HttpClient $http): string {
    $response = await $http->request("https://example.com/");
    $body = await $response->getBody();
    return $body;
}

dantleech · 2020-03-29T09:53:19Z

The issue seems similar to: phpstan/phpstan-doctrine#101

dantleech commented Mar 28, 2020

View reviewed changes

enumag reviewed Mar 28, 2020

View reviewed changes

dantleech changed the title ~~[wip] Add more generic docs~~ Add more generic phpdocs Mar 28, 2020

kelunik reviewed Mar 28, 2020

View reviewed changes

lib/Promise.php Outdated Show resolved Hide resolved

dantleech force-pushed the gh-305-amp-sa-coroutine branch from d884c50 to 7585d05 Compare March 28, 2020 14:28

dantleech commented Mar 28, 2020

View reviewed changes

Remove Psalm prefixes

9f73668

dantleech force-pushed the gh-305-amp-sa-coroutine branch from 3fa7ede to 9f73668 Compare March 28, 2020 18:13

dantleech closed this Mar 28, 2020

dantleech deleted the gh-305-amp-sa-coroutine branch March 28, 2020 18:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add more generic phpdocs #306

Add more generic phpdocs #306

dantleech commented Mar 28, 2020 •

edited

Loading

dantleech Mar 28, 2020

enumag left a comment

dantleech commented Mar 28, 2020

enumag Mar 28, 2020

dantleech Mar 28, 2020

enumag Mar 28, 2020

enumag Mar 28, 2020

dantleech Mar 28, 2020

enumag Mar 28, 2020 •

edited

Loading

dantleech Mar 28, 2020

enumag commented Mar 28, 2020

enumag commented Mar 28, 2020 via email

kelunik commented Mar 28, 2020

dantleech Mar 28, 2020

kelunik Mar 28, 2020

dantleech Mar 28, 2020

ondrejmirtes Mar 28, 2020

kelunik Mar 28, 2020

ondrejmirtes Mar 28, 2020

dantleech commented Mar 28, 2020 •

edited

Loading

dantleech commented Mar 28, 2020

ondrejmirtes commented Mar 28, 2020

dantleech commented Mar 28, 2020 •

edited

Loading

dantleech commented Mar 28, 2020 •

edited

Loading

enumag commented Mar 28, 2020 •

edited

Loading

ondrejmirtes commented Mar 28, 2020

kelunik commented Mar 28, 2020

ondrejmirtes commented Mar 28, 2020

enumag commented Mar 28, 2020 •

edited

Loading

enumag commented Mar 28, 2020 •

edited

Loading

dantleech commented Mar 29, 2020

Add more generic phpdocs #306

Add more generic phpdocs #306

Conversation

dantleech commented Mar 28, 2020 • edited Loading

Choose a reason for hiding this comment

enumag left a comment

Choose a reason for hiding this comment

dantleech commented Mar 28, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

enumag Mar 28, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

enumag commented Mar 28, 2020

enumag commented Mar 28, 2020 via email

kelunik commented Mar 28, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dantleech commented Mar 28, 2020 • edited Loading

dantleech commented Mar 28, 2020

ondrejmirtes commented Mar 28, 2020

dantleech commented Mar 28, 2020 • edited Loading

dantleech commented Mar 28, 2020 • edited Loading

enumag commented Mar 28, 2020 • edited Loading

ondrejmirtes commented Mar 28, 2020

kelunik commented Mar 28, 2020

ondrejmirtes commented Mar 28, 2020

enumag commented Mar 28, 2020 • edited Loading

enumag commented Mar 28, 2020 • edited Loading

dantleech commented Mar 29, 2020

dantleech commented Mar 28, 2020 •

edited

Loading

enumag Mar 28, 2020 •

edited

Loading

dantleech commented Mar 28, 2020 •

edited

Loading

dantleech commented Mar 28, 2020 •

edited

Loading

dantleech commented Mar 28, 2020 •

edited

Loading

enumag commented Mar 28, 2020 •

edited

Loading

enumag commented Mar 28, 2020 •

edited

Loading

enumag commented Mar 28, 2020 •

edited

Loading