fix(eslint-plugin): [non-nullable-type-assertion-style] fix false positive when asserting to a generic type that might be nullish

[djcsdy]

PR Checklist

• Addresses an existing issue: fixes [non-nullable-type-assertion-style] complains when casting to a type parameter that might be nullish #4512
• That issue was marked as accepting prs
• Steps in CONTRIBUTING.md were taken

Overview

Consider the following code:

function first<T>(array: ArrayLike<T>): T | null {
    return array.length > 0 ? (array[0] as T) : null;
}

When compiling with noUncheckedIndexedAccess, the type assertion array[0] as T is required because the original type of array[0] is T | undefined.

However, non-nullable-type-assertion-style complains about the cast, suggesting that it should be array[0]! instead. This is not correct because the type parameter T might itself be nullish. array[0]! is equivalent to array[0] as NonNullable<T>, which is not the same as array[0] as T.

To see the issue more clearly, consider the following slightly more contrived example:

function first<T extends string | null>(array: ArrayLike<T>): T | null {
    return array.length > 0 ? (array[0] as T) : null;
}

non-nullable-type-assertion-style complains about this code too, but here the problem is more obvious. Clearly the type assertion array[0] as T is not equivalent to array[0]! because the type parameter T has a constraint that explicitly allows the type to be null.

This PR loosens non-nullable-type-assertion-style so that in addition to the type assertions it already allows, it will also allow type assertions providing both of the following conditions are true:

1. The asserted type contains a type parameter.
2. The type parameter has no constraint or, if the type parameter has a constraint, then the constraint itself includes nullish types or other type parameters that would meet these conditions.

[nx-cloud]

☁️ Nx Cloud Report

CI ran the following commands for commit da49d03. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch

---

✅ Successfully ran 48 targets

• Node 12.x - nx run-many --target=typecheck --all --parallel
• Node 12.x - nx run-many --target=build --all --parallel --exclude website --exclude website
• Node 12.x - nx prebuild @typescript-eslint/types
• Node 12.x - nx test @typescript-eslint/eslint-plugin-internal
• Node 12.x - nx test @typescript-eslint/eslint-plugin-tslint
• Node 12.x - nx test @typescript-eslint/eslint-plugin
• Node 14.x - nx test @typescript-eslint/eslint-plugin-tslint
• Node 12.x - nx test @typescript-eslint/parser
• Node 12.x - nx test @typescript-eslint/type-utils
• Node 14.x - nx test @typescript-eslint/eslint-plugin
• Node 12.x - nx test @typescript-eslint/utils
• Node 14.x - nx test @typescript-eslint/parser
• Node 12.x - nx test @typescript-eslint/scope-manager
• Node 14.x - nx test @typescript-eslint/type-utils
• Node 12.x - nx test @typescript-eslint/visitor-keys
• Node 12.x - nx test @typescript-eslint/typescript-estree
• Node 14.x - nx test @typescript-eslint/utils
• Node 12.x - nx run-many --target=build --all --parallel --exclude website --exclude website
• Node 12.x - nx run-many --target=build --all --parallel --exclude website --exclude website
• Node 16.x - nx test @typescript-eslint/eslint-plugin-tslint
• Node 14.x - nx test @typescript-eslint/scope-manager
• Node 12.x - nx run-many --target=build --all --parallel --exclude website --exclude website
• Node 16.x - nx test @typescript-eslint/eslint-plugin
• Node 12.x - nx test @typescript-eslint/eslint-plugin-tslint
• Node 14.x - nx test @typescript-eslint/visitor-keys
• Node 12.x - nx test @typescript-eslint/eslint-plugin
• Node 16.x - nx test @typescript-eslint/parser
• Node 12.x - nx test @typescript-eslint/parser
• Node 14.x - nx test @typescript-eslint/typescript-estree
• Node 16.x - nx test @typescript-eslint/type-utils
• Node 12.x - nx test @typescript-eslint/type-utils
• Node 12.x - nx prebuild @typescript-eslint/types
• Node 14.x - nx run-many --target=build --all --parallel --exclude website --exclude website
• Node 16.x - nx test @typescript-eslint/utils
• Node 12.x - nx prebuild @typescript-eslint/types
• Node 12.x - nx test @typescript-eslint/utils
• Node 12.x - nx test @typescript-eslint/scope-manager
• Node 16.x - nx test @typescript-eslint/scope-manager
• Node 12.x - nx test @typescript-eslint/visitor-keys
• Node 16.x - nx test @typescript-eslint/visitor-keys
• Node 12.x - nx test @typescript-eslint/typescript-estree
• Node 12.x - nx prebuild @typescript-eslint/types
• Node 12.x - nx run-many --target=build --all --parallel --exclude website --exclude website
• Node 16.x - nx test @typescript-eslint/typescript-estree
• Node 16.x - nx run-many --target=build --all --parallel --exclude website --exclude website
• Node 14.x - nx prebuild @typescript-eslint/types
• Node 12.x - nx prebuild @typescript-eslint/types
• Node 16.x - nx prebuild @typescript-eslint/types

---

Sent with 💌 from NxCloud.

[typescript-eslint]

Thanks for the PR, @djcsdy!

typescript-eslint is a 100% community driven project, and we are incredibly grateful that you are contributing to that community.

The core maintainers work on this in their personal time, so please understand that it may not be possible for them to review your work immediately.

Thanks again!

---

🙏 Please, if you or your company is finding typescript-eslint valuable, help us sustain the project by sponsoring it transparently on https://opencollective.com/typescript-eslint. As a thank you, your profile/company logo will be added to our main README which receives thousands of unique visitors per day.

[netlify]

✔️ Deploy Preview for typescript-eslint ready!

🔨 Explore the source changes: da49d03

🔍 Inspect the deploy log: https://app.netlify.com/sites/typescript-eslint/deploys/61fda4d5498aac00075a7890

😎 Browse the preview: https://deploy-preview-4509--typescript-eslint.netlify.app

[codecov]

Codecov Report

Merging #4509 (8befbe2) into main (63fbbaa) will increase coverage by 2.08%.
The diff coverage is 100.00%.

❗ Current head 8befbe2 differs from pull request most recent head da49d03. Consider uploading reports for the commit da49d03 to get more accurate results

@@            Coverage Diff             @@
##             main    #4509      +/-   ##
==========================================
+ Coverage   92.49%   94.57%   +2.08%     
==========================================
  Files         346      147     -199     
  Lines       11684     7888    -3796     
  Branches     3335     2534     -801     
==========================================
- Hits        10807     7460    -3347     
+ Misses        608      234     -374     
+ Partials      269      194      -75     

Flag	Coverage Δ
unittest	94.57% <100.00%> (+2.08%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...gin/src/rules/non-nullable-type-assertion-style.ts	100.00% <100.00%> (ø)
...-internal/src/rules/no-typescript-estree-import.ts
packages/scope-manager/src/lib/es2016.full.ts
packages/typescript-estree/src/ts-estree/index.ts
packages/scope-manager/src/lib/es2016.ts
packages/scope-manager/src/scope/BlockScope.ts
...ges/scope-manager/src/lib/es2018.asyncgenerator.ts
packages/utils/src/eslint-utils/RuleCreator.ts
...s/utils/src/ast-utils/eslint-utils/astUtilities.ts
packages/utils/src/eslint-utils/deepMerge.ts
... and 190 more

[JoshuaKGoldberg]

👋 @djcsdy thanks for finding this bug and sending a PR! Much appreciated on the initiative! ❤️

As hinted by the PR template you used, we ask that all PRs address an existing issue. Would you be able to file an issue for this please? I'll still give the PR a review but if we end up discussing in more depth then that should be in an issue.

For the room: this gives bug reports more visibility and lets us comment on them before someone starts work on them. We'd want to confirm an issue is valid (this one certainly is) and what the right fix might be if it's not clear. Soon/eventually I/we/someone will write a GitHub bot to enforce it...

Side note: I re-opened #4010 and sent #4511 to make the PR template a little more clear.

[JoshuaKGoldberg]

Really nice find here. I shudder to think what other subtle edge cases have popped up because of that 😄

[djcsdy]

fwiw this issue can arise even without noUncheckedIndexAccess, but off the top of my head I can't think of an example that isn't totally contrived :-).

[JoshuaKGoldberg]

Oh? I was playing around with this for a good bit to try to find one but couldn't find anything. Is there a code snippet you have in mind?

[djcsdy]

I couldn't find one either to be honest, but I didn't try all that hard. Maybe it really is dependent on the flag. But even if it isn't I think this fix should suffice. I'll do some experimentation.

[JoshuaKGoldberg]

Generally looks good to me! And thank you for the detailed description; it greatly helped me -as someone who doesn't use noUncheckedIndexedAccess much- read what's going on. 😄

Just requesting changes on more testing coverage and deep union type checking. Filing an issue would be nice but I don't want to block this good PR on it -- if you don't have time I can.

[JoshuaKGoldberg]

Wonderful, thanks @djcsdy! This is great. 🔥

I'll go ahead and merge this now because this is a good fix (and the typo hurts me, heh). But if you do have another example -even contrived- please do post back and we can work on that too!