From f9d7577266ceafdc4f72a777cd9a846c5f382507 Mon Sep 17 00:00:00 2001 From: Andy Hanson Date: Tue, 14 Mar 2017 20:14:13 -0700 Subject: [PATCH 1/2] Clean up prefer-for-of Don't unnecessarily use a BlockScopeAwareRuleWalker, just store a stack of for-loop scopes --- src/rules/preferForOfRule.ts | 263 +++++++++++++------------- test/rules/prefer-for-of/test.ts.lint | 4 +- 2 files changed, 138 insertions(+), 129 deletions(-) diff --git a/src/rules/preferForOfRule.ts b/src/rules/preferForOfRule.ts index fab472a2bb4..de69bb04060 100644 --- a/src/rules/preferForOfRule.ts +++ b/src/rules/preferForOfRule.ts @@ -36,168 +36,177 @@ export class Rule extends Lint.Rules.AbstractRule { 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[] { - return this.applyWithWalker(new PreferForOfWalker(sourceFile, this.getOptions())); + return this.applyWithFunction(sourceFile, walk); } } -interface IIncrementorState { +interface IncrementorState { + indexVariableName: string; arrayToken: ts.Identifier; - forLoopEndPosition: number; onlyArrayReadAccess: boolean; } -// a map of incrementors and whether or not they are only used to index into an array reference in the for loop -type IncrementorMap = Map; +function walk(ctx: Lint.WalkContext): void { + const scopes: IncrementorState[] = []; -class PreferForOfWalker extends Lint.BlockScopeAwareRuleWalker { - public createScope() {} // tslint:disable-line:no-empty + return ts.forEachChild(ctx.sourceFile, cb); - public createBlockScope() { - return new Map(); + function cb(node: ts.Node): void { + switch (node.kind) { + case ts.SyntaxKind.ForStatement: + return visitForStatement(node as ts.ForStatement); + case ts.SyntaxKind.Identifier: + return visitIdentifier(node as ts.Identifier); + default: + return ts.forEachChild(node, cb); + } } - protected visitForStatement(node: ts.ForStatement) { - const arrayNodeInfo = this.getForLoopHeaderInfo(node); - const currentBlockScope = this.getCurrentBlockScope(); - let indexVariableName: string | undefined; - if (node.incrementor != null && arrayNodeInfo != null) { - const { indexVariable, arrayToken } = arrayNodeInfo; - indexVariableName = indexVariable.getText(); - - // store `for` loop state - currentBlockScope.set(indexVariableName, { - arrayToken: arrayToken as ts.Identifier, - forLoopEndPosition: node.incrementor.end + 1, - onlyArrayReadAccess: true, - }); + function visitForStatement(node: ts.ForStatement): void { + const arrayNodeInfo = getForLoopHeaderInfo(node); + if (!node.incrementor || !arrayNodeInfo) { + return ts.forEachChild(node, cb); } - super.visitForStatement(node); - - if (indexVariableName != null) { - const incrementorState = currentBlockScope.get(indexVariableName)!; - if (incrementorState.onlyArrayReadAccess) { - this.addFailureFromStartToEnd(node.getStart(), incrementorState.forLoopEndPosition, Rule.FAILURE_STRING); - } + const { indexVariable, arrayToken } = arrayNodeInfo; + const indexVariableName = indexVariable.text; + + // store `for` loop state + const state: IncrementorState = { + indexVariableName, + arrayToken: arrayToken as ts.Identifier, + onlyArrayReadAccess: true, + }; + scopes.push(state); + ts.forEachChild(node.statement, cb); + scopes.pop(); + + if (state.onlyArrayReadAccess) { + ctx.addFailure(node.getStart(), node.statement.getFullStart(), Rule.FAILURE_STRING); + } + } - // remove current `for` loop state - currentBlockScope.delete(indexVariableName); + function visitIdentifier(node: ts.Identifier): void { + const state = getStateForVariable(node.text); + if (state) { + updateIncrementorState(node, state); } } - protected visitIdentifier(node: ts.Identifier) { - const incrementorScope = this.findBlockScope((scope) => scope.has(node.text)); - - if (incrementorScope != null) { - const incrementorState = incrementorScope.get(node.text); - - // check if the identifier is an iterator and is currently in the `for` loop body - if (incrementorState != null && incrementorState.arrayToken != null && incrementorState.forLoopEndPosition < node.getStart()) { - // check if iterator is used for something other than reading data from array - if (node.parent!.kind === ts.SyntaxKind.ElementAccessExpression) { - const elementAccess = node.parent as ts.ElementAccessExpression; - const arrayIdentifier = unwrapParentheses(elementAccess.expression) as ts.Identifier; - if (incrementorState.arrayToken.getText() !== arrayIdentifier.getText()) { - // iterator used in array other than one iterated over - incrementorState.onlyArrayReadAccess = false; - } else if (elementAccess.parent != null && isAssignment(elementAccess.parent)) { - // array position is assigned a new value - incrementorState.onlyArrayReadAccess = false; - } - } else { - incrementorState.onlyArrayReadAccess = false; - } + function getStateForVariable(name: string): IncrementorState | undefined { + for (let i = scopes.length - 1; i >= 0; i--) { + const scope = scopes[i]; + if (scope.indexVariableName === name) { + return scope; } - super.visitIdentifier(node); } + return undefined; } +} - // returns the iterator and array of a `for` loop if the `for` loop is basic. Otherwise, `null` - private getForLoopHeaderInfo(forLoop: ts.ForStatement) { - let indexVariableName: string | undefined; - let indexVariable: ts.Identifier | undefined; - - // assign `indexVariableName` if initializer is simple and starts at 0 - if (forLoop.initializer != null && forLoop.initializer.kind === ts.SyntaxKind.VariableDeclarationList) { - const syntaxList = forLoop.initializer.getChildAt(1); - if (syntaxList.kind === ts.SyntaxKind.SyntaxList && syntaxList.getChildCount() === 1) { - const assignment = syntaxList.getChildAt(0); - if (assignment.kind === ts.SyntaxKind.VariableDeclaration && assignment.getChildCount() === 3) { - const value = assignment.getChildAt(2).getText(); - if (value === "0") { - indexVariable = assignment.getChildAt(0) as ts.Identifier; - indexVariableName = indexVariable.getText(); - } +function updateIncrementorState(node: ts.Identifier, state: IncrementorState): void { + // check if iterator is used for something other than reading data from array + if (node.parent!.kind === ts.SyntaxKind.ElementAccessExpression) { + const elementAccess = node.parent as ts.ElementAccessExpression; + const arrayIdentifier = unwrapParentheses(elementAccess.expression) as ts.Identifier; + if (state.arrayToken.getText() !== arrayIdentifier.getText()) { + // iterator used in array other than one iterated over + state.onlyArrayReadAccess = false; + } else if (elementAccess.parent && isAssignment(elementAccess.parent)) { + // array position is assigned a new value + state.onlyArrayReadAccess = false; + } + } else { + state.onlyArrayReadAccess = false; + } + +} + +// returns the iterator and array of a `for` loop if the `for` loop is basic. +function getForLoopHeaderInfo(forLoop: ts.ForStatement): { indexVariable: ts.Identifier, arrayToken: ts.Expression } | undefined { + let indexVariableName: string | undefined; + let indexVariable: ts.Identifier | undefined; + + // assign `indexVariableName` if initializer is simple and starts at 0 + if (forLoop.initializer && forLoop.initializer.kind === ts.SyntaxKind.VariableDeclarationList) { + const syntaxList = forLoop.initializer.getChildAt(1); + if (syntaxList.kind === ts.SyntaxKind.SyntaxList && syntaxList.getChildCount() === 1) { + const assignment = syntaxList.getChildAt(0); + if (assignment.kind === ts.SyntaxKind.VariableDeclaration && assignment.getChildCount() === 3) { + const value = assignment.getChildAt(2).getText(); + if (value === "0") { + indexVariable = assignment.getChildAt(0) as ts.Identifier; + indexVariableName = indexVariable.getText(); } } } + } - // ensure `for` condition - if (indexVariableName == null - || forLoop.condition == null - || forLoop.condition.kind !== ts.SyntaxKind.BinaryExpression - || forLoop.condition.getChildAt(0).getText() !== indexVariableName - || forLoop.condition.getChildAt(1).getText() !== "<") { + // ensure `for` condition + if (!indexVariableName + || !forLoop.condition + || forLoop.condition.kind !== ts.SyntaxKind.BinaryExpression + || forLoop.condition.getChildAt(0).getText() !== indexVariableName + || forLoop.condition.getChildAt(1).getText() !== "<") { - return null; - } + return undefined; + } - if (forLoop.incrementor == null || !this.isIncremented(forLoop.incrementor, indexVariableName)) { - return null; - } + if (!forLoop.incrementor || !isIncremented(forLoop.incrementor, indexVariableName)) { + return undefined; + } - // ensure that the condition checks a `length` property - const conditionRight = forLoop.condition.getChildAt(2); - if (conditionRight.kind === ts.SyntaxKind.PropertyAccessExpression) { - const propertyAccess = conditionRight as ts.PropertyAccessExpression; - if (indexVariable != null && propertyAccess.name.getText() === "length") { - return { indexVariable: indexVariable!, arrayToken: unwrapParentheses(propertyAccess.expression) }; - } + // ensure that the condition checks a `length` property + const conditionRight = forLoop.condition.getChildAt(2); + if (conditionRight.kind === ts.SyntaxKind.PropertyAccessExpression) { + const propertyAccess = conditionRight as ts.PropertyAccessExpression; + if (indexVariable && propertyAccess.name.getText() === "length") { + return { indexVariable: indexVariable!, arrayToken: unwrapParentheses(propertyAccess.expression) }; } + } - return null; + return undefined; +} + +function isIncremented(node: ts.Node, indexVariableName: string): boolean { + if (!node) { + return false; } - private isIncremented(node: ts.Node, indexVariableName: string) { - if (node == null) { - return false; + // ensure variable is incremented + if (node.kind === ts.SyntaxKind.PrefixUnaryExpression) { + const incrementor = node as ts.PrefixUnaryExpression; + if (incrementor.operator === ts.SyntaxKind.PlusPlusToken && incrementor.operand.getText() === indexVariableName) { + // x++ + return true; } - - // ensure variable is incremented - if (node.kind === ts.SyntaxKind.PrefixUnaryExpression) { - const incrementor = node as ts.PrefixUnaryExpression; - if (incrementor.operator === ts.SyntaxKind.PlusPlusToken && incrementor.operand.getText() === indexVariableName) { - // x++ - return true; - } - } else if (node.kind === ts.SyntaxKind.PostfixUnaryExpression) { - const incrementor = node as ts.PostfixUnaryExpression; - if (incrementor.operator === ts.SyntaxKind.PlusPlusToken && incrementor.operand.getText() === indexVariableName) { - // ++x - return true; - } - } else if (node.kind === ts.SyntaxKind.BinaryExpression) { - const binaryExpression = node as ts.BinaryExpression; - if (binaryExpression.operatorToken.getText() === "+=" - && binaryExpression.left.getText() === indexVariableName - && binaryExpression.right.getText() === "1") { - // x += 1 - return true; - } - if (binaryExpression.operatorToken.getText() === "=" - && binaryExpression.left.getText() === indexVariableName) { - const addExpression = binaryExpression.right as ts.BinaryExpression; - if (addExpression.operatorToken.getText() === "+") { - if (addExpression.right.getText() === indexVariableName && addExpression.left.getText() === "1") { - // x = 1 + x - return true; - } else if (addExpression.left.getText() === indexVariableName && addExpression.right.getText() === "1") { - // x = x + 1 - return true; - } + } else if (node.kind === ts.SyntaxKind.PostfixUnaryExpression) { + const incrementor = node as ts.PostfixUnaryExpression; + if (incrementor.operator === ts.SyntaxKind.PlusPlusToken && incrementor.operand.getText() === indexVariableName) { + // ++x + return true; + } + } else if (node.kind === ts.SyntaxKind.BinaryExpression) { + const binaryExpression = node as ts.BinaryExpression; + if (binaryExpression.operatorToken.getText() === "+=" + && binaryExpression.left.getText() === indexVariableName + && binaryExpression.right.getText() === "1") { + // x += 1 + return true; + } + if (binaryExpression.operatorToken.getText() === "=" + && binaryExpression.left.getText() === indexVariableName) { + const addExpression = binaryExpression.right as ts.BinaryExpression; + if (addExpression.operatorToken.getText() === "+") { + if (addExpression.right.getText() === indexVariableName && addExpression.left.getText() === "1") { + // x = 1 + x + return true; + } else if (addExpression.left.getText() === indexVariableName && addExpression.right.getText() === "1") { + // x = x + 1 + return true; } } } - return false; } + return false; } diff --git a/test/rules/prefer-for-of/test.ts.lint b/test/rules/prefer-for-of/test.ts.lint index 37b012daf1c..d7f4f942fd7 100644 --- a/test/rules/prefer-for-of/test.ts.lint +++ b/test/rules/prefer-for-of/test.ts.lint @@ -69,7 +69,7 @@ function sampleFunc() { for (; o < arr.length; o++) { console.log(arr[o]); } - + // Prefix incrementor for(let p = 0; p < arr.length; ++p) { ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ [0] @@ -127,7 +127,7 @@ function sampleFunc() { console.log(q); } } - + // For-of loops ARE allowed for (var r of arr) { console.log(r); From 32633fd64b1b7ba9dcbbedfc1538629adacaf715 Mon Sep 17 00:00:00 2001 From: Andy Hanson Date: Sat, 25 Mar 2017 08:45:28 -0700 Subject: [PATCH 2/2] Clean up code --- src/rules/preferForOfRule.ts | 175 ++++++++++++++++++----------------- 1 file changed, 88 insertions(+), 87 deletions(-) diff --git a/src/rules/preferForOfRule.ts b/src/rules/preferForOfRule.ts index de69bb04060..5c296d28787 100644 --- a/src/rules/preferForOfRule.ts +++ b/src/rules/preferForOfRule.ts @@ -15,6 +15,7 @@ * limitations under the License. */ +import * as utils from "tsutils"; import * as ts from "typescript"; import * as Lint from "../index"; import { isAssignment, unwrapParentheses } from "../language/utils"; @@ -42,7 +43,7 @@ export class Rule extends Lint.Rules.AbstractRule { interface IncrementorState { indexVariableName: string; - arrayToken: ts.Identifier; + arrayExpr: ts.Expression; onlyArrayReadAccess: boolean; } @@ -64,19 +65,15 @@ function walk(ctx: Lint.WalkContext): void { function visitForStatement(node: ts.ForStatement): void { const arrayNodeInfo = getForLoopHeaderInfo(node); - if (!node.incrementor || !arrayNodeInfo) { + if (!arrayNodeInfo) { return ts.forEachChild(node, cb); } - const { indexVariable, arrayToken } = arrayNodeInfo; + const { indexVariable, arrayExpr } = arrayNodeInfo; const indexVariableName = indexVariable.text; // store `for` loop state - const state: IncrementorState = { - indexVariableName, - arrayToken: arrayToken as ts.Identifier, - onlyArrayReadAccess: true, - }; + const state: IncrementorState = { indexVariableName, arrayExpr, onlyArrayReadAccess: true }; scopes.push(state); ts.forEachChild(node.statement, cb); scopes.pop(); @@ -106,107 +103,111 @@ function walk(ctx: Lint.WalkContext): void { function updateIncrementorState(node: ts.Identifier, state: IncrementorState): void { // check if iterator is used for something other than reading data from array - if (node.parent!.kind === ts.SyntaxKind.ElementAccessExpression) { - const elementAccess = node.parent as ts.ElementAccessExpression; - const arrayIdentifier = unwrapParentheses(elementAccess.expression) as ts.Identifier; - if (state.arrayToken.getText() !== arrayIdentifier.getText()) { - // iterator used in array other than one iterated over - state.onlyArrayReadAccess = false; - } else if (elementAccess.parent && isAssignment(elementAccess.parent)) { - // array position is assigned a new value - state.onlyArrayReadAccess = false; - } - } else { + const elementAccess = node.parent!; + if (!utils.isElementAccessExpression(elementAccess)) { state.onlyArrayReadAccess = false; + return; } + const arrayExpr = unwrapParentheses(elementAccess.expression); + if (state.arrayExpr.getText() !== arrayExpr.getText()) { + // iterator used in array other than one iterated over + state.onlyArrayReadAccess = false; + } else if (isAssignment(elementAccess.parent!)) { + // array position is assigned a new value + state.onlyArrayReadAccess = false; + } } // returns the iterator and array of a `for` loop if the `for` loop is basic. -function getForLoopHeaderInfo(forLoop: ts.ForStatement): { indexVariable: ts.Identifier, arrayToken: ts.Expression } | undefined { - let indexVariableName: string | undefined; - let indexVariable: ts.Identifier | undefined; - - // assign `indexVariableName` if initializer is simple and starts at 0 - if (forLoop.initializer && forLoop.initializer.kind === ts.SyntaxKind.VariableDeclarationList) { - const syntaxList = forLoop.initializer.getChildAt(1); - if (syntaxList.kind === ts.SyntaxKind.SyntaxList && syntaxList.getChildCount() === 1) { - const assignment = syntaxList.getChildAt(0); - if (assignment.kind === ts.SyntaxKind.VariableDeclaration && assignment.getChildCount() === 3) { - const value = assignment.getChildAt(2).getText(); - if (value === "0") { - indexVariable = assignment.getChildAt(0) as ts.Identifier; - indexVariableName = indexVariable.getText(); - } - } - } +function getForLoopHeaderInfo(forLoop: ts.ForStatement): { indexVariable: ts.Identifier, arrayExpr: ts.Expression } | undefined { + const { initializer, condition, incrementor } = forLoop; + if (!initializer || !condition || !incrementor) { + return undefined; } - // ensure `for` condition - if (!indexVariableName - || !forLoop.condition - || forLoop.condition.kind !== ts.SyntaxKind.BinaryExpression - || forLoop.condition.getChildAt(0).getText() !== indexVariableName - || forLoop.condition.getChildAt(1).getText() !== "<") { + // Must start with `var i = 0;` or `let i = 0;` + if (!utils.isVariableDeclarationList(initializer) || initializer.declarations.length !== 1) { + return undefined; + } + const { name: indexVariable, initializer: indexInit } = initializer.declarations[0]; + if (indexVariable.kind !== ts.SyntaxKind.Identifier || indexInit === undefined || !isNumber(indexInit, "0")) { + return undefined; + } + // Must end with `i++` + if (!isIncremented(incrementor, indexVariable.text)) { return undefined; } - if (!forLoop.incrementor || !isIncremented(forLoop.incrementor, indexVariableName)) { + // Condition must be `i < arr.length;` + if (!utils.isBinaryExpression(condition)) { return undefined; } - // ensure that the condition checks a `length` property - const conditionRight = forLoop.condition.getChildAt(2); - if (conditionRight.kind === ts.SyntaxKind.PropertyAccessExpression) { - const propertyAccess = conditionRight as ts.PropertyAccessExpression; - if (indexVariable && propertyAccess.name.getText() === "length") { - return { indexVariable: indexVariable!, arrayToken: unwrapParentheses(propertyAccess.expression) }; - } + const { left, operatorToken, right } = condition; + if (!isIdentifierNamed(left, indexVariable.text) || + operatorToken.kind !== ts.SyntaxKind.LessThanToken || + !utils.isPropertyAccessExpression(right)) { + return undefined; + } + + const { expression: arrayExpr, name } = right; + if (name.text !== "length") { + return undefined; } - return undefined; + return { indexVariable, arrayExpr }; } function isIncremented(node: ts.Node, indexVariableName: string): boolean { - if (!node) { - return false; - } - - // ensure variable is incremented - if (node.kind === ts.SyntaxKind.PrefixUnaryExpression) { - const incrementor = node as ts.PrefixUnaryExpression; - if (incrementor.operator === ts.SyntaxKind.PlusPlusToken && incrementor.operand.getText() === indexVariableName) { - // x++ - return true; - } - } else if (node.kind === ts.SyntaxKind.PostfixUnaryExpression) { - const incrementor = node as ts.PostfixUnaryExpression; - if (incrementor.operator === ts.SyntaxKind.PlusPlusToken && incrementor.operand.getText() === indexVariableName) { - // ++x - return true; + switch (node.kind) { + case ts.SyntaxKind.PrefixUnaryExpression: + case ts.SyntaxKind.PostfixUnaryExpression: { + const { operator, operand } = node as ts.PrefixUnaryExpression | ts.PostfixUnaryExpression; + // `++x` or `x++` + return operator === ts.SyntaxKind.PlusPlusToken && isVar(operand); } - } else if (node.kind === ts.SyntaxKind.BinaryExpression) { - const binaryExpression = node as ts.BinaryExpression; - if (binaryExpression.operatorToken.getText() === "+=" - && binaryExpression.left.getText() === indexVariableName - && binaryExpression.right.getText() === "1") { - // x += 1 - return true; - } - if (binaryExpression.operatorToken.getText() === "=" - && binaryExpression.left.getText() === indexVariableName) { - const addExpression = binaryExpression.right as ts.BinaryExpression; - if (addExpression.operatorToken.getText() === "+") { - if (addExpression.right.getText() === indexVariableName && addExpression.left.getText() === "1") { - // x = 1 + x - return true; - } else if (addExpression.left.getText() === indexVariableName && addExpression.right.getText() === "1") { - // x = x + 1 - return true; + + case ts.SyntaxKind.BinaryExpression: + const { operatorToken, left: updatedVar, right: rhs } = node as ts.BinaryExpression; + if (!isVar(updatedVar)) { + return false; + } + + switch (operatorToken.kind) { + case ts.SyntaxKind.PlusEqualsToken: + // x += 1 + return isOne(rhs); + case ts.SyntaxKind.EqualsToken: { + if (!utils.isBinaryExpression(rhs)) { + return false; + } + const { operatorToken: rhsOp, left, right } = rhs; + // `x = 1 + x` or `x = x + 1` + return rhsOp.kind === ts.SyntaxKind.PlusToken && (isVar(left) && isOne(right) || isOne(left) && isVar(right)); } + default: + return false; } - } + + default: + return false; } - return false; + + function isVar(id: ts.Node): boolean { + return isIdentifierNamed(id, indexVariableName); + } +} + +function isIdentifierNamed(node: ts.Node, text: string): boolean { + return utils.isIdentifier(node) && node.text === text; +} + +function isOne(node: ts.Node): boolean { + return isNumber(node, "1"); +} + +function isNumber(node: ts.Node, value: string): boolean { + return utils.isNumericLiteral(node) && node.text === value; }