Skip to content

AssertEmptyIsDiscouragedRule is auto-fixable - #342

Open
staabm wants to merge 8 commits into
phpstan:2.1.xfrom
staabm:auto-f
Open

staabm wants to merge 8 commits into
phpstan:2.1.xfrom
staabm:auto-f

Conversation

@staabm

@staabm staabm commented Oct 5, 2026 •

Copy link
Copy Markdown
Contributor

closes #338

support scalar types. we skip all regular strings which are not non-falsey, because fixing these would yield ugly code like $s == '' || $s == '0'.

we only support the easiest cases for now which have a unambiguous fix and lead more readable code.

@staabm

staabm commented Oct 5, 2026

Copy link
Copy Markdown
Contributor Author

//cc @SanderMuller

@staabm
staabm requested a review from VincentLanglet October 5, 2026 12:44

@SanderMuller SanderMuller left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I checked the fixes against what assertEmpty() does at runtime, at head e7037550f. make tests and make phpstan pass. In five cases the fixed assertion behaves differently from the original. I ran each original and fixed line with real PHPUnit 9.6:

Case Original Fixed Original result Fixed result
@param int $x, null at runtime assertNotEmpty($x) assertNotSame(0, $x) fails passes
@param non-falsy-string $x, '0' at runtime assertNotEmpty($x) assertNotSame('', $x) fails passes
?\SimpleXMLElement $x, <a/> assertEmpty($x) assertNull($x) passes fails
bool $b assertEmpty(actual: $b) assertFalse(actual: $b) passes Error: Unknown named parameter $actual
int $i, 0 assertEmpty(message: 'm', actual: $i) assertSame('', message: 'm', actual: $i) passes fails
  1. The type comes from $scope->getType(), so a PHPDoc type decides the fix. In a test, the assertion is often what checks that the PHPDoc is right. The negated fixes then accept values that the original rejected (the first two rows). $scope->getNativeType() would avoid that. The non-falsy-string case would then not be fixed at all, but assertEmpty() on a non-falsy string can never pass anyway.

  2. An empty SimpleXMLElement is empty for empty(), so assertNull() is not the same. ?object has the same problem, because the object can be a SimpleXMLElement at runtime. Fixing only when (new ObjectType(\SimpleXMLElement::class))->isSuperTypeOf(...) is no() for the type without null would keep ?\stdClass and skip both.

  3. The rule reads getArgs()[0] as the asserted value and keeps the arguments as they are:

    • With actual: as a named argument, assertFalse(), assertTrue(), assertCount() and assertNotCount() throw an Error, because their parameter is $condition or $haystack. assertSame(), assertNotSame(), assertNull() and assertNotNull() have $actual, so they work.
    • With message: first, the message string is treated as the asserted value, and the fix becomes assertSame('', ...).

    Skipping the fix when any argument is named would cover both.

The other fixes are equivalent for their types, including the extra message argument (assertCount(0, $array, 'message')). float, plain string and unions without null stay unfixed, as the description says.

The three Mutation Testing reds come from this PR. Infection reports 4 escaped mutants, which change ->yes() to !->no() on the isObject(), isBoolean(), isArray() and isInteger() checks. So the tests have no value for which one of these checks is a "maybe" and the fix must not apply. The other CI checks pass.

I only checked the parameter names in PHPUnit 9.6, not in 10 to 12.

@staabm
staabm requested a review from SanderMuller October 5, 2026 14:48
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.

make AssertEmptyIsDiscouragedRule autofixable

3 participants