reactphp / promise

Promises/A implementation for PHP.
https://reactphp.org/promise/
MIT License
2.38k stars 146 forks source link

Arguments expecting callables should describe the full signature of callables #252

Closed stof closed 1 year ago

stof commented 1 year ago

https://github.com/composer/composer/issues/11619 is caused by a wrong usage of the $resolve callback passed as argument to the $resolver callable of the Promise constructor. In version 3, it cannot be called without argument anymore. However, as callable signatures are not described in the argument types, static analysis tools are unable to detect the issue.

clue commented 1 year ago

@stof Thanks for reporting and linking the downstream ticket :+1:

I think we all agree that adding more specific types for callables would be useful. I'm happy to look into this once time allows, otherwise we're happy to accept PRs. :shipit:

For the record, this appears to be clearly documented behavior with its BC implications mentioned in the changelog, so this would be more of feature addition or maintenance rather than a bug, see also related tickets #213, #246 and #247.

stof commented 1 year ago

@clue sure, the upgrade guide does not actually mention anything for the signature of the $resolve callback for the API of the promise constructor (even though I totally understand that this change is related to https://github.com/reactphp/promise/pull/213, it isn't mentioned anywhere). And the changelog entry for that change mention Improve type safety of promises, but this particular change does not provide any type safety given that it is not reported in types...

clue commented 1 year ago

https://github.com/reactphp/promise/blob/baf9ab543a6cd5c73379026b88ad0a648c7a888d/CHANGELOG.md#L48

stof commented 1 year ago

@clue what I mean is that none of the code examples mention that API as an impacted one (and it is the only API for which static analysis won't help, so I would say it would probably have been the most important one to show explicitly)

stof commented 1 year ago

@clue I started to look at that and I'm struggling with the type of the $canceller callback of the promise constructor. It looks like the canceller can actually be called with $resolve and $reject (same than the $resolver callback), but what would be the expected argument for $resolve then ? Does it even make sense to call it ?

stof commented 1 year ago

@clue do you have any insight on the expected behavior for the canceller callback ?

clue commented 1 year ago

See PR #253 which adds stricter types for all callable arguments.

Spent the better half of the day working on all edge cases and making sure this is fully covered in our tests. I hope this helps! :+1: If so, consider supporting this project, for example by becoming a sponsor ❤️

WyriHaximus commented 1 year ago

Thanks for filing this @stof just approved and merged #253. Looking forward to your follow up :+1: