Skip to content
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.

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

fix(eslint-plugin): [no-unnecessary-type-assertion] fix false negative for const variable declarations #8558

Merged
Merged
Show file tree
Hide file tree
Changes from 20 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Expand Up @@ -35,6 +35,10 @@ type Foo = 3;
const foo = 3 as Foo;
```

```ts
const foo = 'foo' as const;
```

```ts
function foo(x: number): number {
return x!; // unnecessary non-null
Expand All @@ -52,7 +56,7 @@ const foo = 3 as number;
```

```ts
const foo = 'foo' as const;
let foo = 'foo' as const;
```

```ts
Expand Down
61 changes: 15 additions & 46 deletions packages/eslint-plugin/src/rules/no-unnecessary-type-assertion.ts
abrahamguo marked this conversation as resolved.
Show resolved Hide resolved
Expand Up @@ -61,37 +61,6 @@ export default createRule<Options, MessageIds>({
const checker = services.program.getTypeChecker();
const compilerOptions = services.program.getCompilerOptions();

/**
* Sometimes tuple types don't have ObjectFlags.Tuple set, like when they're being matched against an inferred type.
* So, in addition, check if there are integer properties 0..n and no other numeric keys
*/
function couldBeTupleType(type: ts.ObjectType): boolean {
const properties = type.getProperties();

if (properties.length === 0) {
return false;
}
let i = 0;

for (; i < properties.length; ++i) {
const name = properties[i].name;

if (String(i) !== name) {
if (i === 0) {
// if there are no integer properties, this is not a tuple
return false;
}
break;
}
}
for (; i < properties.length; ++i) {
if (String(+properties[i].name) === properties[i].name) {
return false; // if there are any other numeric properties, this is not a tuple
}
}
return true;
}

/**
* Returns true if there's a chance the variable has been used before a value has been assigned to it
*/
Expand Down Expand Up @@ -139,6 +108,13 @@ export default createRule<Options, MessageIds>({
);
}

function isConstVariableDeclaration(node: TSESTree.Node): boolean {
return (
node.type === AST_NODE_TYPES.VariableDeclaration &&
node.kind === 'const'
);
}

Comment on lines +111 to +117
Copy link
Member

Choose a reason for hiding this comment

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

[non-actionable] Hmmm, looks like these cases are not reported (playground)

let a: unknown
const b = (a = 1 as 1)
const c = (console.log(1), 1 as 1)

However, I'm not sure we can easily detect all such cases. This function might become too complex...

Copy link
Member

Choose a reason for hiding this comment

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

Yeah I think we can leave these for now. If someone files an issue showing them being buggy in isolation, then that'd indicate there's enough user appetite to start thinking about whether the complexity is worth it.

return {
TSNonNullExpression(node): void {
if (
Expand Down Expand Up @@ -236,28 +212,21 @@ export default createRule<Options, MessageIds>({
if (
options.typesToIgnore?.includes(
context.sourceCode.getText(node.typeAnnotation),
) ||
isConstAssertion(node.typeAnnotation)
)
) {
return;
}

const castType = services.getTypeAtLocation(node);

if (
isTypeFlagSet(castType, ts.TypeFlags.Literal) ||
(tsutils.isObjectType(castType) &&
(tsutils.isObjectFlagSet(castType, ts.ObjectFlags.Tuple) ||
couldBeTupleType(castType)))
) {
// It's not always safe to remove a cast to a literal type or tuple
// type, as those types are sometimes widened without the cast.
return;
abrahamguo marked this conversation as resolved.
Show resolved Hide resolved
}

const uncastType = services.getTypeAtLocation(node.expression);
const typeIsUnchanged = uncastType === castType;

const wouldSameTypeBeInferred = castType.isLiteral()
? // eslint-disable-next-line @typescript-eslint/no-non-null-assertion
isConstVariableDeclaration(node.parent.parent!)
: !isConstAssertion(node.typeAnnotation);

if (uncastType === castType) {
if (typeIsUnchanged && wouldSameTypeBeInferred) {
context.report({
node,
messageId: 'unnecessaryAssertion',
Expand Down
Expand Up @@ -7,7 +7,7 @@ const ruleTester = new RuleTester({
parser: '@typescript-eslint/parser',
});

const messageId = 'useLiteral' as const;
const messageId = 'useLiteral';

ruleTester.run('no-array-constructor', rule, {
valid: [
Expand Down
Expand Up @@ -25,10 +25,34 @@ if (
const name = member.id as TSESTree.StringLiteral;
}
`,
`
const c = 1;
let z = c as number;
`,
`
const c = 1;
let z = c as const;
`,
`
const c = 1;
let z = c as 1;
`,
`
type Bar = 'bar';
const data = {
x: 'foo' as 'foo',
y: 'bar' as Bar,
};
`,
"[1, 2, 3, 4, 5].map(x => [x, 'A' + x] as [number, string]);",
`
let x: Array<[number, string]> = [1, 2, 3, 4, 5].map(
x => [x, 'A' + x] as [number, string],
);
`,
'let y = 1 as 1;',
'const foo = 3 as number;',
'const foo = <number>3;',
'const foo = <3>3;',
'const foo = 3 as 3;',
abrahamguo marked this conversation as resolved.
Show resolved Hide resolved
`
type Tuple = [3, 'hi', 'bye'];
const foo = [3, 'hi', 'bye'] as Tuple;
Expand Down Expand Up @@ -174,9 +198,6 @@ const c = [...a, ...b] as const;
{
code: 'const a = [1, 2] as const;',
},
{
code: "const a = 'a' as const;",
},
{
code: "const a = { foo: 'foo' } as const;",
},
Expand All @@ -190,9 +211,6 @@ const c = <const>[...a, ...b];
{
code: 'const a = <const>[1, 2];',
},
{
code: "const a = <const>'a';",
},
{
code: "const a = <const>{ foo: 'foo' };",
},
Expand Down Expand Up @@ -245,6 +263,48 @@ const item = <object>arr[0];
],

invalid: [
{
code: "const a = 'a' as const;",
output: "const a = 'a';",
errors: [{ messageId: 'unnecessaryAssertion', line: 1 }],
},
{
code: "const a = <const>'a';",
output: "const a = 'a';",
errors: [{ messageId: 'unnecessaryAssertion', line: 1 }],
},
{
code: 'const foo = <3>3;',
output: 'const foo = 3;',
errors: [{ messageId: 'unnecessaryAssertion', line: 1, column: 13 }],
},
{
code: 'const foo = 3 as 3;',
output: 'const foo = 3;',
errors: [{ messageId: 'unnecessaryAssertion', line: 1, column: 13 }],
},
{
code: `
type Foo = 3;
const foo = <Foo>3;
`,
output: `
type Foo = 3;
const foo = 3;
`,
errors: [{ messageId: 'unnecessaryAssertion', line: 3, column: 21 }],
},
{
code: `
type Foo = 3;
const foo = 3 as Foo;
`,
output: `
type Foo = 3;
const foo = 3;
`,
errors: [{ messageId: 'unnecessaryAssertion', line: 3, column: 21 }],
},
{
code: `
const foo = 3;
Expand Down