Repository navigation
Conversation
PHPStan 2.3.5 marks errors it can fix automatically and suggests to run it again with --fix. When the analysis reports such errors, the phpstan task now returns a fixable result that reruns the same command with --fix, like the other fixable tasks. The fixer is only offered when PHPStan reports fixable errors: without them, "phpstan analyse --fix" fails with "No fixable errors found".
| private function hasFixableErrors(Process $process): bool | ||
| { | ||
| return 1 === preg_match( | ||
| '/Run PHPStan again with --fix(?![\w-])/', |
There was a problem hiding this comment.
I'm wondering if this is the best way to detect it is fixable or not.
If they change the text, this will fail.
Most other tasks similar to this one just try to fix the issue without checking if they actually can. So this might be a good addition, i'm just wondering the "how" is good enough or could be detected differently.
There was a problem hiding this comment.
Good question. I considered always offering the fixer like ECS/Rector, but for PHPStan that has two downsides:
- Unlike ECS/Rector, most PHPStan errors (e.g. type errors) can't be fixed automatically. GrumPHP would ask "I can fix some stuff automatically" on almost every failing run, and
phpstan analyse --fixthen exits with 1 ("No fixable errors found"), so the fixer reports ✘. - Up to PHPStan 2.1,
--fixlaunches PHPStan Pro (it downloads the Pro executable and starts it) instead of fixing.
If PHPStan rewords the hint, hasFixableErrors() returns false and the task behaves exactly like today (failed result without fixer), so it degrades gracefully instead of breaking.
There is a structured alternative: with --error-format=json, every fixable message has "fixable": true. The options I see:
- A: keep the hint detection as it is.
- B: on failure, run a second
analyse --error-format=jsononly to detect fixable errors. This keeps PHPStan's table output, but costs an extra run (which should mostly be served from the result cache). - C: switch the task to
--error-format=jsonand render the output in GrumPHP, like the phpcs task does with--report-json. No extra run, but PHPStan's own table output is lost. - D: always offer the fixer like ECS/Rector, with the downsides above.
Which one do you prefer? I'll adapt the PR accordingly.
| private const FIXABLE_IGNORED_OUTPUT = <<<'TXT' | ||
| [ERROR] Found 1 error | ||
|
|
||
| 🔧 1 ignored error can be fixed automatically. Run PHPStan again with --fix-ignored. |
There was a problem hiding this comment.
what's the difference between regular fixable and fixable ignored options?
Should we support both somehow?
There was a problem hiding this comment.
--fix fixes the errors PHPStan reports. --fix-ignored fixes errors that are currently ignored via the baseline, ignoreErrors or @phpstan-ignore, and then removes the ignore entries that are no longer needed. That's mainly useful to shrink a baseline, e.g. after upgrading to PHPStan 2.3.5, where many baselined errors like method.missingOverride became auto-fixable.
I tried combining both (--fix --fix-ignored), but it doesn't work with GrumPHP yet: when only the changed files are passed to PHPStan (the default use_grumphp_paths: true), the ignored errors get fixed but the baseline entries stay, because PHPStan doesn't report unmatched ignores for a list of files, so its later fix rounds never remove them. The next full run (e.g. in CI) then fails with ignore.unmatched. The same happens with --fix-ignored alone.
That's a bug in PHPStan: I reported it in phpstan/phpstan#15704 and opened a fix in phpstan/phpstan-src#7036. Since ignored errors don't make the task fail anyway, this PR only uses --fix. Once the fix is released, adding --fix-ignored (e.g. as an opt-in option) could be a follow-up.
1f28655 to
4952a67
Compare
| private const FIXABLE_IGNORED_OUTPUT = <<<'TXT' | ||
| [ERROR] Found 1 error | ||
|
|
||
| 🔧 1 ignored error can be fixed automatically. Run PHPStan again with --fix-ignored. |
There was a problem hiding this comment.
--fix fixes the errors PHPStan reports. --fix-ignored fixes errors that are currently ignored via the baseline, ignoreErrors or @phpstan-ignore, and then removes the ignore entries that are no longer needed. That's mainly useful to shrink a baseline, e.g. after upgrading to PHPStan 2.3.5, where many baselined errors like method.missingOverride became auto-fixable.
I tried combining both (--fix --fix-ignored), but it doesn't work with GrumPHP yet: when only the changed files are passed to PHPStan (the default use_grumphp_paths: true), the ignored errors get fixed but the baseline entries stay, because PHPStan doesn't report unmatched ignores for a list of files, so its later fix rounds never remove them. The next full run (e.g. in CI) then fails with ignore.unmatched. The same happens with --fix-ignored alone.
That's a bug in PHPStan: I reported it in phpstan/phpstan#15704 and opened a fix in phpstan/phpstan-src#7036. Since ignored errors don't make the task fail anyway, this PR only uses --fix. Once the fix is released, adding --fix-ignored (e.g. as an opt-in option) could be a follow-up.
| private function hasFixableErrors(Process $process): bool | ||
| { | ||
| return 1 === preg_match( | ||
| '/Run PHPStan again with --fix(?![\w-])/', |
There was a problem hiding this comment.
Good question. I considered always offering the fixer like ECS/Rector, but for PHPStan that has two downsides:
- Unlike ECS/Rector, most PHPStan errors (e.g. type errors) can't be fixed automatically. GrumPHP would ask "I can fix some stuff automatically" on almost every failing run, and
phpstan analyse --fixthen exits with 1 ("No fixable errors found"), so the fixer reports ✘. - Up to PHPStan 2.1,
--fixlaunches PHPStan Pro (it downloads the Pro executable and starts it) instead of fixing.
If PHPStan rewords the hint, hasFixableErrors() returns false and the task behaves exactly like today (failed result without fixer), so it degrades gracefully instead of breaking.
There is a structured alternative: with --error-format=json, every fixable message has "fixable": true. The options I see:
- A: keep the hint detection as it is.
- B: on failure, run a second
analyse --error-format=jsononly to detect fixable errors. This keeps PHPStan's table output, but costs an extra run (which should mostly be served from the result cache). - C: switch the task to
--error-format=jsonand render the output in GrumPHP, like the phpcs task does with--report-json. No extra run, but PHPStan's own table output is lost. - D: always offer the fixer like ECS/Rector, with the downsides above.
Which one do you prefer? I'll adapt the PR accordingly.
|
@veewee FYI, Ondřej's answer on the PHPStan issue: phpstan/phpstan#15704 (comment)
(see also https://phpstan.org/blog/why-you-should-always-analyse-whole-project) I think we should take this into account in GrumPHP. By default, the phpstan task only passes the changed files to PHPStan ( Should we analyse the whole project by default, as configured in I'd suggest handling this in a separate PR, so this one stays focused on |
PHPStan 2.3.5 made many built-in errors auto-fixable (e.g.
method.missingOverride,cast.useless,phpunit.*). It marks them with 🔧 and ends its report with "Run PHPStan again with --fix".This PR plugs that into GrumPHP's fixer the same way as the other fixable tasks (ECS, Rector, Mago): when the analysis fails and PHPStan reports fixable errors, the
phpstantask returns aFixableTaskResultthat reruns the samephpstan analysecommand with--fix(same files, configuration, level and memory limit).Why only when PHPStan reports fixable errors?
phpstan analyse --fixexits with 1 and "No fixable errors found", so GrumPHP would ask to fix and then report a failing fixer.--fixlaunched PHPStan Pro instead of fixing.)The hint for fixable ignored errors (
--fix-ignored) is deliberately not taken into account, since ignored errors don't make the task fail. If PHPStan ever rewords the hint, the task falls back to the current non-fixable behaviour.Tested manually against the PHPStan phars
--fixexits with 0 even though the non-fixable error remains, the fixer reports ✔ and only#[\Override]was added.The docs mention that
--fixis experimental in PHPStan and also removes unmatchedignoreErrorsentries from the configuration/baseline and unmatched@phpstan-ignorecomments.New Task Checklist:
phpstan/phpstanis already suggested)doc/tasks/phpstan.md)test/Unit/Task/PhpStanTest.phpcovers fixable, non-fixable and fixable-ignored output and the fix command)run()method readable?run()method using the configuration correctly? (the fix command reuses the same arguments)