Add no-for-in rule (#4747)

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}'.`);

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;

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]),
+        );
     }
 };
 

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

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

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

test/rules/no-for-in/tslint.json

@@ -0,0 +1,5 @@
+{
+  "rules": {
+    "no-for-in": true
+  }
+}