Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Generic/UnusedFunctionParameter: bug fix x 2 (magic methods and error codes) #50

Merged

Conversation

jrfnl
Copy link
Member

@jrfnl jrfnl commented Nov 9, 2023

Description

Recreation of upstream PR squizlabs/PHP_CodeSniffer#3715:

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 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)

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).


Feedback left in the original PR:

The function signature of magic methods is dictated by PHP and unused parameters cannot be removed

Aren't __construct and __invoke exceptions to this?


Response to the feedback:

The function signature of magic methods is dictated by PHP and unused parameters cannot be removed

Aren't __construct and __invoke exceptions to this?

@gsherwood Ow! Good catch. Thanks for that!

I've updated the commit to exclude __construct() and __invoke() now and I've added two extra sets of tests at the bottom of the test file to safeguard that those will continue to trigger errors.

(also updated the commit description above now)

Suggested changelog entry

  • Generic/UnusedFunctionParameter: fixed incorrect errorcode for closures/arrow functions nested within extended classes/classes which implement.
  • Generic/UnusedFunctionParameter: ignore magic methods for which the signature is defined by PHP

…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 jrfnl force-pushed the feature/generic-unusedfunctionparam-various-improvements branch from fbd2dcf to 78ea17b Compare December 5, 2023 22:17
@jrfnl jrfnl merged commit a96d0ee into master Dec 5, 2023
65 checks passed
@jrfnl jrfnl deleted the feature/generic-unusedfunctionparam-various-improvements branch December 5, 2023 22:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant