Generic/UnusedFunctionParameter: bug fix x 2 (magic methods and error codes) #3715
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jrfnl
force-pushed
the
feature/generic-unusedfunctionparam-various-improvements
branch
from
ddf4dbb
to
4933983
Compare
Aren't |
jrfnl
force-pushed
the
feature/generic-unusedfunctionparam-various-improvements
branch
from
4933983
to
047b254
Compare
@gsherwood Ow! Good catch. Thanks for that! I've updated the commit to exclude |
|
(also updated the commit description above now) |
jrfnl
force-pushed
the
feature/generic-unusedfunctionparam-various-improvements
branch
from
047b254
to
767d15d
Compare
…row functions
As things were, if a closure or arrow function declared within a class which extends or implements would have an unused function parameter, it would get the `InExtendedClass` or `InImplementedInterface` addition in the error code.
Those additions were only intended for function declarations where the declaration would potentially be overloading a method from a parent and would have to comply with the method signature of the method in the parent class/interface.
This could lead to underreporting if a standard explicitly excludes the error codes contain `InExtendedClass` and/or `InImplementedInterface`.
Fixed now.
Includes additional unit test, though the tests don't safeguard this much as they don't check the error codes of the messages thrown.
The change can be tested manually by running the new tests against `master`, which will show:
```
163 | WARNING | The method parameter $d is never used (Generic.CodeAnalysis.UnusedFunctionParameter.FoundInExtendedClassAfterLastUsed)
172 | WARNING | The method parameter $d is never used
| | (Generic.CodeAnalysis.UnusedFunctionParameter.FoundInImplementedInterfaceAfterLastUsed)
```
... while with the change in this commit, this will be fixed to:
```
163 | WARNING | The method parameter $d is never used (Generic.CodeAnalysis.UnusedFunctionParameter.FoundAfterLastUsed)
172 | WARNING | The method parameter $d is never used (Generic.CodeAnalysis.UnusedFunctionParameter.FoundAfterLastUsed)
```
The function signature of magic methods - with the exception of `__construct()` and `__invoke()` - is dictated by PHP and unused parameters cannot be removed, which means that the warnings for these can never be resolved, only ignored via annotations. This commit fixes this by checking whether a function is a magic method and if so, bowing out. Includes unit tests. Note: while not all magic methods take arguments, I'm still including the (nearly) full list of magic methods in the property as the other magic methods can be ignored anyway (no arguments).
jrfnl
force-pushed
the
feature/generic-unusedfunctionparam-various-improvements
branch
from
767d15d
to
fcff7fa
Compare
DannyvdSluijs
approved these changes
Aug 24, 2023
|
Closing as replaced by PHPCSStandards/PHP_CodeSniffer#50 |
jrfnl
deleted the
feature/generic-unusedfunctionparam-various-improvements
branch
|
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). |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Generic/UnusedFunctionParameter: ignore class context for closures/arrow functions
As things were, if a closure or arrow function declared within a class which extends or implements would have an unused function parameter, it would get the
InExtendedClassorInImplementedInterfaceaddition in the error code.Those additions were only intended for function declarations where the declaration would potentially be overloading a method from a parent and would have to comply with the method signature of the method in the parent class/interface.
This could lead to underreporting if a standard explicitly excludes the error codes contain
InExtendedClassand/orInImplementedInterface.Fixed now.
Includes additional unit test, though the tests don't safeguard this much as they don't check the error codes of the messages thrown.
The change can be tested manually by running the new tests against
master, which will show:... while with the change in this commit, this will be fixed to:
Generic/UnusedFunctionParameter: ignore magic methods
The function signature of magic methods - with the exception of
__construct()and__invoke()- is dictated by PHP and unused parameters cannot be removed, which means that the warnings for these can never be resolved, only ignored via annotations.This commit fixes this by checking whether a function is a magic method and if so, bowing out.
Includes unit tests.
Note: while not all magic methods take arguments, I'm still including the (nearly) full list of magic methods in the property as the other magic methods can be ignored anyway (no arguments).