Skip to content

Bug: [no-unnecessary-type-assertion] Poor error message for missing type parameter constraint #6951

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

Open
4 tasks done
JoshuaKGoldberg opened this issue Apr 24, 2023 · 8 comments
Labels
accepting prs Go ahead, send a pull request that resolves this issue bug Something isn't working package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin

Comments

@JoshuaKGoldberg
Copy link
Member

JoshuaKGoldberg commented Apr 24, 2023

Before You File a Bug Report Please Confirm You Have Done The Following...

  • I have tried restarting my IDE and the issue persists.
  • I have updated to the latest version of the packages.
  • I have searched for related issues and found none that matched my issue.
  • I have read the FAQ and my problem is not listed.

Playground Link

https://typescript-eslint.io/play/#ts=5.0.3&sourceType=module&code=KYDwDg9gTgLgBDAnmYcAKBDWiDSBLGAZRmgwHNUBeOAbwCg44AbPAZxgB4AVOagVwB2AawEQA7gIB8ACgCUALnRQIAWzbAOAWQxgO7KHgFkANHC6TJAbjoBfa3QAmwAMZMsqZxAHs4DgEaKmNj4RCRQ5MD2oJCwcBisiALOcABmgs4weF7MwABuwEwA4sAwcrQMcFAlfFACcNIYYhgEvn4AdCzscrJxrHDauvqGJnAAqoYwABwAglDhiFa2dEA&eslintrc=N4KABGBEBOCuA2BTAzpAXGUEKQAIBcBPABxQGNoBLY-AWhXkoDt8B6Jge1tiacTJTIAhtEK0ipWkOTJE0fJQ5N0UOdA7RI4MAF8QOoA&tsconfig=N4KABGBEDGD2C2AHAlgGwKYCcDyiAuysAdgM6QBcYoEEkJemy0eAcgK6qoDCAFutAGsylBm3QAacDUh4AhpgDm6PBSjoSRdAA8VUgL4g9QA

Repro Code

export type PartyKitStorage = {
  list<T = unknown>(): Promise<Map<string, T>>;
};

declare const db: PartyKitStorage;

export async function levelGet() {
  return (await db.list()) as Map<string, Uint8Array>;
}

ESLint Config

module.exports = {
  parser: "@typescript-eslint/parser",
  rules: {
    "@typescript-eslint/no-unnecessary-type-assertion": "error",
  },
};

tsconfig

{
  "compilerOptions": {
    "strict": true,
    "target": "esnext"
  }
}

Expected Result

The as assertion is changing the type of db.list from the default inferred list<unknown> to list<Uint8Array>. Saying "This assertion is unnecessary since it does not change the type of the expression." is incorrect.

I think ideally I'd want the lint complaint to indicate that the as should be replaced with an explicit <Uint8Array> on db.list... but I suspect that's quite a lot of work (what if there are many type parameters, and/or they have odd constraints?). So perhaps we should special case the rule's message for cases with type parameters? Not sure.

Actual Result

This assertion is unnecessary since it does not change the type of the expression.

Additional Info

Very similar to #5487 as filed by @srmagura.

Found in the wild trying to enable recommended-requiring-type-checking on https://github.com/partykit/partykit/blob/baa74442a567b303ef0f93edcf3baa6622e0a749/packages/y-partykit/src/storage.ts#L54-L57 (partykit/partykit#125 (comment)).

@JoshuaKGoldberg JoshuaKGoldberg added bug Something isn't working package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin triage Waiting for team members to take a look labels Apr 24, 2023
@devKangMinHyeok
Copy link

devKangMinHyeok commented Apr 24, 2023

I'm just curious about the above code itself, so I'm asking. Is it correct to write the above code as below? (Of course I know this issue talks about the error message not providing the correct content for the context.)

export type PartyKitStorage = {
  list<T = unknown>(): Promise<Map<string, T>>;
};

declare const db: PartyKitStorage;

export async function levelGet() {
  return await db.list<Map<string, Uint8Array>>(); // changed
}

@bradzacher
Copy link
Member

bradzacher commented Apr 24, 2023

This is the old "generic that's only used in a return location" problem.

The as assertion is used by TS to "backfill" the generic parameter changing the return type we see and causing the false-positive.

There's unfortunately no easy way for us to fix this. When we inspect the type of the expression being asserted we don't see Map<string, unknown> - we see Map<string, T> because TS has physically backfilled the generic using the asserted type.
So to us we see Map<string, T> as Map<string, T> - hence we report on it.

@bradzacher
Copy link
Member

It's worth noting we could potentially attempt to fix this by doing some extra checks when we report.

If the expression is a call expression we might be able to interrogate the call signature to see if it is defined with a generic and if the generic is only used in an output location.
It would be a pretty complicated bit of logic and probably even a little slow due to the extra types we need.

Worst case we could just check for a generic call sig and report a different message in that case warning of the known problem.

@bradzacher
Copy link
Member

To be clear - the correct and non-reporting form of this call should be the MUCH simpler

await db.list<Uint8Array>()

No assertion or parentheses needed like that.

@devKangMinHyeok
Copy link

Thank you for your reply. Another thing to be curious about is, in my opinion, in this case, a warning about the case of not explicitly putting a type for the generic type would be sufficient.

For example, in the code below, since the generic type was not explicit while using the list method, shouldn't we warn about this? what about this
I would be very grateful if I could hear what you think.

export type PartyKitStorage = {
  list<T>(): Promise<Map<string, T>>;
};

declare const db: PartyKitStorage;

export async function levelGet() {
  return await db.list(); // warning : Generic types are not explicit
}

@bradzacher
Copy link
Member

bradzacher commented Apr 25, 2023

since the generic type was not explicit while using the list method, shouldn't we warn about this?

It's again because of the anti-pattern of using a type parameter solely in the return type location.
TS just silently defaults these parameters to unknown.

This pattern is impossible to implement safely - so there are just a lot of quirks with it.
For example:

function list<T>(): T {
  // @ts-ignore
  return 1;
}

const x = list<string>();
x.length.toFixed(); // crashes at runtime because the "list" function is unsound

Ideally one should avoid using this pattern wherever possible because it's a hidden safety hole that can lead to hard-to-trace bugs in production.

It's a very "javascripty" pattern (i.e. it's an attempt at adding types to dynamic, untyped programming) that's often used to add "strict" types at some boundary that doesn't have strict type enforcement (eg in this case it looks like it's the network or a storage boundary where it's assumed the values coming across the boundary are of some type).

There are two options really: accept the unsafety and continue onwards and hope you're good, or build a safer API that includes some checks to enforce types. zod is a great example of a library that provides strict runtime checks to enforce the types in a nice API.

@btmills
Copy link
Contributor

btmills commented Feb 28, 2025

I encountered a similar-sounding case with what appears to be a false positive report of a type assertion with a generic parameter, specifically casting Array<Element> to Array<HTMLElement> (playground):

// We originally had this code, but `@typescript-eslint/no-unnecessary-type-assertion`
// reports the assertion is unnecessary.
const arrayOfHTMLElement1 = Array.from(document.querySelectorAll('.foo')) as Array<HTMLElement>;
// The autofix removed the type assertion, but the resulting type is now
// `Array<Element>` rather than `Array<HTMLElement>`, causing a type error in
// later code.
const arrayOfElement = Array.from(document.querySelectorAll('.foo'));

// All of these give `Array<HTMLElement>` without a type assertion.
const arrayOfHTMLElement2 = Array.from<HTMLElement>(document.querySelectorAll('.foo'));
const arrayOfHTMLElement3: Array<HTMLElement> = Array.from(document.querySelectorAll('.foo'));
const arrayOfHTMLElement4 = Array.from(document.querySelectorAll('.foo')) satisfies Array<HTMLElement>;

If this is actually a distinct issue, I'd be happy to report it separately.

We're able to use one of the other three versions, but I'm flagging this because the autofix removed the type assertion and created a new type error later when we map over the array and expect it to contain HTMLElements.

@OmarHawk
Copy link

OmarHawk commented Apr 25, 2025

I encountered a similar-sounding case with what appears to be a false positive report of a type assertion with a generic parameter, specifically casting Array<Element> to Array<HTMLElement> (playground):

// We originally had this code, but @typescript-eslint/no-unnecessary-type-assertion
// reports the assertion is unnecessary.
const arrayOfHTMLElement1 = Array.from(document.querySelectorAll('.foo')) as Array;
// The autofix removed the type assertion, but the resulting type is now
// Array<Element> rather than Array<HTMLElement>, causing a type error in
// later code.
const arrayOfElement = Array.from(document.querySelectorAll('.foo'));

// All of these give Array<HTMLElement> without a type assertion.
const arrayOfHTMLElement2 = Array.from(document.querySelectorAll('.foo'));
const arrayOfHTMLElement3: Array = Array.from(document.querySelectorAll('.foo'));
const arrayOfHTMLElement4 = Array.from(document.querySelectorAll('.foo')) satisfies Array;
If this is actually a distinct issue, I'd be happy to report it separately.

We're able to use one of the other three versions, but I'm flagging this because the autofix removed the type assertion and created a new type error later when we map over the array and expect it to contain HTMLElements.

Just stumbled upon the very same issue. I agree that this is a false positive and especially that autofix shouldn't produce compile time errors in the end.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepting prs Go ahead, send a pull request that resolves this issue bug Something isn't working package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin
Projects
None yet
Development

No branches or pull requests

5 participants