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

Reject invalid overriding of class methods with interface arguments #45854

Closed
5 tasks done
k4zh opened this issue Sep 13, 2021 · 5 comments
Closed
5 tasks done

Reject invalid overriding of class methods with interface arguments #45854

k4zh opened this issue Sep 13, 2021 · 5 comments
Labels
Duplicate An existing issue was already created

Comments

@k4zh
Copy link

k4zh commented Sep 13, 2021

Suggestion

The following code does compile (tested with TypeScript 4.4.2), but I would except it to not compile:

interface IA {
  a: string
}

interface IB extends IA {
  b: string // b cannot be undefined
}

class A {
  public do(i: IA) {
    console.log('i.a', i.a)
  }
}

class B extends A {
  // The compiler does not raise an error on B::do() prototype
  public do(i: IB) {
    // i.b is expected to be defined according to IB
    console.log('i.b cannot be undefined:', i.b)
  }
}

const a: A = new B()
// But here B::do(i: IB) is called with undefined i.b
a.do({ a: '' })

The execution result of this code is:

"i.b cannot be undefined:",  undefined

Hence, B::do(i: IB) method can be called with an IA argument, which means that i.b can be undefined.

The suggestion is to add a compilation error on B::do(i: IB) prototype.

I've hesitated to submit this as a bug since compiler does report an error for the same kind of design error using arguments such as string. For example, the compiler correctly reject the following code:

class A {
  public do() {}
}

class B extends A {
  // Property 'do' in type 'B' is not assignable to the same property in base type 'A'.
  // Type '(s: string) => void' is not assignable to type '() => void'.(2416)
  public do(s: string) {
    console.log(s)
  }
}

🔍 Search Terms

class method override interface argument

✅ Viability Checklist

My suggestion meets these guidelines:

  • This wouldn't be a breaking change in existing TypeScript/JavaScript code
  • This wouldn't change the runtime behavior of existing JavaScript code
  • This could be implemented without emitting different JS based on the types of the expressions
  • This isn't a runtime feature (e.g. library functionality, non-ECMAScript syntax with JavaScript output, new syntax sugar for JS, etc.)
  • This feature would agree with the rest of TypeScript's Design Goals.

⭐ Suggestion

I would suggest to make the compiler to check that in the overriding prototype, interface arguments do not require anything more than what the overridden argument initially provides.

📃 Motivating Example

The code provided as illustration is a motivation example: since the compiler does not report any error, a programmer would consider he will always deal with an IB argument with a defined b property in B:do(i: IB) while it actually can be undefined since the method can be called with an IA argument. In such a situation, unexpected behavior/bugs are possible at run time.

💻 Use Cases

The current workaround is human code review to avoid such programming design error.

@MartinJohns
Copy link
Contributor

@andrewbranch
Copy link
Member

The suggestion to do something different is part of #18770. I’m going to call this a duplicate rather than “Working as Intended” since I don’t want to imply that we would never be open to tightening this up.

FWIW, I feel like the FAQ entry isn't particularly satisfying in the years since --strictFunctionTypes landed; it doesn’t address why methods aren’t covered by that flag. I’m sure there’s a comment or release notes snippet that does somewhere.

@andrewbranch andrewbranch added the Duplicate An existing issue was already created label Sep 13, 2021
@k4zh
Copy link
Author

k4zh commented Sep 13, 2021

Hi @MartinJohns and @andrewbranch

Thank you for your prompt responses!

Actually I was expecting to have an error like this one:

interface IA {
    a: string
}

interface IB extends IA {
    b: string
}

function doB(i: IB) {}

// Type '(i: IB) => void' is not assignable to type '(i: IA) => void'.
//  Types of parameters 'i' and 'i' are incompatible.
//    Property 'b' is missing in type 'IA' but required in type 'IB'.(2322)
const doA: (i: IA) => void = doB

But I get your point.

Thanks

@fatcerberus
Copy link

I’m sure there’s a comment or release notes snippet that does somewhere.

It's mentioned in the PR that implemented the flag. See #18654, specifically:

Methods are excluded specifically to ensure generic classes and interfaces (such as Array<T>) continue to mostly relate covariantly. The impact of strictly checking methods would be a much bigger breaking change as a large number of generic types would become invariant (even so, we may continue to explore this stricter mode).

Which is also mentioned in the release notes for TS 2.6:
https://www.typescriptlang.org/docs/handbook/release-notes/typescript-2-6.html#strict-function-types

@typescript-bot
Copy link
Collaborator

This issue has been marked as a 'Duplicate' and has seen no recent activity. It has been automatically closed for house-keeping purposes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Duplicate An existing issue was already created
Projects
None yet
Development

No branches or pull requests

5 participants