From 1417919d6078b5ebaaf9b9669534a5fa73ceaaca Mon Sep 17 00:00:00 2001 From: yeonjuan Date: Mon, 17 Oct 2022 23:42:30 +0900 Subject: [PATCH 1/4] feat(eslint-plugin): add no-unsafe-declaration-merging --- .../rules/no-unsafe-declaration-merging.md | 54 ++++++++++++++ packages/eslint-plugin/src/configs/all.ts | 1 + packages/eslint-plugin/src/rules/index.ts | 2 + .../rules/no-unsafe-declaration-merging.ts | 56 +++++++++++++++ .../no-unsafe-declaration-merging.test.ts | 71 +++++++++++++++++++ 5 files changed, 184 insertions(+) create mode 100644 packages/eslint-plugin/docs/rules/no-unsafe-declaration-merging.md create mode 100644 packages/eslint-plugin/src/rules/no-unsafe-declaration-merging.ts create mode 100644 packages/eslint-plugin/tests/rules/no-unsafe-declaration-merging.test.ts diff --git a/packages/eslint-plugin/docs/rules/no-unsafe-declaration-merging.md b/packages/eslint-plugin/docs/rules/no-unsafe-declaration-merging.md new file mode 100644 index 00000000000..f974329a716 --- /dev/null +++ b/packages/eslint-plugin/docs/rules/no-unsafe-declaration-merging.md @@ -0,0 +1,54 @@ +--- +description: 'Disallow unsafe declaration merging.' +--- + +> 🛑 This file is source code, not the primary documentation location! 🛑 +> +> See **https://typescript-eslint.io/rules/no-unsafe-declaration-merging** for documentation. + +TypeScript's "declaration merging" supports merging separate declarations with the same name. We can merges + +Declaration merging between classes and interfaces is unsafe. +TypeScript compiler doesn't check whether properties are initialized or not, possibly making runtime errors. + +```ts +interface Foo { + nums: number[]; +} + +class Foo {} + +const foo = new Foo(); + +foo.nums.push(1); // Runtime Error: Cannot read properties of undefined. +``` + +## Examples + + + +### ❌ Incorrect + +```ts +interface Foo {} + +class Foo {} +``` + +### ✅ Correct + +```ts +interface Foo {} +class Bar extends Foo {} + +namespace Baz {} +namespace Baz {} +enum Baz {} + +namespace Qux {} +function Qux() {} +``` + +## Further Reading + +- [Declaration Merging](https://www.typescriptlang.org/docs/handbook/declaration-merging.html) diff --git a/packages/eslint-plugin/src/configs/all.ts b/packages/eslint-plugin/src/configs/all.ts index 8742d36b208..1f6530ead3e 100644 --- a/packages/eslint-plugin/src/configs/all.ts +++ b/packages/eslint-plugin/src/configs/all.ts @@ -108,6 +108,7 @@ export = { '@typescript-eslint/no-unsafe-argument': 'error', '@typescript-eslint/no-unsafe-assignment': 'error', '@typescript-eslint/no-unsafe-call': 'error', + '@typescript-eslint/no-unsafe-declaration-merging': 'error', '@typescript-eslint/no-unsafe-member-access': 'error', '@typescript-eslint/no-unsafe-return': 'error', 'no-unused-expressions': 'off', diff --git a/packages/eslint-plugin/src/rules/index.ts b/packages/eslint-plugin/src/rules/index.ts index 29a47ec2384..851661400fb 100644 --- a/packages/eslint-plugin/src/rules/index.ts +++ b/packages/eslint-plugin/src/rules/index.ts @@ -78,6 +78,7 @@ import noUnnecessaryTypeConstraint from './no-unnecessary-type-constraint'; import noUnsafeArgument from './no-unsafe-argument'; import noUnsafeAssignment from './no-unsafe-assignment'; import noUnsafeCall from './no-unsafe-call'; +import noUnsafeDeclarationMerging from './no-unsafe-declaration-merging'; import noUnsafeMemberAccess from './no-unsafe-member-access'; import noUnsafeReturn from './no-unsafe-return'; import noUnusedExpressions from './no-unused-expressions'; @@ -207,6 +208,7 @@ export default { 'no-unsafe-argument': noUnsafeArgument, 'no-unsafe-assignment': noUnsafeAssignment, 'no-unsafe-call': noUnsafeCall, + 'no-unsafe-declaration-merging': noUnsafeDeclarationMerging, 'no-unsafe-member-access': noUnsafeMemberAccess, 'no-unsafe-return': noUnsafeReturn, 'no-unused-expressions': noUnusedExpressions, diff --git a/packages/eslint-plugin/src/rules/no-unsafe-declaration-merging.ts b/packages/eslint-plugin/src/rules/no-unsafe-declaration-merging.ts new file mode 100644 index 00000000000..748e875ae0e --- /dev/null +++ b/packages/eslint-plugin/src/rules/no-unsafe-declaration-merging.ts @@ -0,0 +1,56 @@ +import type { TSESTree } from '@typescript-eslint/utils'; +import * as ts from 'typescript'; + +import * as util from '../util'; + +const SyntaxKind = ts.SyntaxKind; + +export default util.createRule({ + name: 'no-unsafe-declaration-merging', + meta: { + type: 'problem', + docs: { + description: 'Disallow unsafe declaration merging', + recommended: false, + requiresTypeChecking: true, + }, + messages: { + unsafeMerging: + 'Unsafe declaration merging between classes and interfaces.', + }, + schema: [], + }, + defaultOptions: [], + create(context) { + const parserServices = util.getParserServices(context); + const checker = parserServices.program.getTypeChecker(); + + function checkUnsafeDeclaration( + node: TSESTree.Identifier, + unsafeKind: ts.SyntaxKind, + ): void { + if (node.parent) { + const tsNode = parserServices.esTreeNodeToTSNodeMap.get(node); + const type = checker.getTypeAtLocation(tsNode); + const symbol = type.getSymbol(); + if (symbol?.declarations?.some(decl => decl.kind === unsafeKind)) { + context.report({ + node: node.parent, + messageId: 'unsafeMerging', + }); + } + } + } + + return { + ClassDeclaration(node): void { + if (node.id) { + checkUnsafeDeclaration(node.id, SyntaxKind.InterfaceDeclaration); + } + }, + TSInterfaceDeclaration(node): void { + checkUnsafeDeclaration(node.id, SyntaxKind.ClassDeclaration); + }, + }; + }, +}); diff --git a/packages/eslint-plugin/tests/rules/no-unsafe-declaration-merging.test.ts b/packages/eslint-plugin/tests/rules/no-unsafe-declaration-merging.test.ts new file mode 100644 index 00000000000..b95b74db175 --- /dev/null +++ b/packages/eslint-plugin/tests/rules/no-unsafe-declaration-merging.test.ts @@ -0,0 +1,71 @@ +import rule from '../../src/rules/no-unsafe-declaration-merging'; +import { getFixturesRootDir, RuleTester } from '../RuleTester'; + +const rootPath = getFixturesRootDir(); + +const ruleTester = new RuleTester({ + parser: '@typescript-eslint/parser', + parserOptions: { + sourceType: 'module', + tsconfigRootDir: rootPath, + project: './tsconfig.json', + }, +}); + +ruleTester.run('no-unsafe-declaration-merging', rule, { + valid: [ + ` +interface Foo {} +class Bar implements Foo {} + `, + ` +namespace Foo {} +namespace Foo {} + `, + ` +enum Foo {} +namespace Foo {} + `, + ` +namespace Fooo {} +function Foo() {} + `, + ` +const Foo = class {}; + `, + ], + invalid: [ + { + code: ` +interface Foo {} +class Foo {} + `, + errors: [ + { + messageId: 'unsafeMerging', + }, + { + messageId: 'unsafeMerging', + }, + ], + }, + { + code: ` +namespace Foo { + export interface Bar {} +} +namespace Foo { + export class Bar {} +} + `, + errors: [ + { + messageId: 'unsafeMerging', + }, + { + messageId: 'unsafeMerging', + }, + ], + }, + ], +}); From 28560e0ee9040c43ddc9020d10612e8b433bb639 Mon Sep 17 00:00:00 2001 From: yeonjuan Date: Tue, 18 Oct 2022 21:06:23 +0900 Subject: [PATCH 2/4] Apply reviews --- .../rules/no-unsafe-declaration-merging.md | 4 +- .../rules/no-unsafe-declaration-merging.ts | 26 ++++----- .../no-unsafe-declaration-merging.test.ts | 53 +++++++++++++++++++ 3 files changed, 66 insertions(+), 17 deletions(-) diff --git a/packages/eslint-plugin/docs/rules/no-unsafe-declaration-merging.md b/packages/eslint-plugin/docs/rules/no-unsafe-declaration-merging.md index f974329a716..1b4b97b16a2 100644 --- a/packages/eslint-plugin/docs/rules/no-unsafe-declaration-merging.md +++ b/packages/eslint-plugin/docs/rules/no-unsafe-declaration-merging.md @@ -6,10 +6,10 @@ description: 'Disallow unsafe declaration merging.' > > See **https://typescript-eslint.io/rules/no-unsafe-declaration-merging** for documentation. -TypeScript's "declaration merging" supports merging separate declarations with the same name. We can merges +TypeScript's "declaration merging" supports merging separate declarations with the same name. Declaration merging between classes and interfaces is unsafe. -TypeScript compiler doesn't check whether properties are initialized or not, possibly making runtime errors. +TypeScript compiler doesn't check whether properties are initialized, which can cause lead to TypeScript not detecting code that will cause runtime errors. ```ts interface Foo { diff --git a/packages/eslint-plugin/src/rules/no-unsafe-declaration-merging.ts b/packages/eslint-plugin/src/rules/no-unsafe-declaration-merging.ts index 748e875ae0e..7982fe5c5f4 100644 --- a/packages/eslint-plugin/src/rules/no-unsafe-declaration-merging.ts +++ b/packages/eslint-plugin/src/rules/no-unsafe-declaration-merging.ts @@ -3,15 +3,13 @@ import * as ts from 'typescript'; import * as util from '../util'; -const SyntaxKind = ts.SyntaxKind; - export default util.createRule({ name: 'no-unsafe-declaration-merging', meta: { type: 'problem', docs: { description: 'Disallow unsafe declaration merging', - recommended: false, + recommended: 'strict', requiresTypeChecking: true, }, messages: { @@ -29,27 +27,25 @@ export default util.createRule({ node: TSESTree.Identifier, unsafeKind: ts.SyntaxKind, ): void { - if (node.parent) { - const tsNode = parserServices.esTreeNodeToTSNodeMap.get(node); - const type = checker.getTypeAtLocation(tsNode); - const symbol = type.getSymbol(); - if (symbol?.declarations?.some(decl => decl.kind === unsafeKind)) { - context.report({ - node: node.parent, - messageId: 'unsafeMerging', - }); - } + const tsNode = parserServices.esTreeNodeToTSNodeMap.get(node); + const type = checker.getTypeAtLocation(tsNode); + const symbol = type.getSymbol(); + if (symbol?.declarations?.some(decl => decl.kind === unsafeKind)) { + context.report({ + node, + messageId: 'unsafeMerging', + }); } } return { ClassDeclaration(node): void { if (node.id) { - checkUnsafeDeclaration(node.id, SyntaxKind.InterfaceDeclaration); + checkUnsafeDeclaration(node.id, ts.SyntaxKind.InterfaceDeclaration); } }, TSInterfaceDeclaration(node): void { - checkUnsafeDeclaration(node.id, SyntaxKind.ClassDeclaration); + checkUnsafeDeclaration(node.id, ts.SyntaxKind.ClassDeclaration); }, }; }, diff --git a/packages/eslint-plugin/tests/rules/no-unsafe-declaration-merging.test.ts b/packages/eslint-plugin/tests/rules/no-unsafe-declaration-merging.test.ts index b95b74db175..e92c1ed36e7 100644 --- a/packages/eslint-plugin/tests/rules/no-unsafe-declaration-merging.test.ts +++ b/packages/eslint-plugin/tests/rules/no-unsafe-declaration-merging.test.ts @@ -33,6 +33,31 @@ function Foo() {} ` const Foo = class {}; `, + ` +interface Foo { + props: string; +} + +function bar() { + return class Foo {}; +} + `, + ` +interface Foo { + props: string; +} + +(function bar() { + class Foo {} +})(); + `, + ` +declare global { + interface Foo {} +} + +class Foo {} + `, ], invalid: [ { @@ -43,9 +68,13 @@ class Foo {} errors: [ { messageId: 'unsafeMerging', + line: 2, + column: 11, }, { messageId: 'unsafeMerging', + line: 3, + column: 7, }, ], }, @@ -61,9 +90,33 @@ namespace Foo { errors: [ { messageId: 'unsafeMerging', + line: 3, + column: 20, + }, + { + messageId: 'unsafeMerging', + line: 6, + column: 16, + }, + ], + }, + { + code: ` +declare global { + interface Foo {} + class Foo {} +} + `, + errors: [ + { + messageId: 'unsafeMerging', + line: 3, + column: 13, }, { messageId: 'unsafeMerging', + line: 4, + column: 9, }, ], }, From 944a5a40c95899fb559f99581c5dd0744fbc6a7e Mon Sep 17 00:00:00 2001 From: yeonjuan Date: Tue, 18 Oct 2022 23:40:58 +0900 Subject: [PATCH 3/4] fix --- packages/eslint-plugin/src/configs/strict.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/eslint-plugin/src/configs/strict.ts b/packages/eslint-plugin/src/configs/strict.ts index a9c91f7c1ca..ccd44b85a0a 100644 --- a/packages/eslint-plugin/src/configs/strict.ts +++ b/packages/eslint-plugin/src/configs/strict.ts @@ -27,6 +27,7 @@ export = { '@typescript-eslint/no-unnecessary-boolean-literal-compare': 'warn', '@typescript-eslint/no-unnecessary-condition': 'warn', '@typescript-eslint/no-unnecessary-type-arguments': 'warn', + '@typescript-eslint/no-unsafe-declaration-merging': 'warn', 'no-useless-constructor': 'off', '@typescript-eslint/no-useless-constructor': 'warn', '@typescript-eslint/non-nullable-type-assertion-style': 'warn', From 231ff6feb1aad83fdfee602fe0f42928123d5056 Mon Sep 17 00:00:00 2001 From: Josh Goldberg Date: Tue, 18 Oct 2022 22:55:37 -0400 Subject: [PATCH 4/4] Apply suggestions from code review --- .../eslint-plugin/docs/rules/no-unsafe-declaration-merging.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/eslint-plugin/docs/rules/no-unsafe-declaration-merging.md b/packages/eslint-plugin/docs/rules/no-unsafe-declaration-merging.md index 1b4b97b16a2..9c917aa3ee0 100644 --- a/packages/eslint-plugin/docs/rules/no-unsafe-declaration-merging.md +++ b/packages/eslint-plugin/docs/rules/no-unsafe-declaration-merging.md @@ -9,7 +9,7 @@ description: 'Disallow unsafe declaration merging.' TypeScript's "declaration merging" supports merging separate declarations with the same name. Declaration merging between classes and interfaces is unsafe. -TypeScript compiler doesn't check whether properties are initialized, which can cause lead to TypeScript not detecting code that will cause runtime errors. +The TypeScript compiler doesn't check whether properties are initialized, which can cause lead to TypeScript not detecting code that will cause runtime errors. ```ts interface Foo { @@ -39,7 +39,7 @@ class Foo {} ```ts interface Foo {} -class Bar extends Foo {} +class Bar implements Foo {} namespace Baz {} namespace Baz {}