feat(14417): add optionality to accessors

[a-tarasyuk]

Fixes #14417

[typescript-bot]

The TypeScript team hasn't accepted the linked issue #14417. If you can get it accepted, this PR will have a better chance of being reviewed.

[DanielRosenwasser]

I think the addition of --exactOptionalProperties changes our approach on this, right?

I would expect that accessors cannot be optional, and that they should only explicitly specify | undefined so that --exactOptionalProperties works as expected. I'm curious to get other people's perspectives.

[gabritto]

I think the addition of --exactOptionalProperties changes our approach on this, right?

I would expect that accessors cannot be optional, and that they should only explicitly specify | undefined so that --exactOptionalProperties works as expected. I'm curious to get other people's perspectives.

I have yet to read more and understand the whole set vs define semantics, but is that only applicable to properties that are not methods?

[a-tarasyuk]

@DanielRosenwasser Thanks for the feedback. 👍🏻

I think the addition of --exactOptionalProperties changes our approach on this, right?

TypeScript allows optional methods (foo?() {})/properties (foo?) but disallows optional definitions for getters/setters, so I think this issue is more to fixing this inconsistency than to follow exactOptionalProperties which works in the type checking stage. With enabled exactOptionalProperties TypeScript allows define optional methods/properties, however, a user has to explicitly define undefined.

class Foo {
  a?() {}
  b?: number; 
  get c?() { ... } // Error
}

If it doesn't make sense to allow using ? in getters/setters, I'll drop PR. Also, I think would be useful to add clarification to the issue and remove Help Wanted label.

[DanielRosenwasser]

Thinking on it some more, I think it makes sense structurally. You can have an object that omits these properties and is compatible with the instance type. You may want to add a test for optional accessors that return number with --exactOptionalProperties, and try to assign something where the corresponding number property is always present, but possibly undefined. That assignment should fail. For example

export class C {
    get foo?(): number { return 42; }
    set foo?(value: number) {}
}

declare let x: {
    foo: number | undefined;
}

let y: C = x;

Other thoughts:

• Does set foo?(value: number) make sense syntactically? Or should the ? go after value?
• Does this affect existing .d.ts files? Should downlevel-dts be updated either way?

---

I have yet to read more and understand the whole set vs define semantics, but is that only applicable to properties that are not methods?

Yeah, my concern here had to do with whether the property was placed on the instance vs. the prototype. This doesn't have much to do with if a property was created with Set vs. Define though.

[gabritto]

I discussed the original issue with @sandersn and here's our concerns:

1. Under strict mode, an optional setter behaves badly:

interface I {
    set m?(value: string): void;
}

declare var i: I;
i.m = "oops"; // This is going to throw if `set m()` is not defined on `i`

compared to when we know, statically, that the setter is not defined:

interface I {
    get m(): string;
    // no setter!
}

declare var i: I;
i.m = "oops"; // Compiler error: `m` is treated as a `readonly` property, because we have a getter but not a setter.

The way to check if a setter is present is like this:

interface I {
    set m?(value: string): void;
}

declare var i: I;
if (typeof Object.getOwnPropertyDescriptor(i, 'm')['set'] === 'function') {
    ... // we could narrow the type of `i.m` here, but it seems rather convoluted
}

2. An optional getter is ok in that, if it is not defined on a certain object, trying to read that property doesn't throw:

interface I {
    get m?(): string;
}

declare var i: I;
i.m; // `undefined` if `get m()` is not defined.

but I can't tell how much having that would make sense, considering the benefits vs. the costs in terms of maintenance and confusing users.

I think we might be better off without this feature, because even though at a first glance it might seem like accessors can be treated just like properties, there are some underlying subtleties that makes it more complicated.
@DanielRosenwasser what do you think?

[gabritto]

@DanielRosenwasser @sandersn @rbuckton given the concerns I wrote in the comment above, should we close this PR and its issue?

[weswigham]

I wholeheartedly agree, and am happy to classify this PR as "not accepted". The issue can probably remain as a discussion (and I've relabeled it as such); maybe someone can design something with a set of rules that's not terrible to use, however doubtful that seems given current class field behavior.