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

Question about delete expression + opt chain #313

Open leobalter opened 4 years ago

leobalter commented 4 years ago

I'm not sure what is the accurate intention here.

The text says prohibits listed derivations (in Strict Mode) of UnaryExpression such as MemberExpression . PrivateIdentifier.

The problem is, OptionalChain is not an immediate derivation, but part of a composition.

So I believe the intention is to add he derivations I'm suggesting here. It's a bit verbose but accurate.

The original text would target cases like:

delete ?. # 
delete ?.x.#y

but they are already not possible.

In the suggested prose we disallow:

delete this?.#x // OptionalExpression : MemberExpression `?.` PrivateIdentifier
delete this?.x.#y // OptionalExpression : MemberExpression OptionalChain PrivateIdentifier
delete this?.x?.#x // OptionalExpression : OptionalExpression `?.` PrivateIdentifier
delete this?.x?.y.#x // OptionalExpression : OptionalExpression OptionalChain PrivateIdentifier
delete super()?.#x // OptionalExpression : CallExpression `?.` PrivateIdentifier
delete super()?.x.#y // OptionalExpression : CallExpression OptionalChain PrivateIdentifier
delete super().x?.#x
delete super()?.x?.y.#x

am I missing anything?

caiolima commented 4 years ago

This feels like a bug in the current private fields spec. But one thing that's confusing me is the part ...and the derived |UnaryExpression| is.... It is not clear to me is how I should interpret it. If we consider thatOptionalChain : ?. PrivateIdentifier is the derived |UnaryExpression| is true if part of resolution of UnaryExpression matches OptionalChain :?.PrivateIdentifier, then current text is good. But I can't assure that, because I don't fully understand the meaning of the derived |UnaryExpression|

leobalter commented 4 years ago

IMO, the derivation means an expanded form of the given production, rather than contains. This means something like delete x[y?.#z] is not the case in here.

bakkot commented 4 years ago

I understand the "the derived |UnaryExpression| is |PrimaryExpression| : |IdentifierReference|" to mean that the parse tree you get when parsing the |UnaryExpression| part of an occurrence of delete |UnaryExpression| ends up being a series of productions of the form |A| : |B| all the way down the tree until you end up with |PrimaryExpression| : |IdentifierReference|.

That is, it has to be the whole production, not just a part of it.

And yes, @leobalter is correct that the prose currently written talks about delete ?.#b, which isn't a thing. Unfortunately I don't think proposed fix would work: it talks about |OptionalExpression| : |MemberExpression|?.|PrivateIdentifier|, but that isn't actually a single production in the grammar.

Unless someone has a better idea, I expect the fix is going to involve a new IsPrivateFieldReference defined over the expression grammar.

leobalter commented 4 years ago

Unfortunately I don't think proposed fix would work: it talks about |OptionalExpression| : |MemberExpression| ?. |PrivateIdentifier|, but that isn't actually a single production in the grammar.

Here is my original thought, expanding the productions to the listed derived parts as the final nested parts:

UnaryExpression :
  UpdateExpression : 
    LeftHandSideExpression : 
      OptionalExpression : 
        MemberExpression OptionalChain : // Expands OptionalChain below
          MemberExpression `?.` PrivateIdentifier
          MemberExpression OptionalChain `.` PrivateIdentifier
        CallExpression OptionalChain :  // Expands OptionalChain below
          CallExpression `?.` PrivateIdentifier
          CallExpression OptionalChain `.` PrivateIdentifier
        OptionalExpression OptionalChain :  // Expands OptionalChain below
          OptionalExpression `?.` PrivateIdentifier
          OptionalExpression OptionalChain `.` PrivateIdentifier

This has some limitations but I'm not sure how this proposal wants to tackle this:

// Invalid: MemberExpression OptionalChain `.` PrivateIdentifier
delete x?.y.#z

// Would be valid: MemberExpression OptionalChain `.` IdentifierName
delete x?.#y.z

and etc.

bakkot commented 4 years ago

expanding the productions to the listed derived parts as the final nested parts

Yeah, I see how you got there, I am just not convinced it makes sense to talk about a production where you've partially expanded one of the nonterminals.

leobalter commented 4 years ago

I've lost my previous answer as Chrome just auto scrolled the whole GitHub page to the left until it was completely gone and there was nothing I could do to recover it.

I see your point.

In this case I'd like to clarify if this proposal also wants to prevent delete x?.#y.z so we could just use:

It is a Syntax Error if the |UnaryExpression| is contained in strict mode code and contains <emu-grammar>OptionalChain : `?.` PrivateIdentifier</emu-grammar>, or <emu-grammar>OptionalChain : OptionalChain `.` PrivateIdentifier</emu-grammar>.
bakkot commented 4 years ago

In this case I'd like to clarify if this proposal also wants to prevent delete x?.#y.z

I would not expect it to, that would be surprising.

Edit: also the thing you wrote would also prevent delete f(x?.#y).a, which would be even more surprising.

leobalter commented 4 years ago

Thanks for the clarification.

In this case, we don't have precedent for this, but we could try "ends with" rather than "contains":

It is a Syntax Error if the |UnaryExpression| is contained in strict mode code and ends with <emu-grammar>PrivateIdentifier</emu-grammar>.
bakkot commented 4 years ago

Hm, I think I'd prefer introducing a new abstract operation over that.

leobalter commented 4 years ago

In this cause I need more time to think how to handle this or also open to anyone's suggestion.

Edit: probably following IsIdentifierRef.

lightmare commented 3 years ago

I think this is unnecessarily wordy, as is the current spec with separate restrictions on MemberExpression, CallExpression, and OptionalChain.

Edit: sorry, got it totally wrong, retracting. Edit 2: hopefully I fixed the mistake I made previously.

If I understand correctly, the language you want to forbid as delete argument is generated by this grammar:

MemberExpression . PrivateIdentifier
MemberExpression ?. PrivateIdentifier
CallExpression . PrivateIdentifier
CallExpression ?. PrivateIdentifier
OptionalExpression . PrivateIdentifier
OptionalExpression ?. PrivateIdentifier

Plus I think two of the restrictions in the current spec (?. PrivateIdentifier and OptionalChain . PrivateIdentifier) are wrong — or ineffectual to be more precise — as these don't generate any valid UnaryExpression, i.e. the thing being restricted.

Side question: why is this restriction strict-mode-only? I understand why for regular identifiers, but for private?

nicolo-ribaudo commented 3 years ago

Side question: why is this restriction strict-mode-only? I understand why for regular identifiers, but for private?

Private fields are a syntax error outside of classes (which are always strict), so it doesn't matter how it would behave in sloppy mode.