Skip to content

Commit ca786c3

Browse files
boingoingaddaleax
boingoing
authored and
addaleax
committed
n-api: change napi_callback to return napi_value
Change `napi_callback` to return `napi_value` directly instead of requiring `napi_set_return_value`. When we invoke the callback, we will check the return value and call `SetReturnValue` ourselves. If the callback returns `NULL`, we don't set the return value in v8 which would have the same effect as previously if the callback didn't call `napi_set_return_value`. Seems to be a more natural way to handle return values from callbacks. As a consequence, remove `napi_set_return_value`. Add a `napi_value` to `napi_property_descriptor` to support string values which couldn't be passed in the `utf8name` parameter or symbols as property names. Class names, however, cannot be symbols so this `napi_value` must be a string type in that case. Remove all of the `napi_callback_info` helpers except for `napi_get_cb_info` and make all the parameters to `napi_get_cb_info` optional except for argc. Update all the test collateral according to these changes. Also add `test/addons-napi/common.h` to house some common macros for wrapping N-API calls and error handling. PR-URL: #12248 Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>
1 parent b470a85 commit ca786c3

File tree

32 files changed

+939
-1590
lines changed

32 files changed

+939
-1590
lines changed

‎src/node_api.cc‎

Lines changed: 121 additions & 154 deletions
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,70 @@ class napi_env__ {
3636
napi_extended_error_info last_error;
3737
};
3838

39+
#define RETURN_STATUS_IF_FALSE(env, condition, status) \
40+
do { \
41+
if (!(condition)) { \
42+
return napi_set_last_error((env), (status)); \
43+
} \
44+
} while (0)
45+
46+
#define CHECK_ENV(env) \
47+
if ((env) == nullptr) { \
48+
node::FatalError(__func__, "environment(env) must not be null"); \
49+
}
50+
51+
#define CHECK_ARG(env, arg) \
52+
RETURN_STATUS_IF_FALSE((env), ((arg) != nullptr), napi_invalid_arg)
53+
54+
#define CHECK_MAYBE_EMPTY(env, maybe, status) \
55+
RETURN_STATUS_IF_FALSE((env), !((maybe).IsEmpty()), (status))
56+
57+
#define CHECK_MAYBE_NOTHING(env, maybe, status) \
58+
RETURN_STATUS_IF_FALSE((env), !((maybe).IsNothing()), (status))
59+
60+
// NAPI_PREAMBLE is not wrapped in do..while: try_catch must have function scope
61+
#define NAPI_PREAMBLE(env) \
62+
CHECK_ENV((env)); \
63+
RETURN_STATUS_IF_FALSE((env), (env)->last_exception.IsEmpty(), \
64+
napi_pending_exception); \
65+
napi_clear_last_error((env)); \
66+
v8impl::TryCatch try_catch((env))
67+
68+
#define CHECK_TO_TYPE(env, type, context, result, src, status) \
69+
do { \
70+
auto maybe = v8impl::V8LocalValueFromJsValue((src))->To##type((context)); \
71+
CHECK_MAYBE_EMPTY((env), maybe, (status)); \
72+
(result) = maybe.ToLocalChecked(); \
73+
} while (0)
74+
75+
#define CHECK_TO_OBJECT(env, context, result, src) \
76+
CHECK_TO_TYPE((env), Object, (context), (result), (src), napi_object_expected)
77+
78+
#define CHECK_TO_STRING(env, context, result, src) \
79+
CHECK_TO_TYPE((env), String, (context), (result), (src), napi_string_expected)
80+
81+
#define CHECK_TO_NUMBER(env, context, result, src) \
82+
CHECK_TO_TYPE((env), Number, (context), (result), (src), napi_number_expected)
83+
84+
#define CHECK_TO_BOOL(env, context, result, src) \
85+
CHECK_TO_TYPE((env), Boolean, (context), (result), (src), \
86+
napi_boolean_expected)
87+
88+
#define CHECK_NEW_FROM_UTF8_LEN(env, result, str, len) \
89+
do { \
90+
auto str_maybe = v8::String::NewFromUtf8( \
91+
(env)->isolate, (str), v8::NewStringType::kInternalized, (len)); \
92+
CHECK_MAYBE_EMPTY((env), str_maybe, napi_generic_failure); \
93+
(result) = str_maybe.ToLocalChecked(); \
94+
} while (0)
95+
96+
#define CHECK_NEW_FROM_UTF8(env, result, str) \
97+
CHECK_NEW_FROM_UTF8_LEN((env), (result), (str), -1)
98+
99+
#define GET_RETURN_STATUS(env) \
100+
(!try_catch.HasCaught() ? napi_ok \
101+
: napi_set_last_error((env), napi_pending_exception))
102+
39103
namespace v8impl {
40104

41105
// convert from n-api property attributes to v8::PropertyAttribute
@@ -127,6 +191,22 @@ v8::Local<v8::Value> V8LocalValueFromJsValue(napi_value v) {
127191
return local;
128192
}
129193

194+
static inline napi_status V8NameFromPropertyDescriptor(napi_env env,
195+
const napi_property_descriptor* p,
196+
v8::Local<v8::Name>* result) {
197+
if (p->utf8name != nullptr) {
198+
CHECK_NEW_FROM_UTF8(env, *result, p->utf8name);
199+
} else {
200+
v8::Local<v8::Value> property_value =
201+
v8impl::V8LocalValueFromJsValue(p->name);
202+
203+
RETURN_STATUS_IF_FALSE(env, property_value->IsName(), napi_name_expected);
204+
*result = property_value.As<v8::Name>();
205+
}
206+
207+
return napi_ok;
208+
}
209+
130210
// Adapter for napi_finalize callbacks.
131211
class Finalizer {
132212
protected:
@@ -361,13 +441,19 @@ class CallbackWrapperBase : public CallbackWrapper {
361441
v8::Local<v8::External>::Cast(
362442
_cbdata->GetInternalField(kInternalFieldIndex))->Value());
363443
v8::Isolate* isolate = _cbinfo.GetIsolate();
444+
364445
napi_env env = static_cast<napi_env>(
365446
v8::Local<v8::External>::Cast(
366447
_cbdata->GetInternalField(kEnvIndex))->Value());
367448

368449
// Make sure any errors encountered last time we were in N-API are gone.
369450
napi_clear_last_error(env);
370-
cb(env, cbinfo_wrapper);
451+
452+
napi_value result = cb(env, cbinfo_wrapper);
453+
454+
if (result != nullptr) {
455+
this->SetReturnValue(result);
456+
}
371457

372458
if (!env->last_exception.IsEmpty()) {
373459
isolate->ThrowException(
@@ -608,75 +694,12 @@ void napi_module_register(napi_module* mod) {
608694
node::node_module_register(nm);
609695
}
610696

611-
#define RETURN_STATUS_IF_FALSE(env, condition, status) \
612-
do { \
613-
if (!(condition)) { \
614-
return napi_set_last_error((env), (status)); \
615-
} \
616-
} while (0)
617-
618-
#define CHECK_ENV(env) \
619-
if ((env) == nullptr) { \
620-
node::FatalError(__func__, "environment(env) must not be null"); \
621-
}
622-
623-
#define CHECK_ARG(env, arg) \
624-
RETURN_STATUS_IF_FALSE((env), ((arg) != nullptr), napi_invalid_arg)
625-
626-
#define CHECK_MAYBE_EMPTY(env, maybe, status) \
627-
RETURN_STATUS_IF_FALSE((env), !((maybe).IsEmpty()), (status))
628-
629-
#define CHECK_MAYBE_NOTHING(env, maybe, status) \
630-
RETURN_STATUS_IF_FALSE((env), !((maybe).IsNothing()), (status))
631-
632-
// NAPI_PREAMBLE is not wrapped in do..while: try_catch must have function scope
633-
#define NAPI_PREAMBLE(env) \
634-
CHECK_ENV((env)); \
635-
RETURN_STATUS_IF_FALSE((env), (env)->last_exception.IsEmpty(), \
636-
napi_pending_exception); \
637-
napi_clear_last_error((env)); \
638-
v8impl::TryCatch try_catch((env))
639-
640-
#define CHECK_TO_TYPE(env, type, context, result, src, status) \
641-
do { \
642-
auto maybe = v8impl::V8LocalValueFromJsValue((src))->To##type((context)); \
643-
CHECK_MAYBE_EMPTY((env), maybe, (status)); \
644-
(result) = maybe.ToLocalChecked(); \
645-
} while (0)
646-
647-
#define CHECK_TO_OBJECT(env, context, result, src) \
648-
CHECK_TO_TYPE((env), Object, (context), (result), (src), napi_object_expected)
649-
650-
#define CHECK_TO_STRING(env, context, result, src) \
651-
CHECK_TO_TYPE((env), String, (context), (result), (src), napi_string_expected)
652-
653-
#define CHECK_TO_NUMBER(env, context, result, src) \
654-
CHECK_TO_TYPE((env), Number, (context), (result), (src), napi_number_expected)
655-
656-
#define CHECK_TO_BOOL(env, context, result, src) \
657-
CHECK_TO_TYPE((env), Boolean, (context), (result), (src), \
658-
napi_boolean_expected)
659-
660-
#define CHECK_NEW_FROM_UTF8_LEN(env, result, str, len) \
661-
do { \
662-
auto str_maybe = v8::String::NewFromUtf8( \
663-
(env)->isolate, (str), v8::NewStringType::kInternalized, (len)); \
664-
CHECK_MAYBE_EMPTY((env), str_maybe, napi_generic_failure); \
665-
result = str_maybe.ToLocalChecked(); \
666-
} while (0)
667-
668-
#define CHECK_NEW_FROM_UTF8(env, result, str) \
669-
CHECK_NEW_FROM_UTF8_LEN((env), (result), (str), -1)
670-
671-
#define GET_RETURN_STATUS(env) \
672-
(!try_catch.HasCaught() ? napi_ok \
673-
: napi_set_last_error((env), napi_pending_exception))
674-
675697
// Warning: Keep in-sync with napi_status enum
676698
const char* error_messages[] = {nullptr,
677699
"Invalid pointer passed as argument",
678700
"An object was expected",
679701
"A string was expected",
702+
"A string or symbol was expected",
680703
"A function was expected",
681704
"A number was expected",
682705
"A boolean was expected",
@@ -793,8 +816,14 @@ napi_status napi_define_class(napi_env env,
793816
continue;
794817
}
795818

796-
v8::Local<v8::String> property_name;
797-
CHECK_NEW_FROM_UTF8(env, property_name, p->utf8name);
819+
v8::Local<v8::Name> property_name;
820+
napi_status status =
821+
v8impl::V8NameFromPropertyDescriptor(env, p, &property_name);
822+
823+
if (status != napi_ok) {
824+
return napi_set_last_error(env, status);
825+
}
826+
798827
v8::PropertyAttribute attributes =
799828
v8impl::V8PropertyAttributesFromDescriptor(p);
800829

@@ -822,7 +851,6 @@ napi_status napi_define_class(napi_env env,
822851
v8impl::FunctionCallbackWrapper::Invoke,
823852
cbdata,
824853
v8::Signature::New(isolate, tpl));
825-
t->SetClassName(property_name);
826854

827855
tpl->PrototypeTemplate()->Set(property_name, t, attributes);
828856
} else {
@@ -855,18 +883,6 @@ napi_status napi_define_class(napi_env env,
855883
return GET_RETURN_STATUS(env);
856884
}
857885

858-
napi_status napi_set_return_value(napi_env env,
859-
napi_callback_info cbinfo,
860-
napi_value value) {
861-
NAPI_PREAMBLE(env);
862-
863-
v8impl::CallbackWrapper* info =
864-
reinterpret_cast<v8impl::CallbackWrapper*>(cbinfo);
865-
866-
info->SetReturnValue(value);
867-
return GET_RETURN_STATUS(env);
868-
}
869-
870886
napi_status napi_get_property_names(napi_env env,
871887
napi_value object,
872888
napi_value* result) {
@@ -1102,11 +1118,16 @@ napi_status napi_define_properties(napi_env env,
11021118
for (size_t i = 0; i < property_count; i++) {
11031119
const napi_property_descriptor* p = &properties[i];
11041120

1105-
v8::Local<v8::Name> name;
1106-
CHECK_NEW_FROM_UTF8(env, name, p->utf8name);
1121+
v8::Local<v8::Name> property_name;
1122+
napi_status status =
1123+
v8impl::V8NameFromPropertyDescriptor(env, p, &property_name);
1124+
1125+
if (status != napi_ok) {
1126+
return napi_set_last_error(env, status);
1127+
}
11071128

11081129
v8::PropertyAttribute attributes =
1109-
v8impl::V8PropertyAttributesFromDescriptor(p);
1130+
v8impl::V8PropertyAttributesFromDescriptor(p);
11101131

11111132
if (p->getter != nullptr || p->setter != nullptr) {
11121133
v8::Local<v8::Object> cbdata = v8impl::CreateAccessorCallbackData(
@@ -1117,7 +1138,7 @@ napi_status napi_define_properties(napi_env env,
11171138

11181139
auto set_maybe = obj->SetAccessor(
11191140
context,
1120-
name,
1141+
property_name,
11211142
p->getter ? v8impl::GetterCallbackWrapper::Invoke : nullptr,
11221143
p->setter ? v8impl::SetterCallbackWrapper::Invoke : nullptr,
11231144
cbdata,
@@ -1136,8 +1157,8 @@ napi_status napi_define_properties(napi_env env,
11361157
v8::Local<v8::FunctionTemplate> t = v8::FunctionTemplate::New(
11371158
isolate, v8impl::FunctionCallbackWrapper::Invoke, cbdata);
11381159

1139-
auto define_maybe =
1140-
obj->DefineOwnProperty(context, name, t->GetFunction(), attributes);
1160+
auto define_maybe = obj->DefineOwnProperty(
1161+
context, property_name, t->GetFunction(), attributes);
11411162

11421163
if (!define_maybe.FromMaybe(false)) {
11431164
return napi_set_last_error(env, napi_generic_failure);
@@ -1146,7 +1167,7 @@ napi_status napi_define_properties(napi_env env,
11461167
v8::Local<v8::Value> value = v8impl::V8LocalValueFromJsValue(p->value);
11471168

11481169
auto define_maybe =
1149-
obj->DefineOwnProperty(context, name, value, attributes);
1170+
obj->DefineOwnProperty(context, property_name, value, attributes);
11501171

11511172
if (!define_maybe.FromMaybe(false)) {
11521173
return napi_set_last_error(env, napi_invalid_arg);
@@ -1439,33 +1460,24 @@ napi_status napi_get_cb_info(
14391460
napi_value* this_arg, // [out] Receives the JS 'this' arg for the call
14401461
void** data) { // [out] Receives the data pointer for the callback.
14411462
CHECK_ENV(env);
1442-
CHECK_ARG(env, argc);
1443-
CHECK_ARG(env, argv);
1444-
CHECK_ARG(env, this_arg);
1445-
CHECK_ARG(env, data);
14461463

14471464
v8impl::CallbackWrapper* info =
14481465
reinterpret_cast<v8impl::CallbackWrapper*>(cbinfo);
14491466

1450-
info->Args(argv, std::min(*argc, info->ArgsLength()));
1451-
*argc = info->ArgsLength();
1452-
*this_arg = info->This();
1453-
*data = info->Data();
1454-
1455-
return napi_ok;
1456-
}
1457-
1458-
napi_status napi_get_cb_args_length(napi_env env,
1459-
napi_callback_info cbinfo,
1460-
size_t* result) {
1461-
// Omit NAPI_PREAMBLE and GET_RETURN_STATUS because no V8 APIs are called.
1462-
CHECK_ENV(env);
1463-
CHECK_ARG(env, result);
1464-
1465-
v8impl::CallbackWrapper* info =
1466-
reinterpret_cast<v8impl::CallbackWrapper*>(cbinfo);
1467+
if (argv != nullptr) {
1468+
CHECK_ARG(env, argc);
1469+
info->Args(argv, std::min(*argc, info->ArgsLength()));
1470+
}
1471+
if (argc != nullptr) {
1472+
*argc = info->ArgsLength();
1473+
}
1474+
if (this_arg != nullptr) {
1475+
*this_arg = info->This();
1476+
}
1477+
if (data != nullptr) {
1478+
*data = info->Data();
1479+
}
14671480

1468-
*result = info->ArgsLength();
14691481
return napi_ok;
14701482
}
14711483

@@ -1483,51 +1495,6 @@ napi_status napi_is_construct_call(napi_env env,
14831495
return napi_ok;
14841496
}
14851497

1486-
// copy encoded arguments into provided buffer or return direct pointer to
1487-
// encoded arguments array?
1488-
napi_status napi_get_cb_args(napi_env env,
1489-
napi_callback_info cbinfo,
1490-
napi_value* buf,
1491-
size_t bufsize) {
1492-
// Omit NAPI_PREAMBLE and GET_RETURN_STATUS because no V8 APIs are called.
1493-
CHECK_ENV(env);
1494-
CHECK_ARG(env, buf);
1495-
1496-
v8impl::CallbackWrapper* info =
1497-
reinterpret_cast<v8impl::CallbackWrapper*>(cbinfo);
1498-
1499-
info->Args(buf, bufsize);
1500-
return napi_ok;
1501-
}
1502-
1503-
napi_status napi_get_cb_this(napi_env env,
1504-
napi_callback_info cbinfo,
1505-
napi_value* result) {
1506-
// Omit NAPI_PREAMBLE and GET_RETURN_STATUS because no V8 APIs are called.
1507-
CHECK_ENV(env);
1508-
CHECK_ARG(env, result);
1509-
1510-
v8impl::CallbackWrapper* info =
1511-
reinterpret_cast<v8impl::CallbackWrapper*>(cbinfo);
1512-
1513-
*result = info->This();
1514-
return napi_ok;
1515-
}
1516-
1517-
napi_status napi_get_cb_data(napi_env env,
1518-
napi_callback_info cbinfo,
1519-
void** result) {
1520-
// Omit NAPI_PREAMBLE and GET_RETURN_STATUS because no V8 APIs are called.
1521-
CHECK_ENV(env);
1522-
CHECK_ARG(env, result);
1523-
1524-
v8impl::CallbackWrapper* info =
1525-
reinterpret_cast<v8impl::CallbackWrapper*>(cbinfo);
1526-
1527-
*result = info->Data();
1528-
return napi_ok;
1529-
}
1530-
15311498
napi_status napi_call_function(napi_env env,
15321499
napi_value recv,
15331500
napi_value func,

0 commit comments

Comments
 (0)