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

[no-unnecessary-type-assertion] too restrictive with intended temporary narrowing #3187

Closed
3 tasks done
danielrentz opened this issue Mar 15, 2021 · 3 comments · Fixed by #3235
Closed
3 tasks done
Labels
bug Something isn't working package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin

Comments

@danielrentz
Copy link

  • I have tried restarting my IDE and the issue persists.
  • I have updated to the latest version of the packages.
  • I have read the FAQ and my problem is not listed.

Repro

Release 4.17.0 has changed the rule "no-unnecessary-type-assertion" (PR #3133) in a way that breaks existing code, as it seems it now forbids some legitimate use cases of the ! operator:

{
  "rules": {
    "@typescript-eslint/no-unnecessary-type-assertion": "error"
  }
}
class X {
    public doSomething(): void { }
}

const arr: X[] = [];

let x: X | undefined;
if (arr.length > 0) {
    x = arr.pop()!; // <== known that pop() returns something, needed to temp. narrow `x`
    x.doSomething();
}

if (x) {
    // ...
}

Expected Result
Inside the first if, the assignment to x intentionally narrows the result of pop() because it's sure that the array is not empty.

Actual Result
With v4.17 the narrowing will produce a warning.

Versions

package version
@typescript-eslint/eslint-plugin 4.17.0
@typescript-eslint/parser 4.17.0
TypeScript 4.2.3
ESLint 7.22.0
node 12.16.2
@danielrentz danielrentz added package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin triage Waiting for maintainers to take a look labels Mar 15, 2021
@bradzacher bradzacher added bug Something isn't working and removed triage Waiting for maintainers to take a look labels Mar 15, 2021
@bradzacher
Copy link
Member

A better way to do this code which eliminates the need for a non-null assertion:

class X {
    public doSomething(): void { }
}

const arr: X[] = [];

const x = arr.pop();
if (x) {
    x.doSomething();
}

if (x) {
    // ...
}

@danielrentz
Copy link
Author

Thanks, sure the given example could be rewritten like this. Maybe I have over-simplified the real code, it looks more like so:

class X {
    public doSomething(): void { }
}

const arr: X[] = [];
let x: X | undefined;

// more code that fills `arr` and *sometimes* initializes `x`
// ...

if (someConditions() && (arr.length > 0)) {
    x = arr.pop()!;
    x.doSomething();
    // do more with `x`
}

// ...

So I do not want to overwrite the current value of x if some conditions are met but the array is empty.

Of course this example can also be rewritten to prevent the assertion.

However, my point is that the rule is called "no-unnecessary-type-assertion", and its description says "Warns if a type assertion does not change the type of an expression", but in my opinion this is an example of a necessary type assertion because it intentionally changes the type of the value assigned as well as of the variable being assigned to, also for following code.

@eamodio
Copy link

eamodio commented Mar 23, 2021

Yeah, I agree, with the recent changes, I'll have to disable this rule, because it is flagging intentional uses of ! now.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin
Projects
None yet
3 participants