feat: introduce PhpdocArrayStyleFixer
#7781
Conversation
kubawerlos
force-pushed
the
add_PhpdocListTypeFixer
branch
from
2564675
to
da3adeb
Compare
kubawerlos
force-pushed
the
add_PhpdocListTypeFixer
branch
from
da3adeb
to
a8bcf41
Compare
| * @param array<int> $b | ||
| * @param array<string, int> $c | ||
| - * @param list<int> $d | ||
| + * @param array<int> $d |
There was a problem hiding this comment.
this is not a list. it's array<array-key, int>
suggest removing this ['style' => 'array'] option
There was a problem hiding this comment.
👎🏼 for removing ['style' => 'array'], see #7077
maybe that option should not touch list<> and only change [] syntax 🤔
There was a problem hiding this comment.
Yes, the array|list should be used only when converting from [] syntax.
There was a problem hiding this comment.
It also should be possible to convert from array to list, should that be a separate option or included in list one?
There was a problem hiding this comment.
suggest removing this
['style' => 'array']option
👍 plus the fixer should be renamed to reflect only the #7077 (ie. xxx[] to array<xxx>)
converting xxx[] to list<xxx> is really not possible by a fixer (without static analysis knowledge) as [] does simply not imply a list
There was a problem hiding this comment.
Yes, even "maybe incorrect" changes on PHPDoc must be risky.
There was a problem hiding this comment.
Personally I don't consider it as risky because it does not break the code (it modifies comment, even if that comment is a part of a contract). The only thing that can be broken is SA in the QA process, but by default this fixer should make 1:1 change (from x[] to array<x>) and when someone opts-in for list the result is also expected. It can be used for detecting non-list arrays and allow fixing them, just like @kubawerlos did in the other PR.
I don't see the need for 2 fixers here and it was me who opted for a single, configurable fixer in private chat with @kubawerlos.
There was a problem hiding this comment.
PHPDoc should be considered part of code. SA is part of code. reflection is part of code. So thus if PHPDoc change is risky, the fixer must be risky. Thus I belive two fixers would be better as xxx[] to array<xxx> is not risky, but array<xxx> to list<xxx> is risky.
There was a problem hiding this comment.
There was a problem hiding this comment.
my attention was grabbed outside of GH to look at this particular comment. I was not having a chance to look into all threads in this PR.
Should a fixer changing PHPDoc be risky?
You want to have 2 fixers - as it is in PHP CS Fixer: custom fixers - if so, then we can discuss the riskiness of the other in separate PR, but first let The One decide - @keradus - single, configurable fixer as in this PR or 2 separate fixers as you can see in PHP CS Fixer: custom fixers?
Riskiness is sth that can change the meaning. If we change type (regardless it's declared in code, phpdoc or attribute) to type that may have other meaning, it's risky.
With that, I would, myself, go with 2 fixers:
- one converting array to list-or-iterable (maybe array to list is enough), eg
array<int>->list<int>(but notarray<string, int>norarray{string, string, string}). This is Risky. - one converting array syntax - ie convert
Foo[]toarray<mixed, Foo>(not sure if I would go witharray<Foo>as it is not. Probably yes to allow later risky conversion tolist<Foo>). (and TBH I wonder if we need to support each possible conversion)
| '<?php /** @var list<bool>|list<float>|list<int>|list<string> */', | ||
| '<?php /** @var array<bool>|float[]|array<int>|string[] */', |
There was a problem hiding this comment.
array<T> should never be replaced with list<T> because they are not the same type.
There was a problem hiding this comment.
I know these are not the same type, I don't get why, today, everyone thinks I don't.
To "forbid usage of Foo[] or array<Foo>" it needs to stay (maybe as 3rd option?) as in practice (see the long, first thread here, sponsored by 112 and 17) array<T> is often overused. Anyone carrying about the specific types must then add type to array<T> - if the keys matter, or have it converted to list<T> - if they don't.
There was a problem hiding this comment.
I get your point but I think this rule should only focus on syntax, i.e. T[] ⟷ array<T>. IMO forbidding array<T> is a matter of static analysis and out of scope here.
There was a problem hiding this comment.
But I can see the scenario when developer wants to apply fixer with list as an target type, then run SA, fix not matching entries and keep the correct ones. Ability to apply it with a tool would save a lot of time in cases where there's more actual lists than arrays (comparing to applying array<> everywhere and converting to list<> manually).
There was a problem hiding this comment.
I did not review the code yet, but @bendavies pointed out that fixer's name is not correct, and I agree with him. Foo[] is not a list, it's an array with an undefined key type. Every list is an array, but not every array is a list, that's why the fixer's name should not focus on lists, but on arrays. Possible names:
PhpdocArrayTypeFixerPhpdocArrayStyleFixerPhpdocArrayTypeConventionFixer
kubawerlos
changed the title
PhpdocListTypeFixerPhpdocArrayStyleFixer
|
I've reworked the option - now everyone will find something for themselves. When reviewing it further, please focus on:
Do NOT focus on:
|
| PHP; | ||
|
|
||
| return new FixerDefinition( | ||
| 'PHPDoc list types must be written in configured style.', |
There was a problem hiding this comment.
| 'PHPDoc list types must be written in configured style.', | |
| 'PHPDoc array types must be written in configured style.', |
| private const STRATEGY_FROM_ARRAY_TO_LIST = 'array_to_list'; | ||
| private const STRATEGY_FROM_BRACKETS_TO_ARRAY = 'brackets_to_array'; | ||
| private const STRATEGY_FROM_BRACKETS_TO_LIST = 'brackets_to_array_to_list'; |
There was a problem hiding this comment.
Recently I am more and more convinced that we should use public consts as part of the contract, so people can use these in the config, instead of hardcoding string values. What do you think?
| protected function createConfigurationDefinition(): FixerConfigurationResolverInterface | ||
| { | ||
| return new FixerConfigurationResolver([ | ||
| (new FixerOptionBuilder('strategy', 'Which part of the conversion - brackets (`[]`) to `array` to `list` - to perform.')) |
There was a problem hiding this comment.
Gramatically it's a bit weird, I would simplify it to something like:
| (new FixerOptionBuilder('strategy', 'Which part of the conversion - brackets (`[]`) to `array` to `list` - to perform.')) | |
| (new FixerOptionBuilder('strategy', 'Which type syntax should be used')) |
Allowed values extends this information so it should be clear enough.
| { | ||
| private const STRATEGY_FROM_ARRAY_TO_LIST = 'array_to_list'; | ||
| private const STRATEGY_FROM_BRACKETS_TO_ARRAY = 'brackets_to_array'; | ||
| private const STRATEGY_FROM_BRACKETS_TO_LIST = 'brackets_to_array_to_list'; |
There was a problem hiding this comment.
I am wondering why it's not like this:
| private const STRATEGY_FROM_BRACKETS_TO_LIST = 'brackets_to_array_to_list'; | |
| private const STRATEGY_FROM_BRACKETS_TO_LIST = 'brackets_to_list'; |
I am not sure we should include intermediate state in the strategy's name, I would rather use source-target convention.
|
Replaced by #7812 |
Resolves #4698