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.

Sign up for GitHub

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

Jump to bottom

Implement Object.fromEntries #5622

Merged
merged 2 commits into from Sep 6, 2018
Merged

Implement Object.fromEntries #5622

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3d93036
Implement object.fromEntries
rhuanjl Sep 3, 2018
c279dfd
bytecode regen
rhuanjl Sep 3, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 2 additions & 1 deletion lib/Parser/rterrors.h
View file
Open in desktop
Expand Up @@ -303,7 +303,8 @@ RT_ERROR_MSG(JSERR_ObjectIsNotInitialized, 5617, "%s: Object internal state is n

RT_ERROR_MSG(JSERR_GeneratorAlreadyExecuting, 5618, "%s: Cannot execute generator function because it is currently executing", "", kjstTypeError, 0)
RT_ERROR_MSG(JSERR_LengthIsTooBig, 5619, "Length property would exceed maximum value in output from '%s'", "", kjstTypeError, 0)
// 5620-5626 Unused
RT_ERROR_MSG(JSERR_NonObjectFromIterable, 5620, "Iterable provided to %s must not return non-object or null value.", "", kjstTypeError, 0)
// 5621-5626 Unused
RT_ERROR_MSG(JSERR_NeedConstructor, 5627, "'%s' is not a constructor", "Constructor expected", kjstTypeError, 0)

RT_ERROR_MSG(VBSERR_CantDisplayDate, 32812, "", "The specified date is not available in the current locale's calendar", kjstRangeError, 0)
Expand Down
3 changes: 3 additions & 0 deletions lib/Runtime/Base/JnDirectFields.h
View file
Open in desktop
Expand Up @@ -156,6 +156,7 @@ ENTRY(freeze)
ENTRY(from)
ENTRY(fromCharCode)
ENTRY(fromCodePoint)
ENTRY(fromEntries)
ENTRY(function)
ENTRY(Function)
ENTRY(getDate)
Expand Down Expand Up @@ -611,6 +612,7 @@ ENTRY(InitInternalProperties)
ENTRY(methodName)
ENTRY(registerChakraLibraryFunction)
ENTRY(registerFunction)
ENTRY(staticMethod)
ENTRY(toLength)
ENTRY(toInteger)
ENTRY(arraySpeciesCreate)
Expand Down Expand Up @@ -672,6 +674,7 @@ ENTRY(raiseNeedObject)
ENTRY(raiseNeedObjectOfType)
ENTRY(raiseNeedObjectOrString)
ENTRY(raiseNotAConstructor)
ENTRY(raiseNonObjectFromIterable)
ENTRY(raiseObjectIsAlreadyInitialized)
ENTRY(raiseObjectIsNonExtensible)
ENTRY(raiseOptionValueOutOfRange_3)
Expand Down
4 changes: 2 additions & 2 deletions lib/Runtime/ByteCode/ByteCodeCacheReleaseFileVersion.h
View file
Open in desktop
Expand Up @@ -5,6 +5,6 @@
// NOTE: If there is a merge conflict the correct fix is to make a new GUID.
// This file was generated with tools\update_bytecode_version.ps1

// {6CC5964E-CBF2-4053-99DA-987794182D7E}
// {BAC3A947-4873-4DAF-AC60-915116FFE744}
const GUID byteCodeCacheReleaseFileVersion =
{ 0x6CC5964E, 0xCBF2, 0x4053, { 0x99, 0xDA, 0x98, 0x77, 0x94, 0x18, 0x2D, 0x7E } };
{ 0xBAC3A947, 0x4873, 0x4DAF, { 0xAC, 0x60, 0x91, 0x51, 0x16, 0xFF, 0xE7, 0x44 } };
1 change: 1 addition & 0 deletions lib/Runtime/Library/EngineInterfaceObjectBuiltIns.h
View file
Open in desktop
Expand Up @@ -58,6 +58,7 @@ BuiltInRaiseException1(TypeError, This_NullOrUndefined)
BuiltInRaiseException1(TypeError, NotAConstructor)
BuiltInRaiseException1(TypeError, ObjectIsNonExtensible)
BuiltInRaiseException1(TypeError, LengthIsTooBig)
BuiltInRaiseException1(TypeError, NonObjectFromIterable)
BuiltInRaiseException2(TypeError, NeedObjectOfType)
BuiltInRaiseException1(RangeError, InvalidCurrencyCode)
BuiltInRaiseException(TypeError, MissingCurrencyCode)
Expand Down
2,186 changes: 1,093 additions & 1,093 deletions lib/Runtime/Library/InJavascript/Intl.js.bc.32b.h
View file
Open in desktop

Large diffs are not rendered by default.

2,186 changes: 1,093 additions & 1,093 deletions lib/Runtime/Library/InJavascript/Intl.js.bc.64b.h
View file
Open in desktop

Large diffs are not rendered by default.

2,198 changes: 1,099 additions & 1,099 deletions lib/Runtime/Library/InJavascript/Intl.js.nojit.bc.32b.h
View file
Open in desktop

Large diffs are not rendered by default.

2,200 changes: 1,100 additions & 1,100 deletions lib/Runtime/Library/InJavascript/Intl.js.nojit.bc.64b.h
View file
Open in desktop

Large diffs are not rendered by default.

19 changes: 19 additions & 0 deletions lib/Runtime/Library/JavascriptLibrary.cpp
View file
Open in desktop
Expand Up @@ -3651,6 +3651,18 @@ namespace Js
propertyCount++;
}

if (scriptContext->GetConfig()->IsES7ValuesEntriesEnabled())
{
propertyCount += 2;
}

#ifdef ENABLE_JS_BUILTINS
if (scriptContext->IsJsBuiltInEnabled())
{
propertyCount++;
}
#endif

typeHandler->Convert(objectConstructor, mode, propertyCount);

library->AddMember(objectConstructor, PropertyIds::length, TaggedInt::ToVarUnchecked(1), PropertyConfigurable);
Expand Down Expand Up @@ -3707,6 +3719,13 @@ namespace Js
library->AddFunctionToLibraryObject(objectConstructor, PropertyIds::entries, &JavascriptObject::EntryInfo::Entries, 1));
}

#ifdef ENABLE_JS_BUILTINS
if (scriptContext->IsJsBuiltInEnabled())
{
library->EnsureBuiltInEngineIsReady();
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

}
#endif

objectConstructor->SetHasNoEnumerableProperties(true);

return true;
Expand Down
32 changes: 32 additions & 0 deletions lib/Runtime/Library/JsBuiltIn/JsBuiltIn.js
View file
Open in desktop
Expand Up @@ -17,6 +17,7 @@
ArrayFlat: { className: "Array", methodName: "flat", argumentsCount: 0, forceInline: true /*optional*/ },
ArrayFlatMap: { className: "Array", methodName: "flatMap", argumentsCount: 1, forceInline: true /*optional*/ },
ArrayForEach: { className: "Array", methodName: "forEach", argumentsCount: 1, forceInline: true /*optional*/ },
ObjectFromEntries: { className: "Object", staticMethod: true, methodName: "fromEntries", argumentsCount: 1, forceInline: true /*optional*/ },
};

var setPrototype = platform.builtInSetPrototype;
Expand All @@ -39,10 +40,13 @@
__chakraLibrary.ArrayIterator.prototype = CreateObject(iteratorPrototype);
__chakraLibrary.raiseNeedObjectOfType = platform.raiseNeedObjectOfType;
__chakraLibrary.raiseThis_NullOrUndefined = platform.raiseThis_NullOrUndefined;
__chakraLibrary.raiseNeedObject = platform.raiseNeedObject;
__chakraLibrary.raiseNonObjectFromIterable = platform.raiseNonObjectFromIterable;
__chakraLibrary.raiseLengthIsTooBig = platform.raiseLengthIsTooBig;
__chakraLibrary.raiseFunctionArgument_NeedFunction = platform.raiseFunctionArgument_NeedFunction;
__chakraLibrary.callInstanceFunc = platform.builtInCallInstanceFunction;
__chakraLibrary.functionBind = platform.builtInJavascriptFunctionEntryBind;
__chakraLibrary.objectDefineProperty = _objectDefineProperty;

_objectDefineProperty(__chakraLibrary.ArrayIterator.prototype, 'next',
// Object's getter and setter can get overriden on the prototype, in that case while setting the value attributes, we will end up with TypeError
Expand Down Expand Up @@ -440,4 +444,32 @@

return undefined;
});

platform.registerFunction(FunctionsEnum.ObjectFromEntries, function (iterable) {
// #sec-object.fromentries
"use strict";
if (iterable === null || iterable === undefined) {
__chakraLibrary.raiseNeedObject("Object.fromEntries");
}

const o = {};
const propDescriptor = {
enumerable : true,
configurable : true,
writable : true,
value : undefined
};

let key;
for (const entry of iterable) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Copy link

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@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.

if (typeof entry !== "object" || entry === null) {
__chakraLibrary.raiseNonObjectFromIterable("Object.fromEntries");
}

key = entry[0];
propDescriptor.value = entry[1];
__chakraLibrary.objectDefineProperty(o, key, propDescriptor);
}
return o;
});
});