Skip to content

Make AssertSameWithCountRule auto-fixable - #259

Merged
staabm merged 13 commits into
phpstan:2.1.xfrom
staabm:autof
Oct 6, 2026
Merged

staabm merged 13 commits into
phpstan:2.1.xfrom
staabm:autof

Conversation

@staabm

@staabm staabm commented Nov 11, 2025 •

Copy link
Copy Markdown
Contributor

closes #249


skip named and splat args for auto-fixing. PHPUnit does not support named arguments for assert*.

see https://github.com/sebastianbergmann/phpunit/blob/bfa72f3d37a2fcb76ec57c87728f3623db06c035/src/Framework/Assert.php#L72-L74

@staabm
staabm marked this pull request as ready for review November 11, 2025 09:53
@staabm

staabm commented Dec 6, 2025

Copy link
Copy Markdown
Contributor Author

@ondrejmirtes here another low hanging fruit :-)

@ondrejmirtes ondrejmirtes left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

A fix and a test for tricky situations with named arguments would be nice here 😊

@staabm
staabm changed the base branch from 2.0.x to 2.1.x October 5, 2026 14:51
@staabm staabm changed the title Make AssertSameWithCountRule auto-fixable Make AssertSameWithCountRule auto-fixable Oct 5, 2026
@staabm
staabm requested a balanced review from Copilot October 5, 2026 15:46

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot review overview

🟡 Changes recommended

The fixer can alter unrelated arguments, mishandle unpacking, skip valid fixes, and suppress existing diagnostics.

Review effort: Balanced
Findings: 1 Medium severity

Open (1)
What changed in this PR

Adds autofix support for replacing assertSame(..., count(...)) patterns with assertCount(...).

Changes:

  • Adds AST-based assertion rewriting.
  • Adds autofix fixtures and named-argument coverage.
  • Preserves unsupported recursive-count expressions.
File Description
src/​Rules/​PHPUnit/​AssertSameWithCountRule.php Implements autofix generation.
tests/​Rules/​PHPUnit/​AssertSameWithCountRuleTest.php Tests fixes and named arguments.
tests/​Rules/​PHPUnit/​data/​assert-same-count-fixable.php Provides autofix input cases.
tests/​Rules/​PHPUnit/​data/​assert-same-count-fixable.php.fixed Defines expected fixed output.
tests/​Rules/​PHPUnit/​data/​assert-same-count-named-arguments.php Covers named-argument cases.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread src/Rules/PHPUnit/AssertSameWithCountRule.php Outdated

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot review overview

🟡 Changes recommended

The fixer can incorrectly rewrite non-actual and unpacked arguments, and its named-argument safeguards are not exercised.

Review effort: Balanced
Findings: 1 Medium severity · 1 Low severity

Open (2)
Previously missed (2)

In code that hasn't changed since last review

Medium severity Restrict count transformation to the counted argument

src/​Rules/​PHPUnit/​AssertSameWithCountRule.php:166

Only the second argument is the counted expression, but this branch currently processes every call-like argument. For example, assertSame(count($expected), count($actual)) is rewritten to assertCount($expected, $actual), changing the expected count; call-like message arguments can also unnecessarily suppress the fix. Restrict this transformation to argument index 1 and add a regression case.

Medium severity Skip auto-fixes for unpacked count arguments

src/​Rules/​PHPUnit/​AssertSameWithCountRule.php:177

An unpacked argument is not equivalent to its underlying expression. For example, count(...$arrays) counts the unpacked array while the generated assertCount(..., $arrays) counts the outer array, so this auto-fix changes behavior. Treat an unpacked count argument as non-fixable.

Comment thread tests/Rules/PHPUnit/AssertSameWithCountRuleTest.php

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot review overview

🔵 Needs a closer look

The fixer can incorrectly unwrap call expressions outside the actual-value argument, changing assertion semantics.

Review effort: Balanced
Findings: 1 Medium severity · 1 Low severity

Open (2)
Previously missed (1)

In code that hasn't changed since last review

Medium severity Restrict call argument unwrapping to the actual value

src/​Rules/​PHPUnit/​AssertSameWithCountRule.php:166

Only the second ($actual) argument should be unwrapped. As written, every call-like argument is rewritten, so assertSame(count($expected), count($actual)) becomes assertCount($expected, $actual) instead of preserving count($expected) as the expected integer. Restrict this branch to index 1; this also avoids unnecessarily declining fixes when the expected value or message is another kind of call.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot review overview

🟡 Changes recommended

The fixer can rewrite the expected argument incorrectly and omits valid two-argument count() fixes.

Review effort: Balanced
Findings: 1 High severity · 1 Medium severity

Open (2)
Resolved since last review (1)

Comment thread src/Rules/PHPUnit/AssertSameWithCountRule.php Outdated

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot review overview

🟡 Changes recommended

The autofix incorrectly rewrites unpacked count() arguments and can change assertion results.

Review effort: Balanced
Findings: 1 High severity

Open (1)
Resolved since last review (2)

Comment thread src/Rules/PHPUnit/AssertSameWithCountRule.php

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot review overview

🔵 Needs a closer look

Safe two-argument count() diagnostics remain non-fixable.

Review effort: Balanced
Findings: None

Resolved since last review (1)
Previously missed (1)

In code that hasn't changed since last review

Medium severity Enable auto-fix for two-argument count() calls with normal mode

src/​Rules/​PHPUnit/​AssertSameWithCountRule.php:171

This rejects every two-argument count() call, although the rule already reports calls such as count($value, COUNT_NORMAL) (and calls whose inferred element type makes recursive mode equivalent). Those diagnostics therefore still have no auto-fix even though dropping the proven-normal mode is semantics-preserving; only malformed calls with more than two arguments need to remain unfixed.

@staabm
staabm merged commit b73700a into phpstan:2.1.x Oct 6, 2026
97 checks passed
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.

turn AssertSame* rules autofixable

4 participants