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: Evaluate all computed names before any values in object literals #945

Closed
wants to merge 1 commit into from

Conversation

littledan
Copy link
Member

To make object literals consistent with decorator and field proposals, Brian
Terlson and Yehuda Katz proposed in the object evaluation order proposal that
computed property names in object literals be evaluated before any of the
right-hand sides. This patch implements that proposal.

The specification is organized by making PropertyDeclarationEvaluation return
a List of "fields", which for object literals, means key-value pairs which are
not methods Methods are excluded because they are evaluated in a way which cannot
have side effects or observe the world.

…terals

To make object literals consistent with decorator and field proposals, Brian
Terlson and Yehuda Katz proposed in the object evaluation order proposal that
computed property names in object literals be evaluated before any of the
right-hand sides. This patch implements that proposal.

The specification is organized by making PropertyDeclarationEvaluation return
a List of "fields", which for object literals, means key-value pairs which are
not methods Methods are excluded because they are evaluated in a way which cannot
have side effects or observe the world.
@littledan littledan added needs consensus This needs committee consensus before it can be eligible to be merged. needs test262 tests The proposal should specify how to test an implementation. Ideally via github.com/tc39/test262 normative change Affects behavior required to correctly evaluate some ECMAScript source text labels Jul 4, 2017
@ljharb
Copy link
Member

ljharb commented Jul 5, 2017

Could you provide an example where this would currently be observable?

@michaelficarra
Copy link
Member

@ljharb See tc39/test262#739.

@ljharb
Copy link
Member

ljharb commented Jul 5, 2017

Thanks, so in that test (which currently asserts 1,2,3,4,5,6), it'd need to change to assert 1,4,2,5,3,6?

@getify
Copy link
Contributor

getify commented Jul 5, 2017

I would hope not many people are doing side effects with their computed expressions, but... wow, this new proposed behavior looks super confusing to my eyes -- IIUC, to compute the whole set of property name expressions up-front before consulting their value assignments individually in order. In the linked test, 1, 4, 2, 5, 3, 6 is a very surprising result to me.


As an aside, another convenient way to illustrate the surprise of the proposed order is:

o = {
   [console.log(1)]: console.log(2),
   [console.log(3)]: console.log(4),
   [console.log(5)]: console.log(6)
};

Shown that code, I'd bet 999 out of 1000 JS developers will expect 1, 2, 3, 4, 5, 6 (or maybe 2, 1, 4, 3, 6, 5), not 1, 3, 5, 2, 4, 6.

Whatever (valid, I'm sure) reason there is for proposing this (potentially breaking) change, I wonder if this unintuitive outcome is worse than whatever inconsistency will result from preserving the current behavior.

@littledan
Copy link
Member Author

By the way, the proposal-class-fields specification is based on this PR; I meant to upload it a couple months ago but seem to have forgotten somehow.

@getify This proposal is part of a larger program, proposed by Yehuda and Brian, where it turns out that class declarations cannot be evaluated strictly left-to-right, top-to-bottom for several reasons once there are decorators and/or static public fields with computed property names. In that context, the hope is that these semantics would be simpler to understand as a whole, not more complicated.

@michaelficarra
Copy link
Member

@ljharb That is correct.

@leobalter
Copy link
Member

@getify The way it is presented to developers produce bias against expectations:

o = {
   [1]: 2,
   [3]: 4,
   [5]: 6,
};

// or

o = {
   [1]: 4,
   [2]: 5,
   [3]: 6,
};

I believe this is a reasonable breaking change, as some are pragmatic and opens a good path. This change affects class fields positively.

BTW, Safari does not conform with the current spec or even with the current changes.

@bterlson
Copy link
Member

bterlson commented Jul 5, 2017

Let's get this on the agenda for July!

@littledan
Copy link
Member Author

littledan commented Jul 5, 2017

All engines currently implement the current spec's semantics, not the proposed new semantics.

EDIT: OK, I can't read. Here's the eshost output:

littledan@littledan-ThinkPad-T460p:~$ eshost -e "{ [print(1)]: print(2), [print(3)]: print(4) }"
#### jsc
2
1
4
3
[object Object]

#### chakracore
1
2
3
4
[object Object]

#### spidermonkey
1
2
3
4
[object Object]

#### d8
1
2
3
4
[object Object]

@getify
Copy link
Contributor

getify commented Jul 7, 2017

@leobalter I'd say that object destructuring already strongly "presented" this assumption to developers. Destructuring works left-to-right, then top-down. I (and others) literally teach destructuring as just the declarative form of the individual assignments happening, in order.

And right or wrong, that's the mental model I've also used to think about computed properties. This proposal will mark a significant divergence between those two, and that's the thing I think that's a bad idea.

And this proposal appears to only be doing so in an effort to make more exotic use cases like decorators work. IMO we shouldn't be prioritizing a more sophisticated use-case over a more direct/straightforward one. If decorators can't work in a way that's consistent with how an established feature like destructuring works, then maybe that's a sign that decorators are ill-conceived and need more thought -- not just casually breaking the established mental precedent.

@littledan
Copy link
Member Author

@getify What would you think if classes started to break the top-down, left-right order? This is currently proposed for fields.

@getify
Copy link
Contributor

getify commented Jul 7, 2017

@littledan I think object literals are much more closely aligned, mental-model wise, with destructuring than are class declarations. Class declarations are a different beast and already break several precedents, like for example not needing/allowing intervening commas between items, etc. So, I personally think if the inconsistency needs to happen, it should happen with the form that's already different than on one which is so closely similar.

@getify
Copy link
Contributor

getify commented Jul 7, 2017

BTW, to elaborate slightly on my assertion earlier about the relationship to destructuring (which I implied but didn't explain):

let x = 1;
let {
   [++x]: y = ++x,
   [++x]: z = ++x
} = { 1: 1, 3: 3, 4: 4 };

x;  // 4
y;  // 3
z;  // 4

This is thought of and taught -- most commonly, I'd wager -- as syntactic sugar for:

let x = 1;
let o = { 1: 1, 3: 3, 4: 4 };
let tmp = o[++x];
let y = tmp != undefined ? tmp: ++x;
let tmp2 = o[++x];
let z = tmp2 != undefined ? tmp2 : ++x;

x;  // 4
y;  // 3
z;  // 4

In other words, each destructuring pattern is processed top-down, one at a time, and within each, left-to-right, exactly as if each is its own statement. That syntax is unmistakably similar to object literals, and certainly on purpose as such.

So when you do something almost the same in an object literal, it's not a stretch of the imagination at all to relate the two and have similar expectations:

let x = 1;
let y;
let z = 3;
let o = {
   [++x]: y != undefined ? y : ++x,
   [++x]: z != undefined ? z : ++x
};

o;   // { 2: 3, 4: 3 }

If you went and started messing with object literals' computed properties order, according to this proposal, but didn't change how destructuring works, you break that relationship in ways I find quite surprising.

@allenwb
Copy link
Member

allenwb commented Jul 7, 2017

@littledan @getify note that one reason that classes are different is that they add an additional time dimension to left-right, top-bottom. It is at class definition time or at instance construction time.

@sebmarkbage
Copy link
Contributor

Just dropping some more scenarios here to consider based on TC39 discussions.

let user, profile = {
  user: user = {
    name: 'Sebastian',
  },
  [user.name]: 'Name'
};
let {
  user: {
    name
  },
  [name]: label
} = profile;
let i = offset, {
  [i++]: {
    [i++]: a
  },
  [i++]: {
    [i++]: b
  }
} = nestedMatrix;

@littledan
Copy link
Member Author

We discussed this PR at the July 2017 TC39 meeting, but we didn't come to any sort of consensus. There seemed to be a bunch of support, but also a bunch of concern about losing the ordering (as discussed in this thread).

Do people here think it's worth continuing with this proposal and bringing it up for further discussion in the September meeting?

@bterlson
Copy link
Member

bterlson commented Aug 4, 2017

My understanding is that we got consensus on these semantics in Munich so the lack of "consensus" is around a delta from this PR not the PR itself. This PR is waiting on learning from implementations if this is web-compatible.

However, there were two possible tweaks raised to this PR that as-yet have not been fleshed out:

  1. Only applying this order to decorated object literals
  2. Applying this order everywhere except in assignment targets (to keep the mapping between destructuring and its corresponding imperative desurgaring straight forward as possible).

@littledan
Copy link
Member Author

I'm not really a fan of either of those tweaks. If we think people will care about evaluation order here at all, the worst thing we could do is make it subtly context-dependent.

@littledan
Copy link
Member Author

We're continuing to discuss class decorators in TC39, though object decorators are left for a follow-on proposal.

Where should we go from here on this patch? Should we make this change?

@anba
Copy link
Contributor

anba commented Mar 5, 2018

I haven't yet seen any response from implementers in this thread and I think for this case we definitely want to know how this effects VMs. And, because methods are unaffected by this PR, we also need to consider the changes on property iteration order this PR will cause.

@littledan
Copy link
Member Author

And, because methods are unaffected by this PR, we also need to consider the changes on property iteration order this PR will cause.

@anba Thanks so much for taking a close look at this patch. It was an unintentional bug to exclude methods in object literals here. Due to the organization of the specification, it'll be a somewhat bigger change to treat methods similarly, but the text should be similar to what's found in the private methods proposal.

It'll be a few weeks until I can take the time to write all this out. If anyone wants to take on this exercise, which can help this proposal move forward more quickly, you'd be more than welcome. Let me know here if you have any questions.

@littledan
Copy link
Member Author

A couple years after I wrote this, we haven't discussed this PR in committee again, and it hasn't been implemented. I think it might've been web-compatible to make these sorts of changes in the past, but this window is closing and may have already closed. Does anyone want to take this proposal up? If not, I'll close the PR.

@ljharb
Copy link
Member

ljharb commented Sep 2, 2019

Wasn’t the consistency between objects and classes considered an important thing to ensure as part of class fields?

@littledan
Copy link
Member Author

@ljharb That was, in a way, the motivation for this PR (especially when thinking about decorators). However, this PR is not part of the class fields proposal, and they should not be conflated. Computed class field names must be evaluated before, since they are evaluated different numbers of times. There are several differences between classes and objects, e.g., classes have their own inner scopes, so I don't think a difference here is fatal.

@ljharb
Copy link
Member

ljharb commented Oct 1, 2019

The committee discussed this PR today and decided not to go forward with it, due to the increased risk of web breakage since this was initially planned.

@ljharb ljharb closed this Oct 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs consensus This needs committee consensus before it can be eligible to be merged. needs test262 tests The proposal should specify how to test an implementation. Ideally via github.com/tc39/test262 normative change Affects behavior required to correctly evaluate some ECMAScript source text
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants