test(NODE-4976): ensure all use of BigInt is gated by the useBigInt64 flag #558

W-A-James · 2023-01-30T20:33:31Z

Description

We would like js-bson to be usable in environments which do not support BigInt and so it is important to ensure that any use of BigInt features in the library should be gated by the useBigInt64 flag.

What is changing?

add new evergreen task to run test suite without access to BigInt APIs
update tests in test/node/bigint.test.ts to be skipped when BigInt is not available
updated package.json and package-lock.json to install eslint plugin
updated .eslintrc.json to enable eslint plugin to fail on the usage of BigInt literals and functions so that builds fail when BigInt features are used without explicitly adding an eslint-disable comment above the line using these features

Is there new documentation needed for these changes?

No

Double check the following

Ran npm run lint script
Self-review completed using the steps outlined here
PR title follows the correct format: type(NODE-xxxx)[!]: description
- Example: feat(NODE-1234)!: rewriting everything in coffeescript
Changes are covered by tests

…s_are_unused

…d' of github.com:mongodb/js-bson into NODE-4976/add_tests_to_ensure_bigint_features_are_unused

baileympearson

forgot to publish my changes yesterday

baileympearson · 2023-02-02T19:32:32Z

etc/eslint/no-bigint-literals/index.js

@@ -0,0 +1,20 @@
+module.exports = {


Currently, all BigInt usage is guarded behind useBigInt64, but we should use this lint rule to enforce that for the future. If we only check for literal syntax, future changes that add construction of BigInts programmatically might not be guarded behind the same flag.

Each usage of bigints, literal syntax or programatic construction, should just ignore the lint rule and explain why it's valid to ignore it in that scenario.

baileympearson · 2023-02-02T19:34:09Z

etc/eslint/no-bigint-literals/test/no_bigint_literals.test.js

@@ -0,0 +1,14 @@
+const { rules } = require('../index');


I think we should add infrastructure to test the eslint plugins during our testing runs. Maybe a npm run check:-plugins command in the root package.json, that runs the plugin tests. These should be incorporated into our regular test runs and CI.

baileympearson · 2023-02-02T19:34:31Z

test/node/bigint.test.ts

+    let getBigInt64Calls = 0;
+    let setBigInt64Calls = 0;
+    let getBigUint64Calls = 0;
+    let setBigUint64Calls = 0;
+    before(function () {
+      // eslint-disable-next-line @typescript-eslint/no-unused-vars
+      const { _, exports } = loadCJSModuleBSON({
+        BigInt: null,
+        DataView: class extends DataView {
+          getBigInt64(byteOffset: number, littleEndian?: boolean): bigint {
+            getBigInt64Calls += 1;
+            return super.getBigInt64(byteOffset, littleEndian);
+          }
+          getBigUint64(byteOffset: number, littleEndian?: boolean): bigint {
+            getBigUint64Calls += 1;
+            return super.getBigUint64(byteOffset, littleEndian);
+          }
+          setBigUint64(byteOffset: number, value: bigint, littleEndian?: boolean): void {
+            setBigUint64Calls += 1;
+            super.setBigUint64(byteOffset, value, littleEndian);
+          }
+          setBigInt64(byteOffset: number, value: bigint, littleEndian?: boolean): void {
+            setBigInt64Calls += 1;
+            super.setBigInt64(byteOffset, value, littleEndian);
+          }
+        }
+      });


from slack: investigate using sinon

baileympearson

Mostly minor comments, nice job. CI looks good too

baileympearson · 2023-02-06T19:45:29Z

package.json

    "build": "npm run build:dts && npm run build:bundle",
-    "check:lint": "eslint -v && eslint --ext '.js,.ts' --max-warnings=0 src test && npm run build:dts && npm run check:tsd",
-    "format": "eslint --ext '.js,.ts' src test --fix",
+    "check:lint": "npm run build:eslint-plugin && eslint -v && eslint --ext '.js,.ts' --max-warnings=0 src test && npm run build:dts && npm run check:tsd",
+    "format": "npm run build:eslint-plugin && eslint --ext '.js,.ts' src test --fix",
    "check:coverage": "nyc --check-coverage npm run check:node",
    "prepare": "node etc/prepare.js",


are the changes here necessary? running npm install installs dependencies for all dependencies listed in the package.json. since the plugin is listed as a dev dependency, npm i will install its dependencies as well.

baileympearson · 2023-02-06T20:02:32Z

etc/eslint/no-bigint-usage/test/no_calls_to_BigInt_methods.test.js

+      x = BigInt.asUintN(64, 10n);
+      x = BigInt.toString(10n);
+      x = BigInt.valueOf(10n);
+      x = BigInt.whatever();


Suggested change

x = BigInt.whatever();

x = BigInt.toLocaleString();

We may as well test all 5 static bigint methods if we're going to test 4.

Testing for this case would be somewhat different as BigInt.toLocaleString is an instance method and we'd have to know beforehand that the value the method is being called is a bigint.

This should be possible, but would require a refactor of the plugin to make use of ts-parser, which I could look more into.

Edit:
Just realized that this also applies to BigInt.toString and BigInt.valueOf. I think that for now having the check that we never instantiate a BigInt directly via the contructor or via a literal covers these cases, but as mentioned, I am also willing to look into seeing how much more difficult working with ts-parser would be.

baileympearson · 2023-02-06T20:03:11Z

etc/eslint/no-bigint-usage/README.md

@@ -0,0 +1,5 @@
+# no-bigint-literals


let's update the title and description here to the new package name

…no bigint

W-A-James · 2023-02-06T20:45:39Z

package.json

@@ -86,10 +87,11 @@
  },
  "scripts": {
    "pretest": "npm run build",
-    "test": "npm run check:node && npm run check:web",
+    "test": "npm run check:node && npm run check:web && npm run check:web-no-bigint",


Only testing for web without bigint here as we expect that Node will always support BigInt. Open to changing this if anyone has other thoughts. (@nbbeeken, @addaleax)

The codepaths are largely the same (we mostly take a different path for Buffer related things). I think this is sufficient. I'm mainly concerned with the core serialization / deserialization / EJSON.stringification / EJSON.parsing, which is mostly the same across Node and web.

…tic methods and constructor to eslint builtin rule

W-A-James · 2023-02-06T22:10:59Z

Summarizing a Slack conversation with @baileympearson:

We do not need a custom lint rule to prevent the usage of the BigInt constructor or its static methods as eslint has a no-restricted-globals rule which disallows any reference to globals provided to it in .eslintrc.json. This would prevent something like

const obj = BigInt;  // Unexpected use of 'BigInt' 	no-restricted-globals
obj.asUint64(64, obj(10));

by reporting an error on the assignment.

It is also not necessary to add linting rules on the instance methods of BigInt as running out test suite with BigInt and associated APIs set to null will flag any instances where we are using BigInt functionality that would negatively affect users in restricted environments. In addition, since we prevent calls to the BigInt constructor and use of bigint literals, we should not encounter the use of instance methods outside of APIs that expect to take bigints as inputs (for example serializing a user-provided object that has a bigint field) and so should not pose a problem.

For sake of completeness, the point about instance methods also applies to the use of DataView's (set|get)BigInt methods. In the case of the former, it is only used in cases where we explicitly handle bigint input and for the latter, our restricted environment testing removes this method from the DataView class and so any unexpected use of it will be caught in CI.

baileympearson · 2023-02-07T00:09:08Z

src/extended_json.ts

@@ -93,6 +93,7 @@ function deserializeValue(value: any, options: EJSONOptions = {}) {
      }
      if (in64BitRange) {
        if (options.useBigInt64) {
+          // eslint-disable-next-line no-restricted-globals


One comment, one question

Let's add a comment explaining why we're disabling the rule here. With the custom bigint plugin, it was obvious, but now that we're relying on no-restricted-globals, it's no longer obvious why the linter is disabled for these lines.

Related to the above - can we configure no-restricted-globals to always require an additional message / description of why we're ignoring the rule? It would be nice if we could enforce that when lint rules are skipped, the justification is explained.

From what I'm reading so far, the only way to do this would be to write a custom rule that checks contents of comments for the eslint-disable and eslint-disable-next-line directives and report when they don't contain what usually is an optional description.

Relevant documentation: https://eslint.org/docs/latest/use/configure/rules#comment-descriptions

baileympearson

small comment

W-A-James added 12 commits January 30, 2023 15:30

chore(NODE-4976): Add eslint plugin

6348586

chore(NODE-4976): update package.json

e2f14db

docs(NODE-4976): Reference article

64ad45f

chore(NODE-4976): move eslint plugin to etc directory

acbd460

test(NODE-4976): bigint tests

b681dd5

test(NODE-4976): Complete tests

ac718cd

chore(NODE-4976): update package-lock.json

61630a7

test(NODE-4976): eslint fixes

d39722f

Merge branch 'main' into NODE-4976/add_tests_to_ensure_bigint_feature…

1e22525

…s_are_unused

test(NODE-4976): Add eslint plugin test

6bf2647

chore(NODE-4976): Update package-lock.json

6460a91

Merge branch 'NODE-4976/add_tests_to_ensure_bigint_features_are_unuse…

ff03e79

…d' of github.com:mongodb/js-bson into NODE-4976/add_tests_to_ensure_bigint_features_are_unused

W-A-James marked this pull request as ready for review February 1, 2023 15:03

baileympearson requested changes Feb 3, 2023

View reviewed changes

W-A-James added 4 commits February 3, 2023 14:51

ci(NODE-4976): New task for no bigint envs

5a26c0e

test(NODE-4976): removed unneeded tests

ae8767c

test(NODE-4976): Fix failing tests

63fe625

chore(NODE-4976): add .nvmrc to .gitignore

30a438d

W-A-James requested a review from baileympearson February 3, 2023 20:44

W-A-James added 6 commits February 6, 2023 09:46

chore(NODE-4976): Update eslint plugin

08e1a8c

chore(NODE-4976): Update eslint plugin and package.json

79739b6

fix(NODE-4976): Add eslint ignore comments

23efdc5

chore(NODE-4976): ci WIP

f49bbd7

chore(NODE-4976): Add eslint plugin ci task

ad973ef

chore(NODE-4976): Update package-lock.json

09a1378

baileympearson requested changes Feb 6, 2023

View reviewed changes

W-A-James added 2 commits February 6, 2023 15:42

fix(NODE-4976): Remove unneeded build script and update plugin docs

6042d80

chore(NODE-4976): Add npm run script to test in web environment with …

119880e

…no bigint

W-A-James commented Feb 6, 2023

View reviewed changes

test(NODE-4976): Update plugin tests

363a2e1

W-A-James added 2 commits February 6, 2023 16:44

chore(NODE-4976): Move responsibility to report on uses of BigInt sta…

600df2b

…tic methods and constructor to eslint builtin rule

chore(NODE-4976): Change eslint-disable comments to use the new rules

7aed548

baileympearson reviewed Feb 7, 2023

View reviewed changes

baileympearson requested changes Feb 7, 2023

View reviewed changes

W-A-James requested a review from baileympearson February 7, 2023 16:51

style(NODE-4976): Add comments to eslint-disable

b310a72

baileympearson approved these changes Feb 7, 2023

View reviewed changes

baileympearson changed the title ~~test(NODE-4976): Add tests to ensure that all use of BigInt is gated by the useBigInt64 flag~~ test(NODE-4976): ensure all use of BigInt is gated by the useBigInt64 flag Feb 7, 2023

baileympearson merged commit c095034 into main Feb 7, 2023

baileympearson deleted the NODE-4976/add_tests_to_ensure_bigint_features_are_unused branch February 7, 2023 18:24

W-A-James restored the NODE-4976/add_tests_to_ensure_bigint_features_are_unused branch February 13, 2023 20:33

W-A-James deleted the NODE-4976/add_tests_to_ensure_bigint_features_are_unused branch February 13, 2023 20:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

test(NODE-4976): ensure all use of BigInt is gated by the useBigInt64 flag #558

test(NODE-4976): ensure all use of BigInt is gated by the useBigInt64 flag #558

Uh oh!

W-A-James commented Jan 30, 2023 •

edited

Loading

Uh oh!

baileympearson left a comment

Uh oh!

baileympearson Feb 2, 2023

Uh oh!

baileympearson Feb 2, 2023

Uh oh!

baileympearson Feb 2, 2023

Uh oh!

baileympearson left a comment

Uh oh!

baileympearson Feb 6, 2023

Uh oh!

baileympearson Feb 6, 2023

Uh oh!

W-A-James Feb 6, 2023 •

edited

Loading

Uh oh!

baileympearson Feb 6, 2023

Uh oh!

W-A-James Feb 6, 2023

Uh oh!

baileympearson Feb 7, 2023

Uh oh!

W-A-James commented Feb 6, 2023

Uh oh!

baileympearson Feb 7, 2023

Uh oh!

W-A-James Feb 7, 2023

Uh oh!

baileympearson left a comment

Uh oh!

Uh oh!

test(NODE-4976): ensure all use of BigInt is gated by the useBigInt64 flag #558

test(NODE-4976): ensure all use of BigInt is gated by the useBigInt64 flag #558

Uh oh!

Conversation

W-A-James commented Jan 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

What is changing?

Is there new documentation needed for these changes?

Double check the following

Uh oh!

baileympearson left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

baileympearson left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

W-A-James Feb 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

W-A-James commented Feb 6, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

baileympearson left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

W-A-James commented Jan 30, 2023 •

edited

Loading

W-A-James Feb 6, 2023 •

edited

Loading