-
Notifications
You must be signed in to change notification settings - Fork 48
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement AssertEqualsIsDiscouragedRule #216
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,77 @@ | ||
<?php declare(strict_types = 1); | ||
|
||
namespace PHPStan\Rules\PHPUnit; | ||
|
||
use PhpParser\Node; | ||
use PhpParser\NodeAbstract; | ||
use PHPStan\Analyser\Scope; | ||
use PHPStan\Rules\Rule; | ||
use PHPStan\Rules\RuleErrorBuilder; | ||
use PHPStan\Type\GeneralizePrecision; | ||
use PHPStan\Type\TypeCombinator; | ||
use function count; | ||
use function strtolower; | ||
|
||
/** | ||
* @implements Rule<NodeAbstract> | ||
*/ | ||
class AssertEqualsIsDiscouragedRule implements Rule | ||
{ | ||
|
||
private bool $strictRulesInstalled; | ||
|
||
public function __construct( | ||
bool $strictRulesInstalled | ||
) | ||
{ | ||
$this->strictRulesInstalled = $strictRulesInstalled; | ||
} | ||
|
||
public function getNodeType(): string | ||
{ | ||
return NodeAbstract::class; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I feel like they have a more specific class parent than just NodeAbstract. Feel free to check out and optimize other similar rules as well. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. fixed here and will do for more rules in a separate PR with a 1.4.x target in #217 |
||
} | ||
|
||
public function processNode(Node $node, Scope $scope): array | ||
{ | ||
if (!$this->strictRulesInstalled) { | ||
return []; | ||
} | ||
|
||
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 []; | ||
} | ||
|
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,38 @@ | ||
<?php declare(strict_types = 1); | ||
|
||
namespace PHPStan\Rules\PHPUnit; | ||
|
||
use PHPStan\Rules\Rule; | ||
use PHPStan\Testing\RuleTestCase; | ||
|
||
/** | ||
* @extends RuleTestCase<AssertEqualsIsDiscouragedRule> | ||
*/ | ||
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(true); | ||
} | ||
|
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,37 @@ | ||
<?php | ||
|
||
declare(strict_types=1); | ||
|
||
namespace SameOverEqualsTest; | ||
|
||
use Exception; | ||
use PHPUnit\Framework\TestCase; | ||
|
||
final class AssertSameOverAssertEqualsRule extends TestCase | ||
{ | ||
public function dummyTest(string $string, int $integer, bool $boolean, float $float, ?string $nullableString): void | ||
{ | ||
$null = null; | ||
|
||
$this->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()); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd rather use conditionalTags instead of passing the parameter in the rule.
Also: we can do it only on bleeding edge. Conditional tags can do
and
condition based on multiple parameters since phpstan/phpstan-src#1697 (see the tests).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TIL, thanks