Skip to content
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.

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

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

Open
3 tasks done
jrfnl opened this issue Apr 10, 2024 · 1 comment
Open
3 tasks done

Comments

@jrfnl
Copy link
Member

jrfnl commented Apr 10, 2024

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:

  • I have searched the issue list and am not opening a duplicate issue.
  • I confirm that this bug is a bug in PHP_CodeSniffer and not in one of the external standards.
  • I have verified the issue still exists in the master branch of PHP_CodeSniffer.
@rodrigoprimo
Copy link
Contributor

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);
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants