This repository was archived by the owner on Jan 19, 2019. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 61
[FEAT] [no-for-in-array] Add rule #250
Closed
Closed
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,36 @@ | ||
# Disallow iterating over an array with a for-in loop (no-for-in-array) | ||
|
||
This rule prohibits iterating over an array with a for-in loop. | ||
|
||
## Rule Details | ||
|
||
Rationale from TSLint: | ||
|
||
> A for-in loop (for (var k in o)) iterates over the properties of an Object. | ||
> While it is legal to use for-in loops with array types, it is not common. for-in will iterate over the indices of the array as strings, omitting any “holes” in the array. | ||
> More common is to use for-of, which iterates over the values of an array. If you want to iterate over the indices, alternatives include: | ||
> array.forEach((value, index) => { … }); for (const [index, value] of array.entries()) { … } for (let i = 0; i < array.length; i++) { … } | ||
|
||
Examples of **incorrect** code for this rule: | ||
|
||
```js | ||
for (const x in [3, 4, 5]) { | ||
console.log(x); | ||
} | ||
``` | ||
|
||
Examples of **correct** code for this rule: | ||
|
||
```js | ||
for (const x in { a: 3, b: 4, c: 5 }) { | ||
console.log(x); | ||
} | ||
``` | ||
|
||
## When Not To Use It | ||
|
||
If you want to iterate through a loop using the indices in an array as strings, you can turn off this rule. | ||
|
||
## Related to | ||
|
||
- TSLint: ['no-for-in-array'](https://palantir.github.io/tslint/rules/no-for-in-array/) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,68 @@ | ||
/** | ||
* @fileoverview Disallow iterating over an array with a for-in loop | ||
* @author Benjamin Lichtman | ||
*/ | ||
"use strict"; | ||
const ts = require("typescript"); | ||
const util = require("../util"); | ||
|
||
//------------------------------------------------------------------------------ | ||
// Rule Definition | ||
//------------------------------------------------------------------------------ | ||
|
||
/** | ||
* @type {import("eslint").Rule.RuleModule} | ||
*/ | ||
module.exports = { | ||
meta: { | ||
docs: { | ||
description: "Disallow iterating over an array with a for-in loop", | ||
category: "Functionality", | ||
recommended: false, | ||
extraDescription: [util.tslintRule("no-for-in-array")], | ||
url: util.metaDocsUrl("no-for-in-array"), | ||
}, | ||
fixable: null, | ||
schema: [], | ||
type: "problem", | ||
}, | ||
|
||
create(context) { | ||
return { | ||
ForInStatement(node) { | ||
if ( | ||
!context.parserServices || | ||
!context.parserServices.program | ||
) { | ||
return; | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. TSLint will warn for each rule enabled that requires type information when it's not provided. Should there be some standard way for these to warn in a similar manner? Would that be a separate issue from this PR? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
|
||
/** | ||
* @type {ts.TypeChecker} | ||
*/ | ||
const checker = context.parserServices.program.getTypeChecker(); | ||
const originalNode = context.parserServices.esTreeNodeToTSNodeMap.get( | ||
node | ||
); | ||
|
||
if (!originalNode) { | ||
return; | ||
} | ||
|
||
const type = checker.getTypeAtLocation(originalNode.expression); | ||
|
||
if ( | ||
(typeof type.symbol !== "undefined" && | ||
type.symbol.name === "Array") || | ||
(type.flags & ts.TypeFlags.StringLike) !== 0 | ||
) { | ||
context.report({ | ||
node, | ||
message: | ||
"For-in loops over arrays are forbidden. Use for-of or array.forEach instead.", | ||
}); | ||
} | ||
}, | ||
}; | ||
}, | ||
}; |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Empty file.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,8 @@ | ||
{ | ||
"compilerOptions": { | ||
"target": "es5", | ||
"module": "commonjs", | ||
"strict": true, | ||
"esModuleInterop": true | ||
} | ||
} |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,84 @@ | ||
/** | ||
* @fileoverview Disallow iterating over an array with a for-in loop | ||
* @author Benjamin Lichtman | ||
*/ | ||
"use strict"; | ||
|
||
//------------------------------------------------------------------------------ | ||
// Requirements | ||
//------------------------------------------------------------------------------ | ||
|
||
const rule = require("../../../lib/rules/no-for-in-array"), | ||
RuleTester = require("eslint").RuleTester, | ||
path = require("path"); | ||
|
||
//------------------------------------------------------------------------------ | ||
// Tests | ||
//------------------------------------------------------------------------------ | ||
|
||
// RuleTester.it = function(text, method) { | ||
// return method.call({ break: true }); | ||
// }; | ||
|
||
const rootDir = path.join(process.cwd(), "tests/lib/fixtures"); | ||
const filename = path.join(rootDir, "empty.ts"); | ||
const parserOptions = { | ||
ecmaVersion: 2015, | ||
tsconfigRootDir: rootDir, | ||
project: "./tsconfig.json", | ||
}; | ||
const ruleTester = new RuleTester({ | ||
parserOptions, | ||
parser: "typescript-eslint-parser", | ||
}); | ||
const message = | ||
"For-in loops over arrays are forbidden. Use for-of or array.forEach instead."; | ||
|
||
ruleTester.run("no-for-in-array", rule, { | ||
valid: [ | ||
{ | ||
code: ` | ||
for (const x of [3, 4, 5]) { | ||
console.log(x); | ||
}`, | ||
filename, | ||
}, | ||
{ | ||
filename, | ||
code: ` | ||
for (const x in { a: 1, b: 2, c: 3 }) { | ||
console.log(x); | ||
}`, | ||
}, | ||
], | ||
|
||
invalid: [ | ||
{ | ||
filename, | ||
code: ` | ||
for (const x in [3, 4, 5]) { | ||
console.log(x); | ||
}`, | ||
errors: [ | ||
{ | ||
message, | ||
type: "ForInStatement", | ||
}, | ||
], | ||
}, | ||
{ | ||
filename, | ||
code: ` | ||
const z = [3, 4, 5]; | ||
for (const x in z) { | ||
console.log(x); | ||
}`, | ||
errors: [ | ||
{ | ||
message, | ||
type: "ForInStatement", | ||
}, | ||
], | ||
}, | ||
], | ||
}); |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does
context.parserServices.program
exist whencreate
is first called, as implied by #230? If so, could you...ForInStatement
?checker = context...getTypechecker()
call to outside this returned function?The two would be nice as precedent for other rules that would have multiple methods using the type checker.
Apologies if this is well known; I'm very new to ESLint. 😊
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you should use https://github.com/bradzacher/eslint-plugin-typescript/blob/master/lib/util.js#L114
you need access to
context
and context is parameter of functioncreate(context)