Skip to content

Per-file batch fixing for --fix option#5550

Open
mabar wants to merge 1 commit intophpstan:2.1.xfrom
mabar:fixer-optimization
Open

Per-file batch fixing for --fix option#5550
mabar wants to merge 1 commit intophpstan:2.1.xfrom
mabar:fixer-optimization

Conversation

@mabar
Copy link
Copy Markdown

@mabar mabar commented Apr 26, 2026

Each file's fixable errors are now applied in a single AST traversal that produces one diff per file, replacing the previous one-diff-per-error pipeline merged via PhpMerge.

Why

Previously, every RuleErrorBuilder::fixNode() call produced its own unified diff. Patcher::applyDiffs() then merged N diffs per file using PhpMerge::mergeHunks(), looping with a full-file Differ::diffToArray() per hunk. Cost grows quadratically with hunk count.

How

  • New BatchReplacingNodeVisitor matches all of a file's pending fixes via origNode attribute in a single NodeTraverser pass.
  • Fix callables run eagerly during the rule's pass (Fiber-scope is live, so $scope->getType() works inside callables) and the result node is memoized.
  • After all errors for a file are collected, one clone + one batch traversal + one prettyPrintFile + one diff → one FixedErrorDiff per file.
  • Nested fixes are handled naturally by PhpParser's traversal: when an outer fix is applied, the traverser descends into the new subtree and applies any inner fixes that the outer's callable preserved. Inner fixes whose original node was discarded are silently skipped.
  • Patcher::applyDiffs() is unchanged in shape but only ever sees one diff per file in the new path.

Benchmark

Via new PatcherBenchmarkTest, executed on AMD 9950X3D

hunks LoC baseline refactored speedup
5 100 2 ms 2 ms
20 500 213 ms 10 ms 21×
50 1000 2.38 s 28 ms 85×
100 1000 4.98 s 37 ms 135×

Real-world test

1977-file project, using two custom fixable rules with 376 violations across 145 files, some of them with 2-4k LoC. Without cache, analysis ran for 53.1s without measurable change from the previous state. Fix ran for 62.3s adding 9.2s on top of analysis.

@phpstan-bot
Copy link
Copy Markdown
Collaborator

You've opened the pull request against the latest branch 2.2.x. PHPStan 2.2 is not going to be released for months. If your code is relevant on 2.1.x and you want it to be released sooner, please rebase your pull request and change its target to 2.1.x.

@mabar mabar force-pushed the fixer-optimization branch from 70b6e85 to 3172df5 Compare April 26, 2026 22:33
@mabar mabar changed the base branch from 2.2.x to 2.1.x April 26, 2026 22:33
@mabar mabar force-pushed the fixer-optimization branch from 3172df5 to 5f1db78 Compare April 26, 2026 22:45
@mabar
Copy link
Copy Markdown
Author

mabar commented Apr 26, 2026

@phpstan-bot switched to 2.1.x 😘
Remaining CI failures seem unrelated to the changes

@mabar mabar force-pushed the fixer-optimization branch from 5f1db78 to df4076b Compare April 27, 2026 02:23
@mabar mabar force-pushed the fixer-optimization branch from df4076b to 865f049 Compare April 27, 2026 02:24
Copy link
Copy Markdown
Member

@ondrejmirtes ondrejmirtes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. How do you handle traits? Those files might be "fixed" from different usages at different points during the analysis.
  2. I'm torn about a problem I didn't realize. Let's say we gave two errors, both fixable, one ignored and one not. Running --fix will fix them both, right? We probably can't solve it becaus we need the one diff per file for performance, and at this point we don't (and should not) be aware what errors are ignored and not.

@ondrejmirtes
Copy link
Copy Markdown
Member

Also, probably, depending on the answer to the previous one, I would not attach the diff to an Error anymore. With one diff per file, it doesn't make sense. So we need a new field for this in the result cache, merging logic in the ResultCacheManager too.

@ondrejmirtes
Copy link
Copy Markdown
Member

At the same time it'd also be useful to track how many errors applying a fix fixes...

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