Skip to content

Bug: [no-unnecessary-type-assertion] does not detect unnecessary non-null-assertion on a call expression #8141

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

Jump to bottom
Closed
4 tasks done
lvjiaxuan opened this issue Dec 27, 2023 · 7 comments · Fixed by #8143
Closed
4 tasks done

Bug: [no-unnecessary-type-assertion] does not detect unnecessary non-null-assertion on a call expression #8141

lvjiaxuan opened this issue Dec 27, 2023 · 7 comments · Fixed by #8143
Labels
accepting prs Go ahead, send a pull request that resolves this issue bug Something isn't working package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin

Comments

@lvjiaxuan
Copy link
Contributor

lvjiaxuan commented Dec 27, 2023 •
edited
Loading

Before You File a Proposal Please Confirm You Have Done The Following...

My proposal is suitable for this project

  • I believe my proposal would be useful to the broader TypeScript community (meaning it is not a niche proposal).

Link to the rule's documentation

https://typescript-eslint.io/rules/no-unnecessary-type-assertion

Description

I propose that the no-unnecessary-type-assertion rule should also check for the return type of a function.

Fail

declare function foo(): number;
const a = foo()!; // no error report
const b = foo() as number; // report error but bad fixed: `const b = foo() ; `
const c = <number>foo(); // good now

Pass

declare function foo(): number;
const a = foo();
const b = foo();
const c = foo();

Additional Info

Im ready to send a PR after the issue is marked with the accepting prs label as indicated on the PR Checklist. Welcome to provide more test cases.
@lvjiaxuan lvjiaxuan added enhancement: plugin rule option New rule option for an existing eslint-plugin rule package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin triage Waiting for team members to take a look labels Dec 27, 2023
@auvred
Copy link
Member

auvred commented Dec 27, 2023

const a = foo()!; // no error report

This is most likely a bug, when asserting CallExpression we should check expression's callee, not CallExpression itself

typescript-eslint/packages/eslint-plugin/src/rules/no-unnecessary-type-assertion.ts

Line 172 in cde4d4d

if (isPossiblyUsedBeforeAssigned(node.expression)) {

(because as far I know CallExpression can't have declarations)

typescript-eslint/packages/eslint-plugin/src/rules/no-unnecessary-type-assertion.ts

Lines 99 to 103 in cde4d4d

const declaration = getDeclaration(services, node);
if (!declaration) {
// don't know what the declaration is for some reason, so just assume the worst
return true;
}

const b = foo() as number; // report error but bad fixed: const b = foo() ;

I think it's fine to file a separate issue about this, as it's not related to non-null assertions bug

@Josh-Cena
Copy link
Member 

const b = foo() as number; // report error but bad fixed: `const b = foo() ; `

Why is this a bad fix? You need to run your formatter after autofixing; ESLint does not care about formatting and it's explicitly an antipattern for ESLint autofixers to conform to your formatting patterns.

@bradzacher
Copy link
Member

This is most likely a bug, when asserting CallExpression we should check expression's callee, not CallExpression itself

This is not correct. When you inspect the type of a call expression - you are inspecting the return type of the call. For example if you look at the types tab in the playground and select the call expression - you will see it's type is number. So there is something else that's going on here to cause this bug.

@bradzacher bradzacher changed the title Enhancement: [no-unnecessary-type-assertion] works with function return type Bug: [no-unnecessary-type-assertion] does not detect unnecessary non-null-assertion on a call expression Dec 27, 2023
@bradzacher bradzacher added bug Something isn't working accepting prs Go ahead, send a pull request that resolves this issue and removed triage Waiting for team members to take a look enhancement: plugin rule option New rule option for an existing eslint-plugin rule labels Dec 27, 2023
@bradzacher
Copy link
Member

@Josh-Cena i 100% agree with you! Though in this case it does appear to be an off-by-one error.

It's also done in a really dodgy way - by using "sketchy math" and assuming a specific code shape. We should really just be removing the as token to the end of the asserted type and then. Would be the same "bad fix" but would be less chance of a broken fix.

Tangent aside - I too am fine with us leaving this as is and assuming another rule will pickup the formatting, as is standard and expected in the ESLint ecosystem.

@Josh-Cena
Copy link
Member

Playground

@Josh-Cena
Copy link
Member

Josh-Cena commented Dec 27, 2023 •
edited
Loading

Found #3235 never mind, that one is about assignment, not declaration.

I think it's because this rule ignores everything that doesn't have a declaration, thinking they may be "use before define"?

const a = (1 + 1)!;
const a = new Date()!;

@lvjiaxuan
Copy link
Contributor Author

lvjiaxuan commented Dec 27, 2023 •
edited
Loading 

const b = foo() as number; // report error but bad fixed: `const b = foo() ; `

Why is this a bad fix? You need to run your formatter after autofixing; ESLint does not care about formatting and it's explicitly an antipattern for ESLint autofixers to conform to your formatting patterns.

Thanks for your opinion.
I just reference to the existing test case.

typescript-eslint/packages/eslint-plugin/tests/rules/no-unnecessary-type-assertion.test.ts

Lines 266 to 282 in cde4d4d

{
code: `
type Foo = number;
const foo = (3 + 5) as Foo;
`,
output: `
type Foo = number;
const foo = (3 + 5);
`,
errors: [
{
messageId: 'unnecessaryAssertion',
line: 3,
column: 13,
},
],
},

@lvjiaxuan lvjiaxuan mentioned this issue Dec 27, 2023
Merged
3 tasks
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 20, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
accepting prs Go ahead, send a pull request that resolves this issue bug Something isn't working package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin
Projects
None yet
Milestone
No milestone
4 participants
@bradzacher @lvjiaxuan @Josh-Cena @auvred