Phpinspectionsea: SenselessMethodDuplicationInspection does not recognize context and ignores import statements

Created on 9 May 2021 · 15Comments · Source: kalessil/phpinspectionsea

File A.php:

<?php

use Foo\Bar;

class A {

    public function create() {
        return new Bar();
    }
}

File AA.php:

<?php

use Bar\Bar;

class AA extends A {

    public function create() {
        return new Bar();
    }
}

Current behaviour

Method AA::create() is marked as redundant, since its body is the same as parent method. But but they're different - first creates Foo\Bar and second creates Bar\Bar instance (these two files have different imports and new Bar() have different meaning).

Expected behaviour

No warning.

Environment details

I have an impression that this had changed after Upgrade to PhpStorm 2021.1 (from 2020.3).

bug / false-positive

Source

rob006

All 15 comments

I tried to reproduce with a similar example based on yours and can't:

~~~

namespace A {
use \SimpleXMLElement as Bar;

class A {

    public function create() {
        return new Bar();
    }
}

}

namespace B {
use \DOMDocument as Bar;

class AA extends \A\A {

    public function create() {
        return new Bar();
    }
}

}
~~~

I'm using Phpstorm EAP latest and Php Inspections (EA Ultimate) 3.0.11.2 (Language Level PHP 7.4)

ktomk on 20 May 2021

@ktomk Your example does not have inheritance (which is the source of warning from this inspection). In my example AA extends A.

rob006 on 20 May 2021

@rob006 Yes, missed that one. Even I updated it to this regard (edited previous comment) it's still the same, I can't reproduce with it.

ktomk on 20 May 2021

@ktomk Maybe these classes need to be in separate files to trigger this warning?

34a8ec53

rob006 on 20 May 2021

@rob006 I could now reproduce (created two files), but only if I have both \Foo\Bar and \Bar\Bar undefined. As your screenshot shows in the use Bar\Bar; line, the Bar segments are highlighted so this seems to be the case at your end as well.

The moment the highlighting goes away (and Bar can be resolved) the method is not marked any longer as being able to drop.

In my case it was enough to add a third file stubs.php with the following content:

~~~

namespace Foo {
class Bar {}
}

namespace Bar {
class Bar {}
}
~~~

This should explain why with my single file example it was not an issue, as it fully defined all types in use.

I'd say having the types in use undefined is the parenting problem and the EA inspection only a side-effect of it.

Tested against EA extended and ultimate (current versions) and PhpStorm 2021.1.3 Preview.

ktomk on 21 May 2021

I can still reproduce this when both Bar classes have the same parent with __construct() defined.

<?php

namespace {
    class BaseBar {
        public function __construct() {}
    }
}

namespace Foo {
    class Bar extends \BaseBar {}
}

namespace Bar {
    class Bar extends \BaseBar {}
}

rob006 on 21 May 2021

@rob006 interesting case. for the matter of

[EA] 'create' method can be dropped, as it identical to 'A::create'.
_[on AA::create()]_

this would still make sense, doesn't it? By the inspection name SenselessMethodDuplicationInspection as there is a concrete type with two names (which I would consider more a feature rather than a bug for the inspection and more a bug than a feature of the example code).

So I tried to kick it a bit further. The first experiment was adding return type declarations on the PHP 7.2 language level for the create() methods, both are then public function create(): Bar { and as AA extends from A this highlights then a breakage:

Return type declaration must be compatible with A->create() : Foo\Bar
_[on AA::create()]_

This highlights an error in the type hierarchy between A and AA for create(), that is _otherwise hidden_ (this play clicked for me). To solve it, the base type can be used, both are then public function create(): BaseBar {. Now AA::create() has the original inspection message again as AA::create is senseless, perhaps now _more visible_ due to the return type hint (please try it out it's easier to edit the code than to describe it).

And it makes sense again as the implementation of AA::create() has become senseless again, the method returns the same concrete type as its parent, so why add it? (it also show that the original example implementation is borked)

Now that lead to the question what if that base type is abstract? No difference.

And what if there is no concrete base type, e.g. an interface?

~~~

namespace {
interface BaseBar {}
}

namespace Foo {
class Bar implements \BaseBar {}
}

namespace Bar {
class Bar implements \BaseBar {}
}
~~~

Now the inspection does not kick in any longer. This works with and without return type hinting for A::create() in its hierarchy.

Another experiment I did was not using return type declarations but making the concrete Bar types different. First by _removing_ the constructor on the base class:

~~~

namespace {
class BaseBar {}
}

namespace Foo {
class Bar extends \BaseBar {}
}

namespace Bar {
class Bar extends \BaseBar {}
}
~~~

no inspection error that gives. (this example is noteworthy as it resembles the interface one)

Second by adding the constructor in a sub-type as well:

~~~

namespace {
class BaseBar {
public function __construct() {}
}
}

namespace Foo {
class Bar extends \BaseBar {}
}

namespace Bar {
class Bar extends \BaseBar {
public function __construct() {}
}
}
~~~
(can interchangeably be added in Foo or Bar)

no inspection error that gives.

So the more I try the more the "senseless-inspection" check makes sense as it highlights the named problem that the AA::create() method is senseless when it is. I'm actually a bit impressed.

Honestly I find this a good property of the inspection. And I tried quite some cases to break it.

So this is perhaps the question if the same type with two names is treated equal or not if the name is the only difference.

But this is fairly an esoteric question as in practice the return type hint has shown that the original implementation breaks the hierarchy and I would say that in such cases I want a good inspection tool highlight such an issue:

The return types - declared or not - are breaking the hierarchy. A and AA are conflicting and therefore A is conflicting itself due to being extended by AA. This does not happen per se, it happens due to AA::create() and exactly that one is highlighted.

From what we could have seen, EA inspections nails it, right? Can you spot a corner where it breaks?

ktomk on 21 May 2021

@ktomk Sorry, but I feel you're trying so hard to justify this (buggy) behavior, that you're starting to see things that does not exist. :P Even if that was a problem with type hierarchy, the SenselessMethodDuplicationInspection warning is not correct response to this and reporting identical methods in case they're working differently is clearly a bug. But this is not a problem with type hierarchy, and I have simplified 1-file 100% LSP-correct example that still reproduces this warning:


namespace Foo {
    class Bar {
        public function __construct() {}
    }

    class Foo {
        public function create(): Bar {
            return new Bar();
        }
    }
}

namespace Bar {
    class Bar extends \Foo\Bar {}

    class Foo extends \Foo\Foo  {
        public function create(): \Foo\Bar {
            return new Bar();
        }
    }
}

And I think it is related to the fact, that new Bar() is internally treated as Foo\Bar::__construct() call in both cases. Which is technically correct, because this is what will be executed, but inspection does not consider the fact, that __construct() have different effects in both cases. That is why removing __construct() in parent class, using interface or adding __construct() to child class hides this warning - new Bar() no longer points to the same __construct() implementation.

rob006 on 21 May 2021

@rob006: I beg to differ. Not that it can't be confusing, but also in this example the inspection makes sense to the exact point I raised: The annotated \Bar\Foo:create(): \Foo\Bar method can be dropped as it's return type differs not in type but only in name. Which is why I wrote:

So this is perhaps the question if the same type with two names is treated equal or not if the name is the only difference.

So while PHP allows to write the same code in different types within the same hierarchy, it is commonly accepted that this is superfluous to do.

The moment I add anything to the code that makes the duplicates/type-clones in the hierarchy different in concrete, the inspection warning goes away.

(full LSP support in PHP for return types comes later than PHP 7.2, I think it was 7.4).

ktomk on 21 May 2021

👎1

The annotated \Bar\Foo:create(): \Foo\Bar method can be dropped as it's return type differs not in type but only in name.

What do you mean by "name"? In PHP name of class (as FQN) is a type. Foo\Bar is different type than Bar\Bar, and type matters - Foo\Bar may have different behavior than Bar\Bar and won't pass type check if Bar\Bar is required.

rob006 on 21 May 2021

Consider you have two classes that are identical, and the one extends from the other. You can say they both have the same type, even though you can also say that you have two types as you have the different FQCN.

You request to not highlight senseless methods on them. That's fine but only if you solely look on the FQCN. By the types they can represent, these two types are not needed as two as an expression of the concrete code for two types. Therefore you get the inspection highlighting that there is the case the one or other method is superfluous (e.g. there is a senseless method).

As the tests have shown, you only get the inspection when BarBar and Foo\bar do _not_ have differences, the moment they have differences, it does not kick in. At least for all the cases so far presented.

So while theoretically they may represent a different type - which they could as they have different FQCNs - when they do not - despite different FQCNs - the inspection hints senseless parts.

ktomk on 21 May 2021

😕1 👎1

You may conflate to falsify with justification, and writing about it is certainly hard and confusing, but again with the last example, the moment the concrete type is actually one (and as this is concrete code, this is the case of which you wrote _"may"_, in concrete - and LSP in action):

~~~

namespace Foo {
class Bar {
public function __construct() {}
}

class Foo {
    public function create(): Bar {
        return new Bar();
    }
}

}

namespace Bar {
class Bar extends Foo\Bar {
public function __construct() { }
}

class Foo extends \Foo\Foo {
    public function create(): \Foo\Bar {
        return new Bar();
    }
}

}
~~~

The inspection does not kick in. (Again.)

I'm fine to write senseless code in PHP. Don't get me wrong. I maybe wouldn't complain then about other peoples senseless code as well then. However when I don't do, and using tools to do so, I wouldn't complain then if they would remind me of the senseless parts and it needs an extra round for me to understand what the heck is wrong in concrete. An inspection can only be a first marker to start thinking. But these are only my 2 cents, and I get the impression you're taking it far too seriously for a swift exchange and will step back from this if an open exchange is not possible.

ktomk on 21 May 2021

👎1

@ktomk I simply do not want to continue this discussion with you - it has become pointless at the moment when you decided that type does not mater and you can ignore it while detecting code duplication. Believe me or not, but type matters in PHP (like 90% of exception classes are nothing more that new type with whole implementation taken from parent) and you can't ignore it. This inspection suggests change that changes code behavior and it is not safe to remove this "duplicated" method - let's focus on that.

rob006 on 21 May 2021

This inspection suggests change that changes code behavior and it is not safe to remove this "duplicated" method - let's focus on that.

Can you elaborate what the nature of the change would be? And then explain what would be particularly unsafe with it? (it would be great if you provide working code examples so I can try it out on my end).

ktomk on 21 May 2021

@ea-inspections-team, would you please look into this one? Thanks!