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: Allow [[ReferencedName]] in Reference Records to be a not-yet-resolved property key #3307

Merged

Conversation

rkirsling
Copy link
Member

Resolves #3295.

spec.html Outdated
@@ -19147,8 +19153,7 @@ <h1>
<emu-alg>
1. Let _propertyNameReference_ be ? Evaluation of _expression_.
1. Let _propertyNameValue_ be ? GetValue(_propertyNameReference_).
1. Let _propertyKey_ be ? ToPropertyKey(_propertyNameValue_).
1. Return the Reference Record { [[Base]]: _baseValue_, [[ReferencedName]]: _propertyKey_, [[Strict]]: _strict_, [[ThisValue]]: ~empty~ }.
1. Return the Reference Record { [[Base]]: _baseValue_, [[ReferencedName]]: _propertyNameValue_, [[Strict]]: _strict_, [[ThisValue]]: ~empty~ }.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
1. Return the Reference Record { [[Base]]: _baseValue_, [[ReferencedName]]: _propertyNameValue_, [[Strict]]: _strict_, [[ThisValue]]: ~empty~ }.
1. NOTE: In most cases, ToPropertyKey(_propertyNameValue_) will be performed immediately after this step. However, in the case of `a[b] = c`, it will not be performed until after evaluation of `c`.
1. Return the Reference Record { [[Base]]: _baseValue_, [[ReferencedName]]: _propertyNameValue_, [[Strict]]: _strict_, [[ThisValue]]: ~empty~ }.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wonderful, thanks!

Copy link
Member Author

@rkirsling rkirsling Mar 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

...though it seems like the linter thinks we're performing ToPropertyKey(_propertyNameValue_) instead of mentioning it in a NOTE. 🤔 Tried rephrasing to ditch the parens.

spec.html Outdated
@@ -19147,8 +19153,7 @@ <h1>
<emu-alg>
1. Let _propertyNameReference_ be ? Evaluation of _expression_.
1. Let _propertyNameValue_ be ? GetValue(_propertyNameReference_).
1. Let _propertyKey_ be ? ToPropertyKey(_propertyNameValue_).
1. Return the Reference Record { [[Base]]: _baseValue_, [[ReferencedName]]: _propertyKey_, [[Strict]]: _strict_, [[ThisValue]]: ~empty~ }.
1. Return the Reference Record { [[Base]]: _baseValue_, [[ReferencedName]]: _propertyNameValue_, [[Strict]]: _strict_, [[ThisValue]]: ~empty~ }.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same change is necessary for super:

class C extends class {} {
  m() {
    super[{toString() { console.log('ToPropertyKey'); } }] = console.log('rhs');
  }
}
(new C).m()

prints 'rhs' then 'ToPropertyKey'.

@bakkot
Copy link
Contributor

bakkot commented Mar 29, 2024

Calls to NamedEvaluation where the argument is _lref_.[[ReferencedName]] (e.g.) should probably be preceded with Assert: _lref_.[[ReferencedName]] is a Property Key or private name. since it is not totally obvious.

Alternatively, these could be changed to pass StringValue of |LeftHandSideExpression| (or whichever the relevant nonterminal is) instead. I think that ends up being clearer anyway, and since StringValue is not side-effecting it's ok to just call it a second time (the first having been during Evaluation).

@bakkot
Copy link
Contributor

bakkot commented Mar 29, 2024

Looks good other than those comments, thanks for doing this!

spec.html Show resolved Hide resolved
@rkirsling rkirsling force-pushed the loosen-referenced-name-in-reference-records branch 2 times, most recently from 82117e4 to 4d1a4b5 Compare March 30, 2024 11:02
@rkirsling rkirsling changed the title Allow [[ReferencedName]] in Reference Records to be a not-yet-resolved property key. Normative: Allow [[ReferencedName]] in Reference Records to be a not-yet-resolved property key Apr 10, 2024
@linusg
Copy link
Member

linusg commented Apr 10, 2024

This means the test linked in the referenced issue will be invalidated and needs to be updated or removed, correct?

@rkirsling
Copy link
Member Author

That is correct. If anybody's gung-ho to do it, I won't stop 'em, but otherwise I can do so. 😉

rkirsling added a commit to rkirsling/test262 that referenced this pull request Apr 11, 2024
Update S11.13.1_A7_T3.js now that consensus has been reached on tc39/ecma262#3307.
@rkirsling
Copy link
Member Author

Corresponding test262 PR: tc39/test262#4052

ptomato pushed a commit to rkirsling/test262 that referenced this pull request Apr 12, 2024
Update S11.13.1_A7_T3.js now that consensus has been reached on tc39/ecma262#3307.
@ptomato ptomato added has test262 tests has consensus This has committee consensus. labels Apr 12, 2024
ptomato pushed a commit to tc39/test262 that referenced this pull request Apr 12, 2024
* Update test for o[p] = f()

Update S11.13.1_A7_T3.js now that consensus has been reached on tc39/ecma262#3307.

* Rename test and add an analogous one for super.
@bakkot bakkot added needs test262 tests The proposal should specify how to test an implementation. Ideally via github.com/tc39/test262 and removed has test262 tests labels Apr 17, 2024
@bakkot
Copy link
Contributor

bakkot commented Apr 17, 2024

We should update these tests and also add a test for this case before merging.

rkirsling added a commit to rkirsling/test262 that referenced this pull request Apr 18, 2024
rkirsling added a commit to rkirsling/test262 that referenced this pull request Apr 18, 2024
rkirsling added a commit to rkirsling/test262 that referenced this pull request Apr 18, 2024
@rkirsling
Copy link
Member Author

Done! tc39/test262#4060

ptomato pushed a commit to tc39/test262 that referenced this pull request Apr 22, 2024
@ptomato
Copy link
Contributor

ptomato commented Apr 22, 2024

The additional tests are merged.

@ptomato ptomato 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 Apr 22, 2024
@ljharb ljharb added the editor call to be discussed in the next editor call label Apr 22, 2024
@@ -4264,6 +4264,9 @@ <h1>
1. [id="step-getvalue-toobject"] Let _baseObj_ be ? ToObject(_V_.[[Base]]).
1. If IsPrivateReference(_V_) is *true*, then
1. Return ? PrivateGet(_baseObj_, _V_.[[ReferencedName]]).
1. If _V_.[[ReferencedName]] is neither a String nor a Symbol, then
Copy link
Member

@michaelficarra michaelficarra Apr 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
1. If _V_.[[ReferencedName]] is neither a String nor a Symbol, then
1. If IsPropertyKey(_V_.[[ReferencedName]]) is *false*, then

Alternatively, I'm not sure why we have that AO instead of just saying "is a property key", since "property key" is a well-defined term. So we could get rid of that AO (as a separate PR) and then just use that phrasing.

Same below for PutValue, of course.

edit: Also evaluation of delete 🤦‍♂️

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good; opened #3316.

<td>a String, a Symbol, or a Private Name</td>
<td>The name of the binding. Always a String if [[Base]] value is an Environment Record.</td>
<td>an ECMAScript language value or a Private Name</td>
<td>The name of the binding. Always a String if [[Base]] value is an Environment Record. May temporarily be an ECMAScript language value other than a String or a Symbol in the case of a property reference for which ToPropertyKey has yet to be performed.</td>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think a sentence of the form "May be an ECMAScript language value [...] until yada yada" would be clearer than "May temporarily be [...]".

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good; added an Otherwise, to your suggestion to clarify that this just applies to property reference cases.

spec.html Show resolved Hide resolved
spec.html Outdated Show resolved Hide resolved
@ljharb ljharb removed the editor call to be discussed in the next editor call label Apr 24, 2024
spec.html Outdated
@@ -20501,7 +20507,7 @@ <h1>Runtime Semantics: Evaluation</h1>
1. If |LeftHandSideExpression| is neither an |ObjectLiteral| nor an |ArrayLiteral|, then
1. Let _lref_ be ? Evaluation of |LeftHandSideExpression|.
1. If IsAnonymousFunctionDefinition(|AssignmentExpression|) and IsIdentifierRef of |LeftHandSideExpression| are both *true*, then
1. Let _rval_ be ? NamedEvaluation of |AssignmentExpression| with argument _lref_.[[ReferencedName]].
1. Let _rval_ be ? NamedEvaluation of |AssignmentExpression| with argument StringValue of |LeftHandSideExpression|.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Here and below, this reads a bit awkward given the invocation of the SDO. Perhaps pull out a separate step like Let _lhs_ be the StringValue of |LeftHandSideExpression|.

Very weak preference.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Related: we already wanted to do this for other reasons. 👍

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

spec.html Outdated
@@ -20501,7 +20507,8 @@ <h1>Runtime Semantics: Evaluation</h1>
1. If |LeftHandSideExpression| is neither an |ObjectLiteral| nor an |ArrayLiteral|, then
1. Let _lref_ be ? Evaluation of |LeftHandSideExpression|.
1. If IsAnonymousFunctionDefinition(|AssignmentExpression|) and IsIdentifierRef of |LeftHandSideExpression| are both *true*, then
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know this doesn't have to be part of this PR, but we should update this line to match the other cases below.

Suggested change
1. If IsAnonymousFunctionDefinition(|AssignmentExpression|) and IsIdentifierRef of |LeftHandSideExpression| are both *true*, then
1. If IsAnonymousFunctionDefinition(|AssignmentExpression|) is *true* and IsIdentifierRef of |LeftHandSideExpression| is *true*, then

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've gone ahead and opened a separate PR for it here: #3330

Copy link
Member

@michaelficarra michaelficarra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@bakkot bakkot added the ready to merge Editors believe this PR needs no further reviews, and is ready to land. label May 29, 2024
@ljharb ljharb force-pushed the loosen-referenced-name-in-reference-records branch from 7e98c0e to 6e70429 Compare May 30, 2024 23:19
@ljharb ljharb merged commit 6e70429 into tc39:main May 30, 2024
7 checks passed
anba added a commit to anba/test262 that referenced this pull request Sep 2, 2024
Missing coverage encountered while implementing
<tc39/ecma262#3307> in SpiderMonkey.

Ensure environment lookups are performed in the correct order:
- keyed-destructuring-property-reference-target-evaluation-order-with-bindings.js

Ensure `delete super[elem]` steps are correctly performed:
- delete/super-property-topropertykey.js
- delete/super-property-uninitialized-this.js

Ensure ToPropertyKey for computed property names in object literals
correctly performed:
- object/computed-property-name-topropertykey-before-value-evaluation.js

Ensure `GetSuperBase` is executed before `ToPropertKey`:
- super/prop-expr-getsuperbase-before-topropertykey-*

Ensure `GetThisBinding` is executed first:
- super/prop-expr-uninitialized-this-*
anba added a commit to anba/test262 that referenced this pull request Sep 13, 2024
Missing coverage encountered while implementing
<tc39/ecma262#3307> in SpiderMonkey.

Ensure environment lookups are performed in the correct order:
- keyed-destructuring-property-reference-target-evaluation-order-with-bindings.js

Ensure `delete super[elem]` steps are correctly performed:
- delete/super-property-topropertykey.js
- delete/super-property-uninitialized-this.js

Ensure ToPropertyKey for computed property names in object literals
correctly performed:
- object/computed-property-name-topropertykey-before-value-evaluation.js

Ensure `GetSuperBase` is executed before `ToPropertKey`:
- super/prop-expr-getsuperbase-before-topropertykey-*

Ensure `GetThisBinding` is executed first:
- super/prop-expr-uninitialized-this-*
Ms2ger pushed a commit to anba/test262 that referenced this pull request Sep 20, 2024
Missing coverage encountered while implementing
<tc39/ecma262#3307> in SpiderMonkey.

Ensure environment lookups are performed in the correct order:
- keyed-destructuring-property-reference-target-evaluation-order-with-bindings.js

Ensure `delete super[elem]` steps are correctly performed:
- delete/super-property-topropertykey.js
- delete/super-property-uninitialized-this.js

Ensure ToPropertyKey for computed property names in object literals
correctly performed:
- object/computed-property-name-topropertykey-before-value-evaluation.js

Ensure `GetSuperBase` is executed before `ToPropertKey`:
- super/prop-expr-getsuperbase-before-topropertykey-*

Ensure `GetThisBinding` is executed first:
- super/prop-expr-uninitialized-this-*
Ms2ger pushed a commit to tc39/test262 that referenced this pull request Sep 20, 2024
Missing coverage encountered while implementing
<tc39/ecma262#3307> in SpiderMonkey.

Ensure environment lookups are performed in the correct order:
- keyed-destructuring-property-reference-target-evaluation-order-with-bindings.js

Ensure `delete super[elem]` steps are correctly performed:
- delete/super-property-topropertykey.js
- delete/super-property-uninitialized-this.js

Ensure ToPropertyKey for computed property names in object literals
correctly performed:
- object/computed-property-name-topropertykey-before-value-evaluation.js

Ensure `GetSuperBase` is executed before `ToPropertKey`:
- super/prop-expr-getsuperbase-before-topropertykey-*

Ensure `GetThisBinding` is executed first:
- super/prop-expr-uninitialized-this-*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
has consensus This has committee consensus. has test262 tests ready to merge Editors believe this PR needs no further reviews, and is ready to land.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Reality and spec differ on property key resolution timing for o[p] = f()
8 participants