ADD: ES6 Object.values / Object.entries / Object.fromEntries #902

rPraml · 2021-05-20T14:38:03Z

Adds the missing Object.values / Object.entries / Object.fromEntries methods.

tuchida · 2021-05-20T14:48:21Z

After deleting these lines, try running ./gradlew test --tests Test262SuiteTest.

Lines 732 to 771 in b0fb5f8

    
               ! fromEntries/empty-iterable.js 
        
               ! fromEntries/evaluation-order.js 
        
               ! fromEntries/iterator-closed-for-null-entry.js 
        
               ! fromEntries/iterator-closed-for-string-entry.js 
        
               ! fromEntries/iterator-closed-for-throwing-entry-key-accessor.js 
        
               ! fromEntries/iterator-closed-for-throwing-entry-key-tostring.js 
        
               ! fromEntries/iterator-closed-for-throwing-entry-value-accessor.js 
        
               ! fromEntries/iterator-not-closed-for-next-returning-non-object.js 
        
               ! fromEntries/iterator-not-closed-for-throwing-done-accessor.js 
        
               ! fromEntries/iterator-not-closed-for-throwing-next.js 
        
               ! fromEntries/iterator-not-closed-for-uncallable-next.js 
        
               ! fromEntries/key-order.js 
        
               ! fromEntries/length.js 
        
               ! fromEntries/name.js 
        
               ! fromEntries/prototype.js 
        
               ! fromEntries/requires-argument.js 
        
               ! fromEntries/simple-properties.js 
        
               ! fromEntries/string-entry-object-succeeds.js 
        
               ! fromEntries/string-entry-primitive-throws.js 
        
               ! fromEntries/string-entry-string-object-succeeds.js 
        
               ! fromEntries/supports-symbols.js 
        
               ! fromEntries/to-property-key.js 
        
               ! fromEntries/uses-define-semantics.js 
        
               ! fromEntries/uses-keys-not-iterator.js 
        
               ! entries/exception-during-enumeration.js 
        
               ! entries/function-length.js 
        
               ! entries/function-name.js 
        
               ! entries/function-property-descriptor.js 
        
               ! entries/getter-adding-key.js 
        
               ! entries/getter-making-future-key-nonenumerable.js 
        
               ! entries/getter-removing-future-key.js 
        
               ! entries/inherited-properties-omitted.js 
        
               ! entries/observable-operations.js 
        
               ! entries/primitive-booleans.js 
        
               ! entries/primitive-numbers.js 
        
               ! entries/primitive-strings.js 
        
               ! entries/primitive-symbols.js 
        
               ! entries/symbols-omitted.js 
        
               ! entries/tamper-with-global-object.js 
        
               ! entries/tamper-with-object-keys.js

rhino/testsrc/test262.properties

Lines 826 to 841 in b0fb5f8

    
               ! values/exception-during-enumeration.js 
        
               ! values/function-length.js 
        
               ! values/function-name.js 
        
               ! values/function-property-descriptor.js 
        
               ! values/getter-adding-key.js 
        
               ! values/getter-making-future-key-nonenumerable.js 
        
               ! values/getter-removing-future-key.js 
        
               ! values/inherited-properties-omitted.js 
        
               ! values/observable-operations.js 
        
               ! values/primitive-booleans.js 
        
               ! values/primitive-numbers.js 
        
               ! values/primitive-strings.js 
        
               ! values/primitive-symbols.js 
        
               ! values/symbols-omitted.js 
        
               ! values/tamper-with-global-object.js 
        
               ! values/tamper-with-object-keys.js

rPraml · 2021-05-20T18:45:01Z

I could enable some of the tests. I also applied spotless in an extra commit. (is it correct, that spotless has to be applied on each modified file?)
How should I proceed? I can try to see, if I can get more tests working next week.
Can this PR get merged and can/should I fix the tests in an extra commit?

rbri

nice progress

tuchida · 2021-05-20T20:39:17Z

The JavaScript specification is defined in ECMA262.
https://262.ecma-international.org/11.0/#sec-object.values

It is difficult to read ECMA262 right off the bat, so I recommend reading MDN first.
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/values

rbri · 2021-05-20T20:50:20Z

Regarding spotless - yes every modified file needs spotless applied. Greg merges as soon as he finds the time for reviewing. It will be great if you like to work further on this making more test passing.

gbrail · 2021-05-20T21:08:16Z

Thanks for doing this!

Now that we're getting better at doing this stuff, I'd prefer if we could first figure out if we can implement a feature with as many of the test262 tests passing as possible before merging it.

Looking at the spec, Object.entries and Object.values are pretty straightforward and the implementation looks like it's probably pretty close.

On the other hand, Object.fromEntries is one of those JS functions with a bunch of edge cases and things that have to be generalized. You might want to take a look at the implementation of Array.from, for example.

One way I've found to figure out what I'm missing is to start with the V8 "mjsunit" test suite, and comment out things that don't work because other parts of the spec aren't implemented yet. For example:

https://github.com/v8/v8/blob/dc712da548c7fb433caed56af9a021d964952728/test/mjsunit/harmony/object-fromentries.js

It's often more productive to get things working by running those tests over and over before trying to wade through test262.

What would you think of just trying to finish "entries" and "values" to pass as many tests as we can and then tackling "fromEntries" separately?

tonygermano · 2021-05-21T05:44:40Z

src/org/mozilla/javascript/NativeObject.java

+                  for (int i = 0; i < ids.length; i++) {
+                      if (ids[i] instanceof Integer) {
+                          Integer key = (Integer) ids[i]; 
+                          ids[i] = cx.newArray(scope, new Object[]{ ids[i], obj.get(key, scope) });


I think the first element of the new Array should be a String, even when you need the integer index to do the lookup. Have you thought about implementing the EnumerableOwnPropertyNames abstract method and using it for keys, entries, and values?

rbri · 2021-05-21T09:05:03Z

Looks like someone is waiting for this :-)

https://stackoverflow.com/questions/67627106/typeerror-cannot-find-function-entries-in-object-function-object

rPraml · 2021-05-21T14:55:22Z

So, I got all of the "entries" and "values" test working.

Most of the "fromEntries" tests are using computed-properties like in this example:

var iterable = {
  [Symbol.iterator]: function() {
  ...
  }
}

Unfortunately, they are currently not yet parseable. I gave it a try in this commit 3756fa1 and extended the parser and codegenerators to handle computed properties.

Somehow it seems to work, but I need help here because I'm not sure why ;)
Maybe someone with more experience in parser/bytecode generation can grab this commit and continue my work or give me some feedback here: FOCONIS#2

gbrail · 2021-05-21T17:05:07Z

The fact that we don't have support for new syntax is a fine reason to disable some test262 tests.

But if you have entries() and values() working I'll reiterate my suggestion that you put those in their own PR so that we can all look at it and possibly merge it.

rbri · 2021-05-21T17:44:52Z

Why not also merge the fromEntries(). Most of the tests are passing - we have the same situation for other methods/tests.

rbri · 2021-05-21T17:47:16Z

Btw: have done a test of this whole pr using it in HtmlUnit and it works great

…ect.entries # Conflicts: # src/org/mozilla/javascript/NativeObject.java

rPraml

@gbrail @rbri
I see, the "fromEntries" has a lot of edge cases and discovered other bugs or unimplemented features in rhino.
So I am also torn between leaving the code "as it ist" or remove "fromEntries" again.

Some of the test262 test cases I got working:

They require some special code, e.g. to check Symbol.toPrimitive or if "key" is a function, that it is invoked.
I'm not sure, if this is a speciality of "fromEntries" or should be handled at some other place.

The other test cases often use "computed properties", so either the tests have to be rewritten or computed property support has to be added. I've done this in a very experimental commit
FOCONIS@98ef815

Are there plans, that computed-properties will be added? Maybe someone with more experience can take that commit as basis, as it will fix a lot of other testcases, too (See test262.properties

rPraml · 2021-05-25T09:36:52Z

src/org/mozilla/javascript/NativeObject.java

+                            if (so.has(SymbolKey.TO_PRIMITIVE, so)) {
+                                key = so.get(SymbolKey.TO_PRIMITIVE, so);
+                            }
+                        }


Edge case? if object has Symbol.toPrimitive -> take it.
Shouldn't this be handled by ScriptRuntime.toPrimitive - respectively ScriptableObject.getDefaultValue?

rPraml · 2021-05-25T09:45:53Z

src/org/mozilla/javascript/NativeObject.java

                        if (key instanceof Function) {
-                            Function fun = (Function)key;
-                            key = fun.call(cx, fun.getParentScope(), entry, ScriptRuntime.emptyArgs);
+                            Function fun = (Function) key;


The test fromEntries/evaluation-order.js expects, that method is called.

Can someone point out, where this happens in https://tc39.es/ecma262/#sec-object.fromentries ?
as far as I understand, it calls CreateDataPropertyOnObject and then ToPropertyKey, which calls ToPrimitive

I assume it may happen in "ToPrimitive", but I use ScriptRuntime.toString(key) which invokes ScriptabbleObject.getDefaultValue(String.class) - so maybe there is an implementation error.

rPraml · 2021-05-25T09:49:43Z

src/org/mozilla/javascript/ScriptRuntime.java

@@ -4291,23 +4291,29 @@ public static Scriptable newObjectLiteral(
            Object id = propertyIds[i];
            int getterSetter = getterSetters == null ? 0 : getterSetters[i];
            Object value = propertyValues[i];
-            if (id instanceof String) {
+            if (id instanceof Symbol) {


Required to support arrays like this:

var key = Symbol(); var arr = [key, 'value'];

rPraml · 2021-05-25T09:51:55Z

testsrc/org/mozilla/javascript/tests/es6/NativeObjectTest.java

+        Object result =
+                cx.evaluateString(
+                        scope,
+                        "var obj = {};" + "res = Object.assign(obj);" + "res === obj;",


why does spotless mess this up?

rPraml · 2021-05-25T09:58:18Z

testsrc/test262.properties

@@ -740,35 +739,8 @@ built-ins/Object
    ! fromEntries/iterator-not-closed-for-throwing-done-accessor.js
    ! fromEntries/iterator-not-closed-for-throwing-next.js
    ! fromEntries/iterator-not-closed-for-uncallable-next.js
-    ! fromEntries/key-order.js


Some more of the from-entries tests are passing now. The other tests use "computed-properties" which are currently not supported.

If computed props were supported (or tests would be rewritten) only these tests would fail:

! fromEntries/iterator-closed-for-null-entry.js ! fromEntries/iterator-closed-for-string-entry.js ! fromEntries/iterator-closed-for-throwing-entry-key-accessor.js ! fromEntries/iterator-closed-for-throwing-entry-key-tostring.js ! fromEntries/iterator-closed-for-throwing-entry-value-accessor.js

which comes to my next question: How do you close an iterator?
Do we need something like ScriptRuntime.enumClose beside enumInit / enumNext

gbrail · 2021-05-25T15:55:24Z

I'm sorry that I didn't realize this sooner, but according to the spec, the basic algorithm for pulling items from the iterator and loading the object is the same basic algorithm (in the spec) as the constructors for Map and WeakMap objects.

So, you can look at NativeMap's constructor, specifically the "loadFromIterable" function. You could possibly move that function somewhere else, like ScriptRuntime, and just use it -- it may do a lot of what you already need including closing the iterator.

…ect.entries # Conflicts: # src/org/mozilla/javascript/ScriptRuntime.java

rPraml · 2021-05-26T14:14:19Z

src/org/mozilla/javascript/NativeMap.java

-        ScriptableObject dummy = ensureScriptableObject(cx.newObject(scope, map.getClassName()));
-        final Callable set =
-                ScriptRuntime.getPropFunctionAndThis(dummy.getPrototype(), "set", cx, scope);
+        Scriptable proto = ScriptableObject.getClassPrototype(scope, map.getClassName());


These are the only code change in this class.
I have changed this to getClassPrototype instead of creating a dummy object and I reuse the code in
ScriptRuntime.loadFromIterable

rPraml · 2021-05-26T14:15:59Z

src/org/mozilla/javascript/NativeObject.java

-                            int attrs = so.getAttributes((Symbol) arg);
-                            result = ((attrs & ScriptableObject.DONTENUM) == 0);
-                        }
+                        result = result && isEnumerable((Symbol) arg, thisObj);


moved code to "isEnumerable" method

rPraml · 2021-05-26T14:18:36Z

src/org/mozilla/javascript/ScriptRuntime.java

                }
            } else {
                ScriptableObject so = (ScriptableObject) object;
                Callable getterOrSetter = (Callable) value;
                boolean isSetter = getterSetter == 1;
+                // XXX: Do we have to handle Symbol here.
+                // This will be required, when conputedprops are supported.


Yes, we need to check this. See FOCONIS@6d2a459
But I think this should be done in a separate PR

rPraml · 2021-05-26T14:21:29Z

@gbrail Thanks for the hint. Maybe you can also take a look at this PR again.
It will fix some issues, when Symbol is used as key. But there are still some left, which I would do in a separate PR.

gbrail · 2021-06-01T21:39:44Z

Thanks for cleaning this up. This looks good for now...

However, we had a whole bunch of new formatting changes and this requires another merge. Can you please try to merge with the latest master and resolve conflicts? Thanks!

…ect.entries # Conflicts: # src/org/mozilla/javascript/NativeMap.java

rPraml · 2021-06-02T06:31:08Z

@gbrail Conflicts are solved and build is passing again

rbri · 2021-06-02T17:10:59Z

Thanks a lot for all the work on this and all the others

gbrail · 2021-06-02T17:33:16Z

Thanks for working all that out!

ADD: ES6 Object.values / Object.entries / Object.fromEntries

ae236ba

rPraml added 2 commits May 20, 2021 20:03

enabled some tests

6a913ed

spotless

b4817f0

rbri reviewed May 20, 2021

View reviewed changes

tonygermano reviewed May 21, 2021

View reviewed changes

rPraml added 2 commits May 21, 2021 12:14

enabled test262 for entries/vaules

0fed865

spotless

d3c13ea

rPraml added 4 commits May 25, 2021 07:54

Merge remote-tracking branch 'upstream/master' into object.values-obj…

afe2274

…ect.entries # Conflicts: # src/org/mozilla/javascript/NativeObject.java

FIX: fromEntries/evaluation-order.js

2fb4633

FIX: fromEntries/to-property-key.js

5cdf431

FIX: Support for symbols

88d40c6

rPraml commented May 25, 2021

View reviewed changes

rPraml added 3 commits May 26, 2021 09:38

Merge remote-tracking branch 'upstream/master' into object.values-obj…

3b2e52f

…ect.entries # Conflicts: # src/org/mozilla/javascript/ScriptRuntime.java

used common "loadFromIterable" code

265f5ca

spotless NativeMap

8fa46fa

rPraml commented May 26, 2021

View reviewed changes

rPraml added 3 commits June 2, 2021 07:46

Merge remote-tracking branch 'upstream/master' into object.values-obj…

f0e20c9

…ect.entries # Conflicts: # src/org/mozilla/javascript/NativeMap.java

Spotless NativeMap

c915eea

FIX: lastStoredScriptable got lost during merge resolve

37b6fee

gbrail merged commit 37b6fee into mozilla:master Jun 2, 2021

p-bakker added this to the Release 1.7.14 milestone Oct 13, 2021

p-bakker added the feature Issues considered a new feature label Oct 13, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ADD: ES6 Object.values / Object.entries / Object.fromEntries #902

ADD: ES6 Object.values / Object.entries / Object.fromEntries #902

rPraml commented May 20, 2021

tuchida commented May 20, 2021

rPraml commented May 20, 2021

rbri left a comment

tuchida commented May 20, 2021

rbri commented May 20, 2021

gbrail commented May 20, 2021

tonygermano May 21, 2021

rbri commented May 21, 2021

rPraml commented May 21, 2021 •

edited

Loading

gbrail commented May 21, 2021

rbri commented May 21, 2021

rbri commented May 21, 2021

rPraml left a comment

rPraml May 25, 2021

rPraml May 25, 2021

rPraml May 25, 2021

rPraml May 25, 2021

rPraml May 25, 2021

gbrail commented May 25, 2021

rPraml May 26, 2021

rPraml May 26, 2021

rPraml May 26, 2021

rPraml commented May 26, 2021

gbrail commented Jun 1, 2021

rPraml commented Jun 2, 2021

rbri commented Jun 2, 2021

gbrail commented Jun 2, 2021

ADD: ES6 Object.values / Object.entries / Object.fromEntries #902

ADD: ES6 Object.values / Object.entries / Object.fromEntries #902

Conversation

rPraml commented May 20, 2021

tuchida commented May 20, 2021

rPraml commented May 20, 2021

rbri left a comment

Choose a reason for hiding this comment

tuchida commented May 20, 2021

rbri commented May 20, 2021

gbrail commented May 20, 2021

Choose a reason for hiding this comment

rbri commented May 21, 2021

rPraml commented May 21, 2021 • edited Loading

gbrail commented May 21, 2021

rbri commented May 21, 2021

rbri commented May 21, 2021

rPraml left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gbrail commented May 25, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rPraml commented May 26, 2021

gbrail commented Jun 1, 2021

rPraml commented Jun 2, 2021

rbri commented Jun 2, 2021

gbrail commented Jun 2, 2021

rPraml commented May 21, 2021 •

edited

Loading