Skip to content
This repository has been archived by the owner on Jan 25, 2022. It is now read-only.

Question about delete expression + opt chain #313

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

leobalter
Copy link
Member

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?

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:

```js
delete ?. # 
delete ?.x.#y
```

but they are already not possible.

In the suggested prose we disallow:

```js
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
Copy link
Collaborator

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
Copy link
Member Author

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
Copy link
Contributor

bakkot commented Jul 17, 2020

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
Copy link
Member Author

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
Copy link
Contributor

bakkot commented Jul 17, 2020

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
Copy link
Member Author

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
Copy link
Contributor

bakkot commented Jul 17, 2020

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
Copy link
Member Author

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
Copy link
Contributor

bakkot commented Jul 17, 2020

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

@leobalter
Copy link
Member Author

leobalter commented Jul 17, 2020

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
Copy link

lightmare commented May 30, 2021

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
Copy link
Member

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.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants