From 5e7c3ffc5f55aefc7d98882e73b72f6424c675bb Mon Sep 17 00:00:00 2001 From: Armano Date: Thu, 31 Jan 2019 02:00:39 +0100 Subject: [PATCH 1/7] feat(eslint-plugin): add eslint rule no-useless-constructor --- packages/eslint-plugin/README.md | 3 +- .../docs/rules/no-useless-constructor.md | 70 +++++++++++++++ .../lib/rules/no-useless-constructor.js | 47 ++++++++++ .../tests/lib/rules/no-useless-constructor.js | 88 +++++++++++++++++++ 4 files changed, 207 insertions(+), 1 deletion(-) create mode 100644 packages/eslint-plugin/docs/rules/no-useless-constructor.md create mode 100644 packages/eslint-plugin/lib/rules/no-useless-constructor.js create mode 100644 packages/eslint-plugin/tests/lib/rules/no-useless-constructor.js diff --git a/packages/eslint-plugin/README.md b/packages/eslint-plugin/README.md index f0c11446353e..1ec6df216048 100644 --- a/packages/eslint-plugin/README.md +++ b/packages/eslint-plugin/README.md @@ -121,9 +121,10 @@ Install [`eslint-config-prettier`](https://github.com/prettier/eslint-config-pre | [`@typescript-eslint/no-this-alias`](./docs/rules/no-this-alias.md) | Disallow aliasing `this` (`no-this-assignment` from TSLint) | | | | [`@typescript-eslint/no-triple-slash-reference`](./docs/rules/no-triple-slash-reference.md) | Disallow `/// ` comments (`no-reference` from TSLint) | :heavy_check_mark: | | | [`@typescript-eslint/no-type-alias`](./docs/rules/no-type-alias.md) | Disallow the use of type aliases (`interface-over-type-literal` from TSLint) | | | -| [`@typescript-eslint/no-unnecessary-type-assertion`](./docs/rules/no-unnecessary-type-assertion.md) | Warns if a type assertion does not change the type of an expression (`no-unnecessary-type-assertion` from TSLint) | | :wrench: | +| [`@typescript-eslint/no-unnecessary-type-assertion`](./docs/rules/no-unnecessary-type-assertion.md) | Warns if a type assertion does not change the type of an expression (`no-unnecessary-type-assertion` from TSLint) | | :wrench: | | [`@typescript-eslint/no-unused-vars`](./docs/rules/no-unused-vars.md) | Disallow unused variables (`no-unused-variable` from TSLint) | :heavy_check_mark: | | | [`@typescript-eslint/no-use-before-define`](./docs/rules/no-use-before-define.md) | Disallow the use of variables before they are defined | :heavy_check_mark: | | +| [`@typescript-eslint/no-useless-constructor`](./docs/rules/no-useless-constructor.md) | Disallow unnecessary constructors | | | | [`@typescript-eslint/no-var-requires`](./docs/rules/no-var-requires.md) | Disallows the use of require statements except in import statements (`no-var-requires` from TSLint) | :heavy_check_mark: | | | [`@typescript-eslint/prefer-interface`](./docs/rules/prefer-interface.md) | Prefer an interface declaration over a type literal (type T = { ... }) (`interface-over-type-literal` from TSLint) | :heavy_check_mark: | :wrench: | | [`@typescript-eslint/prefer-namespace-keyword`](./docs/rules/prefer-namespace-keyword.md) | Require the use of the `namespace` keyword instead of the `module` keyword to declare custom TypeScript modules. (`no-internal-module` from TSLint) | :heavy_check_mark: | :wrench: | diff --git a/packages/eslint-plugin/docs/rules/no-useless-constructor.md b/packages/eslint-plugin/docs/rules/no-useless-constructor.md new file mode 100644 index 000000000000..c9f09bc7b824 --- /dev/null +++ b/packages/eslint-plugin/docs/rules/no-useless-constructor.md @@ -0,0 +1,70 @@ +# Disallow unnecessary constructors (no-useless-constructor) + +ES2015 provides a default class constructor if one is not specified. As such, it is unnecessary to provide an empty constructor or one that simply delegates into its parent class, as in the following examples: + +```js +class A { + constructor() {} +} + +class A extends B { + constructor(value) { + super(value); + } +} +``` + +## Rule Details + +This rule flags class constructors that can be safely removed without changing how the class works. + +## Examples + +Examples of **incorrect** code for this rule: + +```js +/*eslint no-useless-constructor: "error"*/ + +class A { + constructor() {} +} + +class A extends B { + constructor(...args) { + super(...args); + } +} +``` + +Examples of **correct** code for this rule: + +```js +/*eslint no-useless-constructor: "error"*/ + +class A {} + +class A { + constructor() { + doSomething(); + } +} + +class A extends B { + constructor() { + super('foo'); + } +} + +class A extends B { + constructor() { + super(); + doSomething(); + } +} +``` + +## When Not To Use It + +If you don't want to be notified about unnecessary constructors, you can safely disable this rule. + +Taken with ❤️ [from ESLint core](https://github.com/eslint/eslint/blob/master/docs/rules/no-useless-constructor.md) diff --git a/packages/eslint-plugin/lib/rules/no-useless-constructor.js b/packages/eslint-plugin/lib/rules/no-useless-constructor.js new file mode 100644 index 000000000000..4a41a64783f0 --- /dev/null +++ b/packages/eslint-plugin/lib/rules/no-useless-constructor.js @@ -0,0 +1,47 @@ +/** + * @fileoverview Disallow unnecessary constructors + * @author Armano + */ +'use strict'; + +const baseRule = require('eslint/lib/rules/no-useless-constructor'); +const util = require('../util'); + +//------------------------------------------------------------------------------ +// Rule Definition +//------------------------------------------------------------------------------ + +module.exports = Object.assign({}, baseRule, { + meta: { + type: 'problem', + docs: { + description: 'Disallow unnecessary constructors', + category: 'ECMAScript 6', + url: util.metaDocsUrl('no-useless-constructor') + }, + schema: baseRule.meta.schema, + messages: baseRule.meta.messages + }, + + create(context) { + const rules = baseRule.create(context); + + //---------------------------------------------------------------------- + // Public + //---------------------------------------------------------------------- + return { + MethodDefinition(node) { + if (!node.value || node.value.type !== 'FunctionExpression') { + return; + } + if ( + node.value.params && + node.value.params.some(param => param.type === 'TSParameterProperty') + ) { + return; + } + rules.MethodDefinition(node); + } + }; + } +}); diff --git a/packages/eslint-plugin/tests/lib/rules/no-useless-constructor.js b/packages/eslint-plugin/tests/lib/rules/no-useless-constructor.js new file mode 100644 index 000000000000..84de4d1b002c --- /dev/null +++ b/packages/eslint-plugin/tests/lib/rules/no-useless-constructor.js @@ -0,0 +1,88 @@ +'use strict'; + +//------------------------------------------------------------------------------ +// Requirements +//------------------------------------------------------------------------------ + +const rule = require('../../../lib/rules/no-useless-constructor'), + RuleTester = require('eslint').RuleTester; + +const ruleTester = new RuleTester({ + parserOptions: { + ecmaVersion: 6, + sourceType: 'module' + }, + parser: '@typescript-eslint/parser' +}); + +const error = { message: 'Useless constructor.', type: 'MethodDefinition' }; + +ruleTester.run('no-useless-constructor', rule, { + valid: [ + 'class A { }', + 'class A { constructor(){ doSomething(); } }', + 'class A extends B { constructor(){} }', + "class A extends B { constructor(){ super('foo'); } }", + 'class A extends B { constructor(foo, bar){ super(foo, bar, 1); } }', + 'class A extends B { constructor(){ super(); doSomething(); } }', + 'class A extends B { constructor(...args){ super(...args); doSomething(); } }', + 'class A { dummyMethod(){ doSomething(); } }', + 'class A extends B.C { constructor() { super(foo); } }', + 'class A extends B.C { constructor([a, b, c]) { super(...arguments); } }', + 'class A extends B.C { constructor(a = f()) { super(...arguments); } }', + 'class A extends B { constructor(a, b, c) { super(a, b); } }', + 'class A extends B { constructor(foo, bar){ super(foo); } }', + 'class A extends B { constructor(test) { super(); } }', + 'class A extends B { constructor() { foo; } }', + 'class A extends B { constructor(foo, bar) { super(bar); } }', + // https://github.com/typescript-eslint/typescript-eslint/issues/15 + 'declare class Foo { constructor(); }', + 'class Foo { constructor(); }', + 'abstract class Foo { constructor(); }', + 'abstract class Foo { abstract constructor(); }', + // https://github.com/typescript-eslint/typescript-eslint/issues/ + 'class Foo { constructor(private name: string) {} }', + 'class Foo { constructor(public name: string) {} }', + 'class Foo { constructor(protected name: string) {} }' + ], + invalid: [ + { + code: 'class A { constructor(){} }', + errors: [error] + }, + { + code: "class A { 'constructor'(){} }", + errors: [error] + }, + { + code: 'class A extends B { constructor() { super(); } }', + errors: [error] + }, + { + code: 'class A extends B { constructor(foo){ super(foo); } }', + errors: [error] + }, + { + code: 'class A extends B { constructor(foo, bar){ super(foo, bar); } }', + errors: [error] + }, + { + code: 'class A extends B { constructor(...args){ super(...args); } }', + errors: [error] + }, + { + code: 'class A extends B.C { constructor() { super(...arguments); } }', + errors: [error] + }, + { + code: + 'class A extends B { constructor(a, b, ...c) { super(...arguments); } }', + errors: [error] + }, + { + code: + 'class A extends B { constructor(a, b, ...c) { super(a, b, ...c); } }', + errors: [error] + } + ] +}); From eeb1dfe2158e8e89f2879e94fa9721f08ec85abc Mon Sep 17 00:00:00 2001 From: Armano Date: Thu, 31 Jan 2019 02:06:29 +0100 Subject: [PATCH 2/7] docs: correct issue id --- .../eslint-plugin/tests/lib/rules/no-useless-constructor.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/eslint-plugin/tests/lib/rules/no-useless-constructor.js b/packages/eslint-plugin/tests/lib/rules/no-useless-constructor.js index 84de4d1b002c..c3dd64063f36 100644 --- a/packages/eslint-plugin/tests/lib/rules/no-useless-constructor.js +++ b/packages/eslint-plugin/tests/lib/rules/no-useless-constructor.js @@ -40,7 +40,7 @@ ruleTester.run('no-useless-constructor', rule, { 'class Foo { constructor(); }', 'abstract class Foo { constructor(); }', 'abstract class Foo { abstract constructor(); }', - // https://github.com/typescript-eslint/typescript-eslint/issues/ + // https://github.com/typescript-eslint/typescript-eslint/issues/48 'class Foo { constructor(private name: string) {} }', 'class Foo { constructor(public name: string) {} }', 'class Foo { constructor(protected name: string) {} }' From f362eccccc69ea714e3c9c64949e6601ef4b43a5 Mon Sep 17 00:00:00 2001 From: Armano Date: Thu, 31 Jan 2019 17:00:31 +0100 Subject: [PATCH 3/7] fix(eslint-plugin): ignore private and protected constructors --- .../lib/rules/no-useless-constructor.js | 7 ++++++- .../tests/lib/rules/no-useless-constructor.js | 16 +++++++++++++++- 2 files changed, 21 insertions(+), 2 deletions(-) diff --git a/packages/eslint-plugin/lib/rules/no-useless-constructor.js b/packages/eslint-plugin/lib/rules/no-useless-constructor.js index 4a41a64783f0..1c943f5d73a9 100644 --- a/packages/eslint-plugin/lib/rules/no-useless-constructor.js +++ b/packages/eslint-plugin/lib/rules/no-useless-constructor.js @@ -31,7 +31,12 @@ module.exports = Object.assign({}, baseRule, { //---------------------------------------------------------------------- return { MethodDefinition(node) { - if (!node.value || node.value.type !== 'FunctionExpression') { + if ( + !node.value || + node.value.type !== 'FunctionExpression' || + node.accessibility === 'private' || + node.accessibility === 'protected' + ) { return; } if ( diff --git a/packages/eslint-plugin/tests/lib/rules/no-useless-constructor.js b/packages/eslint-plugin/tests/lib/rules/no-useless-constructor.js index c3dd64063f36..d1a9269049d7 100644 --- a/packages/eslint-plugin/tests/lib/rules/no-useless-constructor.js +++ b/packages/eslint-plugin/tests/lib/rules/no-useless-constructor.js @@ -43,7 +43,10 @@ ruleTester.run('no-useless-constructor', rule, { // https://github.com/typescript-eslint/typescript-eslint/issues/48 'class Foo { constructor(private name: string) {} }', 'class Foo { constructor(public name: string) {} }', - 'class Foo { constructor(protected name: string) {} }' + 'class Foo { constructor(protected name: string) {} }', + // https://github.com/typescript-eslint/typescript-eslint/pull/167#discussion_r252638401 + 'class Foo { private constructor() {} }', + 'class Foo { protected constructor() {} }' ], invalid: [ { @@ -83,6 +86,17 @@ ruleTester.run('no-useless-constructor', rule, { code: 'class A extends B { constructor(a, b, ...c) { super(a, b, ...c); } }', errors: [error] + }, + // https://github.com/typescript-eslint/typescript-eslint/pull/167#discussion_r252638401 + { + code: + 'class A extends B { public constructor() {} }', + errors: [error] + }, + { + code: + 'class A extends B { public constructor(foo){ super(foo); }', + errors: [error] } ] }); From 643dfd632e70852d4a52616bf5db3e5a1e79f252 Mon Sep 17 00:00:00 2001 From: Armano Date: Thu, 31 Jan 2019 18:01:38 +0100 Subject: [PATCH 4/7] fix(eslint-plugin): add additonal test cases and fix formatting --- .../lib/rules/no-useless-constructor.js | 3 +-- .../tests/lib/rules/no-useless-constructor.js | 18 ++++++------------ 2 files changed, 7 insertions(+), 14 deletions(-) diff --git a/packages/eslint-plugin/lib/rules/no-useless-constructor.js b/packages/eslint-plugin/lib/rules/no-useless-constructor.js index 1c943f5d73a9..8c98443f4567 100644 --- a/packages/eslint-plugin/lib/rules/no-useless-constructor.js +++ b/packages/eslint-plugin/lib/rules/no-useless-constructor.js @@ -34,8 +34,7 @@ module.exports = Object.assign({}, baseRule, { if ( !node.value || node.value.type !== 'FunctionExpression' || - node.accessibility === 'private' || - node.accessibility === 'protected' + node.accessibility ) { return; } diff --git a/packages/eslint-plugin/tests/lib/rules/no-useless-constructor.js b/packages/eslint-plugin/tests/lib/rules/no-useless-constructor.js index d1a9269049d7..3b207cb9c3b8 100644 --- a/packages/eslint-plugin/tests/lib/rules/no-useless-constructor.js +++ b/packages/eslint-plugin/tests/lib/rules/no-useless-constructor.js @@ -45,8 +45,13 @@ ruleTester.run('no-useless-constructor', rule, { 'class Foo { constructor(public name: string) {} }', 'class Foo { constructor(protected name: string) {} }', // https://github.com/typescript-eslint/typescript-eslint/pull/167#discussion_r252638401 + 'class Foo { public constructor() {} }', 'class Foo { private constructor() {} }', - 'class Foo { protected constructor() {} }' + 'class Foo { protected constructor() {} }', + 'class A extends B { public constructor() {} }', + 'class A extends B { protected constructor(foo, bar) { super(bar); } }', + 'class A extends B { private constructor(foo, bar) { super(bar); } }', + 'class A extends B { public constructor(foo){ super(foo); }' ], invalid: [ { @@ -86,17 +91,6 @@ ruleTester.run('no-useless-constructor', rule, { code: 'class A extends B { constructor(a, b, ...c) { super(a, b, ...c); } }', errors: [error] - }, - // https://github.com/typescript-eslint/typescript-eslint/pull/167#discussion_r252638401 - { - code: - 'class A extends B { public constructor() {} }', - errors: [error] - }, - { - code: - 'class A extends B { public constructor(foo){ super(foo); }', - errors: [error] } ] }); From 841171cf2c156798c392cbb20a8db7478a4a94d1 Mon Sep 17 00:00:00 2001 From: Armano Date: Thu, 31 Jan 2019 18:35:47 +0100 Subject: [PATCH 5/7] test(eslint-plugin): fix issue in test case --- .../eslint-plugin/tests/lib/rules/no-useless-constructor.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/eslint-plugin/tests/lib/rules/no-useless-constructor.js b/packages/eslint-plugin/tests/lib/rules/no-useless-constructor.js index 3b207cb9c3b8..d817d12ce8bd 100644 --- a/packages/eslint-plugin/tests/lib/rules/no-useless-constructor.js +++ b/packages/eslint-plugin/tests/lib/rules/no-useless-constructor.js @@ -51,7 +51,7 @@ ruleTester.run('no-useless-constructor', rule, { 'class A extends B { public constructor() {} }', 'class A extends B { protected constructor(foo, bar) { super(bar); } }', 'class A extends B { private constructor(foo, bar) { super(bar); } }', - 'class A extends B { public constructor(foo){ super(foo); }' + 'class A extends B { public constructor(foo){ super(foo); } }' ], invalid: [ { From bafe080e6cbf351d4f2678ed5d5a679f1326ebf2 Mon Sep 17 00:00:00 2001 From: Armano Date: Thu, 31 Jan 2019 18:41:29 +0100 Subject: [PATCH 6/7] docs(eslint-plugin): add example of valid ts code --- .../eslint-plugin/docs/rules/no-useless-constructor.md | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/packages/eslint-plugin/docs/rules/no-useless-constructor.md b/packages/eslint-plugin/docs/rules/no-useless-constructor.md index c9f09bc7b824..0e136d122ac1 100644 --- a/packages/eslint-plugin/docs/rules/no-useless-constructor.md +++ b/packages/eslint-plugin/docs/rules/no-useless-constructor.md @@ -61,6 +61,14 @@ class A extends B { doSomething(); } } + +class A extends B { + constructor(protected name: string) {} +} + +class A extends B { + protected constructor() {} +} ``` ## When Not To Use It From a06d950cd6d5295365da06ce7387f6bd4aa6e867 Mon Sep 17 00:00:00 2001 From: Armano Date: Thu, 31 Jan 2019 19:59:37 +0100 Subject: [PATCH 7/7] fix(eslint-plugin): improve detection of useless constructors --- .../lib/rules/no-useless-constructor.js | 51 +++++++++++++++---- .../tests/lib/rules/no-useless-constructor.js | 26 ++++++---- 2 files changed, 55 insertions(+), 22 deletions(-) diff --git a/packages/eslint-plugin/lib/rules/no-useless-constructor.js b/packages/eslint-plugin/lib/rules/no-useless-constructor.js index 8c98443f4567..c30cdcf872bf 100644 --- a/packages/eslint-plugin/lib/rules/no-useless-constructor.js +++ b/packages/eslint-plugin/lib/rules/no-useless-constructor.js @@ -11,6 +11,41 @@ const util = require('../util'); // Rule Definition //------------------------------------------------------------------------------ +/** + * Check if method with accessibility is not useless + * @param {MethodDefinition} node + * @returns {boolean} + */ +function checkAccessibility(node) { + switch (node.accessibility) { + case 'protected': + case 'private': + return false; + case 'public': + if ( + node.parent.type === 'ClassBody' && + node.parent.parent && + node.parent.parent.superClass + ) { + return false; + } + break; + } + return true; +} + +/** + * Check if method is not unless due to typescript parameter properties + * @param {MethodDefinition} node + * @returns {boolean} + */ +function checkParams(node) { + return ( + !node.value.params || + !node.value.params.some(param => param.type === 'TSParameterProperty') + ); +} + module.exports = Object.assign({}, baseRule, { meta: { type: 'problem', @@ -32,19 +67,13 @@ module.exports = Object.assign({}, baseRule, { return { MethodDefinition(node) { if ( - !node.value || - node.value.type !== 'FunctionExpression' || - node.accessibility - ) { - return; - } - if ( - node.value.params && - node.value.params.some(param => param.type === 'TSParameterProperty') + node.value && + node.value.type === 'FunctionExpression' && + checkAccessibility(node) && + checkParams(node) ) { - return; + rules.MethodDefinition(node); } - rules.MethodDefinition(node); } }; } diff --git a/packages/eslint-plugin/tests/lib/rules/no-useless-constructor.js b/packages/eslint-plugin/tests/lib/rules/no-useless-constructor.js index d817d12ce8bd..c73c5034f121 100644 --- a/packages/eslint-plugin/tests/lib/rules/no-useless-constructor.js +++ b/packages/eslint-plugin/tests/lib/rules/no-useless-constructor.js @@ -36,22 +36,22 @@ ruleTester.run('no-useless-constructor', rule, { 'class A extends B { constructor() { foo; } }', 'class A extends B { constructor(foo, bar) { super(bar); } }', // https://github.com/typescript-eslint/typescript-eslint/issues/15 - 'declare class Foo { constructor(); }', - 'class Foo { constructor(); }', - 'abstract class Foo { constructor(); }', - 'abstract class Foo { abstract constructor(); }', + 'declare class A { constructor(); }', + 'class A { constructor(); }', + 'abstract class A { constructor(); }', + 'abstract class A { abstract constructor(); }', // https://github.com/typescript-eslint/typescript-eslint/issues/48 - 'class Foo { constructor(private name: string) {} }', - 'class Foo { constructor(public name: string) {} }', - 'class Foo { constructor(protected name: string) {} }', + 'class A { constructor(private name: string) {} }', + 'class A { constructor(public name: string) {} }', + 'class A { constructor(protected name: string) {} }', // https://github.com/typescript-eslint/typescript-eslint/pull/167#discussion_r252638401 - 'class Foo { public constructor() {} }', - 'class Foo { private constructor() {} }', - 'class Foo { protected constructor() {} }', + 'class A { private constructor() {} }', + 'class A { protected constructor() {} }', 'class A extends B { public constructor() {} }', 'class A extends B { protected constructor(foo, bar) { super(bar); } }', 'class A extends B { private constructor(foo, bar) { super(bar); } }', - 'class A extends B { public constructor(foo){ super(foo); } }' + 'class A extends B { public constructor(foo){ super(foo); } }', + 'class A extends B { public constructor(foo){} }' ], invalid: [ { @@ -91,6 +91,10 @@ ruleTester.run('no-useless-constructor', rule, { code: 'class A extends B { constructor(a, b, ...c) { super(a, b, ...c); } }', errors: [error] + }, + { + code: 'class A { public constructor() {} }', + errors: [error] } ] });