From f31dd9407d642d3a9999ac78a79855c293fb771c Mon Sep 17 00:00:00 2001
From: Josh Pike <josh@streaka.com>
Date: Thu, 4 Jul 2019 13:07:56 +0800
Subject: [PATCH] Add no-for-in rule (#4747)

---
 src/configs/all.ts                      |  8 +---
 src/configuration.ts                    | 19 ++++-----
 src/rules/completed-docs/exclusions.ts  | 13 +++---
 src/rules/noForInRule.ts                | 55 +++++++++++++++++++++++++
 src/rules/noImplicitDependenciesRule.ts |  6 +--
 test/rules/no-for-in/test.ts.lint       | 20 +++++++++
 test/rules/no-for-in/tslint.json        |  5 +++
 7 files changed, 98 insertions(+), 28 deletions(-)
 create mode 100644 src/rules/noForInRule.ts
 create mode 100644 test/rules/no-for-in/test.ts.lint
 create mode 100644 test/rules/no-for-in/tslint.json

diff --git a/src/configs/all.ts b/src/configs/all.ts
index 35631ae4e10..8c3a6ff35e5 100644
--- a/src/configs/all.ts
+++ b/src/configs/all.ts
@@ -18,7 +18,6 @@
 import { join as joinPaths } from "path";
 
 import { findRule } from "../ruleLoader";
-import { hasOwnProperty } from "../utils";
 
 // tslint:disable object-literal-sort-keys
 // tslint:disable object-literal-key-quotes
@@ -51,6 +50,7 @@ export const rules = {
     },
     "no-any": true,
     "no-empty-interface": true,
+    "no-for-in": true,
     "no-import-side-effect": true,
     // Technically this is not the strictest setting, but don't want to conflict with "typedef"
     "no-inferrable-types": { options: ["ignore-params"] },
@@ -298,11 +298,7 @@ export const RULES_EXCLUDED_FROM_ALL_CONFIG = [
 
 // Exclude typescript-only rules from jsRules, otherwise it's identical.
 export const jsRules: { [key: string]: any } = {};
-for (const key in rules) {
-    if (!hasOwnProperty(rules, key)) {
-        continue;
-    }
-
+for (const key of Object.keys(rules)) {
     const Rule = findRule(key, joinPaths(__dirname, "..", "rules"));
     if (Rule === undefined) {
         throw new Error(`Couldn't find rule '${key}'.`);
diff --git a/src/configuration.ts b/src/configuration.ts
index baf9d59bf0c..d5e3a60c873 100644
--- a/src/configuration.ts
+++ b/src/configuration.ts
@@ -24,7 +24,7 @@ import * as path from "path";
 import { FatalError, showWarningOnce } from "./error";
 import { IOptions, RuleSeverity } from "./language/rule/rule";
 import { findRule } from "./ruleLoader";
-import { arrayify, hasOwnProperty, stripComments, tryResolvePackage } from "./utils";
+import { arrayify, stripComments, tryResolvePackage } from "./utils";
 
 export interface IConfigurationFile {
     /**
@@ -321,7 +321,10 @@ export function extendConfigurationFile(
     targetConfig: IConfigurationFile,
     nextConfigSource: IConfigurationFile,
 ): IConfigurationFile {
-    function combineProperties<T>(targetProperty: T | undefined, nextProperty: T | undefined): T {
+    function combineProperties<T extends { [key: string]: any }>(
+        targetProperty: T | undefined,
+        nextProperty: T | undefined,
+    ): T {
         const combinedProperty: { [key: string]: any } = {};
         add(targetProperty);
         // next config source overwrites the target config object
@@ -330,10 +333,8 @@ export function extendConfigurationFile(
 
         function add(property: T | undefined): void {
             if (property !== undefined) {
-                for (const name in property) {
-                    if (hasOwnProperty(property, name)) {
-                        combinedProperty[name] = property[name];
-                    }
+                for (const name of Object.keys(property)) {
+                    combinedProperty[name] = property[name];
                 }
             }
         }
@@ -578,10 +579,8 @@ export function parseConfigFile(
     function parseRules(config: RawRulesConfig | undefined): Map<string, Partial<IOptions>> {
         const map = new Map<string, Partial<IOptions>>();
         if (config !== undefined) {
-            for (const ruleName in config) {
-                if (hasOwnProperty(config, ruleName)) {
-                    map.set(ruleName, parseRuleOptions(config[ruleName], defaultSeverity));
-                }
+            for (const ruleName of Object.keys(config)) {
+                map.set(ruleName, parseRuleOptions(config[ruleName], defaultSeverity));
             }
         }
         return map;
diff --git a/src/rules/completed-docs/exclusions.ts b/src/rules/completed-docs/exclusions.ts
index f575fa2df9c..e6746628174 100644
--- a/src/rules/completed-docs/exclusions.ts
+++ b/src/rules/completed-docs/exclusions.ts
@@ -15,7 +15,6 @@
  * limitations under the License.
  */
 
-import { hasOwnProperty } from "../../utils";
 import { DESCRIPTOR_OVERLOADS, DocType } from "../completedDocsRule";
 
 import { BlockExclusion, IBlockExclusionDescriptor } from "./blockExclusion";
@@ -49,13 +48,11 @@ const addRequirements = (exclusionsMap: ExclusionsMap, descriptors: IInputExclus
         return;
     }
 
-    for (const docType in descriptors) {
-        if (hasOwnProperty(descriptors, docType)) {
-            exclusionsMap.set(
-                docType as DocType,
-                createRequirementsForDocType(docType as DocType, descriptors[docType]),
-            );
-        }
+    for (const docType of Object.keys(descriptors)) {
+        exclusionsMap.set(
+            docType as DocType,
+            createRequirementsForDocType(docType as DocType, descriptors[docType]),
+        );
     }
 };
 
diff --git a/src/rules/noForInRule.ts b/src/rules/noForInRule.ts
new file mode 100644
index 00000000000..4a485c45b55
--- /dev/null
+++ b/src/rules/noForInRule.ts
@@ -0,0 +1,55 @@
+/**
+ * @license
+ * Copyright 2019 Palantir Technologies, Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+import { isForInStatement } from "tsutils";
+import * as ts from "typescript";
+
+import * as Lint from "../index";
+
+export class Rule extends Lint.Rules.AbstractRule {
+    public static metadata: Lint.IRuleMetadata = {
+        description: "Ban the usage of for...in statements.",
+        optionExamples: [true],
+        options: null,
+        optionsDescription: "Not configurable.",
+        rationale:
+            "for...in statements are legacy JavaScript syntax which usually require a verbose `hasOwnProperty` check inside their loop body. These statements can be fully replaced with for...of statements in modern JS & TS.",
+        ruleName: "no-for-in",
+        type: "typescript",
+        typescriptOnly: false,
+    };
+
+    public static FAILURE_STRING_FACTORY(): string {
+        return `Use a for...of statement instead of for...in. If iterating over an object, use Object.keys() to access its enumerable keys.`;
+    }
+
+    public apply(sourceFile: ts.SourceFile): Lint.RuleFailure[] {
+        return this.applyWithFunction(sourceFile, walk);
+    }
+}
+
+function walk(ctx: Lint.WalkContext) {
+    function cb(node: ts.Node): void {
+        if (isForInStatement(node)) {
+            const msg: string = Rule.FAILURE_STRING_FACTORY();
+            ctx.addFailureAt(node.getStart(), node.getWidth(), msg);
+        }
+        return ts.forEachChild(node, cb);
+    }
+
+    return ts.forEachChild(ctx.sourceFile, cb);
+}
diff --git a/src/rules/noImplicitDependenciesRule.ts b/src/rules/noImplicitDependenciesRule.ts
index 681f910365f..1a124dddf17 100644
--- a/src/rules/noImplicitDependenciesRule.ts
+++ b/src/rules/noImplicitDependenciesRule.ts
@@ -165,10 +165,8 @@ function getDependencies(fileName: string, options: Options): Set<string> {
 }
 
 function addDependencies(result: Set<string>, dependencies: Dependencies) {
-    for (const name in dependencies) {
-        if (dependencies.hasOwnProperty(name)) {
-            result.add(name);
-        }
+    for (const name of Object.keys(dependencies)) {
+        result.add(name);
     }
 }
 
diff --git a/test/rules/no-for-in/test.ts.lint b/test/rules/no-for-in/test.ts.lint
new file mode 100644
index 00000000000..a3dd806a466
--- /dev/null
+++ b/test/rules/no-for-in/test.ts.lint
@@ -0,0 +1,20 @@
+const columns = [{ data: [1] }];
+for (let i = 0; i < columns[0].data.length; i++) {
+	columns.map((x) => x.data[i]);
+}
+
+const object = { test: 1, test2: 1, test3: 1 };
+for (const key in object) {
+~~~~~~~~~~~~~~~~~~~~~~~~~~~
+	if (object.hasOwnProperty(key)) {
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+		const element = object[key];
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+	}
+~~
+}
+~ [Use a for...of statement instead of for...in. If iterating over an object, use Object.keys() to access its enumerable keys.]
+
+for (const key of Object.keys(object)) {
+	const value = object[key];
+}
diff --git a/test/rules/no-for-in/tslint.json b/test/rules/no-for-in/tslint.json
new file mode 100644
index 00000000000..2a62503d3e3
--- /dev/null
+++ b/test/rules/no-for-in/tslint.json
@@ -0,0 +1,5 @@
+{
+  "rules": {
+    "no-for-in": true
+  }
+}