From 9d3aeb7852edc8a61953a568b3ae6769073492fd Mon Sep 17 00:00:00 2001 From: yassin-kammoun-sonarsource Date: Thu, 1 Feb 2024 15:14:03 +0100 Subject: [PATCH 1/4] Ignore decorated classes --- packages/jsts/src/rules/S6647/cb.fixture.ts | 5 +++++ packages/jsts/src/rules/S6647/rule.ts | 12 +++++++++++- 2 files changed, 16 insertions(+), 1 deletion(-) diff --git a/packages/jsts/src/rules/S6647/cb.fixture.ts b/packages/jsts/src/rules/S6647/cb.fixture.ts index 9f2e97b1d69..698a761b2f1 100644 --- a/packages/jsts/src/rules/S6647/cb.fixture.ts +++ b/packages/jsts/src/rules/S6647/cb.fixture.ts @@ -19,3 +19,8 @@ class SubClass extends SuperClass { super(); } } + +@Decorator() +class Decorated { + constructor() {} +} diff --git a/packages/jsts/src/rules/S6647/rule.ts b/packages/jsts/src/rules/S6647/rule.ts index 442891ab528..e6f1fc1e8f4 100644 --- a/packages/jsts/src/rules/S6647/rule.ts +++ b/packages/jsts/src/rules/S6647/rule.ts @@ -52,6 +52,15 @@ function checkParams(node: TSESTree.MethodDefinition): boolean { ); } +/** + * Check if the enclosing class is not decorated. + */ +function checkDecorator(node: TSESTree.MethodDefinition): boolean { + return !( + node.parent.parent?.type === 'ClassDeclaration' && node.parent.parent.decorators?.length > 0 + ); +} + const eslintNoUselessConstructor = eslintRules['no-useless-constructor']; const originalRule: Rule.RuleModule = { @@ -68,7 +77,8 @@ const originalRule: Rule.RuleModule = { node.value.type === 'FunctionExpression' && node.kind === 'constructor' && checkAccessibility(node as TSESTree.MethodDefinition) && - checkParams(node as TSESTree.MethodDefinition) + checkParams(node as TSESTree.MethodDefinition) && + checkDecorator(node as TSESTree.MethodDefinition) ) { rules.MethodDefinition!(node); } From ce886e71ebf2a597ccd6f605601c82ba29af34f6 Mon Sep 17 00:00:00 2001 From: yassin-kammoun-sonarsource Date: Thu, 1 Feb 2024 15:31:42 +0100 Subject: [PATCH 2/4] Ignore classes inheriting protected constructors --- packages/jsts/src/rules/S6647/cb.fixture.ts | 10 ++++++ packages/jsts/src/rules/S6647/rule.ts | 34 ++++++++++++++++++++- 2 files changed, 43 insertions(+), 1 deletion(-) diff --git a/packages/jsts/src/rules/S6647/cb.fixture.ts b/packages/jsts/src/rules/S6647/cb.fixture.ts index 698a761b2f1..0b9cc009d9e 100644 --- a/packages/jsts/src/rules/S6647/cb.fixture.ts +++ b/packages/jsts/src/rules/S6647/cb.fixture.ts @@ -24,3 +24,13 @@ class SubClass extends SuperClass { class Decorated { constructor() {} } + +class Alpha { + protected constructor() {} +} + +class Beta extends Alpha { + constructor() { + super(); + } +} diff --git a/packages/jsts/src/rules/S6647/rule.ts b/packages/jsts/src/rules/S6647/rule.ts index e6f1fc1e8f4..df1c4ea9992 100644 --- a/packages/jsts/src/rules/S6647/rule.ts +++ b/packages/jsts/src/rules/S6647/rule.ts @@ -21,6 +21,7 @@ import type { TSESTree } from '@typescript-eslint/utils'; import { Rule } from 'eslint'; import { eslintRules } from '../core'; import { decorate } from './decorator'; +import { getVariableFromName } from '../helpers'; /** * Check if method with accessibility is not useless @@ -61,6 +62,36 @@ function checkDecorator(node: TSESTree.MethodDefinition): boolean { ); } +/** + * Check if the enclosing class does not inherit a protected constructor. + */ +function checkInheritance(node: TSESTree.MethodDefinition, context: Rule.RuleContext): boolean { + if ( + node.parent.type === 'ClassBody' && + 'superClass' in node.parent.parent && + node.parent.parent.superClass + ) { + const superClass = node.parent.parent.superClass as TSESTree.Identifier; + const variable = getVariableFromName(context, superClass.name); + for (const def of variable?.defs ?? []) { + if (def.node.type === 'ClassDeclaration') { + const decl = def.node as TSESTree.ClassDeclaration; + if ( + decl.body.body.some( + member => + member.type === 'MethodDefinition' && + member.kind === 'constructor' && + member.accessibility === 'protected', + ) + ) { + return false; + } + } + } + } + return true; +} + const eslintNoUselessConstructor = eslintRules['no-useless-constructor']; const originalRule: Rule.RuleModule = { @@ -78,7 +109,8 @@ const originalRule: Rule.RuleModule = { node.kind === 'constructor' && checkAccessibility(node as TSESTree.MethodDefinition) && checkParams(node as TSESTree.MethodDefinition) && - checkDecorator(node as TSESTree.MethodDefinition) + checkDecorator(node as TSESTree.MethodDefinition) && + checkInheritance(node as TSESTree.MethodDefinition, context) ) { rules.MethodDefinition!(node); } From 4bf04bde458ad667fcffda712aebdaba9b2c4f88 Mon Sep 17 00:00:00 2001 From: yassin-kammoun-sonarsource Date: Mon, 5 Feb 2024 12:07:16 +0100 Subject: [PATCH 3/4] Ignore classes inheriting from imported symbols --- packages/jsts/src/rules/S6647/cb.fixture.ts | 8 ++++++++ packages/jsts/src/rules/S6647/rule.ts | 3 +++ 2 files changed, 11 insertions(+) diff --git a/packages/jsts/src/rules/S6647/cb.fixture.ts b/packages/jsts/src/rules/S6647/cb.fixture.ts index 0b9cc009d9e..aa1808697a4 100644 --- a/packages/jsts/src/rules/S6647/cb.fixture.ts +++ b/packages/jsts/src/rules/S6647/cb.fixture.ts @@ -1,3 +1,5 @@ +import { Lambda } from 'lambda'; + class Foo { private constructor() { // this is ok @@ -34,3 +36,9 @@ class Beta extends Alpha { super(); } } + +class Gamma extends Lambda { + constructor() { + super(); + } +} diff --git a/packages/jsts/src/rules/S6647/rule.ts b/packages/jsts/src/rules/S6647/rule.ts index df1c4ea9992..499c09e32c8 100644 --- a/packages/jsts/src/rules/S6647/rule.ts +++ b/packages/jsts/src/rules/S6647/rule.ts @@ -74,6 +74,9 @@ function checkInheritance(node: TSESTree.MethodDefinition, context: Rule.RuleCon const superClass = node.parent.parent.superClass as TSESTree.Identifier; const variable = getVariableFromName(context, superClass.name); for (const def of variable?.defs ?? []) { + if (def.type === 'ImportBinding') { + return false; + } if (def.node.type === 'ClassDeclaration') { const decl = def.node as TSESTree.ClassDeclaration; if ( From 2a51cb11fde40231588cb68979e7bc54ec1d4be2 Mon Sep 17 00:00:00 2001 From: yassin-kammoun-sonarsource Date: Thu, 1 Feb 2024 15:42:36 +0100 Subject: [PATCH 4/4] Update ruling expectations --- .../jsts/ag-grid/typescript-S6647.json | 11 ------- .../jsts/fireface/typescript-S6647.json | 3 -- .../jsts/ionic2-auth/typescript-S6647.json | 8 ----- .../expected/jsts/rxjs/typescript-S6647.json | 32 ------------------- 4 files changed, 54 deletions(-) delete mode 100644 its/ruling/src/test/expected/jsts/ag-grid/typescript-S6647.json delete mode 100644 its/ruling/src/test/expected/jsts/ionic2-auth/typescript-S6647.json delete mode 100644 its/ruling/src/test/expected/jsts/rxjs/typescript-S6647.json diff --git a/its/ruling/src/test/expected/jsts/ag-grid/typescript-S6647.json b/its/ruling/src/test/expected/jsts/ag-grid/typescript-S6647.json deleted file mode 100644 index 67b2ac74f04..00000000000 --- a/its/ruling/src/test/expected/jsts/ag-grid/typescript-S6647.json +++ /dev/null @@ -1,11 +0,0 @@ -{ -"ag-grid:src/ts/rowModels/infinite/infiniteCache.ts": [ -20 -], -"ag-grid:src/ts/rowModels/pagination/paginationComp.ts": [ -35 -], -"ag-grid:src/ts/widgets/agCheckbox.ts": [ -34 -] -} diff --git a/its/ruling/src/test/expected/jsts/fireface/typescript-S6647.json b/its/ruling/src/test/expected/jsts/fireface/typescript-S6647.json index 4c15c9e8350..1fe18ffc757 100644 --- a/its/ruling/src/test/expected/jsts/fireface/typescript-S6647.json +++ b/its/ruling/src/test/expected/jsts/fireface/typescript-S6647.json @@ -1,7 +1,4 @@ { -"fireface:src/app-header/app-header.component.ts": [ -15 -], "fireface:src/common/avatar.model.ts": [ 38 ] diff --git a/its/ruling/src/test/expected/jsts/ionic2-auth/typescript-S6647.json b/its/ruling/src/test/expected/jsts/ionic2-auth/typescript-S6647.json deleted file mode 100644 index 25b74353c9f..00000000000 --- a/its/ruling/src/test/expected/jsts/ionic2-auth/typescript-S6647.json +++ /dev/null @@ -1,8 +0,0 @@ -{ -"ionic2-auth:app/pages/tabs/tabs.ts": [ -18 -], -"ionic2-auth:app/services/auth/auth.ts": [ -6 -] -} diff --git a/its/ruling/src/test/expected/jsts/rxjs/typescript-S6647.json b/its/ruling/src/test/expected/jsts/rxjs/typescript-S6647.json deleted file mode 100644 index 028801556c6..00000000000 --- a/its/ruling/src/test/expected/jsts/rxjs/typescript-S6647.json +++ /dev/null @@ -1,32 +0,0 @@ -{ -"rxjs:src/Subject.ts": [ -38 -], -"rxjs:src/observable/NeverObservable.ts": [ -46 -], -"rxjs:src/operator/dematerialize.ts": [ -62 -], -"rxjs:src/operator/exhaust.ts": [ -62 -], -"rxjs:src/operator/isEmpty.ts": [ -30 -], -"rxjs:src/operator/materialize.ts": [ -66 -], -"rxjs:src/operator/pairwise.ts": [ -59 -], -"rxjs:src/operator/race.ts": [ -77 -], -"rxjs:src/operator/switch.ts": [ -71 -], -"rxjs:src/operator/toArray.ts": [ -29 -] -}