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

Convert array<V> phpdoc to array<array-key, V> #7837

Open
mvorisek opened this issue Feb 14, 2024 · 4 comments
Open

Convert array<V> phpdoc to array<array-key, V> #7837

mvorisek opened this issue Feb 14, 2024 · 4 comments

Comments

@mvorisek
Copy link
Contributor

Feature request

I understand that very commonly array<V>, ie. array phpdoc /wo key type, very commonly represents list<V>, hovewer definitely not always.

This is a feature request to fix array<V> to array<array-key, V> phpdoc instead which is trully equivalent and non-risky, so it can be applied safely even for large codebases and improved gradually (ie. to list<V>, key type improved or left if any key is allowed).

related with #7796 - but a new fixer should be probably introduced to be named better (like PhpdocArrayWithoutKeyTypeFixer) and be non-risky, maybe #7812 can be made non-risky, as IMO it is non-risky, see unresolved #7812 (comment)

/cc @Wirone @kubawerlos @keradus

@Wirone
Copy link
Member

Wirone commented Feb 14, 2024

I believe the risky flag can't be removed, as this was explicitly requested by @keradus to treat it as such. But definitely PhpdocArrayTypeFixer fits better for this case.

@mvorisek
Copy link
Contributor Author

mvorisek commented Feb 14, 2024

I believe the risky flag can't be removed, as this was explicitly requested by @keradus to treat it as such. But definitely PhpdocArrayTypeFixer fits better for this case.

@Wirone can you please prove with https://phpstan.org/try demo that the usecase you defend, ie. #7812 (comment), more precisely the Collection|V[] from https://phpstan.org/writing-php-code/phpdoc-types#iterables, behaves really differently than when written as Collection|array<array-key, V>? IMO phpstan converts V[] to array<V> immediately which is internally equivalent to array<array-key, V>. But I might be wrong. Maybe this is somehow different thanks to "some phpstan collection resolving extension"? In any case, the current risky reasoning is not clear to me.

@mvorisek mvorisek changed the title PhpdocListTypeFixer - array<V> to array<array-key, V> Convert array<V> phpdoc to array<array-key, V> Feb 14, 2024
@Wirone
Copy link
Member

Wirone commented Feb 14, 2024

I didn't defend anything there, I even questioned the need of marking that fixer as risky. Just pointed that if we consider it as risky, then description should be better than it was at that point, pointing to the only difference known to me. Especially this part:

Collection implements Traversable so Collection|Foo[] is interpreted as a Collection object that iterates over Foo. The array part isn’t applied.

As a quick check, there's a difference between this (Cannot access offset 0 on Collection&iterable<Foo>.), and this (Cannot access offset 0 on array<Foo>|Collection.). I don't know if it has any serious implications when this fixer is applied, but since it may change the way how PHPStan interprets the type it may for example lead to different errors reported, so when someone has it in baseline file, it suddenly can fail with 2 new errors (one for ignored error not found during analysis, and second with the "new" error). Have in mind this is only my assumption. Riskiness is ambiguous here IMHO, one rabbi will say "yes" whereas other might say "no" 😉.

@mvorisek
Copy link
Contributor Author

mvorisek commented Feb 14, 2024

Thank you - you found the difference!

@kubawerlos the #7812 risky description should be improved:

-Risky when using ``T[]`` in union types.
+Risky when using ``T[]`` in union type with iterable type without generics explicitly defined like `Collection|T[]`.

and preferably with the link to this post or to the 3 cases/explanations above.

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

No branches or pull requests

2 participants