proposal-class-fields icon indicating copy to clipboard operation
proposal-class-fields copied to clipboard

Question about delete expression + opt chain

Open leobalter opened this issue 5 years ago • 12 comments

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?

leobalter avatar Jul 16 '20 23:07 leobalter

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|

caiolima avatar Jul 17 '20 12:07 caiolima

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.

leobalter avatar Jul 17 '20 15:07 leobalter

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.

bakkot avatar Jul 17 '20 16:07 bakkot

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.

leobalter avatar Jul 17 '20 19:07 leobalter

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.

bakkot avatar Jul 17 '20 19:07 bakkot

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>.

leobalter avatar Jul 17 '20 19:07 leobalter

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.

bakkot avatar Jul 17 '20 19:07 bakkot

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>.

leobalter avatar Jul 17 '20 19:07 leobalter

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

bakkot avatar Jul 17 '20 20:07 bakkot

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

Edit: probably following IsIdentifierRef.

leobalter avatar Jul 17 '20 20:07 leobalter

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

lightmare avatar May 30 '21 11:05 lightmare

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.

nicolo-ribaudo avatar May 30 '21 17:05 nicolo-ribaudo