Fix bugs and adopt PHPCSUtils in AlwaysReturnInFilterSniff#881
Merged
Fix bugs and adopt PHPCSUtils in AlwaysReturnInFilterSniff#881
Conversation
The `isReturningVoid()` method wrapped `Tokens::$emptyTokens` in an extra array, preventing whitespace tokens from being skipped. This meant `return ;` (with a space) was not detected as a void return. Additional improvements: - Eliminate duplicate `findNext()` call in `processFunction()` - Use `FunctionDeclarations::getName()` for reliable name matching - Use `TextStrings::stripQuotes()` instead of `substr()` for stripping quotes from callback function names - Handle arrow function (`T_FN`) callbacks, which always return a value implicitly Ref #520
GaryJones
force-pushed
the
GaryJones/fix-always-return
branch
from
f10c972 to
74ba5b6
Compare
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Summary
The
AlwaysReturnInFilterSniffhad a subtle bug inisReturningVoid()whereTokens::$emptyTokenswas wrapped in an extra array ([ Tokens::$emptyTokens ]). Since PHPCS'sfindNextcompares each value in the exclusion array against token codes using===, the nested array never matched anything, meaning no tokens were skipped. This causedreturn ;(with whitespace before the semicolon) to go undetected as a void return, whilereturn;worked by accident because the next token happened to beT_SEMICOLON.This PR fixes that bug and makes several related quality improvements:
The duplicate
findNextcall inprocessFunction()— where the while condition called it and discarded the result, then the next line called it identically — is collapsed into a single assignment-in-condition pattern, matching the style used elsewhere in the codebase.Manual name resolution via
findNext(Tokens::$emptyTokens, ...)followed by a string content check is replaced withFunctionDeclarations::getName()from PHPCSUtils, andsubstr($content, 1, -1)for quote stripping is replaced withTextStrings::stripQuotes().Arrow function (
T_FN) callbacks are now handled: since they always implicitly return a value, they are safe as filter callbacks and the sniff now returns early rather than falling through to flag them.New test cases cover the void return with whitespace bug fix and both arrow function scenarios.
Test plan
composer testpasses (specificallyAlwaysReturnInFilterUnitTest)return ;with whitespace is now correctly detected as a void returnfn($x) => ...) do not trigger false positives