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

Conversation

andy-hanson
Copy link
Contributor

PR checklist

  • Addresses an existing issue: #0000
  • New feature, bugfix, or enhancement
    • Includes tests
  • Documentation update

Overview of change:

Don't unnecessarily use a BlockScopeAwareRuleWalker, just store a stack of for-loop scopes.

@andy-hanson andy-hanson force-pushed the prefer-for-of branch 2 times, most recently from 42a503c to 7a0b9b7 Compare March 15, 2017 03:33
Don't unnecessarily use a BlockScopeAwareRuleWalker, just store a stack of for-loop scopes
Copy link
Contributor

@ajafff ajafff left a comment

Choose a reason for hiding this comment

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

don't know how to say this in a nice way ...
this code still looks pretty bad

I left some comments on the really bad parts. I will review again after these changes

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

}
} else if (node.kind === ts.SyntaxKind.PostfixUnaryExpression) {
const incrementor = node as ts.PostfixUnaryExpression;
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

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

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?

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

const binaryExpression = node as ts.BinaryExpression;
if (binaryExpression.operatorToken.getText() === "+="
&& 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

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

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

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

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

@ajafff
Copy link
Contributor

ajafff commented Mar 28, 2017

AFAICT the rule will bail out if you use an identifier with the same name as the index variable somewhere inside the loop body. That's no blocker for now as the old implementation behaved the same I guess.

for (let i = 0; i < arr.length; ++i) {
    {
        let i; // i is not used in element access, the rule bails out
    }
    class Foo {
        private i; // same as above
        constructor(i: number) {} // same as above
    }
    let foo: { i: number } = { i: 1 }; // same as above for both identifiers
}

Copy link
Contributor

@ajafff ajafff left a comment

Choose a reason for hiding this comment

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

code looks much better now, thanks

@adidahiya adidahiya changed the title Clean up prefer-for-of Clean up prefer-for-of rule Mar 29, 2017
@adidahiya adidahiya merged commit d030e8f into palantir:master Mar 29, 2017
@andy-hanson andy-hanson deleted the prefer-for-of branch March 30, 2017 00:12
@ajafff ajafff mentioned this pull request Jul 2, 2017
4 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants