Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

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

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged

Conversation

ronami
Copy link
Member

@ronamironami commentedSep 24, 2024•
edited
Loading

PR Checklist

Overview

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

The rule forbids using type assertions to narrow a type:

functionf(){returnMath.random()<0.5 ?42 :'oops';}// Type `number | string` is not assignable to type `number` 🚫constz=f()asnumber;

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

constitems=[1,2,3,4];// Type `number` is assignable to type `number | undefined` ✅constnumber=items[0]asnumber|undefined;

Things to consider

Non null assertions

Non-null assertions are a way to narrow down a type, as long as the type includesnull orundefined:

declareconstfoo:()=>string|undefined;foo()!;

Reporting non-null assertions can create a lot of noise, especially on nested member expressions. Since there's already a rulethat 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, andnon-nullable-type-assertion-style will auto-fix between the two):

declareconstfoo:()=>string|undefined;foo()asstring;

Sinceno-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 andnon-nullable-type-assertion-style:

declareconstfoo:()=>string|undefined;foo()asstring;

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 aboutlong error messages, but I think it's worth discussing how types are displayed.

The following example:

interfaceFoo{type:'foo';}interfaceBar{type:'bar';}declareconstfoo:Foo|Bar;constbar=fooasBar;

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 tounknown, followed by whatever type it should be:

foo(barasunknownasnumber);

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 onmicrosoft/TypeScript#56448, which madeisTypeAssignableTo() 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.

kirkwaiblinger, Josh-Cena, phillip-le, jmargeta, controversial, Samuel-Therrien-Beslogic, TSMMark, nucleartux, and nicojs reacted with heart emojicontroversial, Samuel-Therrien-Beslogic, and TSMMark reacted with rocket emoji
@typescript-eslint
Copy link
Contributor

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 onhttps://opencollective.com/typescript-eslint.

@netlifyNetlify
Copy link

netlifybot commentedSep 24, 2024•
edited
Loading

✅ Deploy Preview fortypescript-eslint ready!

NameLink
🔨 Latest commit033fd0b
🔍 Latest deploy loghttps://app.netlify.com/sites/typescript-eslint/deploys/6736313f87da7c0007f7e450
😎 Deploy Previewhttps://deploy-preview-10051--typescript-eslint.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
Lighthouse
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 yourNetlify site configuration.

@nx-cloudNx Cloud
Copy link

nx-cloudbot commentedSep 24, 2024•
edited
Loading

☁️ Nx Cloud Report

CI is running/has finished running commands for commit033fd0b. 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 💌 fromNxCloud.

@ronamironami marked this pull request as ready for reviewSeptember 24, 2024 21:28
@codecovCodecov
Copy link

codecovbot commentedSep 24, 2024•
edited
Loading

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
FlagCoverage Δ
unittest86.62% <100.00%> (+0.44%)⬆️

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

Files with missing linesCoverage Δ
...slint-plugin/src/rules/no-unsafe-type-assertion.ts100.00% <100.00%> (ø)

... and93 files with indirect coverage changes

@ronamironami marked this pull request as draftSeptember 24, 2024 21:32
@ronamironamiforce-pushed theno-unsafe-type-assertion branch from9d898f9 to7c44606CompareSeptember 25, 2024 19:41
@ronamironami marked this pull request as ready for reviewSeptember 25, 2024 19:51
@ronamironami marked this pull request as draftSeptember 28, 2024 12:06
@ronamironami marked this pull request as ready for reviewSeptember 28, 2024 14:16
Copy link
Member

@kirkwaiblingerkirkwaiblinger left a comment

Choose a reason for hiding this comment

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

Great work on a first pass! And, I'm personallysuper 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 😁

ronami reacted with heart emoji
@kirkwaiblingerkirkwaiblinger added the awaiting responseIssues waiting for a reply from the OP or another party labelOct 10, 2024
Copy link
Member

@JoshuaKGoldbergJoshuaKGoldberg left a comment

Choose a reason for hiding this comment

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

I agree with Kirk 🙂

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

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

Choose a reason for hiding this comment

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

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

ronami reacted with thumbs up emoji
Copy link
Member

@JoshuaKGoldbergJoshuaKGoldberg left a comment

Choose a reason for hiding this comment

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

(wrong button...)

Copy link
Member

@JoshuaKGoldbergJoshuaKGoldberg left a comment

Choose a reason for hiding this comment

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

ronami reacted with thumbs up emoji
>
> 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.

Choose a reason for hiding this comment

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

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 sayingall type assertions are unsafe.

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

ronami reacted with thumbs up emoji
Copy link
MemberAuthor

Choose a reason for hiding this comment

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

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

@ronami
Copy link
MemberAuthor

Just posting a thought whilehttps://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, ishttps://github.com/typescript-eslint/typescript-eslint/pull/10051/files#r1828085145 pending for something from me?

@JoshuaKGoldberg
Copy link
Member

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

@ronami
Copy link
MemberAuthor

ronami commentedNov 10, 2024•
edited
Loading

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 thanyour recent request, I think the PR is ready for a re-review.

JoshuaKGoldberg reacted with thumbs up emojiJoshuaKGoldberg reacted with heart emojiJoshuaKGoldberg reacted with rocket emoji

Copy link
Member

@JoshuaKGoldbergJoshuaKGoldberg left a comment

Choose a reason for hiding this comment

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

🙌

Comment on lines 14 to 36
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));
}

Choose a reason for hiding this comment

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

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!

ronami reacted with thumbs up emoji
@JoshuaKGoldbergJoshuaKGoldberg added the awaiting responseIssues waiting for a reply from the OP or another party labelNov 10, 2024
@github-actionsgithub-actionsbot removed the awaiting responseIssues waiting for a reply from the OP or another party labelNov 10, 2024
@kirkwaiblinger
Copy link
Member

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

I think that's all resolved! 👍

Copy link
Member

@kirkwaiblingerkirkwaiblinger left a comment

Choose a reason for hiding this comment

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

brilliant, brilliant, brilliant! ❤️

brilliant

(this is an approval except for the typo)

ronami reacted with heart emojironami reacted with rocket emoji
@kirkwaiblingerkirkwaiblinger added the 1 approval>=1 team member has approved this PR; we're now leaving it open for more reviews before we merge labelNov 11, 2024
Co-authored-by: Kirk Waiblinger <kirk.waiblinger@gmail.com>
kirkwaiblinger
kirkwaiblinger previously approved these changesNov 11, 2024
JoshuaKGoldberg
JoshuaKGoldberg previously approved these changesNov 14, 2024
Copy link
Member

@JoshuaKGoldbergJoshuaKGoldberg left a comment

Choose a reason for hiding this comment

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

Whoohoo! Thanks!

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

compeek and TSMMark reacted with hooray emojironami, controversial, and TSMMark reacted with heart emoji
@JoshuaKGoldbergJoshuaKGoldberg dismissed stale reviews fromkirkwaiblinger and themself via033fd0bNovember 14, 2024 17:19
@JoshuaKGoldbergJoshuaKGoldberg merged commit9ebdff4 intotypescript-eslint:mainNov 14, 2024
62 checks passed
@ronamironami deleted the no-unsafe-type-assertion branchNovember 14, 2024 17:50
omril1 pushed a commit to omril1/typescript-eslint that referenced this pull requestNov 16, 2024
…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.mdxCo-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>
renovatebot added a commit to mmkal/eslint-plugin-mmkal that referenced this pull requestNov 18, 2024
##### [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.
renovatebot added a commit to andrei-picus-tink/auto-renovate that referenced this pull requestNov 19, 2024
| 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.
@github-actionsgithub-actionsbot locked asresolvedand limited conversation to collaboratorsNov 22, 2024
Sign up for freeto subscribe to this conversation on GitHub. Already have an account?Sign in.
Reviewers

@JoshuaKGoldbergJoshuaKGoldbergJoshuaKGoldberg left review comments

@kirkwaiblingerkirkwaiblingerkirkwaiblinger left review comments

Assignees
No one assigned
Labels
1 approval>=1 team member has approved this PR; we're now leaving it open for more reviews before we merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Rule proposal: no-unsafe-type-assertion
3 participants
@ronami@JoshuaKGoldberg@kirkwaiblinger
[8]ページ先頭
©2009-2025 Movatter.jp