Don't require to implement optional abstract properties

Qwertiy commented 3 years ago

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 https://github.com/microsoft/TypeScript/issues/40632)
`abstract propertyMustImplementToo?: number`	No	Yes	Yes (except https://github.com/microsoft/TypeScript/issues/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:

[x] This wouldn't be a breaking change in existing TypeScript/JavaScript code
[x] This wouldn't change the runtime behavior of existing JavaScript code
[x] This could be implemented without emitting different JS based on the types of the expressions
[x] This isn't a runtime feature (e.g. library functionality, non-ECMAScript syntax with JavaScript output, etc.)
[x] 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.

RyanCavanaugh commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

@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?

It's strange to mark as optional something, that would be created in the same statement where it is marked as optional.
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 commented 3 years ago

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

ExE-Boss commented 3 years ago

@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 commented 3 years ago

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

sandersn commented 3 years ago

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 commented 3 years ago

@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 commented 3 years ago

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 commented 3 years ago

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

trusktr commented 3 years ago

@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 commented 2 years ago

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 commented 1 year ago

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 commented 1 year ago

FWIW this is also functionality we need. Same case.

MGREMY commented 4 weeks ago

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

microsoft / TypeScript