Improvements to util.inspect #2360

Closed
wants to merge 15 commits into
from

Projects

None yet

6 participants

@Benvie

Use all the colors available, identify constructors, make showHidden more useful by hiding useless function properties, improve quote escaping, color names to identify hidden and readonly properties, more.

@tj
tj commented Dec 18, 2011

if there's and "and" in one commit msg you'll do better splitting it up

@Benvie

It can be two commits indeed, but that's pretty much the limit. The major portion is the change to inspect which is one large component.

@TooTallNate TooTallNate commented on an outdated diff Dec 18, 2011
}
-exports.inspect = inspect;
+exports.is = is;
+
+
+// returns true for strings, numbers, booleans, null, undefined, NaN
+function isPrimitive(o){
+ return Object(o) !== o;
+}
+exports.isPrimitive = isPrimitive;
+
+
+var isUndefined = is('Undefined'),
@TooTallNate
TooTallNate Dec 18, 2011

idk if this one is right:

> Object.prototype.toString.call(undefined)
'[object global]'
@Benvie

So you don't need to compile Node just to see what this does, here's some example output on Windows:

example image

@TooTallNate TooTallNate commented on an outdated diff Dec 18, 2011
+ return Object(o) !== o;
+}
+exports.isPrimitive = isPrimitive;
+
+
+var isUndefined = is('Undefined'),
+ isFunction = is('Function'),
+ isBoolean = is('Boolean'),
+ isString = is('String'),
+ isNumber = is('Number'),
+ isRegExp = is('RegExp'),
+ isObject = is('Object'), // inherits from Object, "plain object"
+ isArray = Array.isArray,
+ isError = is('Error'),
+ isDate = is('Date'),
+ isNull = is('Null');
@TooTallNate
TooTallNate Dec 18, 2011

Same here:

> Object.prototype.toString.call(null)
'[object global]'
@TooTallNate

So some of this is nice. I'm kinda neutral personally. But there's no way this would get merged without some test cases. And I'm guessing that some of the existing tests break with this as well...

@Benvie

The is[type] stiff I'll break out like TJ mentioned. The formatting stuff doesn't really rely on it. It basically just uses Object.prototype.toString and works from there, along with a bit of isPrimitive which is also simple.

@Benvie

And yeah test cases I'll absolutely do. This is a major core change, but I just wanted to float it up here to see if it'd even be considered before putting major effort into refining it further. If it looks like something that'd be considered upon further refinement and whatnot I'll definitely put the effort in to get there. I'm going to split the is[Type] bits out too because they're far more likely to impact actually live code. The inspect stuff is for debugging almost exclusively.

@Benvie

Also just to be super clear, what version of Node are you using with this Object.prototype.toString.call(undefined).

By the spec, and as per v8 >3.7, the result of Object.prototype.toString.call(undefined) is '[object Undefined]'. null for that returns Null, etc. Basically it's incredibly accurate with the versions of v8 I've tested.

Object.prototype.toString.call(this) returns [object global] when testing the global object which looks right to me. window is that in browsers.

@TooTallNate

@Benvie The 0.4.x branch exhibits that behavior, so I guess it doesn't matter too much. Learned something new :)
But still, perhaps more efficient tests for null and undefined are already available to us:

typeoof v === 'undefined'
v === null
@Benvie

Generally as I've seen, the null test happens after primitive tests due to the whole null is an object but not anything else useful flub.

@Benvie

Also to note, with a tiny bit more work it's possible to map stuff to something besides ansi. In that case you plreace the ansi color function with something that wraps html spans instrad of escape sequences: http://benvie.github.com/dom.js/

@Benvie

Modified to only include the inspect related stuff, as that was the primary thing I wanted to improve.

@bnoordhuis
Node.js Foundation member

I like the pretty colors. This won't land in v0.6 but I can see it landing in the master branch. make test should pass and it'll need tests for the new features.

Not sure if I like the resolveGetters option. Getters can have side effects, it could lead to hard to track down bugs. What about recursion? What if you have a getter that (warning: pseudo syntax) returns util.inspect(this, resolveGetters=true)?

@Benvie

You point about getters has clarified my thought process on what I'm trying to do. I'm going to remove that option and also remove any bound references to builtin functions in use for formatting and replace them with dynamic references, like Object.prototype.toString. This matches the existing functionality more closely. After I finish work on this pull I'm going to start a new one to address issues relating ECMAScript6 as well the general issue of dynamic references to modifiable builtin functions.

Here's my current thought process for this current patch, now a bit more clarified thanks to the getter comment.

It's critical to prevent unintended side-effects when doing introspection; the purpose is to observe without modifying. The API provides a hook using the inspect property on an object if a user specifically wants to cause side effects when inspecting. All other side effects should be considered bugs.

As an illustration of this, here's an example. A user created class/prototype where the children property is a getter that does fs.readdirSync and returns an array of new instances each representing a path in the directory. Inspecting an object that represents / would result in a synchronous mapping of the whole drive, only limited by depthLimit.

There's three avenues of triggering unexpected/unwanted side-effects:

  • Accessors. o.prop and o.prop = val. Property access becomes function calls. This is the most important to check for during introspection because accessors are usually made explicitly to cause side effects during property lookup. This can be avoided using Object.getOwnPropertyDescriptor (or the deprecated Object.prototype.__lookupSetter__).

  • Type coercion. A non-primitive value should never touch touch any math or bitwise operators or any comparison operator that isn't === or !==. All of them trigger obj.valueOf or obj.toString or both.

  • Functions on builtin constructors and prototypes. With this category execution is explicit unlike the previous two, but the builtin functionality has known or no side effects (usually none). These functions can be changed and and code run in the same context with uncached references can cause unknown side effects by executing them. Benign non-side effect examples:

    • Shim Array.isArray to return true for Array-like objects.
    • Shim Object.prototype.toString to check for a _class property and return '[object '+this._class+']' if found.

Currently node prevents side-effects directly from getters, but not from using object.__lookup[GS]etter in the process. It prevents side-effects from the second as far as I've found. It does not prevent the third.

This pull fixes the issue with __lookup[GS]etter__. It only partially addressed the third, but I'm going to remove any cached bindings that don't match Node's current functionality. That change should be addressed separately and hit all the problems at once.

@tj
tj commented Dec 22, 2011

you're over-thinking it IMHO, people may replace these things but they shoudn't, and if they do they're asking for it. Like I've mentioned before to others you cannot possible defend against every single thing that could be replaced in javascript, nor should you

@Benvie

It's definitely overthinking things from the perspective of ES5. But the reason I started modifying inspect to begin with was because it's largely broken for ES6 and Proxies. The question of what actions trigger what side-effects is a much, much bigger question. So I'm trying to set this up now in a way that way overthinks it for ES5 but is required for ES6.

@Benvie

I'm not going to add more to this pull, but I wanted to just write it down here for future reference because I'm going to make further pulls regarding those issues.

@Benvie

I think it's looking pretty good now. Amongst other improvements it now passes all existing tests and about doubled the number of tests for util.inspect.

@bnoordhuis
Node.js Foundation member

@Benvie: Is this patch against master or v0.6?

@Benvie

It's against master. Looking at the history there's only one commit difference between master and 0.6 (change date formatting from Date.prototype.toUTCString to Date.prototype.toString which is included in this) so it likely could go in either without issue. It has no external dependencies.

Benvie added some commits Dec 18, 2011
@Benvie Benvie Significant modifications to util.js. Changes includes new type detec…
…tion functions and new inspect formatting.
1402bd5
@Benvie Benvie Revert changes to the various isType functions. Make non-configurable…
… properties always display as constants instead of just capslock ones. Reorganize a bit to more closely match original organization of util.js.
16049ea
@Benvie Benvie Change objectToString back to unbound function. Change quoting to be …
…more friendly to copy/pasting back into javascript. Show prototypes for constructors when using showHidden. Change check for labeling a color as constant when non-writable or getter with no setter.
ff213b9
@Benvie Benvie Change colors to identify hidden constants from enumerable constants,…
… rearrange the type colors a bit to work better.
be86f47
@Benvie Benvie Fix quoting b381db9
@Benvie Benvie Fix so sparse arrays aren't abbreviated. Fix quoting from adding an e…
…xtra slash sometimes. Add special arrows to non-color formatter. Match error formatting from prior version of inspect with brackets and all properties. Match prior version of formatting hidden names with brackets.
8804b36
@Benvie Benvie Add Proxy, WeakMap, Set, and Map to list of known globals so testing …
…with --harmony is possible.
bae19ce
@Benvie Benvie Add tests for util.inspect
* Constructor detection and formatting
* Quote formatting
* Property name quoting
* RegExp formatting
* Accessor formatting
* Circular references
* Recurse limit
48c4b8c
@Benvie Benvie Add tests for util.inspect
* Constructor detection and formatting
* Quote formatting
* Property name quoting
* RegExp formatting
* Accessor formatting
* Circular references
* Recurse limit
cc9881a
@Benvie Benvie Improve constructor detection. Make V8 c++ accessors no always show u…
…p as read only.
9e9ab8c
@Benvie Benvie Break out `isConstructor` into its own function and also export it. 29aaf64
@Benvie Benvie Add in background colors ansi escape sequence list. Get branch up to …
…latest version. Add in a few fixes I've found along the way in the version I'm using outside of node core
4297318
@Benvie Benvie Missed different names in the merge, fixes the build 66fd77b
@Benvie Benvie Fix some merge issues, add in showing of cistom [[protos]], fix spaci…
…ng a bit, set name variable separate from key so the raw key can be checked.
243b640
@Benvie Benvie Fix another issue with merge, looks good now. c0b82b5
@nyxtom

What's the status of this? Are there still outstanding issues with this pull or is it just backlogged?

@isaacs

There are three problems.

  1. It does not apply cleanly against master. It should be rebased.
  2. It does quite a bit more stuff. I'd want to make sure it doesn't cause any performance regressions for standard non-colored uses of console.log and the like.
  3. I'm not really seeing many people begging for this. (Could be just other things have been more important, I don't know).

@TooTallNate You are the committer who's been the most active in util.inspect and console in recent history. I'll leave it up to you. If you think this is a good idea, please re-open, and consider it yours to review/land. I have no strong opinions. There's no urgency, so feel free to re-open and leave it untouched for another year. Just closing it so that if no one cares, it will go uncared-for.

@isaacs isaacs closed this Feb 9, 2013
@TooTallNate

I think the majority of this has been moved into @benvie's "ultra repl".

The functionality in the current core util.inspect() probably won't be changing this drastically.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment