Skip to content
This repository has been archived by the owner on Mar 25, 2021. It is now read-only.

Commit

Permalink
Clean up prefer-for-of rule implementation (#2348)
Browse files Browse the repository at this point in the history
  • Loading branch information
@andy-hanson @adidahiya
andy-hanson authored and adidahiya committed Mar 29, 2017
1 parent faec4ec commit d030e8f
Show file tree
Hide file tree
Showing 2 changed files with 140 additions and 130 deletions.
266 changes: 138 additions & 128 deletions src/rules/preferForOfRule.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand All @@ -36,168 +37,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 {
arrayToken: ts.Identifier;
forLoopEndPosition: number;
interface IncrementorState {
indexVariableName: string;
arrayExpr: ts.Expression;
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<string, IIncrementorState>;
function walk(ctx: Lint.WalkContext<void>): void {
const scopes: IncrementorState[] = [];

class PreferForOfWalker extends Lint.BlockScopeAwareRuleWalker<void, IncrementorMap> {
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 (!arrayNodeInfo) {
return ts.forEachChild(node, cb);
}

super.visitForStatement(node);
const { indexVariable, arrayExpr } = arrayNodeInfo;
const indexVariableName = indexVariable.text;

if (indexVariableName != null) {
const incrementorState = currentBlockScope.get(indexVariableName)!;
if (incrementorState.onlyArrayReadAccess) {
this.addFailureFromStartToEnd(node.getStart(), incrementorState.forLoopEndPosition, Rule.FAILURE_STRING);
}
// store `for` loop state
const state: IncrementorState = { indexVariableName, arrayExpr, onlyArrayReadAccess: true };
scopes.push(state);
ts.forEachChild(node.statement, cb);
scopes.pop();

// remove current `for` loop state
currentBlockScope.delete(indexVariableName);
if (state.onlyArrayReadAccess) {
ctx.addFailure(node.getStart(), node.statement.getFullStart(), Rule.FAILURE_STRING);
}
}

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;
}
}
super.visitIdentifier(node);
function visitIdentifier(node: ts.Identifier): void {
const state = getStateForVariable(node.text);
if (state) {
updateIncrementorState(node, state);
}
}

// 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 getStateForVariable(name: string): IncrementorState | undefined {
for (let i = scopes.length - 1; i >= 0; i--) {
const scope = scopes[i];
if (scope.indexVariableName === name) {
return scope;
}
}
return undefined;
}
}

// 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() !== "<") {
function updateIncrementorState(node: ts.Identifier, state: IncrementorState): void {
// check if iterator is used for something other than reading data from array
const elementAccess = node.parent!;
if (!utils.isElementAccessExpression(elementAccess)) {
state.onlyArrayReadAccess = false;
return;
}

return null;
}
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;
}
}

if (forLoop.incrementor == null || !this.isIncremented(forLoop.incrementor, indexVariableName)) {
return null;
}
// returns the iterator and array of a `for` loop if the `for` loop is basic.
function getForLoopHeaderInfo(forLoop: ts.ForStatement): { indexVariable: ts.Identifier, arrayExpr: ts.Expression } | undefined {
const { initializer, condition, incrementor } = forLoop;
if (!initializer || !condition || !incrementor) {
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) };
}
}
// 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;
}

return null;
// Must end with `i++`
if (!isIncremented(incrementor, indexVariable.text)) {
return undefined;
}

private isIncremented(node: ts.Node, indexVariableName: string) {
if (node == null) {
return false;
// Condition must be `i < arr.length;`
if (!utils.isBinaryExpression(condition)) {
return undefined;
}

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 { indexVariable, arrayExpr };
}

function isIncremented(node: ts.Node, indexVariableName: string): boolean {
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);
}

// 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;
case ts.SyntaxKind.BinaryExpression:
const { operatorToken, left: updatedVar, right: rhs } = node as ts.BinaryExpression;
if (!isVar(updatedVar)) {
return false;
}
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;

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;
}
}
return false;

default:
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;
}
4 changes: 2 additions & 2 deletions test/rules/prefer-for-of/test.ts.lint
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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]
Expand Down Expand Up @@ -127,7 +127,7 @@ function sampleFunc() {
console.log(q);
}
}

// For-of loops ARE allowed
for (var r of arr) {
console.log(r);
Expand Down

0 comments on commit d030e8f

Please sign in to comment.