Repository navigation
Inconsistent return value with enabled NODE_ADDON_API_ENABLE_MAYBE directive #1246
Description
If preprocessor directives NAPI_DISABLE_CPP_EXCEPTIONS and NODE_ADDON_API_ENABLE_MAYBE are enabled, Napi API calls should return Maybe type https://github.com/nodejs/node-addon-api/blob/HEAD/doc/error_handling.md#handling-errors-with-maybe-type-and-c-exceptions-disabled.
But many Napi methods violate this rule. For example all Napi::Number conversion methods (Int32Value(), Uint32Value(), FloatValue(), DoubleValue()) and Napi::BigInt conversion methods return unwrapped value as is:
Lines 829 to 834 in 39267ba
Lines 90 to 94 in 39267ba
In the following example variable x will have 0 value if info[0] is not a number:
void Foo(const Napi::CallbackInfo& info) {
auto x = info[0].As<Napi::Number>().Int32Value(); // expected x type is Maybe<int32_t>,
// actual type is int32_t
}
Activity
mhdawson commentedon Jan 6, 2023
We discussed in the team meeting today and originally these methods were intentionally left out of those that returns Maybes. @legendecas will take another look and comment.
legendecas commentedon Jan 11, 2023
When a method returns
Maybe<>type, it indicates that it may call into JavaScript. However, those number value getters don't call into JavaScript at all. Nonnapi_okstatus simply means that the type assumption ofNapi::Value::As()is failed.I would find this issue related to unchecked typecast with
Napi::Value::As(). As documented at https://github.com/nodejs/node-addon-api/blob/main/doc/value.md#as,Napi::Value::As()should be used when the type of value is known and the value is the assumed type.I'd propose a new type cast method
Napi::Value::Cast()to enforce the type assumption, or add a new definitionNAPI_ENABLE_TYPE_CHECKto allowNapi::Value::As()to enforce the type assumption.mhdawson commentedon Jan 13, 2023
@vmoroz suggested we make the macro NAPI_ENABLE_AS_TYPE_CHECK
mhdawson commentedon Jan 13, 2023
From follow on discussion NAPI_ENABLE_TYPE_CHECK_ON_AS was suggested