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

[DeadCode] Skip using coealesce assign operator on return on RemoveUnusedPrivatePropertyRector #2476

Merged
merged 27 commits into from
Jun 11, 2022

Conversation

samsonasik
Copy link
Member

The following code should be skipped:

final class SkipUsingCoalesceAssignOperator
{
    private SomeService $someService;

    private array $types = [];

    public function __construct(SomeService $someService)
    {
        $this->someService = $someService;
    }

    public function get(string $key)
    {
        return $this->types[$key] ??= $this->someService->resolve();
    }
}

@samsonasik
Copy link
Member Author

Using coealesce assign operator can be used to cache value to no need refill when already filled.

@samsonasik
Copy link
Member Author

Fixed 🎉

@samsonasik
Copy link
Member Author

The next check possibly need check because AssignOp is part of self::MODIFYING_NODE_TYPES

        if ($parent !== null && $this->typeChecker->isInstanceOf($parent, self::MODIFYING_NODE_TYPES)) {
            return true;
        }

@samsonasik
Copy link
Member Author

it seems can be handled by check current stmt is Expression

@samsonasik samsonasik marked this pull request as draft June 11, 2022 02:52
@samsonasik
Copy link
Member Author

that seems cause invalid result:

+    /**
+     * @var int
+     */
+    private const I = 0;
     public function __construct(
         private readonly TypeFactory $typeFactory,
@@ @@
         $unionedTypes = [];
         foreach ($collectedNestedArrayTypes as $collectedNestedArrayType) {
             $arrayType = $collectedNestedArrayType->getType();
-            for ($i = 0; $i < $collectedNestedArrayType->getArrayNestingLevel(); ++$i) {
+            for (; self::I < $collectedNestedArrayType->getArrayNestingLevel(); ++self::I) {
                 $arrayType = new ArrayType($collectedNestedArrayType->getKeyType(), $arrayType);
             }
    ----------- end diff -----------
Applied rules:
 * ChangeReadOnlyVariableWithDefaultValueToConstantRector
 [ERROR] Could not process "rules/Arguments/ArgumentDefaultValueReplacer.php"   
         file, due to:                                                          
         "System error: "Syntax error, unexpected ')', expecting '[' or         
         T_OBJECT_OPERATOR or T_NULLSAFE_OBJECT_OPERATOR or '{':184"            
         Run Rector with "--debug" option and post the report here:             
        [ https://github.com/rectorphp/rector/i](https://github.com/rectorphp/rector/issues/new)
ssues/new". On line: 54

@samsonasik
Copy link
Member Author

Let's locate AssignOp in current stmt

@samsonasik samsonasik marked this pull request as ready for review June 11, 2022 03:05
@samsonasik
Copy link
Member Author

it seems cause error in other rule:

There was 1 failure:
1) Rector\Tests\Php81\Rector\Property\ReadOnlyPropertyRector\ReadOnlyPropertyRectorTest::test with data set #6 (Symplify\SmartFileSystem\SmartFileInfo Object (...))
rules-tests/Php81/Rector/Property/ReadOnlyPropertyRector/Fixture/skip_array_dim_fetch_from_array_object_in_assign_var.php.inc
Failed asserting that string matches format description.
--- Expected
+++ Actual
@@ @@
 final class SkipArrayDimFetchFromArrayObjectInAssignVar
 {
-    private ArrayObject $array;
+    private readonly ArrayObject $array;

@samsonasik
Copy link
Member Author

Fixed by verify class constains local property fetch fd3542a , set flag to removed on removal property fetch

@samsonasik
Copy link
Member Author

it seems cause another error:

There was 1 failure:
1) Rector\Core\Tests\Issues\SplitMultiAssignRemovePrivate\SplitMultiAssignRemovePrivateTest::test with data set #1 (Symplify\SmartFileSystem\SmartFileInfo Object (...))
tests/Issues/SplitMultiAssignRemovePrivate/Fixture/multi_assign_extract.php.inc
Failed asserting that string matches format description.
--- Expected
+++ Actual
@@ @@
 final class MultiAssignExtract
 {
-    private $content_type;
-
     public function run()

@samsonasik samsonasik force-pushed the skip-using-coealesce-assign-operator branch from 3f29daf to 20ffa56 Compare June 11, 2022 05:33
@samsonasik samsonasik marked this pull request as draft June 11, 2022 05:36
@samsonasik
Copy link
Member Author

fixed with check total property fetch that removed goes to 0 before removing property 1e50564

@samsonasik samsonasik marked this pull request as ready for review June 11, 2022 06:43
@samsonasik
Copy link
Member Author

it seems it cause error in other rules:

There were 2 failures:
1) Rector\Tests\Naming\Rector\Class_\RenamePropertyToMatchTypeRector\RenamePropertyToMatchTypeRectorTest::test with data set #18 (Symplify\SmartFileSystem\SmartFileInfo Object (...))
rules-tests/Naming/Rector/Class_/RenamePropertyToMatchTypeRector/Fixture/skip_used_by_trait.php.inc
Failed asserting that string matches format description.
--- Expected
+++ Actual
@@ @@
     /**
      * @var EliteManager
      */
-    private $eventManager;
+    private $eliteManager;
     public function __construct(EliteManager $eventManager)
     {
-        $this->eventManager = $eventManager;
+        $this->eliteManager = $eventManager;
     }
 }
/home/runner/work/rector-src/rector-src/packages/Testing/PHPUnit/AbstractRectorTestCase.php:154
/home/runner/work/rector-src/rector-src/packages/Testing/PHPUnit/AbstractRectorTestCase.php:115
/home/runner/work/rector-src/rector-src/rules-tests/Naming/Rector/Class_/RenamePropertyToMatchTypeRector/RenamePropertyToMatchTypeRectorTest.php:18
2) Rector\Tests\Php74\Rector\Property\TypedPropertyRector\TypedPropertyRectorTest::test with data set #52 (Symplify\SmartFileSystem\SmartFileInfo Object (...))
rules-tests/Php74/Rector/Property/TypedPropertyRector/Fixture/static_property_filled_by_trait.php.inc
Failed asserting that string matches format description.
--- Expected
+++ Actual
@@ @@
 {
     use FillerTrait;
-    /**
-     * @var AnotherClass
-     */
-    private static $someStaticProperty;
+    private static \Rector\Tests\Php74\Rector\Property\TypedPropertyRector\Source\AnotherClass $someStaticProperty;
 }

@samsonasik samsonasik force-pushed the skip-using-coealesce-assign-operator branch from ca1d35e to 5e3643b Compare June 11, 2022 07:31
@samsonasik
Copy link
Member Author

samsonasik commented Jun 11, 2022

it seems property fetch in trait can't get parent ClassLike, resolved by count as is 5e3643b

@samsonasik
Copy link
Member Author

Fixed 🎉 phpstan notice is unrelated.

Run vendor/bin/phpstan analyse --ansi --error-format symplify
Note: Using configuration file /home/runner/work/rector-src/rector-src/phpstan.neon.
Config file /home/runner/work/rector-src/rector-src/vendor/symplify/phpstan-rules/packages/symfony/config/services.neon does not exist or isn't readable
Error: Process completed with exit code 1.

@samsonasik samsonasik changed the title [DeadCode] Skip using coealesce assign operator on RemoveUnusedPrivatePropertyRector [DeadCode] Skip using coealesce assign operator on return on RemoveUnusedPrivatePropertyRector Jun 11, 2022
@samsonasik
Copy link
Member Author

re-trigger CI

@samsonasik samsonasik force-pushed the skip-using-coealesce-assign-operator branch from 3ea68c9 to 7637863 Compare June 11, 2022 11:57
@samsonasik
Copy link
Member Author

rebased.

@samsonasik
Copy link
Member Author

All checks have passed 🎉 @TomasVotruba I think it is ready.

@TomasVotruba
Copy link
Member

Thank you 👍

@TomasVotruba TomasVotruba merged commit 9679ed6 into main Jun 11, 2022
@TomasVotruba TomasVotruba deleted the skip-using-coealesce-assign-operator branch June 11, 2022 12:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants