feat(eslint-plugin): new rule `no-unsafe-type-assertion` #10051

ronami · 2024-09-24T21:13:56Z

PR Checklist

Addresses an existing open issue: fixes Rule proposal: no-unsafe-type-assertion #7173
That issue was marked as accepting prs
Steps in Contributing were taken

Overview

This PR addresses #7173 and introduces the new no-unsafe-type-assertion rule for using type assertions in a type-safe way.

The rule forbids using type assertions to narrow a type:

function f() {
  return Math.random() < 0.5 ? 42 : 'oops';
}

// Type `number | string` is not assignable to type `number` 🚫
const z = f() as number;

Instead, it only supports type casting for widening a type:

const items = [1, 2, 3, 4];

// Type `number` is assignable to type `number | undefined` ✅
const number = items[0] as number | undefined;

Things to consider

Non null assertions

Non-null assertions are a way to narrow down a type, as long as the type includes null or undefined:

declare const foo: () => string | undefined;
foo()!;

Reporting non-null assertions can create a lot of noise, especially on nested member expressions. Since there's already a rule that forbids non-null assertions, I've decided not to report these a second time.

Non-null assertions can be written differently as a type-cast (the two examples are semantically the same, and non-nullable-type-assertion-style will auto-fix between the two):

declare const foo: () => string | undefined;
foo() as string;

Since no-non-null-assertion doesn't cover the type-cast way, this rule will report it as unsafe, even though it's essentially the same as the non-null operator (which won't be reported).

This does mean, though, that usage like this will be reported by both this rule and non-nullable-type-assertion-style:

declare const foo: () => string | undefined;
foo() as string;

Possibly long lint errors

The current implementation displays the full names of both the type and the asserted type. If any of them is a big union, it can create long error messages:

Unsafe type assertion: type `string` is not assignable to type `"no-redundant-type-constituents" | "no-unsafe-type-assertion" | "no-unsafe-member-access" | "no-unsafe-assignment" | "naming-convention" | "adjacent-overload-signatures" | ... 121 more ... | "use-unknown-in-catch-callback-variable"`

While some of them can be reduced (by mimicking TypeScript's errors and only showing the most specific type that doesn't match), some error messages may be truncated.

In some cases, this can get way out of hand. For example, this type assertion has the following lint error:

Unsafe type assertion: type `NonNullable<ClassDeclarationWithName | ClassDeclarationWithOptionalName | FunctionDeclarationWithName | ... 199 more ... | undefined>` is not assignable to type `TSModuleDeclaration`

I think this is too verbose, but I see many other rules using similar logic to display type names.

What's your opinion on this?

Testing this on a large project

I've run the rule locally on @typescript-eslint's repo. There are 98 failures; most seem valid, but I found some issues. Most issues I've encountered are related to how straightforward (or unclear) the error message is rather than its correctness.

I wrote about long error messages, but I think it's worth discussing how types are displayed.

The following example:

interface Foo {
  type: 'foo';
}
interface Bar {
  type: 'bar';
}

declare const foo: Foo | Bar;
const bar = foo as Bar;

Will fail with:

Unsafe type assertion: type `Foo | Bar` is not assignable to type `Bar`

Types are referenced by their name (same for type constraints) rather than explaining why these types don't match.

I'll be happy to hear your opinions on this.

Forbids a common pattern

A common (though unsafe) pattern is to cast a value to unknown, followed by whatever type it should be:

foo(bar as unknown as number);

This escape hatch lets developers cast a value explicitly to something else, knowing it's unsafe.

I think it's reasonable to have an option not to report such cases so the developer doesn't have to use an eslint-ignore comment in addition to a very verbose assertion.

Relies on TypeScript 5.4 or above

This rule relies on microsoft/TypeScript#56448, which made isTypeAssignableTo() public and can't be used with earlier versions of TypeScript.

I added a note about this in the docs, but I don't know if there should be a runtime check along with it. I couldn't find anything on similar rules.

typescript-eslint · 2024-09-24T21:13:59Z

Thanks for the PR, @ronami!

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.

netlify · 2024-09-24T21:14:11Z

✅ Deploy Preview for typescript-eslint ready!

Name	Link
🔨 Latest commit	`033fd0b`
🔍 Latest deploy log	https://app.netlify.com/sites/typescript-eslint/deploys/6736313f87da7c0007f7e450
😎 Deploy Preview	https://deploy-preview-10051--typescript-eslint.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.
Lighthouse	1 paths audited Performance: 96 (🔴 down 3 from production) Accessibility: 100 (no change from production) Best Practices: 92 (no change from production) SEO: 98 (no change from production) PWA: 80 (no change from production) View the detailed breakdown and full score reports

To edit notification comments on pull requests, go to your Netlify site configuration.

nx-cloud · 2024-09-24T21:14:51Z

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 033fd0b. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this CI Pipeline Execution

✅ Successfully ran 2 targets

Sent with 💌 from NxCloud.

codecov · 2024-09-24T21:29:44Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 86.62%. Comparing base (be5bc0f) to head (033fd0b).
Report is 88 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #10051      +/-   ##
==========================================
+ Coverage   86.18%   86.62%   +0.44%     
==========================================
  Files         428      433       +5     
  Lines       14989    15193     +204     
  Branches     4345     4434      +89     
==========================================
+ Hits        12918    13161     +243     
+ Misses       1725     1675      -50     
- Partials      346      357      +11

Flag	Coverage Δ
unittest	`86.62% <100.00%> (+0.44%)`	⬆️

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

Files with missing lines	Coverage Δ
...slint-plugin/src/rules/no-unsafe-type-assertion.ts	`100.00% <100.00%> (ø)`

... and 93 files with indirect coverage changes

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

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

kirkwaiblinger

Great work on a first pass! And, I'm personally super excited for this to be getting worked on. 🙂

Looking forward to seeing where the discussions on all the complicated edge cases go! Hopefully I've given you plenty to think about 😁

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

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

packages/eslint-plugin/docs/rules/no-unsafe-type-assertion.mdx

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

JoshuaKGoldberg

I agree with Kirk 🙂

Love how straightforward this implementation is... and it pains me to say we might need to make it more complicated.

JoshuaKGoldberg · 2024-10-10T15:05:19Z

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

+    },
+    messages: {
+      unsafeTypeAssertion:
+        'Unsafe type assertion: type `{{type}}` is not assignable to type `{{asserted}}`',


Thinking on the long error messages: we haven't had to really solve this problem before, which is why we don't have good helpers. Most of our error messages either don't mention types or only refer to small names such as identifiers.

For this rule, I think we can generally assume that the explicitly written type should be simple enough to report with. What do you think about this?

Unsafe type assertion: type '{{type}}' is more narrow than the original type.

I don't love "original type" but 🤷 can't think of anything nicer...

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

JoshuaKGoldberg

(wrong button...)

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

JoshuaKGoldberg

Just posting a thought while https://github.com/typescript-eslint/typescript-eslint/pull/10051/files#r1828085145 is pending.

JoshuaKGoldberg · 2024-11-10T16:54:25Z

packages/eslint-plugin/docs/rules/no-unsafe-type-assertion.mdx

+>
+> See **https://typescript-eslint.io/rules/no-unsafe-type-assertion** for documentation.
+
+This rule forbids using type assertions to narrow a type, as it can lead to unsafe assumptions that bypass TypeScript’s type-checking.


unsafe assumptions

What unsafe assumptions?

Someone reading this who isn't already familiar with the concepts wouldn't likely understand what it means. I think it'd even be easy to misinterpret this as saying all type assertions are unsafe.

Could you add a section to this page explaining this for someone who doesn't already understand?

I definitely agree! I've changed/expanded the opening paragraph to explain this better. Wdyt?

ronami · 2024-11-10T18:42:05Z

Just posting a thought while https://github.com/typescript-eslint/typescript-eslint/pull/10051/files#r1828085145 is pending.

Thanks @JoshuaKGoldberg; I'll update this section in a bit. To make sure, is https://github.com/typescript-eslint/typescript-eslint/pull/10051/files#r1828085145 pending for something from me?

JoshuaKGoldberg · 2024-11-10T18:54:55Z

Last I checked it was requesting more test coverage. If that's done then we can re-review. 👍

ronami · 2024-11-10T19:13:48Z

Last I checked it was requesting more test coverage. If that's done then we can re-review. 👍

The PR has gone through several changes since then (with the help of @kirkwaiblinger ❤️). Other than your recent request, I think the PR is ready for a re-review.

JoshuaKGoldberg

🙌

JoshuaKGoldberg · 2024-11-10T20:03:40Z

packages/eslint-plugin/src/util/isTypeUnchanged.ts

+  if (
+    isTypeFlagSet(uncast, ts.TypeFlags.Undefined) &&
+    isTypeFlagSet(cast, ts.TypeFlags.Undefined) &&
+    tsutils.isCompilerOptionEnabled(
+      compilerOptions,
+      'exactOptionalPropertyTypes',
+    )
+  ) {
+    const uncastParts = tsutils
+      .unionTypeParts(uncast)
+      .filter(part => !isTypeFlagSet(part, ts.TypeFlags.Undefined));
+
+    const castParts = tsutils
+      .unionTypeParts(cast)
+      .filter(part => !isTypeFlagSet(part, ts.TypeFlags.Undefined));
+
+    if (uncastParts.length !== castParts.length) {
+      return false;
+    }
+
+    const uncastPartsSet = new Set(uncastParts);
+    return castParts.every(part => uncastPartsSet.has(part));
+  }


I think if there's a test case that passes only with this logic, then yeah, let's use it. Otherwise just a === is sufficient IMO.

Great extract+reuse!

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

kirkwaiblinger · 2024-11-11T21:27:35Z

To make sure, is https://github.com/typescript-eslint/typescript-eslint/pull/10051/files#r1828085145 pending for something from me?

I think that's all resolved! 👍

kirkwaiblinger

brilliant, brilliant, brilliant! ❤️

(this is an approval except for the typo)

packages/eslint-plugin/docs/rules/no-unsafe-type-assertion.mdx

Co-authored-by: Kirk Waiblinger <kirk.waiblinger@gmail.com>

JoshuaKGoldberg

Whoohoo! Thanks!

Really excited to finally have a meaningful rule around unsafe assertions in. This has been a sore spot for so long.

…eslint#10051) * initial implementation * tests * docs * more tests * use checker.typeToString() over getTypeName() * use link * oops * add tests * remove unnecessary typescript 5.4 warning * adjust format to new rules * update error message to be more concise * match implementation to be inline with no-unsafe-* rules * rework tests * refactor * update snapshots * fix error message showing original type instead of asserted type * update snapshots * add a warning for object stubbing on test files * fix linting * adjust test to lint fixes * simplify type comparison * rework code-comments and rename variables * rework the opening paragraph to make it more beginner-friendly * Update packages/eslint-plugin/docs/rules/no-unsafe-type-assertion.mdx Co-authored-by: Kirk Waiblinger <kirk.waiblinger@gmail.com> * fix: narrow/widen in description --------- Co-authored-by: Kirk Waiblinger <kirk.waiblinger@gmail.com> Co-authored-by: Josh Goldberg <git@joshuakgoldberg.com>

@developer-bandi

##### [v8.15.0](https://github.com/typescript-eslint/typescript-eslint/blob/HEAD/packages/eslint-plugin/CHANGELOG.md#8150-2024-11-18) ##### 🚀 Features - **eslint-plugin:** \[prefer-nullish-coalescing] fix detection of `ignoreConditionalTests` involving boolean `!` operator ([#10299](typescript-eslint/typescript-eslint#10299)) - **eslint-plugin:** new rule `no-unsafe-type-assertion` ([#10051](typescript-eslint/typescript-eslint#10051)) - **eslint-plugin:** added related-getter-setter-pairs rule ([#10192](typescript-eslint/typescript-eslint#10192)) ##### 🩹 Fixes - **utils:** add defaultOptions to meta in rule ([#10339](typescript-eslint/typescript-eslint#10339)) - **eslint-plugin:** report deprecations used in default export ([#10330](typescript-eslint/typescript-eslint#10330)) - **eslint-plugin:** \[explicit-module-boundary-types] and \[explicit-function-return-type] don't report on `as const satisfies` ([#10315](typescript-eslint/typescript-eslint#10315)) - **eslint-plugin:** \[await-thenable, return-await] don't flag awaiting unconstrained type parameter as unnecessary ([#10314](typescript-eslint/typescript-eslint#10314)) - **eslint-plugin:** \[consistent-indexed-object-style] handle circular mapped types ([#10301](typescript-eslint/typescript-eslint#10301)) ##### ❤️ Thank You - Josh Goldberg ✨ - Kim Sang Du [@developer-bandi](https://github.com/developer-bandi) - Luis Sebastian Urrutia Fuentes [@LuisUrrutia](https://github.com/LuisUrrutia) - Phillip Huang - Ronen Amiel - Szydlak [@wszydlak](https://github.com/wszydlak) You can read about our [versioning strategy](https://main--typescript-eslint.netlify.app/users/versioning) and [releases](https://main--typescript-eslint.netlify.app/users/releases) on our website.

@developer-bandi

| datasource | package | from | to | | ---------- | -------------------------------- | ------ | ------ | | npm | @typescript-eslint/eslint-plugin | 8.14.0 | 8.15.0 | | npm | @typescript-eslint/parser | 8.14.0 | 8.15.0 | ## [v8.15.0](https://github.com/typescript-eslint/typescript-eslint/blob/HEAD/packages/eslint-plugin/CHANGELOG.md#8150-2024-11-18) ##### 🚀 Features - **eslint-plugin:** \[prefer-nullish-coalescing] fix detection of `ignoreConditionalTests` involving boolean `!` operator ([#10299](typescript-eslint/typescript-eslint#10299)) - **eslint-plugin:** new rule `no-unsafe-type-assertion` ([#10051](typescript-eslint/typescript-eslint#10051)) - **eslint-plugin:** added related-getter-setter-pairs rule ([#10192](typescript-eslint/typescript-eslint#10192)) ##### 🩹 Fixes - **utils:** add defaultOptions to meta in rule ([#10339](typescript-eslint/typescript-eslint#10339)) - **eslint-plugin:** report deprecations used in default export ([#10330](typescript-eslint/typescript-eslint#10330)) - **eslint-plugin:** \[explicit-module-boundary-types] and \[explicit-function-return-type] don't report on `as const satisfies` ([#10315](typescript-eslint/typescript-eslint#10315)) - **eslint-plugin:** \[await-thenable, return-await] don't flag awaiting unconstrained type parameter as unnecessary ([#10314](typescript-eslint/typescript-eslint#10314)) - **eslint-plugin:** \[consistent-indexed-object-style] handle circular mapped types ([#10301](typescript-eslint/typescript-eslint#10301)) ##### ❤️ Thank You - Josh Goldberg ✨ - Kim Sang Du [@developer-bandi](https://github.com/developer-bandi) - Luis Sebastian Urrutia Fuentes [@LuisUrrutia](https://github.com/LuisUrrutia) - Phillip Huang - Ronen Amiel - Szydlak [@wszydlak](https://github.com/wszydlak) You can read about our [versioning strategy](https://main--typescript-eslint.netlify.app/users/versioning) and [releases](https://main--typescript-eslint.netlify.app/users/releases) on our website.

ronami marked this pull request as ready for review September 24, 2024 21:28

ronami marked this pull request as draft September 24, 2024 21:32

ronami added 5 commits September 25, 2024 21:50

initial implementation

8f38cec

tests

037978c

docs

a50625b

more tests

15ce7f8

use checker.typeToString() over getTypeName()

7c44606

ronami force-pushed the no-unsafe-type-assertion branch from 9d898f9 to 7c44606 Compare September 25, 2024 19:41

ronami commented Sep 25, 2024

View reviewed changes

packages/eslint-plugin/src/rules/no-unsafe-type-assertion.ts Outdated Show resolved Hide resolved

ronami commented Sep 25, 2024

View reviewed changes

packages/eslint-plugin/tests/rules/no-unsafe-type-assertion.test.ts Outdated Show resolved Hide resolved

ronami marked this pull request as ready for review September 25, 2024 19:51

ronami added 2 commits September 25, 2024 22:54

use link

cc4ea8b

oops

e31cf39

ronami marked this pull request as draft September 28, 2024 12:06

ronami added 2 commits September 28, 2024 16:31

add tests

8a8965f

Merge branch 'main' into no-unsafe-type-assertion

5bbe0c9

ronami marked this pull request as ready for review September 28, 2024 14:16

kirkwaiblinger requested changes Oct 9, 2024

View reviewed changes

kirkwaiblinger added the awaiting response Issues waiting for a reply from the OP or another party label Oct 10, 2024

JoshuaKGoldberg approved these changes Oct 10, 2024

View reviewed changes

JoshuaKGoldberg requested changes Oct 10, 2024

View reviewed changes

ronami added 5 commits October 11, 2024 21:10

Merge branch 'main' into no-unsafe-type-assertion

f0c429e

remove unnecessary typescript 5.4 warning

08577a8

adjust format to new rules

2ab1c87

Merge branch 'main' into no-unsafe-type-assertion

8f29e89

update error message to be more concise

a5dbd5b

kirkwaiblinger reviewed Oct 27, 2024

View reviewed changes

packages/eslint-plugin/src/rules/no-unsafe-type-assertion.ts Outdated Show resolved Hide resolved

ronami mentioned this pull request Nov 4, 2024

Enhancement: [no-unsafe-*] misses any violations on union and object types #10158

Closed

4 tasks

JoshuaKGoldberg reviewed Nov 10, 2024

View reviewed changes

JoshuaKGoldberg requested changes Nov 10, 2024

View reviewed changes

JoshuaKGoldberg added the awaiting response Issues waiting for a reply from the OP or another party label Nov 10, 2024

ronami added 3 commits November 10, 2024 22:54

simplify type comparison

9980c9e

rework code-comments and rename variables

8b0912f

rework the opening paragraph to make it more beginner-friendly

07ae890

ronami requested review from JoshuaKGoldberg and kirkwaiblinger November 10, 2024 21:45

github-actions bot removed the awaiting response Issues waiting for a reply from the OP or another party label Nov 10, 2024

kirkwaiblinger requested changes Nov 11, 2024

View reviewed changes

packages/eslint-plugin/docs/rules/no-unsafe-type-assertion.mdx Outdated Show resolved Hide resolved

kirkwaiblinger added the 1 approval >=1 team member has approved this PR; we're now leaving it open for more reviews before we merge label Nov 11, 2024

Update packages/eslint-plugin/docs/rules/no-unsafe-type-assertion.mdx

5d8c521

Co-authored-by: Kirk Waiblinger <kirk.waiblinger@gmail.com>

kirkwaiblinger previously approved these changes Nov 11, 2024

View reviewed changes

JoshuaKGoldberg previously approved these changes Nov 14, 2024

View reviewed changes

fix: narrow/widen in description

033fd0b

JoshuaKGoldberg dismissed stale reviews from kirkwaiblinger and themself via 033fd0b November 14, 2024 17:19

JoshuaKGoldberg merged commit 9ebdff4 into typescript-eslint:main Nov 14, 2024
62 checks passed

ronami deleted the no-unsafe-type-assertion branch November 14, 2024 17:50

JoshuaKGoldberg mentioned this pull request Nov 20, 2024

Docs: Add blog post about history and usage of TypeScript's isTypeAssignableTo API #10364

Open

2 tasks

github-actions bot locked as resolved and limited conversation to collaborators Nov 22, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(eslint-plugin): new rule `no-unsafe-type-assertion` #10051

feat(eslint-plugin): new rule `no-unsafe-type-assertion` #10051

ronami commented Sep 24, 2024 •

edited

Loading

typescript-eslint bot commented Sep 24, 2024

netlify bot commented Sep 24, 2024 •

edited

Loading

nx-cloud bot commented Sep 24, 2024 •

edited

Loading

codecov bot commented Sep 24, 2024 •

edited

Loading

kirkwaiblinger left a comment

JoshuaKGoldberg left a comment

JoshuaKGoldberg Oct 10, 2024

JoshuaKGoldberg left a comment

JoshuaKGoldberg left a comment

JoshuaKGoldberg Nov 10, 2024

ronami Nov 10, 2024

ronami commented Nov 10, 2024

JoshuaKGoldberg commented Nov 10, 2024

ronami commented Nov 10, 2024 •

edited

Loading

JoshuaKGoldberg left a comment

JoshuaKGoldberg Nov 10, 2024

kirkwaiblinger commented Nov 11, 2024

kirkwaiblinger left a comment

JoshuaKGoldberg left a comment

feat(eslint-plugin): new rule no-unsafe-type-assertion #10051

feat(eslint-plugin): new rule no-unsafe-type-assertion #10051

Conversation

ronami commented Sep 24, 2024 • edited Loading

PR Checklist

Overview

Things to consider

Non null assertions

Possibly long lint errors

Testing this on a large project

Forbids a common pattern

Relies on TypeScript 5.4 or above

typescript-eslint bot commented Sep 24, 2024

netlify bot commented Sep 24, 2024 • edited Loading

✅ Deploy Preview for typescript-eslint ready!

nx-cloud bot commented Sep 24, 2024 • edited Loading

☁️ Nx Cloud Report

codecov bot commented Sep 24, 2024 • edited Loading

Codecov Report

kirkwaiblinger left a comment

Choose a reason for hiding this comment

JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

JoshuaKGoldberg Oct 10, 2024

Choose a reason for hiding this comment

JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

JoshuaKGoldberg Nov 10, 2024

Choose a reason for hiding this comment

ronami Nov 10, 2024

Choose a reason for hiding this comment

ronami commented Nov 10, 2024

JoshuaKGoldberg commented Nov 10, 2024

ronami commented Nov 10, 2024 • edited Loading

JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

JoshuaKGoldberg Nov 10, 2024

Choose a reason for hiding this comment

kirkwaiblinger commented Nov 11, 2024

kirkwaiblinger left a comment

Choose a reason for hiding this comment

JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

feat(eslint-plugin): new rule `no-unsafe-type-assertion` #10051

feat(eslint-plugin): new rule `no-unsafe-type-assertion` #10051

ronami commented Sep 24, 2024 •

edited

Loading

netlify bot commented Sep 24, 2024 •

edited

Loading

nx-cloud bot commented Sep 24, 2024 •

edited

Loading

codecov bot commented Sep 24, 2024 •

edited

Loading

ronami commented Nov 10, 2024 •

edited

Loading