-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
Rule proposal: Ban method override type narrowing #8964
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
Huh. Without really having thought about this, my initial impression is that I would have thought that TS would allow subclasses to accept wider parameters and return narrower return types. But if I understand correctly it allows only narrower types both for parameters and return values, is that right? |
Currently TS supports both wider and narrower parameter types. I don't think either should be allowed |
Subclasses accepting wider parameters is safe and idiomatic. Banning it will make some OOP patterns needlessly complicated. Subclasses accepting narrower parameters is because methods are fundamentally unsafe: see https://typescript-eslint.io/rules/method-signature-style/ I'm slightly in favor of such a rule that bans overriding methods accepting narrower input. |
The rule should not be solely confined to inheritance. IMO, it should also detect general assignability, but not sure if that's performant. Just suggesting the rule to at least be generically named. |
On the naming, noting that the best technical term is probably "heritage methods" but we chose to use "inherited methods" as the user-facing terminology in #8765 (comment). Will probably want to be consistent here. Also - v8 is now live, so we can start using |
Looking a bit into this, I think it's closely related (possibly even a duplicate) to #277. To my understanding, the following examples should all be reported and belong under a single rule: // --- Inheritance ---
export class A {
f(_p: number | string): void {}
}
export class B extends A {
// Property 'f' in type 'B' is not assignable to the same property in base type 'A'.
f(_p: string): void {}
}
// --- Interfaces ---
export interface C {
f(_p: number | string): void;
}
export interface D extends C {
// Property 'f' in type 'D' is not assignable to the same property in base type 'C'.
f(_p: string): void;
}
// --- Interface implementation ---
export interface E {
f(_p: number | string): void;
}
export class F implements E {
// Property 'f' in type 'F' is not assignable to the same property in base type 'E'.
f(_p: string): void {}
}
// --- Assignment ---
class G {
f(_p: number | string): void {}
}
const e = new G();
// Property 'f' is not assignable to the same property in base type 'G'.
e.f = (_p: number): void => {}; Based on the above, I'm personally not sure if there's a reason to have both issues open. I might be wrong on this, so I would be happy to hear other opinions. |
👍 that's a good point. I'd be in favor of combining these two issues into one. It might make sense to rename #277 to be more specific? |
Yes, I think so too 👍 I'm having a hard time thinking of a fitting name. I like @kirkwaiblinger's suggestion to use "heritage methods" the most (in #8964 (comment)). |
I actually meant to suggest we not use "heritage methods" in a user-facing context if we can avoid it. It's a technical term that makes sense to us but feels super daunting to me as a user. Hence the decision to go with "inherited methods" in #8765 (comment) (after a rather lengthy discussion). Let's continue conversation on #277 |
Before You File a Proposal Please Confirm You Have Done The Following...
My proposal is suitable for this project
Description
Validate that overriden method signatures match the base signature (i.e. no narrowing of types)
Fail Cases
Pass Cases
Additional Info
TS issue was closed as "working as intended" but this is very dangerous, since this breaks some type safety, meaning that this introduces a risk of runtime errors
The text was updated successfully, but these errors were encountered: