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] Do not remove parameter on RemoveUnusedPrivatePropertyRector on constructor only usage in the middle of parameter #1212

Conversation

samsonasik
Copy link
Member

Given the following code:

class SkipConstructorOnlyInMiddleParameterUsed
{
    /**
     * @var int
     */
    private $contentFinder;

    public function __construct(int $contentFinder, \stdClass $stdClass)
    {
        $this->contentFinder = $contentFinder;
        $this->init($stdClass);
    }

    private function init(\stdClass $stdClass)
    {
        var_dump($stdClass);
    }
}

It produce:

-    public function __construct(int $contentFinder, \stdClass $stdClass)
+    public function __construct(\stdClass $stdClass)
     {
-        $this->contentFinder = $contentFinder;
         $this->init($stdClass);

which can make the caller of the class error. It needs to be skipped.

@samsonasik samsonasik force-pushed the skip-remove-unused-private-property-in-middle-parameter-construct branch from 879913c to 71ae512 Compare November 11, 2021 11:31
@TomasVotruba
Copy link
Member

The constructor might be BC break in case of scalar values. But the removal of property assign and property seems legit.

@samsonasik
Copy link
Member Author

This is only effect if the removed parameter not in the last used parameter, as the caller which call:

(new SkipConstructorOnlyInMiddleParameterUsed(1, new stdClass));

will got error:

Uncaught TypeError: SkipConstructorOnlyInMiddleParameterUsed::__construct(): Argument #1 ($stdClass) must be of type stdClass, int given

ref https://3v4l.org/A6PvC

@samsonasik
Copy link
Member Author

Ok, it seems the removal should be only for :

  • property
  • assign

but keep the constructor parameter.

@samsonasik samsonasik changed the title [DeadCode] Skip RemoveUnusedPrivatePropertyRector on constructor only usage in the middle of parameter [DeadCode] Don't remove parameter on RemoveUnusedPrivatePropertyRector on constructor only usage in the middle of parameter Nov 11, 2021
@samsonasik
Copy link
Member Author

@TomasVotruba I updated failing fixture to only remove property and assign ,but keep the constructor parameter as it happen not in last parameter

1f72f8e

@samsonasik samsonasik changed the title [DeadCode] Don't remove parameter on RemoveUnusedPrivatePropertyRector on constructor only usage in the middle of parameter [DeadCode] Do not remove parameter on RemoveUnusedPrivatePropertyRector on constructor only usage in the middle of parameter Nov 11, 2021
@samsonasik
Copy link
Member Author

Fixed 🎉

@samsonasik samsonasik force-pushed the skip-remove-unused-private-property-in-middle-parameter-construct branch from 8ef0593 to 873a2ab Compare November 11, 2021 12:49
@samsonasik
Copy link
Member Author

All checks have passed 🎉 @TomasVotruba it is ready for review.

Comment on lines 32 to 6
} No newline at end of file
<?php

declare(strict_types=1);

namespace Rector\Tests\DowngradePhp70\Rector\FuncCall\DowngradeDirnameLevelsRector;

Copy link
Member

Choose a reason for hiding this comment

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

Btw, what has changed here? The file looks identical. EOLs?

Copy link
Member Author

Choose a reason for hiding this comment

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

that's seems spacing and eol

@TomasVotruba TomasVotruba merged commit 3478989 into main Nov 11, 2021
@TomasVotruba TomasVotruba deleted the skip-remove-unused-private-property-in-middle-parameter-construct branch November 11, 2021 14:13
@TomasVotruba
Copy link
Member

Thank you 👏

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