Skip to content
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

Rule proposal: Ban method override type narrowing #8964

Open
6 tasks done
Woodz opened this issue Apr 22, 2024 · 3 comments
Open
6 tasks done

Rule proposal: Ban method override type narrowing #8964

Woodz opened this issue Apr 22, 2024 · 3 comments
Labels
enhancement: new plugin rule New rule request for eslint-plugin package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin triage Waiting for maintainers to take a look

Comments

@Woodz
Copy link

Woodz commented Apr 22, 2024

Before You File a Proposal Please Confirm You Have Done The Following...

My proposal is suitable for this project

  • My proposal specifically checks TypeScript syntax, or it proposes a check that requires type information to be accurate.
  • My proposal is not a "formatting rule"; meaning it does not just enforce how code is formatted (whitespace, brace placement, etc).
  • I believe my proposal would be useful to the broader TypeScript community (meaning it is not a niche proposal).

Description

Validate that overriden method signatures match the base signature (i.e. no narrowing of types)

Fail Cases

class A {
  f(p: string | number) {
    console.log('A', p);
  }
}

class B extends A {
  f(p: string) {
    console.log('B', p, p.charCodeAt(0));
  }
}

const arrayOfA: Array<A> = [new A(), new B()];
for (const item of arrayOfA) {
  item.f(123);
}

Pass Cases

class A {
  f(p: string | number) {
    console.log('A', p);
  }
}

class B extends A {
  f(p: string | number) { <-- Needs to match base class method signature
    console.log('B', p, p.charCodeAt(0));
  }
}

const arrayOfA: Array<A> = [new A(), new B()];
for (const item of arrayOfA) {
  item.f(123);
}

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

@Woodz Woodz added enhancement: new plugin rule New rule request for eslint-plugin package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin triage Waiting for maintainers to take a look labels Apr 22, 2024
@Woodz Woodz changed the title Rule proposal: <a short description of my proposal> Rule proposal: Ban method override type narrowing Apr 22, 2024
@kirkwaiblinger
Copy link
Member

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?

@Woodz
Copy link
Author

Woodz commented Apr 22, 2024

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

@Josh-Cena
Copy link
Member

Josh-Cena commented Apr 22, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement: new plugin rule New rule request for eslint-plugin package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin triage Waiting for maintainers to take a look
Projects
None yet
Development

No branches or pull requests

3 participants