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.

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

util: Handle null prototype on inspect #22331

Closed
wants to merge 3 commits into from

Conversation

@antsmartian
Copy link
Contributor

antsmartian commented Aug 15, 2018

Fixes : #22141

The implementation and texts are taken from the discussion over here:
#22141 (comment), #22141 (comment)

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
@@ -1487,7 +1487,6 @@ assert.strictEqual(util.inspect('"\'${a}'), "'\"\\'${a}'");
[() => {}, '[Function]'],
[[1, 2], '[ 1, 2 ]'],
[[, , 5, , , , ], '[ <2 empty items>, 5, <3 empty items> ]'],
[{ a: 5 }, '{ a: 5 }'],

This comment has been minimized.

Copy link
@antsmartian

antsmartian Aug 15, 2018

Author Contributor

Had to remove this test case as this we are checking the message for both cases (with prototype and without prototype). So it was failing with this change. May be I need to do the same test case outside this loop?

This comment has been minimized.

Copy link
@BridgeAR

BridgeAR Aug 15, 2018

Member

None of these tests should work as before if we detect a null prototype on each of these data types. So the test has to be rewritten anyway.

@antsmartian antsmartian changed the title util: Handle null prototype on inspecting util: Handle null prototype on inspect Aug 15, 2018
@jasnell jasnell requested a review from BridgeAR Aug 15, 2018
Copy link
Member

BridgeAR left a comment

This is a good start but it should not only check for objects but for any type that we explicitly test for and mark it as having a null prototype.

@@ -1487,7 +1487,6 @@ assert.strictEqual(util.inspect('"\'${a}'), "'\"\\'${a}'");
[() => {}, '[Function]'],
[[1, 2], '[ 1, 2 ]'],
[[, , 5, , , , ], '[ <2 empty items>, 5, <3 empty items> ]'],
[{ a: 5 }, '{ a: 5 }'],

This comment has been minimized.

Copy link
@BridgeAR

BridgeAR Aug 15, 2018

Member

None of these tests should work as before if we detect a null prototype on each of these data types. So the test has to be rewritten anyway.

@TimothyGu

This comment has been minimized.

Copy link
Member

TimothyGu commented Aug 16, 2018

I think this solves the superficial case of #22141 where a prototype is null, but not the general problem of deepEqual that we are not displaying the actual reason why the assertion fails. I am particularly not too great of a fan of special casing null.

My 2¢.

@antsmartian

This comment has been minimized.

Copy link
Contributor Author

antsmartian commented Aug 16, 2018

@TimothyGu Yes, but when using strictEqual (or any other assert) we get the appropriate error message when the assertion fails, for example with this patch we get:

AssertionError [ERR_ASSERTION]: Expected inputs to be strictly equal:
+ actual - expected

+ [Object: null prototype] {}
- {}

I'm not sure, what do you mean by:

but not the general problem of deepEqual that we are not displaying the actual reason why the assertion fails

Am I missing something here?

@devsnek

This comment has been minimized.

Copy link
Member

devsnek commented Aug 16, 2018

@antsman whatever changes you make here should be made to assertion, not inspection, is Timothy's point. I feel the same way.

@antsmartian

This comment has been minimized.

Copy link
Contributor Author

antsmartian commented Aug 16, 2018

@devsnek Ok got it. @BridgeAR thoughts?

@antsmartian

This comment has been minimized.

Copy link
Contributor Author

antsmartian commented Aug 16, 2018

I guess the real problem here is that, when assert fails with null prototype we are not displaying the right error message (no indication that the object had null prototype). Looking at the source code, I see when there is a assert failure, we do call inspect to build up the diff for the error message. If i'm understanding it correctly, then I feel inspect is the right place to handle this, because it doesn't seems to be an assert issue.

May be I'm wrong here too. Open for thoughts.

@BridgeAR

This comment has been minimized.

Copy link
Member

BridgeAR commented Aug 17, 2018

@TimothyGu @devsnek for me this is not about assert but about the general handling of null prototypes. I would like to know if the object of kind xyz has a prototype or not. Currently util.inspect does not distinguish these cases but IMO that should be the case.

> {}
{}
> Object.create(null)
{} // I would like to know that this is a different object than the one above.
@TimothyGu

This comment has been minimized.

Copy link
Member

TimothyGu commented Aug 23, 2018

I would still say that a solution that works for all prototypes rather than special casing null – even in util.inspect() – is the way to go.

@BridgeAR

This comment has been minimized.

Copy link
Member

BridgeAR commented Aug 23, 2018

@TimothyGu that is the plan. Currently all prototypes are handled in inspect besides the null prototype.

@TimothyGu

This comment has been minimized.

Copy link
Member

TimothyGu commented Aug 23, 2018

For reference, Chrome DevTools use a pseudo-__proto__ property for such purposes.

@antsmartian

This comment has been minimized.

Copy link
Contributor Author

antsmartian commented Aug 23, 2018

@BridgeAR @TimothyGu Just an update. I'm working on the same for handling all prototypes. Will update the PR in a day or two.. :)

@antsmartian antsmartian force-pushed the antsmartian:util_proto branch 2 times, most recently from b6fccaa to 7ddca64 Aug 25, 2018
@antsmartian

This comment has been minimized.

Copy link
Contributor Author

antsmartian commented Aug 25, 2018

@BridgeAR I have implemented the check on all the types. Can you please have a look at it?

@antsmartian antsmartian force-pushed the antsmartian:util_proto branch from 7ddca64 to cd98c73 Aug 25, 2018
Copy link
Member

BridgeAR left a comment

This is already looking really good! Thanks a lot for looking into it!

Just a few comments and we should make sure the null prototype is also printed in case a tag exists but no constructor.

[new SharedArrayBuffer(2), 'SharedArrayBuffer { byteLength: undefined }']
'[DataView: null prototype] {\n byteLength: undefined,\n ' +
'byteOffset: undefined,\n buffer: undefined }'],
[new SharedArrayBuffer(2), '[SharedArrayBuffer : null prototype] ' +

This comment has been minimized.

Copy link
@BridgeAR

BridgeAR Aug 25, 2018

Member

Looks like a typo after SharedArrayBuffer?

This comment has been minimized.

Copy link
@antsmartian

antsmartian Aug 25, 2018

Author Contributor

Addressed all other comments, but I couldn't able to see any typo 🤔 Sorry could you please explain me, whats wrong here? cc @BridgeAR

This comment has been minimized.

Copy link
@BridgeAR

BridgeAR Aug 25, 2018

Member

See my comment above :-)

assert.strictEqual(
util.inspect(Object.setPrototypeOf(value, null)),
expected
);

This comment has been minimized.

Copy link
@BridgeAR

BridgeAR Aug 25, 2018

Member

Please add

value.foo = 'bar';
assert.notStrictEqual(util.inspect(value), expected);
delete value.foo;
value[Symbol('foo')] = 'yeah';
assert.notStrictEqual(util.inspect(value), expected);
as well. Otherwise important things would not be tested anymore.

value.foo = 'bar';
assert.notStrictEqual(util.inspect(value), expected);
delete value.foo;
value[Symbol('foo')] = 'yeah';
assert.notStrictEqual(util.inspect(value), expected);
});

[
[new Set([1, 2]), '[Set: null prototype] { 1, 2 }'],

This comment has been minimized.

Copy link
@BridgeAR

BridgeAR Aug 25, 2018

Member

Would you please also add Arrays, Dataviews and ArrayBuffers? Errors, Dates and regular expressions should be handled in a different PR as they do not show subclassing at all and that should be fixed as well.

lib/util.js Outdated
function checkNullPrototype(constructor, type) {
if (constructor === '')
return `[${type}: null prototype]`;
return constructor;

This comment has been minimized.

Copy link
@BridgeAR

BridgeAR Aug 25, 2018

Member

Please inline this check into the getPrefix check. That way the code should be simpler and pretty straight forward.

This comment has been minimized.

Copy link
@antsmartian

antsmartian Aug 25, 2018

Author Contributor

Yeah, thought about it. Will work on that and update the PR.

@antsmartian antsmartian force-pushed the antsmartian:util_proto branch from cd98c73 to 09c796c Aug 25, 2018
lib/util.js Outdated
@@ -752,9 +784,11 @@ function formatValue(ctx, value, recurseTimes) {
// Fast path for ArrayBuffer and SharedArrayBuffer.
// Can't do the same for DataView because it has a non-primitive
// .buffer property that we need to recurse for.
let prefix = getPrefix(constructor, tag);
const arrayType = isArrayBuffer(value) ? 'ArrayBuffer ' :
'SharedArrayBuffer ';

This comment has been minimized.

Copy link
@BridgeAR

BridgeAR Aug 25, 2018

Member

Due to the changes the types have to be 'ArrayBuffer' and 'SharedArrayBuffer' (without the whitespace at the end).

[new SharedArrayBuffer(2), 'SharedArrayBuffer { byteLength: undefined }']
'[DataView: null prototype] {\n byteLength: undefined,\n ' +
'byteOffset: undefined,\n buffer: undefined }'],
[new SharedArrayBuffer(2), '[SharedArrayBuffer : null prototype] ' +

This comment has been minimized.

Copy link
@BridgeAR

BridgeAR Aug 25, 2018

Member

See my comment above :-)

lib/util.js Outdated
if (prefix === '') {
prefix = isArrayBuffer(value) ? 'ArrayBuffer ' : 'SharedArrayBuffer ';
prefix = arrayType;
}

This comment has been minimized.

Copy link
@BridgeAR

BridgeAR Aug 25, 2018

Member

This condition should now be obsolete.

lib/util.js Outdated
newVal = new clazz(value.length || 0);
} else if (isTypedArray(value)) {
const clazz = findTypedConstructor(value) || Uint8Array;
let clazz;
if (Object.getPrototypeOf(value)) {

This comment has been minimized.

Copy link
@BridgeAR

BridgeAR Aug 25, 2018

Member

If the value has a prototype, we do not have to search for it anymore. Therefore it would be best to write this as:

const clazz = Object.getPrototypeOf(value) || findTypedConstructor(value) || clazzWithNullPrototype(Uint8Array, 'Uint8Array')

This comment has been minimized.

Copy link
@antsmartian

antsmartian Aug 26, 2018

Author Contributor

If I understand this correctly. If we write like the following:

const clazz = Object.getPrototypeOf(value) || findTypedConstructor(value) || clazzWithNullPrototype(Uint8Array, 'Uint8Array')

when there is no prototype, then findTypedConstructor will definitely return the typed array constructor. So we won't reach clazzWithNullPrototype (when there is no prototype) -- missing the subclass stuff.

This comment has been minimized.

Copy link
@BridgeAR

BridgeAR Aug 28, 2018

Member

You are correct, findTypedConstructor should always return the actual type.

Therefore it should likely be:

let clazz = Object.getPrototypeOf(value);
if (!clazz) {
  const constructor = findTypedConstructor(value);
  clazz = clazzWithNullProrotype(constructor, constructor.name);
}
lib/util.js Outdated
}
return `[${fallback}: null prototype] `;
}

if (constructor !== '') {

This comment has been minimized.

Copy link
@BridgeAR

BridgeAR Aug 25, 2018

Member

This condition tests for constructor !== '' so instead of testing in another if for the other case, we can just move the code above below this if. In that case we already know, that the constructor is an empty string and don't have to check again.

This comment has been minimized.

Copy link
@antsmartian

antsmartian Aug 26, 2018

Author Contributor

Well this is true, but the line:

return `[${fallback}: null prototype] `;

definitely needs a check whether a constructor is empty or not.

This comment has been minimized.

Copy link
@BridgeAR

BridgeAR Aug 28, 2018

Member

What I mean is:

if (constructor !== '') {
  ... as before ...
}
if (tag !== '' && fallback !== 'tag') {
  return `[${fallback}: null prototype] [${tag}] `;
} 
return `[${fallback}: null prototype] `;

That way we are sure the constructor is definitely empty.

@antsmartian

This comment has been minimized.

Copy link
Contributor Author

antsmartian commented Aug 28, 2018

@BridgeAR Sorry to trouble you, may be you missed my last comments? :)

@antsmartian antsmartian force-pushed the antsmartian:util_proto branch from 09c796c to 9dc530c Aug 28, 2018
@antsmartian

This comment has been minimized.

Copy link
Contributor Author

antsmartian commented Aug 29, 2018

Ping @BridgeAR

Copy link
Member

BridgeAR left a comment

LGTM. Great work :-)

lib/util.js Outdated
@@ -497,20 +497,19 @@ function getConstructorName(obj) {
}

function getPrefix(constructor, tag, fallback) {

This comment has been minimized.

Copy link
@BridgeAR

BridgeAR Aug 29, 2018

Member

Non-blocking nit: adding the line seems obsolete.

lib/util.js Outdated
// Creates a subclass and name
// the constructor as `${clazz} : null prototype`
function clazzWithNullPrototype(clazz, name) {

This comment has been minimized.

Copy link
@BridgeAR

BridgeAR Aug 29, 2018

Member

Non-blocking nit: I would remove the newline here.

'[DataView: null prototype] {\n byteLength: undefined,\n ' +
'byteOffset: undefined,\n buffer: undefined }'],
[new SharedArrayBuffer(2), '[SharedArrayBuffer: null prototype] ' +
'{ byteLength: undefined }']

This comment has been minimized.

Copy link
@BridgeAR

BridgeAR Aug 29, 2018

Member

Nit: this now seems to be a duplicated entry with the test below. Since the test below tests more than this one, it would be best to move this entry (or just keep it).

@BridgeAR

This comment has been minimized.

@antsmartian antsmartian force-pushed the antsmartian:util_proto branch from 9dc530c to 9112c00 Aug 30, 2018
antsmartian and others added 3 commits Sep 8, 2018
@antsmartian antsmartian force-pushed the antsmartian:util_proto branch from 4f52f0f to 215700c Sep 27, 2018
@antsmartian

This comment has been minimized.

Copy link
Contributor Author

antsmartian commented Sep 27, 2018

@BridgeAR I have moved the logic to internal/inspect.js. Please check and see if we can merge this.

@antsmartian

This comment has been minimized.

Copy link
Contributor Author

antsmartian commented Oct 1, 2018

@BridgeAR Anything else blocking this PR from getting merged?

BridgeAR added a commit to BridgeAR/node that referenced this pull request Oct 2, 2018
This makes sure the `null` prototype is always detected properly.

PR-URL: nodejs#22331
Fixes: nodejs#22141
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: John-David Dalton <john.david.dalton@gmail.com>
@BridgeAR

This comment has been minimized.

Copy link
Member

BridgeAR commented Oct 2, 2018

Landed in f4e4ef5 🎉

@antsmartian thanks for sticking to it and being patient!

@BridgeAR BridgeAR closed this Oct 2, 2018
@targos targos added this to Backport requested in v10.x Oct 3, 2018
antsmartian added a commit to antsmartian/node that referenced this pull request Oct 14, 2018
This makes sure the `null` prototype is always detected properly.

PR-URL: nodejs#22331
Fixes: nodejs#22141
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: John-David Dalton <john.david.dalton@gmail.com>
antsmartian added a commit to antsmartian/node that referenced this pull request Oct 14, 2018
    This makes sure the  prototype is always detected properly.

    PR-URL: nodejs#22331
    Fixes: nodejs#22141
    Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
    Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
    Reviewed-By: James M Snell <jasnell@gmail.com>
    Reviewed-By: John-David Dalton <john.david.dalton@gmail.com>
antsmartian added a commit to antsmartian/node that referenced this pull request Oct 14, 2018
    This makes sure the  prototype is always detected properly.

    PR-URL: nodejs#22331
    Fixes: nodejs#22141
    Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
    Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
    Reviewed-By: James M Snell <jasnell@gmail.com>
    Reviewed-By: John-David Dalton <john.david.dalton@gmail.com>
antsmartian added a commit to antsmartian/node that referenced this pull request Oct 14, 2018
  This makes sure the  prototype is always detected properly.

  PR-URL: nodejs#22331
  Fixes: nodejs#22141
  Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
  Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
  Reviewed-By: James M Snell <jasnell@gmail.com>
  Reviewed-By: John-David Dalton <john.david.dalton@gmail.com>
antsmartian added a commit to antsmartian/node that referenced this pull request Oct 15, 2018
  This makes sure the  prototype is always detected properly.

  PR-URL: nodejs#22331
  Fixes: nodejs#22141
  Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
  Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
  Reviewed-By: James M Snell <jasnell@gmail.com>
  Reviewed-By: John-David Dalton <john.david.dalton@gmail.com>
@antsmartian antsmartian deleted the antsmartian:util_proto branch Oct 17, 2018
@antsmartian

This comment has been minimized.

Copy link
Contributor Author

antsmartian commented Oct 19, 2018

@BridgeAR Sorry to ping on the closed thread. If you remember, one of the early comments you mentioned that:

Would you please also add Arrays, Dataviews and ArrayBuffers? Errors, Dates and regular >expressions should be handled in a different PR as they do not show subclassing at all and that >should be fixed as well.

I was looking to implement for Error this morning. Lets say for this example:

class MyError extends Error {}
new MyError()

// want to print:
MyError
  at . . . 

//but currently prints:
Error:
  at

Also as said in the issue, we need to handle null cases as well (but that should be simple given we have getConstructorName and getPrefix in place). I have a made rough implementation:

diff --git a/lib/internal/util/inspect.js b/lib/internal/util/inspect.js
index 4cff372b33..c4e59116f1 100644
--- a/lib/internal/util/inspect.js
+++ b/lib/internal/util/inspect.js
 function formatError(value) {
+
+  // Preserve the sub-classing info
+  // in Error stack
+  function WrapError(error) {
+    const constructor = getConstructorName(value);
+    const prefix = getPrefix(constructor, '', 'Error').trim();
+
+    this.name = prefix;
+    this.message = error.message;
+
+    const stackStart = error.stack && value.stack.indexOf('\n    at');
+    if (error.stack && stackStart !== -1) {
+      let stack = error.stack;
+      stack = stack.slice(stackStart, stack.length);
+      stack = prefix + (this.message ? `: ${this.message}` : '') + stack;
+      this.stack = stack;
+    } else {
+      this.stack = error.stack;
+    }
+
+    return this;
+  }
+
+  // eslint-disable-next-line no-restricted-syntax
+  value = new  WrapError(value);
+
   return value.stack || errorToString(value);
 }

But looks like stack property shouldn't be mutated at all, just in case of message property was muted (test-internal-error.js does have a test for the same). I was wondering, how we can achieve that 🤔 Because when someone calls util.inspect with error object, I'm not sure how we can find if the message property is muted or not.

@BridgeAR

This comment has been minimized.

Copy link
Member

BridgeAR commented Oct 19, 2018

@antsmartian feel free to chat with me on IRC (even though I am not always around).

It is indeed not a good idea to mutate the input at all. Instead, we should be able to just create the output as we expect it to look like and we only change to change a single string: the stack after accessing it.

We also already have the constructor name and don't have to get it again.

So it should be similar to:

function formatError(value, constructorName) {
  if (value.stack) {
    let stack = value.stack;
    if (!stack.startsWith(constructorName)) { // Note: this check is likely not sufficient.
      return stack.replace(..., constructorName);
    }
    return stack;
  }
  return errorToString(value);
}
@antsmartian

This comment has been minimized.

Copy link
Contributor Author

antsmartian commented Oct 19, 2018

@BridgeAR Thanks. (can you just tell me your IRC handle, I searched for bridgear, doesn't look like its correct)

antsmartian added a commit to antsmartian/node that referenced this pull request Oct 30, 2018
  This makes sure the  prototype is always detected properly.

  PR-URL: nodejs#22331
  Fixes: nodejs#22141
  Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
  Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
  Reviewed-By: James M Snell <jasnell@gmail.com>
  Reviewed-By: John-David Dalton <john.david.dalton@gmail.com>
MylesBorins added a commit that referenced this pull request Nov 12, 2018
This makes sure the  prototype is always detected properly.

Backport-PR-URL: #23655
PR-URL: #22331
Fixes: #22141
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: John-David Dalton <john.david.dalton@gmail.com>
@MylesBorins MylesBorins moved this from Backport requested to Backported in v10.x Nov 12, 2018
@codebytere codebytere mentioned this pull request Nov 27, 2018
rvagg added a commit that referenced this pull request Nov 28, 2018
This makes sure the  prototype is always detected properly.

Backport-PR-URL: #23655
PR-URL: #22331
Fixes: #22141
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: John-David Dalton <john.david.dalton@gmail.com>
MylesBorins added a commit that referenced this pull request Nov 29, 2018
This makes sure the  prototype is always detected properly.

Backport-PR-URL: #23655
PR-URL: #22331
Fixes: #22141
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: John-David Dalton <john.david.dalton@gmail.com>
@codebytere codebytere mentioned this pull request Nov 29, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
v10.x
  
Backported
10 participants
You can’t perform that action at this time.