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

add a test for eval?.(code) #2667

Merged
merged 1 commit into from
Jun 22, 2020
Merged

Conversation

mysticatea
Copy link
Contributor

This PR adds a test for eval?.(code). It's indirect eval.

  12.3.9.2 Runtime Semantics: ChainEvaluation
    OptionalChain: ?. Arguments
      1. Let thisChain be this OptionalChain.
      2. Let tailCall be IsInTailPosition(thisChain).
      3. Return ? EvaluateCall(baseValue, baseReference, Arguments, tailCall).

https://tc39.es/ecma262/#sec-optional-chaining-chain-evaluation

@leobalter leobalter merged commit 28c6ea2 into tc39:master Jun 22, 2020
@leobalter
Copy link
Member

leobalter commented Jun 22, 2020

Calling implementors to check this out: @syg @codehag @anba @kmiller68 @rkirsling.

I hope we can avoid a new de-facto direct eval.

@anba
Copy link
Contributor

anba commented Jun 22, 2020

@mysticatea (thank you!) filed https://bugzilla.mozilla.org/show_bug.cgi?id=1647169 against SpiderMonkey, which we already fixed today.

@mysticatea mysticatea deleted the eval-optional-call branch June 22, 2020 19:01
@syg
Copy link
Contributor

syg commented Jun 22, 2020

I actually feel like this would be the least confusing if it were a direct eval.

@leobalter
Copy link
Member

I'm currently reviewing the current tests for Optional Chaining. I can update the tests from indirect to direct eval if we have a matching PR in ECMA-262. Just let me know!

moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this pull request Jun 22, 2020
Test case will be added through the next test262 sync:
- tc39/test262#2667

Differential Revision: https://phabricator.services.mozilla.com/D80416
xeonchen pushed a commit to xeonchen/gecko that referenced this pull request Jun 23, 2020
Test case will be added through the next test262 sync:
- tc39/test262#2667

Differential Revision: https://phabricator.services.mozilla.com/D80416
@rkirsling
Copy link
Member

rkirsling commented Jun 23, 2020

To echo @syg, I find it surprising (both as a reviewer and as an implementer) that this would be an exception to the "code with ?. behaves the same as code without it when the LHS is non-nullish" rule of thumb.

tc39/proposal-optional-chaining#21 suggests that this was not an accident, but given that browser-hosted engines all implemented and shipped what we expected the spec to say, I believe we need a proper discussion in order to resolve this divergence (even if we decide to leave the spec as it is).

I'm not sure how this discussion should be conducted (or if there's a good precedent for such a situation)—does a spec issue thread suffice or should we add a topic to the next meeting agenda?

@leobalter
Copy link
Member

I'm not sure how this discussion should be conducted (or if there's a good precedent for such a situation)—does a spec issue thread suffice or should we add a topic to the next meeting agenda?

I believe so. We will need someone to champion this work at ECMA-262. For this specific change I believe it needs a Normative PR making sure that eval?.(x) will trigger PerformEval with the direct flag set as true.

@ljharb
Copy link
Member

ljharb commented Jun 23, 2020

We can have the discussion without the normative PR, but filing it does accelerate things in the event the change gets consensus.

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

Successfully merging this pull request may close these issues.

6 participants