From 1f6fe6b51a8e20dbbb0b28226b467ff66cb372df Mon Sep 17 00:00:00 2001 From: liuxingbaoyu <30521560+liuxingbaoyu@users.noreply.github.com> Date: Sun, 2 Jun 2024 17:20:12 +0800 Subject: [PATCH 1/3] fix --- .../rules/no-unnecessary-type-assertion.ts | 69 ++++++++++++------- .../no-unnecessary-type-assertion.test.ts | 26 +++++++ 2 files changed, 71 insertions(+), 24 deletions(-) diff --git a/packages/eslint-plugin/src/rules/no-unnecessary-type-assertion.ts b/packages/eslint-plugin/src/rules/no-unnecessary-type-assertion.ts index 3523fd4a477e..6fbd8e07c138 100644 --- a/packages/eslint-plugin/src/rules/no-unnecessary-type-assertion.ts +++ b/packages/eslint-plugin/src/rules/no-unnecessary-type-assertion.ts @@ -1,3 +1,4 @@ +import type { Scope } from '@typescript-eslint/scope-manager'; import type { TSESTree } from '@typescript-eslint/utils'; import { AST_NODE_TYPES, AST_TOKEN_TYPES } from '@typescript-eslint/utils'; import * as tsutils from 'ts-api-utils'; @@ -80,33 +81,53 @@ export default createRule({ ) && // ignore class properties as they are compile time guarded // also ignore function arguments as they can't be used before defined - ts.isVariableDeclaration(declaration) && - // is it `const x!: number` - declaration.initializer === undefined && - declaration.exclamationToken === undefined && - declaration.type !== undefined + ts.isVariableDeclaration(declaration) ) { - // check if the defined variable type has changed since assignment - const declarationType = checker.getTypeFromTypeNode(declaration.type); - const type = getConstrainedTypeAtLocation(services, node); if ( - declarationType === type && - // `declare`s are never narrowed, so never skip them - !( - ts.isVariableDeclarationList(declaration.parent) && - ts.isVariableStatement(declaration.parent.parent) && - tsutils.includesModifier( - getModifiers(declaration.parent.parent), - ts.SyntaxKind.DeclareKeyword, - ) - ) + ts.isVariableDeclarationList(declaration.parent) && + // var + declaration.parent.flags === ts.NodeFlags.None + ) { + const declaratorNode: TSESTree.VariableDeclarator = + services.tsNodeToESTreeNodeMap.get(declaration); + const scope = context.sourceCode.getScope(node); + const declaratorScope = context.sourceCode.getScope(declaratorNode); + let parentScope: Scope | null = declaratorScope; + while ((parentScope = parentScope.upper)) { + if (parentScope === scope) { + return true; + } + } + } + + if ( + // is it `const x!: number` + declaration.initializer === undefined && + declaration.exclamationToken === undefined && + declaration.type !== undefined ) { - // possibly used before assigned, so just skip it - // better to false negative and skip it, than false positive and fix to compile erroring code - // - // no better way to figure this out right now - // https://github.com/Microsoft/TypeScript/issues/31124 - return true; + // check if the defined variable type has changed since assignment + const declarationType = checker.getTypeFromTypeNode(declaration.type); + const type = getConstrainedTypeAtLocation(services, node); + if ( + declarationType === type && + // `declare`s are never narrowed, so never skip them + !( + ts.isVariableDeclarationList(declaration.parent) && + ts.isVariableStatement(declaration.parent.parent) && + tsutils.includesModifier( + getModifiers(declaration.parent.parent), + ts.SyntaxKind.DeclareKeyword, + ) + ) + ) { + // possibly used before assigned, so just skip it + // better to false negative and skip it, than false positive and fix to compile erroring code + // + // no better way to figure this out right now + // https://github.com/Microsoft/TypeScript/issues/31124 + return true; + } } } return false; diff --git a/packages/eslint-plugin/tests/rules/no-unnecessary-type-assertion.test.ts b/packages/eslint-plugin/tests/rules/no-unnecessary-type-assertion.test.ts index e2a130c39d9b..f019ce88f2ae 100644 --- a/packages/eslint-plugin/tests/rules/no-unnecessary-type-assertion.test.ts +++ b/packages/eslint-plugin/tests/rules/no-unnecessary-type-assertion.test.ts @@ -348,6 +348,16 @@ const bar = foo.a as string | undefined | bigint; `, languageOptions: { parserOptions: optionsWithExactOptionalPropertyTypes }, }, + { + code: ` +if (Math.random()) { + { + var x = 1; + } +} +x!; + `, + }, ], invalid: [ @@ -1076,5 +1086,21 @@ const bar = foo.a; ], languageOptions: { parserOptions: optionsWithExactOptionalPropertyTypes }, }, + { + code: ` +var x = 1; +x!; + `, + output: ` +var x = 1; +x; + `, + errors: [ + { + messageId: 'unnecessaryAssertion', + line: 3, + }, + ], + }, ], }); From 78911009387b64327b173d589fd8ac265fc5e3f2 Mon Sep 17 00:00:00 2001 From: liuxingbaoyu <30521560+liuxingbaoyu@users.noreply.github.com> Date: Fri, 2 Aug 2024 17:44:02 +0800 Subject: [PATCH 2/3] review --- .../rules/no-unnecessary-type-assertion.ts | 12 ++++++++++- .../no-unnecessary-type-assertion.test.ts | 20 +++++++++++++++++++ 2 files changed, 31 insertions(+), 1 deletion(-) diff --git a/packages/eslint-plugin/src/rules/no-unnecessary-type-assertion.ts b/packages/eslint-plugin/src/rules/no-unnecessary-type-assertion.ts index 6fbd8e07c138..4aa9ba891fca 100644 --- a/packages/eslint-plugin/src/rules/no-unnecessary-type-assertion.ts +++ b/packages/eslint-plugin/src/rules/no-unnecessary-type-assertion.ts @@ -83,12 +83,22 @@ export default createRule({ // also ignore function arguments as they can't be used before defined ts.isVariableDeclaration(declaration) ) { + // For var declarations, we need to check whether the node + // is actually in a descendant of its declaration or not. If not, + // it may be used before defined. + + // eg + // if (Math.random() < 0.5) { + // var x: number = 2; + // } else { + // x!.toFixed(); + // } if ( ts.isVariableDeclarationList(declaration.parent) && // var declaration.parent.flags === ts.NodeFlags.None ) { - const declaratorNode: TSESTree.VariableDeclarator = + const declaratorNode: TSESTree.VariableDeclaration = services.tsNodeToESTreeNodeMap.get(declaration); const scope = context.sourceCode.getScope(node); const declaratorScope = context.sourceCode.getScope(declaratorNode); diff --git a/packages/eslint-plugin/tests/rules/no-unnecessary-type-assertion.test.ts b/packages/eslint-plugin/tests/rules/no-unnecessary-type-assertion.test.ts index f019ce88f2ae..afc0b3c3b9cc 100644 --- a/packages/eslint-plugin/tests/rules/no-unnecessary-type-assertion.test.ts +++ b/packages/eslint-plugin/tests/rules/no-unnecessary-type-assertion.test.ts @@ -1102,5 +1102,25 @@ x; }, ], }, + { + code: ` +var x = 1; +{ + x!; +} + `, + output: ` +var x = 1; +{ + x; +} + `, + errors: [ + { + messageId: 'unnecessaryAssertion', + line: 4, + }, + ], + }, ], }); From 66ab8e1e23339466bba0cd591f1466095e3d006e Mon Sep 17 00:00:00 2001 From: liuxingbaoyu <30521560+liuxingbaoyu@users.noreply.github.com> Date: Fri, 16 Aug 2024 04:44:42 +0800 Subject: [PATCH 3/3] review --- .../src/rules/no-unnecessary-type-assertion.ts | 5 ++++- .../eslint-plugin/tests/fixtures/tsconfig.json | 3 ++- .../tests/fixtures/var-declaration.ts | 1 + .../rules/no-unnecessary-type-assertion.test.ts | 14 ++++++++++++++ 4 files changed, 21 insertions(+), 2 deletions(-) create mode 100644 packages/eslint-plugin/tests/fixtures/var-declaration.ts diff --git a/packages/eslint-plugin/src/rules/no-unnecessary-type-assertion.ts b/packages/eslint-plugin/src/rules/no-unnecessary-type-assertion.ts index 4aa9ba891fca..6b69d9e381d4 100644 --- a/packages/eslint-plugin/src/rules/no-unnecessary-type-assertion.ts +++ b/packages/eslint-plugin/src/rules/no-unnecessary-type-assertion.ts @@ -96,7 +96,10 @@ export default createRule({ if ( ts.isVariableDeclarationList(declaration.parent) && // var - declaration.parent.flags === ts.NodeFlags.None + declaration.parent.flags === ts.NodeFlags.None && + // If they are not in the same file it will not exist. + // This situation must not occur using before defined. + services.tsNodeToESTreeNodeMap.has(declaration) ) { const declaratorNode: TSESTree.VariableDeclaration = services.tsNodeToESTreeNodeMap.get(declaration); diff --git a/packages/eslint-plugin/tests/fixtures/tsconfig.json b/packages/eslint-plugin/tests/fixtures/tsconfig.json index 6ae5e64730b1..c16815aaf1ac 100644 --- a/packages/eslint-plugin/tests/fixtures/tsconfig.json +++ b/packages/eslint-plugin/tests/fixtures/tsconfig.json @@ -12,6 +12,7 @@ "file.ts", "consistent-type-exports.ts", "mixed-enums-decl.ts", - "react.tsx" + "react.tsx", + "var-declaration.ts" ] } diff --git a/packages/eslint-plugin/tests/fixtures/var-declaration.ts b/packages/eslint-plugin/tests/fixtures/var-declaration.ts new file mode 100644 index 000000000000..5253648a8d2f --- /dev/null +++ b/packages/eslint-plugin/tests/fixtures/var-declaration.ts @@ -0,0 +1 @@ +var varDeclarationFromFixture = 1; diff --git a/packages/eslint-plugin/tests/rules/no-unnecessary-type-assertion.test.ts b/packages/eslint-plugin/tests/rules/no-unnecessary-type-assertion.test.ts index afc0b3c3b9cc..d292c7bbe19d 100644 --- a/packages/eslint-plugin/tests/rules/no-unnecessary-type-assertion.test.ts +++ b/packages/eslint-plugin/tests/rules/no-unnecessary-type-assertion.test.ts @@ -1088,6 +1088,20 @@ const bar = foo.a; }, { code: ` +varDeclarationFromFixture!; + `, + output: ` +varDeclarationFromFixture; + `, + errors: [ + { + messageId: 'unnecessaryAssertion', + line: 2, + }, + ], + }, + { + code: ` var x = 1; x!; `,