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: improve the actionability of the error messages #440

Open
1 task
jrfnl opened this issue Apr 10, 2024 · 0 comments

Comments

@jrfnl
Copy link
Member

jrfnl commented Apr 10, 2024

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.
@jrfnl jrfnl changed the title Generic.CodeAnalysis.JumbledIncrementor: improve the actionability of the error messages Generic.CodeAnalysis.JumbledIncrementer: improve the actionability of the error messages Apr 10, 2024
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

1 participant