Generic/UnusedFunctionParameter: improve code coverage

[rodrigoprimo]

Description

This PR improves the code coverage for the Generic.CodeAnalysis.UnusedFunctionParameter sniff.

It also includes three commits that change the sniff code directly.

The first commit removes an unused variable.

The second commit renames two $tmp variables to have more descriptive names (the new names are a bit long, so I ended up splitting two lines into multiple lines to improve the readability of the code).

The third commit removes an if condition that, as far as I could check, is not reachable. More information is in the commit message.

Happy to drop those three commits as they are outside of the scope of #146.

I might be missing something, but while working on the new tests, I noticed that the following code triggers the sniff while I believe it should not trigger it:

class InterfaceMethodNotImplement implements SomeInterface {
    public function notImplemented2($param) {
        return 'string' . 'concatenation';
    }
}

The sniff is not triggered for methods that are part of a class that implements an interface and that return on the first line. Is there are reason that I'm missing that would justify triggering this sniff for such methods when there is more than one non-empty token between the return statement and the semicolon?

Related issues/external references

Part of #146

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

PR checklist

• I have checked there is no other PR open for the same change.
• I have read the Contribution Guidelines.
• I grant the project the right to include and distribute the code under the BSD-3-Clause license (and I have the right to grant these rights).
• I have added tests to cover my changes.
• I have verified that the code complies with the projects coding standards.
• [Required for new sniffs] I have added XML documentation for the sniff.

[jrfnl]

I might be missing something, but while working on the new tests, I noticed that the following code triggers the sniff while I believe it should not trigger it:

class InterfaceMethodNotImplement implements SomeInterface {
    public function notImplemented2($param) {
        return 'string' . 'concatenation';
    }
}

The sniff is not triggered for methods that are part of a class that implements an interface and that return on the first line. Is there are reason that I'm missing that would justify triggering this sniff for such methods when there is more than one non-empty token between the return statement and the semicolon?

@rodrigoprimo This may just have been an oversight when the sniff was originally created, though I'm not sure it is.
The code pattern being looked for is typically something like return false; when the interface method implementation does not contain any logic.
As soon as more is being done in the return statement than returning a simple value, it is an indication that the statement needs to be looked at and can not be presumed to be an interface method.
Think return is_something( $a ) ? $a : false;.

Keep in mind that the sniff cannot check whether a method being implemented is an interface method as it doesn't have access to the method names expected by the interface, it can only make assumptions based on common code patterns.

Note: might be an idea to open an issue for questions like this instead of tightly coupling them to a pull request, which can delay the pull request (as the question would need to be answered before the PR can be merged).

[jrfnl]

Pro-tip: when posting code samples, add the language after the triple backtick to allow GH to highlight the code for better readability. I.e. ```php

[jrfnl]

Thanks @rodrigoprimo ! LGTM.

I'll rebase the PR (without changes) before merging it.

For the future - when making functional changes, please explain those briefly in the PR description instead of pointing to a commit message.
(or include commit message(s) in the PR description)

[jrfnl]

Actually... I'm making one small change and that's removing a comment which is redundant in the last commit.