rectorphp / rector

Instant Upgrades and Automated Refactoring of any PHP 5.3+ code
https://getrector.com
MIT License
8.72k stars 687 forks source link

Incorrect behavior of RenameClassRector #7417

Closed grandmaster44 closed 1 year ago

grandmaster44 commented 2 years ago

Bug Report

Subject Details
Rector version last dev-main
Installed as composer dependency

Minimal PHP Code Causing Issue

See https://getrector.org/demo/224e1a17-d4f4-4d71-bafe-f1d4cf129846

<?php

final class DemoFile extends Demo
{
    /**
     * @var int
     */
    private $test = 5;

    public function run()
    {
        $demo = new Demo();
        return 5;

        // we never get here
        return 10;
    }
}

final class Demo {
}

final class Demo2 {
}

Responsible rules

<?php

use Rector\Config\RectorConfig;
use Rector\Php74\Rector\Property\TypedPropertyRector;
use Rector\Set\ValueObject\SetList;
use Rector\Renaming\Rector\Name\RenameClassRector;
use Rector\DeadCode\Rector\Stmt\RemoveUnreachableStatementRector;

return static function (RectorConfig $rectorConfig): void {
    $rectorConfig->skip([
        TypedPropertyRector::class => ['/*'], 
        RenameClassRector::class => ['/*']
    ]);

    $rectorConfig->rule(TypedPropertyRector::class);
    $rectorConfig->rule(RemoveUnreachableStatementRector::class);
    $rectorConfig->ruleWithConfiguration(RenameClassRector::class, [
        Demo::class => Demo2::class,
    ]);
};

Expected Behavior

Rector should skip RenameClassRector rule (same as TypedPropertyRector was skipped)

samsonasik commented 2 years ago

The change is expected, the extends Demo not changed because it is final class, if the class is not final, it will change extends and new instantion, ref :

https://getrector.org/demo/8402e93c-63be-44f0-a7bd-509f904a9061

if you want to skip the RenameClassRector, you can do skip it with provide correct path:

    $rectorConfig->skip([
        RenameClassRector::class => __DIR__ . '/app/Entity'
    ]);
grandmaster44 commented 2 years ago

Thanks for the reply.

The problem is that I want to skip RenameClassRector rule, but event I provide a full path to the file, it isn't skipped:

https://getrector.org/demo/94c60b5b-3228-4719-8128-9cec1cf8e7dd

samsonasik commented 2 years ago

RenameClassRector seems use post rector, which skip by path may overlapped, if you don't need, you can skip it entirely without path:

$rectorConfig->skip([ RenameClassRector::class,

//...

]);

Pada tanggal Kam, 25 Agu 2022 15:22, grandmaster44 @.***> menulis:

Thanks for the reply.

The problem is that I want to skip RenameClassRector rule, but event I provide a full path to the file, it isn't skipped:

https://getrector.org/demo/94c60b5b-3228-4719-8128-9cec1cf8e7dd

— Reply to this email directly, view it on GitHub https://github.com/rectorphp/rector/issues/7417#issuecomment-1226939401, or unsubscribe https://github.com/notifications/unsubscribe-auth/AADQHAEDKMQIJ252XRSVGT3V24UMXANCNFSM57R7OKHQ . You are receiving this because you were assigned.Message ID: @.***>

samsonasik commented 2 years ago

see https://getrector.org/demo/4b5954ac-1563-456c-9ad0-f96913f0b784

Pada tanggal Kam, 25 Agu 2022 15:36, abdul malik ikhsan < @.***> menulis:

RenameClassRector seems use post rector, which skip by path may overlapped, if you don't need, you can skip it entirely without path:

$rectorConfig->skip([ RenameClassRector::class,

//...

]);

Pada tanggal Kam, 25 Agu 2022 15:22, grandmaster44 < @.***> menulis:

Thanks for the reply.

The problem is that I want to skip RenameClassRector rule, but event I provide a full path to the file, it isn't skipped:

https://getrector.org/demo/94c60b5b-3228-4719-8128-9cec1cf8e7dd

— Reply to this email directly, view it on GitHub https://github.com/rectorphp/rector/issues/7417#issuecomment-1226939401, or unsubscribe https://github.com/notifications/unsubscribe-auth/AADQHAEDKMQIJ252XRSVGT3V24UMXANCNFSM57R7OKHQ . You are receiving this because you were assigned.Message ID: @.***>

grandmaster44 commented 2 years ago

Skipping the RenameClassRector rule is not a solution.

I want to rename class in every file except the single one. How can I achieve this ?

TomasVotruba commented 2 years ago

See README: https://github.com/rectorphp/rector/blob/main/docs/how_to_ignore_rule_or_paths.md

samsonasik commented 2 years ago

It seems not possible, as now, as it applied in post rector as well, see ClassRenamingPostRector

https://github.com/rectorphp/rector/blob/dbe8618a4c26d574ff4b457a6e853514afe90bc5/packages/PostRector/Rector/ClassRenamingPostRector.php#L35-L39

that's executed if $this->renamedClassesDataCollector->getOldToNewClasses() not empty.

The question is why you're doing that, because changing only in specific file for RenameClassRector seems not right, because when class used, it will point different object.

grandmaster44 commented 2 years ago

I discovered that I can skip the rule by combination of two rules (RenameClassRector and ClassRenamingPostRector)

https://getrector.org/demo/acfd37ce-819f-4248-b13c-e229c438cfa7

<?php

use Rector\Config\RectorConfig;
use Rector\Php74\Rector\Property\TypedPropertyRector;
use Rector\Set\ValueObject\SetList;
use Rector\Renaming\Rector\Name\RenameClassRector;
use Rector\DeadCode\Rector\Stmt\RemoveUnreachableStatementRector;
use Rector\PostRector\Rector\ClassRenamingPostRector;

return static function (RectorConfig $rectorConfig): void {
    $rectorConfig->skip([
        TypedPropertyRector::class => ['/rector_analyzed_file.php'],
        RenameClassRector::class => ['/rector_analyzed_file.php'],
        ClassRenamingPostRector::class => ['/rector_analyzed_file.php'],
    ]);

    $rectorConfig->rule(TypedPropertyRector::class);
    $rectorConfig->rule(RemoveUnreachableStatementRector::class);
    $rectorConfig->ruleWithConfiguration(RenameClassRector::class, [
        Demo::class => Demo2::class,
    ]);
};

The reason for skipping a single file from RenameClassRector rule may be that I want to force the usage of NewMailer in new features and thus allow OldMailer to be used in old ones.

Old Feature (skip class rename):

//@deprecated
final class DeprecatedVendorMailer{}

//@deprecated
final class OldSendMailService {
    public function __construct(DeprecatedVendorMailer $mailer) {}
}

New Feature (check that NewMailer has been used - if not - rename class - from DeprecatedVendorMailer to NewVendorMailer):

final class NewVendorMailer{}

final class NewSendMailService {
    public function __construct(NewVendorMailer $mailer) {}
}
samsonasik commented 2 years ago

@grandmaster44 awesome 👍 , yes, it seems skip both RenameClassRector and ClassRenamingPostRector seems the way to go as they share the $this->renamedClassesDataCollector->getOldToNewClasses() data

samsonasik commented 1 year ago

Re-open, I will create PR to properly fix it.