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

Bug: Interface implementations allow optional properties to be required #56321

Closed
guidsdo opened this issue Nov 6, 2023 · 9 comments
Closed
Labels
Working as Intended The behavior described is the intended behavior; this is not a bug

Comments

@guidsdo
Copy link

guidsdo commented Nov 6, 2023

πŸ”Ž Search Terms

Interface optional properties required

πŸ•— Version & Regression Information

Bug exists for a long time, so no big new change for that here but I couldn't find a previous issue that discussed it.

⏯ Playground Link

https://www.typescriptlang.org/play?#code/JYOwLgpgTgZghgYwgAgJIHEoQpKyDeAUMsgOZY4AUIcAthAPwBcyAzmFKKQJQsBuAe2AATANyEAvoUIIANnFatkmbLmTBaAB1kR64JRgpqiJAPSnkAFQAWwJaAQCoWBGFkBPdVp16wyMNYooLjwSAA0bA5BfgqsAK709n4A7sCysshwsslw7kqkOJnINPTEZEbUdBAs7JwgPPxCwgRlJI4grAI6AHSyAqSUAAYAEhDpAsjCEHB4ACT4JRASg9ziJFJShObIAHIT0FBOyABGEAhwcawoAUHg0KEowgIQrCAA5H5YAI5xwFhFfCycWuE1OyE0sQgwhkAg6fnIqmgqDusEQ1TQKhw0GQAF5ihBksojNBKKtCAisVBkSE0d0KWBSeIthYAKLOI6nc6Xa6BZByWKZVxxLIeZDfX5YJRwZCA2TAmFw8qIqAAYXkilx+MJmNwjPJxNV6tYdIqqyAA

πŸ’» Code

interface IGreeter {
  greet(name?: string): void;
}

class Greeter implements IGreeter {
  // This incorrectly implement the interface, since it assumes it will always get a name
  greet(name: string): void {
    console.log(`Hello dear ${name}`);
  }
}

// No error because the interface doesn't require a value to be passed
const greeterInterface: IGreeter = new Greeter();
greeterInterface.greet();

// Error because the class actually requires a value
const greeterClass = new Greeter();
greeterClass.greet();

πŸ™ Actual behavior

It allows you to implement the IGreeter interface incorrectly by requiring a parameter in a method which is optional according to the interface.

πŸ™‚ Expected behavior

An error would be expected in the Greeter class that indicates that the IGreeter interface is not correctly implemented.

Additional information about the issue

For strict mode, I can't think of a reason why this isn't throwing an error. It does throw an error if you define the property in the interface as | undefined. There is a similar issue for interface properties, where public str?: string; doesn't require it to be implemented in the class but public str: string | undefined does.

@guidsdo guidsdo changed the title Interface implementations allow optional properties to be required Bug: Interface implementations allow optional properties to be required Nov 6, 2023
@whzx5byb
Copy link

whzx5byb commented Nov 6, 2023

Per https://github.com/microsoft/TypeScript/wiki/FAQ#common-bugs-that-arent-bugs :

  • Methods are always bivariant in their argument, while function properties are contravariant in their argument under strictFunctionTypes. More discussion here.

To make your expected behavior, you should declare greet as a function property.

interface IGreeter {
  greet: (name?: string) => void;
}

@guidsdo
Copy link
Author

guidsdo commented Nov 6, 2023

That doesn't explain why it works for string | undefined and not for ?: string.

I see that PR is merged in 2017 and the whole ecosystem got way more compatible with Typescript and strict typing, so it might be a good moment to re-evaluate and see if this change would still break a lot of code.

@whzx5byb
Copy link

whzx5byb commented Nov 6, 2023

That doesn't explain why it works for string | undefined and not for ?: string.

No, it doesn't work for string | undefined for the same reason. There will still be an potential runtime error as below shows.

interface IGreeter {
  greet(name: string | undefined): void;
}

class Greeter implements IGreeter {
  // This incorrectly implement the interface, since it assumes it will always get a name
  greet(name: string): void {
    console.log(`Hello dear ${name.concat('')}`);
  }
}

// No compiler error because the method will accept undefined
const greeterInterface: IGreeter = new Greeter();
greeterInterface.greet(undefined);

//But you will get runtime error: Cannot read properties of undefined (reading 'concat')

@guidsdo
Copy link
Author

guidsdo commented Nov 6, 2023

Ah yes you're right, it only throws a compiler error if you call it like this (playground):

const greeterInterface: IGreeter = new Greeter();
greeterInterface.greet();

Which is also odd. Either way I still think this is an important bug, curious what the maintainers think since I also can find ways to work around it.

@whzx5byb
Copy link

whzx5byb commented Nov 6, 2023

Well, this is another issue which is being discussed at #12400. At the moment, undefined parameter and optional parameters are not equivalent. You must explicitly provide an undefined value for the former when call the function.

@MartinJohns
Copy link
Contributor

way I still think this is an important bug

But it's not a bug. It's very much intentional, although unfortunate. The FAQ provides slightly more information: https://github.com/microsoft/TypeScript/wiki/FAQ#why-are-function-parameters-bivariant

@fatcerberus
Copy link

Also from #18654:

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

Yes, code in the wild has gotten more strict-friendly since then, but as breaking changes go, making all collections invariant still seems like a bridge too far, even today.

@RyanCavanaugh RyanCavanaugh added the Working as Intended The behavior described is the intended behavior; this is not a bug label Nov 6, 2023
@RyanCavanaugh
Copy link
Member

so it might be a good moment to re-evaluate and see if this change would still break a lot of code.

Nothing has changed regarding the underlying problem here (arrays need to be covariant so that you can ever make one), so I don't see what a re-evaluation would do.

@typescript-bot
Copy link
Collaborator

This issue has been marked as "Working as Intended" and has seen no recent activity. It has been automatically closed for house-keeping purposes.

@typescript-bot typescript-bot closed this as not planned Won't fix, can't repro, duplicate, stale Nov 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Working as Intended The behavior described is the intended behavior; this is not a bug
Projects
None yet
Development

No branches or pull requests

6 participants