From b6c8b0c6b19c7c9bb0230d5171c69e1a2546eb8c Mon Sep 17 00:00:00 2001 From: Tanmoy Bhowmik Date: Tue, 10 Sep 2019 05:30:59 +0530 Subject: [PATCH] [new-rule-option] check-super-calls option for unnecessary-constructor rule (#4813) --- src/rules/unnecessaryConstructorRule.ts | 95 ++++++++- .../check-super-calls/test.ts.fix | 147 ++++++++++++++ .../check-super-calls/test.ts.lint | 188 ++++++++++++++++++ .../check-super-calls/tslint.json | 8 + .../default/test.ts.fix | 159 +++++++++++++++ .../{ => default}/test.ts.lint | 90 +++++++++ .../{ => default}/tslint.json | 0 .../rules/unnecessary-constructor/test.ts.fix | 69 ------- 8 files changed, 678 insertions(+), 78 deletions(-) create mode 100644 test/rules/unnecessary-constructor/check-super-calls/test.ts.fix create mode 100644 test/rules/unnecessary-constructor/check-super-calls/test.ts.lint create mode 100644 test/rules/unnecessary-constructor/check-super-calls/tslint.json create mode 100644 test/rules/unnecessary-constructor/default/test.ts.fix rename test/rules/unnecessary-constructor/{ => default}/test.ts.lint (50%) rename test/rules/unnecessary-constructor/{ => default}/tslint.json (100%) delete mode 100644 test/rules/unnecessary-constructor/test.ts.fix diff --git a/src/rules/unnecessaryConstructorRule.ts b/src/rules/unnecessaryConstructorRule.ts index 616db5e3870..930523b4dec 100644 --- a/src/rules/unnecessaryConstructorRule.ts +++ b/src/rules/unnecessaryConstructorRule.ts @@ -15,18 +15,36 @@ * limitations under the License. */ -import { isConstructorDeclaration, isParameterProperty } from "tsutils"; +import { + isCallExpression, + isConstructorDeclaration, + isExpressionStatement, + isParameterProperty, +} from "tsutils"; import * as ts from "typescript"; import * as Lint from "../index"; import { Replacement } from "../language/rule/rule"; +interface Options { + checkSuperCall: boolean; +} + +const OPTION_CHECK_SUPER_CALL = "check-super-calls"; + export class Rule extends Lint.Rules.AbstractRule { public static metadata: Lint.IRuleMetadata = { description: "Prevents blank constructors, as they are redundant.", - optionExamples: [true], - options: null, - optionsDescription: "Not configurable.", + optionExamples: [true, [true, { [OPTION_CHECK_SUPER_CALL]: true }]], + options: { + properties: { + [OPTION_CHECK_SUPER_CALL]: { type: "boolean" }, + }, + type: "object", + }, + optionsDescription: Lint.Utils.dedent` + An optional object with the property '${OPTION_CHECK_SUPER_CALL}'. + This is to check for unnecessary constructor parameters for super call`, rationale: Lint.Utils.dedent` JavaScript implicitly adds a blank constructor when there isn't one. It's not necessary to manually add one in. @@ -39,12 +57,71 @@ export class Rule extends Lint.Rules.AbstractRule { public static FAILURE_STRING = "Remove unnecessary empty constructor."; public apply(sourceFile: ts.SourceFile): Lint.RuleFailure[] { - return this.applyWithFunction(sourceFile, walk); + const options: Options = { + checkSuperCall: + this.ruleArguments.length !== 0 && + (this.ruleArguments[0] as { "check-super-calls"?: boolean })[ + "check-super-calls" + ] === true, + }; + + return this.applyWithFunction(sourceFile, walk, options); } } -const isEmptyConstructor = (node: ts.ConstructorDeclaration): boolean => - node.body !== undefined && node.body.statements.length === 0; +const containsSuper = ( + statement: ts.Statement, + constructorParameters: ts.NodeArray, +): boolean => { + if ( + isExpressionStatement(statement) && + isCallExpression(statement.expression) && + ts.SyntaxKind.SuperKeyword === statement.expression.expression.kind + ) { + const superArguments = statement.expression.arguments; + + if (superArguments.length < constructorParameters.length) { + return true; + } + + if (superArguments.length === constructorParameters.length) { + if (constructorParameters.length === 0) { + return true; + } + + for (const constructorParameter of constructorParameters) { + for (const superArgument of superArguments) { + if (constructorParameter.name.kind !== superArgument.kind) { + return false; + } + } + } + + return true; + } + } + + return false; +}; + +const isEmptyOrContainsOnlySuper = (node: ts.ConstructorDeclaration, options: Options): boolean => { + if (node.body !== undefined) { + const { checkSuperCall } = options; + + if (node.body.statements.length === 0) { + return true; + } + + if (checkSuperCall) { + return ( + node.body.statements.length === 1 && + containsSuper(node.body.statements[0], node.parameters) + ); + } + } + + return false; +}; const containsConstructorParameter = (node: ts.ConstructorDeclaration): boolean => // If this has any parameter properties @@ -59,11 +136,11 @@ const isAccessRestrictingConstructor = (node: ts.ConstructorDeclaration): boolea const containsDecorator = (node: ts.ConstructorDeclaration): boolean => node.parameters.some(p => p.decorators !== undefined); -function walk(context: Lint.WalkContext) { +function walk(context: Lint.WalkContext) { const callback = (node: ts.Node): void => { if ( isConstructorDeclaration(node) && - isEmptyConstructor(node) && + isEmptyOrContainsOnlySuper(node, context.options) && !containsConstructorParameter(node) && !containsDecorator(node) && !isAccessRestrictingConstructor(node) diff --git a/test/rules/unnecessary-constructor/check-super-calls/test.ts.fix b/test/rules/unnecessary-constructor/check-super-calls/test.ts.fix new file mode 100644 index 00000000000..a5c97d4ba14 --- /dev/null +++ b/test/rules/unnecessary-constructor/check-super-calls/test.ts.fix @@ -0,0 +1,147 @@ +export class ExportedClass { +} + +class PublicConstructor { +} + +class ProtectedConstructor { + protected constructor() { } +} + +class PrivateConstructor { + private constructor() { } +} + +class SameLine { } + +class WithPrecedingMember { + public member: string; +} + +class WithFollowingMethod { + public method() {} +} + +const classExpression = class { +} + +class ContainsContents { + constructor() { + console.log("Hello!"); + } +} + +class CallsSuper extends PublicConstructor { +} + +class ContainsParameter { +} + +class PrivateContainsParameter { + private constructor(x: number) { } +} + +class ProtectedContainsParameter { + protected constructor(x: number) { } +} + +class ContainsParameterDeclaration { + constructor(public x: number) { } +} + +class ContainsParameterAndParameterDeclaration { + constructor(x: number, public y: number) { } +} + +class ConstructorOverload { +} + +interface IConstructorSignature { + new(): {}; +} + +class DecoratedParameters { + constructor(@Optional x: number) {} +} + +class X { + constructor(private param1: number, param2: number) {} +} + +export class Y extends X { + constructor(param1: number) { + super(param1, 2); + } +} + +class Base { + constructor(public param: number) {} +} + +class Super extends Base { + public param: number; + constructor(param1: number) { + super(param1); + this.param = param1; + } +} + +class Super extends Base { +} + +class Super extends Base { + constructor(param1: number, public param2: number) { + super(param1); + } +} + +class Super extends Base { +} + +class Super extends Base { + constructor(param1: number) { + super(param1 + 1); + } +} + +class Super extends Base { + constructor() { + super(1); + } +} + +class Super extends Base { + constructor(param1: number) { + super(1); + } +} + +const test = (param: number) => number; + +class Super extends Base { + constructor(param1: number) { + super(test(param1)); + } +} + +class Base2 { + constructor(public param1: number, param2: number) {} +} + +class Super extends Base2 { + constructor(param1: number, param2: number) { + super(test(param2), param1); + } +} + +class Super extends Base2 { +} + +class Super extends Base { + public param1: number; + constructor(public param2: number) { + super(param2); + this.param1 = param2; + } +} + diff --git a/test/rules/unnecessary-constructor/check-super-calls/test.ts.lint b/test/rules/unnecessary-constructor/check-super-calls/test.ts.lint new file mode 100644 index 00000000000..fe419035572 --- /dev/null +++ b/test/rules/unnecessary-constructor/check-super-calls/test.ts.lint @@ -0,0 +1,188 @@ +export class ExportedClass { + constructor() { } + ~~~~~~~~~~~~~~~~~ [0] +} + +class PublicConstructor { + public constructor() { } + ~~~~~~~~~~~~~~~~~~~~~~~~ [0] +} + +class ProtectedConstructor { + protected constructor() { } +} + +class PrivateConstructor { + private constructor() { } +} + +class SameLine { constructor() { } } + ~~~~~~~~~~~~~~~~~ [0] + +class WithPrecedingMember { + public member: string; + constructor() {} + ~~~~~~~~~~~~~~~~ [0] +} + +class WithFollowingMethod { + constructor() {} + ~~~~~~~~~~~~~~~~ [0] + public method() {} +} + +const classExpression = class { + constructor() { } + ~~~~~~~~~~~~~~~~~ [0] +} + +class ContainsContents { + constructor() { + console.log("Hello!"); + } +} + +class CallsSuper extends PublicConstructor { + constructor() { + ~~~~~~~~~~~~~~~ + super(); +~~~~~~~~~~~~~~~~ + } +~~~~~ [0] +} + +class ContainsParameter { + constructor(x: number) { } + ~~~~~~~~~~~~~~~~~~~~~~~~~~ [0] +} + +class PrivateContainsParameter { + private constructor(x: number) { } +} + +class ProtectedContainsParameter { + protected constructor(x: number) { } +} + +class ContainsParameterDeclaration { + constructor(public x: number) { } +} + +class ContainsParameterAndParameterDeclaration { + constructor(x: number, public y: number) { } +} + +class ConstructorOverload { + constructor(x: number); + constructor(x: number, y?: number) { } + ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ [0] +} + +interface IConstructorSignature { + new(): {}; +} + +class DecoratedParameters { + constructor(@Optional x: number) {} +} + +class X { + constructor(private param1: number, param2: number) {} +} + +export class Y extends X { + constructor(param1: number) { + super(param1, 2); + } +} + +class Base { + constructor(public param: number) {} +} + +class Super extends Base { + public param: number; + constructor(param1: number) { + super(param1); + this.param = param1; + } +} + +class Super extends Base { + constructor(param: number) { + ~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + super(param); +~~~~~~~~~~~~~~~~~~~~~ + } +~~~~~ [0] +} + +class Super extends Base { + constructor(param1: number, public param2: number) { + super(param1); + } +} + +class Super extends Base { + constructor(param1: number, param2: number) { + ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + super(param1); +~~~~~~~~~~~~~~~~~~~~~~ + } +~~~~~ [0] +} + +class Super extends Base { + constructor(param1: number) { + super(param1 + 1); + } +} + +class Super extends Base { + constructor() { + super(1); + } +} + +class Super extends Base { + constructor(param1: number) { + super(1); + } +} + +const test = (param: number) => number; + +class Super extends Base { + constructor(param1: number) { + super(test(param1)); + } +} + +class Base2 { + constructor(public param1: number, param2: number) {} +} + +class Super extends Base2 { + constructor(param1: number, param2: number) { + super(test(param2), param1); + } +} + +class Super extends Base2 { + constructor(param1: number, param2: number) { + ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + super(param2, param1); +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + } +~~~~~ [0] +} + +class Super extends Base { + public param1: number; + constructor(public param2: number) { + super(param2); + this.param1 = param2; + } +} + +[0]: Remove unnecessary empty constructor. diff --git a/test/rules/unnecessary-constructor/check-super-calls/tslint.json b/test/rules/unnecessary-constructor/check-super-calls/tslint.json new file mode 100644 index 00000000000..387e6c2b68e --- /dev/null +++ b/test/rules/unnecessary-constructor/check-super-calls/tslint.json @@ -0,0 +1,8 @@ +{ + "rules": { + "unnecessary-constructor": [ + true, + { "check-super-calls": true } + ] + } +} diff --git a/test/rules/unnecessary-constructor/default/test.ts.fix b/test/rules/unnecessary-constructor/default/test.ts.fix new file mode 100644 index 00000000000..a274aa9f2f7 --- /dev/null +++ b/test/rules/unnecessary-constructor/default/test.ts.fix @@ -0,0 +1,159 @@ +export class ExportedClass { +} + +class PublicConstructor { +} + +class ProtectedConstructor { + protected constructor() { } +} + +class PrivateConstructor { + private constructor() { } +} + +class SameLine { } + +class WithPrecedingMember { + public member: string; +} + +class WithFollowingMethod { + public method() {} +} + +const classExpression = class { +} + +class ContainsContents { + constructor() { + console.log("Hello!"); + } +} + +class CallsSuper extends PublicConstructor { + constructor() { + super(); + } +} + +class ContainsParameter { +} + +class PrivateContainsParameter { + private constructor(x: number) { } +} + +class ProtectedContainsParameter { + protected constructor(x: number) { } +} + +class ContainsParameterDeclaration { + constructor(public x: number) { } +} + +class ContainsParameterAndParameterDeclaration { + constructor(x: number, public y: number) { } +} + +class ConstructorOverload { +} + +interface IConstructorSignature { + new(): {}; +} + +class DecoratedParameters { + constructor(@Optional x: number) {} +} + +class X { + constructor(private param1: number, param2: number) {} +} + +export class Y extends X { + constructor(param1: number) { + super(param1, 2); + } +} + +class Base { + constructor(public param: number) {} +} + +class Super extends Base { + public param: number; + constructor(param1: number) { + super(param1); + this.param = param1; + } +} + +class Super extends Base { + constructor(param: number) { + super(param); + } +} + +class Super extends Base { + constructor(param1: number, public param2: number) { + super(param1); + } +} + +class Super extends Base { + constructor(param1: number, param2: number) { + super(param1); + } +} + +class Super extends Base { + constructor(param1: number) { + super(param1 + 1); + } +} + +class Super extends Base { + constructor() { + super(1); + } +} + +class Super extends Base { + constructor(param1: number) { + super(1); + } +} + +const test = (param: number) => number; + +class Super extends Base { + constructor(param1: number) { + super(test(param1)); + } +} + +class Base2 { + constructor(public param1: number, param2: number) {} +} + +class Super extends Base2 { + constructor(param1: number, param2: number) { + super(test(param2), param1); + } +} + +class Super extends Base2 { + constructor(param1: number, param2: number) { + super(param2, param1); + } +} + +class Super extends Base { + public param1: number; + constructor(public param2: number) { + super(param2); + this.param1 = param2; + } +} + diff --git a/test/rules/unnecessary-constructor/test.ts.lint b/test/rules/unnecessary-constructor/default/test.ts.lint similarity index 50% rename from test/rules/unnecessary-constructor/test.ts.lint rename to test/rules/unnecessary-constructor/default/test.ts.lint index 86febfa49a7..0203b8cf994 100644 --- a/test/rules/unnecessary-constructor/test.ts.lint +++ b/test/rules/unnecessary-constructor/default/test.ts.lint @@ -83,4 +83,94 @@ class DecoratedParameters { constructor(@Optional x: number) {} } +class X { + constructor(private param1: number, param2: number) {} +} + +export class Y extends X { + constructor(param1: number) { + super(param1, 2); + } +} + +class Base { + constructor(public param: number) {} +} + +class Super extends Base { + public param: number; + constructor(param1: number) { + super(param1); + this.param = param1; + } +} + +class Super extends Base { + constructor(param: number) { + super(param); + } +} + +class Super extends Base { + constructor(param1: number, public param2: number) { + super(param1); + } +} + +class Super extends Base { + constructor(param1: number, param2: number) { + super(param1); + } +} + +class Super extends Base { + constructor(param1: number) { + super(param1 + 1); + } +} + +class Super extends Base { + constructor() { + super(1); + } +} + +class Super extends Base { + constructor(param1: number) { + super(1); + } +} + +const test = (param: number) => number; + +class Super extends Base { + constructor(param1: number) { + super(test(param1)); + } +} + +class Base2 { + constructor(public param1: number, param2: number) {} +} + +class Super extends Base2 { + constructor(param1: number, param2: number) { + super(test(param2), param1); + } +} + +class Super extends Base2 { + constructor(param1: number, param2: number) { + super(param2, param1); + } +} + +class Super extends Base { + public param1: number; + constructor(public param2: number) { + super(param2); + this.param1 = param2; + } +} + [0]: Remove unnecessary empty constructor. diff --git a/test/rules/unnecessary-constructor/tslint.json b/test/rules/unnecessary-constructor/default/tslint.json similarity index 100% rename from test/rules/unnecessary-constructor/tslint.json rename to test/rules/unnecessary-constructor/default/tslint.json diff --git a/test/rules/unnecessary-constructor/test.ts.fix b/test/rules/unnecessary-constructor/test.ts.fix deleted file mode 100644 index 24e5163d8ba..00000000000 --- a/test/rules/unnecessary-constructor/test.ts.fix +++ /dev/null @@ -1,69 +0,0 @@ -export class ExportedClass { -} - -class PublicConstructor { -} - -class ProtectedConstructor { - protected constructor() { } -} - -class PrivateConstructor { - private constructor() { } -} - -class SameLine { } - -class WithPrecedingMember { - public member: string; -} - -class WithFollowingMethod { - public method() {} -} - -const classExpression = class { -} - -class ContainsContents { - constructor() { - console.log("Hello!"); - } -} - -class CallsSuper extends PublicConstructor { - constructor() { - super(); - } -} - -class ContainsParameter { -} - -class PrivateContainsParameter { - private constructor(x: number) { } -} - -class ProtectedContainsParameter { - protected constructor(x: number) { } -} - -class ContainsParameterDeclaration { - constructor(public x: number) { } -} - -class ContainsParameterAndParameterDeclaration { - constructor(x: number, public y: number) { } -} - -class ConstructorOverload { -} - -interface IConstructorSignature { - new(): {}; -} - -class DecoratedParameters { - constructor(@Optional x: number) {} -} -