Skip to content

Conversation

xiemaisi
Copy link

A cheap and cheerful query inspired by this PVS Studio check. It flags loops of the form

for(...; i < a.length; ...) {
  ... a[c] ...
}

where all index operations on a inside the loop body use a constant index c and not the loop variable i.

Dist-compare shows a bunch of true positives. The alerts on ChakraCore are false positives (the tests do this on purpose), but they are filtered away by default anyway.

Testing on LGTM.com, I also found this plausible-looking result in the source code for the Angular.js homepage. (There are a few copies of the same code in some other projects.)

@xiemaisi xiemaisi added the JS label Sep 10, 2019
@xiemaisi xiemaisi requested a review from a team as a code owner September 10, 2019 08:46
@ghost
Copy link

ghost commented Sep 11, 2019

LGTM. I have nothing to add.

Interestingly, the rdf-store alert is in code from a parser generator (https://github.com/pegjs/pegjs).

ghost
ghost previously approved these changes Sep 11, 2019
@xiemaisi
Copy link
Author

Ping @Semmle/doc for query help review.

Copy link
Contributor

@shati-patel shati-patel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One comment, otherwise LGTM.

<p>
If the loop variable of a <code>for</code> loop ranges over the indices of an array, that variable
would normally be used as an array index in the body of the loop. If, instead, the loop body only
refers to array elements at constant indices, this may indicate a logic error or left-over testing
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
refers to array elements at constant indices, this may indicate a logic error or left-over testing
refers to array elements at constant indices, this may indicate a logic error or leftover testing

@semmle-qlci semmle-qlci merged commit 72db219 into github:master Sep 11, 2019
@xiemaisi xiemaisi deleted the js/unused-index-variable branch October 17, 2019 10:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants