Skip to content

Generic.CodeAnalysis.JumbledIncrementer: improve the actionability of the error messages #440

Open
@jrfnl

Description

@jrfnl

Inspired by PR #385 (thought which came up while reviewing the changes made in #385).

Is your feature request related to a problem?

Given the following code sample (taken from the existing tests):

<?php
for ($i = 0; $i < 20; $i++) {
    for ($j = 0; $j < 5; $i += 2) {
        for ($k = 0; $k > 3; $i++) {
            
        }
    }
}

The sniff will currently throw the following errors:

FILE: Generic/Tests/CodeAnalysis/JumbledIncrementerUnitTest.inc
---------------------------------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 3 WARNINGS AFFECTING 2 LINES
---------------------------------------------------------------------------------------------------------------
  2 | WARNING | Loop incrementor ($i) jumbling with inner loop (Generic.CodeAnalysis.JumbledIncrementer.Found)
  2 | WARNING | Loop incrementor ($i) jumbling with inner loop (Generic.CodeAnalysis.JumbledIncrementer.Found)
  3 | WARNING | Loop incrementor ($i) jumbling with inner loop (Generic.CodeAnalysis.JumbledIncrementer.Found)
---------------------------------------------------------------------------------------------------------------

... which for a short code snippet as above is sort-of clear, but for longer loops can mean a lot of searching within the loops to see where the "jumbling" is happening.

In more detail - as things are now:

  • The first warning relates to the $i++ on line 2 being "jumbled" by the $i += 2 on line 3.
  • The second warning relates to the $i++ on line 2 being "jumbled" by the $i++ on line 4.
  • The third warning relates to the $i += 2 on line 3 being "jumbled" by the $i++ on line 4.

Describe the solution you'd like

I believe the message could be made more actionable by mentioning the original incrementor ($i) and the line on which it is found (this is in place already), but then would also mention the line where the jumbling takes place, or even the line + the code snippet doing the jumbling.

It could also be considered to throw the warning on the line where the "jumbling" takes place and mention the line where the original incrementor is found in the warning message.

Additional context (optional)

To do this will necessitate significant changes/refactoring of the sniff as the sniff would need to keep track of the stack pointers of the variables seen, not just the name of the variables.

  • I intend to create a pull request to implement this feature.

Metadata

Metadata

Assignees

No one assigned

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions