feat: Introduce StaticPrivateMethodFixer

[Slamdunk]

References:

1. https://twitter.com/BackEndTea/status/1174225152034385920 (/cc @BackEndTea)
2. Methods\StaticRule ergebnis/phpstan-rules#133

 class Foo
 {
     public function bar()
     {
-        return $this->baz();
+        return self::baz();
     }
     
-    private function baz()
+    private static function baz()
     {
         return 1;
     }
 }

• Must not contain $this references
• Must not contain non-static closures see feat: Introduce StaticPrivateMethodFixer #4557 (comment) updated to: Must not contain closures
• Must not contain debug_backtrace() calls
• Must be marked as risky, no dynamic reference handled such as $this->{$method}() or $$var inside the method
• Must run after ProtectedToPrivateFixer
• Must run before StaticLambdaFixer

[Ocramius]

Restrictions:

• Must not contain non-static closures
• Must not contain debug_backtrace() usage
• Must not contain $this
• Must not contain $$var
• must not be declared by interface

[Slamdunk]

Must not contain debug_backtrace() usage

Why? How could a code calling debug_backtrace() be affected by this change?
I mean affected runtime; the output would obviously be different, but I would not consider this as an issue.

Must not contain $$var

This could only affects $$var referencing to $this or global variables AFAICS, we'll point this out in the Risky signature but not handle them

[Ocramius]

Why? How could a code calling debug_backtrace() be affected by this change?

debbug_backtrace()[1]['object'] stops working

[dkarlovi]

Must not contain non-static closures
Must run before StaticLambdaFixer

Wouldn't running after static lambda relax the first requirement?

[BackEndTea]

Wouldn't running after static lambda relax the first requirement?

These two kinda have circular dependency, idk if php-cs-fixer can handle that atm.

[Slamdunk]

Must not contain non-static closures
Must run before StaticLambdaFixer

Wouldn't running after static lambda relax the first requirement?

Heh, that's tricky, it should run before to relax methods containing non-static closures, but also after to fix non-static closures using private methods now declared static.
As pointed out by @BackEndTea here's a circular dependency.

[Ocramius]

You can solve the dependency by running these recursively, which would require changing the tool architecture.

Your exit condition being "nothing got fixed", or "nesting level limit reached".

[dkarlovi]

I mean, it's the same effect as running it twice, I guess. It's a bit annoying if you have an unstable rule combo like this: you run CS and commit, your CI runs it again and fails because it changed something too.

[Slamdunk]

@keradus always pushed for avoiding such architectures for clear reasons. I think I'll propose the first production-ready version of the Fixer to completely ignore private method containing any closure

[Slamdunk]

must not be declared by interface

@Ocramius you killed me 😢 this practically makes the Fixer impossible to be implemented without context awareness

[dkarlovi]

[BackEndTea]

must not be declared by interface

Interfaces can't declare private methods right?

[Slamdunk]

Interfaces can't declare private methods right?

No, but the resulting new method can be present in the interface, and such creating a conflict

[BackEndTea]

I don't understand, could you provide an example

[Slamdunk]

I thought this https://3v4l.org/G6vS6 could be possible but turns out it can't, so either @Ocramius knows something we don't or he was wrong; and it would be really weird for him to be wrong 😛

[Ocramius]

Maybe annoying, but still a good amount of improvement 👍

[Ocramius]

@Slamdunk sorry, I missed the limitation to private methods in my thougt process 😛

[dkarlovi]

TBH I really assumed you actually could put private methods in interfaces, because... you know, PHP.

TIL you can't.

[BackEndTea]

I think the fixer can also be applied inside of Traits

[Slamdunk]

Mmm, I think not

// file Foo.php
trait Foo
{
    private function bar() {}
}
// file Baz.php
class Baz
{
    use Foo;
    
    public function wut()
    {
        return $this->bar();
    }
}

I cannot fix Foo::bar because I would break Baz::wut behavior; well, not entirely break as php still run without issue, but a static analyzer would complain

[Slamdunk]

I'm sorry I just read #4557 (comment): yes we can fix Traits too pointing the possible side effects into the risky description.

But honestly the two scenarios are much differently spread imho:

1. A class that uses a private method of a trait: ok, bad but common, we can't use this fixer for Trait as we would (logically) break the code
2. A trait that uses a private method of a class: WUT? Are you kidding me? Yeah it's feasible, but if you write crappy code it's not our fault

I feel safe to fix classes and not fix traits, what do you think?

[BackEndTea]

Seems like a good idea.

I don't think it will be a common problem, but it is one of those nasty edge cases.

[BackEndTea]

One more possible edge case that (may) not be relevant yet. How about $this etc in php 7.4 arrow functions

[Slamdunk]

One more possible edge case that (may) not be relevant yet. How about $this etc in php 7.4 arrow functions

I was planning to pinpoint it in an issue like #4382 as I haven't a clear idea on the topic as of yet, nor on the way this library intend to support the new code structure

[Slamdunk]

@SpacePossum may we tag this for 2.17?

[coveralls]

coverage: 94.895% (+0.01%) from 94.882%
when pulling df2bc41 on Slamdunk:static_private_method
into 395ce85 on PHP-CS-Fixer:master.