Unused interface methods not considered unused?

[RobertMe]

Hi,

This looks like a very promising project to clean up some legacy project. But it seems like it doesn't consider a method defined in an interface to be "unused" as long as that interface is implemented? In other words, if I drop the method from the interface it will consider the implementation to be an unused public method. But with the method declared in the interface it doesn't do so.

If this feature is considered it might need to be an option though? As an interface can also be used for some public API in which case the methods declared in the API should be considered the same as /** @api */ marked methods I assume.

[TomasVotruba]

Hi,

thanks for the idea. Could you share a code example of your what you mean?

[RobertMe]

Untested example:

interface TestInterface
{
  public function run(): void;
}

class Test implements TestInterface
{
  public function run(): void {}
}

It seems like this won't mark Test::run() / TestInterface::run() as unused. While when commenting the method in the interface it does mark Test::run() as unused.

So like this, it is marked as unused:

interface TestInterface
{
}

class Test implements TestInterface
{
  public function run(): void {}
}

I have been looking into the code of this extension and haven't found some obvious case where it would consider this to be a used method though. So it might be that it's something else leading up to this. So if based on your knowledge / experience this should mark the method as unused I'm gonna have to dig deeper into this.

So please consider this more like a (support) question at first, instead of a bug report / feature request :) In case it turns out to be a bug or missing feature I can try to get this implemented myself as well :)

[RobertMe]

I have been looking into the code of this extension and haven't found some obvious case where it would consider this to be a used method though.

Scratch that: https://github.com/TomasVotruba/unused-public/blob/c64cf32233d4a06f1a45499666b15d025184f709/src/PublicClassMethodMatcher.php#L45 this method does "ignore" all methods which implement or override a method.

So I will consider this to be a feature request to put this behind a config option, which I will try to implement myself :)

[RobertMe]

So after a bit of hacking:

1. Methods defined in interfaces are ignored
2. Methods called on class types are not considered to be called on the interface (while they are considered to be called on parent classes)

It is my believe that these two things can safely be changed without being "breaking" and IMO could be considered as bug fix. So it IMO shouldn't be considered for a configuration option to enable these checks. The only "condition" for this change would be that the @api doc should be handled properly on interfaces, if that's not the case already by virtue of the existing code.

So I'll make a PR for this today / in the upcoming days, including some tests. I already hacked the code in my projects vendor/ installation and with these two changes it works fine / as I would expect it to work.

[TomasVotruba]

I see, thanks for real example. I think we skip interface method on purpose, as it would create a BC break. This interface could be used elsewhere, in open-source project and should be changed with care and human-discovery.

Suggesting such methods as unused could be risky and cause more harm then value. That's why we focus on less risky and more obvious unused public methods first.

[RobertMe]

But doesn't that also depend on more things? An @internal interface shouldn't be used by others, while an @api interface always would be open. In case the interface isn't annotated with @internal / @api there might be a distinction between libraries which are used by others (doesn't necessarily have to be open source, and might also be libraries used between multiple projects in a corporate environment) and libraries (/code) which isn't used by others. I.e.: I believe that some projects state that only interfaces / classes / methods which are explicitly marked with @api to be usable by the outside world. While others state that "everything" is usable by the outside world, unless annotated with @internal.

So personally I think that the annotations must be taken into account (where currently only @api is considered already for methods which aren't in an interface, but not @internal). And there would be a need for a setting which determines what should happen in those cases where no such annotation is present. In which case the default for this setting will still be to not consider methods in interfaces to be unused, but then there still is the option to consider those to be unused (and thus removable).

[TomasVotruba]

Indeed, its rather complex issue. Goal of this package is to focus on the easy picks, so it can be relied on :)

In your case, I think its great place for a custom phpstan package a test it in the wild.