PHPCSStandards / PHP_CodeSniffer

PHP_CodeSniffer tokenizes PHP files and detects violations of a defined set of coding standards.
BSD 3-Clause "New" or "Revised" License
907 stars 55 forks source link

Generic.CodeAnalysis.JumbledIncrementer: prevent false positives / check with higher accuracy #441

Open jrfnl opened 6 months ago

jrfnl commented 6 months ago

The following issues were found during the review of PR #385.

Describe the bug

While the sniff works well for the most common case of a variable being incremented/decremented in the third expression in the condition of a for loop, it does not take other possible operations involving variables into account, making it highly inaccurate for all but the most common case.

Code sample

for ($i = 0; $i < 20; $i++) {
    for ($j = 0; $j < 20; $j = $i + 10) { // $i is not being written to, so is not being jumbled here.
    }
}

$j = 0;
for ($i = 40; $i > 20; $i -= $j) { // $j is not being written to here ...
    for (; $j < 20; $j++) { // ... so $j is not being jumbled here.
    }
}

To reproduce

Steps to reproduce the behavior:

  1. Create a file called test.php with the code sample above...
  2. Run phpcs test.php --standard=Generic --sniffs=Generic.CodeAnalysis.JumbledIncrementer
  3. See error message displayed
    2 | WARNING | Loop incrementor ($i) jumbling with inner loop (Generic.CodeAnalysis.JumbledIncrementer.Found)
    8 | WARNING | Loop incrementer ($j) jumbling with inner loop (Generic.CodeAnalysis.JumbledIncrementer.Found)

Expected behavior

No warnings to be thrown when a variable seen in the third expression is not being written to.

Additional context

Here are some additional code samples for inspiration as each of these will also result in a false positive:

for ($i = 0; $i < 20; $i++) {
    for ($j = 0; $j < 20; $j[$i]++) {
    }
}

for ($i['key'] = 0; $i['key'] < 20; $i['key']++) {
    for ($i['otherkey'] = 0; $i['otherkey'] < 20; $i['otherkey']++) {
    }
}

for (; $this->propA < 20 && $this->propB > 0; $this->propA++, --$this->propB) {
    for (; $this->propC < 20; $this->propC++) {
    }
}

$var = 50;
for ($i = 0; $i < 20; $i = functionCall($var)) {
    for (; $var < 20; --$var) {
    }
}

$i = 0
for ($varA = 0, $varB = 0; $i < 20; $varA++, $varB++)) {
    for (; $i < 20; $i = $varA + $varB)) {
    }
}

Please confirm:

rodrigoprimo commented 5 months ago

Adding one more false positive from #385:

$label = 'label';

for ($i = 1; $i <= 10; $i++, print $label) {
    for ($j = 0; $j < 5; $j++, print $label);
}