Skip to content

[CodeQuality] Skip loose comparison on assertNotEquals as well on AssertEqualsToSameRector #436

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.

Sign up for GitHub

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

Merged
samsonasik merged 2 commits into from
Dec 26, 2024
Merged

[CodeQuality] Skip loose comparison on assertNotEquals as well on AssertEqualsToSameRector #436

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ed386c4
[CodeQuality] Skip loose comparison on assertNotEquals as well on Ass…
samsonasik Dec 26, 2024
8677f07
[CodeQuality] Skip loose comparison on assertNotEquals as well on Ass…
samsonasik Dec 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions .../MethodCall/AssertEqualsToSameRector/Fixture/skip_loose_compare_assert_not_equals.php.inc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
<?php

namespace Rector\PHPUnit\Tests\CodeQuality\Rector\MethodCall\AssertEqualsToSameRector\Fixture;

use PHPUnit\Framework\TestCase;

final class SkipLooseCompareAssertNotEquals extends TestCase
{
public function test(float $float)
{
$this->assertNotEquals(1, $float);
}
}
Copy link
Member

@TomasVotruba TomasVotruba Dec 26, 2024

Choose a reason for hiding this comment

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

This should be covered a separate rule that would update both name and type.

-$this->assertNotEquals(1, $float);
+$this->assertNotSame(1.0, $float);

Copy link
Member Author

@samsonasik samsonasik Dec 26, 2024

Choose a reason for hiding this comment

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

loose comparison needs to be verified manually, eg int vs float, it can be true while on assertSame it be false, on opposite, it happen as well, make always not same, which actually can be equal

Copy link
Member Author

@samsonasik samsonasik Dec 26, 2024

Choose a reason for hiding this comment

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

Oh, I see what you mean, that's value need to be changed, however, that's need to be checked as constant value, otherwise, cast (float)/(int) may be needed

Copy link
Member

@TomasVotruba TomasVotruba Dec 26, 2024

Choose a reason for hiding this comment

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

Yes, it should be applied on constant int/float values only. I always handle this change manually, often ~100s of cases and it's such a pain :D

59 changes: 33 additions & 26 deletions rules/CodeQuality/Rector/MethodCall/AssertEqualsToSameRector.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
namespace Rector\PHPUnit\CodeQuality\Rector\MethodCall;

use PhpParser\Node;
use PhpParser\Node\Arg;
use PhpParser\Node\Expr;
use PhpParser\Node\Expr\ClassConstFetch;
use PhpParser\Node\Expr\MethodCall;
Expand Down Expand Up @@ -103,39 +104,45 @@ public function refactor(Node $node): ?Node
return null;
}

if ($this->isName($node->name, 'assertEquals')) {
$firstArgType = $this->nodeTypeResolver->getNativeType($args[0]->value);
$secondArgType = TypeCombinator::removeNull($this->nodeTypeResolver->getNativeType($args[1]->value));
if ($this->shouldSkipLooseComparison($args)) {
return null;
}

// loose comparison
if ($firstArgType instanceof IntegerType && ($secondArgType instanceof FloatType || $secondArgType instanceof StringType)) {
return null;
}
$hasChanged = $this->identifierManipulator->renameNodeWithMap($node, self::RENAME_METHODS_MAP);
return $hasChanged ? $node : null;
}

if ($firstArgType instanceof FloatType && ($secondArgType instanceof IntegerType || $secondArgType instanceof StringType)) {
return null;
}
/**
* @param Arg[] $args
*/
private function shouldSkipLooseComparison(array $args): bool
{
$firstArgType = $this->nodeTypeResolver->getNativeType($args[0]->value);
$secondArgType = TypeCombinator::removeNull($this->nodeTypeResolver->getNativeType($args[1]->value));

if ($firstArgType instanceof StringType && $secondArgType instanceof ObjectType && $this->isObjectType(
$args[1]->value,
new ObjectType('Stringable')
)) {
return null;
}
// loose comparison
if ($firstArgType instanceof IntegerType && ($secondArgType instanceof FloatType || $secondArgType instanceof StringType)) {
return true;
}

// compare to mixed type is can be anything
if ($secondArgType instanceof MixedType) {
return null;
}
if ($firstArgType instanceof FloatType && ($secondArgType instanceof IntegerType || $secondArgType instanceof StringType)) {
return true;
}

// can happen with magic process
if ($secondArgType instanceof NeverType) {
return null;
}
if ($firstArgType instanceof StringType && $secondArgType instanceof ObjectType && $this->isObjectType(
$args[1]->value,
new ObjectType('Stringable')
)) {
return true;
}

$hasChanged = $this->identifierManipulator->renameNodeWithMap($node, self::RENAME_METHODS_MAP);
return $hasChanged ? $node : null;
// compare to mixed type is can be anything
if ($secondArgType instanceof MixedType) {
return true;
}

// can happen with magic process
return $secondArgType instanceof NeverType;
}

private function shouldSkipConstantArrayType(Expr $expr): bool
Expand Down
Loading