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

Incorrect behavior of RemoveUnusedPrivateMethodRector #6613

Closed
bertrandjamin opened this issue Aug 4, 2021 · 7 comments
Closed

Incorrect behavior of RemoveUnusedPrivateMethodRector #6613

bertrandjamin opened this issue Aug 4, 2021 · 7 comments
Labels

Comments

@bertrandjamin
Copy link

Bug Report

The rule RemoveUnusedPrivateMethodRector don't handle correctly abstract private functions in a trait.

ℹ️ This a new PHP8 capability : https://www.php.net/manual/en/language.oop5.traits.php#language.oop5.traits.abstract.

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

Minimal PHP Code Causing Issue

See https://getrector.org/demo/1ebf50d7-992f-63ba-9a8a-bd39c81b9232

<?php

trait MyTrait
{
    abstract private function privateMethod();
        
    public function publicMethod() {
        $this->privateMethod();
    }
}

class MyClass
{
    use MyTrait;

    private function privateMethod()
    {
        //code
    }

}

Responsible rules

  • RemoveUnusedPrivateMethodRector

Expected Behavior

As the abstract privateMethod is used in publicMethod the rule should not remove the privateMethod implementation in MyClass

Thanks for your help

@TomasVotruba
Copy link
Member

Thank you for your report and demo link!

Could you send a failing test case in a pull-request, so we have it covered in Rector?
Here is step by step tutorial how to add it: https://github.com/rectorphp/rector/blob/master/docs/how_to_add_test_for_rector_rule.md

Bl00D4NGEL pushed a commit to Bl00D4NGEL/rector-src that referenced this issue Aug 5, 2021
TomasVotruba pushed a commit to rectorphp/rector-src that referenced this issue Aug 6, 2021
* Add failing test fixture for ReturnTypeFromReturnNewRector

# Failing Test for ReturnTypeFromReturnNewRector

Based on https://getrector.org/demo/1ebef269-093c-6afe-9c29-afdc8959befb

refs rectorphp/rector#6595

* Add failing test fixture for RemoveUnusedPrivateMethodRector

# Failing Test for RemoveUnusedPrivateMethodRector

Based on https://getrector.org/demo/1ebf50d7-992f-63ba-9a8a-bd39c81b9232

refs: rectorphp/rector#6613

* fix test case skip_abstract_private_method_in_trait.php.inc

* remove file from previous PR

* fix fixture by moving namespace to top

* import TraitUse

* use AstResolver

* added test case to check if non-abstract private methods are considered

* refac: moved "is used" checks into it's own class to reduce class complexity

* rename constructor arg to represent class name
@Bl00D4NGEL
Copy link
Contributor

@bertrandjamin the issue is now fixed.

Given that the abstract private function is PHP 8.0+ we should probably consider writing a downgrade rector for that, since I didn't find any in the PHP 8.0 downgrades., right @TomasVotruba?

@bertrandjamin
Copy link
Author

bertrandjamin commented Aug 6, 2021

@Bl00D4NGEL thanks !
I planned to work on it in few days but you were faster than me :)

I let you or @TomasVotruba close the issue according to his answer.

@bertrandjamin
Copy link
Author

bertrandjamin commented Aug 6, 2021

@Bl00D4NGEL after reading your PR I have a question about https://github.com/Bl00D4NGEL/rector-src/blob/06b4aef9aa4607df84ea7f37fa641809ac0b503c/rules-tests/DeadCode/Rector/ClassMethod/RemoveUnusedPrivateMethodRector/Fixture/remove_if_method_non_abstract_private_in_trait.php.inc

Given the php documentation (https://www.php.net/manual/en/language.oop5.traits.php#language.oop5.traits.precedence) a method in the class can override the one created in the trait.
So it seems that this Fixture breaks functional code.

Am I missing something ?

@Bl00D4NGEL
Copy link
Contributor

@bertrandjamin you would be correct. I tested it in the PHPSandbox: http://sandbox.onlinephpfunctions.com/code/fa84c5aaa7ca8433d7b7e486a82bc678f21aaac7 and it seems like you actually can overwrite the private function, my bad there.
Is this worth a new issue or should this be fixed within this issue @TomasVotruba ?

@TomasVotruba
Copy link
Member

@Bl00D4NGEL I think you can fix it here, as it's closely related.

@samsonasik
Copy link
Member

It seems already fixed, closing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants