NotOptimalIfConditions

warning on by default

Group: Control flow · PhpStorm name: NotOptimalIfConditionsInspection

Looks at the conditions of if/elseif statements and flags four kinds of problems: a cheap operand evaluated after an expensive one in an &&/|| chain (short-circuiting would save work if swapped), and/or keyword operators (low precedence, error-prone), an equality comparison on a value that the same && chain also tests with instanceof (likely a logic bug), and an instanceof check made redundant by another instanceof on the same subject against a related type.

Example

<?php
// D1 ordering
if (strlen($name) > 3 && $enabled) {}
if (\json_encode($id) || empty($cache)) {}
if (isset($rows[md5($k)]) && $limit) {}
if (!(trim($v) && ($v))) {}
if ($ok) {} elseif (json_decode($raw) || is_string($w)) {}
if (preg_match('/^v\d/', $tag) && $strict) {}
if ($enabled && strlen($name) > 3) {}
if (is_int($n) && $n) {}
if (($total = $cart->sum()) && $total > 10) {}
if (count($items) > 0 && $items[0]) {}
if (($head = array_pop($stack)) && $stack) {}
if (!isset($map[lower($key)]) && !array_key_exists($key, $map)) {}
// S4: side effects are never reordered
if (fwrite($log, $line) !== false && $verbose) {}
if (rename($tmp, $target) || $force) {}
if (session_start() && $user) {}
if (preg_match('/^v(\d+)/', $tag, $parts) && $strict) {}
if (recompute($totals) > 0 && $dirty) {}       // user function: impure
if (strlen(file_get_contents($path)) && $ok) {}
if ($repo->find($id) || empty($cache)) {}       // method call: impure
if (isset($rows[md5($k)]) && $rows) {}          // S3: $rows itself is guarded
if (fetch($a) and $b) {} // one operand for D1

// property fetches
final class Folder {
    public bool $open = false;
    public array $files = [];
    public bool $isEmpty { get => $this->files === []; }
    public function __construct(public ?string $label = null) {}
}
final class Lazy {
    public function __get(string $n) { return load($n); }
}
function scan(Folder $f, Lazy $l, $any, string $p) {
    if (strlen($p) > 3 && $f->open) {}
    if (trim($p) || $f->label) {}
    if (strlen($p) > 3 && !$f->isEmpty) {}      // get hook: computed, impure
    if (is_dir($p) && $l->cached) {}            // __get(): computed, impure
    if (strlen($p) > 3 && $any->flag) {}        // untyped receiver: unknown
    if ($f->isEmpty || $f->open) {}             // computed neighbour: impure
}

// D2 keyword operators
if ($p AND $q) {}
if ($p || ($q or $r)) {}
if ($p xor $q) {}

// D3 equality next to instanceof
if ($node != null && $node instanceof Leaf && $node->ok) {}
if ($node instanceof Leaf || $node === null) {}

// D4 redundant instanceof
interface Shape {}
class Circle implements Shape {}
class Ring extends Circle {}
if ($s instanceof Shape || $s instanceof Ring) {}
if ($s instanceof Circle && $s instanceof Shape) {}
if ($s instanceof Circle || $t instanceof Shape) {}

Reported:

• line 3: Cheaper check placed after a costlier one; evaluate it first.
• line 4: Cheaper check placed after a costlier one; evaluate it first.
• line 5: Cheaper check placed after a costlier one; evaluate it first.
• line 6: Cheaper check placed after a costlier one; evaluate it first.
• line 7: Cheaper check placed after a costlier one; evaluate it first.
• line 8: Cheaper check placed after a costlier one; evaluate it first.
• line 24: Use '&&' instead of 'and'.
• line 37: Cheaper check placed after a costlier one; evaluate it first.
• line 38: Cheaper check placed after a costlier one; evaluate it first.
• line 46: Use '&&' instead of 'and'.
• line 47: Use '||' instead of 'or'.
• line 51: Equality check on a value also tested with instanceof; verify the logic.
• line 58: Redundant instanceof: another check on the same value already covers this type.
• line 59: Redundant instanceof: another check on the same value already covers this type.

Options

Option	Type	Default	Effect
REPORT_LITERAL_OPERATORS	bool	true	enables D2
REPORT_INSTANCE_OF_FLAWS	bool	true	enables D3 and D4
SUGGEST_OPTIMIZING_CONDITIONS	bool	true	enables D1

Upstream tests set one option to true without disabling the others, so each fixture is effectively evaluated with all checks on.

Configure

In custos.json:

{
  "rules": {
    "NotOptimalIfConditions": {
      "enabled": false,
      "options": {
        "REPORT_LITERAL_OPERATORS": true,
        "REPORT_INSTANCE_OF_FLAWS": true,
        "SUGGEST_OPTIMIZING_CONDITIONS": true
      }
    }
  }
}

Suppress

Before the statement or declaration (or the first statement of the file), either of:

// @custos-ignore NotOptimalIfConditions

/**
 * @noinspection NotOptimalIfConditionsInspection
 */