Add new early return sniff when function is just a return var declara…#2851
Conversation
📝 WalkthroughWalkthroughIntroduces a new PHP_CodeSniffer sniff that detects patterns where a variable is initialized, followed by an if block, and the function returns that variable. The sniff identifies such patterns and provides an automatic fixer to transform them into early-return structures when the if body contains a minimum number of statements (default: 5). The ruleset.xml is reorganized with updated sniff registrations. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Crabcyborg
deleted the
add_new_early_return_sniff_when_function_is_var_declarion_if_and_return
branch
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@phpcs-sniffs/Formidable/Sniffs/CodeAnalysis/FlipIfToEarlyReturnInitialValueSniff.php`:
- Around line 519-549: negation for simple non-flippable comparisons in
negateCondition produces '! $x > 0' which breaks precedence, and
flipComparisonOperator is missing mappings for '<' and '>' so they never get
flipped; update negateCondition to wrap the fallback negation in parentheses
(return '! ( ' . $condition . ' )' instead of '! ' . $condition) and extend
flipComparisonOperator to include mappings for '>' => '<=', '>=' => '<', '<' =>
'>=', and '<=' => '>' (and their spaced variants if the method normalizes
spacing) so comparisons like $x > 0 and $x < 0 are correctly flipped rather than
left to the fallback.
🧹 Nitpick comments (1)
phpcs-sniffs/Formidable/Sniffs/CodeAnalysis/FlipIfToEarlyReturnInitialValueSniff.php (1)
319-336: Drop unused$phpcsFileparameters in helpers.
countStatementsInScope()andgetConditionString()don’t use$phpcsFile, which triggers PHPMD. Remove the parameter and update call sites.♻️ Proposed refactor
- $statementCount = $this->countStatementsInScope( $phpcsFile, $tokens, $ifToken ); + $statementCount = $this->countStatementsInScope( $tokens, $ifToken ); - $condition = $this->getConditionString( $phpcsFile, $tokens, $ifToken ); + $condition = $this->getConditionString( $tokens, $ifToken );- private function countStatementsInScope( File $phpcsFile, array $tokens, $scopeToken ) { + private function countStatementsInScope( array $tokens, $scopeToken ) {- private function getConditionString( File $phpcsFile, array $tokens, $ifToken ) { + private function getConditionString( array $tokens, $ifToken ) {Also applies to: 347-368
| private function negateCondition( $condition ) { | ||
| $condition = trim( $condition ); | ||
|
|
||
| // Check if this is a compound condition (has && or || at top level). | ||
| $isCompound = $this->hasTopLevelOperator( $condition ); | ||
|
|
||
| // Only remove leading ! if it's a simple condition (not compound). | ||
| if ( ! $isCompound ) { | ||
| // If condition starts with !, remove it. | ||
| if ( strpos( $condition, '! ' ) === 0 ) { | ||
| return substr( $condition, 2 ); | ||
| } | ||
|
|
||
| if ( strpos( $condition, '!' ) === 0 && strpos( $condition, '!=' ) !== 0 ) { | ||
| return substr( $condition, 1 ); | ||
| } | ||
|
|
||
| // Try to flip comparison operators. | ||
| $flipped = $this->flipComparisonOperator( $condition ); | ||
|
|
||
| if ( $flipped !== false ) { | ||
| return $flipped; | ||
| } | ||
|
|
||
| // Simple condition, just add ! | ||
| return '! ' . $condition; | ||
| } | ||
|
|
||
| // For compound conditions, wrap in parentheses and negate. | ||
| return '! ( ' . $condition . ' )'; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
PHP operator precedence ! vs comparison operators
💡 Result:
The unary NOT operator (!) has higher precedence than PHP's comparison operators (==, ===, <, >, <=, >=). So ! binds first:
- !$a == $b is parsed as (! $a) == $b.
- To negate a comparison you must parenthesize: !($a == $b).
Source: PHP operator precedence table. [1]
References
[1] PHP Manual — Operator Precedence.
🏁 Script executed:
# Find and examine the PHP file to verify the claims
fd "FlipIfToEarlyReturnInitialValueSniff.php" --type fRepository: Strategy11/formidable-forms
Length of output: 155
🏁 Script executed:
# Read the PHP file to verify the code and operator map
wc -l phpcs-sniffs/Formidable/Sniffs/CodeAnalysis/FlipIfToEarlyReturnInitialValueSniff.phpRepository: Strategy11/formidable-forms
Length of output: 159
🏁 Script executed:
# Get the negateCondition function and surrounding code (lines 519-549)
sed -n '510,560p' phpcs-sniffs/Formidable/Sniffs/CodeAnalysis/FlipIfToEarlyReturnInitialValueSniff.phpRepository: Strategy11/formidable-forms
Length of output: 1394
🏁 Script executed:
# Check lines 600-623 for related code
sed -n '595,630p' phpcs-sniffs/Formidable/Sniffs/CodeAnalysis/FlipIfToEarlyReturnInitialValueSniff.phpRepository: Strategy11/formidable-forms
Length of output: 1006
🏁 Script executed:
# Find the operatorMap definition
rg -n "operatorMap" phpcs-sniffs/Formidable/Sniffs/CodeAnalysis/FlipIfToEarlyReturnInitialValueSniff.php -A 10Repository: Strategy11/formidable-forms
Length of output: 688
Fix negation for </> to avoid operator precedence bugs in the fixer.
For conditions like $x > 0, the fallback at line 544 emits ! $x > 0, which PHP parses as (! $x) > 0 due to ! having higher precedence than comparison operators. This changes semantics. Additionally, the flipComparisonOperator() method (lines 600–623) lacks entries for < and > operators, preventing them from being flipped. Wrap the fallback negation in parentheses and add both operators to the map.
Proposed fix
- // Simple condition, just add !
- return '! ' . $condition;
+ // Simple condition: wrap to avoid precedence surprises.
+ return '! ( ' . $condition . ' )'; $operatorMap = array(
'!==' => '===',
'===' => '!==',
+ '>' => '<=',
+ '<' => '>=',
'!=' => '==',
'==' => '!=',
'>=' => '<',
'<=' => '>',
);🤖 Prompt for AI Agents
In
`@phpcs-sniffs/Formidable/Sniffs/CodeAnalysis/FlipIfToEarlyReturnInitialValueSniff.php`
around lines 519 - 549, negation for simple non-flippable comparisons in
negateCondition produces '! $x > 0' which breaks precedence, and
flipComparisonOperator is missing mappings for '<' and '>' so they never get
flipped; update negateCondition to wrap the fallback negation in parentheses
(return '! ( ' . $condition . ' )' instead of '! ' . $condition) and extend
flipComparisonOperator to include mappings for '>' => '<=', '>=' => '<', '<' =>
'>=', and '<=' => '>' (and their spaced variants if the method normalizes
spacing) so comparisons like $x > 0 and $x < 0 are correctly flipped rather than
left to the fallback.
…hen_function_is_var_declarion_if_and_return Add new early return sniff when function is just a return var declara…
1 participant
…tion and if
This fixes issues in Pro, but it doesn't look to be an issue in Lite.
Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.