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

Use \Closure instead of callable #27

Merged
merged 3 commits into from Dec 1, 2021
Merged

Use \Closure instead of callable #27

merged 3 commits into from Dec 1, 2021

Conversation

kelunik
Copy link
Member

@kelunik kelunik commented Nov 25, 2021

callable is a problematic type, because it's scope dependent. It can therefore not be used for typed properties, but \Closure can be used as a safe replacement, especially with first-class syntax introduced in PHP 8.1: (...).

Resolves #14.

@trowski
Copy link
Member

trowski commented Dec 1, 2021

I'm undecided about this – callable can be problematic, but it's not a subtle bug if used incorrectly, it becomes quite obvious. It also seems in PhpStorm we are able to type parameters as, e.g., callable(string):void, whereas the same is not possible with Closure.

@kelunik
Copy link
Member Author

kelunik commented Dec 1, 2021

I guess we'd have to introduce separate psalm types to allow for the call signatures?

@WyriHaximus
Copy link
Contributor

Have to agree with @trowski here, that I'm undecided about this. But I'm also fine with giving this a try and see how it works out during 0.x before making a stable decision.

@kelunik
Copy link
Member Author

kelunik commented Dec 1, 2021

Let's give this a try then. 👍

@kelunik kelunik merged commit 498362a into main Dec 1, 2021
@kelunik kelunik deleted the closure branch December 1, 2021 22:00
@trowski
Copy link
Member

trowski commented Dec 1, 2021

After talking to @kelunik, we realized Psalm does support the same syntax for declaring Closure params/return types. We can always roll back to callable without a big impact to users.

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

Successfully merging this pull request may close these issues.

Use Closure instead of callable?
3 participants