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 1 commit
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
263 changes: 136 additions & 127 deletions src/rules/preferForOfRule.ts
View file
Open in desktop
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,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

arrayToken is not guaranteed to be 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)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

elementAccess.parent is never undefined, so you can simply assert it

// 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) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

if (forLoop.initializer && isVariableDeclarationList(forLoop.initializer) && forLoop.initializer.declarations.length === 1) {
    const declaration = forLoop.initializer.declarations[0];
    if (declaration.name.kind === ts.SyntaxKind.Identifier &&
        declaration.initializer !== undefined && isNumericLiteral(declaration.initializer) && declaration.initializer.text === "0") {
        indexVariable = declaration.name;
        indexVariableName = indexVariable.text;
    }
}

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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

!isBinaryExpression(forLoop.condition) ||
!isVariable(forLoop.condition.left, indexVariableName) || // see my other comments regarding indexVariableName
forLoop.condition.operatorToken.kind !== ts.SyntaxKind.LessThanToken ||
// condition from line 159
!isPropertyAccessExpression(forLoop.condition.right) ||
forLoop.condition.right.name.text !== "length"

|| 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) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use typeguards?

const incrementor = node as ts.PrefixUnaryExpression;
if (incrementor.operator === ts.SyntaxKind.PlusPlusToken && incrementor.operand.getText() === indexVariableName) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you could replace incrementor.operand.getText() === indexVariableName with isIdentifier(incrementor.operand) && incrementor.operand.text === indexVariableName

maybe make this a function because you also need it below

// 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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could be joined with the if block above as they do exactly the same

if (incrementor.operator === ts.SyntaxKind.PlusPlusToken && incrementor.operand.getText() === indexVariableName) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just return, no need for an if

// ++x
return true;
}
} else if (node.kind === ts.SyntaxKind.BinaryExpression) {
const binaryExpression = node as ts.BinaryExpression;
if (binaryExpression.operatorToken.getText() === "+="
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

operatorToken.kind === ts.SyntaxKind.PlusEqualsToken
same below with EqualsToken and PlusToken

&& binaryExpression.left.getText() === indexVariableName
&& binaryExpression.right.getText() === "1") {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isNumericLiteral(binaryExpression.right) && binaryExpression.right.text === "1"

make this a function a reuse it for the 2 conditions below, too

// 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
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