Squiz/NonExecutableCode: simplify + improve handling of comments and closures

[jrfnl]

Description

Some tweaks for the sniff which I noticed while reviewing #3770.

Squiz/NonExecutableCode: make sniff more code style independent

When determining whether a return statement is the last code token in a function body, comments should be ignored, but weren't.

Fixed now. Includes tests.

Squiz/NonExecutableCode: flag redundant return statements in closures too

A return statement which doesn't return a value at the end of a function body would be flagged as "not required" for named functions, but not so for anonymous functions.

Fixed now. Includes tests.

Squiz/NonExecutableCode: fold duplicate code

Follow up on commits 0e10f43 and 01754d9, which both deal with fixing bugs where the sniff would not handle if/elseif/else conditions without curly braces correctly.

This commit merges the two near duplicate code blocks, which the above mentioned commits introduced, each containing code doing essentially the same thing.

Also note that T_ELSE is handled separately now as else does not take parentheses and can therefore not be a parenthesis owner.

This change is already covered by pre-existing tests.

Suggested changelog entry

• Squiz/NonExecutableCode: redundant return statements just before a function close brace will now be flagged more often.

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)
  • This change is only breaking for integrators, not for external standards or end-users.
• Documentation improvement

[fredden]

While reviewing this pull request, I noticed there is a false-negative in this sniff. With the following test file, I get a warning about Squiz.PHP.NonExecutableCode.ReturnNotRequired on line 9, but not on line 6. Even if I remove line 9, I still don't get a warning about line 6.

<?php

function test() {
    if (true) {
        echo 'yes';
        return;
    }

    return;
}

Is this something which is in scope here, or should I raise this separately so it can be fixed in a different pull request?

[jrfnl]

While reviewing this pull request, I noticed there is a false-negative in this sniff. With the following test file, I get a warning about Squiz.PHP.NonExecutableCode.ReturnNotRequired on line 9, but not on line 6. Even if I remove line 9, I still don't get a warning about line 6.

<?php

function test() {
    if (true) {
        echo 'yes';
        return;
    }

    return;
}

Is this something which is in scope here, or should I raise this separately so it can be fixed in a different pull request?

In my opinion that is outside the scope of this PR. The ReturnNotRequired check currently only checks for a return statement without a return value, which is directly followed by the function close brace.

Expanding that check to find other potentially redundant return statements could get complex and I think this might even be better off in a separate sniff.

[jrfnl]

Closing as replaced by PHPCSStandards/PHP_CodeSniffer#92

[jrfnl]

FYI: this fix is included in today's PHP_CodeSniffer 3.8.0 release.

As per #3932, development on PHP_CodeSniffer will continue in the PHPCSStandards/PHP_CodeSniffer repository. If you want to stay informed, you may want to start "watching" that repo (or watching releases from that repo).