Skip to content

Don't require to implement optional abstract properties #40635

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

Open
5 tasks done
Qwertiy opened this issue Sep 18, 2020 · 17 comments
Open
5 tasks done

Don't require to implement optional abstract properties #40635

Qwertiy opened this issue Sep 18, 2020 · 17 comments
Labels
Awaiting More Feedback This means we'd like to hear from more people who would be helped by this feature Suggestion An idea for TypeScript

Comments

@Qwertiy
Copy link

Qwertiy commented Sep 18, 2020

Search Terms

abstract optional override

Suggestion

If in abstract class abstract property is marked as optional, allow child classes not to implement it.
So I suggest to remove an error for property x of class D in the following code:

https://www.typescriptlang.org/play?ts=4.0.2#code/IYIwzgLgTsDGEAJYBthjAgggg3gKAUIQAcoB7CAU3koBMFRIZ4EpLhayA7ZATwQAeAfgBcCLgFcAtiEpQCRUhWpV6jaHERsO3Pgl5jJMuQgA+CCV1qUAZgEsudPAsLEJIZHdgJOAZSkQABYAFACUuC5EhLDcYGTIlAB0yGQA5sFBdmCJAgA0CJnZvKGRAL545XgoaBgAIgiUAlRWGNj4UUpUNPTanDz8-AC8CACMAAwVzo4A7gi1YYl+ASGhQA

abstract class A {
    protected abstract readonly x?: number
    protected abstract readonly y: number | undefined

    public doSmth() {
        console.log(this.x, this.y)
    }
}

class D extends A {
    protected readonly y = 10
}

new D().doSmth()

But property y still must be implemented.

Note that we already can do some sort of it

https://www.typescriptlang.org/play?ts=4.0.2#code/JYOwLgpgTgZghgYwgAgJLIN4Chm+QBygHt9owBPAYThAGUBrYfAfgC5kQBXAWwCNoceQiTLkAspwDOYVN3wAbCNwjh2XPtGQAfZJxAATCDFAR9g3OeTKwACyL7qdRiwAUASnYA3IsDN4rELb2EtKyCkoqYO5ePmYAvlhYcLzSUIhgyAjycJKSyACCmJbCpFAUjgxMbBw8-FCWyanpBMSlFCEycorKqjUaUNq6BkYmfniNYGkIGSWiHWHdkQAqRETV6nWJ-tZ2DjSVrh7I3r4NKZPNO8FSneE9UUcnY7gTUxlX+vNdEeAra9HHWJYBJYLI5PKUQoQAAekAMeUK2AA9AAqSwAOSIIAAtK9mmDcsgAOSQonIfRECB5EBEDLAb73ZCgGzQYCQfTIPHTAL9YkfL53SJkmDEbiZbKEon5IkAOhcACYAKwARkVbgxWNx5ze4vBxNJ5Mp1NpTIZkSZIBZUDZpk52suSjqfMCuwFi1+q2Fot1kulcqVqvV-kxOK5GQJeRJ0sNVI4JvpgvAFqtNo5YZ5TqJszK4huCx+YC9RDFEeJfoVKrVGtD9u5pajZIpsZpdLNSeZrPZdqa3OUvKzrTmebbYD+RZLEsj5YDVZRSOBiVLlHQCfdYDy6GRaP8lEn+tQZNACCIUCgEGm8nIpsT64tkFgiBQRIPMssS3IpH3h7y3GAuVAADmyC2CgMBEPI8hEAA7oBLQiGUwCxiKxbAR+T4Huw2btMON4ADQBEEnw4WuCoACzygADOqc7AkAA

interface I {
    propertyCanSkip?: number
    propertyMustImplement: number | undefined
    
    methodCanSkip?(): void
    methodMustImplement(): void
}

abstract class A {
    propertyCanSkip?: number
    abstract propertyMustImplement: number | undefined
    abstract propertyMustImplementToo?: number

    methodCanSkip?(): void
    abstract methodMustImplement(): void
    abstract methodMustImplementToo?(): void
}

class CA extends A {
/*
    Non-abstract class 'CA' does not implement inherited abstract member 'methodMustImplement' from class 'A'.(2515)
    Non-abstract class 'CA' does not implement inherited abstract member 'methodMustImplementToo' from class 'A'.(2515)
    Non-abstract class 'CA' does not implement inherited abstract member 'propertyMustImplement' from class 'A'.(2515)
    Non-abstract class 'CA' does not implement inherited abstract member 'propertyMustImplementToo' from class 'A'.(2515)
*/
}

class CI implements I {
/*
    Class 'CI' incorrectly implements interface 'I'.
    Type 'CI' is missing the following properties from type 'I': propertyMustImplement, methodMustImplement(2420)
*/
}

But there is a set of problems for properties

We have two ways to override property (via property declaration or via getter and setter). And now (in TS4) the limitation have changed. For nonabstract property base class always defines how it should be implemented in children.

Let's look what implementations are possible (don't forget about useDefineForClassFields compiler flag that makes it more important):

Code Property can be omitted Child can implement as property Child can implement as get/set
propertyCanSkip?: number Yes Yes No
abstract propertyMustImplement: number | undefined No Yes Yes (except #40632)
abstract propertyMustImplementToo?: number No Yes Yes (except #40632)
get getter?(): number N/A N/A N/A
abstract get getterMustImplement(): number | undefined No No Yes
abstract getterToo?(): number N/A N/A N/A

It's easy to see, that if the property should really be optional, there is only one way to make it such which will not allow to implement it as getter and setter. But we have 2 absolutely identical lines with optional and nonoptional abstract property. I see no sense for them to be synonyms as ? in the 3rd line definitely says that the property is optional, but doesn't give me ability to make so in further code.

So I propose to change this table in following way:

Code Property can be omitted Child can implement as property Child can implement as get/set
propertyCanSkip?: number Yes Yes No
abstract propertyMustImplement: number | undefined No Yes Yes
abstract propertyCanSkipToo?: number Yes Yes Yes
get getter?(): number N/A N/A N/A
abstract get getterMustImplement(): number | undefined No No Yes
abstract get getterCanSkipToo?(): number Yes No Yes

Abstract getter is NOT a part of this feature request, just shown for consistency.

Use Cases

Provide ability to list and use for reading an optional property in abstract class without limiting a way of its implementation in child classes. Such problem occured in a real project because of migration from TS3 to TS4. Before that it was possible, but because of breaking changes of TS4 it's not anymore.

https://www.typescriptlang.org/play?ts=4.0.2#code/IYIwzgLgTsDGEAJYBthjAgggg3gKAUIQAcoB7CAU3koBMEpLhayA7ZATwTAFsIALAPwAuBKwCuPEJSh4CRYuJDIAlrAQAzABQBKXPKJFYbMGWSUAdMjIBzLQJVgLvAToMIAvni94UaDABCCJQAHlSstBjY+IYu-AgAvAgAjAAM3nJ+6AgAwsFhlBFR+oY2lIhxurgM5eJQrAgAssACFjARZDxVXj5ZGAAi+eGRWPo+rJQA7ggBuhbabhPTOXMLeEsI-au6QA

abstract class A {
    protected readonly smth?: number

    public f() {
        console.log(this.smth)
    }
}

class B extends A {
    smth = 10
}

class C extends A {
    get smth() { return Math.random() }
}

class D extends A {
}

new B().f()
new C().f()
new D().f()

Examples

See above.

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, etc.)
  • This feature would agree with the rest of TypeScript's Design Goals.

Why it's not a breaking change?

It changes behavior of existing construction, but it's not a breaking change in terms of code.

If you had your code working and you have

abstract propertyMustImplementToo?: number

in it, that means that you implemented this property in all child classes. So after its meaning changes all you code keeps being valid and compiles into absolutely the same javascript code as before. Nothing changed.

At the same time, for further development you have to decide whether you want to allow child classes to skip the property or not. If yes, or you don't care - keep it with ? as it is. If no then update it to

abstract propertyMustImplementToo: number | undefined

without any other changes needed.

Related Issues:

#6413
#22939

@RyanCavanaugh RyanCavanaugh added Awaiting More Feedback This means we'd like to hear from more people who would be helped by this feature Suggestion An idea for TypeScript labels Sep 21, 2020
@RyanCavanaugh
Copy link
Member

Discussed this with @sandersn a bit and we think this is reasonable, but also that the current behavior is also very defensible. In general we're stare decisis for longstanding behavior that hasn't received any other feedback, so would prefer to leave this alone unless there's strong evidence that the majority of the people using this particular set of modifiers feels the same way.

@sandersn
Copy link
Member

Another point from our discussion; if #40632 is fixed, these two programs emit different code with useDefineForClassFields: true:

abstract class C { p?: number }

emits a defineProperty.

abstract class C { abstract p?: number }

would not emit a defineProperty.

Right now these two programs both emit the [[Define]], and they both check the same way, so you can get the desired effect today by leaving off abstract.

@Qwertiy
Copy link
Author

Qwertiy commented Sep 21, 2020

@sandersn why do you compare

abstract class C { p?: number }
abstract class C { abstract p?: number }

they are already different.

But

abstract class C { abstract p?: number }
abstract class C { abstract p: number | undefined }

currently are the same and won't become different after the fix.

so you can get the desired effect today by leaving off abstract.

I don't understand it.

Here is the code that is valid for TS3 (playground)

abstract class A {
    protected readonly smth?: number

    public f() {
        console.log(this.smth)
    }
}

class B extends A {
    smth = 10
}

class C extends A {
    get smth() { return Math.random() }
}

class D extends A {
}

new B().f()
new C().f()
new D().f()

If I switch to TS4 (playground), I'll get an error:

'smth' is defined as a property in class 'A', but is overridden here in 'C' as an accessor. (2611)

I expected abstract to solve the problem, but if I add it, the error above really disappears, but the other one occurs (playground):

Non-abstract class 'D' does not implement inherited abstract member 'smth' from class 'A'. (2515)

You say I can remove abstract to get it working. Remove from where?


By the way, another moment you've shown:

with useDefineForClassFields: true:

abstract class C { p?: number }

emits a defineProperty.

Is it really a good idea?

  1. It's strange to mark as optional something, that would be created in the same statement where it is marked as optional.
  2. Such generation will completely disallow having real optional fields in classes. Any declaration (except of abstract) will produce a property, but without declaration it's impossible to use the property. So you'll never can create no property.

Seems like for this code the same thing I wrote should be applied:

p?: number;              // Optional property - don't create
p: number | undefined;   // Nonoptional property - create

@sandersn
Copy link
Member

Typescript doesn't distinguish between p?: number and p: number | undefined in properties. The first is a shortcut for the second.

@ExE-Boss
Copy link
Contributor

@sandersn
Does that mean that with a type like:

interface Foo {
	 bar: string;
	 baz: string | undefined;
}

It’s valid to do:

const foo: Foo = { bar: "bar" };

By omitting baz even with strictNullChecks?

@sandersn
Copy link
Member

No, I am wrong. p?: number is equivalent to p?: number | undefined BUT is different from p: number | undefined.

@sandersn
Copy link
Member

I said

abstract class C { p?: number }
abstract class K { abstract p?: number }

are checked the same today (and emitted, but that's bug #40699)

class D extends C { } // doesn't need to be abstract, p is optional
class B extends K { } // currently: B must be abstract

This issue requests that B be valid code, and not required to be abstract. But that's the same as D.

@Qwertiy
Copy link
Author

Qwertiy commented Sep 30, 2020

@sandersn I don't understand you still. Here is code for TS3 - how do you propose to update class A without changing B, C and D so that the code will be valid for TS4? Full example, please.

@sandersn
Copy link
Member

B vs C is a separate issue. For B and D, you don't need to update A if you want smth to be optional. If you want smth to be required, make it abstract.

@Qwertiy
Copy link
Author

Qwertiy commented Sep 30, 2020

@sandersn I want optional field and all 3 classes compatible with A. For me it seems impossible now, isn't it?

@trusktr
Copy link
Contributor

trusktr commented Feb 13, 2021

@sandersn It doesn't seem that you addressed the problem @Qwertiy described in his above comment.

The problem is, no matter which way is chosen (you're describing the two ways we can choose), there is a type error regardless.

How do you make that example work with and without abstract? A solution is needed.

@LumaKernel
Copy link

LumaKernel commented Sep 18, 2021

I think this is the only way to implement conditionally optional implementation requirement.

playground

abstract class A<T extends string = ""> {
  abstract f: "" extends T ? (T | undefined) : T;
}

// ERROR: Non-abstract class 'B' does not implement inherited abstract member 'f' from class 'A<"">'.(2515)
class B extends A {
  // now, it is needed to implement f even if it can be undefined
  // f = undefined;
}

class C extends A<"a" | "b" | "c"> {
  // implementation is needed
  f = "a" as const;
}

I have no idea about necessity of it.

@bogdanb
Copy link

bogdanb commented Nov 1, 2022

I’m having something like the code below (simplified) in our project, which we’d like to run with :

abstract class BaseClass {
    abstract readonly requiredProperty: string;
    abstract readonly optionalProperty?: string;
}

The idea is that sub-classes must specify how the properties are defined (including if they are based on getters or values), and once they do that they can’t be changed, but the optionalProperty either is a string or it’s not present at all. Note that exactOptionalPropertyTypes makes the distinction between “a property is not present” and “a property has the undefined value” explicit.

In apparent contradiction with some of the comments above, it seems impossible to actually define a subclass of BaseClass whose instances don’t have the optionalProperty at all.

(Well, I can define it by adding a constructor that explicitly deletes the property, but that’s not what I mean. TypeScript complains about deleting readonly properties, so I’d need to add some casts.)

@JacobBeaudry
Copy link

FWIW this is also functionality we need. Same case.

@MGREMY
Copy link

MGREMY commented Aug 19, 2024

Same here, could be great to have this (similar to virtual in .net)

@miguel-leon
Copy link

I hit a requirement that's impossible with current typescript:

A class with a method signature that subclasses may or may not implement.

  • If the method is abstract, then the subclass can't skip its implementation.
  • If the method is not abstract, then the subclass is required to add override under noImplicitOverride, which is misleading because it's not actually overriding anything.

These kinds of problems are encountered only by api designers, so the "majority of people" will never be affected by this. If that's what's expected, then there's no hope for changes like this to happen.

@RobbieTheWagner
Copy link

I am hitting the same issue as @miguel-leon. I would like to define a method signature in the parent and have children optionally implement it. I have not found a way to do so with TS yet.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting More Feedback This means we'd like to hear from more people who would be helped by this feature Suggestion An idea for TypeScript
Projects
None yet
Development

No branches or pull requests