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

Add JSRT support for native callbacks with new.target #4529

Merged
merged 2 commits into from
Jan 11, 2018
Merged

Add JSRT support for native callbacks with new.target #4529

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
bfbfbaa
Add JSRT support for native callbacks with `new.target`
boingoing Jan 6, 2018
ac00296
Respond to review feedback
boingoing Jan 11, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions bin/ChakraCore/ChakraCore.def
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,3 +58,5 @@ JsReleaseSharedArrayBufferContentHandle

JsLessThan
JsLessThanOrEqual

JsCreateEnhancedFunction
211 changes: 211 additions & 0 deletions bin/NativeTests/JsRTApiTest.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -660,6 +660,217 @@ namespace JsRTApiTest
JsRTApiTest::RunWithAttributes(JsRTApiTest::ExternalFunctionTest);
}

JsValueRef CALLBACK ExternalEnhancedFunctionTestCallback(JsValueRef callee, JsValueRef *arguments, unsigned short argumentCount, JsNativeFunctionInfo *info, void *callbackData)
{
REQUIRE(callbackData != nullptr);
REQUIRE(*static_cast<int*>(callbackData) == 123);
REQUIRE(argumentCount == 2);

bool success = false;
JsValueRef _true;
REQUIRE(JsGetTrueValue(&_true) == JsNoError);
JsValueRef _false;
REQUIRE(JsGetFalseValue(&_false) == JsNoError);


REQUIRE(JsStrictEquals(_true, arguments[0], &success) == JsNoError);
REQUIRE(success);
REQUIRE(JsStrictEquals(_false, arguments[1], &success) == JsNoError);
REQUIRE(success);

REQUIRE(!info->isConstructCall);
REQUIRE(info->thisArg == arguments[0]);

JsValueRef undefined;
REQUIRE(JsGetUndefinedValue(&undefined) == JsNoError);
REQUIRE(JsStrictEquals(undefined, info->newTargetArg, &success) == JsNoError);
REQUIRE(success);

JsValueRef _null;
REQUIRE(JsGetNullValue(&_null) == JsNoError);
return _null;
}

JsValueRef CALLBACK ExternalEnhancedConstructorFunctionTestCallback(JsValueRef callee, JsValueRef *arguments, unsigned short argumentCount, JsNativeFunctionInfo *info, void *callbackData)
{
REQUIRE(callbackData != nullptr);
REQUIRE(*static_cast<int*>(callbackData) == 456);
REQUIRE(argumentCount == 3);

bool success = false;
JsValueRef _true;
REQUIRE(JsGetTrueValue(&_true) == JsNoError);
JsValueRef _false;
REQUIRE(JsGetFalseValue(&_false) == JsNoError);
JsValueRef _null;
REQUIRE(JsGetNullValue(&_null) == JsNoError);

REQUIRE(info->thisArg == arguments[0]);
REQUIRE(JsStrictEquals(_true, arguments[1], &success) == JsNoError);
REQUIRE(success);
REQUIRE(JsStrictEquals(_false, arguments[2], &success) == JsNoError);
REQUIRE(success);

REQUIRE(info->isConstructCall);

JsValueType t;
REQUIRE(JsGetValueType(info->newTargetArg, &t) == JsNoError);
REQUIRE(t == JsFunction);
REQUIRE(JsGetValueType(info->thisArg, &t) == JsNoError);
REQUIRE(t == JsObject);

return info->thisArg;
}

void ExternalEnhancedFunctionTest(JsRuntimeAttributes attributes, JsRuntimeHandle runtime)
{
int sentinel = 123;
JsValueRef function = JS_INVALID_REFERENCE;
REQUIRE(JsCreateEnhancedFunction(ExternalEnhancedFunctionTestCallback, nullptr, &sentinel, &function) == JsNoError);
JsValueRef _true;
REQUIRE(JsGetTrueValue(&_true) == JsNoError);
JsValueRef _false;
REQUIRE(JsGetFalseValue(&_false) == JsNoError);
JsValueRef args[2] = { _true, _false };
JsValueRef _null;
REQUIRE(JsGetNullValue(&_null) == JsNoError);
JsValueRef result;
REQUIRE(JsCallFunction(function, args, 2, &result) == JsNoError);
bool success;
REQUIRE(JsStrictEquals(_null, result, &success) == JsNoError);
REQUIRE(success);

sentinel = 456;
function = JS_INVALID_REFERENCE;
REQUIRE(JsCreateEnhancedFunction(ExternalEnhancedConstructorFunctionTestCallback, nullptr, &sentinel, &function) == JsNoError);
JsValueRef ctorArgs[3] = { _null, _true, _false };
REQUIRE(JsConstructObject(function, ctorArgs, 3, &result) == JsNoError);
JsValueType t;
REQUIRE(JsGetValueType(result, &t) == JsNoError);
REQUIRE(t == JsObject);
}

TEST_CASE("ApiTest_ExternalEnhancedFunctionTest", "[ApiTest]")
{
JsRTApiTest::RunWithAttributes(JsRTApiTest::ExternalEnhancedFunctionTest);
}

struct ExternalEnhancedBaseClassFunctionTestInfo
{
JsValueRef derived;
JsValueRef base;
};

JsValueRef CALLBACK ExternalEnhancedBaseClassFunctionTestCallback(JsValueRef callee, JsValueRef *arguments, unsigned short argumentCount, JsNativeFunctionInfo *info, void *callbackData)
{
REQUIRE(callbackData != nullptr);

ExternalEnhancedBaseClassFunctionTestInfo* testinfo = (ExternalEnhancedBaseClassFunctionTestInfo*)callbackData;
JsValueType t;
REQUIRE(JsGetValueType(testinfo->derived, &t) == JsNoError);
REQUIRE(t == JsFunction);
REQUIRE(JsGetValueType(testinfo->base, &t) == JsNoError);
REQUIRE(t == JsFunction);
REQUIRE(argumentCount == 2);

JsPropertyIdRef propId;
bool success = false;
JsValueRef _true;
REQUIRE(JsGetTrueValue(&_true) == JsNoError);
JsValueRef _false;
REQUIRE(JsGetFalseValue(&_false) == JsNoError);

REQUIRE(info->thisArg == arguments[0]);
REQUIRE(JsStrictEquals(_true, arguments[1], &success) == JsNoError);
REQUIRE(success);

REQUIRE(info->isConstructCall);
REQUIRE(JsGetValueType(info->newTargetArg, &t) == JsNoError);
REQUIRE(t == JsFunction);
REQUIRE(JsGetValueType(info->thisArg, &t) == JsNoError);
REQUIRE(t == JsObject);

// new.target === Derived
REQUIRE(JsStrictEquals(info->newTargetArg, testinfo->derived, &success) == JsNoError);
REQUIRE(success);

// this.constructor === Derived
REQUIRE(JsGetPropertyIdFromName(_u("constructor"), &propId) == JsNoError);
JsValueRef thisCtor = JS_INVALID_REFERENCE;
REQUIRE(JsGetProperty(info->thisArg, propId, &thisCtor) == JsNoError);
REQUIRE(JsStrictEquals(thisCtor, testinfo->derived, &success) == JsNoError);
REQUIRE(success);

// this.__proto__ === Derived.prototype
JsValueRef thisProto = JS_INVALID_REFERENCE;
REQUIRE(JsGetPrototype(info->thisArg, &thisProto) == JsNoError);
JsValueRef derivedPrototype = JS_INVALID_REFERENCE;
REQUIRE(JsGetPropertyIdFromName(_u("prototype"), &propId) == JsNoError);
REQUIRE(JsGetProperty(testinfo->derived, propId, &derivedPrototype) == JsNoError);
REQUIRE(JsStrictEquals(thisProto, derivedPrototype, &success) == JsNoError);
REQUIRE(success);

return info->thisArg;
}

void ExternalEnhancedBaseClassFunctionTest(JsRuntimeAttributes attributes, JsRuntimeHandle runtime)
{
ExternalEnhancedBaseClassFunctionTestInfo info = { nullptr, nullptr };
JsValueRef name = JS_INVALID_REFERENCE;
REQUIRE(JsCreateString("BaseClass", 10, &name) == JsNoError);
JsValueRef base = JS_INVALID_REFERENCE;
REQUIRE(JsCreateEnhancedFunction(ExternalEnhancedBaseClassFunctionTestCallback, name, &info, &base) == JsNoError);
info.base = base;

JsValueRef global = JS_INVALID_REFERENCE;
REQUIRE(JsGetGlobalObject(&global) == JsNoError);
JsPropertyIdRef propId;
REQUIRE(JsGetPropertyIdFromName(_u("BaseClass"), &propId) == JsNoError);
REQUIRE(JsSetProperty(global, propId, base, false) == JsNoError);

bool success = false;
JsValueType t;
JsValueRef derived = JS_INVALID_REFERENCE;
REQUIRE(JsRunScript(
_u("class Derived extends BaseClass {") \
_u(" constructor() {") \
_u(" super(true);") \
_u(" }") \
_u("};"), JS_SOURCE_CONTEXT_NONE, _u(""), &derived) == JsNoError);

info.derived = derived;
REQUIRE(JsGetValueType(derived, &t) == JsNoError);
REQUIRE(t == JsFunction);

JsValueRef instance = JS_INVALID_REFERENCE;
REQUIRE(JsRunScript(
_u("new Derived();"), JS_SOURCE_CONTEXT_NONE, _u(""), &instance) == JsNoError);

REQUIRE(JsGetValueType(instance, &t) == JsNoError);
REQUIRE(t == JsObject);

// instance.constructor === Derived
REQUIRE(JsGetPropertyIdFromName(_u("constructor"), &propId) == JsNoError);
JsValueRef instanceCtor = JS_INVALID_REFERENCE;
REQUIRE(JsGetProperty(instance, propId, &instanceCtor) == JsNoError);
REQUIRE(JsStrictEquals(instanceCtor, derived, &success) == JsNoError);
REQUIRE(success);

// instance.__proto__ === Derived.prototype
JsValueRef instanceProto = JS_INVALID_REFERENCE;
REQUIRE(JsGetPrototype(instance, &instanceProto) == JsNoError);
JsValueRef derivedPrototype = JS_INVALID_REFERENCE;
REQUIRE(JsGetPropertyIdFromName(_u("prototype"), &propId) == JsNoError);
REQUIRE(JsGetProperty(derived, propId, &derivedPrototype) == JsNoError);
REQUIRE(JsStrictEquals(instanceProto, derivedPrototype, &success) == JsNoError);
REQUIRE(success);
}

TEST_CASE("ApiTest_ExternalEnhancedBaseClassFunctionTest", "[ApiTest]")
{
JsRTApiTest::RunWithAttributes(JsRTApiTest::ExternalEnhancedBaseClassFunctionTest);
}

void ExternalFunctionNameTest(JsRuntimeAttributes attributes, JsRuntimeHandle runtime)
{
auto testConstructorName = [=](JsValueRef function, PCWCHAR expectedName, size_t expectedNameLength)
Expand Down
5 changes: 4 additions & 1 deletion bin/ch/ChakraRtInterface.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,14 +8,15 @@ struct JsAPIHooks
{
typedef JsErrorCode (WINAPI *JsrtCreateRuntimePtr)(JsRuntimeAttributes attributes, JsThreadServiceCallback threadService, JsRuntimeHandle *runtime);
typedef JsErrorCode (WINAPI *JsrtCreateContextPtr)(JsRuntimeHandle runtime, JsContextRef *newContext);
typedef JsErrorCode(WINAPI *JsrtSetObjectBeforeCollectCallbackPtr)(JsRef ref, void* callbackState, JsObjectBeforeCollectCallback objectBeforeCollectCallback);
typedef JsErrorCode (WINAPI *JsrtSetObjectBeforeCollectCallbackPtr)(JsRef ref, void* callbackState, JsObjectBeforeCollectCallback objectBeforeCollectCallback);
typedef JsErrorCode (WINAPI *JsrtSetRuntimeMemoryLimitPtr)(JsRuntimeHandle runtime, size_t memoryLimit);
typedef JsErrorCode (WINAPI *JsrtSetCurrentContextPtr)(JsContextRef context);
typedef JsErrorCode (WINAPI *JsrtGetCurrentContextPtr)(JsContextRef* context);
typedef JsErrorCode (WINAPI *JsrtDisposeRuntimePtr)(JsRuntimeHandle runtime);
typedef JsErrorCode (WINAPI *JsrtCreateObjectPtr)(JsValueRef *object);
typedef JsErrorCode (WINAPI *JsrtCreateExternalObjectPtr)(void* data, JsFinalizeCallback callback, JsValueRef *object);
typedef JsErrorCode (WINAPI *JsrtCreateFunctionPtr)(JsNativeFunction nativeFunction, void *callbackState, JsValueRef *function);
typedef JsErrorCode (WINAPI *JsrtCreateEnhancedFunctionPtr)(JsEnhancedNativeFunction nativeFunction, JsValueRef metadata, void *callbackState, JsValueRef *function);
typedef JsErrorCode (WINAPI *JsCreateNamedFunctionPtr)(JsValueRef name, JsNativeFunction nativeFunction, void *callbackState, JsValueRef *function);
typedef JsErrorCode (WINAPI *JsrtSetPropertyPtr)(JsValueRef object, JsPropertyIdRef property, JsValueRef value, bool useStrictRules);
typedef JsErrorCode (WINAPI *JsrtGetGlobalObjectPtr)(JsValueRef *globalObject);
Expand Down Expand Up @@ -110,6 +111,7 @@ struct JsAPIHooks
JsrtCreateObjectPtr pfJsrtCreateObject;
JsrtCreateExternalObjectPtr pfJsrtCreateExternalObject;
JsrtCreateFunctionPtr pfJsrtCreateFunction;
JsrtCreateEnhancedFunctionPtr pfJsrtCreateEnhancedFunction;
JsCreateNamedFunctionPtr pfJsrtCreateNamedFunction;
JsrtSetPropertyPtr pfJsrtSetProperty;
JsrtGetGlobalObjectPtr pfJsrtGetGlobalObject;
Expand Down Expand Up @@ -318,6 +320,7 @@ class ChakraRTInterface
static JsErrorCode WINAPI JsCreateObject(JsValueRef *object) { return HOOK_JS_API(CreateObject(object)); }
static JsErrorCode WINAPI JsCreateExternalObject(void *data, JsFinalizeCallback callback, JsValueRef *object) { return HOOK_JS_API(CreateExternalObject(data, callback, object)); }
static JsErrorCode WINAPI JsCreateFunction(JsNativeFunction nativeFunction, void *callbackState, JsValueRef *function) { return HOOK_JS_API(CreateFunction(nativeFunction, callbackState, function)); }
static JsErrorCode WINAPI JsCreateEnhancedFunction(JsEnhancedNativeFunction nativeFunction, JsValueRef metadata, void *callbackState, JsValueRef *function) { return HOOK_JS_API(CreateEnhancedFunction(nativeFunction, metadata, callbackState, function)); }
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know if this is a particularly productive bikeshed, but how set in stone are we on this name? I feel like we could instead call these ES6Functions rather than EnhancedFunctions? I fear of a situation like https://msdn.microsoft.com/en-us/library/dd317812(v=vs.85).aspx, where we might need to change this API again in the future and we are stuck with CreateFunction, CreateEnhancedFunction, and... CreateExtraEnhancedFunction? ES6Function might be confused with arrow functions, so maybe this doesn't have a good solution.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree with the sentiment. We can not change JsCreateFunction or JsNativeFunction easily, though. I considered just naming the new one JsCreateFunction2 to avoid giving it a name and just acknowledge that it's newer. Don't think ES6Function means much, though. I had named it JsNativeFunctionWithInfo but the function JsCreateFunctionWithInfo sounds like you need to pass info in order to create the function. JsNativeFunctionWithNewTarget is too specific. Maybe JsNativeSubclassableFunction or JsSubclassableNativeFunction but that may discourage use for ordinary function callbacks?

Copy link
Contributor

Choose a reason for hiding this comment

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

My thinking for JsCreateES6Function was that the important difference here compared to JsCreateFunction was that you gained access to an ES6 feature, new.target. If the eventual goal is to get this into Windows, is there any value in actually following the naming scheme with JsCreateFunctionEx? The only concern is that a new spec will add a new feature and we will be in the same bad situation right now, except that we will have a new naming ambiguity.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As long as it hasn't shipped in Windows, we're treating these API as experimental. So I suppose we can change the name until that point. Not that we should change it many times or anything, but the option is open.

static JsErrorCode WINAPI JsCreateNamedFunction(JsValueRef name, JsNativeFunction nativeFunction, void *callbackState, JsValueRef *function) { return HOOK_JS_API(CreateNamedFunction(name, nativeFunction, callbackState, function)); }
static JsErrorCode WINAPI JsSetProperty(JsValueRef object, JsPropertyIdRef property, JsValueRef value, bool useStrictRules) { return HOOK_JS_API(SetProperty(object, property, value, useStrictRules)); }
static JsErrorCode WINAPI JsGetGlobalObject(JsValueRef *globalObject) { return HOOK_JS_API(GetGlobalObject(globalObject)); }
Expand Down
47 changes: 47 additions & 0 deletions lib/Jsrt/ChakraCore.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,53 @@ typedef JsErrorCode(CHAKRA_CALLBACK * FetchImportedModuleFromScriptCallBack)(_In
/// </returns>
typedef JsErrorCode(CHAKRA_CALLBACK * NotifyModuleReadyCallback)(_In_opt_ JsModuleRecord referencingModule, _In_opt_ JsValueRef exceptionVar);

/// <summary>
/// A structure containing information about a native function callback.
/// </summary>
typedef struct JsNativeFunctionInfo
{
JsValueRef thisArg;
JsValueRef newTargetArg;
bool isConstructCall;
}JsNativeFunctionInfo;

/// <summary>
/// A function callback.
/// </summary>
/// <param name="callee">
/// A function object that represents the function being invoked.
/// </param>
/// <param name="arguments">The arguments to the call.</param>
/// <param name="argumentCount">The number of arguments.</param>
/// <param name="info">Additional information about this function call.</param>
/// <param name="callbackState">
/// The state passed to <c>JsCreateFunction</c>.
/// </param>
/// <returns>The result of the call, if any.</returns>
typedef _Ret_maybenull_ JsValueRef(CHAKRA_CALLBACK * JsEnhancedNativeFunction)(_In_ JsValueRef callee, _In_ JsValueRef *arguments, _In_ unsigned short argumentCount, _In_ JsNativeFunctionInfo *info, _In_opt_ void *callbackState);

/// <summary>
/// Creates a new enhanced JavaScript function.
/// </summary>
/// <remarks>
/// Requires an active script context.
/// </remarks>
/// <param name="nativeFunction">The method to call when the function is invoked.</param>
/// <param name="metadata">If this is not <c>JS_INVALID_REFERENCE</c>, it is converted to a string and used as the name of the function.</param>
/// <param name="callbackState">
/// User provided state that will be passed back to the callback.
/// </param>
/// <param name="function">The new function object.</param>
/// <returns>
/// The code <c>JsNoError</c> if the operation succeeded, a failure code otherwise.
/// </returns>
CHAKRA_API
JsCreateEnhancedFunction(
_In_ JsEnhancedNativeFunction nativeFunction,
_In_opt_ JsValueRef metadata,
_In_opt_ void *callbackState,
_Out_ JsValueRef *function);

/// <summary>
/// Initialize a ModuleRecord from host
/// </summary>
Expand Down
Loading