Add noForIn rule

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"] },
@@ -296,11 +296,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 {
     /**
@@ -317,23 +317,28 @@ function resolveConfigurationPath(filePath: string, relativeTo?: string) {
     }
 }
 
+interface KeyValue {
+    [s: string]: any;
+}
+
 export function extendConfigurationFile(
     targetConfig: IConfigurationFile,
     nextConfigSource: IConfigurationFile,
 ): IConfigurationFile {
-    function combineProperties<T>(targetProperty: T | undefined, nextProperty: T | undefined): T {
+    function combineProperties(
+        targetProperty: KeyValue | undefined,
+        nextProperty: KeyValue | undefined,
+    ): KeyValue {
         const combinedProperty: { [key: string]: any } = {};
         add(targetProperty);
         // next config source overwrites the target config object
         add(nextProperty);
-        return combinedProperty as T;
+        return combinedProperty;
 
-        function add(property: T | undefined): void {
+        function add(property: KeyValue | 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 +583,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,58 @@
+/**
+ * @license
+ * Copyright 2013 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:
+            "Recommended the avoidance of 'for-in' statements. They can be replaced by Object.keys in a 'for-of' loop.",
+        optionExamples: [true],
+        options: null,
+        optionsDescription: "Not configurable.",
+        rationale:
+            "A for(... of ...) loop is easier to implement and read when a for(... in ...) loop, as for(... in ...) require a hasOwnProperty check on objects to ensure proper behaviour.",
+        ruleName: "no-for-in",
+        type: "typescript",
+        typescriptOnly: false,
+    };
+
+    public static FAILURE_STRING_FACTORY(initializer: string, expression: string): string {
+        return `Do not use the 'for in' statement: 'for (${initializer} in ${expression})'. If this is an object, use 'Object.keys' instead. If this is an array use a standard 'for' or 'for of' loop instead.`;
+    }
+
+    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 initializer: string = node.initializer.getText();
+            const expression: string = node.expression.getText();
+
+            const msg: string = Rule.FAILURE_STRING_FACTORY(initializer, expression);
+            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,23 @@
+// this should pass
+const columns = [{ data: [1] }];
+for (let i = 0; i < columns[0].data.length; i++) {
+	columns.map((x) => x.data[i]);
+}
+
+// this should NOT pass
+const object = {test:1, test2:1 ,test3:1};
+for (const key in object) {
+~~~~~~~~~~~~~~~~~~~~~~~~~~~
+	if (object.hasOwnProperty(key)) {.
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+		const element = object[key];
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+	}
+~~
+}
+~ [Do not use the 'for in' statement: 'for (const key in object)'. If this is an object, use 'Object.keys' instead. If this is an array use a standard 'for' or 'for of' loop instead.]
+
+// this should pass
+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
+  }
+}