Skip to content

Commit c127b71

Browse files
boingoingMylesBorins
boingoing
authored and
MylesBorins
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. Backport-PR-URL: #19447 PR-URL: #12248 Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>
1 parent a6af97f commit c127b71

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
@@ -38,6 +38,70 @@ class napi_env__ {
3838
napi_extended_error_info last_error;
3939
};
4040

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

43107
// convert from n-api property attributes to v8::PropertyAttribute
@@ -129,6 +193,22 @@ v8::Local<v8::Value> V8LocalValueFromJsValue(napi_value v) {
129193
return local;
130194
}
131195

196+
static inline napi_status V8NameFromPropertyDescriptor(napi_env env,
197+
const napi_property_descriptor* p,
198+
v8::Local<v8::Name>* result) {
199+
if (p->utf8name != nullptr) {
200+
CHECK_NEW_FROM_UTF8(env, *result, p->utf8name);
201+
} else {
202+
v8::Local<v8::Value> property_value =
203+
v8impl::V8LocalValueFromJsValue(p->name);
204+
205+
RETURN_STATUS_IF_FALSE(env, property_value->IsName(), napi_name_expected);
206+
*result = property_value.As<v8::Name>();
207+
}
208+
209+
return napi_ok;
210+
}
211+
132212
// Adapter for napi_finalize callbacks.
133213
class Finalizer {
134214
protected:
@@ -363,13 +443,19 @@ class CallbackWrapperBase : public CallbackWrapper {
363443
v8::Local<v8::External>::Cast(
364444
_cbdata->GetInternalField(kInternalFieldIndex))->Value());
365445
v8::Isolate* isolate = _cbinfo.GetIsolate();
446+
366447
napi_env env = static_cast<napi_env>(
367448
v8::Local<v8::External>::Cast(
368449
_cbdata->GetInternalField(kEnvIndex))->Value());
369450

370451
// Make sure any errors encountered last time we were in N-API are gone.
371452
napi_clear_last_error(env);
372-
cb(env, cbinfo_wrapper);
453+
454+
napi_value result = cb(env, cbinfo_wrapper);
455+
456+
if (result != nullptr) {
457+
this->SetReturnValue(result);
458+
}
373459

374460
if (!env->last_exception.IsEmpty()) {
375461
isolate->ThrowException(
@@ -610,75 +696,12 @@ void napi_module_register(napi_module* mod) {
610696
node::node_module_register(nm);
611697
}
612698

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

798-
v8::Local<v8::String> property_name;
799-
CHECK_NEW_FROM_UTF8(env, property_name, p->utf8name);
821+
v8::Local<v8::Name> property_name;
822+
napi_status status =
823+
v8impl::V8NameFromPropertyDescriptor(env, p, &property_name);
824+
825+
if (status != napi_ok) {
826+
return napi_set_last_error(env, status);
827+
}
828+
800829
v8::PropertyAttribute attributes =
801830
v8impl::V8PropertyAttributesFromDescriptor(p);
802831

@@ -824,7 +853,6 @@ napi_status napi_define_class(napi_env env,
824853
v8impl::FunctionCallbackWrapper::Invoke,
825854
cbdata,
826855
v8::Signature::New(isolate, tpl));
827-
t->SetClassName(property_name);
828856

829857
tpl->PrototypeTemplate()->Set(property_name, t, attributes);
830858
} else {
@@ -857,18 +885,6 @@ napi_status napi_define_class(napi_env env,
857885
return GET_RETURN_STATUS(env);
858886
}
859887

860-
napi_status napi_set_return_value(napi_env env,
861-
napi_callback_info cbinfo,
862-
napi_value value) {
863-
NAPI_PREAMBLE(env);
864-
865-
v8impl::CallbackWrapper* info =
866-
reinterpret_cast<v8impl::CallbackWrapper*>(cbinfo);
867-
868-
info->SetReturnValue(value);
869-
return GET_RETURN_STATUS(env);
870-
}
871-
872888
napi_status napi_get_property_names(napi_env env,
873889
napi_value object,
874890
napi_value* result) {
@@ -1104,11 +1120,16 @@ napi_status napi_define_properties(napi_env env,
11041120
for (size_t i = 0; i < property_count; i++) {
11051121
const napi_property_descriptor* p = &properties[i];
11061122

1107-
v8::Local<v8::Name> name;
1108-
CHECK_NEW_FROM_UTF8(env, name, p->utf8name);
1123+
v8::Local<v8::Name> property_name;
1124+
napi_status status =
1125+
v8impl::V8NameFromPropertyDescriptor(env, p, &property_name);
1126+
1127+
if (status != napi_ok) {
1128+
return napi_set_last_error(env, status);
1129+
}
11091130

11101131
v8::PropertyAttribute attributes =
1111-
v8impl::V8PropertyAttributesFromDescriptor(p);
1132+
v8impl::V8PropertyAttributesFromDescriptor(p);
11121133

11131134
if (p->getter != nullptr || p->setter != nullptr) {
11141135
v8::Local<v8::Object> cbdata = v8impl::CreateAccessorCallbackData(
@@ -1119,7 +1140,7 @@ napi_status napi_define_properties(napi_env env,
11191140

11201141
auto set_maybe = obj->SetAccessor(
11211142
context,
1122-
name,
1143+
property_name,
11231144
p->getter ? v8impl::GetterCallbackWrapper::Invoke : nullptr,
11241145
p->setter ? v8impl::SetterCallbackWrapper::Invoke : nullptr,
11251146
cbdata,
@@ -1138,8 +1159,8 @@ napi_status napi_define_properties(napi_env env,
11381159
v8::Local<v8::FunctionTemplate> t = v8::FunctionTemplate::New(
11391160
isolate, v8impl::FunctionCallbackWrapper::Invoke, cbdata);
11401161

1141-
auto define_maybe =
1142-
obj->DefineOwnProperty(context, name, t->GetFunction(), attributes);
1162+
auto define_maybe = obj->DefineOwnProperty(
1163+
context, property_name, t->GetFunction(), attributes);
11431164

11441165
if (!define_maybe.FromMaybe(false)) {
11451166
return napi_set_last_error(env, napi_generic_failure);
@@ -1148,7 +1169,7 @@ napi_status napi_define_properties(napi_env env,
11481169
v8::Local<v8::Value> value = v8impl::V8LocalValueFromJsValue(p->value);
11491170

11501171
auto define_maybe =
1151-
obj->DefineOwnProperty(context, name, value, attributes);
1172+
obj->DefineOwnProperty(context, property_name, value, attributes);
11521173

11531174
if (!define_maybe.FromMaybe(false)) {
11541175
return napi_set_last_error(env, napi_invalid_arg);
@@ -1441,33 +1462,24 @@ napi_status napi_get_cb_info(
14411462
napi_value* this_arg, // [out] Receives the JS 'this' arg for the call
14421463
void** data) { // [out] Receives the data pointer for the callback.
14431464
CHECK_ENV(env);
1444-
CHECK_ARG(env, argc);
1445-
CHECK_ARG(env, argv);
1446-
CHECK_ARG(env, this_arg);
1447-
CHECK_ARG(env, data);
14481465

14491466
v8impl::CallbackWrapper* info =
14501467
reinterpret_cast<v8impl::CallbackWrapper*>(cbinfo);
14511468

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

1470-
*result = info->ArgsLength();
14711483
return napi_ok;
14721484
}
14731485

@@ -1485,51 +1497,6 @@ napi_status napi_is_construct_call(napi_env env,
14851497
return napi_ok;
14861498
}
14871499

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

0 commit comments

Comments
 (0)