napi: add bigint support #21226

devsnek · 2018-06-09T04:20:57Z

Checklist

/cc @nodejs/n-api passing a bigint to a napi function still throws Invalid argument and i can't figure out why.

/cc @nodejs/v8 should we block on this until we have a way to get values from bigint?

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

hashseed · 2018-06-09T07:02:46Z

It's not entirely clear what is expected from a BigInt API in V8. You can discuss here: https://bugs.chromium.org/p/v8/issues/detail?id=7712

mhdawson · 2018-06-11T17:41:11Z

This should also wait until we have completed #19962 which will be updated so new functions go in initially as experimental.

mhdawson

Making as changes requested so that it does not land until we document how to guard as experimental initially.

TimothyGu

Missing documentation. Particularly important is the question of endianness for the word array-based functions.

TimothyGu · 2018-07-06T00:35:56Z

src/node_api.cc

+  RETURN_STATUS_IF_FALSE(env, val->IsBigInt(), napi_bigint_expected);
+
+  val.As<v8::BigInt>()->ToWordsArray(
+      sign_bit, reinterpret_cast<int*>(word_count), words);


The most significant part of word_count might not get cleared after the cast.

how should i fix this

just word_count = 0; before executing ToWordsArray

TimothyGu · 2018-07-06T00:37:46Z

src/node_api.cc

+  v8::Local<v8::Context> context = env->isolate->GetCurrentContext();
+
+  v8::MaybeLocal<v8::BigInt> b = v8::BigInt::NewFromWords(
+      context, reinterpret_cast<int>(sign_bit), word_count, words);


We should throw when word_count > INT_MAX.

devsnek · 2018-07-06T01:36:57Z

napi type checks fail between napi_valuetype and error_messages with the NAPI_EXPERIMENTAL gate. @mhdawson @kfarnung any ideas?

kfarnung · 2018-07-06T02:09:06Z

src/node_api_types.h

@@ -68,6 +75,9 @@ typedef enum {
  napi_name_expected,
  napi_function_expected,
  napi_number_expected,
+#ifdef NAPI_EXPERIMENTAL


I don't think it's really necessary to gate them with NAPI_EXPERIMENTAL, just the new functions.

Do you agree @mhdawson @digitalinfinity @gabrielschulhof

Perhaps we must not gate here because if two APIs (such as thread_safe and bigint) add new status codes while in experimental, they will have to come out of experimental in the order their added status codes appear, otherwise their emergence from experimental will itself constitute an ABI break, since their status codes would have to be moved before the bracket.

/me go PR the removal of the gate now 🙂

This also means that if we backport bigint but not thread_safe, we'll have to backport the status codes added by thread_safe so that the status codes for bigint will have the same value betweeen the backport and the current version.

kfarnung · 2018-07-06T02:09:13Z

src/node_api_types.h

@@ -68,6 +75,9 @@ typedef enum {
  napi_name_expected,
  napi_function_expected,
  napi_number_expected,
+#ifdef NAPI_EXPERIMENTAL
+  napi_bigint_expected,
+#endif  // NAPI_EXPERIMENTAL


We should only ever add enum values to the bottom since they need to be stable over time.

kfarnung · 2018-07-06T02:09:48Z

src/node_api_types.h

@@ -41,6 +41,9 @@ typedef enum {
  napi_null,
  napi_boolean,
  napi_number,
+#ifdef NAPI_EXPERIMENTAL
+  napi_bigint,


Same here, we want existing values to remain unchanged.

kfarnung · 2018-07-06T02:10:26Z

src/node_api.h

@@ -635,6 +635,34 @@ NAPI_EXTERN napi_status napi_remove_env_cleanup_hook(napi_env env,

 #ifdef NAPI_EXPERIMENTAL

+NAPI_EXTERN napi_status napi_create_bigint_int64(napi_env env,


nit: can we add these after the existing ones?

gabrielschulhof · 2018-07-06T02:07:55Z

doc/api/n-api.md

@@ -105,6 +105,9 @@ typedef enum {
  napi_name_expected,
  napi_function_expected,
  napi_number_expected,
+#ifdef NAPI_EXPERIMENTAL
+  napi_bigint_expected,


This value should go to the end of the enum, otherwise we break ABI. Once you've moved it to the end of the enum, you also need to append a string explaining the error message to the end of the array of error messages, otherwise you get a failed assertion.

gabrielschulhof · 2018-07-06T02:08:13Z

doc/api/n-api.md

@@ -1220,6 +1223,9 @@ typedef enum {
  napi_null,
  napi_boolean,
  napi_number,
+#ifdef NAPI_EXPERIMENTAL
+  napi_bigint,


kfarnung

We should only be adding to existing enums, not inserting new values.

TimothyGu

I'd also like to mention in the docs that the output may be undefined if a method fails. Consider napi_get_value_bigint_words(). I'd assume the content of sign_bit to be indeterminate if that function fails for whatever reason.

TimothyGu · 2018-07-06T01:45:04Z

doc/api/n-api.md

+
+Returns `napi_ok` if the API succeeded.
+
+This API is used to convert the C `int64_t` type to the JavaScript `BigInt`


s/is used to convert/converts/

Ditto below.

TimothyGu · 2018-07-06T01:48:34Z

doc/api/n-api.md

+
+The resulting number is calculated as:
+
+`(-1)^sign_bit * (words[0] * (2^64)^0 + words[1] * (2^64)^1 + ...)`


The format could probably be improved a bit:

(–1)^sign_bit (words[0] × (2⁶⁴)⁰ + words[1] × (2⁶⁴)¹ + …)

TBH I'd still like to see a declarative explanation of what this means ("words is expressed in little endian").

TimothyGu · 2018-07-06T01:49:26Z

doc/api/n-api.md

+- `[out] result`: A `napi_value` representing a JavaScript `BigInt`.
+
+Returns `napi_ok` if the API succeeded.
+


Missing a small blurb of what the function does, like the other functions do.

This API converts an array of unsigned 64-bit words into a single BigInt value.

TimothyGu · 2018-07-06T01:50:29Z

doc/api/n-api.md

+- `[in] value`: `napi_value` represenging JavaScript `BigInt`.
+- `[out] result`: C int64_t primitive equivalent of the given JavaScript
+  `BigInt`.
+- `[out] lossless`: Indicates if the bigint value was truncated losslessly.


Inconsistent capitalization: decide on BigInt or BigInt.

TimothyGu · 2018-07-06T01:50:50Z

doc/api/n-api.md

+```
+
+- `[in] env`: The environment that the API is invoked under
+- `[in] value`: `napi_value` represenging JavaScript `BigInt`.


typo: representing

TimothyGu · 2018-07-06T01:51:39Z

doc/api/n-api.md

+`napi_bigint_expected`.
+
+This API returns the C int64_t primitive equivalent of the given JavaScript
+`BigInt`. If needed it will truncate the value, setting `lossless` to `true`.


This behavior sounds a bit counterintuitive to me: doesn't truncation lose some value? Why is lossless set to true then?

TimothyGu · 2018-07-06T01:54:03Z

doc/api/n-api.md

+```C
+napi_status napi_get_bigint_word_count(napi_env env,
+                                       napi_value value,
+                                       size_t* word_count);


Instead of a separate API, could we instead make the functionality included in napi_get_value_bigint_words() when words pointer is set to NULL?

this would require also having to set sign_bit to NULL but i like the idea, will work on it.

TimothyGu · 2018-07-06T01:54:29Z

doc/api/n-api.md

+- `[out] words`: Pointer to 64-bit word array.
+
+Returns `napi_ok` if the API succeeded.
+


Also missing a sentence describing what the function does. I'd rather the longer descriptions in the parameter list be moved down here.

TimothyGu · 2018-07-06T01:56:06Z

doc/api/n-api.md

+- `[in] value`: `napi_value` representing JavaScript `BigInt`.
+- `[out] sign_bit`: Integer representing if the JavaScript `BigInt` is positive
+   or negative.
+- `[out] word_count`: Must be initialized to the length of the `words` array.


I know this convention hasn't been used in the document yet, but [in/out]?

TimothyGu · 2018-07-06T01:58:06Z

src/node_api.cc

+  v8::Local<v8::Context> context = env->isolate->GetCurrentContext();
+
+  if (word_count > INT_MAX) {
+    return napi_set_last_error(env, napi_generic_failure);


I'd rather this be consistent with what V8 does when the word count is too big, which is to throw a RangeError.

if v8 handles it, all we have to do is nothing... right? the try_catch should carry the error

No, V8 only handles the error conditions within the range of int. We still need to handle the cases of greater than INT_MAX.

devsnek · 2018-07-06T03:54:39Z

not really sure what happened but its back to throwing Invalid argument when a bigint is passed

kfarnung · 2018-07-10T17:18:16Z

doc/api/n-api.md

+- `[in/out] word_count`: Must be initialized to the length of the `words`
+   array. Upon return, it will be set to the actual number of words that
+   would be needed to store this `BigInt`.
+- `[out] words`: Pointer to 64-bit word array.


It should probably indicate that the words array needs to be pre-allocated for clarity.

kfarnung · 2018-07-10T17:23:48Z

test/addons-napi/test_bigint/test.js

+    assert.strictEqual(num, TestUint64(num));
+  }
+  assert.strictEqual(num, TestWords(num));
+});


It would probably be good to have a test or two for "lossy" conversions.

devsnek · 2018-07-12T00:55:45Z

CI https://ci.nodejs.org/job/node-test-pull-request/15818/

devsnek · 2018-07-12T03:14:17Z

landing tomorrow afternoon (cst) if no one objects by then

/cc @addaleax @TimothyGu @mhdawson

devsnek · 2018-07-12T18:03:06Z

landed in 1849a2b

PR-URL: #21226 Reviewed-By: Gabriel Schulhof <[email protected]> Reviewed-By: Kyle Farnung <[email protected]>

Notable changes: * console: * The `console.timeLog()` method has been implemented. (#21312) * deps: * Upgrade to libuv 1.22.0. (#21731) * Upgrade to ICU 62.1 (Unicode 11, CLDR 33.1). (#21728) * http: * Added support for passing both `timeout` and `agent` options to `http.request`. (#21204) * napi: * Added experimental support for functions dealing with bigint numbers. (#21226) * process: * The `process.hrtime.bigint()` method has been implemented. (#21256) * Added the `--title` command line argument to set the process title on startup. (#21477) * trace_events: * Added process_name metadata. (#21477)

Notable changes: * console: * The `console.timeLog()` method has been implemented. (#21312) * deps: * Upgrade to libuv 1.22.0. (#21731) * Upgrade to ICU 62.1 (Unicode 11, CLDR 33.1). (#21728) * http: * Added support for passing both `timeout` and `agent` options to `http.request`. (#21204) * napi: * Added experimental support for functions dealing with bigint numbers. (#21226) * process: * The `process.hrtime.bigint()` method has been implemented. (#21256) * Added the `--title` command line argument to set the process title on startup. (#21477) * trace_events: * Added process_name metadata. (#21477) * Added new collaborators * codebytere - Shelley Vohr PR-URL: #21851

Notable changes: * console: * The `console.timeLog()` method has been implemented. (#21312) * deps: * Upgrade to libuv 1.22.0. (#21731) * Upgrade to ICU 62.1 (Unicode 11, CLDR 33.1). (#21728) * http: * Added support for passing both `timeout` and `agent` options to `http.request`. (#21204) * inspector: * Expose the original console API in `require('inspector').console`. (#21659) * napi: * Added experimental support for functions dealing with bigint numbers. (#21226) * process: * The `process.hrtime.bigint()` method has been implemented. (#21256) * Added the `--title` command line argument to set the process title on startup. (#21477) * trace_events: * Added process_name metadata. (#21477) * Added new collaborators * codebytere - Shelley Vohr PR-URL: #21851

devsnek added the node-api Issues and PRs related to the Node-API. label Jun 9, 2018

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Jun 9, 2018

devsnek force-pushed the feature/napi-bigint branch from df04ae3 to 5e695db Compare June 9, 2018 04:30

devsnek added wip Issues and PRs that are still a work in progress. and removed lib / src Issues and PRs related to general changes in the lib or src directory. c++ Issues and PRs that require attention from people who are familiar with C++. labels Jun 9, 2018

mhdawson previously requested changes Jun 11, 2018

View reviewed changes

devsnek force-pushed the feature/napi-bigint branch 3 times, most recently from 73faabe to 9474d13 Compare June 28, 2018 15:02

devsnek mentioned this pull request Jun 28, 2018

bigint support nodejs/node-addon-api#292

Merged

devsnek requested a review from addaleax June 28, 2018 15:47

devsnek force-pushed the feature/napi-bigint branch from 9474d13 to 6000350 Compare June 28, 2018 15:57

devsnek force-pushed the feature/napi-bigint branch from 6000350 to f5e1a37 Compare July 5, 2018 23:02

TimothyGu previously requested changes Jul 6, 2018

View reviewed changes

kfarnung reviewed Jul 6, 2018

View reviewed changes

gabrielschulhof reviewed Jul 6, 2018

View reviewed changes

kfarnung previously requested changes Jul 6, 2018

View reviewed changes

TimothyGu reviewed Jul 6, 2018

View reviewed changes

devsnek force-pushed the feature/napi-bigint branch 2 times, most recently from fe73e59 to 5102bd1 Compare July 6, 2018 03:31

kfarnung reviewed Jul 10, 2018

View reviewed changes

squash! misc

129b5b4

kfarnung reviewed Jul 10, 2018

View reviewed changes

squash! false false false

fb87701

kfarnung approved these changes Jul 12, 2018

View reviewed changes

squash! test lossless

32e8e45

devsnek closed this Jul 12, 2018

devsnek added a commit that referenced this pull request Jul 12, 2018

napi: add bigint support

1849a2b

PR-URL: #21226 Reviewed-By: Gabriel Schulhof <[email protected]> Reviewed-By: Kyle Farnung <[email protected]>

devsnek deleted the feature/napi-bigint branch July 12, 2018 18:03

devsnek added notable-change PRs with changes that should be highlighted in changelogs. and removed wip Issues and PRs that are still a work in progress. labels Jul 12, 2018

targos pushed a commit that referenced this pull request Jul 14, 2018

napi: add bigint support

3096ee5

PR-URL: #21226 Reviewed-By: Gabriel Schulhof <[email protected]> Reviewed-By: Kyle Farnung <[email protected]>

targos added the semver-minor PRs that contain new features and should be released in the next minor version. label Jul 17, 2018

targos mentioned this pull request Jul 17, 2018

v10.7.0 proposal #21851

Merged

devsnek restored the feature/napi-bigint branch July 18, 2018 15:52

MylesBorins added dont-land-on-v6.x labels Aug 10, 2018

		@@ -635,6 +635,34 @@ NAPI_EXTERN napi_status napi_remove_env_cleanup_hook(napi_env env,

		#ifdef NAPI_EXPERIMENTAL

		NAPI_EXTERN napi_status napi_create_bigint_int64(napi_env env,


		Returns `napi_ok` if the API succeeded.

		This API is used to convert the C `int64_t` type to the JavaScript `BigInt`


		The resulting number is calculated as:

		`(-1)^sign_bit * (words[0] * (2^64)^0 + words[1] * (2^64)^1 + ...)`

		- `[out] result`: A `napi_value` representing a JavaScript `BigInt`.

		Returns `napi_ok` if the API succeeded.

		- `[out] words`: Pointer to 64-bit word array.

		Returns `napi_ok` if the API succeeded.

napi: add bigint support #21226

napi: add bigint support #21226

Conversation

devsnek commented Jun 9, 2018 • edited Loading

Checklist

hashseed commented Jun 9, 2018

mhdawson commented Jun 11, 2018

mhdawson left a comment

Choose a reason for hiding this comment

TimothyGu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devsnek commented Jul 6, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kfarnung left a comment

Choose a reason for hiding this comment

TimothyGu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devsnek Jul 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devsnek Jul 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devsnek commented Jul 6, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devsnek commented Jul 12, 2018

devsnek commented Jul 12, 2018 • edited Loading

devsnek commented Jul 12, 2018

devsnek commented Jun 9, 2018 •

edited

Loading

devsnek Jul 6, 2018 •

edited

Loading

devsnek Jul 6, 2018 •

edited

Loading

devsnek commented Jul 12, 2018 •

edited

Loading