Skip to content

[v10.x backport] util: handle null prototype on inspect#23655

Closed
antsmartian wants to merge 1 commit intonodejs:v10.x-stagingfrom
antsmartian:backport-22331-to-v10.x
Closed

[v10.x backport] util: handle null prototype on inspect#23655
antsmartian wants to merge 1 commit intonodejs:v10.x-stagingfrom
antsmartian:backport-22331-to-v10.x

Conversation

@antsmartian
Copy link
Copy Markdown
Contributor

@antsmartian antsmartian commented Oct 14, 2018 •
edited
Loading 

This makes sure the  prototype is always detected properly.

PR-URL: https://github.com/nodejs/node/pull/22331
Fixes: https://github.com/nodejs/node/issues/22141
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: John-David Dalton <[email protected]>

Backport : https://github.com/nodejs/node/pull/22331/files

  • 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

@nodejs-github-bot nodejs-github-bot added util Issues and PRs related to the built-in util module. v10.x labels Oct 14, 2018
@antsmartian antsmartian force-pushed the backport-22331-to-v10.x branch from 92dc0a6 to e983968 Compare October 14, 2018 15:16
@antsmartian
Copy link
Copy Markdown
Contributor Author

antsmartian commented Oct 14, 2018 •
edited
Loading

The build is failing (https://travis-ci.com/nodejs/node/jobs/151670153) because WeakMap and BigUint64Array had different OP on 10.x. We need the following change:

diff --git a/test/parallel/test-util-inspect.js b/test/parallel/test-util-inspect.js
index b74fe00a61..f12b49c073 100644
--- a/test/parallel/test-util-inspect.js
+++ b/test/parallel/test-util-inspect.js
@@ -1678,8 +1678,8 @@ util.inspect(process);
   [new Map([[1, 2]]), '[Map: null prototype] { 1 => 2 }'],
   [new Promise((resolve) => setTimeout(resolve, 10)),
    '[Promise: null prototype] { <pending> }'],
-  [new WeakSet(), '[WeakSet: null prototype] { <items unknown> }'],
-  [new WeakMap(), '[WeakMap: null prototype] { <items unknown> }'],
+  [new WeakSet(), '[WeakSet: null prototype] { [items unknown] }'],
+  [new WeakMap(), '[WeakMap: null prototype] { [items unknown] }'],
   [new Uint8Array(2), '[Uint8Array: null prototype] [ 0, 0 ]'],
   [new Uint16Array(2), '[Uint16Array: null prototype] [ 0, 0 ]'],
   [new Uint32Array(2), '[Uint32Array: null prototype] [ 0, 0 ]'],
@@ -1688,8 +1688,8 @@ util.inspect(process);
   [new Int32Array(2), '[Int32Array: null prototype] [ 0, 0 ]'],
   [new Float32Array(2), '[Float32Array: null prototype] [ 0, 0 ]'],
   [new Float64Array(2), '[Float64Array: null prototype] [ 0, 0 ]'],
-  [new BigInt64Array(2), '[BigInt64Array: null prototype] [ 0n, 0n ]'],
-  [new BigUint64Array(2), '[BigUint64Array: null prototype] [ 0n, 0n ]'],
+  [new BigInt64Array(2), '[BigInt64Array: null prototype] [ 0, 0 ]'],
+  [new BigUint64Array(2), '[BigUint64Array: null prototype] [ 0, 0 ]'],
   [new ArrayBuffer(16), '[ArrayBuffer: null prototype] ' +
    '{ byteLength: undefined }'],
   [new DataView(new ArrayBuffer(16)),

If I add this change, that should go as different commit or to be combined in the same commit? cc @BridgeAR @targos. Thanks!

@targos
Copy link
Copy Markdown
Member

targos commented Oct 14, 2018

You can combine in the same commit. Thanks for doing that!

@antsmartian antsmartian force-pushed the backport-22331-to-v10.x branch from e983968 to 98c716d Compare October 15, 2018 01:21
@antsmartian
Copy link
Copy Markdown
Contributor Author

antsmartian commented Oct 15, 2018

Thanks @targos. I have taken care those changes.

@BridgeAR
Copy link
Copy Markdown
Member

BridgeAR commented Oct 17, 2018

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

@antsmartian
Copy link
Copy Markdown
Contributor Author

antsmartian commented Oct 30, 2018

hmmm, looks like conflict. Will get rebased with master.

@antsmartian
util: handle null prototype on inspect
a39f341 
  This makes sure the  prototype is always detected properly.

  PR-URL: nodejs#22331
  Fixes: nodejs#22141
  Reviewed-By: Ruben Bridgewater <[email protected]>
  Reviewed-By: Matteo Collina <[email protected]>
  Reviewed-By: James M Snell <[email protected]>
  Reviewed-By: John-David Dalton <[email protected]>
@antsmartian antsmartian force-pushed the backport-22331-to-v10.x branch from 98c716d to a39f341 Compare October 30, 2018 15:34
@antsmartian
Copy link
Copy Markdown
Contributor Author

antsmartian commented Oct 30, 2018

@BridgeAR I have now rebased with staging branch, can you have a look at it and merge if all good please?

@BethGriggs
Copy link
Copy Markdown
Member

BethGriggs commented Nov 1, 2018

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

@MylesBorins
Copy link
Copy Markdown
Contributor

MylesBorins commented Nov 4, 2018

rebuild of OSX which failed due to infra: https://ci.nodejs.org/job/node-test-commit-osx/22376/

MylesBorins pushed a commit that referenced this pull request Nov 12, 2018
@antsmartian @MylesBorins
util: handle null prototype on inspect
3df083c 
This makes sure the  prototype is always detected properly.

Backport-PR-URL: #23655
PR-URL: #22331
Fixes: #22141
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: John-David Dalton <[email protected]>
@MylesBorins
Copy link
Copy Markdown
Contributor

MylesBorins commented Nov 12, 2018

landed in 3df083c

@antsmartian
Copy link
Copy Markdown
Contributor Author

antsmartian commented Nov 12, 2018

@MylesBorins Thanks!

rvagg pushed a commit that referenced this pull request Nov 28, 2018
@antsmartian @rvagg
util: handle null prototype on inspect
ef72b01 
This makes sure the  prototype is always detected properly.

Backport-PR-URL: #23655
PR-URL: #22331
Fixes: #22141
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: John-David Dalton <[email protected]>
MylesBorins pushed a commit that referenced this pull request Nov 29, 2018
@antsmartian @MylesBorins
util: handle null prototype on inspect
90014b6 
This makes sure the  prototype is always detected properly.

Backport-PR-URL: #23655
PR-URL: #22331
Fixes: #22141
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: John-David Dalton <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnell jasnell jasnell approved these changes

@BridgeAR BridgeAR BridgeAR approved these changes

Assignees

No one assigned

Labels

util Issues and PRs related to the built-in util module.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@antsmartian @targos @BridgeAR @BethGriggs @MylesBorins @jasnell @nodejs-github-bot