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

max()/min() should expect non-empty-array #2163

Closed
wants to merge 1 commit into from
Closed

Conversation

staabm
Copy link
Contributor

@staabm staabm commented Jan 4, 2023

fix was easier then expected ;)

closes phpstan/phpstan#7239

@staabm
Copy link
Contributor Author

staabm commented Jan 4, 2023

@staabm staabm marked this pull request as ready for review January 4, 2023 12:24
@phpstan-bot
Copy link
Collaborator

This pull request has been marked as ready for review.

@ondrejmirtes
Copy link
Member

These failures are the reason why I don't like narrowing the parameter types of built-in functions...

I don't see it being justified enough to force user to write non-empty-array PHPDoc in their own code after patch PHPStan version upgrade...

@staabm
Copy link
Contributor Author

staabm commented Jan 4, 2023

I agree with this sentiment in general.

in this case though the error we protect the user from is a fatal error (since php8+):
https://3v4l.org/OE4CD
https://3v4l.org/vhUXS

@ondrejmirtes
Copy link
Member

Yeah but in existing code, it's very likely these arrays are always empty, although not annotated like that...

@staabm
Copy link
Contributor Author

staabm commented Jan 4, 2023

not sure I get your point: do you mean there are a lot of situations in which the array will be empty but we don't know about it?

@hemberger
Copy link

Yeah but in existing code, it's very likely these arrays are always empty, although not annotated like that...

I think maybe "are always non-empty" is what was meant?

In the codebases that I've been analyzing recently, there are definitely edge cases where min/max will be passed an empty array unintentionally. No warning is emitted by PHPStan currently (as of 1.9.13), though I was actually more aware of these cases prior to the fix to the PHP 8+ min/max return types in #2161, since I would get an (erroneous) warning about using its result, e.g. int|false when int was expected.

It always seemed to me that PHPStan was designed to not let errors like these raise naturally, and so enforcing non-empty-array seems technically correct (and genuinely useful in some cases). But it is likely to be a pedantic warning in many cases, and it is an arduous task to assess the "emptiness" of every array. Does it perhaps warrant a stricter analysis parameter such as checkEmptyArrayPassedToMinMax?

Either way, I fully trust you two to settle on the solution that is best for the PHP community. :)

@staabm
Copy link
Contributor Author

staabm commented Jan 19, 2023

I think one way could be to enable the feature only in bleeding edge, or alternatively only in phpstan-strict-rules?

@VincentLanglet
Copy link
Contributor

the error in

sebastianbergmann/phpunit@93d4bf4/src/TextUI/DefaultResultPrinter.php#LL541C30-L541C30

seems valid

I might be wrong but itt's not really valid for

$lines = preg_split('/\r\n|\r|\n/', $buffer);
$padding = max(array_map('\strlen', $lines));

I would expect preg_split to returns a non-empty-array (like the explode), then array_map doesn't touch the number of arguments.

Currently it's considered as an array (https://phpstan.org/r/284ce5f5-f222-4518-8144-a90ead09b60c)
Do you think preg_split return type can be changed to non-empty-array|false ?

@ondrejmirtes ondrejmirtes mentioned this pull request Feb 6, 2023
@VincentLanglet
Copy link
Contributor

I think one way could be to enable the feature only in bleeding edge, or alternatively only in phpstan-strict-rules?

To me, bleeding edge can be considered as "I'm sure we will release this feature in 2.0, but we want to avoid BC break".
Here, it's not sure we want this change because it could add to many noise in some codebase.

What about an experimental flag ?
experimental: false allow to disable some debatable features, and it still allow to release this to get opinion on such change ?

@ondrejmirtes
Copy link
Member

Did bleedingEdge support here: 06b746d

Merged this PR as: 35b4cac

Thank you.

@staabm staabm deleted the maxmin branch April 5, 2023 08:49
@staabm
Copy link
Contributor Author

staabm commented Apr 5, 2023

thanks

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

Successfully merging this pull request may close these issues.

max()/min() should expect non-empty-array when called with a single arg
5 participants