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

Psalm in combination with v1.8.0 #53

Closed
gdejong opened this issue Dec 7, 2021 · 6 comments
Closed

Psalm in combination with v1.8.0 #53

gdejong opened this issue Dec 7, 2021 · 6 comments
Labels

Comments

@gdejong
Copy link

gdejong commented Dec 7, 2021

Hi,

Since the recent v1.8.0 release I am having some problems with Psalm.

There is an example listed in the README of this project:

Or if you're using react/promise v2.2.0 or up:

React\Promise\Timer\timeout($promise, 10.0)
    ->then(function ($value) {
        // the operation finished within 10.0 seconds
    })
    ->otherwise(function (React\Promise\Timer\TimeoutException $error) {
        // the operation has failed due to a timeout
    })
    ->otherwise(function ($error) {
        // the input operation has failed due to some other error
    })
;

However when running Psalm with errorLevel 5 or lower I get:

ERROR: UndefinedInterfaceMethod - Method React\Promise\PromiseInterface::otherwise does not exist (see https://psalm.dev/181)
            ->otherwise(function (\React\Promise\Timer\TimeoutException $error) {

I guess this is caused by the return type hint of PromiseInterface that was added to \React\Promise\Timer\timeout.

This timeout() function is returning a new Promise(), would it be an idea to change the docblock to @return Promise or @return ExtendedPromiseInterface?

@gdejong
Copy link
Author

gdejong commented Jan 4, 2022

@clue Can I please get your opinion on this? Thank you!

@SimonFrings
Copy link
Member

Hey @gdejong, thanks for bringing this up 👍

I just looked into react/promise#202, this addresses a similar topic. I think the answer for this ticket would be the same as in there. This is also something that regards promise v2, this won't be a thing in the new v3. We're not sure when v3 will be released, but we're certain that this will come in the nearer future.

What are your thoughts on this?

@gdejong
Copy link
Author

gdejong commented Feb 8, 2022

For me to most important thing would be to get Psalm working/happy again at a strict level. I could understand if you won't fix this for v2 since you are working on v3. I am curious, are you testing v3 in projects that use Psalm to statically analyze the code? Because that would be great in my opinion :)

@SimonFrings
Copy link
Member

Funny coincidence, @WyriHaximus opened up an discussion in psalm#7559 a week ago. To quote @WyriHaximus: "we want to make sure that promises are type safe in both Psalm's and PHPStan's eyes".
This goes hand in hand with reactphp/promise#188.

That said, this shouldn't be a problem anymore when talking about promise v3 which is the development version at this moment (development but stable ^^). We don't have psalm integrated inside the ReactPHP projects itself, but we're using ReactPHP in some other projects in combination with psalm.

If you want to try this out you can always install the current dev-master, I hope this helps and gives you some insights in our plans :)

@clue clue added the question label Feb 9, 2022
@clue
Copy link
Member

clue commented Feb 9, 2022

What @SimonFrings says 👍

@gdejong Thanks for reporting, I think we all agree that type safety is definitely super useful and something we're working on with reactphp/promise#188 and reactphp/promise#202, so I don't think there's much that can or needs to be done here.

I believe this has been answered, so I'm closing this for now. Please come back with more details if this problem persists and we can always reopen this 👍

@clue clue closed this as completed Feb 9, 2022
@WyriHaximus
Copy link
Member

I'll upgrade that example with the sleep method from this package 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants