Generic/UselessOverridingMethod: small improvements to the sniff code

[rodrigoprimo]

Description

This PR implements the following improvements to the Generic.CodeAnalysis.UselessOverridingMethod sniff, which were suggested by @jrfnl in this comment in another PR:

3. The "Skip for ... other method." block is now covered via the differentParentMethod() code snippet.
   I can see a bug in the condition though - function/method names in PHP are case-insensitive.
   There is currently no test where the function declaration name is not in the same case as the name used in the function call.
   Such a test should be added and the code should be flagged as potentially useless override, which would need a small fix in the condition of that code block.
4. I'm missing a test where the function call has an open parenthesis, but no close parenthesis.
   Adding this test would highlight that some extra defensive coding would be helpful.
   The "Skip for invalid code." block, which checks for the open parenthesis, should probably also check for a matching close parenthesis via isset($tokens[$next]['parenthesis_closer']) !== false.
   The redundant function call on line 118 - count($tokens) - can then be also removed and the $count in the for condition replaced. (That function call was redundant anyhow and should have used $phpcsFile->numTokens instead)

9. Looking critically, the $next === false condition seems redundant in nearly all conditions (and there are no tests conceivable which could ever hit the condition).
   If the function declaration would not have a scope closer, the scope opener would not be set, so $next can never be false as there will always be a close curly after whatever code we're looking at.
   Having said that, if the $next === false bits would be removed, it might be prudent to update the "Skip function without body." block to also verify there is a scope closer.
10. I'm missing a test with non-OO function declaration. Use of the parent keyword in the body of such a function would be invalid no matter what, however, for the purposes of this sniff, I think it would be more appropriate to ignore such functions as they are, for sure, not a "method override".

Item number 10 was implemented in two commits. One for removing the redundant $next === false condition and another to handle non-OO function declarations. As for handling non-OO function declarations, I changed the sniff code to bail early when the T_FUNCTION does not correspond to a class or anonymous class method. I used $phpcsFile->hasCondition() for that. This is the first time that I'm using this method, please let me know if you see a problem with this approach or if there is something that I missed. My assumption is that functions and also trait, interface, and enum methods cannot have a parent so the sniff can bail early in those cases.

Please let me know if you prefer that I split all those fixes into multiple PRs.

Suggested changelog entry

Generic/UselessOverridingMethod: account for method and parent methods with the same name but different case

I'm unsure if the other changes should be mentioned in the changelog or not. Maybe a second entry referencing "minor performance improvements" (which is most of the other changes but not all other changes)?

Related issues/external references

Discussed in #250 (review)

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 changed the sniff code to bail early when the T_FUNCTION does not correspond to a class or anonymous class method. I used $phpcsFile->hasCondition() for that. This is the first time that I'm using this method, please let me know if you see a problem with this approach or if there is something that I missed.

@rodrigoprimo I haven't looked at the code yet, but thought I'd respond to this comment straight away.

Have you considered nested function declarations ? i.e. https://3v4l.org/f34jJ

My assumption is that functions and also trait, interface, and enum methods cannot have a parent so the sniff can bail early in those cases.

• Interfaces: correct, not because they can't have a parent (they can, interfaces can extend each other and the class implementing the interface could have a parent class), but because methods in interfaces cannot contain code (and the sniff only looks at the body of methods, not at type declarations).
• Enums: correct, those cannot be extended, so cannot have a parent.
• Trait methods can have a "parent" and should be examined as they are basically "copied in" to the class using the trait and that class can have a parent class.
  It may not be good practice to have assumptions like that in a trait, but that's not the concern of this sniff.

[rodrigoprimo]

Thanks for your questions, @jrfnl.

Have you considered nested function declarations ? i.e. https://3v4l.org/f34jJ

Good point. I haven't considered nested functions when working on this.

I did a few tests and modified the original commit to also bail early when it finds a nested function, but now I'm worried that the condition that I introduced might caught more than just nested functions, and I might be introducing a bug. Is there another way to differentiate methods and functions? Maybe the risks of the change that I'm introducing are not worth the benefits? I am happy to follow your advice on this as you have way more experience than I do.

Trait methods can have a "parent" and should be examined as they are basically "copied in" to the class using the trait and that class can have a parent class.
It may not be good practice to have assumptions like that in a trait, but that's not the concern of this sniff.

Sorry, my original message could have been more clear. What I meant to say is that a method in an interface, enum, or trait cannot call a parent method as far as I could test. That being said, I can see that I made a mistake when testing traits and that a trait method can actually call a parent method. Thanks for pointing that out.

I have updated the original commit to not bail early for traits, and I included some tests covering trait methods.

[rodrigoprimo]

@jrfnl, I made some changes to the code that checks if a given function should be checked by the sniff or not after our conversation yesterday, and now this PR is ready for review. Thanks again for your help.

[jrfnl]

Hi @rodrigoprimo, thanks for working on this. Looking good! Just needs some dotting of the i's and crossing of the t's.

P.S.: Sorry, I made a small edit in the PR description as the quoted text didn't line up with the comment in the other PR (which was confusing me).

[jrfnl]

Please let me know if you prefer that I split all those fixes into multiple PRs.

@rodrigoprimo No need. The way you've done it now with atomic commits for each change is exactly how I like it and allows for reviewing each change individually 👍🏻

[rodrigoprimo]

@jrfnl, thanks for all your comments. This PR is ready for another review.

[rodrigoprimo]

This PR is ready for another review.

[jrfnl]

Thanks @rodrigoprimo ! This looks good to me now. Nice clean up and improvements for this sniff.

I will rebase the PR interactively to get back to the original atomic commits and then merge the PR once the build has passed.