-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
Comments
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
} |
This is the old "generic that's only used in a return location" problem. The 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 |
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. Worst case we could just check for a generic call sig and report a different message in that case warning of the known problem. |
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. |
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 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
} |
It's again because of the anti-pattern of using a type parameter solely in the return type location. This pattern is impossible to implement safely - so there are just a lot of quirks with it. 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. |
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 // 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 |
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. |
Before You File a Bug Report Please Confirm You Have Done The Following...
Playground Link
https://typescript-eslint.io/play/#ts=5.0.3&sourceType=module&code=KYDwDg9gTgLgBDAnmYcAKBDWiDSBLGAZRmgwHNUBeOAbwCg44AbPAZxgB4AVOagVwB2AawEQA7gIB8ACgCUALnRQIAWzbAOAWQxgO7KHgFkANHC6TJAbjoBfa3QAmwAMZMsqZxAHs4DgEaKmNj4RCRQ5MD2oJCwcBisiALOcABmgs4weF7MwABuwEwA4sAwcrQMcFAlfFACcNIYYhgEvn4AdCzscrJxrHDauvqGJnAAqoYwABwAglDhiFa2dEA&eslintrc=N4KABGBEBOCuA2BTAzpAXGUEKQAIBcBPABxQGNoBLY-AWhXkoDt8B6Jge1tiacTJTIAhtEK0ipWkOTJE0fJQ5N0UOdA7RI4MAF8QOoA&tsconfig=N4KABGBEDGD2C2AHAlgGwKYCcDyiAuysAdgM6QBcYoEEkJemy0eAcgK6qoDCAFutAGsylBm3QAacDUh4AhpgDm6PBSjoSRdAA8VUgL4g9QA
Repro Code
ESLint Config
tsconfig
Expected Result
The
as
assertion is changing the type ofdb.list
from the default inferredlist<unknown>
tolist<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>
ondb.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
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)).The text was updated successfully, but these errors were encountered: