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

Conditional return type for container #7141

Closed
snapshotpl opened this issue Apr 27, 2022 · 12 comments
Closed

Conditional return type for container #7141

snapshotpl opened this issue Apr 27, 2022 · 12 comments

Comments

@snapshotpl
Copy link

snapshotpl commented Apr 27, 2022

Bug report

I'd like to create stub for Container with conditional return type, however looks like is not possible to do that because bug or missing feature.

Code snippet that reproduces the problem

https://phpstan.org/r/b559c7cd-8851-4953-a7ec-0d8b5b3cc714

Expected output

No error and correct type

@ondrejmirtes
Copy link
Member

ondrejmirtes commented Apr 27, 2022

class-string<T>|string gets normalized to string: https://phpstan.org/developing-extensions/type-system#type-normalization

But I think we might make this version work: https://phpstan.org/r/233f9fb4-884a-4d35-8807-66aa281c067b

WDYT @rvanvelzen ?

@snapshotpl
Copy link
Author

snapshotpl commented Apr 27, 2022

Your version looks better, but my works on psalm 🤔

@rvanvelzen
Copy link
Contributor

rvanvelzen commented Apr 28, 2022

To me it seems better to keep templated types when normalizing, which should make the original case work. I.e. string|class-stringstring but string|class-string<T>string|class-string<T>.

That being said, inferring from conditional types would be nice too.

@ondrejmirtes
Copy link
Member

ondrejmirtes commented Apr 28, 2022

I worry it’s not possible, string type contains all class-strings, even with generics. Changing that could break a lot of assumptions. But feel free to try it 😊

@rvanvelzen
Copy link
Contributor

rvanvelzen commented Apr 28, 2022

class-string<T> with an unbounded T is always a super type of a constant string, so the provided tests won't work as expected using ($id is class-string<T> ? T : mixed)syntax - it will always returnT`.

@ondrejmirtes
Copy link
Member

ondrejmirtes commented May 9, 2022

I think that we can make https://phpstan.org/r/233f9fb4-884a-4d35-8807-66aa281c067b work - ($id is class-string<T> ? T : mixed) is (string is class-string<T> ? T : mixed) so there shouldn't be an issue.

@zonuexe
Copy link
Contributor

zonuexe commented Jun 23, 2022

I got here from #7513.

This issue is very inconvenient for the general use case of PHP container libraries, but I agree that the phpstan/phpstan-src#1459 and phpstan/phpstan-src#1262 approaches are ugly hacks that make supertypes inconsistent.

#7141 (comment) looks like a good way to do it. However, we also note that @param is not only machine-readable, but also human-friendly documentation.

@ondrejmirtes
Copy link
Member

ondrejmirtes commented Sep 19, 2022

Implemented by phpstan/phpstan-src#1465

@staabm
Copy link
Contributor

staabm commented Sep 19, 2022

the initial example has less errors, but the return type seems still wrong?

https://phpstan.org/r/2210407c-a794-4637-8781-ffc19bd6d7a3

@ondrejmirtes
Copy link
Member

ondrejmirtes commented Sep 19, 2022

Only the one without @param is supposed to work.

@snapshotpl
Copy link
Author

snapshotpl commented Sep 19, 2022

Awesome! Great job!

@github-actions
Copy link

github-actions bot commented Oct 21, 2022

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 21, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

5 participants