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

src,doc: add SyntaxError napi support #40736

Closed
wants to merge 16 commits into from
Closed

src,doc: add SyntaxError napi support #40736

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
bdf8809
src,doc: add SyntaxError napi support
Nov 6, 2021
2f2a2b3
fix: properly declare the test helper methods
Nov 6, 2021
6fafc06
mark the new APIs as experimental
Nov 6, 2021
d3e7ac7
fix tests on MacOS
Nov 6, 2021
a9cc563
rename napi_ to node_api_
Nov 9, 2021
c22c337
fix: lint
Nov 9, 2021
5c52bec
Update doc/api/n-api.md
mhdawson Nov 15, 2021
f733054
Update doc/api/n-api.md
mhdawson Nov 15, 2021
c6adcb4
Update doc/api/n-api.md
mhdawson Nov 15, 2021
e0398c6
Update src/js_native_api.h
mhdawson Nov 15, 2021
e4165cc
Update src/js_native_api.h
mhdawson Nov 15, 2021
99216f4
Update src/js_native_api.h
mhdawson Nov 15, 2021
606cc0b
Update src/js_native_api.h
mhdawson Nov 15, 2021
f2c8ad6
Update src/js_native_api_v8.cc
mhdawson Nov 15, 2021
a8906c4
Update src/js_native_api_v8.cc
mhdawson Nov 15, 2021
8c2a1b3
squash: run make format -md
mhdawson Nov 15, 2021
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
51 changes: 48 additions & 3 deletions doc/api/n-api.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1065,12 +1065,11 @@ JavaScript value to be thrown.
The following utility functions are also available in case native code
needs to throw an exception or determine if a `napi_value` is an instance
of a JavaScript `Error` object: [`napi_throw_error`][],
[`napi_throw_type_error`][], [`napi_throw_range_error`][] and
[`napi_is_error`][].
[`napi_throw_type_error`][], [`napi_throw_range_error`][], [`napi_throw_syntax_error`][] and [`napi_is_error`][].

The following utility functions are also available in case native
code needs to create an `Error` object: [`napi_create_error`][],
[`napi_create_type_error`][], and [`napi_create_range_error`][],
[`napi_create_type_error`][], [`napi_create_range_error`][] and [`napi_create_syntax_error`][],
where result is the `napi_value` that refers to the newly created
JavaScript `Error` object.

Expand Down Expand Up @@ -1180,6 +1179,26 @@ Returns `napi_ok` if the API succeeded.

This API throws a JavaScript `RangeError` with the text provided.

#### `napi_throw_syntax_error`

<!-- YAML
added: REPLACEME
-->

```c
NAPI_EXTERN napi_status napi_throw_syntax_error(napi_env env,
const char* code,
const char* msg);
mhdawson marked this conversation as resolved.
Show resolved Hide resolved
```
mhdawson marked this conversation as resolved.
Show resolved Hide resolved

* `[in] env`: The environment that the API is invoked under.
* `[in] code`: Optional error code to be set on the error.
* `[in] msg`: C string representing the text to be associated with the error.

Returns `napi_ok` if the API succeeded.

This API throws a JavaScript `SyntaxError` with the text provided.

#### `napi_is_error`

<!-- YAML
Expand Down Expand Up @@ -1277,6 +1296,30 @@ Returns `napi_ok` if the API succeeded.

This API returns a JavaScript `RangeError` with the text provided.

#### `napi_create_syntax_error`

<!-- YAML
added: REPLACEME
-->

```c
NAPI_EXTERN napi_status napi_create_syntax_error(napi_env env,
napi_value code,
napi_value msg,
napi_value* result);
mhdawson marked this conversation as resolved.
Show resolved Hide resolved
```

* `[in] env`: The environment that the API is invoked under.
* `[in] code`: Optional `napi_value` with the string for the error code to be
associated with the error.
* `[in] msg`: `napi_value` that references a JavaScript `string` to be used as
the message for the `Error`.
* `[out] result`: `napi_value` representing the error created.

Returns `napi_ok` if the API succeeded.

This API returns a JavaScript `SyntaxError` with the text provided.

#### `napi_get_and_clear_last_exception`

<!-- YAML
Expand Down Expand Up @@ -6257,6 +6300,7 @@ the add-on's file name during loading.
[`napi_create_external_arraybuffer`]: #napi_create_external_arraybuffer
[`napi_create_range_error`]: #napi_create_range_error
[`napi_create_reference`]: #napi_create_reference
[`napi_create_syntax_error`]: #napi_create_syntax_error
[`napi_create_type_error`]: #napi_create_type_error
[`napi_define_class`]: #napi_define_class
[`napi_delete_async_work`]: #napi_delete_async_work
Expand Down Expand Up @@ -6290,6 +6334,7 @@ the add-on's file name during loading.
[`napi_threadsafe_function_call_js`]: #napi_threadsafe_function_call_js
[`napi_throw_error`]: #napi_throw_error
[`napi_throw_range_error`]: #napi_throw_range_error
[`napi_throw_syntax_error`]: #napi_throw_syntax_error
[`napi_throw_type_error`]: #napi_throw_type_error
[`napi_throw`]: #napi_throw
[`napi_unwrap`]: #napi_unwrap
Expand Down
7 changes: 7 additions & 0 deletions src/js_native_api.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,10 @@ NAPI_EXTERN napi_status napi_create_range_error(napi_env env,
napi_value code,
napi_value msg,
napi_value* result);
NAPI_EXTERN napi_status napi_create_syntax_error(napi_env env,
Copy link
Member

Choose a reason for hiding this comment

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

This is a new api and it should be enabled / disabled throw NAPI_EXPERIMENTAL:

#ifdef NAPI_EXPERIMENTAL
NAPI_EXTERN 
napi_status napi_create_syntax_error(napi_env env,
                                     napi_value code,
                                     napi_value msg,
                                     napi_value* result);
#endif  // NAPI_EXPERIMENTAL

napi_value code,
napi_value msg,
napi_value* result);
mhdawson marked this conversation as resolved.
Show resolved Hide resolved

// Methods to get the native napi_value from Primitive type
NAPI_EXTERN napi_status napi_typeof(napi_env env,
Expand Down Expand Up @@ -370,6 +374,9 @@ NAPI_EXTERN napi_status napi_throw_type_error(napi_env env,
NAPI_EXTERN napi_status napi_throw_range_error(napi_env env,
const char* code,
const char* msg);
NAPI_EXTERN napi_status napi_throw_syntax_error(napi_env env,
Copy link
Member

Choose a reason for hiding this comment

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

This is a new api and it should be enabled / disabled throw NAPI_EXPERIMENTAL:

#ifdef NAPI_EXPERIMENTAL
NAPI_EXTERN 
napi_status napi_throw_syntax_error(napi_env env,
                                    const char* code,
                                    const char* msg,);
#endif  // NAPI_EXPERIMENTAL

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure, it's here: 6fafc06

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@NickNaso I see that adding this ifdef failed the tests on Mac (due to implicit declaration of function 'napi_throw_syntax_error' is invalid in C99 [-Werror,-Wimplicit-function-declaration]), tho not on the other platforms.

I assume it's either that the compiler is stricter on Mac (or defined to be stricter), or that the tests in this setup are not running with NAPI_EXPERIMENTAL.

What do you recommend to do?

Copy link
Member

Choose a reason for hiding this comment

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

You should add this declaration:

#define NAPI_EXPERIMENTAL

to the head of file /test/js-native-api/test_error/test_error.c

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@NickNaso Thanks. It helped.

The failing action (test-asan / test-asan (pull_request)) seems flaky, I see:

[UNEXPECTED_FAILURE][FAIL] Entry 0 startTime is approximately correct (up to 20ms difference allowed)
assert_approx_equals: expected 4102.578573999926 +/- 20 but got 4077.9045040002093
    at Test.<anonymous> (/home/runner/work/node/node/test/fixtures/wpt/user-timing/mark.any.js:61:8)
    at Test.step (/home/runner/work/node/node/test/fixtures/wpt/resources/testharness.js:2087:25)
    at test (/home/runner/work/node/node/test/fixtures/wpt/resources/testharness.js:557:30)
    at test_mark (/home/runner/work/node/node/test/fixtures/wpt/user-timing/mark.any.js:59:4)
    at /home/runner/work/node/node/test/fixtures/wpt/user-timing/mark.any.js:117:3
Command: /home/runner/work/node/node/out/Release/node  /home/runner/work/node/node/test/wpt/test-user-timing.js mark.any.js

I don't have the permissions to re-run. can you please?

Copy link
Member

Choose a reason for hiding this comment

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

@NickNaso yes, that is what we agreed to do for new functions use node_api instead of napi

It is what we said we'd do in https://nodejs.medium.com/renaming-n-api-to-node-api-27aa8ca30ed8

Please @idan-at could you please rename the functions you added like reported below?

  • node_api_throw_syntax_error
  • node_api_create_syntax_error

Copy link
Contributor Author

Choose a reason for hiding this comment

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

of course - did it here a9cc563

const char* code,
const char* msg);
mhdawson marked this conversation as resolved.
Show resolved Hide resolved
NAPI_EXTERN napi_status napi_is_error(napi_env env,
napi_value value,
bool* result);
Expand Down
38 changes: 38 additions & 0 deletions src/js_native_api_v8.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1747,6 +1747,26 @@ napi_status napi_create_range_error(napi_env env,
return napi_clear_last_error(env);
}

napi_status napi_create_syntax_error(napi_env env,
napi_value code,
napi_value msg,
napi_value* result) {
mhdawson marked this conversation as resolved.
Show resolved Hide resolved
CHECK_ENV(env);
CHECK_ARG(env, msg);
CHECK_ARG(env, result);

v8::Local<v8::Value> message_value = v8impl::V8LocalValueFromJsValue(msg);
RETURN_STATUS_IF_FALSE(env, message_value->IsString(), napi_string_expected);

v8::Local<v8::Value> error_obj =
v8::Exception::SyntaxError(message_value.As<v8::String>());
STATUS_CALL(set_error_code(env, error_obj, code, nullptr));

*result = v8impl::JsValueFromV8LocalValue(error_obj);

return napi_clear_last_error(env);
}

napi_status napi_typeof(napi_env env,
napi_value value,
napi_valuetype* result) {
Expand Down Expand Up @@ -1964,6 +1984,24 @@ napi_status napi_throw_range_error(napi_env env,
return napi_clear_last_error(env);
}

napi_status napi_throw_syntax_error(napi_env env,
const char* code,
const char* msg) {
mhdawson marked this conversation as resolved.
Show resolved Hide resolved
NAPI_PREAMBLE(env);

v8::Isolate* isolate = env->isolate;
v8::Local<v8::String> str;
CHECK_NEW_FROM_UTF8(env, str, msg);

v8::Local<v8::Value> error_obj = v8::Exception::SyntaxError(str);
STATUS_CALL(set_error_code(env, error_obj, nullptr, code));

isolate->ThrowException(error_obj);
// any VM calls after this point and before returning
// to the javascript invoker will fail
return napi_clear_last_error(env);
}

napi_status napi_is_error(napi_env env, napi_value value, bool* result) {
// Omit NAPI_PREAMBLE and GET_RETURN_STATUS because V8 calls here cannot
// throw JS exceptions.
Expand Down
23 changes: 23 additions & 0 deletions test/js-native-api/test_error/test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,10 @@ assert.throws(() => {
test_error.throwTypeError();
}, /^TypeError: type error$/);

assert.throws(() => {
test_error.throwSyntaxError();
}, /^SyntaxError: syntax error$/);

[42, {}, [], Symbol('xyzzy'), true, 'ball', undefined, null, NaN]
.forEach((value) => assert.throws(
() => test_error.throwArbitrary(value),
Expand Down Expand Up @@ -90,6 +94,13 @@ assert.throws(
message: 'TypeError [type error]'
});

assert.throws(
() => test_error.throwSyntaxErrorCode(),
{
code: 'ERR_TEST_CODE',
message: 'SyntaxError [syntax error]'
});

let error = test_error.createError();
assert.ok(error instanceof Error, 'expected error to be an instance of Error');
assert.strictEqual(error.message, 'error');
Expand All @@ -104,6 +115,11 @@ assert.ok(error instanceof TypeError,
'expected error to be an instance of TypeError');
assert.strictEqual(error.message, 'type error');

error = test_error.createSyntaxError();
assert.ok(error instanceof SyntaxError,
'expected error to be an instance of SyntaxError');
assert.strictEqual(error.message, 'syntax error');

error = test_error.createErrorCode();
assert.ok(error instanceof Error, 'expected error to be an instance of Error');
assert.strictEqual(error.code, 'ERR_TEST_CODE');
Expand All @@ -123,3 +139,10 @@ assert.ok(error instanceof TypeError,
assert.strictEqual(error.message, 'TypeError [type error]');
assert.strictEqual(error.code, 'ERR_TEST_CODE');
assert.strictEqual(error.name, 'TypeError');

error = test_error.createSyntaxErrorCode();
assert.ok(error instanceof SyntaxError,
'expected error to be an instance of SyntaxError');
assert.strictEqual(error.message, 'SyntaxError [syntax error]');
assert.strictEqual(error.code, 'ERR_TEST_CODE');
assert.strictEqual(error.name, 'SyntaxError');
36 changes: 36 additions & 0 deletions test/js-native-api/test_error/test_error.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,11 @@ static napi_value throwTypeError(napi_env env, napi_callback_info info) {
return NULL;
}

static napi_value throwSyntaxError(napi_env env, napi_callback_info info) {
NODE_API_CALL(env, napi_throw_syntax_error(env, NULL, "syntax error"));
return NULL;
}

static napi_value throwErrorCode(napi_env env, napi_callback_info info) {
NODE_API_CALL(env, napi_throw_error(env, "ERR_TEST_CODE", "Error [error]"));
return NULL;
Expand All @@ -57,6 +62,11 @@ static napi_value throwTypeErrorCode(napi_env env, napi_callback_info info) {
return NULL;
}

static napi_value throwSyntaxErrorCode(napi_env env, napi_callback_info info) {
NODE_API_CALL(env,
napi_throw_syntax_error(env, "ERR_TEST_CODE", "SyntaxError [syntax error]"));
return NULL;
}

static napi_value createError(napi_env env, napi_callback_info info) {
napi_value result;
Expand Down Expand Up @@ -85,6 +95,15 @@ static napi_value createTypeError(napi_env env, napi_callback_info info) {
return result;
}

static napi_value createSyntaxError(napi_env env, napi_callback_info info) {
napi_value result;
napi_value message;
NODE_API_CALL(env, napi_create_string_utf8(
env, "syntax error", NAPI_AUTO_LENGTH, &message));
NODE_API_CALL(env, napi_create_syntax_error(env, NULL, message, &result));
return result;
}

static napi_value createErrorCode(napi_env env, napi_callback_info info) {
napi_value result;
napi_value message;
Expand Down Expand Up @@ -123,6 +142,19 @@ static napi_value createTypeErrorCode(napi_env env, napi_callback_info info) {
return result;
}

static napi_value createSyntaxErrorCode(napi_env env, napi_callback_info info) {
napi_value result;
napi_value message;
napi_value code;
NODE_API_CALL(env,
napi_create_string_utf8(
env, "SyntaxError [syntax error]", NAPI_AUTO_LENGTH, &message));
NODE_API_CALL(env, napi_create_string_utf8(
env, "ERR_TEST_CODE", NAPI_AUTO_LENGTH, &code));
NODE_API_CALL(env, napi_create_syntax_error(env, code, message, &result));
return result;
}

static napi_value throwArbitrary(napi_env env, napi_callback_info info) {
napi_value arbitrary;
size_t argc = 1;
Expand All @@ -139,16 +171,20 @@ napi_value Init(napi_env env, napi_value exports) {
DECLARE_NODE_API_PROPERTY("throwError", throwError),
DECLARE_NODE_API_PROPERTY("throwRangeError", throwRangeError),
DECLARE_NODE_API_PROPERTY("throwTypeError", throwTypeError),
DECLARE_NODE_API_PROPERTY("throwSyntaxError", throwSyntaxError),
DECLARE_NODE_API_PROPERTY("throwErrorCode", throwErrorCode),
DECLARE_NODE_API_PROPERTY("throwRangeErrorCode", throwRangeErrorCode),
DECLARE_NODE_API_PROPERTY("throwTypeErrorCode", throwTypeErrorCode),
DECLARE_NODE_API_PROPERTY("throwSyntaxErrorCode", throwSyntaxErrorCode),
DECLARE_NODE_API_PROPERTY("throwArbitrary", throwArbitrary),
DECLARE_NODE_API_PROPERTY("createError", createError),
DECLARE_NODE_API_PROPERTY("createRangeError", createRangeError),
DECLARE_NODE_API_PROPERTY("createTypeError", createTypeError),
DECLARE_NODE_API_PROPERTY("createSyntaxError", createSyntaxError),
DECLARE_NODE_API_PROPERTY("createErrorCode", createErrorCode),
DECLARE_NODE_API_PROPERTY("createRangeErrorCode", createRangeErrorCode),
DECLARE_NODE_API_PROPERTY("createTypeErrorCode", createTypeErrorCode),
DECLARE_NODE_API_PROPERTY("createSyntaxErrorCode", createSyntaxErrorCode),
};

NODE_API_CALL(env, napi_define_properties(
Expand Down