tc39 / proposal-class-fields

Orthogonally-informed combination of public and private fields proposals
https://arai-a.github.io/ecma262-compare/?pr=1668
1.72k stars 113 forks source link

Add OptionalChain support #301

Closed jridgewell closed 4 years ago

jridgewell commented 4 years ago

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

Closes https://github.com/tc39/proposal-optional-chaining/issues/28

littledan commented 4 years ago

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

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

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

mheiber commented 4 years ago

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

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

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

QuickJS also has the same behavior.

jridgewell commented 4 years ago

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

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

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.

Jamesernator commented 4 years ago

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

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

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

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

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

littledan commented 4 years ago

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

claudepache commented 4 years ago

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 https://github.com/tc39/proposal-optional-chaining/issues/28 (in particular https://github.com/tc39/proposal-optional-chaining/issues/28#issuecomment-519547723) for supporting a?.#b, and I don’t see that they were refuted or wilfully dismissed.

Probably I missed something?

caiolima commented 4 years ago

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

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

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