TableErrorFormatter: Limit number of errors#5136
TableErrorFormatter: Limit number of errors#5136ondrejmirtes merged 25 commits intophpstan:2.1.xfrom
Conversation
VincentLanglet
left a comment
VincentLanglet
left a comment
There was a problem hiding this comment.
A (big) file with looooot of errors will still display way more than 1000 errors since you break between files.
Also I find weird that something like PHPSTAN_ERRORS_LIMIT=1 will display more than 1 error (if the first file has more than 1 error).
Should we break inside the file foreach too ?
yeah, I did it intentionally like this. |
ondrejmirtes
left a comment
ondrejmirtes
left a comment
There was a problem hiding this comment.
Instead of PHPSTAN_ERRORS_LIMIT variable where user can shoot themselves in the foot with a non-sense value like 3, I'd more like a boolean-like var with the name of PHPSTAN_TABLE_ERROR_FORMATTER_FORCE_SHOW_ALL_ERRORS. The default (undefined or 0) would only show the first 1000 errors, setting to 1 would show everything and hide the note.
| if ($errorsBudget > 0 && $printedErrors > $errorsBudget) { | ||
| $note = []; | ||
| $note[] = sprintf('Result is limited to the first %d errors', $errorsBudget); | ||
| $note[] = '- Consider lowering the PHPStan level'; |
There was a problem hiding this comment.
Only output this on rule level > 0.
There was a problem hiding this comment.
On this line show "1000+ errors" when applicable. Yeah, it's meant to be annoying :)
|
latest result 
|
| $fileErrors[$fileSpecificError->getFile()][] = $fileSpecificError; | ||
| } | ||
|
|
||
| $forceShowAll = getenv('PHPSTAN_TABLE_ERROR_FORMATTER_FORCE_SHOW_ALL_ERRORS'); |
There was a problem hiding this comment.
Would be nice to have this name here in a private constant, it's referred to twice in the source code.
| if (in_array($forceShowAll, [false, '0'], true)) { | ||
| $errorsBudget = self::ERRORS_LIMIT; | ||
| } else { | ||
| $errorsBudget = 0; |
| #[AutowiredParameter] | ||
| private ?string $editorUrlTitle, | ||
| #[AutowiredParameter] | ||
| private ?int $level, |
There was a problem hiding this comment.
usedLevel might work a bit better, that's what's used across the codebase for this.
|
Memo to me: check whether the new limit also affects the baseline generation |
It won't, you just touched TableErrorFormatter, baseline is generated with a different one. |
|
|
||
| $printedErrors += count($rows); | ||
| if ($errorsBudget !== null && $printedErrors > $errorsBudget) { | ||
| $rows = array_slice($rows, 0, $errorsBudget - ($printedErrors - count($rows))); |
There was a problem hiding this comment.
Last thing - I'm not sure whether this line works correctly, a lot is going on there.
For example it's easy to make a mistake in $errorsBudget - ($printedErrors - count($rows)) based on whether $printedErrors already includes current count($rows) or if the += operation happened after this computation.
I'm thinking this should really have some unit tests.
My suggestion - constructor of TableErrorFormatter should have a new optional nullable parameter. If it's null, then the autodetection based on the env variable will happen. If it's not null then that will become the error limit.
The unit test will then construct TableErrorFormatter with a specific value, can be 5. And we should test the output for < 5, exactly 5 and > 5.
| int $generateErrorsCount = 4, | ||
| ): void | ||
| { | ||
| putenv('COLUMNS=200'); |
There was a problem hiding this comment.
took me a while to realize that I need this env var in the test, to get stable results across local workstation and CI :-)
|
@ondrejmirtes finally the tests "work" cross plattform. had to skip windows because of slight line-break differences enforced by symfony/console. but I think that doesn't matter for the tests we do here. |
This reverts commit ddf2bc9.
|
remaining errors also happen on 2.1.x - so seem unrelated |
|
Based on the issue description the performance issue should be given by symfony/console. Why not fix the issue there and possibly patch it until released? I personally prefer all errors shown as very often I do ctrl+f and want to know how many occurences of something are present. |
|
Love it, thank you! |
|
@mvorisek For your use-case, see the error message that's printed when there are 1000+ errors. |
|
opened a upstream issue, so symfony knows about the performance and might improve it in the future: |
4 participants
closes phpstan/phpstan#14232
"mago benchmark" running PHPStan with warmed cache on macbook m4 pro with PHP 8.3.34
before this PR: 20-21 seconds
after this PR: 3-4 seconds
running on wordpress with warmed result cache
note the difference in "Elapsed time"
before the PR
after the PR
../../bin/phpstan -vPHPSTAN_ERRORS_LIMIT=0 ../../bin/phpstan -vPHPSTAN_ERRORS_LIMIT=50 ../../bin/phpstan -v