feat(eslint-plugin): add new rule no-empty-function (#626)

scottohara · bradzacher · commit 747bfcbd54b1 · 2019-06-19T22:24:08.000-07:00
diff --git a/packages/eslint-plugin/README.md b/packages/eslint-plugin/README.md
@@ -142,6 +142,7 @@ Then you should add `airbnb` (or `airbnb-base`) to your `extends` section of `.e
 | [`@typescript-eslint/member-ordering`](./docs/rules/member-ordering.md)                                   | Require a consistent member declaration order                                                                                                       |                    |          |                   |
 | [`@typescript-eslint/no-angle-bracket-type-assertion`](./docs/rules/no-angle-bracket-type-assertion.md)   | Enforces the use of `as Type` assertions instead of `<Type>` assertions                                                                             | :heavy_check_mark: |          |                   |
 | [`@typescript-eslint/no-array-constructor`](./docs/rules/no-array-constructor.md)                         | Disallow generic `Array` constructors                                                                                                               | :heavy_check_mark: | :wrench: |                   |
+| [`@typescript-eslint/no-empty-function`](./docs/rules/no-empty-function.md)                               | Disallow empty functions                                                                                                                            |                    |          |                   |
 | [`@typescript-eslint/no-empty-interface`](./docs/rules/no-empty-interface.md)                             | Disallow the declaration of empty interfaces                                                                                                        | :heavy_check_mark: |          |                   |
 | [`@typescript-eslint/no-explicit-any`](./docs/rules/no-explicit-any.md)                                   | Disallow usage of the `any` type                                                                                                                    | :heavy_check_mark: |          |                   |
 | [`@typescript-eslint/no-extra-parens`](./docs/rules/no-extra-parens.md)                                   | Disallow unnecessary parentheses                                                                                                                    |                    | :wrench: |                   |
diff --git a/packages/eslint-plugin/docs/rules/no-empty-function.md b/packages/eslint-plugin/docs/rules/no-empty-function.md
@@ -0,0 +1,47 @@
+# Disallow Empty Functions (@typescript-eslint/no-empty-function)
+
+Empty functions can reduce readability because readers need to guess whether it’s intentional or not. So writing a clear comment for empty functions is a good practice.
+
+## Rule Details
+
+The `@typescript-eslint/no-empty-function` rule extends the `no-empty-function` rule from ESLint core, and adds support for handling Typescript specific code that would otherwise trigger the rule.
+
+One example of valid Typescript specific code that would otherwise trigger the `no-empty-function` rule is the use of [parameter properties](https://www.typescriptlang.org/docs/handbook/classes.html#parameter-properties) in constructor functions:
+
+```typescript
+class Person {
+  constructor(private firstName: string, private surname: string) {}
+}
+```
+
+The above code is functionally equivalent to:
+
+```typescript
+class Person {
+  private firstName: string;
+  private surname: string;
+
+  constructor(firstName: string, surname: string) {
+    this.firstName = firstName;
+    this.surname = surname;
+  }
+}
+```
+
+Parameter properties enable both the _declaration_ and _initialization_ of member properties in a single location, avoiding the boilerplate & duplication that is common when initializing member properties from parameter values in a constructor function.
+
+In these cases, although the constructor has an empty function body, it is technically valid and should not trigger an error.
+
+See the [ESLint documentation](https://eslint.org/docs/rules/no-empty-function) for more details on the `no-empty-function` rule.
+
+## Rule Changes
+
+```cjson
+{
+    // note you must disable the base rule as it can report incorrect errors
+    "no-empty-function": "off",
+    "@typescript-eslint/no-empty-function": "error"
+}
+```
+
+<sup>Taken with ❤️ [from ESLint core](https://github.com/eslint/eslint/blob/master/docs/rules/no-empty-function.md)</sup>
diff --git a/packages/eslint-plugin/src/rules/index.ts b/packages/eslint-plugin/src/rules/index.ts
@@ -16,6 +16,7 @@ import memberNaming from './member-naming';
 import memberOrdering from './member-ordering';
 import noAngleBracketTypeAssertion from './no-angle-bracket-type-assertion';
 import noArrayConstructor from './no-array-constructor';
+import noEmptyFunction from './no-empty-function';
 import noEmptyInterface from './no-empty-interface';
 import noExplicitAny from './no-explicit-any';
 import noExtraParens from './no-extra-parens';
@@ -73,6 +74,7 @@ export default {
   'member-ordering': memberOrdering,
   'no-angle-bracket-type-assertion': noAngleBracketTypeAssertion,
   'no-array-constructor': noArrayConstructor,
+  'no-empty-function': noEmptyFunction,
   'no-empty-interface': noEmptyInterface,
   'no-explicit-any': noExplicitAny,
   'no-extra-parens': noExtraParens,
diff --git a/packages/eslint-plugin/src/rules/no-empty-function.ts b/packages/eslint-plugin/src/rules/no-empty-function.ts
@@ -0,0 +1,104 @@
+import {
+  TSESTree,
+  AST_NODE_TYPES,
+} from '@typescript-eslint/experimental-utils';
+import baseRule from 'eslint/lib/rules/no-empty-function';
+import * as util from '../util';
+
+type Options = util.InferOptionsTypeFromRule<typeof baseRule>;
+type MessageIds = util.InferMessageIdsTypeFromRule<typeof baseRule>;
+
+export default util.createRule<Options, MessageIds>({
+  name: 'no-empty-function',
+  meta: {
+    type: 'suggestion',
+    docs: {
+      description: 'Disallow empty functions',
+      category: 'Best Practices',
+      recommended: false,
+    },
+    schema: baseRule.meta.schema,
+    messages: baseRule.meta.messages,
+  },
+  defaultOptions: [
+    {
+      allow: [],
+    },
+  ],
+  create(context) {
+    const rules = baseRule.create(context);
+
+    /**
+     * Checks if the node is a constructor
+     * @param node the node to ve validated
+     * @returns true if the node is a constructor
+     * @private
+     */
+    function isConstructor(
+      node: TSESTree.FunctionDeclaration | TSESTree.FunctionExpression,
+    ): boolean {
+      return !!(
+        node.parent &&
+        node.parent.type === 'MethodDefinition' &&
+        node.parent.kind === 'constructor'
+      );
+    }
+
+    /**
+     * Check if the method body is empty
+     * @param node the node to be validated
+     * @returns true if the body is empty
+     * @private
+     */
+    function isBodyEmpty(
+      node: TSESTree.FunctionDeclaration | TSESTree.FunctionExpression,
+    ): boolean {
+      return !node.body || node.body.body.length === 0;
+    }
+
+    /**
+     * Check if method has parameter properties
+     * @param node the node to be validated
+     * @returns true if the body has parameter properties
+     * @private
+     */
+    function hasParameterProperties(
+      node: TSESTree.FunctionDeclaration | TSESTree.FunctionExpression,
+    ): boolean {
+      return (
+        node.params &&
+        node.params.some(
+          param => param.type === AST_NODE_TYPES.TSParameterProperty,
+        )
+      );
+    }
+
+    /**
+     * Checks if the method is a concise constructor (no function body, but has parameter properties)
+     * @param node the node to be validated
+     * @returns true if the method is a concise constructor
+     * @private
+     */
+    function isConciseConstructor(
+      node: TSESTree.FunctionDeclaration | TSESTree.FunctionExpression,
+    ) {
+      // Check TypeScript specific nodes
+      return (
+        isConstructor(node) && isBodyEmpty(node) && hasParameterProperties(node)
+      );
+    }
+
+    return {
+      FunctionDeclaration(node: TSESTree.FunctionDeclaration) {
+        if (!isConciseConstructor(node)) {
+          rules.FunctionDeclaration(node);
+        }
+      },
+      FunctionExpression(node: TSESTree.FunctionExpression) {
+        if (!isConciseConstructor(node)) {
+          rules.FunctionExpression(node);
+        }
+      },
+    };
+  },
+});
diff --git a/packages/eslint-plugin/tests/rules/no-empty-function.test.ts b/packages/eslint-plugin/tests/rules/no-empty-function.test.ts
@@ -0,0 +1,69 @@
+import rule from '../../src/rules/no-empty-function';
+import { RuleTester } from '../RuleTester';
+
+const ruleTester = new RuleTester({
+  parser: '@typescript-eslint/parser',
+});
+
+ruleTester.run('no-empty-function', rule, {
+  valid: [
+    {
+      code: `class Person {
+        private name: string
+        constructor(name: string) {
+          this.name = name;
+        }
+      }`,
+    },
+    {
+      code: `class Person {
+        constructor(private name: string) {}
+      }`,
+    },
+    {
+      code: `class Person {
+        constructor(name: string) {}
+      }`,
+      options: [{ allow: ['constructors'] }],
+    },
+    {
+      code: `class Person {
+        otherMethod(name: string) {}
+      }`,
+      options: [{ allow: ['methods'] }],
+    },
+  ],
+
+  invalid: [
+    {
+      code: `class Person {
+        constructor(name: string) {}
+      }`,
+      errors: [
+        {
+          messageId: 'unexpected',
+          data: {
+            name: 'constructor',
+          },
+          line: 2,
+          column: 35,
+        },
+      ],
+    },
+    {
+      code: `class Person {
+        otherMethod(name: string) {}
+      }`,
+      errors: [
+        {
+          messageId: 'unexpected',
+          data: {
+            name: "method 'otherMethod'",
+          },
+          line: 2,
+          column: 35,
+        },
+      ],
+    },
+  ],
+});
diff --git a/packages/eslint-plugin/typings/eslint-rules.d.ts b/packages/eslint-plugin/typings/eslint-rules.d.ts
@@ -155,6 +155,24 @@ declare module 'eslint/lib/rules/no-dupe-args' {
   export = rule;
 }
 
+declare module 'eslint/lib/rules/no-empty-function' {
+  import { TSESLint, TSESTree } from '@typescript-eslint/experimental-utils';
+
+  const rule: TSESLint.RuleModule<
+    'unexpected',
+    [
+      {
+        allow?: string[];
+      }
+    ],
+    {
+      FunctionDeclaration(node: TSESTree.FunctionDeclaration): void;
+      FunctionExpression(node: TSESTree.FunctionExpression): void;
+    }
+  >;
+  export = rule;
+}
+
 declare module 'eslint/lib/rules/no-implicit-globals' {
   import { TSESLint, TSESTree } from '@typescript-eslint/experimental-utils';
 
