Repository navigation
Make --fix fast and complete: linear patching, multi-node fixes, fix rounds - #6902
Merged
Merged
Conversation
Patcher re-diffed the whole accumulated file against the original for every diff, which is quadratic in file size times number of fixes and made --fix hang on files with hundreds of fixable errors. All diffs share the same base, so their change runs can be spliced into the original in one pass. A diff overlapping an already accepted one is now skipped on its own instead of discarding every fix in the file, and the summary no longer counts skipped errors as fixed. This removes the ondrejmirtes/php-merge dependency. Amp-Thread-ID: https://ampcode.com/threads/T-01a198ef-d81e-7532-81f4-438bae7a4db3
sebastian/diff strips the common trailing lines by prepending to an array, which is quadratic, so a one-line fix near the top of an 11k-line file took 0.7 s to diff. Trim the common prefix and suffix in linear time and only diff the changed middle. The output is byte-identical. Amp-Thread-ID: https://ampcode.com/threads/T-01a198ef-d81e-7532-81f4-438bae7a4db3
Reverts 62a1233 now that --fix no longer hangs on many fixes in one file. Amp-Thread-ID: https://ampcode.com/threads/T-01a198ef-d81e-7532-81f4-438bae7a4db3
Errors of CollectedDataNode rules come without the file AST, so the fix is computed on the file parsed by the cached analysis parser. The rule has to take the fixed node from the same parser so that it is found by identity. A reused CollectedDataNode rule result is discarded when a file it fixes has changed, otherwise its diffs would never apply. Amp-Thread-ID: https://ampcode.com/threads/T-01a198ef-d81e-7532-81f4-438bae7a4db3
RuleTestCase::fix() takes the file the fixes are applied to when it is not the analysed file, analyses it as well because traits are analysed only in analysed files, and applies only the diffs of that file. Amp-Thread-ID: https://ampcode.com/threads/T-01a198ef-d81e-7532-81f4-438bae7a4db3
The fix was computed on the AST of the analysed file, so a node from the file the error points to was never found and the fix was silently dropped. Amp-Thread-ID: https://ampcode.com/threads/T-01a198ef-d81e-7532-81f4-438bae7a4db3
Fixes an error by removing a node that is in an array, like a statement, a class member, an argument, or an array item. A fixNode() callback can return NodeVisitor::REMOVE_NODE to the same effect. A node that is not in an array cannot be removed and the error is not fixable. Amp-Thread-ID: https://ampcode.com/threads/T-01a198ef-d81e-7532-81f4-438bae7a4db3
The fixer is going to apply diffs to edited contents that are not written to disk yet. Amp-Thread-ID: https://ampcode.com/threads/T-01a198ef-d81e-7532-81f4-438bae7a4db3
ondrejmirtes
force-pushed
the
fix-in-rounds
branch
6 times, most recently
from
November 2, 2026 07:33
620a9fe to
f6a1605
Compare
ondrejmirtes
force-pushed
the
fix-in-rounds
branch
from
November 2, 2026 07:54
9039b23 to
b435dc2
Compare
Errors ignored by ignoreErrors, the baseline, or inline @PHPStan-Ignore comments can be fixed with --fix-ignored, alone or together with --fix. The table error formatter mentions them in grey, also when the build is green. Amp-Thread-ID: https://ampcode.com/threads/T-01a198ef-d81e-7532-81f4-438bae7a4db3
Each call adds a change to the fix of the error instead of replacing the previous one, and can point to another file. The changes of one file are applied in a single traversal, several changes of the same node in the order they were added. An error gets one diff per file, and Patcher applies the diffs of an error together or not at all, also across files. When some change cannot be applied, the error is not fixable. Amp-Thread-ID: https://ampcode.com/threads/T-01a198ef-d81e-7532-81f4-438bae7a4db3
When fixes remove every constant of a const group, every property of a property group, every use of a use statement etc., the remaining statement is invalid PHP, so ReplacingNodeVisitor removes it too. Amp-Thread-ID: https://ampcode.com/threads/T-01a198ef-d81e-7532-81f4-438bae7a4db3
ondrejmirtes
force-pushed
the
fix-in-rounds
branch
from
November 2, 2026 08:41
b435dc2 to
8a86477
Compare
After the first round of fixes is written, --fix analyses the changed code again in a new process, which also saves the result cache, and fixes what is still fixable: fixes skipped because they conflicted with another fix, and errors that the previous fixes caused. It stops when nothing is applied or after FIX_ROUNDS_LIMIT rounds. The reported count is the number of fixes applied in all rounds, and errors that are still fixable at the end are reported as not fixed. Amp-Thread-ID: https://ampcode.com/threads/T-01a198ef-d81e-7532-81f4-438bae7a4db3
A CollectedDataNode rule parses the files it fixes itself. The parser cache keeps the nodes of at most 256 files, so in a big project the file was parsed again before the fix was made and none of the nodes were found. Matching the nodes by class and position in the file works with any parse of the same file contents. Amp-Thread-ID: https://ampcode.com/threads/T-01a198ef-d81e-7532-81f4-438bae7a4db3
Rounds after the first one showed no progress bar, so a long analysis of the fixed code looked stuck. The new process now streams its progress bar, every round reports how many errors it fixed in how many files, and the changed files are listed at the end. Amp-Thread-ID: https://ampcode.com/threads/T-01a198ef-d81e-7532-81f4-438bae7a4db3
--fix fixes errors itself since a long time, the note only adds noise to every run. Amp-Thread-ID: https://ampcode.com/threads/T-01a198ef-d81e-7532-81f4-438bae7a4db3
The contents of a file were cached by its path for the whole process. When the file changed between analyses in one process, like in PHPStan Pro or in tests, the diffs were computed against the old contents and were never applied.
Like phpstan analyse --fix, the fixed code is analysed again until nothing is left to fix, so fixes that overlap with other fixes and fixes of errors caused by other fixes are applied too. Later rounds analyse copies of the files in a temporary directory, next to each other. A test whose fix does not make its error go away now fails, the same fix would not finish with --fix either. FixIssetRule renamed variables forever, it now renames each one once.
ErrorFixer::fix() applies the fixes of the given errors to the files and tells which errors were fixed and which were skipped because their fixes overlap. Error::isFixable() tells whether an error has a fix. Extensions like an error formatter can fix errors without the diffs and the Patcher, which stay internal. --fix uses it too.
The instructions for agents now say how many of the reported errors can be fixed automatically, so the agent runs --fix before fixing the rest by hand. Amp-Thread-ID: https://ampcode.com/threads/T-01a198ef-d81e-7532-81f4-438bae7a4db3
Format-preserving printing needs token indexes, which the PHPDoc parser used by analysis does not collect, so the editor parses with its own configuration. editText() edits a PHPDoc that is not attached to a node. Amp-Thread-ID: https://ampcode.com/threads/T-01a198ef-d81e-7532-81f4-438bae7a4db3
ignore.unmatchedIdentifier removes the identifier from the @PHPStan-Ignore comment, or the whole ignore when it is the only one. ignore.unmatchedLine removes the @phpstan-ignore-line / -next-line ignore. Plain comments are edited as text and left alone when they have other contents. PHPDocs are edited with PhpDocEditor, so their other tags are kept. Amp-Thread-ID: https://ampcode.com/threads/T-01a198ef-d81e-7532-81f4-438bae7a4db3
Matches the table output, so an error becoming fixable or no longer fixable shows up in the diff. Amp-Thread-ID: https://ampcode.com/threads/T-01a198ef-d81e-7532-81f4-438bae7a4db3
ondrejmirtes
force-pushed
the
fix-in-rounds
branch
from
November 2, 2026 08:45
8a86477 to
0933083
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Makes
--fixandfixNode()fast and robust enough to promote. Each commit stands on its own.Performance
Patcherapplies diffs in a single linear pass, and thephp-mergedependency is removed. Applying 1000 fixes from the result cache to an 11k-line file used to hang; it now takes 0.3–0.4 s.Rule API
fixNode()works inCollectedDataNoderules, inside traits, and in files set withRuleErrorBuilder::file().RuleErrorBuilder::removeNode(). It removes a node that sits in an array, such as a statement, an argument or an array item.fixNode()/removeNode()call of an error is kept; later calls no longer overwrite earlier ones. One fix can touch several nodes in several files and is applied all or nothing. A statement whose items were all removed (const, a property group,use,global,static,unset,echo) is removed too.fixNode()andremoveNode()are no longer@internal.CLI
--fix-ignoredoption, with a grey hint for fixable ignored errors. It can be used alone or together with--fix.--fixnow runs in rounds. After a round's fixes are written, the changed code is analysed again in a new process, which also saves the result cache. That round applies fixes skipped because of conflicts and fixes errors caused by the previous round. Fixing stops when nothing more is applied or afterFIX_ROUNDS_LIMIT(10) rounds, and the fixed count is accurate.Measured (warm result cache)
Tests: new rule tests in
tests/PHPStan/Fixable, and a newe2e/fixtest with custom rules (chained fixes, conflict retry,--fix-ignored, the round limit).