Improvements for the Generic.CodeAnalysis.UnusedFunctionParameter sniff

[jrfnl]

This PR contains one improvement to the existing code and two enhancements. It also contains two minor efficiency tweaks.

Each change is contained in its own commit.

Efficiency tweaks

• Don't make a function call in a loop control structure
• Bow out early if the function doesn't have any parameters

Only allow for empty function bodies if function implements interface

The sniff currently allowed for unused parameters in functions with empty bodies to allow for selective interface implementation.
The sniff, however, did not verify if the function was in a class which implemented an interface.
This has now been fixed.

Allow for ignoring functions in child classes and classes which implement interfaces.

The function signature of methods in extended classes has to mirror the method as defined in the parent class/interface.
The overloaded method may, however, not use all params.

This change introduces a new $ignore_extended_classes property which defaults to false to retain the original behaviour.
When this property is set to true, the sniff will blanket ignore all functions in classes which extend another class and in classes which implement interfaces.

This change introduces two new errorcodes which allow a user to ignore these errors by excluding the errorcode from their custom ruleset.

The new errorcodes are:

• FoundInExtendedClass
• FoundInImplementedInterface

Allow for unused parameters in functions used as callbacks.

(Callback) functions may not use all parameters passed, but have to allow in their function signature for the ones they do use to be passed to them.

This change introduces a new $allow_for_callbacks property which defaults to false to retain the original behaviour.
When this property is set to true, the sniff will only report on unused parameters after the last used parameter.

This commit introduces additional differentiation in the error codes used based on:

• whether the only param was not used (Found)
• whether a param was not used, but positioned before the last used param (FoundBeforeLastUsed)
• whether a param was not used and positioned after the last used param (FoundAfterLastUsed)

These codes will combine with the FoundInExtendedClass and FoundInImplementedInterface error codes as introduced in the previous commit.

Unit tests are included for all changes.

[jrfnl]

Any opinions on this PR ?

[gsherwood]

I understand the extra validation of methods in a class with implemented interfaces. Thanks for that.

The ignore_extended_classes option makes a lot of sense, but I'm wondering if there is a better way to do this than using an option.

This sniff only generates warnings because everything needs to be independently verified. I think it might be better to use a new message code for the different types of code structures this sniff checks. If you don't want to ever see warnings for specific structures, you can mute the errors in a ruleset. But if you don't mute the warnings, you can at least see that the messages are different and might need to be verified differently. You could even bump some warnings to errors, or change the message content.

Right now there is Generic.CodeAnalysis.UnusedFunctionParameter.Found but maybe there should be a second Generic.CodeAnalysis.UnusedFunctionParameter.FoundInExtendedClass warning as well.

The allow_for_callbacks option is a bit confusing to me - maybe just because of the name. It seems to change the behaviour of the sniff for all functions and methods without any way of knowing if the function was being used as a callback. It seems like a very specific name for something that seems quite broad.

So again, maybe different warning codes could be used to differentiate between method params before and after the last used param. So maybe this sniff needs the following message codes:

• Generic.CodeAnalysis.UnusedFunctionParameter.Found
• Generic.CodeAnalysis.UnusedFunctionParameter.FoundAfterLast
• Generic.CodeAnalysis.UnusedFunctionParameter.FoundInExtendedClass
• Generic.CodeAnalysis.UnusedFunctionParameter.FoundAfterLastInExtendedClass

What are your thoughts on that idea?

[jrfnl]

I went with the options to prevent breaking backward compatibility for the sniff. Current users expect the current behaviour which would be changed if I added errorcodes without the options.

I'd be happy to change the PR to add new errorcodes instead, but the code changes will be more extensive in that case I think.

The allow_for_callbacks option is a bit confusing to me - maybe just because of the name. It seems to change the behaviour of the sniff for all functions and methods without any way of knowing if the function was being used as a callback. It seems like a very specific name for something that seems quite broad.

Normally - if one is in full control of the code, function declarations and function calls -, you would define a function to only expect the params it needs and be called as such.
However, when callbacks are used you are not in control of which params are passed and if you work within a framework, you may not have that luxury either.
Typical example in this case is WordPress. WordPress works with actions and filters, which in practice are callbacks. You can hook your own functions onto these actions and filters, but have no control over which params are passed to the function, so need to set up your "hooked-in" functions to be able to receive all the params you need.
If you need only param 1 and 3, you still need to set up your function to receive param 2, though you can safely ignore param 4 and 5 in your function declaration.

The issue of superfluous params before the last param used generally only comes in play when callbacks are used as otherwise one could have set up the functions in a leaner way, which is why I named the options after it.

To be fair, maybe this should be changed no matter what without the option as if you use a param after an unused param, it generally means you can't removed the unused param from your function declaration anyway.

[gsherwood]

To be fair, maybe this should be changed no matter what without the option as if you use a param after an unused param, it generally means you can't removed the unused param from your function declaration anyway.

The fact that a param further along in the list was used probably means that the earlier param had to be there, even if it isn't used. It might be there because the function sig is being inherited from elsewhere (like with a callback) or it might be there for backwards compatibility (maybe it is a deprecated arg). Either way, it's probably needed, but it could use a review.

Given this sniff produces warnings, I think that the message should still be generated as it indicates a manual review might be required. But if the error codes were different, you could just mute this message.

Which brings me to...

I'd be happy to change the PR to add new errorcodes instead, but the code changes will be more extensive in that case I think.

I've been thinking about error code and sniff properties a fair bit since you submitted those docs for the wiki. I think PHPCS would be better without so many sniff options, and I think this might be something I target for 4.0.

Ideally, you'd be able to set things like indent globally rather than per sniff, meaning those sniff props can be removed. I'd also remove all the error props and force the errors to instead be excluded in the ruleset.

I'd also like to be able to use error codes to differentiate between different code structures instead of using properties to change sniff behaviour, allowing for those errors to be muted. You can use a ruleset to exclude those specific messages globally, or just for a subset of files. If the ignoreFile, ignoreStart/End and ignoreLine comment syntax is extended, you could also mute those errors for specific code blocks. I think this is neater than changing sniff settings inline and it standardises how you interact with the PHPCS rules.

All that just to say: I think it's best to stop adding sniff properties if there is another way to do things. So I'd prefer this sniff to use different errors codes so it is ready for a future push away from properties.

[jrfnl]

@gsherwood Thanks for your reply. I'll have a go at rewriting the changes with that in mind when I have some time.

I think PHPCS would be better without so many sniff options

When I created the list, I was actually surprised that there were so few properties considering how many sniffs there are.

I can see how a move towards errorcodes would be useful as they are more exposed than properties, however, in a number of cases, a sniff will stop (return) after throwing an error and ignore the rest of the sniff. IMHO that behaviour will need to be looked at as well when moving away from properties as properties are now sometimes a good way to skip over certain checks and prevent a sniff from returning early and that would become impossible if only relying on errorcodes.

[jrfnl]

@gsherwood Ok, I've redone the PR, removed the properties and focused on having different errorcodes.

The current PR results in the following list of errorcodes for this sniff:

• Found
• FoundBeforeLastUsed
• FoundAfterLastUsed
• FoundInExtentedClass
• FoundInExtentedClassBeforeLastUsed
• FoundInExtentedClassAfterLastUsed
• FoundInImplementedInterface
• FoundInImplementedInterfaceBeforeLastUsed
• FoundInImplementedInterfaceAfterLastUsed

I'll update the issue description above with the information on the new implementation.

As the list of codes is quite extensive now, I can imagine that combining the various Found codes with their equivalent FoundAfterLastUsed could be beneficial.
IMHO the different FoundBeforeLastUsed codes should stay separate as those are the ones which users will most often want to exclude.

[jrfnl]

Rebased for merge conflicts/ v 3.0 release

[alexander-akait]

@jrfnl @gsherwood what is status?

[jrfnl]

Rebased for merge conflicts & updated code style.

[jrfnl]

@gsherwood Is there anything I can do to move this forward ?

[gsherwood]

Thanks for making the error code changes.

Narrowly missed the 2 year anniversary of this one...

[jrfnl]

Thanks for merging this in!