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

Jump to bottom

Clean up error codes in napi somewhat #13179

Merged
merged 7 commits into from
Aug 13, 2024
Merged

Clean up error codes in napi somewhat #13179

Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
e48f60f
Clean up error codes in napi somewhat
Jarred-Sumner Aug 8, 2024
16a1808
Merge branch 'main' into jarred/napi-err
Jarred-Sumner Aug 13, 2024
3492aed
Update src/bun.js/bindings/napi.cpp
Jarred-Sumner Aug 13, 2024
3b28b2f
Update src/bun.js/bindings/napi.cpp
Jarred-Sumner Aug 13, 2024
3ea3b57
Update src/bun.js/bindings/napi.cpp
Jarred-Sumner Aug 13, 2024
f506a91
Update src/bun.js/bindings/napi.cpp
Jarred-Sumner Aug 13, 2024
8ec6a1c
Update src/bun.js/bindings/napi.cpp
Jarred-Sumner Aug 13, 2024
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
188 changes: 126 additions & 62 deletions src/bun.js/bindings/napi.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -578,10 +578,14 @@ extern "C" napi_status napi_get_property(napi_env env, napi_value object,
{
NAPI_PREMABLE

if (UNLIKELY(!result)) {
if (UNLIKELY(!result || !env)) {
return napi_invalid_arg;
}

if (UNLIKELY(!object)) {
return napi_object_expected;
}

auto globalObject = toJS(env);
auto& vm = globalObject->vm();

Expand Down Expand Up @@ -1199,19 +1203,70 @@ napi_define_properties(napi_env env, napi_value object, size_t property_count,
return napi_ok;
}

static void throwErrorWithCode(JSC::JSGlobalObject* globalObject, const char* msg_utf8, const char* code_utf8, const WTF::Function<JSObject*(JSC::JSGlobalObject*, const WTF::String&)>& createError)
{
auto& vm = globalObject->vm();
auto scope = DECLARE_THROW_SCOPE(vm);

auto message = msg_utf8 ? WTF::String::fromUTF8(msg_utf8) : String();
auto code = msg_utf8 ? WTF::String::fromUTF8(code_utf8) : String();

auto* error = createError(globalObject, message);
if (!code.isEmpty()) {
error->putDirect(vm, WebCore::builtinNames(vm).codePublicName(), JSC::jsString(vm, code), 0);
}

scope.throwException(globalObject, Exception::create(vm, error));
}

static JSValue createErrorForNapi(napi_env env, napi_value code, napi_value msg, const WTF::Function<JSObject*(JSC::JSGlobalObject*, const WTF::String&)>& constructor)
{
auto* globalObject = toJS(env);
JSC::VM& vm = globalObject->vm();
auto catchScope = DECLARE_CATCH_SCOPE(vm);

JSValue codeValue = toJS(code);
WTF::String message;

if (msg) {
JSValue messageValue = toJS(msg);
message = messageValue.toWTFString(globalObject);
if (catchScope.exception()) {
catchScope.clearException();
return {};
}
}

if (catchScope.exception()) {
Jarred-Sumner marked this conversation as resolved.
Show resolved Hide resolved
catchScope.clearException();
Jarred-Sumner marked this conversation as resolved.
Show resolved Hide resolved
return {};
Jarred-Sumner marked this conversation as resolved.
Show resolved Hide resolved
}
Copy link
Contributor

Choose a reason for hiding this comment

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

are these redundant?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think so. This check should be deleted because it looks like the one above would catch any exceptions

Jarred-Sumner marked this conversation as resolved.
Show resolved Hide resolved

auto* error = constructor(globalObject, message);

if (codeValue && error) {
error->putDirect(vm, WebCore::builtinNames(vm).codePublicName(), codeValue, 0);
}

if (catchScope.exception()) {
catchScope.clearException();
return {};
}

return error;
}

extern "C" napi_status napi_throw_error(napi_env env,
const char* code,
const char* msg)
{
NAPI_PREMABLE
Zig::GlobalObject* globalObject = toJS(env);

JSC::VM& vm = globalObject->vm();
auto throwScope = DECLARE_THROW_SCOPE(vm);
throwErrorWithCode(globalObject, msg, code, [](JSC::JSGlobalObject* globalObject, const WTF::String& message) {
return JSC::createError(globalObject, message);
});

auto message = msg != nullptr ? WTF::String::fromUTF8(msg) : "Error"_s;
auto error = JSC::createError(globalObject, message);
JSC::throwException(globalObject, throwScope, error);
return napi_ok;
}

Expand Down Expand Up @@ -1269,6 +1324,9 @@ extern "C" napi_status napi_add_finalizer(napi_env env, napi_value js_object,
napi_ref* result)
{
NAPI_PREMABLE
if (UNLIKELY(!env)) {
return napi_invalid_arg;
}
Zig::GlobalObject* globalObject = toJS(env);
JSC::VM& vm = globalObject->vm();

Expand All @@ -1292,6 +1350,10 @@ extern "C" napi_status napi_reference_unref(napi_env env, napi_ref ref,
uint32_t* result)
{
NAPI_PREMABLE
if (UNLIKELY(!result || !env || !ref)) {
return napi_invalid_arg;
}

NapiRef* napiRef = toJS(ref);
napiRef->unref();
if (LIKELY(result)) {
Expand All @@ -1307,7 +1369,7 @@ extern "C" napi_status napi_get_reference_value(napi_env env, napi_ref ref,
napi_value* result)
{
NAPI_PREMABLE
if (UNLIKELY(!result)) {
if (UNLIKELY(!result || !env || !ref)) {
return napi_invalid_arg;
}
NapiRef* napiRef = toJS(ref);
Expand All @@ -1325,6 +1387,9 @@ extern "C" JSC__JSValue napi_get_reference_value_internal(NapiRef* napiRef)
extern "C" napi_status napi_reference_ref(napi_env env, napi_ref ref,
uint32_t* result)
{
if (UNLIKELY(!result || !env || !ref)) {
return napi_invalid_arg;
}
NAPI_PREMABLE
NapiRef* napiRef = toJS(ref);
napiRef->ref();
Expand All @@ -1337,6 +1402,9 @@ extern "C" napi_status napi_reference_ref(napi_env env, napi_ref ref,
extern "C" napi_status napi_delete_reference(napi_env env, napi_ref ref)
{
NAPI_PREMABLE
if (UNLIKELY(!env || !ref)) {
return napi_invalid_arg;
}
NapiRef* napiRef = toJS(ref);
delete napiRef;
return napi_ok;
Expand Down Expand Up @@ -1365,7 +1433,6 @@ extern "C" napi_status napi_is_detached_arraybuffer(napi_env env,
}

auto arrayBuffer = jsArrayBuffer->impl();

*result = arrayBuffer->isDetached();
return napi_ok;
}
Expand Down Expand Up @@ -1497,16 +1564,15 @@ extern "C" napi_status node_api_create_syntax_error(napi_env env,
return napi_invalid_arg;
}

JSValue messageValue = toJS(msg);
JSValue codeValue = toJS(code);
auto globalObject = toJS(env);
JSC::VM& vm = globalObject->vm();
auto* err = messageValue && !messageValue.isUndefinedOrNull() ? createSyntaxError(globalObject, messageValue.toWTFString(globalObject)) : createSyntaxError(globalObject);
if (codeValue && !codeValue.isUndefinedOrNull()) {
err->putDirect(vm, WebCore::builtinNames(vm).codePublicName(), codeValue, 0);
auto err = createErrorForNapi(env, code, msg, [](JSC::JSGlobalObject* globalObject, const WTF::String& message) {
return JSC::createSyntaxError(globalObject, message);
});

if (UNLIKELY(!err)) {
return napi_generic_failure;
}

*result = reinterpret_cast<napi_value>(JSC::JSValue::encode(err));
*result = toNapi(err);
return napi_ok;
}

Expand All @@ -1516,16 +1582,12 @@ extern "C" napi_status node_api_throw_syntax_error(napi_env env,
{
NAPI_PREMABLE

auto message = msg ? WTF::String::fromUTF8(msg) : String();
auto globalObject = toJS(env);
JSC::VM& vm = globalObject->vm();
auto* err = createSyntaxError(globalObject, message);
if (code) {
err->putDirect(vm, WebCore::builtinNames(vm).codePublicName(), JSC::jsString(vm, String::fromUTF8(code)), 0);
}

auto scope = DECLARE_THROW_SCOPE(vm);
scope.throwException(globalObject, err);
throwErrorWithCode(globalObject, msg, code, [](JSC::JSGlobalObject* globalObject, const WTF::String& message) {
return JSC::createSyntaxError(globalObject, message);
});

return napi_ok;
}

Expand All @@ -1535,31 +1597,34 @@ extern "C" napi_status napi_throw_type_error(napi_env env, const char* code,
NAPI_PREMABLE
Zig::GlobalObject* globalObject = toJS(env);

JSC::VM& vm = globalObject->vm();
auto throwScope = DECLARE_THROW_SCOPE(vm);
throwErrorWithCode(globalObject, msg, code, [](JSC::JSGlobalObject* globalObject, const WTF::String& message) {
return JSC::createTypeError(globalObject, message);
});

auto message = WTF::String::fromUTF8(msg);
auto error = JSC::createTypeError(globalObject, message);
JSC::throwException(globalObject, throwScope, error);
return napi_ok;
}

extern "C" napi_status napi_create_type_error(napi_env env, napi_value code,
napi_value msg,
napi_value* result)
{
Zig::GlobalObject* globalObject = toJS(env);
JSC::VM& vm = globalObject->vm();
if (UNLIKELY(!result || !env)) {
return napi_invalid_arg;
}

JSC::JSValue codeValue = toJS(code);
JSC::JSValue messageValue = toJS(msg);
auto err = createErrorForNapi(env, code, msg, [](JSC::JSGlobalObject* globalObject, const WTF::String& message) {
if (message.isEmpty()) {
return JSC::createTypeError(globalObject);
}

auto error = JSC::createTypeError(globalObject, messageValue.toWTFString(globalObject));
if (codeValue) {
error->putDirect(vm, WebCore::builtinNames(vm).codePublicName(), codeValue, 0);
return JSC::createTypeError(globalObject, message);
});

if (UNLIKELY(!err)) {
return napi_generic_failure;
}

*result = reinterpret_cast<napi_value>(JSC::JSValue::encode(error));
*result = toNapi(err);
return napi_ok;
}

Expand All @@ -1573,23 +1638,19 @@ extern "C" napi_status napi_create_error(napi_env env, napi_value code,
return napi_invalid_arg;
}

Zig::GlobalObject* globalObject = toJS(env);
JSC::VM& vm = globalObject->vm();

JSC::JSValue codeValue = toJS(code);
JSC::JSValue messageValue = toJS(msg);
auto err = createErrorForNapi(env, code, msg, [](JSC::JSGlobalObject* globalObject, const WTF::String& message) {
if (message.isEmpty()) {
return JSC::createError(globalObject, String("Error"_s));
}

WTF::String message = messageValue.toWTFString(globalObject);
if (message.isEmpty()) {
message = "Error"_s;
}
return JSC::createError(globalObject, message);
});

auto* error = JSC::createError(globalObject, message);
if (codeValue) {
error->putDirect(vm, WebCore::builtinNames(vm).codePublicName(), codeValue, 0);
if (UNLIKELY(!err)) {
return napi_generic_failure;
}

*result = reinterpret_cast<napi_value>(JSC::JSValue::encode(error));
*result = toNapi(err);
return napi_ok;
}
extern "C" napi_status napi_throw_range_error(napi_env env, const char* code,
Expand All @@ -1598,18 +1659,19 @@ extern "C" napi_status napi_throw_range_error(napi_env env, const char* code,
NAPI_PREMABLE
Zig::GlobalObject* globalObject = toJS(env);

JSC::VM& vm = globalObject->vm();
auto throwScope = DECLARE_THROW_SCOPE(vm);
throwErrorWithCode(globalObject, msg, code, [](JSC::JSGlobalObject* globalObject, const WTF::String& message) {
return JSC::createRangeError(globalObject, message);
});

auto message = WTF::String::fromUTF8(msg);
auto error = JSC::createRangeError(globalObject, message);
JSC::throwException(globalObject, throwScope, error);
return napi_ok;
}

extern "C" napi_status napi_object_freeze(napi_env env, napi_value object_value)
{
NAPI_PREMABLE
if (UNLIKELY(!env || !object_value)) {
return napi_invalid_arg;
}

Zig::GlobalObject* globalObject = toJS(env);
JSC::VM& vm = globalObject->vm();
Expand Down Expand Up @@ -1673,16 +1735,18 @@ extern "C" napi_status napi_create_range_error(napi_env env, napi_value code,
return napi_invalid_arg;
}

Zig::GlobalObject* globalObject = toJS(env);
auto err = createErrorForNapi(env, code, msg, [](JSC::JSGlobalObject* globalObject, const WTF::String& message) {
if (message.isEmpty()) {
return JSC::createRangeError(globalObject, String("Range error"_s));
}

JSC::JSValue codeValue = toJS(code);
JSC::JSValue messageValue = toJS(msg);
return JSC::createRangeError(globalObject, message);
});

auto error = JSC::createRangeError(globalObject, messageValue.toWTFString(globalObject));
if (codeValue) {
error->putDirect(globalObject->vm(), WebCore::builtinNames(globalObject->vm()).codePublicName(), codeValue, 0);
if (UNLIKELY(!err)) {
return napi_generic_failure;
}
*result = reinterpret_cast<napi_value>(error);
Jarred-Sumner marked this conversation as resolved.
Show resolved Hide resolved

Copy link
Collaborator Author

@Jarred-Sumner Jarred-Sumner Aug 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

*result = toNapi(err);

Jarred-Sumner marked this conversation as resolved.
Show resolved Hide resolved
return napi_ok;
}

Expand Down
Loading