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: possibly confusing util.inspect() output #15474

Closed
vsemozhetbyt opened this issue Sep 19, 2017 · 10 comments
Closed

util: possibly confusing util.inspect() output #15474

vsemozhetbyt opened this issue Sep 19, 2017 · 10 comments
Labels
util Issues and PRs related to the built-in util module.

Comments

@vsemozhetbyt
Copy link
Contributor

vsemozhetbyt commented Sep 19, 2017

  • Version: 8.5.0
  • Platform: Windows 7 x64
  • Subsystem: util

I am trying to make a simple object autovivification with Proxy:

function autoVivify() {
  return new Proxy({}, {
    get(target, property) {
      if (target[property] === undefined) target[property] = autoVivify();
      return target[property];
    },
  });
}

const obj = autoVivify();

obj.a.b.c = 1;

console.log(obj);
{ a:
   { b: { c: 1, [Symbol(util.inspect.custom)]: [Object] },
     [Symbol(util.inspect.custom)]: { [Symbol(util.inspect.custom)]: [Object] } },
  [Symbol(util.inspect.custom)]: { [Symbol(util.inspect.custom)]: { [Symbol(util.inspect.custom)]: [Object] } } }

I understand that this is an edge case and maybe the output is correct. But I feel somehow that I should report this in case it is not completely intended. Feel free to close if it is by design.

@vsemozhetbyt vsemozhetbyt added the util Issues and PRs related to the built-in util module. label Sep 19, 2017
@vsemozhetbyt
Copy link
Contributor Author

More complicated case:

'use strict';

function autoVivify(obj) {
  return new Proxy(obj, {
    get(target, property) {
      if (target[property] === undefined) target[property] = autoVivify({});
      return target[property];
    },
  });
}

const object = {};
const proxy = autoVivify(object);

proxy.a.b.c = 'I am alive!';

console.log(proxy.a.b.c);
console.log(object.a.b.c);

console.log(object);
console.log(proxy);
I am alive!
I am alive!
{ a:
   { b: { c: 'I am alive!', [Symbol(util.inspect.custom)]: [Object] },
     [Symbol(util.inspect.custom)]: { [Symbol(util.inspect.custom)]: [Object] } } }
{ a:
   { b:
      { c: 'I am alive!',
        [Symbol(util.inspect.custom)]: [Object],
        [Symbol(Symbol.toStringTag)]: [Object] },
     [Symbol(util.inspect.custom)]:
      { [Symbol(util.inspect.custom)]: [Object],
        [Symbol(Symbol.toStringTag)]: [Object] },
     [Symbol(Symbol.toStringTag)]: { [Symbol(util.inspect.custom)]: [Object] } },
  [Symbol(util.inspect.custom)]: { [Symbol(util.inspect.custom)]: { [Symbol(util.inspect.custom)]: [Object] } } }

@BridgeAR
Copy link
Member

I do not think that this is intended. It changed from 7 -> 8 because of #9726. But showing symbols is not the issue here out of my perspective but that the util.inspect.custom symbol is added to the output by default.

I plan on having a look at this.

@refack
Copy link
Contributor

refack commented Sep 20, 2017

IMHO the issue is that [Symbol(util.inspect.custom)] and [Symbol(Symbol.toStringTag)] are accessed without checking whether they are in the subject, so they get autovivified.

const maybeCustomInspect = value[customInspectSymbol] || value.inspect;

P.S. @vsemozhetbyt autovivification with proxies is tricky exactly because of this - frameworks, and language features test for fields on the proxy and trigger vivification. When I use this pattern, I run it once or twice, see which fields are accessed, then filter them out in the proxy 🤷‍♂️

@BridgeAR
Copy link
Member

@refack you are absolutely right. There is little that can be done on utils side to improve the situation. Even though I think it is weird that customInspect is true as default. I think it would be good to change that to false. This was introduced in 2012 in this commit 4eb5399 and I think this should have never landed because having a individual inspect function on the object itself that you are inspecting is somewhat of a weird concept. That is also the reason why the symbol was introduced (a much cleaner approach, even though I personally would have preferred a option with which you can just pass on a function). As it is to late to remove it we might still think about changing the default to false.

@vsemozhetbyt
Copy link
Contributor Author

@refack In addition, objects autovivified with proxies appear not to be serializable (RangeError: Maximum call stack size exceeded from JSON.stringify()). But in this case, they are just a useful occasion to think about custom inspect edges)

@refack
Copy link
Contributor

refack commented Sep 20, 2017

@BridgeAR we could do something less performant, but "safer":

const maybeCustomInspect = customInspectSymbol in value ? value[customInspectSymbol] :
                           'inspect' in value ? value.inspect : null;

@vsemozhetbyt you need to filter out toJSON:

function autoVivify(obj) {
  return new Proxy(obj, {
    get(target, property) {
      if (property == 'toJSON') return;
      if (property == util.inspect.custom) return;
      if (!property in target) target[property] = autoVivify({});
      return target[property];
    },
  });
}

@BridgeAR
Copy link
Member

@refack is that really safer? It moves the check to a different trap and in this case the trap would not be called but I do not yet see a big advantage for this approach.

@refack
Copy link
Contributor

refack commented Sep 20, 2017

@BridgeAR You're right. Actually I now think the inspection should go to a lower level V8 API, and not done in JS.
Both inspection and serialization inevitably will trip a few traps in proxies...

@addaleax
Copy link
Member

I don’t think this is very different from e.g. when [Symbol.toPrimitive]() is called on such a proxy – the results are going to be the same as here, right? In that case, special-casing for util.inspect doesn’t seem to make sense to me.

@BridgeAR
Copy link
Member

Well, after thinking about this further I guess everything is working as expected and also my comment about changing the default is probably not a good idea as it should probably "just work" when ever a user wants a individual inspection instead of having to explicitly activate it.

Therefore I think there is little left to do here and I close the issue. If someone disagrees - please feel free to reopen.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
util Issues and PRs related to the built-in util module.
Projects
None yet
Development

No branches or pull requests

4 participants