Normative: Allow null or undefined in Reference Records #2267

codehag · 2021-01-05T15:39:18Z

Hi folks,

The goal of this pr is to address the concern brought up in #1224 -- which has become more pressing. With the introduction of private fields, implementations will again diverge on this point from the specification.

There were two suggested solutions to this, one was re-evaluation, which was rejected in past meetings. The other was reordering.

This pr allows null and undefined as values in Reference Records. This means that throwing is moved to ToObject in most cases.

We are not too sure about the super code though, and could use some extra eyes. See the past discussion here

jorendorff · 2021-01-05T23:04:53Z

I think this patch is the simplest way to implement "reordering".

Currently, the spec says null.y = f() should fail before calling f; it happens in the CheckObjectCoercible calls that the patch removes.

With the patch, the same check will effectively happen anyway, but later, in the ToObject calls that are made fallible (changing from ! to ?).

caiolima · 2021-01-12T17:34:04Z

Just for the record, I left a comment about this PR on codehag#3.

ljharb · 2021-01-25T19:38:15Z

Does this PR need any test262 tests?

codehag · 2021-01-25T19:38:42Z

Yes, it needs some adjustments. I think someone may have started it already, if not I will look into it (soon).

jugglinmike · 2021-05-20T23:48:27Z

I'm writing some Test262 tests for this, and I wanted to confirm my interpretation.

I believe that with this patch applied, web reality will continue to diverge when it comes to the ToPropertyKey operation in the "MemberExpression [ Expression ]" production. That's not related to the Class Fields proposal, so it may be an intentional decision to limit scope. I'd like to verify since this is implied by the motivating bug report and since web reality is part of the goal.

function prop() {
  print('property name value');
  return { toString() { print('property name string'); } };
}
[][prop()] = print('value');

Today, the standard specifies the following sequence:

property name value
property name string
value

...but SpiderMonkey, V8, and JSC behave as follows:

property name value
value
property name string

With this patch applied, my reading is that the spec will continue to describe the same sequence it does today.

bakkot · 2021-05-21T00:16:19Z

@jugglinmike That matches my interpretation. Specifically, this PR only relates to the point at which we check for null and undefined, not the order (and count) of property key coercion discussed in #467 (which will remain open after this PR).

jugglinmike · 2021-05-21T00:19:52Z

Got it. Thanks for confirming, Kevin! (And oof about the differences in count.)

jugglinmike · 2021-05-21T02:10:57Z

spec.html

@@ -14205,7 +14202,7 @@ <h1>Runtime Semantics: Evaluation</h1>
            1. Return *true*.
          1. If IsPropertyReference(_ref_) is *true*, then
            1. If IsSuperReference(_ref_) is *true*, throw a *ReferenceError* exception.
-            1. [id="step-delete-operator-toobject"] Let _baseObj_ be ! ToObject(_ref_.[[Base]]).
+            1. [id="step-delete-operator-toobject"] Let _baseObj_ be ? ToObject(_ref_.[[Base]]).


This appears to change an edge case related to the null and undefined check--one that may be unintentional.

class C { static m() { delete super.x; } } Object.setPrototypeOf(C, null); C.m();

In the spec today, evaluating super.x will produce a TypeError due to its use of RequireObjectCoercible (that is, before the runtime semantics of the delete operator begin).

With this change applied, evaluation makes it to the delete operator, but the ReferenceError from the IsSuperReference check will be thrown before ToObject has a chance to throw a TypeError.

There's implementation divergence here, too. Today, V8 and JSC throw the ReferenceError while SpiderMonkey and Engine262 throw the TypeError.

If my reading is accurate and we want to preserve the currently-specified behavior, it may be as simple as swapping this step and the one immediately before it.

Pardon me: the method in the example code ought to be static. I'll update it accordingly.

Thanks for pointing this out.

Personally I think the behavior in this PR is reasonable: checking whether something is a super reference seems more "static" than checking that the resolved value is object-coercible, so it makes sense to do the super check first. (In fact, I think it would have been possible to forbid delete super.x at parse time; I'm not sure why we made it a runtime error instead.)

Given that, and since we got consensus on this PR as written, I'm going to go ahead with the current behavior. But I'll call it out at the next meeting to make sure everyone is aware, and give people a chance to raise an issue if they'd like it modified.

Got it. Here's the corresponding change for Test262.

These tests support the following normative change Normative: Allow null or undefined in Reference Records tc39/ecma262#2267

These tests support the following normative change "Normative: Allow null or undefined in Reference Records" tc39/ecma262#2267 The tests concerning the `delete` operator increase coverage to verify behavior which, though related, is not altered by the normative change. These tests are intended to guard against regressions as engines implement the new semantics.

devsnek · 2021-07-18T17:30:39Z

This has tests now right?

bakkot · 2021-07-18T18:08:05Z

Indeed, looks like tests landed in tc39/test262#2993. I'll get this rebased and we can land it (pending another stamp, which I'll ping other editors for at the editor call).

jmdyck · 2021-07-19T03:21:47Z

spec.html

@@ -18452,9 +18451,8 @@ <h1>
      </dl>
      <emu-alg>
        1. Assert: _identifierName_ is an |IdentifierName|.
-        1. Let _bv_ be ? RequireObjectCoercible(_baseValue_).


By removing the call to RequireObjectCoercible, it appears that EvaluatePropertyAccessWithIdentifierKey can no longer return an abrupt completion, so invocations of it should be prefixed with ! rather than ?.

Nice catch, done.

The following proposed change modifies the semantics this test was originally written to verify: Normative: Allow null or undefined in Reference Records tc39/ecma262#2267

syg

lgtm

anba · 2021-07-22T12:43:49Z

Don't we also want to remove the RequireObjectCoercible calls before private references?

This check is no longer needed after <tc39/ecma262#2267>. Differential Revision: https://phabricator.services.mozilla.com/D120609

Co-authored-by: codehag <[email protected]> Co-authored-by: Kevin Gibbons <[email protected]>

This is part of an old normative change that happened soon after Andreas made `super` closer to spec in 1270df2. See tc39/ecma262#2267 This was introduced into bytecode by virtue of copy and paste :^) Bytecode results: Summary: Diff Tests: +2 ✅ -2 ❌

ljharb added needs consensus This needs committee consensus before it can be eligible to be merged. normative change Affects behavior required to correctly evaluate some ECMAScript source text web reality labels Jan 5, 2021

ljharb added has consensus This has committee consensus. and removed needs consensus This needs committee consensus before it can be eligible to be merged. labels Jan 25, 2021

jridgewell mentioned this pull request Jan 25, 2021

January 2021 babel/proposals#70

Open

ljharb added the needs test262 tests The proposal should specify how to test an implementation. Ideally via github.com/tc39/test262 label Jan 25, 2021

jugglinmike reviewed May 21, 2021

View reviewed changes

jugglinmike added a commit to bocoup/test262 that referenced this pull request May 21, 2021

Add tests for normative change

4d508ea

These tests support the following normative change Normative: Allow null or undefined in Reference Records tc39/ecma262#2267

jugglinmike mentioned this pull request May 21, 2021

Test sequence during deletion of SuperProperty tc39/test262#2992

Merged

jugglinmike mentioned this pull request May 21, 2021

Add tests for change in Reference Records tc39/test262#2993

Merged

ljharb force-pushed the master branch 3 times, most recently from 3d0c24c to 7a79833 Compare June 29, 2021 02:21

bakkot added has test262 tests and removed needs test262 tests The proposal should specify how to test an implementation. Ideally via github.com/tc39/test262 labels Jul 18, 2021

bakkot force-pushed the null-or-undefined-reference-records branch from 7f38832 to d4cfaf1 Compare July 18, 2021 20:09

bakkot approved these changes Jul 18, 2021

View reviewed changes

bakkot added the editor call to be discussed in the next editor call label Jul 18, 2021

jmdyck reviewed Jul 19, 2021

View reviewed changes

jugglinmike mentioned this pull request Jul 19, 2021

Update test for deletion of SuperProperty tc39/test262#3055

Merged

syg approved these changes Jul 21, 2021

View reviewed changes

codehag force-pushed the null-or-undefined-reference-records branch from 24de565 to e01feba Compare August 10, 2021 10:24

syg approved these changes Aug 11, 2021

View reviewed changes

bakkot added ready to merge Editors believe this PR needs no further reviews, and is ready to land. and removed editor call to be discussed in the next editor call labels Aug 14, 2021

Normative: Allow null or undefined in Reference Records (tc39#2267)

d09532c

Co-authored-by: codehag <[email protected]> Co-authored-by: Kevin Gibbons <[email protected]>

ljharb force-pushed the null-or-undefined-reference-records branch from e01feba to d09532c Compare August 15, 2021 19:36

ljharb merged commit d09532c into tc39:master Aug 15, 2021

mathiasbynens pushed a commit to mathiasbynens/ecma262 that referenced this pull request Oct 18, 2021

Normative: Allow null or undefined in Reference Records (tc39#2267)

b6c84e0

Co-authored-by: codehag <[email protected]> Co-authored-by: Kevin Gibbons <[email protected]>

This was referenced Feb 10, 2022

Incorrect tests after change to evaluation order for member assignment tc39/test262#3407

Open

Is null[{ toString: () => { console.log('hit') } }] supposed to hit the console.log? #2659

Closed

h2oche mentioned this pull request Mar 14, 2022

'null' check before property access es-meta/esmeta#62

Closed

1 task

iamstolis mentioned this pull request Nov 10, 2022

[Bug]: Symbol.toPrimitive in property acess oracle/graaljs#673

Open

Lubrsi mentioned this pull request Jul 5, 2023

LibJS/Bytecode: Use proper this for receiver in get/set for super expr and avoid RequireObjectCoercible when creating super references SerenityOS/serenity#19820

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Normative: Allow null or undefined in Reference Records #2267

Normative: Allow null or undefined in Reference Records #2267

codehag commented Jan 5, 2021 •

edited

Loading

jorendorff commented Jan 5, 2021

caiolima commented Jan 12, 2021

ljharb commented Jan 25, 2021

codehag commented Jan 25, 2021

jugglinmike commented May 20, 2021

bakkot commented May 21, 2021

jugglinmike commented May 21, 2021

jugglinmike May 21, 2021 •

edited

Loading

jugglinmike May 21, 2021

bakkot Jul 18, 2021

jugglinmike Jul 19, 2021

devsnek commented Jul 18, 2021

bakkot commented Jul 18, 2021 •

edited

Loading

jmdyck Jul 19, 2021

bakkot Jul 19, 2021

syg left a comment

anba commented Jul 22, 2021

Normative: Allow null or undefined in Reference Records #2267

Normative: Allow null or undefined in Reference Records #2267

Conversation

codehag commented Jan 5, 2021 • edited Loading

jorendorff commented Jan 5, 2021

caiolima commented Jan 12, 2021

ljharb commented Jan 25, 2021

codehag commented Jan 25, 2021

jugglinmike commented May 20, 2021

bakkot commented May 21, 2021

jugglinmike commented May 21, 2021

jugglinmike May 21, 2021 • edited Loading

Choose a reason for hiding this comment

jugglinmike May 21, 2021

Choose a reason for hiding this comment

bakkot Jul 18, 2021

Choose a reason for hiding this comment

jugglinmike Jul 19, 2021

Choose a reason for hiding this comment

devsnek commented Jul 18, 2021

bakkot commented Jul 18, 2021 • edited Loading

jmdyck Jul 19, 2021

Choose a reason for hiding this comment

bakkot Jul 19, 2021

Choose a reason for hiding this comment

syg left a comment

Choose a reason for hiding this comment

anba commented Jul 22, 2021

codehag commented Jan 5, 2021 •

edited

Loading

jugglinmike May 21, 2021 •

edited

Loading

bakkot commented Jul 18, 2021 •

edited

Loading