phpmd / phpmd

PHPMD is a spin-off project of PHP Depend and aims to be a PHP equivalent of the well known Java tool PMD. PHPMD can be seen as an user friendly frontend application for the raw metrics stream measured by PHP Depend.
https://phpmd.org
BSD 3-Clause "New" or "Revised" License
2.34k stars 347 forks source link

PHP 8.1 Enums static methods support #1073

Open Mapteg34 opened 8 months ago

Mapteg34 commented 8 months ago

Current Behavior

Any of

Enum::from();
Enum::tryFrom();
Enum::cases();

trigger message Avoid using static access to class

Expected Behavior

No message, because this methods by design: https://www.php.net/manual/en/class.backedenum.php

Steps To Reproduce:

Try to use any method of https://www.php.net/manual/en/class.backedenum.php.


Please fix this behavior

kylekatarnls commented 8 months ago

I agree it's not ideal, our doc says:

The only case when static access is acceptable is when used for factory methods.

A.K.A. Named constructors, from and tryFrom are such cases where it's generally accepted as a relevant usage of static method access. But with the limited possibilities (handling legacy code that is not strongly typed) we are currently unable to accurately detect if a method is actually such an acceptable case where we should not raise.

So at the moment it's on user to manually suppress those accesses that are valid.

It's far from being straightforward but when those methods are correctly type with : self or : ?self we could actually detect it's a named constructor and then not raise it.

For Enum::cases() it would be yet another difficulty level, as it's still acceptable use IMO by the fact that it's an array of self but even with the strong typing, we only see array so no easy static way to whitelist the method.

AJenbo commented 8 months ago

But with the limited possibilities (handling legacy code that is not strongly typed) we are currently unable to accurately detect if a method is actually such an acceptable case where we should not raise.

Would such methods not piratically always be strongly typed unless the class name is gotten dynamically as a string?

For Enum::cases() it would be yet another difficulty level, as it's still acceptable use IMO by the fact that it's an array of self but even with the strong typing, we only see array so no easy static way to whitelist the method.

I have somehow forgotten, but are we not able to read types from PHPDoc (array<self>)?

kylekatarnls commented 8 months ago

Would such methods not piratically always be strongly typed unless the class name is gotten dynamically as a string?

Yes we can just consider public function from(): self and ignore /** @return self */ public function from() stuff.

I have somehow forgotten, but are we not able to read types from PHPDoc (array<self>)?

It's significantly less efficient to parse annotations (and the multiple forms: array<self>, list<self>, self[] etc.)

AJenbo commented 8 months ago

Arh i was thinking we could do it specifically for the enum class

kylekatarnls commented 8 months ago

We can, it's a quick-win, and the same way we can also include other PHP native static constructors such as DateTime::createFromFormat(), then it will be a list that we have to maintain and then we need to provide a way for users to declare their own methods to be whitelisted (I think the @SuppressWarning needs to be done for each call at the moment and cannot be made once on declaration, but I should check this assertion).