Skip to content

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

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 22 commits into from
Mar 18, 2024
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
d37bdd5
false negative for consts
abrahamguo Feb 27, 2024
60ac5e5
add more testcases
abrahamguo Feb 27, 2024
cedae49
remove unused fn
abrahamguo Feb 27, 2024
39c7d4b
add NNA
abrahamguo Feb 27, 2024
9f01e2a
split into two ifs
abrahamguo Feb 27, 2024
b44cfe3
restore comment
abrahamguo Feb 27, 2024
054f71c
Merge branch 'main' of github.com:abrahamguo/typescript-eslint into n…
abrahamguo Feb 27, 2024
78f8744
finish
abrahamguo Feb 28, 2024
314d912
change order for diffs
abrahamguo Feb 28, 2024
3307af5
fix error triggered by new lint logic
abrahamguo Feb 28, 2024
5d7dd2a
WIP clean up tests
abrahamguo Mar 4, 2024
aa1ea22
WIP
abrahamguo Mar 4, 2024
37406fc
WIP
abrahamguo Mar 4, 2024
09f23b9
remove duplicate tests
abrahamguo Mar 4, 2024
898de07
Merge branch 'main' of github.com:abrahamguo/typescript-eslint into n…
abrahamguo Mar 7, 2024
e40b518
fix docs
abrahamguo Mar 7, 2024
83a77d1
make test cases more self-contained
abrahamguo Mar 7, 2024
9dcc70b
suppress lint error
abrahamguo Mar 7, 2024
fadd011
Merge branch 'no-unnecessary-type-assertion' of github.com:abrahamguo…
abrahamguo Mar 7, 2024
5e04444
Merge branch 'main' of github.com:abrahamguo/typescript-eslint into n…
abrahamguo Mar 8, 2024
60133ea
remove unnecessary type assertions
abrahamguo Mar 8, 2024
69f2cca
Merge branch 'main' into no-unnecessary-type-assertion
JoshuaKGoldberg Mar 17, 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
6 changes: 5 additions & 1 deletion packages/eslint-plugin/docs/rules/no-unnecessary-type-assertion.mdx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,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 @@ -55,7 +59,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
View file
Open in desktop
Original file line number Diff line number Diff line change
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;
}

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
2 changes: 1 addition & 1 deletion packages/eslint-plugin/tests/rules/no-array-constructor.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
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
76 changes: 68 additions & 8 deletions packages/eslint-plugin/tests/rules/no-unnecessary-type-assertion.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
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;',
`
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
8 changes: 4 additions & 4 deletions packages/typescript-estree/tests/lib/persistentParse.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,7 @@ function baseTests(

it('allows parsing of deeply nested new files', () => {
const PROJECT_DIR = setup(tsConfigIncludeAll, false);
const bazSlashBar = 'baz/bar' as const;
const bazSlashBar = 'baz/bar';

// parse once to: assert the config as correct, and to make sure the program is setup
expect(() => parseFile('foo', PROJECT_DIR)).not.toThrow();
Expand All @@ -149,7 +149,7 @@ function baseTests(
fs.mkdirSync(path.join(PROJECT_DIR, 'src', 'bat'));
fs.mkdirSync(path.join(PROJECT_DIR, 'src', 'bat', 'baz'));

const bazSlashBar = 'bat/baz/bar' as const;
const bazSlashBar = 'bat/baz/bar';

// write a new file and attempt to parse it
writeFile(PROJECT_DIR, bazSlashBar);
Expand All @@ -159,7 +159,7 @@ function baseTests(

it('allows renaming of files', () => {
const PROJECT_DIR = setup(tsConfigIncludeAll, true);
const bazSlashBar = 'baz/bar' as const;
const bazSlashBar = 'baz/bar';

// parse once to: assert the config as correct, and to make sure the program is setup
expect(() => parseFile('foo', PROJECT_DIR)).not.toThrow();
Expand Down Expand Up @@ -291,7 +291,7 @@ describe('persistent parse', () => {

it('handles tsconfigs with no includes/excludes (nested)', () => {
const PROJECT_DIR = setup({}, false);
const bazSlashBar = 'baz/bar' as const;
const bazSlashBar = 'baz/bar';

// parse once to: assert the config as correct, and to make sure the program is setup
expect(() => parseFile('foo', PROJECT_DIR)).not.toThrow();
Expand Down