Typed callable when mixed used#2986
Conversation
| @@ -1 +1 @@ | |||
| /** @var callable(array<mixed>) */ | |||
| /** @var callable(array<mixed>):mixed */ | |||
There was a problem hiding this comment.
Please keep these 2 and add 2 new extra files, so it's clear there is no change in behavior
There was a problem hiding this comment.
But this behavior it's what we need. We need to disallow typed callable without return type via phpstan/phpstan#3008
There was a problem hiding this comment.
How would you explain people a rule that changes their phpdocs?
There was a problem hiding this comment.
IMO mixed means same no return type. So this change just improve callable definition.
Currently rector changes my phpdocs by remove mixed, so who would you explain me this change ;-)
There was a problem hiding this comment.
That's just moving problem from to right. Soon someone will send the revert PR :)
The solution is to respect both cases.
There was a problem hiding this comment.
@ondrejmirtes maybe you can add something to discussion? Your voice is important because currently this trigger error in phpstan phpstan/phpstan#3008
There was a problem hiding this comment.
@snapshotpl Probably. I have focused on many other issues since.
There was a problem hiding this comment.
callable(X) without return typehint in PHPDoc was never supported, because it's ambiguous for the AST parser. Better to use the callable(X): mixed syntax.
Is it IdentifierTypeNode with (X) description, or is it CallableTypeNode? There's no way to tell. I added some tests with the current behaviour based on the issue phpstan/phpdoc-parser@e2b89c5
There was a problem hiding this comment.
@TomasVotruba what are you thing now?
This change is introduced by my request, so probably nobody cares about "revert" this behavior.
If it's BC break you can start with 0.8, but IMO callable, if typed, should always contains return type.
There was a problem hiding this comment.
Format must be preserved. It's the most discouraging to see no change yet 200 changed files
|
@TomasVotruba which rector should I disable to prevent this change? |
|
That's not possible, It's part of the printer. |
|
So I'm blocked to 0.7 upgrade... |
|
Until it's fixed, yes |
|
Any plan to move it forward? IMO This change is fine, however I see you have different opinion. |
|
Anything that preserves format is ok |
|
On second thought, let's give it a try and wait for community feedback. Thank you for your work 👍 |
|
🍻 |
rectorphp/rector-src@ee04ee1 Fix multi property default assign (#2986)
3 participants
Covers #2881 and #2884