Bug 180731 – Web Inspector: Support for JavaScript BigInt

• Component: Web Inspector
• Status: RESOLVED FIXED

[Joseph Pecoraro]

Support for JavaScript BigInt

JS BigInt type:
<https://tc39.github.io/proposal-bigint>

Needs RemoteObject support (InjectedScriptSource.js and RemoteObject.js) for the new "bigint" type. We should be able to serialize / deserialize it as a string once the BigInt constructor is available.

For this bug, we should be able to distinguish between a regular int and a BigInt in the Console / ObjectPreviews. I'd expect BigInt to always have the `n` suffix to make it clear it is a BigInt. Examples:

  js> 1
  1

  js> 1n
  1n

  js> [1, 1n]
  [1, 1n]

  js> {int: 1, bigint: 1n}
  {int: 1, bigint: 1n}

Other:
* CodeMirror / Esprima support may need to be updated to support BigInt literal syntax.
* TypeProfiler support for the "bigint" type
* I'd expect a BigInt to SyntaxHighlight and display the same color as numbers
* Update Models/NativeFunctionParameters.js for native function parameter info (BigInt, BigInt.prototype, new TypedArray functions)

[Radar WebKit Bug Importer]

<rdar://problem/36298748>

[Joseph Pecoraro]

Created attachment 349493 [details]
[PATCH] WIP

I'm going to wait on BigInt being enabled by default in JavaScriptCore but here is some initial work that gets us most of the way there.

• BigInt RemoteObject support ✔︎
• BigInt TypeProfiler support ✔︎
• BigInt CodeMirror support ✔︎ (via <https://github.com/codemirror/CodeMirror/pull/5411>)
• BigInt Esprima support (not yet implemented upstream, might want to implement it)
• Inspector tests for BigInt (awaiting feature getting enabled by default)

[Devin Rousso]

Comment on attachment 349493 [details]
[PATCH] WIP

View in context: https://bugs.webkit.org/attachment.cgi?id=349493&action=review

> Source/WebInspectorUI/UserInterface/External/CodeMirror/javascript.js:115
> +    } else if (ch == "0" && stream.match(/^(?:x[\da-f]+|o[0-7]+|b[01]+)n?/i)) {

I thought we tried to avoid editing CodeMirror, since any additions we make may get overridden in the future?  Is there a GitHub issue to go along with this?

[Joseph Pecoraro]

(In reply to Devin Rousso from comment #3)
> Comment on attachment 349493 [details]
> [PATCH] WIP
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=349493&action=review
> 
> > Source/WebInspectorUI/UserInterface/External/CodeMirror/javascript.js:115
> > +    } else if (ch == "0" && stream.match(/^(?:x[\da-f]+|o[0-7]+|b[01]+)n?/i)) {
> 
> I thought we tried to avoid editing CodeMirror, since any additions we make
> may get overridden in the future?  Is there a GitHub issue to go along with
> this?

See the previous comment:
BigInt CodeMirror support (via <https://github.com/codemirror/CodeMirror/pull/5411>)

I didn't want to update CodeMirror wholesale, but this is a cherry-picked commit in upstream that applies cleanly to our version.

[Joseph Pecoraro]

Esprima support will be added in:
https://bugs.webkit.org/show_bug.cgi?id=200796

[Joseph Pecoraro]

Created attachment 376476 [details]
[IMAGE] Console - ObjectView with BigInt

[Joseph Pecoraro]

Created attachment 376477 [details]
[IMAGE] Debugger - TypeProfiler with BigInt

[Joseph Pecoraro]

Created attachment 376478 [details]
[IMAGE] Debugger - Popover with BigInt

[Joseph Pecoraro]

Created attachment 376479 [details]
[IMAGE] Timelines - Heap Snapshot with BigInt

[Joseph Pecoraro]

We can enable Web Inspector support for BigInt now to be ready for it if/when it does get enabled.

[Joseph Pecoraro]

Created attachment 376480 [details]
[PATCH] Proposed Fix

[EWS Watchlist]

This patch modifies the inspector protocol. Please ensure that any frontend changes appropriately use feature checks for new protocol features.

[EWS Watchlist]

Attachment 376480 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/JSBigInt.h:89:  The parameter name "vm" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/runtime/JSBigInt.h:205:  The parameter name "vm" adds no information, so it should be removed.  [readability/parameter_name] [5]
ERROR: Source/JavaScriptCore/runtime/JSBigInt.h:206:  The parameter name "vm" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 3 in 31 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Joseph Pecoraro]

Created attachment 376538 [details]
[PATCH] Proposed Fix

[Joseph Pecoraro]

Created attachment 376578 [details]
[PATCH] Proposed Fix - New Color

[Joseph Pecoraro]

Created attachment 376579 [details]
[IMAGE] Light Mode

[Joseph Pecoraro]

Created attachment 376580 [details]
[IMAGE] Dark Mode

[Devin Rousso]

Comment on attachment 376578 [details]
[PATCH] Proposed Fix - New Color

View in context: https://bugs.webkit.org/attachment.cgi?id=376578&action=review

> LayoutTests/inspector/model/remote-object-expected.txt:153
> +  "_value": "<filtered bigint>"

Odd that the filter only happens here, and not for `456n` below 🤔 that seems like a bug

> LayoutTests/inspector/model/resources/remote-object-utilities.js:24
> +            return "<filtered bigint>";

Perhaps instead of "filtering" the bigint, we could `toString()` it?  That way, we still get some identifiable/recognizable data in the output.

> Source/JavaScriptCore/inspector/InjectedScriptSource.js:47
> +    if (typeof obj === "bigint")

Why not use the `isBigInt` you define below?

> Source/JavaScriptCore/inspector/InjectedScriptSource.js:49
>  

Style: extra newline

> Source/JavaScriptCore/inspector/InjectedScriptSource.js:912
> +                delete this.value;

I'd prefer we avoided using `delete`.  Could we split this into the checks above?
```
    // `undefined` and `BigInt` are not JSON serializable.
    if (this.type !== "undefined" && this.type !== "bigint")
        this.value = object;

...

    // Provide user-friendly number values.
    if (this.type === "number" || this.type === "bigint")
        this.description = toStringDescription(object)
```

> Source/JavaScriptCore/inspector/agents/InspectorHeapAgent.cpp:190
> +        JSBigInt* bigInt = asBigInt(cell);
> +        resultString = JSBigInt::tryGetString(vm, bigInt, 10);

I'd just inline this, as it's pretty short.
```
    resultString = JSBigInt::tryGetString(vm, asBigInt(cell), 10);
```

> Source/JavaScriptCore/inspector/protocol/Runtime.json:160
> +                { "name": "isBigInt", "type": "boolean", "description": "Indicates if this type description has been type BigInt." }

I wish all of these were optional :(

> Source/JavaScriptCore/runtime/JSBigInt.cpp:63
>  const ClassInfo JSBigInt::s_info =
> -    { "JSBigInt", nullptr, nullptr, nullptr, CREATE_METHOD_TABLE(JSBigInt) };
> +    { "BigInt", nullptr, nullptr, nullptr, CREATE_METHOD_TABLE(JSBigInt) };

Style: most other files like this one have this on one line.

> Source/JavaScriptCore/runtime/RuntimeType.cpp:83
> +    if (type == TypeSymbol)
> +        return "Symbol"_s;

Yikes!  How long has this been missing?  Actually, is this function even used by anything?

> Source/WebInspectorUI/UserInterface/Images/TypeBigInt.svg:6
> +    <path fill="hsl(192, 50%, 75%)" d="M 13 1 L 3 1 C 1.9 1 1 1.9 1 3 L 1 13 C 1 14.1 1.9 15 3 15 L 13 15 C 14.1 15 15 14.1 15 13 L 15 3 C 15 1.9 14.1 1 13 1 L 13 1 Z"/>
> +    <path fill="hsl(192, 48%, 45%)" d="M 13 1 L 3 1 C 1.9 1 1 1.9 1 3 L 1 13 C 1 14.1 1.9 15 3 15 L 13 15 C 14.1 15 15 14.1 15 13 L 15 3 C 15 1.9 14.1 1 13 1 M 13 2 C 13.552 2 14 2.449 14 3 L 14 13 C 14 13.552 13.552 14 13 14 L 3 14 C 2.449 14 2 13.552 2 13 L 2 3 C 2 2.449 2.449 2 3 2 L 13 2"/>
> +    <path fill="hsl(192, 40%, 50%)" d="M 7.7266 9.8457 L 7.7266 9.5417 C 7.7836 9.568701 7.8366 9.5987 7.8856 9.6337 C 7.9926 9.7117 8.0186 9.7607 8.025599 9.7997 C 7.9596 9.8147 7.8636 9.8317 7.7266 9.8457 M 7.9306 3.0457 L 5.0876 3.0457 L 4.0876 3.0457 L 4.0876 4.0457 L 4.0876 11.999701 L 4.0876 12.999701 L 5.0876 12.999701 L 8.190599 12.999701 C 9.2586 12.999701 10.1216 12.7637 10.7536 12.2987 C 11.4706 11.7637 11.8476 10.9817 11.8476 10.0337 C 11.8476 9.419701 11.6646 8.485701 10.7176 7.7477 C 11.3386 7.1267 11.5086 6.4107 11.5086 5.8137 C 11.5086 4.9037 11.1306 4.1627 10.4156 3.6727 C 9.8006 3.2507 8.987599 3.0457 7.9306 3.0457 M 6.7266 10.8717 L 7.1006 10.8717 C 7.8306 10.8717 8.3356 10.7967 8.6116 10.6477 C 8.8896 10.499701 9.027599 10.228701 9.027599 9.8347 C 9.027599 9.428699 8.841599 9.0927 8.4716 8.8237 C 8.0996 8.554701 7.6316 8.4207 7.0656 8.4207 L 6.7266 8.4207 L 6.7266 10.8717 M 6.7266 7.3567 L 7.1396 7.3567 C 7.6286 7.3567 8.0186 7.2417 8.3056 7.0127 C 8.5956 6.782701 8.7386 6.471701 8.7386 6.077701 C 8.7386 5.7117 8.6226 5.4637 8.3926 5.3337 C 8.1586 5.2027 7.7216 5.1377 7.0776 5.1377 L 6.7266 5.1377 L 6.7266 7.3567 M 7.9306 4.0457 C 8.770599 4.0457 9.4116 4.1967 9.850599 4.4977 C 10.2896 4.7987 10.5086 5.2377 10.5086 5.8137 C 10.5086 6.7707 9.8386 7.447701 8.501599 7.8457 C 10.0656 8.207701 10.8476 8.936701 10.8476 10.0337 C 10.8476 10.6677 10.6186 11.1527 10.1616 11.492701 C 9.7036 11.8297 9.0456 11.999701 8.190599 11.999701 L 5.0876 11.999701 L 5.0876 4.0457 L 7.9306 4.0457"/>

We normally use `rgb` colors in our SVG images.  I'm not against using `hsl` instead (I prefer `hsl`), but we should decide whether to be consistent with `rgb` or switch everything (or just new images) to `hsl`.

> Source/WebInspectorUI/UserInterface/Protocol/RemoteObject.js:71
> +                console.assert(value === undefined);

I'd use `this._value` since you've assigned `value` to it.

> Source/WebInspectorUI/UserInterface/Protocol/RemoteObject.js:72
> +                console.assert(description.endsWith("n"));

Ditto (71) with `this._description`.

> Source/WebInspectorUI/UserInterface/Protocol/RemoteObject.js:73
> +                if (globalThis.BigInt)

Given that Web Inspector is _always_ a web page, perhaps we should be consistent in using `window`?

> Source/WebInspectorUI/UserInterface/Protocol/RemoteObject.js:74
> +                    this._value = globalThis.BigInt(description.substring(0, description.length - 1));

Our normal style is to do a check with `window.XXX` and then just use it "normally" if true (e.g. `new XXX` or `XXX(...)`).

> Source/WebInspectorUI/UserInterface/Protocol/RemoteObject.js:95
> +        console.assert(globalThis.BigInt);

Ditto (73).

> Source/WebInspectorUI/UserInterface/Protocol/RemoteObject.js:98
> +        return new WI.RemoteObject(undefined, undefined, "bigint", undefined, undefined, description, undefined, undefined, undefined);

It's cases like these that make me prefer using an `options = {}` style constructor (>_<)

> Source/WebInspectorUI/UserInterface/Views/CodeMirrorAdditions.js:307
> +

Style: extra newline

> Source/WebInspectorUI/UserInterface/Views/HeapSnapshotInstanceDataGridNode.js:302
> +                if (this._node.className === "BigInt" && globalThis.BigInt) {

Ditto (RemoteObject.js:73).

> Source/WebInspectorUI/UserInterface/Views/HeapSnapshotInstanceDataGridNode.js:306
> +                    let primitiveRemoteObject = WI.RemoteObject.fromPrimitiveValue(string);

If `!window.BigInt`, won't this trigger the `console.assert(value === undefined);` in `WI.RemoteObject`'s constructor?

> Source/WebInspectorUI/UserInterface/Views/Variables.css:106
> +    --syntax-highlight-bigint-color: hsl(192, 100%, 38%);

I personally think this is a bit too bright, and clashes with Symbol too much.  I'd rather have `BigInt` be closer to `Number`, as that's the "closest" type.  I think the "difference" between `String` and `RegExp` is a good example of this.

> Source/WebInspectorUI/UserInterface/Views/Variables.css:286
> +        --syntax-highlight-bigint-color: hsl(195, 100%, 85%);

Ditto (106).

> Source/WebInspectorUI/UserInterface/Views/Variables.css:-291
> -        --syntax-highlight-regex-group-color: var(--text-color-gray-medium);

Nice! The orange looks WAY better =D

[Joseph Pecoraro]

Comment on attachment 376578 [details]
[PATCH] Proposed Fix - New Color

View in context: https://bugs.webkit.org/attachment.cgi?id=376578&action=review

>> LayoutTests/inspector/model/remote-object-expected.txt:153
>> +  "_value": "<filtered bigint>"
> 
> Odd that the filter only happens here, and not for `456n` below 🤔 that seems like a bug

This is the `value`. Previews are always strings.

>> Source/JavaScriptCore/runtime/RuntimeType.cpp:83
>> +        return "Symbol"_s;
> 
> Yikes!  How long has this been missing?  Actually, is this function even used by anything?

Doesn't appear to be used, might only have been for debugging.

>> Source/WebInspectorUI/UserInterface/Protocol/RemoteObject.js:71
>> +                console.assert(value === undefined);
> 
> I'd use `this._value` since you've assigned `value` to it.

I want to be checking the parameters that came in and not a potentially modified member variable.

>> Source/WebInspectorUI/UserInterface/Protocol/RemoteObject.js:73
>> +                if (globalThis.BigInt)
> 
> Given that Web Inspector is _always_ a web page, perhaps we should be consistent in using `window`?

I'd rather use globalThis since this is a feature of a JavaScript environment not a Web environment, but I can switch to window.

>> Source/WebInspectorUI/UserInterface/Views/HeapSnapshotInstanceDataGridNode.js:306
>> +                    let primitiveRemoteObject = WI.RemoteObject.fromPrimitiveValue(string);
> 
> If `!window.BigInt`, won't this trigger the `console.assert(value === undefined);` in `WI.RemoteObject`'s constructor?

Ahh yes, I think I can remove the !BigInt check now that I support that in RemoteObject.

>> Source/WebInspectorUI/UserInterface/Views/Variables.css:106
>> +    --syntax-highlight-bigint-color: hsl(192, 100%, 38%);
> 
> I personally think this is a bit too bright, and clashes with Symbol too much.  I'd rather have `BigInt` be closer to `Number`, as that's the "closest" type.  I think the "difference" between `String` and `RegExp` is a good example of this.

The entire reason for having a separate color is to not be very close to number, since we want to be able to quickly see when BigInt and Number are being used nearby, since that may result in TypeErrors.

>> Source/WebInspectorUI/UserInterface/Views/Variables.css:-291
>> -        --syntax-highlight-regex-group-color: var(--text-color-gray-medium);
> 
> Nice! The orange looks WAY better =D

This was just removed because it was the same value in light mode. This is for regex groups.

[Joseph Pecoraro]

Created attachment 376703 [details]
[PATCH] Proposed Fix

[Devin Rousso]

Comment on attachment 376703 [details]
[PATCH] Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=376703&action=review

r=me, nice work!

> Source/WebInspectorUI/UserInterface/Protocol/RemoteObject.js:76
> +                    this._value = `${description} [BigInt Not Enabled in Web Inspector]`;

Should "Not Enabled" be capitalized?  Wouldn't this always be shown for WK1?  If so, perhaps we should just leave the `_value` as a string.

[Joseph Pecoraro]

> > Source/WebInspectorUI/UserInterface/Protocol/RemoteObject.js:76
> > +                    this._value = `${description} [BigInt Not Enabled in Web Inspector]`;
> 
> Should "Not Enabled" be capitalized?  Wouldn't this always be shown for WK1?
> If so, perhaps we should just leave the `_value` as a string.

It'll be the case in WK1 until we enable BigInt by default. This `value` doesn't really get seen or used anywhere so this isn't a problem. Maybe we don't even create it at all.

I don't believe this hits WK1 tests because it is using the same process as the DumpRenderTree which gets the JSC::Option enabled.

[WebKit Commit Bot]

Comment on attachment 376703 [details]
[PATCH] Proposed Fix

Clearing flags on attachment: 376703

Committed r248898: <https://trac.webkit.org/changeset/248898>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Ryan Haddad]

This caused inspector/timeline/line-column.html to fail on the bots, but it looks like it may just be a rebaseline:

--- /Volumes/Data/slave/highsierra-release-tests-wk2/build/layout-test-results/inspector/timeline/line-column-expected.txt
+++ /Volumes/Data/slave/highsierra-release-tests-wk2/build/layout-test-results/inspector/timeline/line-column-actual.txt
@@ -45,7 +45,7 @@
           "functionName": "",
           "url": "",
           "scriptId": "<filtered>",
-          "lineNumber": 138,
+          "lineNumber": 141,
           "columnNumber": 97
         }
       ],
@@ -107,7 +107,7 @@
               "functionName": "",
               "url": "",
               "scriptId": "<filtered>",
-              "lineNumber": 138,
+              "lineNumber": 141,
               "columnNumber": 97
             }
           ],
@@ -160,7 +160,7 @@
           "functionName": "",
           "url": "",
           "scriptId": "<filtered>",
-          "lineNumber": 138,
+          "lineNumber": 141,
           "columnNumber": 97
         }
       ],
@@ -220,7 +220,7 @@
           "functionName": "",
           "url": "",
           "scriptId": "<filtered>",
-          "lineNumber": 138,
+          "lineNumber": 141,
           "columnNumber": 97
         }
       ],

[Ryan Haddad]

(In reply to Ryan Haddad from comment #25)
> This caused inspector/timeline/line-column.html to fail on the bots, but it
> looks like it may just be a rebaseline:

Rebaselined in https://trac.webkit.org/changeset/248908/webkit

[Joseph Pecoraro]

(In reply to Ryan Haddad from comment #26)
> (In reply to Ryan Haddad from comment #25)
> > This caused inspector/timeline/line-column.html to fail on the bots, but it
> > looks like it may just be a rebaseline:
> 
> Rebaselined in https://trac.webkit.org/changeset/248908/webkit

Thanks!