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

#6840 Report useless array filter #1077

Merged
merged 4 commits into from
Mar 18, 2022

Conversation

leongersen
Copy link
Contributor

@leongersen leongersen commented Mar 16, 2022

New branch based on 1.5.x, I could not reopen #1076.

Implements phpstan/phpstan#6840.

@leongersen leongersen force-pushed the useless-array-filter-2 branch 2 times, most recently from 8317d8e to 91c6a48 Compare March 16, 2022 11:49
@leongersen leongersen changed the base branch from 1.4.x to 1.5.x March 16, 2022 11:49
@leongersen leongersen closed this Mar 16, 2022
@leongersen leongersen reopened this Mar 16, 2022
@ondrejmirtes
Copy link
Member

I'm looking at this - it's really nice, Some observations that would make this even better:

  1. Calling array_filter without callback on always-truthy values is similarly useless as on always-falsy values. Please detect that too.
  2. We should also detect situations with array_filter($array, null, ARRAY_FILTER_USE_KEY) and array_filter($array, null, ARRAY_FILTER_USE_BOTH) - not sure how the latter one behaves, needs to be tested.

@leongersen
Copy link
Contributor Author

  1. Calling array_filter without callback on always-truthy values

That's what this PR checks for. Do you mean the opposite? Calling array_filter on only falsy values will always result in [] and is likely an error, but not without effect.

  1. Sounds good, I'll add support for that.

@ondrejmirtes
Copy link
Member

Yes, that's what I meant, the rule should report both always-truthy and always-falsy arrays :)

@herndlm
Copy link
Contributor

herndlm commented Mar 17, 2022

USE_BOTH without a callback only uses the first arg (= value) AFAIK, see also https://github.com/phpstan/phpstan-src/blob/1.4.10/tests/PHPStan/Analyser/data/array-filter.php#L36

But better double-check :) if the referenced test is wrong, I need to fix smth in the ArrayFilter extension

@leongersen
Copy link
Contributor Author

I checked, passing null explicitly as the callback (with or without the mode parameter) results in the following warning:

Warning: array_filter() expects parameter 2 to be a valid callback, no array or string given

@herndlm
Copy link
Contributor

herndlm commented Mar 17, 2022

Ah maybe that explains why the key mode test above my rererence also looks weird 😅 would be interesting if and how it filters

@leongersen leongersen force-pushed the useless-array-filter-2 branch from 91c6a48 to 58fc6cf Compare March 17, 2022 15:07
@leongersen
Copy link
Contributor Author

It doesn't filter at all:

var_dump(@array_filter([0,1], null)); => NULL

I've added support for reporting arrays with only falsey values.

@leongersen leongersen force-pushed the useless-array-filter-2 branch from 8d6237f to 4ac9256 Compare March 17, 2022 16:16
@leongersen
Copy link
Contributor Author

leongersen commented Mar 17, 2022

The failing test is unrelated.

@ondrejmirtes
Copy link
Member

The failure is definitely related as the file contains this: array_filter([]);

Feel free to modify the file to contains something that's valid, like [0, 1, 2] :)

@leongersen leongersen force-pushed the useless-array-filter-2 branch from 4ac9256 to 713dc09 Compare March 17, 2022 17:04
@leongersen
Copy link
Contributor Author

Turns out file-without-errors is not without errors 🙈

@leongersen leongersen force-pushed the useless-array-filter-2 branch 2 times, most recently from bb60b4d to 7f6853a Compare March 17, 2022 17:22
@ondrejmirtes ondrejmirtes force-pushed the useless-array-filter-2 branch from 7f6853a to b29a267 Compare March 18, 2022 12:41
@ondrejmirtes
Copy link
Member

I pushed some extra commits to adjust this to my taste.

@ondrejmirtes ondrejmirtes merged commit 610b396 into phpstan:1.5.x Mar 18, 2022
@ondrejmirtes
Copy link
Member

Thank you.

@ondrejmirtes
Copy link
Member

Also to keep the BC promise (https://phpstan.org/user-guide/backward-compatibility-promise) I had to do this: 7627b52

The library of performed rules on analysed code, and the meaning of rule levels does not change in minor/patch versions. This means that PHPStan will not look for new categories of errors in a minor/patch version. New rules are added only in major versions, and also as part of bleeding edge which is an opt-in preview of the next major version.

@herndlm
Copy link
Contributor

herndlm commented Mar 18, 2022

@leongersen fyi I think null callables would work fine, they only work since PHP 8 and weirdly only ever filter by value (= ignore any given mode)

See https://3v4l.org/Cj17A

@leongersen leongersen deleted the useless-array-filter-2 branch March 28, 2022 07:20
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.

3 participants