Page MenuHomePhabricator

Add a sniff for unneeded __construct functions
Open, LowPublic

Description

A new sniff should be added to detect __construct functions that only call the parent construct and nothing else.

Example[1]
public function __construct() {
	parent::__construct();
}

These are unneeded.

[1] https://gerrit.wikimedia.org/r/#/c/mediawiki/extensions/ChessBrowser/+/563005/5/includes/ChessParser.php@102

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald Transcript

Same applied to tests' setup and teardown

protected function setUp() : void {
	parent::setUp();
}

protected function tearDown() : void {
	parent::tearDown();
}

I've sent patches to fix the ones I've come across so far: https://gerrit.wikimedia.org/r/#/q/topic:unneeded-override+(status:open+OR+status:merged)

Does it make sense to expand the idea of this sniff to all methods that do nothing but calling their own parent? Or is there a reason to ever keep anything like shown in the examples above?

I think the sniff should not touch methods that contain comments or a non-trivial PHPDoc block, even if the executable code is nothing but a parent call.

I also think the sniff should not touch methods that use different parameter names than the parent. Good, self-explaining parameter names are documentation as well. There might be a situation where the parameter names of the parent are bad, and a subclass tries to improve this, without going through the trouble of having to change the parent the same time.

That said, I believe there is not that much value in creating such a sniff. It will probably be one of the slower sniffs we have (needs to check the content of all methods), for not much value. But I might be wrong. Maybe there is a quick way to detect trivial cases as shown in the examples above.

Does it make sense to expand the idea of this sniff to all methods that do nothing but calling their own parent? Or is there a reason to ever keep anything like shown in the examples above?

I think the sniff should not touch methods that contain comments or a non-trivial PHPDoc block, even if the executable code is nothing but a parent call.

I also think the sniff should not touch methods that use different parameter names than the parent. Good, self-explaining parameter names are documentation as well. There might be a situation where the parameter names of the parent are bad, and a subclass tries to improve this, without going through the trouble of having to change the parent the same time.

That said, I believe there is not that much value in creating such a sniff. It will probably be one of the slower sniffs we have (needs to check the content of all methods), for not much value. But I might be wrong. Maybe there is a quick way to detect trivial cases as shown in the examples above.

There are some methods that are overriden to change the contract of the method (thinking specifically of RevisionArchiveRecord, MutableRevisionRecord, and RevisionStoreRecord overrides) but those should be covered by the "non-trivial PHPDoc block" check.

Agree that checking all of the contents would be slow, but perhaps we should only implement this for the most common methods - the ones I've come across with unneeded overrides are setUp, and tearDown, and I caught a use of __construct in code review.

That being said, I found a few more in searching for these, and can send patches for them as I come across more if a sniff wouldn't be efficient

There is a Generic.CodeAnalysis.UselessOverridingMethod which is disabled due to some false positives

Have not tested if it would catch all the examples from here

I think an upstream sniff, if well-maintained, would be preferrable. There are several caveats in implementing what this task asks for: comments, differences in the docblock (but watch out for @inheritDoc, which on its own doesn't justify a method override), a different signature, different method visibility are the first things that come to mind.