Implement computed properties #1503
Conversation
It has a bug: code like this doesn't work correctly:
```javascript
var i = 0;
var o = {
[i++]: i++,
[i++]: i++,
}
```
This is because this implementation does not evaluate keys and values
interleaved, as the spec says.
Accidentally broken bug 757410, working on it
I had run the update of test262 using Java 17, which makes some more tests pass locally but not on CI. I have re-run it now under Java 11. Should fix the CI run.
|
Wow, this is a really important and complete PR! Thanks for all the details. I looked at the code and nothing jumped out at me, but give us all a few days to try this out and digest to see if anyone else has comments. |
|
I have one idea which might make things more complicated in the short run: I've noticed that the code in ScriptRuntime that tries to optimize object literal creation is actually less efficient than just creating an object and setting properties. In other words, this: let o = { a: 1, b: 2 }; is actually slower than: let o = {}; o.a = 1; o.b = 2; This will become more the case if I ever get any InvokeDynamic optimizations to have a good effect. So at least in bytecode mode, do we really need all that? Or would it be easier for you to unwind the object literal creation bytecode to just create an object and set each field one at a time? |
|
The section in test262.properties related to computed properties needs to be uncommented (remove the trailing ~), so the tests are included: rhino/tests/testsrc/test262.properties Line 2671 in c51f1c1 Hopefully they'll all pass afterwards 😃 |
IMHO the current strategy could also have some benefits - for example, we could create the slot map with a good initial size. And, honestly, if we want to do that change, I'd rather to do it in a separate PR following this one. 🙂 |
Done! Most of the ones not passing are using var object = {
*['a']() {
yield 1;
yield 2;
}
};But, the following also does not work, so it's not really related to this PR: var object = {
*a() {
yield 1;
yield 2;
}
}; |
|
Yup, that generation syntax is another issue: #1413 I remember previous attempts to implement computed properties had challenges getting property order requirements from the spec correct. Any idea of that works correctly in your PR? |
|
Thinking more about the object initialization implemented in the runtime -- I actually get that now, I think that actually if we ever do anything with property maps like V8 (and I have a number of experiments in flight that do that), then leaving things the way they are are better, so that we can in the future optimize for the case when we know what properties to add and in which order. |
Assuming you are talking about the fact that we need to "interleave" keys and values, it does. I.e. this does the same as v8 or any other js engine: x = 0;
o = {
[++x]: ++x,
[++x]: ++x,
}; |
|
@andreabergia how do you feel about this -- are you ready for another look and a possible merge, or do you plan to keep working on this and then ask for that later? |
From my side, this is ready to be merged. 🙂 🚀 |
|
It looks like a bunch of the test262 tests are triggering an IllegalArgumentException. This seems to be because something in the parser is triggering "Kit.codeBug". It's fine for invalid code to cause a RhinoException, but if we get to "codeBug" then we need to fix the parser even to handle illegal stuff without throwing the wrong kind of exception. These don't show up as test failures because these tests are marked to fail anyway, but they show up as test output and are stack traces that we don't want to emulate. Can you check them out? Stack trace here from java.lang.IllegalStateException: FAILED ASSERTION: ts.cursor=83, ts.tokenBeg=82, currentToken=91 |
Oh I missed that! I’ll take a look, thanks. |
|
Likely reason you missed this is because we don't print such exceptions atm, due to this. Once that one is fixed, we can enable printing unexpected exceptions while running tests (at least in the test262TestSuite) |
We don't support it, but we want to give back a proper error message rather than a generic assertion failed.
I think I fixed that, by just making the parser/code gen not throw an error, but a proper exception. The problem is in destructuring, supporting stuff like: var { [a]: b } = {};
function({ [a]: b }) {};I've tried to implement it, but it is quite complicated (I've hit issue #1074 too). And, in any case, I think the implementation can be considered good even without the support in destructuring. |
|
I think that this looks good and I'm happy to merge it. Thanks for doing all this! |
|
Super! |
This (rather large) PR implements computed properties, i.e. you can do things like:
A more obvious use case is something like
o = { [Symbol.iterator]: () => { ... } }. Quite a few more test262 cases now pass (around 50).I'm going to go into some details about the implementation to help the review, since there are a lot of changes. My apologies for the wall of text. 🙂
Closes #913
Background
Spec reference: https://tc39.es/ecma262/#sec-object-initializer
An important detail, per the spec, is that JS requires that the evaluation of the expressions for computed property keys and values are interleaved, e.g.:
should print
{"1":2,"3":4}.Implementation notes
Parser
The changes to
Parserare simple. The only noteworthy detail is that I have created a new subclass ofAstNodecalledComputedPropertyKey. This is useful for two things:andreain the scope, not a property calledandreain the object - having a wrapper node simplifies this;IR Factory
In
IRFactorythe existing fragment of code was generating the array of property keys and store it in a property calledOBJECT_IDS_PROPof the node. The new code also stores the computed properties in a new propertyOBJECT_IDS_COMPUTED_PROP. The two arrays have the same length, and there is anullelement in the position of properties where we have a computed property, whose expression is at the corresponding index in the other array.There are also some simple changes to handle the new ast node.
Interpreter
The existing code, for an object such as
o = { x: 11, 2: 12 }, would generate the following bytecode:Let's go through this:
LITERAL_NEW, which is pushing an array of length 2 (reading the index register) on the stack for the properties' values (not the keys!);11and set it, push12and set it;InterpreterDatawas used to store a java array containing the property keys (i.e.new Object[]{"x", 2})OBJECTLIT, which reads the property keys from the literal array and the property values from the stack and initializes the object.(There is a small additional detail: there is a third array on the stack that records whether a property is standard, a setter, a getter, or a method. This was not touched by this PR).
However, this approach doesn't work anymore if we need to replace an entry in the property keys. Therefore, I have changed the bytecode to do the following:
To implement this I have done some changes in the interpreter:
LITERAL_KEYS, that pushes the literal array with the property keys onto the stack. It is followed by one byte (0/1) which specifies whether we have to copy the literal array or not (if we don't have computed properties we save an array copy and just push the one in the literal table, since we won't change it);LITERAL_KEY_SETto replace an entry of the property keys array (which is assumed to be already on the stack), similar to the the existingLITERAL_SET;OBJECTLITinstruction will now read both arrays from the stack.Given this code:
this is the generated bytecode:
You can see that we execute in order the expressions
1(first value), thenf(x)(second key), then42(second value), and finally the valuef(2)(third value).Compiled classes
For the compiled classes, the existing code would do something like this: for the JS fragment
{ x: 11, 2: 12 }the following java bytecode would be generated:It's pretty simple: we simply create an array with two elements, set the first one to the constant x and the second one to the constant 2. In general, the code would first fill the array with all the keys, and then the array with all the values, evaluating each key and value's expression. However, to implement the interleaved evaluation required by the spec, the changes in
BodyCodegenare a bit bigger.First, I split the implementation of an object literal and an array literal in two functions for clarity. Then, we create both arrays and fill them one entry at a time, so the code becomes something like:
As an example, for the following JS:
we get the following JVM bytecode:
There is a special case for handling Rhino bug 757410 - if we are inside a generator, we instead do something like this:
The reasons for this are explained in #70
Other changes
The last interesting change is the one in
ScriptRuntime- the old code knew that a property key was always a string or an integer. Now, it can be any JS object, so we have to useScriptRuntime.toStringrather than just aninstanceof String.Further enhancements
There are some improvements that could be done on this, but they will come in separated PR. This one is big enough as it is. 🙂