Add OptionalChain support

[jridgewell]

This adds the ?. PrivateIdentifier and OptionalChain . PrivateIdentifier grammars and related runtime semantics.

Closes tc39/proposal-optional-chaining#28

[littledan]

This is deliberately omitted, as we've discussed several times before, including on that linked thread. So, I'd like to close this as wontfix. cc @mheiber

[jridgewell]

So, I'd like to close this as wontfix.

We should either add it, or forbid it now and add it later. Right now we're in kind of a broken state.

Chrome and XS (the only browsers that support private fields and optional chains?) currently allow OptionalChain . PrivateIdentifier but don't parse ?. PrivateIdentifier.

class Foo {
  static #x = 1;

  static test() {
    const o = { Foo };
    return o?.Foo.#x
  }
}

print(Foo.test());

#### v8, xs
1

Babel's private fields transform is broken, but I haven't looked into why yet.

I think it's weird to have them "fix" it now just to revert that and support the combination later.

[ljharb]

o?.Foo.#x definitely should be allowed; the only thing we should maybe be deferring to later is o?.#x.

[mheiber]

What's the best way to progress this discussion? Would it make sense to have optional chaining support for private class fields in a follow-on proposal?

[ljharb]

@mheiber to be clear; "optional chaining support for private class fields" means o?.#x to me; being able to use private fields within a chain that happens to be "behind" an optional lookup needs to be accepted right out of the gate (ie, o?.foo.#x)

[littledan]

Considering it a separate follow-on proposal feels like overkill to me, and splitting proposals into a million pieces doesn't seem like a great way to keep all the stakeholders in the loop to resolve cross-cutting concerns. I think a PR in this repository, and a discussion at TC39 seeking consensus, is the right place to discuss this issue.

I reviewed this too quickly. I was responding negatively to the idea of x?.#y, which is only half of this PR. I'd rather not support x?.#y because the semantic concept of, "a value which is either null/undefined or an instance of this class" feels like a category error; optional chaining makes more sense for data-like things, which doesn't really mesh with instances of a particular class.

On the other hand, I agree with @ljharb that we should add x?.y.#z. Intuitively, I think it would "feel syntactically weird" if it didn't. (Sorry that both of those arguments are a bit fuzzy; I'm willing to be flexible here.)

I'd welcome a PR which just added the one form and not the other. Then, we could discuss it at a future TC39 meeting. I'd also welcome a volunteer presenter for this; it doesn't have to be me.

[jridgewell]

QuickJS also has the same behavior.

[jridgewell]

Did we spec OptionalChain incorrectly? Seeing as the 3 engines that support it also support OptionalChain . PrivateIdentifier. I figured it'd be an unstated parse error, but everything worked anyways.

I agree with @ljharb that we should add x?.y.#z. Intuitively, I think it would "feel syntactically weird" if it didn't.

👍 . I'll trim this PR.

I'd rather not support x?.#y because the semantic concept of, "a value which is either null/undefined or an instance of this class" feels like a category error; optional chaining makes more sense for data-like things, which doesn't really mesh with instances of a particular class.

I disagree, but we can have that discussion afterwards.

[littledan]

Did we spec OptionalChain incorrectly?

I'm not sure what is incorrect. The lack of a grammar production means it was prohibited.

I guess many people made the same guess at the syntax combination being allowed. We didn't document the interaction. I believe @mheiber added tests at some point, but that may have been after implementations.

I disagree, but we can have that discussion afterwards.

Sure; I'd say that we could land this trimmed patch, and consider adding the other part as either a PR to this repo (during Stage 3) or a needs-consensus PR (after Stage 4). At the same time, I would like to come to a prompt conclusion on this question; it's too bad we left it ambiguous for so long. So let's see if we can do that at the March 2020 TC39 meeting.

[jridgewell]

Updated to only allow OptionalChain . PrivateIdentifier. I opted to leave an explicit Early Error for ?. PrivateIdentifier, but I can remove this if you'd like.

[littledan]

Wasn't the idea that this production would be omitted?

[ljharb]

See line 198, it’s an explicit syntax error (like the optional template literal syntax, eg)

[littledan]

Ah, I see. I guess I find this editorial strategy strange--isn't just about everything that's not in the grammar reserved in the future? We haven't used this editorial strategy for other things that we've been thinking about including in the future, like private fields in object literals, or concise private field references (without this). I don't know why this should be treated differently.

[jridgewell]

I left it this way because the only 3 implementations that support privates and optional chain didn't realize that mixing them isn't in the spec. Conveniently none of them screwed up this production, but maybe the next implementation would.

[littledan]

Good point; we should be clear here. How about a note explaining that this production is specifically omitted?

[littledan]

Right, I'm suggesting to remove the grammatical production and early error, and instead simply have a note that explains that the grammatical production is deliberately excluded.

[caiolima]

FYI, there are Test262 cases for this tc39/test262#2408. We also considered this issue while implementing private features on WebKit. Unfortunately, I don't remember who report this issue.

[littledan]

@caiolima Are there any tests for the other case, x?.y.#z? (I think we should land this patch, which permits this grammar, but currently, it's not permitted in the grammar.)

[jridgewell]

Done.

[caiolima]

@caiolima Are there any tests for the other case, x?.y.#z? (I think we should land this patch, which permits this grammar, but currently, it's not permitted in the grammar.)

I don't think we have, but I'll double check and open a PR to add it.

[littledan]

Thanks for applying the changes I suggested.

[littledan]

Another way to phrase this is,

?. |PrivateIdentifier| is not part of the grammar for |OptionalChain|; therefore, expressions like x?.#y cause a SyntaxError when parsed as an |Expression|.

Depends how pedantic you and the editors want to be. But in particular, I don't think "currently" is needed--everything in the spec or proposal is "currently".

[devsnek]

is there any reason to not make x?.#y valid?

[jridgewell]

Let's follow up with discussion in #302.

[Jamesernator]

I'd rather not support x?.#y because the semantic concept of, "a value which is either null/undefined or an instance of this class" feels like a category error; optional chaining makes more sense for data-like things, which doesn't really mesh with instances of a particular class.

For recursive instances this is pretty important, I've been hitting this the last couple days as I've been wanting to do stuff like this.#parentNode?.updateCount(count) and such.

[caiolima]

For recursive instances this is pretty important, I've been hitting this the last couple days as I've been wanting to do stuff like this.#parentNode?.updateCount(count) and such.

Even without this PR, this.#parentNode?.updateCount(count) is a valid syntax, since this.#parentNode is a MemberExpression. You can check it on complete grammar.

[Jamesernator]

Even without this PR, this.#parentNode?.updateCount(count) is a valid syntax, since this.#parentNode is a MemberExpression. You can check it on complete grammar.

Oops I forgot to put the # on the #updateCount() as well.

[caiolima]

For the record, we reached consensus on also allowing o?.#field. I think it would be fine add it in this PR as well.

[jridgewell]

This PR has been updated to allow both OptionalChain . PrivateIdentifier and ?. PrivateIdentifier syntax forms. Both reached consensus in today's plenary.

[littledan]

Thanks for bearing with all the back and forth, and updating this patch. Merging per TC39 consensus.

[claudepache]

Hi,

Sorry, I missed this thread.

@littledan: This is deliberately omitted, as we've discussed several times before, including on that linked thread

Arguments were given in tc39/proposal-optional-chaining#28 (in particular tc39/proposal-optional-chaining#28 (comment)) for supporting a?.#b, and I don’t see that they were refuted or wilfully dismissed.

Probably I missed something?

[caiolima]

Hi @claudepache, this PR was merged into class fields proposal. This mean that o?.#field is supported. Isn't it what tc39/proposal-optional-chaining#28 was looking for?

[claudepache]

@caiolima

The title of this PR is misleading: following the discussion in this very thread, o?.#field was explicitly made not supported (i.e., a SyntaxError).

[claudepache]

It seems that the discussion continued on #302. I’ll comment further there, sorry the noise.