What is the rationale behind LocallyCalledStaticMethodToNonStaticRector to be considered as CodeQuality?

[simPod]

Hello.

LocallyCalledStaticMethodToNonStaticRector is in CodeQuality list.

I wonder what is the rationale behind it. Why is it considered to be of higher code quality?

Thanks

[TomasVotruba]

Hi,

similar rules exist there as well.
Where would you rather place it?

[simPod]

Hm, there's Removing Static group https://github.com/rectorphp/rector/blob/156239b3926a9b7f64d22d5dc2d5bfe3879cbe95/docs/rector_rules_overview.md#removingstatic so I'd have it only there and not in code quality.

[TomasVotruba]

There should be just one rule now. The "removing static" shall be gone as missleading :)

[TomasVotruba]

Ref rectorphp/rector-src#4242

[simPod]

Also there's similar rule in PHPUnit quality list PreferPHPUnitThisCallRector. I'm also not sure about any code quality++ benefit. Since neither is officially recommended by phpunit, I prefer to call phpunit's static methods statically, the same way as I call any other static method anywhere. Not sure there's a reason for having an exception for phpunit 🤔

[TomasVotruba]

In case of any non-fitting rule to your style, the best is to skip them - https://getrector.com/documentation/ignoring-rules-or-paths
Set changes won't happen as it's not possible to one size fits-all sets.

[simPod]

Sure but the question was not about style but the rationale behind it. How it improves the code quality.

[TomasVotruba]

It's rather logical to call similar methods the same way.

[giosh94mhz]

Hi @simPod and @TomasVotruba !

I was asking myself the same question while testing/integrating rector in my project.

The doubt arise because when enabling "codeQuality: true" I expect to enable all rule which are known to improve code quality, but by reading the thread it seems to me that the real meaning is "all rules under code quality category", which may gives some mixed results.

In my opinion, though, this rule is wrong by itself. Converting method will force PHP under the hood to pass in a $this reference that is never used. So, there shouldn't be a LocallyCalledStaticMethodToNonStaticRector but more likely a LocallyCalledNonStaticMethodWithoutThisToStaticRector. This could be universally accepted as improving code quality and speed (a little bit, but still).

[MacDada]

Well, it's just changed my perfectly good private static method into a non–static one.

That made me think: hmm, interesting, I wonder why? How does that improve my code? Why would a non-static call be preferred over a static call?

And actually… I still don't know :-]

So I'm disabling it, unless someone can open my mind to the benefits…

[calebdw]

static should only be used when you're in a parent and need to call an implementation on a child. It used late-static binding because the particular class is not known until runtime.

self should be used when you're only ever to call that method on the class that you're currently in. In your case, it is impossible to call a private method from a child class so self is correct.

Using self should be more performant and help static analysis more because it doesn't need to compute the correct class at runtime

[MacDada]

Sorry, maybe I wasn't clear enough… I know the difference between self::something and static::something :)

This is the change that Rector does if I don't skip LocallyCalledStaticMethodToNonStaticRector:

The tokensToHtml method does not need access to $this, it is simply a "helper" that could be kinda anywhere. How does the change from self::tokensToHtml() to $this->tokensToHtml() improve my code? Why is this kind of change in the codeQuality set? Does this change "improve quality" of my test code? How?

[calebdw]

It's a private method that is only being called from an instance method, no need for it to be static (you already have $this in the method and you can directly access the method on it). Otherwise PHP needs to go through a whole other code path to get the class, find the method, etc.

[MacDada]

no need for it to be static

no need to be object method as well ;-)

actually, I'd consider an object as being "more complicated" (as it is class+instance) than just class -> so conceptually class methods (static methods) should could be preferred over object methods (as a lesser, simpler concepts).

Otherwise PHP needs to go through a whole other code path to get the class, find the method, etc.

Ah, so $this is a kind of (micro?) optimization?