napi: 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.

Author: boingoing

src/node_api.cc

@@ -32,6 +32,70 @@ class napi_env__ {
   napi_extended_error_info last_error;
 };
 
+#define RETURN_STATUS_IF_FALSE(env, condition, status)                  \
+  do {                                                                  \
+    if (!(condition)) {                                                 \
+      return napi_set_last_error((env), (status));                      \
+    }                                                                   \
+  } while (0)
+
+#define CHECK_ENV(env)                                               \
+  if ((env) == nullptr) {                                            \
+    node::FatalError(__func__, "environment(env) must not be null"); \
+  }
+
+#define CHECK_ARG(env, arg) \
+  RETURN_STATUS_IF_FALSE((env), ((arg) != nullptr), napi_invalid_arg)
+
+#define CHECK_MAYBE_EMPTY(env, maybe, status) \
+  RETURN_STATUS_IF_FALSE((env), !((maybe).IsEmpty()), (status))
+
+#define CHECK_MAYBE_NOTHING(env, maybe, status) \
+  RETURN_STATUS_IF_FALSE((env), !((maybe).IsNothing()), (status))
+
+// NAPI_PREAMBLE is not wrapped in do..while: try_catch must have function scope
+#define NAPI_PREAMBLE(env)                                       \
+  CHECK_ENV((env));                                              \
+  RETURN_STATUS_IF_FALSE((env), (env)->last_exception.IsEmpty(), \
+                         napi_pending_exception);                \
+  napi_clear_last_error((env));                                  \
+  v8impl::TryCatch try_catch((env))
+
+#define CHECK_TO_TYPE(env, type, context, result, src, status)                \
+  do {                                                                        \
+    auto maybe = v8impl::V8LocalValueFromJsValue((src))->To##type((context)); \
+    CHECK_MAYBE_EMPTY((env), maybe, (status));                                \
+    (result) = maybe.ToLocalChecked();                                        \
+  } while (0)
+
+#define CHECK_TO_OBJECT(env, context, result, src) \
+  CHECK_TO_TYPE((env), Object, (context), (result), (src), napi_object_expected)
+
+#define CHECK_TO_STRING(env, context, result, src) \
+  CHECK_TO_TYPE((env), String, (context), (result), (src), napi_string_expected)
+
+#define CHECK_TO_NUMBER(env, context, result, src) \
+  CHECK_TO_TYPE((env), Number, (context), (result), (src), napi_number_expected)
+
+#define CHECK_TO_BOOL(env, context, result, src)            \
+  CHECK_TO_TYPE((env), Boolean, (context), (result), (src), \
+    napi_boolean_expected)
+
+#define CHECK_NEW_FROM_UTF8_LEN(env, result, str, len)                   \
+  do {                                                                   \
+    auto str_maybe = v8::String::NewFromUtf8(                            \
+        (env)->isolate, (str), v8::NewStringType::kInternalized, (len)); \
+    CHECK_MAYBE_EMPTY((env), str_maybe, napi_generic_failure);           \
+    result = str_maybe.ToLocalChecked();                                 \
+  } while (0)
+
+#define CHECK_NEW_FROM_UTF8(env, result, str) \
+  CHECK_NEW_FROM_UTF8_LEN((env), (result), (str), -1)
+
+#define GET_RETURN_STATUS(env)      \
+  (!try_catch.HasCaught() ? napi_ok \
+                         : napi_set_last_error((env), napi_pending_exception))
+
 namespace v8impl {
 
 // convert from n-api property attributes to v8::PropertyAttribute
@@ -123,6 +187,22 @@ v8::Local<v8::Value> V8LocalValueFromJsValue(napi_value v) {
   return local;
 }
 
+napi_status V8NameFromPropertyDescriptor(napi_env env,
+                                         const napi_property_descriptor* p,
+                                         v8::Local<v8::Name>* result) {
+  if (p->utf8name != nullptr) {
+    CHECK_NEW_FROM_UTF8(env, *result, p->utf8name);
+  } else {
+    v8::Local<v8::Value> property_value =
+      v8impl::V8LocalValueFromJsValue(p->name);
+
+    RETURN_STATUS_IF_FALSE(env, property_value->IsName(), napi_name_expected);
+    *result = property_value.As<v8::Name>();
+  }
+
+  return napi_ok;
+}
+
 // Adapter for napi_finalize callbacks.
 class Finalizer {
  protected:
@@ -357,13 +437,19 @@ class CallbackWrapperBase : public CallbackWrapper {
         v8::Local<v8::External>::Cast(
             _cbdata->GetInternalField(kInternalFieldIndex))->Value());
     v8::Isolate* isolate = _cbinfo.GetIsolate();
+
     napi_env env = static_cast<napi_env>(
         v8::Local<v8::External>::Cast(
             _cbdata->GetInternalField(kEnvIndex))->Value());
 
     // Make sure any errors encountered last time we were in N-API are gone.
     napi_clear_last_error(env);
-    cb(env, cbinfo_wrapper);
+
+    napi_value result = cb(env, cbinfo_wrapper);
+
+    if (result != nullptr) {
+      this->SetReturnValue(result);
+    }
 
     if (!env->last_exception.IsEmpty()) {
       isolate->ThrowException(
@@ -604,75 +690,12 @@ void napi_module_register(napi_module* mod) {
   node::node_module_register(nm);
 }
 
-#define RETURN_STATUS_IF_FALSE(env, condition, status)                  \
-  do {                                                                  \
-    if (!(condition)) {                                                 \
-      return napi_set_last_error((env), (status));                      \
-    }                                                                   \
-  } while (0)
-
-#define CHECK_ENV(env)                                               \
-  if ((env) == nullptr) {                                            \
-    node::FatalError(__func__, "environment(env) must not be null"); \
-  }
-
-#define CHECK_ARG(env, arg) \
-  RETURN_STATUS_IF_FALSE((env), ((arg) != nullptr), napi_invalid_arg)
-
-#define CHECK_MAYBE_EMPTY(env, maybe, status) \
-  RETURN_STATUS_IF_FALSE((env), !((maybe).IsEmpty()), (status))
-
-#define CHECK_MAYBE_NOTHING(env, maybe, status) \
-  RETURN_STATUS_IF_FALSE((env), !((maybe).IsNothing()), (status))
-
-// NAPI_PREAMBLE is not wrapped in do..while: try_catch must have function scope
-#define NAPI_PREAMBLE(env)                                       \
-  CHECK_ENV((env));                                              \
-  RETURN_STATUS_IF_FALSE((env), (env)->last_exception.IsEmpty(), \
-                         napi_pending_exception);                \
-  napi_clear_last_error((env));                                  \
-  v8impl::TryCatch try_catch((env))
-
-#define CHECK_TO_TYPE(env, type, context, result, src, status)                \
-  do {                                                                        \
-    auto maybe = v8impl::V8LocalValueFromJsValue((src))->To##type((context)); \
-    CHECK_MAYBE_EMPTY((env), maybe, (status));                                \
-    (result) = maybe.ToLocalChecked();                                        \
-  } while (0)
-
-#define CHECK_TO_OBJECT(env, context, result, src) \
-  CHECK_TO_TYPE((env), Object, (context), (result), (src), napi_object_expected)
-
-#define CHECK_TO_STRING(env, context, result, src) \
-  CHECK_TO_TYPE((env), String, (context), (result), (src), napi_string_expected)
-
-#define CHECK_TO_NUMBER(env, context, result, src) \
-  CHECK_TO_TYPE((env), Number, (context), (result), (src), napi_number_expected)
-
-#define CHECK_TO_BOOL(env, context, result, src)            \
-  CHECK_TO_TYPE((env), Boolean, (context), (result), (src), \
-    napi_boolean_expected)
-
-#define CHECK_NEW_FROM_UTF8_LEN(env, result, str, len)                   \
-  do {                                                                   \
-    auto str_maybe = v8::String::NewFromUtf8(                            \
-        (env)->isolate, (str), v8::NewStringType::kInternalized, (len)); \
-    CHECK_MAYBE_EMPTY((env), str_maybe, napi_generic_failure);           \
-    result = str_maybe.ToLocalChecked();                                 \
-  } while (0)
-
-#define CHECK_NEW_FROM_UTF8(env, result, str) \
-  CHECK_NEW_FROM_UTF8_LEN((env), (result), (str), -1)
-
-#define GET_RETURN_STATUS(env)      \
-  (!try_catch.HasCaught() ? napi_ok \
-                         : napi_set_last_error((env), napi_pending_exception))
-
 // Warning: Keep in-sync with napi_status enum
 const char* error_messages[] = {nullptr,
                                 "Invalid pointer passed as argument",
                                 "An object was expected",
                                 "A string was expected",
+                                "A string or symbol was expected",
                                 "A function was expected",
                                 "A number was expected",
                                 "A boolean was expected",
@@ -789,10 +812,16 @@ napi_status napi_define_class(napi_env env,
       continue;
     }
 
-    v8::Local<v8::String> property_name;
-    CHECK_NEW_FROM_UTF8(env, property_name, p->utf8name);
+    v8::Local<v8::Name> property_name;
+    napi_status status =
+      v8impl::V8NameFromPropertyDescriptor(env, p, &property_name);
+
+    if (status != napi_ok) {
+      return napi_set_last_error(env, status);
+    }
+
     v8::PropertyAttribute attributes =
-        v8impl::V8PropertyAttributesFromDescriptor(p);
+      v8impl::V8PropertyAttributesFromDescriptor(p);
 
     // This code is similar to that in napi_define_properties(); the
     // difference is it applies to a template instead of an object.
@@ -809,7 +838,7 @@ napi_status napi_define_class(napi_env env,
         attributes);
     } else if (p->method != nullptr) {
       v8::Local<v8::Object> cbdata =
-          v8impl::CreateFunctionCallbackData(env, p->method, p->data);
+        v8impl::CreateFunctionCallbackData(env, p->method, p->data);
 
       RETURN_STATUS_IF_FALSE(env, !cbdata.IsEmpty(), napi_generic_failure);
 
@@ -818,7 +847,6 @@ napi_status napi_define_class(napi_env env,
           v8impl::FunctionCallbackWrapper::Invoke,
           cbdata,
           v8::Signature::New(isolate, tpl));
-      t->SetClassName(property_name);
 
       tpl->PrototypeTemplate()->Set(property_name, t, attributes);
     } else {
@@ -851,18 +879,6 @@ napi_status napi_define_class(napi_env env,
   return GET_RETURN_STATUS(env);
 }
 
-napi_status napi_set_return_value(napi_env env,
-                                  napi_callback_info cbinfo,
-                                  napi_value value) {
-  NAPI_PREAMBLE(env);
-
-  v8impl::CallbackWrapper* info =
-      reinterpret_cast<v8impl::CallbackWrapper*>(cbinfo);
-
-  info->SetReturnValue(value);
-  return GET_RETURN_STATUS(env);
-}
-
 napi_status napi_get_property_names(napi_env env,
                                     napi_value object,
                                     napi_value* result) {
@@ -1098,8 +1114,13 @@ napi_status napi_define_properties(napi_env env,
   for (size_t i = 0; i < property_count; i++) {
     const napi_property_descriptor* p = &properties[i];
 
-    v8::Local<v8::Name> name;
-    CHECK_NEW_FROM_UTF8(env, name, p->utf8name);
+    v8::Local<v8::Name> property_name;
+    napi_status status =
+      v8impl::V8NameFromPropertyDescriptor(env, p, &property_name);
+
+    if (status != napi_ok) {
+      return napi_set_last_error(env, status);
+    }
 
     v8::PropertyAttribute attributes =
           v8impl::V8PropertyAttributesFromDescriptor(p);
@@ -1113,7 +1134,7 @@ napi_status napi_define_properties(napi_env env,
 
       auto set_maybe = obj->SetAccessor(
         context,
-        name,
+        property_name,
         p->getter ? v8impl::GetterCallbackWrapper::Invoke : nullptr,
         p->setter ? v8impl::SetterCallbackWrapper::Invoke : nullptr,
         cbdata,
@@ -1132,8 +1153,8 @@ napi_status napi_define_properties(napi_env env,
       v8::Local<v8::FunctionTemplate> t = v8::FunctionTemplate::New(
           isolate, v8impl::FunctionCallbackWrapper::Invoke, cbdata);
 
-      auto define_maybe =
-          obj->DefineOwnProperty(context, name, t->GetFunction(), attributes);
+      auto define_maybe = obj->DefineOwnProperty(
+        context, property_name, t->GetFunction(), attributes);
 
       if (!define_maybe.FromMaybe(false)) {
         return napi_set_last_error(env, napi_generic_failure);
@@ -1142,7 +1163,7 @@ napi_status napi_define_properties(napi_env env,
       v8::Local<v8::Value> value = v8impl::V8LocalValueFromJsValue(p->value);
 
       auto define_maybe =
-          obj->DefineOwnProperty(context, name, value, attributes);
+          obj->DefineOwnProperty(context, property_name, value, attributes);
 
       if (!define_maybe.FromMaybe(false)) {
         return napi_set_last_error(env, napi_invalid_arg);
@@ -1435,33 +1456,24 @@ napi_status napi_get_cb_info(
     napi_value* this_arg,  // [out] Receives the JS 'this' arg for the call
     void** data) {         // [out] Receives the data pointer for the callback.
   CHECK_ENV(env);
-  CHECK_ARG(env, argc);
-  CHECK_ARG(env, argv);
-  CHECK_ARG(env, this_arg);
-  CHECK_ARG(env, data);
 
   v8impl::CallbackWrapper* info =
       reinterpret_cast<v8impl::CallbackWrapper*>(cbinfo);
 
-  info->Args(argv, std::min(*argc, info->ArgsLength()));
-  *argc = info->ArgsLength();
-  *this_arg = info->This();
-  *data = info->Data();
-
-  return napi_ok;
-}
-
-napi_status napi_get_cb_args_length(napi_env env,
-                                    napi_callback_info cbinfo,
-                                    size_t* result) {
-  // Omit NAPI_PREAMBLE and GET_RETURN_STATUS because no V8 APIs are called.
-  CHECK_ENV(env);
-  CHECK_ARG(env, result);
-
-  v8impl::CallbackWrapper* info =
-      reinterpret_cast<v8impl::CallbackWrapper*>(cbinfo);
+  if (argv != nullptr) {
+    CHECK_ARG(env, argc);
+    info->Args(argv, std::min(*argc, info->ArgsLength()));
+  }
+  if (argc != nullptr) {
+    *argc = info->ArgsLength();
+  }
+  if (this_arg != nullptr) {
+    *this_arg = info->This();
+  }
+  if (data != nullptr) {
+    *data = info->Data();
+  }
 
-  *result = info->ArgsLength();
   return napi_ok;
 }
 
@@ -1479,51 +1491,6 @@ napi_status napi_is_construct_call(napi_env env,
   return napi_ok;
 }
 
-// copy encoded arguments into provided buffer or return direct pointer to
-// encoded arguments array?
-napi_status napi_get_cb_args(napi_env env,
-                             napi_callback_info cbinfo,
-                             napi_value* buf,
-                             size_t bufsize) {
-  // Omit NAPI_PREAMBLE and GET_RETURN_STATUS because no V8 APIs are called.
-  CHECK_ENV(env);
-  CHECK_ARG(env, buf);
-
-  v8impl::CallbackWrapper* info =
-      reinterpret_cast<v8impl::CallbackWrapper*>(cbinfo);
-
-  info->Args(buf, bufsize);
-  return napi_ok;
-}
-
-napi_status napi_get_cb_this(napi_env env,
-                             napi_callback_info cbinfo,
-                             napi_value* result) {
-  // Omit NAPI_PREAMBLE and GET_RETURN_STATUS because no V8 APIs are called.
-  CHECK_ENV(env);
-  CHECK_ARG(env, result);
-
-  v8impl::CallbackWrapper* info =
-      reinterpret_cast<v8impl::CallbackWrapper*>(cbinfo);
-
-  *result = info->This();
-  return napi_ok;
-}
-
-napi_status napi_get_cb_data(napi_env env,
-                             napi_callback_info cbinfo,
-                             void** result) {
-  // Omit NAPI_PREAMBLE and GET_RETURN_STATUS because no V8 APIs are called.
-  CHECK_ENV(env);
-  CHECK_ARG(env, result);
-
-  v8impl::CallbackWrapper* info =
-      reinterpret_cast<v8impl::CallbackWrapper*>(cbinfo);
-
-  *result = info->Data();
-  return napi_ok;
-}
-
 napi_status napi_call_function(napi_env env,
                                napi_value recv,
                                napi_value func,