From 7d8c62993ff53857b97ce500412c6bb407cacb3f Mon Sep 17 00:00:00 2001 From: Markus Staab Date: Tue, 10 Dec 2024 11:16:58 +0100 Subject: [PATCH 1/4] Implement AssertSameOverAssertEqualsRule --- README.md | 1 + rules.neon | 1 + .../PHPUnit/AssertEqualsIsDiscouragedRule.php | 64 +++++++++++++++++++ .../AssertEqualsIsDiscouragedRuleTest.php | 38 +++++++++++ .../data/assert-equals-is-discouraged.php | 37 +++++++++++ 5 files changed, 141 insertions(+) create mode 100644 src/Rules/PHPUnit/AssertEqualsIsDiscouragedRule.php create mode 100644 tests/Rules/PHPUnit/AssertEqualsIsDiscouragedRuleTest.php create mode 100644 tests/Rules/PHPUnit/data/assert-equals-is-discouraged.php diff --git a/README.md b/README.md index 205cbe4..526085b 100644 --- a/README.md +++ b/README.md @@ -22,6 +22,7 @@ It also contains this strict framework-specific rules (can be enabled separately * Check that you are not using `assertSame()` with `false` as expected value. `assertFalse()` should be used instead. * Check that you are not using `assertSame()` with `null` as expected value. `assertNull()` should be used instead. * Check that you are not using `assertSame()` with `count($variable)` as second parameter. `assertCount($variable)` should be used instead. +* Check that you are not using `assertEquals()` with same types (`assertSame()` should be used) ## How to document mock objects in phpDocs? diff --git a/rules.neon b/rules.neon index 023e11c..85afa76 100644 --- a/rules.neon +++ b/rules.neon @@ -2,6 +2,7 @@ rules: - PHPStan\Rules\PHPUnit\AssertSameBooleanExpectedRule - PHPStan\Rules\PHPUnit\AssertSameNullExpectedRule - PHPStan\Rules\PHPUnit\AssertSameWithCountRule + - PHPStan\Rules\PHPUnit\AssertEqualsIsDiscouragedRule - PHPStan\Rules\PHPUnit\ClassCoversExistsRule - PHPStan\Rules\PHPUnit\ClassMethodCoversExistsRule - PHPStan\Rules\PHPUnit\MockMethodCallRule diff --git a/src/Rules/PHPUnit/AssertEqualsIsDiscouragedRule.php b/src/Rules/PHPUnit/AssertEqualsIsDiscouragedRule.php new file mode 100644 index 0000000..3c83671 --- /dev/null +++ b/src/Rules/PHPUnit/AssertEqualsIsDiscouragedRule.php @@ -0,0 +1,64 @@ + + */ +class AssertEqualsIsDiscouragedRule implements Rule +{ + + public function getNodeType(): string + { + return NodeAbstract::class; + } + + public function processNode(Node $node, Scope $scope): array + { + if (!AssertRuleHelper::isMethodOrStaticCallOnAssert($node, $scope)) { + return []; + } + + if (count($node->getArgs()) < 2) { + return []; + } + if (!$node->name instanceof Node\Identifier || strtolower($node->name->name) !== 'assertequals') { + return []; + } + + $leftType = TypeCombinator::removeNull($scope->getType($node->getArgs()[0]->value)); + $rightType = TypeCombinator::removeNull($scope->getType($node->getArgs()[1]->value)); + + if ($leftType->isConstantScalarValue()->yes()) { + $leftType = $leftType->generalize(GeneralizePrecision::lessSpecific()); + } + if ($rightType->isConstantScalarValue()->yes()) { + $rightType = $rightType->generalize(GeneralizePrecision::lessSpecific()); + } + + if ( + ($leftType->isScalar()->yes() && $rightType->isScalar()->yes()) + && ($leftType->isSuperTypeOf($rightType)->yes()) + && ($rightType->isSuperTypeOf($leftType)->yes()) + ) { + return [ + RuleErrorBuilder::message( + 'You should use assertSame() instead of assertEquals(), because both values are scalars of the same type', + )->identifier('phpunit.assertEquals')->build(), + ]; + } + + return []; + } + +} diff --git a/tests/Rules/PHPUnit/AssertEqualsIsDiscouragedRuleTest.php b/tests/Rules/PHPUnit/AssertEqualsIsDiscouragedRuleTest.php new file mode 100644 index 0000000..e739ee4 --- /dev/null +++ b/tests/Rules/PHPUnit/AssertEqualsIsDiscouragedRuleTest.php @@ -0,0 +1,38 @@ + + */ +final class AssertEqualsIsDiscouragedRuleTest extends RuleTestCase +{ + + private const ERROR_MESSAGE = 'You should use assertSame() instead of assertEquals(), because both values are scalars of the same type'; + + public function testRule(): void + { + $this->analyse([__DIR__ . '/data/assert-equals-is-discouraged.php'], [ + [self::ERROR_MESSAGE, 19], + [self::ERROR_MESSAGE, 22], + [self::ERROR_MESSAGE, 23], + [self::ERROR_MESSAGE, 24], + [self::ERROR_MESSAGE, 25], + [self::ERROR_MESSAGE, 26], + [self::ERROR_MESSAGE, 27], + [self::ERROR_MESSAGE, 28], + [self::ERROR_MESSAGE, 29], + [self::ERROR_MESSAGE, 30], + [self::ERROR_MESSAGE, 32], + ]); + } + + protected function getRule(): Rule + { + return new AssertEqualsIsDiscouragedRule(); + } + +} diff --git a/tests/Rules/PHPUnit/data/assert-equals-is-discouraged.php b/tests/Rules/PHPUnit/data/assert-equals-is-discouraged.php new file mode 100644 index 0000000..727408d --- /dev/null +++ b/tests/Rules/PHPUnit/data/assert-equals-is-discouraged.php @@ -0,0 +1,37 @@ +assertSame(5, $integer); + static::assertSame(5, $integer); + + $this->assertEquals('', $string); + $this->assertEquals(null, $string); + static::assertEquals(null, $string); + static::assertEquals($nullableString, $string); + $this->assertEquals(2, $integer); + $this->assertEquals(2.2, $float); + static::assertEquals((int) '2', (int) $string); + $this->assertEquals(true, $boolean); + $this->assertEquals($string, $string); + $this->assertEquals($integer, $integer); + $this->assertEquals($boolean, $boolean); + $this->assertEquals($float, $float); + $this->assertEquals($null, $null); + $this->assertEquals((string) new Exception(), (string) new Exception()); + $this->assertEquals([], []); + $this->assertEquals(new Exception(), new Exception()); + static::assertEquals(new Exception(), new Exception()); + } +} From f91d89ef67614e32e8e0527a2380e66fdbabb39c Mon Sep 17 00:00:00 2001 From: Markus Staab Date: Thu, 12 Dec 2024 21:48:38 +0100 Subject: [PATCH 2/4] Error only when strict-rules are installed in tandem --- composer.json | 2 +- rules.neon | 8 +++++++- src/Rules/PHPUnit/AssertEqualsIsDiscouragedRule.php | 13 +++++++++++++ .../PHPUnit/AssertEqualsIsDiscouragedRuleTest.php | 2 +- 4 files changed, 22 insertions(+), 3 deletions(-) diff --git a/composer.json b/composer.json index 5237f06..3453041 100644 --- a/composer.json +++ b/composer.json @@ -7,7 +7,7 @@ ], "require": { "php": "^7.4 || ^8.0", - "phpstan/phpstan": "^2.0" + "phpstan/phpstan": "^2.0.4" }, "conflict": { "phpunit/phpunit": "<7.0" diff --git a/rules.neon b/rules.neon index 85afa76..5da7b84 100644 --- a/rules.neon +++ b/rules.neon @@ -2,7 +2,6 @@ rules: - PHPStan\Rules\PHPUnit\AssertSameBooleanExpectedRule - PHPStan\Rules\PHPUnit\AssertSameNullExpectedRule - PHPStan\Rules\PHPUnit\AssertSameWithCountRule - - PHPStan\Rules\PHPUnit\AssertEqualsIsDiscouragedRule - PHPStan\Rules\PHPUnit\ClassCoversExistsRule - PHPStan\Rules\PHPUnit\ClassMethodCoversExistsRule - PHPStan\Rules\PHPUnit\MockMethodCallRule @@ -18,3 +17,10 @@ services: deprecationRulesInstalled: %deprecationRulesInstalled% tags: - phpstan.rules.rule + + - + class: PHPStan\Rules\PHPUnit\AssertEqualsIsDiscouragedRule + arguments: + strictRulesInstalled: %strictRulesInstalled% + tags: + - phpstan.rules.rule diff --git a/src/Rules/PHPUnit/AssertEqualsIsDiscouragedRule.php b/src/Rules/PHPUnit/AssertEqualsIsDiscouragedRule.php index 3c83671..90bebb0 100644 --- a/src/Rules/PHPUnit/AssertEqualsIsDiscouragedRule.php +++ b/src/Rules/PHPUnit/AssertEqualsIsDiscouragedRule.php @@ -18,6 +18,15 @@ class AssertEqualsIsDiscouragedRule implements Rule { + private bool $strictRulesInstalled; + + public function __construct( + bool $strictRulesInstalled + ) + { + $this->strictRulesInstalled = $strictRulesInstalled; + } + public function getNodeType(): string { return NodeAbstract::class; @@ -25,6 +34,10 @@ public function getNodeType(): string public function processNode(Node $node, Scope $scope): array { + if (!$this->strictRulesInstalled) { + return []; + } + if (!AssertRuleHelper::isMethodOrStaticCallOnAssert($node, $scope)) { return []; } diff --git a/tests/Rules/PHPUnit/AssertEqualsIsDiscouragedRuleTest.php b/tests/Rules/PHPUnit/AssertEqualsIsDiscouragedRuleTest.php index e739ee4..9e96af4 100644 --- a/tests/Rules/PHPUnit/AssertEqualsIsDiscouragedRuleTest.php +++ b/tests/Rules/PHPUnit/AssertEqualsIsDiscouragedRuleTest.php @@ -32,7 +32,7 @@ public function testRule(): void protected function getRule(): Rule { - return new AssertEqualsIsDiscouragedRule(); + return new AssertEqualsIsDiscouragedRule(true); } } From 9f1c63a5b5fb4d4370c13fb42c91c5a6580fa490 Mon Sep 17 00:00:00 2001 From: Markus Staab Date: Fri, 13 Dec 2024 11:35:20 +0100 Subject: [PATCH 3/4] use conditionalTags --- rules.neon | 8 ++++---- src/Rules/PHPUnit/AssertEqualsIsDiscouragedRule.php | 13 ------------- .../PHPUnit/AssertEqualsIsDiscouragedRuleTest.php | 2 +- 3 files changed, 5 insertions(+), 18 deletions(-) diff --git a/rules.neon b/rules.neon index 5da7b84..84b7149 100644 --- a/rules.neon +++ b/rules.neon @@ -9,6 +9,10 @@ rules: - PHPStan\Rules\PHPUnit\NoMissingSpaceInMethodAnnotationRule - PHPStan\Rules\PHPUnit\ShouldCallParentMethodsRule +conditionalTags: + PHPStan\Rules\PHPUnit\AssertEqualsIsDiscouragedRule: + phpstan.rules.rule: [%strictRulesInstalled%, %featureToggles.bleedingEdge%] + services: - class: PHPStan\Rules\PHPUnit\DataProviderDeclarationRule @@ -20,7 +24,3 @@ services: - class: PHPStan\Rules\PHPUnit\AssertEqualsIsDiscouragedRule - arguments: - strictRulesInstalled: %strictRulesInstalled% - tags: - - phpstan.rules.rule diff --git a/src/Rules/PHPUnit/AssertEqualsIsDiscouragedRule.php b/src/Rules/PHPUnit/AssertEqualsIsDiscouragedRule.php index 90bebb0..3c83671 100644 --- a/src/Rules/PHPUnit/AssertEqualsIsDiscouragedRule.php +++ b/src/Rules/PHPUnit/AssertEqualsIsDiscouragedRule.php @@ -18,15 +18,6 @@ class AssertEqualsIsDiscouragedRule implements Rule { - private bool $strictRulesInstalled; - - public function __construct( - bool $strictRulesInstalled - ) - { - $this->strictRulesInstalled = $strictRulesInstalled; - } - public function getNodeType(): string { return NodeAbstract::class; @@ -34,10 +25,6 @@ public function getNodeType(): string public function processNode(Node $node, Scope $scope): array { - if (!$this->strictRulesInstalled) { - return []; - } - if (!AssertRuleHelper::isMethodOrStaticCallOnAssert($node, $scope)) { return []; } diff --git a/tests/Rules/PHPUnit/AssertEqualsIsDiscouragedRuleTest.php b/tests/Rules/PHPUnit/AssertEqualsIsDiscouragedRuleTest.php index 9e96af4..e739ee4 100644 --- a/tests/Rules/PHPUnit/AssertEqualsIsDiscouragedRuleTest.php +++ b/tests/Rules/PHPUnit/AssertEqualsIsDiscouragedRuleTest.php @@ -32,7 +32,7 @@ public function testRule(): void protected function getRule(): Rule { - return new AssertEqualsIsDiscouragedRule(true); + return new AssertEqualsIsDiscouragedRule(); } } From 828d05130c8d87137c0050b155f2af976316e454 Mon Sep 17 00:00:00 2001 From: Markus Staab Date: Fri, 13 Dec 2024 11:47:45 +0100 Subject: [PATCH 4/4] use more specific node type --- src/Rules/PHPUnit/AssertEqualsIsDiscouragedRule.php | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/Rules/PHPUnit/AssertEqualsIsDiscouragedRule.php b/src/Rules/PHPUnit/AssertEqualsIsDiscouragedRule.php index 3c83671..7aea39a 100644 --- a/src/Rules/PHPUnit/AssertEqualsIsDiscouragedRule.php +++ b/src/Rules/PHPUnit/AssertEqualsIsDiscouragedRule.php @@ -3,7 +3,7 @@ namespace PHPStan\Rules\PHPUnit; use PhpParser\Node; -use PhpParser\NodeAbstract; +use PhpParser\Node\Expr\CallLike; use PHPStan\Analyser\Scope; use PHPStan\Rules\Rule; use PHPStan\Rules\RuleErrorBuilder; @@ -13,14 +13,14 @@ use function strtolower; /** - * @implements Rule + * @implements Rule */ class AssertEqualsIsDiscouragedRule implements Rule { public function getNodeType(): string { - return NodeAbstract::class; + return CallLike::class; } public function processNode(Node $node, Scope $scope): array