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): [non-nullable-type-assertion-style] fix false positive when asserting to a generic type that might be nullish #4509

Merged

Conversation

djcsdy
Copy link
Contributor

@djcsdy djcsdy commented Feb 3, 2022

PR Checklist

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
Copy link

nx-cloud bot commented Feb 3, 2022

☁️ 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

Sent with 💌 from NxCloud.

@typescript-eslint
Copy link
Contributor

typescript-eslint bot commented Feb 3, 2022

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
Copy link

netlify bot commented Feb 3, 2022

✔️ 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

@djcsdy djcsdy changed the title non-nullable-type-assertion-style: Don't complain when casting to a type parameter that might be nullish fix(eslint-plugin): non-nullable-type-assertion-style: Don't complain when casting to a type parameter that might be nullish Feb 3, 2022
@codecov
Copy link

codecov bot commented Feb 3, 2022

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
Copy link
Contributor

JoshuaKGoldberg commented Feb 3, 2022

👋 @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.

{
"extends": "./tsconfig.json",
"compilerOptions": {
"noUncheckedIndexedAccess": true
Copy link
Contributor

@JoshuaKGoldberg JoshuaKGoldberg Feb 3, 2022

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@djcsdy djcsdy Feb 4, 2022

Choose a reason for hiding this comment

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

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 :-).

Copy link
Contributor

@JoshuaKGoldberg JoshuaKGoldberg Feb 4, 2022

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

@djcsdy djcsdy Feb 7, 2022

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@JoshuaKGoldberg JoshuaKGoldberg left a comment

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 JoshuaKGoldberg added the awaiting response label Feb 4, 2022
@djcsdy djcsdy requested a review from JoshuaKGoldberg Feb 4, 2022
Copy link
Contributor

@JoshuaKGoldberg JoshuaKGoldberg left a comment

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!

@JoshuaKGoldberg JoshuaKGoldberg enabled auto-merge (squash) Feb 4, 2022
@JoshuaKGoldberg JoshuaKGoldberg removed the awaiting response label Feb 4, 2022
@bradzacher bradzacher added the bug label Feb 5, 2022
@bradzacher bradzacher changed the title fix(eslint-plugin): non-nullable-type-assertion-style: Don't complain when casting to a type parameter that might be nullish fix(eslint-plugin): [non-nullable-type-assertion-style] Don't complain when casting to a type parameter that might be nullish Feb 5, 2022
@bradzacher bradzacher disabled auto-merge Feb 5, 2022
@bradzacher bradzacher changed the title fix(eslint-plugin): [non-nullable-type-assertion-style] Don't complain when casting to a type parameter that might be nullish fix(eslint-plugin): [non-nullable-type-assertion-style] fix false positive when asserting to a generic type that might be nullish Feb 5, 2022
@bradzacher bradzacher merged commit 4209362 into typescript-eslint:main Feb 5, 2022
15 checks passed
@djcsdy djcsdy deleted the non-nullable-type-assertion-style branch Feb 7, 2022
lonyele pushed a commit to lonyele/typescript-eslint that referenced this issue Feb 12, 2022
…itive when asserting to a generic type that might be nullish (typescript-eslint#4509)
crapStone pushed a commit to Calciumdibromid/CaBr2 that referenced this issue Feb 16, 2022
This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [@typescript-eslint/eslint-plugin](https://github.com/typescript-eslint/typescript-eslint) | devDependencies | minor | [`5.10.2` -> `5.12.0`](https://renovatebot.com/diffs/npm/@typescript-eslint%2feslint-plugin/5.10.2/5.12.0) |
| [@typescript-eslint/parser](https://github.com/typescript-eslint/typescript-eslint) | devDependencies | minor | [`5.10.2` -> `5.12.0`](https://renovatebot.com/diffs/npm/@typescript-eslint%2fparser/5.10.2/5.12.0) |

---

### Release Notes

<details>
<summary>typescript-eslint/typescript-eslint (@&#8203;typescript-eslint/eslint-plugin)</summary>

### [`v5.12.0`](https://github.com/typescript-eslint/typescript-eslint/blob/HEAD/packages/eslint-plugin/CHANGELOG.md#&#8203;5120-httpsgithubcomtypescript-eslinttypescript-eslintcomparev5110v5120-2022-02-14)

[Compare Source](typescript-eslint/typescript-eslint@v5.11.0...v5.12.0)

##### Bug Fixes

-   **eslint-plugin:** \[init-declarations] fix nested namespace ([#&#8203;4544](typescript-eslint/typescript-eslint#4544)) ([fe910e6](typescript-eslint/typescript-eslint@fe910e6))
-   **eslint-plugin:** \[no-unnecessary-type-arguments] Use Symbol to check if it's the same type ([#&#8203;4543](typescript-eslint/typescript-eslint#4543)) ([5b7d8df](typescript-eslint/typescript-eslint@5b7d8df))
-   support nested object deconstructuring with type annotation ([#&#8203;4548](typescript-eslint/typescript-eslint#4548)) ([4da9278](typescript-eslint/typescript-eslint@4da9278))

##### Features

-   add checking property definition for allowNames option ([#&#8203;4542](typescript-eslint/typescript-eslint#4542)) ([e32bef6](typescript-eslint/typescript-eslint@e32bef6))

### [`v5.11.0`](https://github.com/typescript-eslint/typescript-eslint/blob/HEAD/packages/eslint-plugin/CHANGELOG.md#&#8203;5110-httpsgithubcomtypescript-eslinttypescript-eslintcomparev5102v5110-2022-02-07)

[Compare Source](typescript-eslint/typescript-eslint@v5.10.2...v5.11.0)

##### Bug Fixes

-   **eslint-plugin:** \[no-magic-numbers] fix invalid schema merging ([#&#8203;4517](typescript-eslint/typescript-eslint#4517)) ([b95f796](typescript-eslint/typescript-eslint@b95f796))
-   **eslint-plugin:** \[non-nullable-type-assertion-style] fix false positive when asserting to a generic type that might be nullish ([#&#8203;4509](typescript-eslint/typescript-eslint#4509)) ([4209362](typescript-eslint/typescript-eslint@4209362))

##### Features

-   **eslint-plugin:** \[explicit-function-return-type] add allowedNames ([#&#8203;4440](typescript-eslint/typescript-eslint#4440)) ([936e252](typescript-eslint/typescript-eslint@936e252))

#### [5.10.2](typescript-eslint/typescript-eslint@v5.10.1...v5.10.2) (2022-01-31)

##### Bug Fixes

-   **eslint-plugin:** \[no-restricted-imports] allow relative type imports with patterns configured ([#&#8203;4494](typescript-eslint/typescript-eslint#4494)) ([4a6d217](typescript-eslint/typescript-eslint@4a6d217))

#### [5.10.1](typescript-eslint/typescript-eslint@v5.10.0...v5.10.1) (2022-01-24)

**Note:** Version bump only for package [@&#8203;typescript-eslint/eslint-plugin](https://github.com/typescript-eslint/eslint-plugin)

</details>

<details>
<summary>typescript-eslint/typescript-eslint (@&#8203;typescript-eslint/parser)</summary>

### [`v5.12.0`](https://github.com/typescript-eslint/typescript-eslint/blob/HEAD/packages/parser/CHANGELOG.md#&#8203;5120-httpsgithubcomtypescript-eslinttypescript-eslintcomparev5110v5120-2022-02-14)

[Compare Source](typescript-eslint/typescript-eslint@v5.11.0...v5.12.0)

**Note:** Version bump only for package [@&#8203;typescript-eslint/parser](https://github.com/typescript-eslint/parser)

### [`v5.11.0`](https://github.com/typescript-eslint/typescript-eslint/blob/HEAD/packages/parser/CHANGELOG.md#&#8203;5110-httpsgithubcomtypescript-eslinttypescript-eslintcomparev5102v5110-2022-02-07)

[Compare Source](typescript-eslint/typescript-eslint@v5.10.2...v5.11.0)

**Note:** Version bump only for package [@&#8203;typescript-eslint/parser](https://github.com/typescript-eslint/parser)

#### [5.10.2](typescript-eslint/typescript-eslint@v5.10.1...v5.10.2) (2022-01-31)

**Note:** Version bump only for package [@&#8203;typescript-eslint/parser](https://github.com/typescript-eslint/parser)

#### [5.10.1](typescript-eslint/typescript-eslint@v5.10.0...v5.10.1) (2022-01-24)

**Note:** Version bump only for package [@&#8203;typescript-eslint/parser](https://github.com/typescript-eslint/parser)

</details>

---

### Configuration

📅 **Schedule**: At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

 **Rebasing**: Renovate will not automatically rebase this PR, because other commits have been found.

🔕 **Ignore**: Close this PR and you won't be reminded about these updates again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, click this checkbox.

---

This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).

Co-authored-by: cabr2-bot <cabr2.help@gmail.com>
Co-authored-by: crapStone <crapstone@noreply.codeberg.org>
Reviewed-on: https://codeberg.org/Calciumdibromid/CaBr2/pulls/1157
Reviewed-by: crapStone <crapstone@noreply.codeberg.org>
Co-authored-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>
Co-committed-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 27, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants