Skip to content

Conversation

@VincentLanglet
Copy link
Contributor

@VincentLanglet VincentLanglet commented Dec 8, 2022

@VincentLanglet VincentLanglet marked this pull request as ready for review December 8, 2022 15:56
@orklah
Copy link
Collaborator

orklah commented Dec 8, 2022

Well, technically, they can return negative, even if it doesn't makes much sense.

What's PHPStan's stance on this?

@VincentLanglet
Copy link
Contributor Author

What's PHPStan's stance on this?

PHPStan rely on PHPStorm stubs.
So it returns errors like

Return type (int) of method Foo::count() should be covariant with return type (int<0, max>) of method  
         Countable::count()

But might be worth asking to @ondrejmirtes if he's ok with int<0, max> from PHPStorms stubs or if he want to override it.

Currently if I change to int<0, max>, I get an error with psalm, that's why I opened this PR.
(And I will still get the error since I currently use Psalm 4 since all plugins are not compatible with v5 ^^')

@ondrejmirtes
Copy link

I tried it but it broke too much stuff, so currently it's only in bleedingEdge and will be enabled for everyone in PHPStan 2.0: https://github.com/phpstan/phpstan-src/blob/1.9.x/stubs/bleedingEdge/Countable.stub

Current non-bleedingEdge users get int only.

@VincentLanglet
Copy link
Contributor Author

VincentLanglet commented Dec 8, 2022

I tried it but it broke too much stuff, so currently it's only in bleedingEdge and will be enabled for everyone in PHPStan 2.0: https://github.com/phpstan/phpstan-src/blob/1.9.x/stubs/bleedingEdge/Countable.stub
Current non-bleedingEdge users get int only.

Oh, I see.
Since i always use bleedingEdge that's why I had the error.

Still, in the futur, int<0, max> will be use then.
So it's a good thing to start adding PR on different projects to use a more precise PHPdoc.

Psalm doesn't have the bleeding edge solution, so I dunno if it's too early to merge this then.
We need to rely on bleedingEdge users to open PR like doctrine/collections#355 before

@orklah orklah added the release:fix The PR will be included in 'Fixes' section of the release notes label Dec 8, 2022
@orklah
Copy link
Collaborator

orklah commented Dec 8, 2022

I'm fine with that then :) Thanks!

@orklah orklah merged commit e4b64af into vimeo:master Dec 8, 2022
@VincentLanglet VincentLanglet deleted the count branch December 9, 2022 00:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

release:fix The PR will be included in 'Fixes' section of the release notes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants