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

Clean up prefer-for-of rule #2348

Merged
merged 2 commits into from
Mar 29, 2017
Merged

Clean up prefer-for-of rule #2348

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f9d7577
Clean up prefer-for-of
andy-hanson Mar 15, 2017
32633fd
Clean up code
andy-hanson Mar 25, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
266 changes: 138 additions & 128 deletions src/rules/preferForOfRule.ts
View file
Open in desktop
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
Open in desktop
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