Improve early return sniff negating logic#2819
Conversation
📝 WalkthroughWalkthroughA private helper method Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 |
![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/FlipIfToEarlyReturnSniff.php`:
- Around line 246-272: Update hasTopLevelOperator() to treat PHP word operators
("and", "or", "xor") as compound operators in addition to "&&" and "||" so the
FlipIfToEarlyReturnSniff no longer treats `$a and $b` as a simple condition;
specifically, when scanning the condition string in hasTopLevelOperator(),
detect top-level occurrences of the tokens and/or using word-boundary checks
(e.g. '\band\b', '\bor\b', '\bxor\b') while still respecting parentheses depth
and ignoring matches inside sub-expressions or strings, and return true when any
such top-level word operator is found so the caller (the $isCompound check) will
wrap compound conditions in parentheses before negation.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
phpcs-sniffs/Formidable/Sniffs/CodeAnalysis/FlipIfToEarlyReturnSniff.php
🧰 Additional context used
📓 Path-based instructions (1)
**/*.php
⚙️ CodeRabbit configuration file
**/*.php: - Do not make suggestions when we use x.x for a version placeholder. This is used like "@SInCE x.x" in a PHP doc comment, but also like 'x.x' when functions like _deprecated_function() are used.
- Treat these placeholders as acceptable in this codebase.
Files:
phpcs-sniffs/Formidable/Sniffs/CodeAnalysis/FlipIfToEarlyReturnSniff.php
🧬 Code graph analysis (1)
phpcs-sniffs/Formidable/Sniffs/CodeAnalysis/FlipIfToEarlyReturnSniff.php (1)
phpcs-sniffs/Formidable/Sniffs/CodeAnalysis/FlipForeachIfToContinueSniff.php (1)
flipComparisonOperator(271-299)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: PHP 7.4 tests in WP 6.9
- GitHub Check: PHP 8 tests in WP 6.9
- GitHub Check: Cypress
- GitHub Check: Cypress
- GitHub Check: PHP 7.4 tests in WP 6.9
- GitHub Check: PHP 8 tests in WP 6.9
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| // 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 ); | ||
| } | ||
| if ( strpos( $condition, '!' ) === 0 && strpos( $condition, '!=' ) !== 0 ) { | ||
| return substr( $condition, 1 ); | ||
| } | ||
|
|
||
| // Try to flip comparison operators. | ||
| $flipped = $this->flipComparisonOperator( $condition ); | ||
| // Try to flip comparison operators. | ||
| $flipped = $this->flipComparisonOperator( $condition ); | ||
|
|
||
| if ( $flipped !== false ) { | ||
| return $flipped; | ||
| } | ||
| if ( $flipped !== false ) { | ||
| return $flipped; | ||
| } | ||
|
|
||
| // Otherwise, add negation. | ||
| // If condition is simple (no spaces or operators at top level), just add ! | ||
| // If complex, wrap in parentheses. | ||
| if ( $this->isSimpleCondition( $condition ) ) { | ||
| // 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
🏁 Script executed:
cd /root/repo && head -300 phpcs-sniffs/Formidable/Sniffs/CodeAnalysis/FlipIfToEarlyReturnSniff.php | tail -80Repository: Strategy11/formidable-forms
Length of output: 131
🏁 Script executed:
cd /root/repo && sed -n '240,320p' phpcs-sniffs/Formidable/Sniffs/CodeAnalysis/FlipIfToEarlyReturnSniff.phpRepository: Strategy11/formidable-forms
Length of output: 131
🌐 Web query:
PHP operator precedence: and vs && or vs || negation semantics
💡 Result:
Key points (from the PHP manual):
-
Precedence order (relevant subset): ! (logical NOT) → && → || → = (assignment) → and / xor / or. [1]
-
Practical consequences:
- && and || bind tighter than assignment, so
- $a = true && false; // parsed as $a = (true && false) → $a === false. [1]
- and and or have lower precedence than assignment, so
- $a = true and false; // parsed as ($a = true) and false → $a === true. [1]
- && and || bind tighter than assignment, so
-
Negation: the unary ! has higher precedence than the logical binary ops, so ! $x is evaluated before &&/||. Use parentheses when mixing negation with assignment or other operators to make intent explicit. [1]
Recommendation: always use parentheses when precedence might be unclear (or prefer &&/|| with explicit parentheses instead of and/or) to avoid surprising behavior. [1]
Source: PHP manual — Operator precedence. [1]
🏁 Script executed:
git ls-files | grep -i "flipif" | head -5Repository: Strategy11/formidable-forms
Length of output: 220
🏁 Script executed:
sed -n '240,320p' phpcs-sniffs/Formidable/Sniffs/CodeAnalysis/FlipIfToEarlyReturnSniff.phpRepository: Strategy11/formidable-forms
Length of output: 1908
🏁 Script executed:
sed -n '275,315p' phpcs-sniffs/Formidable/Sniffs/CodeAnalysis/FlipIfToEarlyReturnSniff.phpRepository: Strategy11/formidable-forms
Length of output: 845
🏁 Script executed:
sed -n '1,100p' phpcs-sniffs/Formidable/Sniffs/CodeAnalysis/FlipIfToEarlyReturnSniff.php | head -60Repository: Strategy11/formidable-forms
Length of output: 1356
🏁 Script executed:
sed -n '100,200p' phpcs-sniffs/Formidable/Sniffs/CodeAnalysis/FlipIfToEarlyReturnSniff.phpRepository: Strategy11/formidable-forms
Length of output: 3192
🏁 Script executed:
# Check if word operators appear in the test suite or examples to assess practical impact
git grep -n "and\|or\|xor" phpcs-sniffs/Formidable/Sniffs/CodeAnalysis/FlipIfToEarlyReturnSniff.phpRepository: Strategy11/formidable-forms
Length of output: 6415
🏁 Script executed:
# Also check for test files
git ls-files | grep -i "test\|spec" | grep -i flipRepository: Strategy11/formidable-forms
Length of output: 53
🏁 Script executed:
sed -n '318,375p' phpcs-sniffs/Formidable/Sniffs/CodeAnalysis/FlipIfToEarlyReturnSniff.phpRepository: Strategy11/formidable-forms
Length of output: 1614
Extend hasTopLevelOperator() to detect word operators (and/or/xor).
The method currently checks only && and ||, missing word operators. Conditions like $a and $b are treated as simple and negated to ! $a and $b, which changes semantics due to PHP operator precedence (where ! binds tighter than and/or/xor, unlike &&/||). Use parentheses consistently by detecting all compound operators.
🔧 Suggested update to detect word operators
- // Check for && or ||.
+ // Check for && or ||.
if ( $i < $len - 1 ) {
$twoChars = $condition[ $i ] . $condition[ $i + 1 ];
if ( $twoChars === '&&' || $twoChars === '||' ) {
return true;
}
}
+
+ // Check for word operators (and/or/xor) with word boundaries.
+ if ( ctype_alpha( $char ) ) {
+ $remaining = substr( $condition, $i );
+ foreach ( array( 'and', 'or', 'xor' ) as $wordOp ) {
+ $lenOp = strlen( $wordOp );
+ if ( strncmp( $remaining, $wordOp, $lenOp ) === 0 ) {
+ $before = $i === 0 ? ' ' : $condition[ $i - 1 ];
+ $after = $i + $lenOp >= $len ? ' ' : $condition[ $i + $lenOp ];
+ if ( ! ctype_alnum( $before ) && $before !== '_' && ! ctype_alnum( $after ) && $after !== '_' ) {
+ return true;
+ }
+ }
+ }
+ }🤖 Prompt for AI Agents
In `@phpcs-sniffs/Formidable/Sniffs/CodeAnalysis/FlipIfToEarlyReturnSniff.php`
around lines 246 - 272, Update hasTopLevelOperator() to treat PHP word operators
("and", "or", "xor") as compound operators in addition to "&&" and "||" so the
FlipIfToEarlyReturnSniff no longer treats `$a and $b` as a simple condition;
specifically, when scanning the condition string in hasTopLevelOperator(),
detect top-level occurrences of the tokens and/or using word-boundary checks
(e.g. '\band\b', '\bor\b', '\bxor\b') while still respecting parentheses depth
and ignoring matches inside sub-expressions or strings, and return true when any
such top-level word operator is found so the caller (the $isCompound check) will
wrap compound conditions in parentheses before negation.
1 participant
The previous logic broke a condition.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.