Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Normative: Add Optional Chaining #1646

Open
wants to merge 12 commits into
base: master
from

Conversation

@DanielRosenwasser
Copy link
Contributor

DanielRosenwasser commented Jul 25, 2019

This change adds the relevant updates for the stage 3 Optional Chaining proposal. The intended changes can be viewed at https://tc39.es/proposal-optional-chaining/.

The Test262 PR is available at tc39/test262#2212

@ljharb
ljharb approved these changes Jul 25, 2019
@ljharb ljharb requested review from zenparsing, tc39/ecma262-editors and waldemarhorwat Jul 25, 2019
@ljharb ljharb added the needs tests label Jul 25, 2019
@ljharb ljharb requested a review from jridgewell Jul 25, 2019
@Wavez
Wavez approved these changes Oct 11, 2019
@ljharb ljharb removed the request for review from zenparsing Oct 12, 2019
@ljharb ljharb requested review from syg, michaelficarra and bakkot Oct 30, 2019
@DanielRosenwasser DanielRosenwasser force-pushed the DanielRosenwasser:optional-chaining branch from 5c5191a to 022a158 Oct 31, 2019
@ljharb ljharb added has tests and removed needs tests labels Oct 31, 2019
spec.html Show resolved Hide resolved
spec.html Show resolved Hide resolved
spec.html Show resolved Hide resolved
spec.html Show resolved Hide resolved
@syg

This comment has been minimized.

Copy link
Contributor

syg commented Nov 12, 2019

Could someone update this PR to reflect the upstream fixes and editorial changes?

spec.html Outdated Show resolved Hide resolved
@DanielRosenwasser DanielRosenwasser force-pushed the DanielRosenwasser:optional-chaining branch from 022a158 to af15638 Nov 13, 2019
@DanielRosenwasser

This comment has been minimized.

Copy link
Contributor Author

DanielRosenwasser commented Nov 13, 2019

I've integrated the upstream changes and addressed feedback provided apart from the Evaluate_______PropertyAccess naming discussion which I'll try to address shortly.

spec.html Show resolved Hide resolved
spec.html Outdated
<emu-grammar>OptionalChain : `?.` IdentifierName</emu-grammar>
<emu-alg>
1. If _symbol_ is a |ReservedWord|, return *false*.
1. If _symbol_ is an |Identifier| and StringValue of _symbol_ is the same value as the StringValue of |IdentifierName|, return *true*.

This comment has been minimized.

Copy link
@bakkot

bakkot Nov 13, 2019

Contributor

If #1519 lands before this PR, this step will need to be removed both places it occurs.

spec.html Outdated Show resolved Hide resolved
@bakkot
bakkot approved these changes Nov 14, 2019
Copy link
Contributor

bakkot left a comment

LGTM other than the assert.

@DanielRosenwasser DanielRosenwasser force-pushed the DanielRosenwasser:optional-chaining branch from bb51e63 to 3d3a71e Nov 15, 2019
@rkirsling

This comment has been minimized.

Copy link
Member

rkirsling commented Nov 15, 2019

Hmm, Expression / Identifier is one thing, but I don't think I understand why Key is in there.

(FWIW, not to bikeshed on and on, but one more potential option could be EvaluatePropertyAccessBy[Identifier|Value|PrivateName].)

@DanielRosenwasser

This comment has been minimized.

Copy link
Contributor Author

DanielRosenwasser commented Nov 15, 2019

@rkirsling it was just to disambiguate that the Identifier or Expression is the key, not the base value. Maybe a better name would be LookUpPropertyByIdentifierName and LookUpPropertyByValue.

@rkirsling

This comment has been minimized.

Copy link
Member

rkirsling commented Nov 16, 2019

Not sure how the others feel, but that suggestion sounds pretty good to me.

@michaelficarra

This comment has been minimized.

Copy link
Member

michaelficarra commented Nov 18, 2019

I really hate bikeshedding something as unimportant as this, but really, "Identifier"/"IdentifierName" should not be in these names.

@syg

This comment has been minimized.

Copy link
Contributor

syg commented Nov 18, 2019

I am fine with literally anything for those names so long as it doesn't require me to intuit what's dynamic and what's static.

@devsnek

This comment has been minimized.

Copy link
Member

devsnek commented Nov 18, 2019

In engine262 we'd call them EvaluateMemberExpressionWithIdentifier and EvaluateMemberExpressionWithExpression

@DanielRosenwasser

This comment has been minimized.

Copy link
Contributor Author

DanielRosenwasser commented Nov 18, 2019

I think using the syntactically-guided names (as it currently stands) is probably the best way forward. I'm also fine with switching to EvaluatePropertyAccessWith[Identifier|Expression]Key if there's a preference there.

Keep in mind that this can be switched later if it causes legitimate issues, so I'd urge people not to keep picking out new colors for the bikeshed. 😄

@DanielRosenwasser DanielRosenwasser force-pushed the DanielRosenwasser:optional-chaining branch from 3d3a71e to d0d60dc Nov 18, 2019
@syg
syg approved these changes Nov 19, 2019
Copy link
Contributor

syg left a comment

Keep in mind that this can be switched later if it causes legitimate issues, so I'd urge people not to keep picking out new colors for the bikeshed.

Agreed. This editorial concern certainly doesn't block stage 4 and can be easily changed after merging into master.

@rkirsling

This comment has been minimized.

Copy link
Member

rkirsling commented Nov 19, 2019

I'm fine with virtually everything that's been suggested (particularly the recent suggestions involving By / With), but I do think the current naming in particular is hard to parse. It makes me ask, "What's a key property? Or is it a key property access?" But alas, it's an expression-keyed access of a property.

@DanielRosenwasser DanielRosenwasser force-pushed the DanielRosenwasser:optional-chaining branch from d0d60dc to 7d687c4 Nov 19, 2019
@ljharb ljharb assigned ljharb and unassigned DanielRosenwasser Nov 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.