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
Browse files Browse the repository at this point in the history 
Don't unnecessarily use a BlockScopeAwareRuleWalker, just store a stack of for-loop scopes
  • Loading branch information
@andy-hanson
andy-hanson committed Mar 15, 2017
1 parent 2dad217 commit 42a503c
Show file tree
Hide file tree
Showing 2 changed files with 138 additions and 129 deletions.
263 changes: 136 additions & 127 deletions src/rules/preferForOfRule.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<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 (!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 = getScopeForVariable(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 getScopeForVariable(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, incrementorState: 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 (incrementorState.arrayToken.getText() !== arrayIdentifier.getText()) {
// iterator used in array other than one iterated over
incrementorState.onlyArrayReadAccess = false;
} else if (elementAccess.parent && isAssignment(elementAccess.parent)) {
// array position is assigned a new value
incrementorState.onlyArrayReadAccess = false;
}
} else {
incrementorState.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;
}
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 42a503c

Please sign in to comment.