fix(eslint-plugin): [no-unsafe-type-assertion] fix for unsafe assertion to a constrained type parameter #10461

controversial · 2024-12-05T18:09:56Z

PR Checklist

Addresses an existing open issue: fixes Bug: [no-unsafe-type-assertion] Fails to report unsafe assertions involving constrained type parameters #10453
That issue was marked as accepting prs
Steps in Contributing were taken

Overview

Attempts to fix the issue described in #10453:

type Obj = { foo: string };
function func<T extends Obj>() {
  const o: Obj = { foo: 'hi' };
  // this type assertion is unsafe because T could be instantiated with a more specific subtype of Obj
  // no-unsafe-type-assertion should (but didn’t) report this
  const myObj = o as T;
}

After experimenting with the logic in no-unsafe-type-assertion, I found that obtaining the “types to check” using services.getTypeAtLocation in place of getConstrainedTypeAtLocation (i.e. omitting a call to getBaseConstraintOfType) allows the subsequent isTypeAssignableTo check to correctly catch this case.

Additionally, I added a new error message for this case: if the “expression type” is not assignable to the “asserted type”, but the asserted type has a constraint to which the expression type is assignable, the rule will report the following:

Unsafe type assertion: the original type is assignable to the constraint of type '{{type}}', but '{{type}}' could be instantiated with a different subtype of its constraint.

This is my first contribution to this project and I’m broadly unfamiliar with both ESLint plugin development and TypeScript internals, so it’s very possible that I’m missing a hidden implication of this change—i.e. I’m not sure if there was an important reason to use getConstrainedTypeAtLocation in the original implementation. However, the existing tests for this rule are still passing.

…ric types

…rtion

typescript-eslint · 2024-12-05T18:09:59Z

Thanks for the PR, @controversial!

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-12-05T18:10:18Z

✅ Deploy Preview for typescript-eslint ready!

Name	Link
🔨 Latest commit	`239f3df`
🔍 Latest deploy log	https://app.netlify.com/sites/typescript-eslint/deploys/6761bd5ffc147e0008e6d79d
😎 Deploy Preview	https://deploy-preview-10461--typescript-eslint.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.
Lighthouse	1 paths audited Performance: 92 (🟢 up 14 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-12-05T18:10:52Z

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 239f3df. 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 32 targets

Sent with 💌 from NxCloud.

codecov · 2024-12-05T18:35:04Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 86.79%. Comparing base (b2ce158) to head (239f3df).
Report is 10 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #10461   +/-   ##
=======================================
  Coverage   86.78%   86.79%           
=======================================
  Files         445      445           
  Lines       15366    15376   +10     
  Branches     4475     4478    +3     
=======================================
+ Hits        13336    13346   +10     
  Misses       1675     1675           
  Partials      355      355

Flag	Coverage Δ
unittest	`86.79% <100.00%> (+<0.01%)`	⬆️

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 1 file with indirect coverage changes

controversial · 2024-12-05T19:25:18Z

With respect to a custom error message, @kirkwaiblinger suggested:

As a first pass, I think we could just model it after the TS error message?

Unsafe type assertion: 'Object' is assignable to the constraint of type 'CustomObjectT', but 'CustomObjectT' could be instantiated with a different subtype of constraint 'Object'.

In the original PR of this rule, #10051, a discussion about the appropriate verbosity of the error message resolved to omit any types other than the explicitly written type from the error message in order to prevent excessively verbose errors.

With that in mind, I’m starting out with the following error message:

Unsafe type assertion: the original type is assignable to the constraint of type 'CustomObjectT', but 'CustomObjectT' could be instantiated with a different subtype of its constraint.

I’m happy to amend this message to be more verbose if we decide to go that way, though!

kirkwaiblinger

What a great first PR to typescript-eslint! This is very much on the right track, and in a tricky area of the codebase, too.

Left a few comments and questions.

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

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

kirkwaiblinger · 2024-12-06T19:57:38Z

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

+        checker.getBaseConstraintOfType(assertedType);
+      const isAssignableToConstraint =
+        !!assertedTypeConstraint &&
+        checker.isTypeAssignableTo(


(no specific changes requested in the following)

While I was thinking deeply, it occurred to make that I don't think that asserting from any concrete type to any type parameter is ever safe, so I was wondering whether we could gain some performance by checking for this early and avoiding any checker.isTypeAssignableTo() checks..

This would sacrifice the ability to have distinct error messages for whether it's not assignable because of ordinary unassignability (string | number as T where T extends string) or due to the generic subtype instantiation (string as T where T extends string | number), it could potentially be mitigated by a separate error message just saying A concrete type can never be safely assigned to a type parameter (or similar).

However, I think we still need the current infrastructure in order to deal with the case of assignment from one type parameter to another type parameter (since T -> V where V extends T is ok), so I'm leaning against going that route, i.e. leaning towards your current strategy. If you want to give this a thought, though, I'm curious for your opinion as well!

…ssage + light refactor to prefer early returns over nested conditionals

…r extends unconstrained parameter”

controversial · 2024-12-06T22:01:16Z

Thanks so much for the thoughtful review @kirkwaiblinger !!

I added the test cases from your playground and addressed the missing cases. For the missing case of unconstrained type parameters, I added another message to match the one Typescript uses:

Unsafe type assertion: '{{type}}' could be instantiated with an arbitrary type which could be unrelated to the original type.

Here’s an updated playground link!

The only test case that tripped me up a bit was the one you had called parameterExtendsOtherParameter:

function parameterExtendsOtherParameter<T, V extends T>(x: T, y: V) {
  x = y;
  y as T; // allowed

  y = x;
  x as V; // banned; generic arbitrary subtype <-- currently has wrong error message.
}

My first impression was that this case should raise the “x is assignable to the constraint of V” message. However, my current implementation instead says “V could be instantiated with an arbitrary type.” Notably, though (and somewhat to my surprise) this behavior is consistent with the message that Typescript gives for this case, so I think it’s probably ok as-is?

I added a new parameterExtendsOtherParameter test where T is not unconstrained; here, both TypeScript (for assignment) and my implementation raise “is assignable to the constraint of V” like I expected. I renamed the original test to parameterExtendsUnconstrainedParameter.

I’m curious to hear your thoughts about whether we should try to produce “assignable to the constraint” for this edge case?

controversial · 2024-12-16T21:22:06Z

Merged the latest main to resolve merge conflicts from #10472

When you get a chance, let me know if there’s anything else you need from me @kirkwaiblinger :)

kirkwaiblinger · 2024-12-17T02:47:49Z

My first impression was that this case should raise the “x is assignable to the constraint of V” message. However, my current implementation instead says “V could be instantiated with an arbitrary type.” Notably, though (and somewhat to my surprise) this behavior is consistent with the message that Typescript gives for this case, so I think it’s probably ok as-is?

I feel strongly that TS is wrong here, since V plainly cannot be unrelated to T. It's guaranteed to be a subtype of T, since... V extends T 🤷 . However, I agree that it makes sense to match their behavior here, and we can follow up by filing an issue to TS if we like. (Feel free to do this if you're interested! Otherwise I probably will at some point 🙂)

I added a new parameterExtendsOtherParameter test where T is not unconstrained; here, both TypeScript (for assignment) and my implementation raise “is assignable to the constraint of V” like I expected. I renamed the original test to parameterExtends**Unconstrained**Parameter.

🎉

kirkwaiblinger

Really outstanding work! Great attention to detail on noticing the additional error message, nicely done on pushing back where it made sense to do so!

Trivial cleanup requested but this is otherwise an enthusiastic approval from me! 🙂 We'll probably leave this open for a little bit as well for another team member to take a look if they like, since it's nuanced stuff.

Thanks!

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

unit tests should only test one thing

controversial · 2024-12-17T16:38:16Z

cleanup completed! thanks again for your thoughtful reviews, excited to have this merged at some point :)

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

@controversial

| datasource | package | from | to | | ---------- | -------------------------------- | ------ | ------ | | npm | @typescript-eslint/eslint-plugin | 8.18.1 | 8.18.2 | | npm | @typescript-eslint/parser | 8.18.1 | 8.18.2 | ## [v8.18.2](https://github.com/typescript-eslint/typescript-eslint/blob/HEAD/packages/eslint-plugin/CHANGELOG.md#8182-2024-12-23) ##### 🩹 Fixes - **eslint-plugin:** \[no-unnecessary-condition] handle noUncheckedIndexedAccess true ([#10514](typescript-eslint/typescript-eslint#10514)) - **eslint-plugin:** \[consistent-type-assertions] allow default assertionStyle option ([#10512](typescript-eslint/typescript-eslint#10512)) - **eslint-plugin:** \[no-unnecessary-type-arguments] handle type/value context ([#10503](typescript-eslint/typescript-eslint#10503)) - **eslint-plugin:** \[no-unsafe-type-assertion] fix for unsafe assertion to a constrained type parameter ([#10461](typescript-eslint/typescript-eslint#10461)) - **eslint-plugin:** \[consistent-indexed-object-style] use a suggestion over an auto-fix if can't reliably determine that produced index signature is valid ([#10490](typescript-eslint/typescript-eslint#10490)) - **eslint-plugin:** \[no-unnecessary-condition] don't flag values of an unconstrained or valid type parameter ([#10473](typescript-eslint/typescript-eslint#10473)) - **eslint-plugin:** \[prefer-reduce-type-parameter] don't report cases in which the fix results in a type error ([#10494](typescript-eslint/typescript-eslint#10494)) - **eslint-plugin:** \[no-deprecated] not reporting usages of deprecated declared constants as object value ([#10498](typescript-eslint/typescript-eslint#10498)) ##### ❤️ Thank You - Luke Deen Taylor [@controversial](https://github.com/controversial) - Ronen Amiel - Scott O'Hara - YeonJuan [@yeonjuan](https://github.com/yeonjuan) - Yukihiro Hasegawa [@y-hsgw](https://github.com/y-hsgw) 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.

@controversial

##### [v8.18.2](https://github.com/typescript-eslint/typescript-eslint/blob/HEAD/packages/eslint-plugin/CHANGELOG.md#8182-2024-12-23) ##### 🩹 Fixes - **eslint-plugin:** \[no-unnecessary-condition] handle noUncheckedIndexedAccess true ([#10514](typescript-eslint/typescript-eslint#10514)) - **eslint-plugin:** \[consistent-type-assertions] allow default assertionStyle option ([#10512](typescript-eslint/typescript-eslint#10512)) - **eslint-plugin:** \[no-unnecessary-type-arguments] handle type/value context ([#10503](typescript-eslint/typescript-eslint#10503)) - **eslint-plugin:** \[no-unsafe-type-assertion] fix for unsafe assertion to a constrained type parameter ([#10461](typescript-eslint/typescript-eslint#10461)) - **eslint-plugin:** \[consistent-indexed-object-style] use a suggestion over an auto-fix if can't reliably determine that produced index signature is valid ([#10490](typescript-eslint/typescript-eslint#10490)) - **eslint-plugin:** \[no-unnecessary-condition] don't flag values of an unconstrained or valid type parameter ([#10473](typescript-eslint/typescript-eslint#10473)) - **eslint-plugin:** \[prefer-reduce-type-parameter] don't report cases in which the fix results in a type error ([#10494](typescript-eslint/typescript-eslint#10494)) - **eslint-plugin:** \[no-deprecated] not reporting usages of deprecated declared constants as object value ([#10498](typescript-eslint/typescript-eslint#10498)) ##### ❤️ Thank You - Luke Deen Taylor [@controversial](https://github.com/controversial) - Ronen Amiel - Scott O'Hara - YeonJuan [@yeonjuan](https://github.com/yeonjuan) - Yukihiro Hasegawa [@y-hsgw](https://github.com/y-hsgw) 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.

controversial added 2 commits December 5, 2024 12:57

Add failing test case for unsafe type assertion with constrained gene…

7529400

…ric types

Use actual type instead of “constrained type” for no-unsafe-type-asse…

9117da0

…rtion

controversial mentioned this pull request Dec 5, 2024

Bug: [no-unsafe-type-assertion] Fails to report unsafe assertions involving constrained type parameters #10453

Closed

4 tasks

controversial changed the title ~~fix(eslint-plugin): [no-unsafe-type-assertion] Fix for type parameters with generic constraints~~ fix(eslint-plugin): [no-unsafe-type-assertion] fix for type parameters with generic constraints Dec 5, 2024

tweak test cases

ece33d8

Custom error message for “assignable to the constraint” case

354dc32

controversial added 3 commits December 5, 2024 14:35

rename “assignable to the constraint” error message

05bdf78

spelling

3578532

tweak message

a1cf417

controversial changed the title ~~fix(eslint-plugin): [no-unsafe-type-assertion] fix for type parameters with generic constraints~~ fix(eslint-plugin): [no-unsafe-type-assertion] fix for unsafe assertion to a constrained type parameter Dec 5, 2024

kirkwaiblinger requested changes Dec 6, 2024

View reviewed changes

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

controversial added 5 commits December 6, 2024 15:58

Add new test cases from @kirkwaiblinger

d7a7600

Move isAssignableToConstraint check into isAssertionSafe

60cba85

Properly handle unconstrained type parameters, including new error me…

e560ef3

…ssage + light refactor to prefer early returns over nested conditionals

distinguish between “parameter extends other parameter” and “paramete…

c39c4b7

…r extends unconstrained parameter”

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

3c60099

controversial requested a review from kirkwaiblinger December 6, 2024 22:24

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

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

7719c45

kirkwaiblinger requested changes Dec 17, 2024

View reviewed changes

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

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

kirkwaiblinger requested a review from a team December 17, 2024 03:01

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 Dec 17, 2024

clean up extra pieces in unit tests

504df89

unit tests should only test one thing

test lines

239f3df

kirkwaiblinger approved these changes Dec 17, 2024

View reviewed changes

kirkwaiblinger requested a review from a team December 17, 2024 18:06

github-actions bot removed the 1 approval >=1 team member has approved this PR; we're now leaving it open for more reviews before we merge label Dec 17, 2024

controversial commented Dec 17, 2024

View reviewed changes

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

JoshuaKGoldberg merged commit 4c91ed5 into typescript-eslint:main Dec 21, 2024
62 of 63 checks passed

github-actions bot locked as resolved and limited conversation to collaborators Dec 30, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(eslint-plugin): [no-unsafe-type-assertion] fix for unsafe assertion to a constrained type parameter #10461

fix(eslint-plugin): [no-unsafe-type-assertion] fix for unsafe assertion to a constrained type parameter #10461

controversial commented Dec 5, 2024 •

edited

Loading

typescript-eslint bot commented Dec 5, 2024

netlify bot commented Dec 5, 2024 •

edited

Loading

nx-cloud bot commented Dec 5, 2024 •

edited

Loading

codecov bot commented Dec 5, 2024 •

edited

Loading

controversial commented Dec 5, 2024 •

edited

Loading

kirkwaiblinger left a comment

kirkwaiblinger Dec 6, 2024

controversial commented Dec 6, 2024 •

edited

Loading

controversial commented Dec 16, 2024

kirkwaiblinger commented Dec 17, 2024

kirkwaiblinger left a comment

controversial commented Dec 17, 2024

fix(eslint-plugin): [no-unsafe-type-assertion] fix for unsafe assertion to a constrained type parameter #10461

fix(eslint-plugin): [no-unsafe-type-assertion] fix for unsafe assertion to a constrained type parameter #10461

Conversation

controversial commented Dec 5, 2024 • edited Loading

PR Checklist

Overview

typescript-eslint bot commented Dec 5, 2024

netlify bot commented Dec 5, 2024 • edited Loading

✅ Deploy Preview for typescript-eslint ready!

nx-cloud bot commented Dec 5, 2024 • edited Loading

☁️ Nx Cloud Report

codecov bot commented Dec 5, 2024 • edited Loading

Codecov Report

controversial commented Dec 5, 2024 • edited Loading

kirkwaiblinger left a comment

Choose a reason for hiding this comment

kirkwaiblinger Dec 6, 2024

Choose a reason for hiding this comment

controversial commented Dec 6, 2024 • edited Loading

controversial commented Dec 16, 2024

kirkwaiblinger commented Dec 17, 2024

kirkwaiblinger left a comment

Choose a reason for hiding this comment

controversial commented Dec 17, 2024

controversial commented Dec 5, 2024 •

edited

Loading

netlify bot commented Dec 5, 2024 •

edited

Loading

nx-cloud bot commented Dec 5, 2024 •

edited

Loading

codecov bot commented Dec 5, 2024 •

edited

Loading

controversial commented Dec 5, 2024 •

edited

Loading

controversial commented Dec 6, 2024 •

edited

Loading