Skip to content

fix(eslint-plugin): [no-unnecessary-type-assertion] conflict with TS for variables used before assignment #9209

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Aug 19, 2024
Merged

fix(eslint-plugin): [no-unnecessary-type-assertion] conflict with TS for variables used before assignment #9209

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1f6fe6b
fix
liuxingbaoyu Jun 2, 2024
7891100
review
liuxingbaoyu Aug 2, 2024
66ab8e1
review
liuxingbaoyu Aug 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
82 changes: 58 additions & 24 deletions packages/eslint-plugin/src/rules/no-unnecessary-type-assertion.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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';
Expand Down Expand Up @@ -80,33 +81,66 @@ export default createRule<Options, MessageIds>({
) &&
// 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);
// 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 (
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 &&
// If they are not in the same file it will not exist.
// This situation must not occur using before defined.
services.tsNodeToESTreeNodeMap.has(declaration)
) {
// 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;
const declaratorNode: TSESTree.VariableDeclaration =
services.tsNodeToESTreeNodeMap.get(declaration);
Comment on lines +104 to +105
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I noticed it when rebasing, I'm not sure if this will be affected.🤔
#9660

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@auvred Could you double check this?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@liuxingbaoyu Thanks for calling this out. Good attention to detail!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for such a long reply! I missed the notification :(

I think we can't just rely on services.tsNodeToESTreeNodeMap.get when dealing with declarations, as these declarations may be contained in other files that tsNodeToESTreeNodeMap doesn't know about.

Imagine the following:

// var-declaration.ts
var varDeclarationFromFixture = 1;

// file.ts
varDeclarationFromFixture!;

The rule throws runtime error on this code.

FAIL tests/rules/no-unnecessary-type-assertion.test.ts
  ● Test suite failed to run

    TypeError: Converting circular structure to JSON
        --> starting at object with constructor 'Object'
        |     property 'expression' -> object with constructor 'Object'
        --- property 'parent' closes the circle
        at stringify (<anonymous>)

      at messageParent (../../node_modules/jest-worker/build/workers/messageParent.js:29:19)

Test Suites: 1 failed, 1 total
Tests:       0 total
Snapshots:   0 total
Time:        5.074 s
Ran all test suites matching /no-unnecessary-type-assertion/i.

Since it requires more than one file to reproduce, I can't reproduce it in online playground, so here's the patch:

diff --git a/packages/eslint-plugin/tests/fixtures/tsconfig.json b/packages/eslint-plugin/tests/fixtures/tsconfig.json
index 6ae5e6473..c16815aaf 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 000000000..5253648a8
--- /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 afc0b3c3b..b636eec84 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: 3,
+        },
+      ],
+    },
+    {
+      code: `
 var x = 1;
 x!;
       `,

You can run git apply to apply it locally.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the test! It seems we can skip it in this case, since the declaration in the different file never appears used before assignment.

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
) {
// 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;
Expand Down
3 changes: 2 additions & 1 deletion packages/eslint-plugin/tests/fixtures/tsconfig.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
"file.ts",
"consistent-type-exports.ts",
"mixed-enums-decl.ts",
"react.tsx"
"react.tsx",
"var-declaration.ts"
]
}
1 change: 1 addition & 0 deletions packages/eslint-plugin/tests/fixtures/var-declaration.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
var varDeclarationFromFixture = 1;
60 changes: 60 additions & 0 deletions packages/eslint-plugin/tests/rules/no-unnecessary-type-assertion.test.ts
View file
Open in desktop

This comment was marked as outdated.

Sign in to view

Original file line number Diff line number Diff line change
Expand Up @@ -348,6 +348,16 @@ const bar = foo.a as string | undefined | bigint;
`,
languageOptions: { parserOptions: optionsWithExactOptionalPropertyTypes },
},
{
code: `
if (Math.random()) {
{
var x = 1;
}
}
x!;
`,
},
],

invalid: [
Expand Down Expand Up @@ -1076,5 +1086,55 @@ const bar = foo.a;
],
languageOptions: { parserOptions: optionsWithExactOptionalPropertyTypes },
},
{
code: `
varDeclarationFromFixture!;
`,
output: `
varDeclarationFromFixture;
`,
errors: [
{
messageId: 'unnecessaryAssertion',
line: 2,
},
],
},
{
code: `
var x = 1;
x!;
`,
output: `
var x = 1;
x;
`,
errors: [
{
messageId: 'unnecessaryAssertion',
line: 3,
},
],
},
{
code: `
var x = 1;
{
x!;
}
`,
output: `
var x = 1;
{
x;
}
`,
errors: [
{
messageId: 'unnecessaryAssertion',
line: 4,
},
],
},
],
});
Loading