Implement Object.fromEntries #5622

rhuanjl · 2018-08-23T00:46:05Z

This PR implements the stage 3 proposal Object.fromEntries.
Proposal spec text is here: https://tc39.github.io/proposal-object-from-entries/

Notes:

Implementation done as a JsBuiltin - note this requires initialising the JsBuiltins for accessing the Object constructor - could this have negative effects?
This implementation passes the test262 tests for this feature.

fixes: #5590

rhuanjl · 2018-08-23T00:47:03Z

lib/Runtime/Base/JnDirectFields.h

@@ -140,8 +140,6 @@ ENTRY(every)
 ENTRY(exec)
 ENTRY2(false_, _u("false")) // "false" cannot be an identifier in C++ so using "false_" instead
 ENTRY(flags)
-ENTRY(flat)
-ENTRY(flatMap)


Unrelated cleanup - these Entries were not needed (added when I implemented .flat and .flatMap but not actually needed for JsBuiltin methods)

Is this what was causing issues in the legacy win7 and win8 builds?

I don't have a windows7/8 setup to test on but this was my best guess - the assertion was saying a property name was invalid so I guessed that the mechanism it was using relied on this ENTRY macros.

rhuanjl · 2018-08-23T00:47:38Z

lib/Runtime/Library/JsBuiltIn/JsBuiltIn.js

+        let key;
+        for (const entry of iterable) {
+            if (typeof entry !== "object") {
+                __chakraLibrary.raiseNeedObject("Object.prototype.fromEntries");


This isn't a great error message but not sure what I should use.

I think this isn't required; if I understand the spec correctly, then this part of the implementation is an inlined version of CreateDataPropertyOnObject, and the this of that function is the o here which is guaranteed to be an object

Oh, I think I'm mistaken; this is corresponding to the internals of AddEntriesFromIterable, which does require that the iterable returns objects. Nevermind then!

s/Object.prototype.fromEntries/Object.fromEntries/

MSLaguana · 2018-08-23T02:01:34Z

lib/Runtime/Library/JsBuiltIn/JsBuiltIn.js

+        // #sec-object.fromentries
+        "use strict";
+        if (this === null || this === undefined) {
+            __chakraLibrary.raiseNeedObject("Object.prototype.fromEntries");


Is this required in the spec? I don't see language in the spec referencing the this of the function

MSLaguana · 2018-08-23T02:17:57Z

Apart from the failing tests which look like they just need baseline updates to account for this new method, this change looks good to me!

MSLaguana · 2018-08-23T02:20:31Z

Actually some of the tests are failing with crashes, an assertion to do with property IDs not being nullptr. EDIT: Initially I thought that it was to do with builtins not being present, but that's not the case. The lite build is running fine, but the win7 legacy build is failing with a crash.

MSLaguana

Thanks @rhuanjl! I'll run this past our internal CI and see if anything falls out.

MSLaguana · 2018-08-23T16:27:01Z

Looks like no significant issues on our internal CI. I need to update a few more baselines, but that's not a big problem. Could you please squash your fixup commit in, and once I've fixed the internal baselines I'll get this merged in. Thanks again!

rhuanjl · 2018-08-23T16:35:14Z

Thanks for testing this @MSLaguana I've squashed the fixes commit.

jackhorton · 2018-08-23T17:04:11Z

lib/Runtime/Library/JsBuiltInEngineInterfaceExtensionObject.cpp

@@ -352,7 +375,7 @@ namespace Js
        func->GetFunctionProxy()->SetIsPublicLibraryCode();
        func->GetFunctionProxy()->EnsureDeserialized()->SetDisplayName(methodName->GetString(), methodName->GetLength(), 0);

-        DynamicObject* prototype = GetPrototypeFromName(JavascriptOperators::GetPropertyId(className, scriptContext), scriptContext);
+        DynamicObject* prototype = GetPrototypeFromName(JavascriptOperators::GetPropertyId(className, scriptContext), staticMethod, scriptContext);


small nit: we have been trying to prefer bool to BOOL, and we turned off the warning about type mismatch for that conversion so it should always be implicit.

jackhorton · 2018-08-23T17:06:13Z

lib/Runtime/Library/JsBuiltInEngineInterfaceExtensionObject.cpp

+                return library->stringConstructor;
+
+            default:
+                AssertMsg(false, "Unable to find a constructor that match with this className.");


I would AssertOrFailFast for any cost we never expect to run, since the extra if check should never actually affect runtime performance.

I was copying code below that just did an Assert for this - relevant point though - should only come up when developing a new builtin and a silent fail isn't great.

jackhorton · 2018-08-23T17:07:31Z

lib/Runtime/Library/JsBuiltInEngineInterfaceExtensionObject.cpp

@@ -344,6 +366,7 @@ namespace Js
        JavascriptString* methodName = JavascriptString::FromVar(methodNameProperty);
        int argumentsCount = TaggedInt::ToInt32(argumentsCountProperty);

+        BOOL staticMethod = JavascriptConversion::ToBoolean(staticMethodProperty, scriptContext);
        BOOL forceInline = JavascriptConversion::ToBoolean(forceInlineProperty, scriptContext);


May want to add Assert(Is()) for staticMethod and forceInline above (forceInline isn't your change but the remaining properties all have asserts). aliasProperty also doesn't have an assert, but I can't see where its used?

I intended it as an optional property - if it's undefined ToBoolean will give false which is intended so an Assert here would be wrong. aliasProperty is used for the array iterator only (hence also optional) so again should not be asserted.

jackhorton · 2018-08-23T17:09:01Z

test/Object/fromEntries.js

+            assert.doesNotThrow(()=>{Object.fromEntries(String());}, "Object.fromEntries does not throw when called with String Object parameter with length 0");
+            assert.throws(()=>{Object.fromEntries(String("anything"));}, TypeError, "Object.fromEntries throws when called with String Object parameter with length > 0");
+            assert.throws(()=>{Object.fromEntries({});}, TypeError, "Object.fromEntries throws when called with Object literal");
+            assert.throws(()=>{Object.fromEntries({a : "5", b : "10"});}, TypeError, "Object.fromEntries throws when called with Object literal");


Proxy/array-like object/new Array()/getter cases would be nice.

jackhorton · 2018-08-23T17:11:20Z

lib/Runtime/Library/JsBuiltIn/JsBuiltIn.js

+        };
+
+        let key;
+        for (const entry of iterable) {


This can be tainted by overwriting Array.prototype[Symbol.iterator], I believe. Thats why I never used it in Intl, anyways. We should probably check, or use a regular for loop to be safe?

Per the proposed spec this method is specifically meant to use the iterator method of the provided iterable - i.e. if using an array and Array.prototype[Symbol.iterator] is overwritten it's meant to behave differently.

Oh, weird. All of the Intl things that iterate through arrays call out for (var x in obj) { if (HasOwnProperty(obj, x) { … } } or something similar. May want to add a test for tainting the iterator to make sure it is tainted, then, rather than that it isn't.

It's because the method is designed to be usable with other iterables, not just arrays e.g. generator functions etc.

Yeah, see DETAILS.md in the proposal.

This is to match the very similar behavior of the Map constructor (which is in fact defined using the same abstract operation).

May want to add a test for tainting the iterator to make sure it is tainted, then, rather than that it isn't.

There's one in test262! Not sure if you're using that or would consider it sufficient, just thought I'd mention.

@bakkot I ran this against test262 before submitting the PR but the Chakracore CI doesn't run test262 so I've added alternate tests for this - check the diff of this PR for tests.

rhuanjl · 2018-08-23T20:00:40Z

@jackhorton I think I've addressed your points. (BOOL -> bool, the unreachable Assert -> AssertOrFailfast, extra test cases) Let me know if good to squash.

MSLaguana · 2018-08-23T20:58:21Z

lib/Runtime/Library/JavascriptLibrary.cpp

+#ifdef ENABLE_JS_BUILTINS
+        if (scriptContext->IsJsBuiltInEnabled())
+        {
+            library->EnsureBuiltInEngineIsReady();


Turns out this bit here is causing some problems in our internal tests that were initially obscured by the baseline changes.

@jackhorton how do we deal with the Intl object? We do lazy initialization there right? Perhaps we need to do the same here?

The specific issue is that when calling into script from here, there is a check that we can handle stack overflow, and that is failing in some cases.

Oh right, this is the lazy initialization of the Object object, so this is already a similar pattern to what we do for intl.

I simply copied what was done for the array prototype - is there some difference between doing it for a prototype vs a constructor.

I believe that the difference is in the calling code, not here. I believe I'll need to add some additional error handling elsewhere since that code path also doesn't seem to handle calling into a JS builtin at all, despite sounding like it should.

A quick look shows that the EnsureBuiltInEngineIsReady doesn't have try/catch whereas EnsureIntlObjectReady does - what's confusing to me is that the EnsureBuiltInEngineIsReady method was previously used for ArrayPrototype without any problems.

MSLaguana · 2018-08-23T23:04:05Z

Had to update the bytecode GUID since there were changes (essentially to invalidate any cached bytecode that may exist) but otherwise looks good. Once this CI pass completes I'll check it in. Thanks!

boingoing

LGTM

bakkot · 2018-08-23T23:35:54Z

lib/Runtime/Library/JsBuiltIn/JsBuiltIn.js

+        // #sec-object.fromentries
+        "use strict";
+        if (iterable === null || iterable === undefined) {
+            __chakraLibrary.raiseNeedObject("Object.prototype.fromEntries");


s/Object.prototype.fromEntries/Object.fromEntries/

sethbrenith

ljharb · 2018-08-24T15:40:35Z

test/Object/fromEntries.js

+
+function verifyObject(expected, actual)
+{
+    for (let i in expected)


should this include a has own property check on expected, too?

Erm - yeah this was a mistake it was meant to do hasProperty checks on expected not on actual (as the verifyProperty bit checks the actuals; it's exactly backwards, will fix.

MSLaguana · 2018-08-24T15:51:56Z

@rhuanjl would you mind squashing the error message fixup (and if you feel like it, the GUID update) into your original commit, and also regenerating the bytecode to account for that change again?

rhuanjl · 2018-08-24T17:03:34Z

@MSLaguana I've rebased to master, fixed the merge conflicts (vs the recent addition of Array.prototype.forEach as a JsBuiltin) regenerated the bytecode and squashed.

As there was a merge conflict does it need another new GUID? Per the comment in ByteCodeCacheReleaseFileVersion.h ?

MSLaguana · 2018-08-24T17:06:43Z

Was the merge conflict in the ByteCodeCacheReleaseFileVersion.h file itself? If so, then we will need to regenerate, but otherwise it shouldn't be required.

rhuanjl · 2018-08-24T17:08:22Z

No - there was a merge conflict vs the generated bytecode files and JsBuiltins.js but that PR did not update the GUID so no conflict on ByteCodeCacheReleaseFileVersion.h

jackhorton · 2018-08-24T17:10:11Z

In general, as long as a change to bytecode (such as this) gets merged with any new GUID, its fine. So if you had a GUID change and then rebased ontop of another GUID change, there would be a conflict, but you could either take your GUID change or generate another new one, since either are different from your parent. If that makes sense.

This whole bytecode situation sucks.

rhuanjl · 2018-08-24T17:59:00Z

@dotnet-bot test Ubuntu shared_ubuntu_linux_debug please

bakkot · 2018-08-24T18:13:18Z

lib/Runtime/Library/JsBuiltIn/JsBuiltIn.js

+
+        let key;
+        for (const entry of iterable) {
+            if (typeof entry !== "object") {


Hm. You may want to add || entry === null to this check, just so you get a better error message. It's the right semantics either way - if entry is null, it will throw a TypeError at the key = entry[0] line - but it may be surprising that the messages differ when entry is null vs any other primitive, especially since those are forbidden by the same part of the spec.

Currently the error message for failing on the key = entry[0] line is better than the error message for failing this check, so would rather not update unless devising a new specific error message.

Note CC will report that the error occurs in the line of user code containing the fromEntries call - the code in JsBuiltIn.js is hidden from the end user of CC.

The raiseNeedObject error simply gives "Object expected" as the error message whereas key = entry[0] line will give the slightly more helpful "Unable to get property '0' of undefined or null reference".

If combining them into one error it should probably be something specific like "Iterable provided to Object.fromEntries must not return non-object or null value"

For what its worth, it is possible to add a new error :) In general our error messages havent been something we have worried about too much, but if it bothers you, feel free to hook a new error up. Plenty of specific cases have their own unique error text.

rhuanjl · 2018-08-24T19:24:07Z

Stumped by this test failure (same test has failed twice now) - it's an ASM.js test that's meant to test for an out of memory error - it obviously does not use Object.fromEntries, so unsure how it's being affected, also it didn't error with earlier versions of this PR.

jackhorton · 2018-08-24T19:52:06Z

@MikeHolman any ideas about the AsmJS failure?

MikeHolman · 2018-08-24T21:47:15Z

Weird, looks like the output is correct but exit code is bad. Any idea what is exit code -11? I think the only place your code interacts would be with the new library->EnsureBuiltInEngineIsReady(); call in InitializeObjectConstructor, so maybe some side effect of that is causing us to OOM earlier than expected or some other side effect like that.

rhuanjl · 2018-08-24T23:13:13Z

@MikeHolman I did a bit of digging - this error is debug only at least... signal 11 = segmentation fault.

The segfault occurs in the loop here:
https://github.com/Microsoft/ChakraCore/blob/45a62718a6993b2edaca5d7aaae115c28d64b507/lib/Common/Memory/RecyclerWriteBarrierManager.cpp#L507-L519

I'm afraid I know next to nothing about how CC's recycler works so I don't think I'll be able to fix this.

MikeHolman · 2018-08-24T23:31:23Z

Seems like an existing bug anyway, I guess we are writing to the card table for a non-GC object. IMO it's fine to disable the test and open an issue.

rhuanjl · 2018-08-25T18:05:13Z

I've added some extra updates - see separate commit:

"fixed" the failing test by not calling WScript.quit in it - this way it's still testing what it was meant to but isn't crashing, there's still a problem here but it's a somewhat obscure xplat debug only problem that possibly only triggers with fatal errors for out of memory disabled, opened an issue to track Modified test: Xplat Debug only Recycler crash on exit #5636
Added the new error message discussed with @jackhorton and @bakkot
Updated the logic for the number of members in the ObjectConstructor initialiser - noted it was wrong:
- made it only add 1 for .fromEntries if builtIns are enabled
- made it add 2 for ES7 values and entries this seemed to be a historic error meaning that currently the ObjectConstructor always had to change type during initialisation.

Hopefully this is good for merge though - let me know if good to squash (kept the new changes in a separate commit so easier to review)

MSLaguana · 2018-08-27T17:25:13Z

Bumping bytecode GUID again since some changes needed updates to private tests again. Feel free to squash it together when things are ready.

I believe that @jackhorton is working on fixing some internal test baselines which were broken by the Array.prototype.forEach change, and once that's settled we can land the other bytecode-impacting PRs in some sequence.

rhuanjl · 2018-09-01T16:36:42Z

I've assumed I should hold on updating this until #5651 is merged to avoid extra merge conflicts - as far as I'm concerned this is done apart from resolving the merge conflict; please let me know if not.

dilijev · 2018-09-01T18:09:01Z

@rhuanjl I believe @jackhorton was saying he would yield to your PR since he can update and merge his own PR, and thus has less latency than your PR does.

jackhorton · 2018-09-04T16:28:18Z

Looks like you have already regen'd bytecode, so I will try to merge yours first today and then get mine in after.

jackhorton · 2018-09-04T16:28:36Z

@dotnet-bot test all please

MSLaguana · 2018-09-04T16:52:26Z

@dotnet-bot test this please

MSLaguana · 2018-09-04T18:03:16Z

Looks like the webhooks to talk with Jenkins is timing out.

mmitche · 2018-09-04T18:06:40Z

@dotnet-bot test this please

mmitche · 2018-09-04T18:07:08Z

@MSLaguana There's an issue open for this. I'm investigating https://github.com/dotnet/core-eng/issues/4149

jackhorton · 2018-09-05T04:05:44Z

@rhuanjl update on this, our internal tests are down until https://azure.microsoft.com/en-us/status/ is happy again :/ it tests clean locally, but we have already missed a bunch of builds due to this outage, and I don't want to compound any issues by merging this before things are green.

Merge pull request #5622 from rhuanjl:ObjectfromEntries This PR implements the stage 3 proposal Object.fromEntries. Proposal spec text is here: https://tc39.github.io/proposal-object-from-entries/ Notes: 1. Implementation done as a JsBuiltin - note this requires initialising the JsBuiltins for accessing the Object constructor - could this have negative effects? 2. This implementation passes the test262 tests for this feature. fixes: #5590

rhuanjl commented Aug 23, 2018

View reviewed changes

MSLaguana reviewed Aug 23, 2018

View reviewed changes

MSLaguana self-assigned this Aug 23, 2018

MSLaguana approved these changes Aug 23, 2018

View reviewed changes

rhuanjl force-pushed the ObjectfromEntries branch from 864729d to 844c679 Compare August 23, 2018 16:32

jackhorton suggested changes Aug 23, 2018

View reviewed changes

jackhorton approved these changes Aug 23, 2018

View reviewed changes

rhuanjl force-pushed the ObjectfromEntries branch from 9d533fc to 4f03f21 Compare August 23, 2018 20:26

MSLaguana reviewed Aug 23, 2018

View reviewed changes

boingoing approved these changes Aug 23, 2018

View reviewed changes

bakkot reviewed Aug 23, 2018

View reviewed changes

ljharb mentioned this pull request Aug 23, 2018

Stage 4 tracking tc39/proposal-object-from-entries#21

Closed

14 tasks

sethbrenith approved these changes Aug 24, 2018

View reviewed changes

ljharb approved these changes Aug 24, 2018

View reviewed changes

rhuanjl force-pushed the ObjectfromEntries branch from 2086a7e to c4a03d8 Compare August 24, 2018 16:58

bakkot reviewed Aug 24, 2018

View reviewed changes

dilijev added External Contributor Bytecode-Update This PR updates bytecode and will cause merge conflicts with other PRs with this label labels Aug 25, 2018

rhuanjl force-pushed the ObjectfromEntries branch from c4a03d8 to f983cd5 Compare August 25, 2018 17:15

rhuanjl mentioned this pull request Aug 25, 2018

Modified test: Xplat Debug only Recycler crash on exit #5636

Open

rhuanjl added 2 commits September 3, 2018 20:30

Implement object.fromEntries

3d93036

bytecode regen

c279dfd

rhuanjl force-pushed the ObjectfromEntries branch from cea2faf to c279dfd Compare September 3, 2018 19:53

rhuanjl closed this Sep 4, 2018

rhuanjl reopened this Sep 4, 2018

chakrabot merged commit c279dfd into chakra-core:master Sep 6, 2018

rhuanjl deleted the ObjectfromEntries branch September 6, 2018 07:41

Implement Object.fromEntries #5622

Implement Object.fromEntries #5622

Conversation

rhuanjl commented Aug 23, 2018

rhuanjl Aug 23, 2018 • edited

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MSLaguana commented Aug 23, 2018

MSLaguana commented Aug 23, 2018 • edited

MSLaguana left a comment

Choose a reason for hiding this comment

MSLaguana commented Aug 23, 2018

rhuanjl commented Aug 23, 2018

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

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

rhuanjl commented Aug 23, 2018

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

MSLaguana commented Aug 23, 2018

boingoing left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sethbrenith left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MSLaguana commented Aug 24, 2018

rhuanjl commented Aug 24, 2018

MSLaguana commented Aug 24, 2018

rhuanjl commented Aug 24, 2018

jackhorton commented Aug 24, 2018

rhuanjl commented Aug 24, 2018

Choose a reason for hiding this comment

rhuanjl Aug 24, 2018 • edited

Choose a reason for hiding this comment

jackhorton Aug 25, 2018 • edited

Choose a reason for hiding this comment

rhuanjl commented Aug 24, 2018

jackhorton commented Aug 24, 2018

MikeHolman commented Aug 24, 2018 • edited

rhuanjl commented Aug 24, 2018 • edited

MikeHolman commented Aug 24, 2018

rhuanjl commented Aug 25, 2018 • edited

MSLaguana commented Aug 27, 2018

rhuanjl commented Sep 1, 2018

dilijev commented Sep 1, 2018

jackhorton commented Sep 4, 2018

jackhorton commented Sep 4, 2018

MSLaguana commented Sep 4, 2018

MSLaguana commented Sep 4, 2018

mmitche commented Sep 4, 2018

mmitche commented Sep 4, 2018

jackhorton commented Sep 5, 2018

rhuanjl Aug 23, 2018 •

edited

MSLaguana commented Aug 23, 2018 •

edited

rhuanjl Aug 24, 2018 •

edited

jackhorton Aug 25, 2018 •

edited

MikeHolman commented Aug 24, 2018 •

edited

rhuanjl commented Aug 24, 2018 •

edited

rhuanjl commented Aug 25, 2018 •

edited