Skip to content

Test when the fixer writes new class names - #6934

Merged
staabm merged 1 commit into
phpstan:2.3.xfrom
SanderMuller:skip-unneeded-class-name-traversal
Nov 9, 2026
Merged

staabm merged 1 commit into
phpstan:2.3.xfrom
SanderMuller:skip-unneeded-class-name-traversal

Conversation

@SanderMuller

@SanderMuller SanderMuller commented Nov 4, 2026 •

Copy link
Copy Markdown
Contributor

087443f skips the traversal that writes new class names when no fix adds a name or changes a PHPDoc. It makes the same change as the first version of this PR, so this PR now adds only the tests.

  • NewClassNamesVisitorTest checks NewClassNamesVisitor::hasNewNames() directly. Each branch of the check makes at least one case fail when it is removed.
  • AddNamesRule gets an @add-doc-name tag that changes only the PHPDoc. Without it, every fix in the tests also added a name in PHP code, so no test covered the PHPDoc branch through RuleErrorTransformer.

🤖 Generated with Claude Code

@staabm

staabm commented Nov 8, 2026

Copy link
Copy Markdown
Contributor

ondrej worked on fixer performance. please double check whether this PR is still improving performance after rebase

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@SanderMuller
SanderMuller force-pushed the skip-unneeded-class-name-traversal branch from 2d5de61 to 89c716a Compare November 9, 2026 10:49
@SanderMuller SanderMuller changed the title Skip writing new class names when no fix added one Test when the fixer writes new class names Nov 9, 2026
@SanderMuller

Copy link
Copy Markdown
Contributor Author

087443f makes the same change as this PR, so after the rebase no src change is left, and the PR adds no speed. On bug-14996.php, 087443f alone takes the analysis from 2.07 s to 1.49 s (hyperfine, 10 runs each).

087443f has no test for the check, so I pushed the tests alone. NewClassNamesVisitorTest tests the check, and AddNamesRule gets a fix that changes only a PHPDoc.

@staabm

staabm commented Nov 9, 2026 •

Copy link
Copy Markdown
Contributor

so do I understand correctly that this PR now contains a test which succeeds atm but fails with 087443f reverted?

@SanderMuller

Copy link
Copy Markdown
Contributor Author

so do I understand correctly that this PR now contains a test which succeeds atm but fails with 087443f reverted?

Correct, Ondrej's work made our changes no longer needed, but this PR had a test that was missing in Ondrej's work that could have value to keep

@staabm
staabm merged commit de42b4f into phpstan:2.3.x Nov 9, 2026
575 checks passed
@staabm

staabm commented Nov 9, 2026

Copy link
Copy Markdown
Contributor

thank you!

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.

2 participants