Skip to content

Improve early return sniff negating logic #2819

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.

Sign up for GitHub

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

Merged
Crabcyborg merged 1 commit into from
Jan 16, 2026
Merged

Improve early return sniff negating logic #2819

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
77 changes: 61 additions & 16 deletions phpcs-sniffs/Formidable/Sniffs/CodeAnalysis/FlipIfToEarlyReturnSniff.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -243,32 +243,77 @@ private function dedentCode( $code ) {
private function negateCondition( $condition ) {
$condition = trim( $condition );

// If condition starts with !, remove it.
if ( strpos( $condition, '! ' ) === 0 ) {
return substr( $condition, 2 );
}
// 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 . ' )';
Comment on lines +246 to 272

@coderabbitai coderabbitai Bot Jan 16, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

cd /root/repo && head -300 phpcs-sniffs/Formidable/Sniffs/CodeAnalysis/FlipIfToEarlyReturnSniff.php | tail -80

Repository: Strategy11/formidable-forms

Length of output: 131

🏁 Script executed:

cd /root/repo && sed -n '240,320p' phpcs-sniffs/Formidable/Sniffs/CodeAnalysis/FlipIfToEarlyReturnSniff.php

Repository: 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]

  • 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 -5

Repository: Strategy11/formidable-forms

Length of output: 220

🏁 Script executed:

sed -n '240,320p' phpcs-sniffs/Formidable/Sniffs/CodeAnalysis/FlipIfToEarlyReturnSniff.php

Repository: Strategy11/formidable-forms

Length of output: 1908

🏁 Script executed:

sed -n '275,315p' phpcs-sniffs/Formidable/Sniffs/CodeAnalysis/FlipIfToEarlyReturnSniff.php

Repository: Strategy11/formidable-forms

Length of output: 845

🏁 Script executed:

sed -n '1,100p' phpcs-sniffs/Formidable/Sniffs/CodeAnalysis/FlipIfToEarlyReturnSniff.php | head -60

Repository: Strategy11/formidable-forms

Length of output: 1356

🏁 Script executed:

sed -n '100,200p' phpcs-sniffs/Formidable/Sniffs/CodeAnalysis/FlipIfToEarlyReturnSniff.php

Repository: 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.php

Repository: Strategy11/formidable-forms

Length of output: 6415

🏁 Script executed:

# Also check for test files
git ls-files | grep -i "test\|spec" | grep -i flip

Repository: Strategy11/formidable-forms

Length of output: 53

🏁 Script executed:

sed -n '318,375p' phpcs-sniffs/Formidable/Sniffs/CodeAnalysis/FlipIfToEarlyReturnSniff.php

Repository: 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.

}

/**
* Check if a condition has && or || at the top level (not inside parentheses).
*
* @param string $condition The condition to check.
*
* @return bool
*/
private function hasTopLevelOperator( $condition ) {
$parenDepth = 0;
$len = strlen( $condition );

for ( $i = 0; $i < $len; $i++ ) {
$char = $condition[ $i ];

if ( $char === '(' ) {
++$parenDepth;
continue;
}

if ( $char === ')' ) {
--$parenDepth;
continue;
}

// Only check at top level.
if ( $parenDepth !== 0 ) {
continue;
}

// Check for && or ||.
if ( $i < $len - 1 ) {
$twoChars = $condition[ $i ] . $condition[ $i + 1 ];

if ( $twoChars === '&&' || $twoChars === '||' ) {
return true;
}
}
}

return false;
}

/**
* Try to flip a comparison operator in a condition.
*
Expand Down