Skip to content
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

Move instanceof PHPStan Type to ->is*() take 1 #6416

Merged
merged 30 commits into from
Nov 12, 2024
Merged

Conversation

samsonasik
Copy link
Member

@samsonasik samsonasik commented Nov 12, 2024

Cherry-pick from #6415
Ref rectorphp/rector#8815 (comment)

samsonasik and others added 30 commits November 12, 2024 07:14
@samsonasik
Copy link
Member Author

All checks have passed 🎉 @TomasVotruba I am merging it ;)

@samsonasik samsonasik merged commit 3949fc2 into main Nov 12, 2024
36 checks passed
@samsonasik samsonasik deleted the move-instanceof-to-is branch November 12, 2024 00:18
@samsonasik
Copy link
Member Author

Tested on CodeIgniter 4 project, it somehow cause invalid change on non-native property:

➜  CodeIgniter4 git:(develop) ✗ vendor/bin/rector 
 956/956 [▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓] 100%
1 file with changes
===================

1) system/Database/Forge.php:572

    ---------- begin diff ----------
@@ @@
             }

             // Most databases don't support creating indexes from within the CREATE TABLE statement
-            if (! empty($this->keys)) {
+            if ($this->keys !== []) {
                 for ($i = 0, $sqls = $this->_processIndexes($table), $c = count($sqls); $i < $c; $i++) {
                     $this->db->query($sqls[$i]);
                 }
    ----------- end diff -----------

Applied rules:
 * SimplifyEmptyCheckOnEmptyArrayRector
 * ClassPropertyAssignToConstructorPromotionRector

which hsould be skipped, which ! empty() cover null check as well, I will look it first to avoid regression if possible, some is*() method seems cause some magic check on union.

@samsonasik
Copy link
Member Author

After some check, SimplifyEmptyCheckOnEmptyArrayRector check on default value as well, so seems ok, and there is fixture for it

final class NonTypedPropertyHasDefaultValueNeverAssigned
{
/** @var array */
public $property = [];
public function isEmpty(): bool
{
return empty($this->property);
}
}
?>
-----
<?php
namespace Rector\Tests\CodeQuality\Rector\Empty_\SimplifyEmptyCheckOnEmptyArrayRector\Fixture;
final class NonTypedPropertyHasDefaultValueNeverAssigned
{
/** @var array */
public $property = [];
public function isEmpty(): bool
{
return $this->property === [];
}
}

which property be null is rather rare, I also create a patch on CodeIgniter4 side based on it:

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.

2 participants