New Rule: prefer-for-of (palantir#1435)

Adds a new rule to prefer for-of loops when the index in a standard for loop is never used.

src/rules/preferForOfRule.ts

@@ -0,0 +1,122 @@
+/**
+ * @license
+ * Copyright 2016 Palantir Technologies, Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+import * as Lint from "../lint";
+import * as ts from "typescript";
+
+export class Rule extends Lint.Rules.AbstractRule {
+    /* tslint:disable:object-literal-sort-keys */
+    public static metadata: Lint.IRuleMetadata = {
+        ruleName: "prefer-for-of",
+        description: "Recommends a 'for-of' loop over a standard 'for' loop if the index is only used to access the array being iterated.",
+        rationale: "A for(... of ...) loop is easier to implement and read when the index is not needed.",
+        optionsDescription: "Not configurable.",
+        options: null,
+        optionExamples: ["true"],
+        type: "typescript",
+    };
+    /* tslint:enable:object-literal-sort-keys */
+
+    public static FAILURE_STRING = "Expected a 'for-of' loop instead of a 'for' loop with this simple iteration";
+
+    public apply(sourceFile: ts.SourceFile): Lint.RuleFailure[] {
+        const languageService = Lint.createLanguageService(sourceFile.fileName, sourceFile.getFullText());
+        return this.applyWithWalker(new PreferForOfWalker(sourceFile, this.getOptions(), languageService));
+    }
+}
+
+class PreferForOfWalker extends Lint.RuleWalker {
+    constructor(sourceFile: ts.SourceFile, options: Lint.IOptions, private languageService: ts.LanguageService) {
+        super(sourceFile, options);
+    }
+
+    public visitForStatement(node: ts.ForStatement) {
+        const arrayAccessNode = this.locateArrayNodeInForLoop(node);
+
+        if (arrayAccessNode !== undefined) {
+            // Skip arrays thats just loop over a hard coded number
+            // If we are accessing the length of the array, then we are likely looping over it's values
+            if (arrayAccessNode.kind === ts.SyntaxKind.PropertyAccessExpression && arrayAccessNode.getLastToken().getText() === "length") {
+                let incrementorVariable = node.incrementor.getFirstToken();
+                if (/\+|-/g.test(incrementorVariable.getText())) {
+                    // If it's formatted as `++i` instead, we need to get the OTHER token
+                    incrementorVariable = node.incrementor.getLastToken();
+                }
+                const arrayToken = arrayAccessNode.getChildAt(0);
+                const loopSyntaxText = node.statement.getText();
+                // Find all usages of the incrementor variable
+                const fileName = this.getSourceFile().fileName;
+                const highlights = this.languageService.getDocumentHighlights(fileName, incrementorVariable.getStart(), [fileName]);
+
+                if (highlights && highlights.length > 0) {
+                    // There are *usually* three usages when setting up the for loop,
+                    // so remove those from the count to get the count inside the loop block
+                    const incrementorCount = highlights[0].highlightSpans.length - 3;
+
+                    // Find `array[i]`-like usages by building up a regex 
+                    const arrayTokenForRegex = arrayToken.getText().replace(".", "\\.");
+                    const incrementorForRegex = incrementorVariable.getText().replace(".", "\\.");
+                    const regex = new RegExp(`${arrayTokenForRegex}\\[\\s*${incrementorForRegex}\\s*\\]`, "g");
+                    const accessMatches = loopSyntaxText.match(regex);
+                    const matchCount = (accessMatches || []).length;
+
+                    // If there are more usages of the array item being access than the incrementor variable
+                    // being used, then this loop could be replaced with a for-of loop instead.
+                    // This means that the incrementor variable is not used on its own anywhere and is ONLY
+                    // used to access the array item.
+                    if (matchCount >= incrementorCount) {
+                        const failure = this.createFailure(node.getStart(), node.getWidth(), Rule.FAILURE_STRING);
+                        this.addFailure(failure);
+                    }
+                }
+            }
+        }
+
+        super.visitForStatement(node);
+    }
+
+    private locateArrayNodeInForLoop(forLoop: ts.ForStatement): ts.Node {
+        // Some oddly formatted (yet still valid!) `for` loops might not have children in the condition
+        // See: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/for
+        if (forLoop.condition !== undefined) {
+            let arrayAccessNode = forLoop.condition.getChildAt(2);
+            // If We haven't found it, maybe it's not a standard for loop, try looking in the initializer for the array
+            // Something like `for(var t=0, len=arr.length; t < len; t++)`
+            if (arrayAccessNode.kind !== ts.SyntaxKind.PropertyAccessExpression && forLoop.initializer !== undefined) {
+                for (let initNode of forLoop.initializer.getChildren()) {
+                    // look in `var t=0, len=arr.length;`
+                    if (initNode.kind === ts.SyntaxKind.SyntaxList) {
+                        for (let initVar of initNode.getChildren()) {
+                            // look in `t=0, len=arr.length;`
+                            if (initVar.kind === ts.SyntaxKind.VariableDeclaration) {
+                                for (let initVarPart of initVar.getChildren()) {
+                                    // look in `len=arr.length`
+                                    if (initVarPart.kind === ts.SyntaxKind.PropertyAccessExpression) {
+                                        arrayAccessNode = initVarPart;
+                                    }
+                                }
+                            }
+                        }
+                    }
+                }
+            }
+            return arrayAccessNode;
+        } else {
+            return undefined;
+        }
+    }
+}

src/tsconfig.json

@@ -136,6 +136,7 @@
         "rules/oneVariablePerDeclarationRule.ts",
         "rules/onlyArrowFunctionsRule.ts",
         "rules/orderedImportsRule.ts",
+        "rules/preferForOfRule.ts",
         "rules/quotemarkRule.ts",
         "rules/radixRule.ts",
         "rules/restrictPlusOperandsRule.ts",

test/rules/prefer-for-of/test.ts.lint

@@ -0,0 +1,109 @@
+function sampleFunc() {
+
+    //This loop only uses the iterator to access the array item, so we can recommend a for-of loop here
+    for (var a = 0; a <= obj.arr.length; a++) {
+    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+        console.log(obj.arr[a]);
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+    }
+~~~~~ [0]
+
+    //Same as above, but no curly braces
+    for (var b = 0; b <= obj.arr.length; b++) console.log(obj.arr[b]);
+    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ [0]
+
+    //the index is used by itself, so a normal for loop is allowed here
+    for (var c = 0; c <= arr.length; c++) {
+        doMath(c);
+    }
+
+    //Same as above, but no curly braces
+    for (var d = 0; d <= arr.length; d++) doMath(d);
+
+    //the index is used by itself, so a normal for loop is allowed here
+    for (var e = 0; e <= arr.length; e++) {
+        if(e > 5) {
+            doMath(e);
+        }
+        console.log(arr[e]);
+    }
+
+    //This iterates off of a hard-coded number and should be allowed
+    for (var f = 0; f <= 40; f++) {
+        doMath(f);
+    }
+
+    //Same as above, but no curly braces
+    for (var g = 0; g <= 40; g++) doMath(g);
+
+    //Loop set up different, but uses the index alone - this is ok
+    for(var h=0, len=arr.length; h < len; h++) {
+        doMath(h);
+    }
+
+    //Same as above, but no curly braces
+    for(var i=0, len=arr.length; i < len; i++) doMath(i);
+
+    //Loop set up different, but uses the index alone - this is ok
+    for(var j=0, len=arr.length; j < len; j++){
+        if(j > 5) {
+            doMath(j);
+        }
+        console.log(arr[j]);
+    }
+
+    //Loop set up different, only uses the index to access the array - this should fail
+    for(var k=0, len=arr.length; k < len; k++) {
+    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+        console.log(arr[k]);
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+    }
+~~~~~ [0]
+
+    //Same as above, but no curly braces
+    for(var l=0, len=arr.length; l < len; l++) console.log(arr[l]);
+    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ [0]
+
+    //Odd for loop setups
+    var m = 0;
+    for (;;) {
+        if (m > 3) break;
+        console.log(m);
+        m++;
+    }
+
+    var n = 0;
+    for (; n < 9; n++) {
+        console.log(n);
+    }
+
+    var o = 0;
+    for (; o < arr.length; o++) {
+    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+        console.log(arr[o]);
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+    }
+~~~~~ [0]
+
+    //Prefix incrementor
+    for(let p = 0; p < arr.length; ++p) {
+    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+        arr[p].whatever();
+~~~~~~~~~~~~~~~~~~~~~~~~~~
+    }
+~~~~~ [0]
+
+    //For in loops ARE allowed
+    for (var q in obj) {
+        if (obj.hasOwnProperty(q)) {
+            console.log(q);
+        }
+    }
+
+    //For of loops ARE allowed
+    for (var r of arr) {
+        console.log(r);
+    }
+}
+
+[0]: Expected a 'for-of' loop instead of a 'for' loop with this simple iteration

test/rules/prefer-for-of/tslint.json

@@ -0,0 +1,5 @@
+{
+  "rules": {
+    "prefer-for-of": true
+  }
+}

test/tsconfig.json

@@ -131,6 +131,7 @@
         "../src/rules/oneVariablePerDeclarationRule.ts",
         "../src/rules/onlyArrowFunctionsRule.ts",
         "../src/rules/orderedImportsRule.ts",
+        "../src/rules/preferForOfRule.ts",
         "../src/rules/quotemarkRule.ts",
         "../src/rules/radixRule.ts",
         "../src/rules/restrictPlusOperandsRule.ts",