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

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

Closed
stof opened this issue Sep 4, 2023 · 8 comments · Fixed by #253
Closed

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

stof opened this issue Sep 4, 2023 · 8 comments · Fixed by #253
Milestone

Comments

@stof
Copy link

stof commented Sep 4, 2023

composer/composer#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.

@stof stof added the bug label Sep 4, 2023
@clue clue added new feature and removed bug labels Sep 4, 2023
@clue
Copy link
Member

clue commented Sep 4, 2023

@stof Thanks for reporting and linking the downstream ticket 👍

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
Copy link
Author

stof commented Sep 4, 2023

@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 #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
Copy link
Member

clue commented Sep 4, 2023

It is no longer possible to resolve a promise without a value (use `null` instead) or reject a promise without a reason (use `Throwable` instead).

@stof
Copy link
Author

stof commented Sep 4, 2023

@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
Copy link
Author

stof commented Sep 4, 2023

@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
Copy link
Author

stof commented Sep 7, 2023

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

@clue
Copy link
Member

clue commented Sep 9, 2023

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! 👍 If so, consider supporting this project, for example by becoming a sponsor ❤️

@clue clue added this to the v3.1.0 milestone Sep 9, 2023
@WyriHaximus
Copy link
Member

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

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

Successfully merging a pull request may close this issue.

3 participants