Deprecate object.inspect for custom inspection #16393

Trott · 2017-10-22T21:51:57Z

There are two semver-major changes here:

deprecate object.inspect for custom inspection. See Deprecate object.inspect for custom inspection #15549.
change util.deprecate() so that if the same deprecation code is used on multiple calls to util.deprecate(), it only prints the deprecation warning once. This makes it so that every call to util.inspect() doesn't print a new deprecation message (since it will re-create the custom inspect function each time). This may not technically be semver-major, but since the deprecation is semver-major and it kinda needs this, might as well be safe and bundle them.

Refs: #15549

Checklist

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

Affected core subsystem(s)

util

addaleax · 2017-10-22T21:57:13Z

lib/util.js

+        'Custom inspection function on Objects via .inspect() is deprecated',
+        'DEP0079'
+      );
+    }


I don’t think we’d want to generate a deprecated function every time value.inspect is present. I’d prefer at least checking value[customInspectSymbol] first, before going through this trouble. (And if you want to go further, maybe a WeakMap to keep the generated closures cached for performance – but that might not be that important for deprecated features…)

It should also only trigger in case it is a function. Right now it would always trigger.

@addaleax OK, took care of the "it will generate a deprecated function every time" issue.

For simplicity, I'd fire the warning here, and not delegate it to the actual call, or just set a flag and fire next to the call. That saves the closure creation.

@BridgeAR Yeah, definitely should have a typeof check there. Will add that in. Thanks!

typeof check added along with test that exercises the additional code branch.

refack · 2017-10-22T22:09:05Z

I don’t think we’d want to generate a deprecated function every time value.inspect is present.

That was my thought in #16266.
This approach also puts the onus on the user of util.inspect while the deprecated code would probably come from a 3rd party library, and the end user has no way around it.

Trott · 2017-10-22T22:23:37Z

This approach also puts the onus on the user of util.inspect while the deprecated code would probably come from a 3rd party library, and the end user has no way around it.

Kind of true for nearly any runtime deprecation we implement, isn't it? (And a good reason not to accept them lightly.)

vsemozhetbyt · 2017-10-22T22:38:13Z

doc/api/util.md

+* `msg` {string} A warning message to display when the deprecated function is
+  invoked.
+* `code` {string} A deprecation code. See the [list of deprecated APIs][] for a 
+  list of codes. deprecations.html#deprecations_list_of_deprecated_apis


Seems like a typo)

@vsemozhetbyt Whoops, yes. will fix. Thanks!

@vsemozhetbyt typo fixed!

BridgeAR · 2017-10-22T22:37:42Z

lib/internal/util.js

+        if (!codesWarned[code]) {
+          process.emitWarning(msg, 'DeprecationWarning', code, deprecated);
+          codesWarned[code] = true;
+        }


This should be obsolete due to the warned variable. It should actually do exactly this.

@BridgeAR No, warned doesn't take care of this situation. warned is scoped to the function so this will emit twice currently:

{ const fn1 = util.deprecate(() => { ... }, 'foo', 'DEP0001'); const fn2 = util.deprecate(() => { ... }, 'foo', 'DEP0001'); fn1(); fn2();

That is correct but normally the deprecation function is only called once with a message. We could remove the warned variable though as it got obsolete with codesWarned.

@BridgeAR I thought about removing warned but it's still used if code is not provided.

BridgeAR · 2017-10-22T22:37:48Z

lib/util.js

+        'Custom inspection function on Objects via .inspect() is deprecated',
+        'DEP0079'
+      );
+    }


It should also only trigger in case it is a function. Right now it would always trigger.

refack · 2017-10-22T22:42:27Z

Kind of true for nearly any runtime deprecation we implement, isn't it? (And a good reason not to accept them lightly.)

This is a little more complex since there's nothing wrong with the call to util.inspect it's the object you're inspecting which is the "offender" 🤔. Yes tricky tricky.

vsemozhetbyt · 2017-10-22T22:43:42Z

doc/api/util.md

-time it is called. After the warning is emitted, the wrapped `function`
-is called.
+`DeprecationWarning` using the `process.on('warning')` event. The warning will
+be emitted and printed to `stderr` the first time it is called. After the


Is the 'it' here a bit confusing?

Is the 'it' here a bit confusing?

@vsemozhetbyt Yes. I'll reword it.

OK, clarified now, I hope.

Trott · 2017-10-23T00:47:43Z

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

Trott · 2017-10-23T04:26:24Z

CI comments:

AIX failure is build related and unrelated to this change. Green on re-run at https://ci.nodejs.org/job/node-test-commit-aix/9580/
Lint error: Oh wow, that's embarrassing. Fixed.

Trott · 2017-10-23T04:27:11Z

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

Trott · 2017-10-23T04:38:26Z

Squashedi into four logical commits.

small refactor to eslint comments in test-util-inspect in preparation for changes.
improve doc of existing util.deprecate()
change util.deprecate() to emit the deprecation warning only once per deprecation code
runtime deprecate custom .inspect property on objects for util.inspect()

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

gireeshpunathil · 2017-10-23T04:40:26Z

AIX build failure:
ld:fork(): Resource temporarily unavailable
I don't remember hearing about this before, so looks spurious. Will pass it this time and investigate if this repeats.

Trott · 2017-10-23T04:42:02Z

AIX build failure:
ld:fork(): Resource temporarily unavailable
I don't remember hearing about this before, so looks spurious. Will pass it this time and investigate if this repeats.

@gireeshpunathil Already investigated and fixed. A bunch of stalled citgm processes. See nodejs/build#900 (comment).

Trott · 2017-10-23T04:42:55Z

@gireeshpunathil Oh whoops, sorry, clearly not the same thing! Yeah, thanks! (EDIT: Well, the fork() thing is what's discussed and worked around in that link above. But there's a different error on AIX in that last CI run that is not what's in that link above as far as I can tell.)

Trott · 2017-10-23T04:43:53Z

Retrying on AIX: https://ci.nodejs.org/job/node-test-commit-aix/9585/

Trott · 2017-10-23T04:45:40Z

@gireeshpunathil OK, I see the source of my confusion. The fork() thing you quoted is the problem I linked to. But there's that AIX run that doesn't have that but has this instead:

Removing tools/gyp/pylib/gyp/input.pyc
Removing tools/gyp/pylib/gyp/simple_copy.pyc
Removing tools/gyp/pylib/gyp/xcode_emulation.pyc
Removing tools/gyp_node.pyc

stderr: 
	at org.jenkinsci.plugins.gitclient.CliGitAPIImpl.launchCommandIn(CliGitAPIImpl.java:1924)
	at org.jenkinsci.plugins.gitclient.CliGitAPIImpl.launchCommandIn(CliGitAPIImpl.java:1892)
	at org.jenkinsci.plugins.gitclient.CliGitAPIImpl.launchCommandIn(CliGitAPIImpl.java:1888)
	at org.jenkinsci.plugins.gitclient.CliGitAPIImpl.launchCommand(CliGitAPIImpl.java:1533)
	at org.jenkinsci.plugins.gitclient.CliGitAPIImpl.launchCommand(CliGitAPIImpl.java:1545)
	at org.jenkinsci.plugins.gitclient.CliGitAPIImpl.clean(CliGitAPIImpl.java:732)
	at hudson.plugins.git.GitAPI.clean(GitAPI.java:311)
	at sun.reflect.GeneratedMethodAccessor61.invoke(Unknown Source)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:55)
	at java.lang.reflect.Method.invoke(Method.java:508)
	at hudson.remoting.RemoteInvocationHandler$RPCRequest.perform(RemoteInvocationHandler.java:884)
	at hudson.remoting.RemoteInvocationHandler$RPCRequest.call(RemoteInvocationHandler.java:859)
	at hudson.remoting.RemoteInvocationHandler$RPCRequest.call(RemoteInvocationHandler.java:818)
	at hudson.remoting.UserRequest.perform(UserRequest.java:152)
	at hudson.remoting.UserRequest.perform(UserRequest.java:50)
	at hudson.remoting.Request$2.run(Request.java:332)
	at hudson.remoting.InterceptingExecutorService$1.call(InterceptingExecutorService.java:68)
	at java.util.concurrent.FutureTask.run(FutureTask.java:277)
	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1160)
	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:635)
	at hudson.remoting.Engine$1$1.run(Engine.java:85)
	at java.lang.Thread.run(Thread.java:785)
	at ......remote call to Channel to /140.211.9.100(Native Method)
	at hudson.remoting.Channel.attachCallSiteStackTrace(Channel.java:1654)
	at hudson.remoting.UserResponse.retrieve(UserRequest.java:311)
	at hudson.remoting.Channel.call(Channel.java:905)
	at hudson.remoting.RemoteInvocationHandler.invoke(RemoteInvocationHandler.java:257)
	at com.sun.proxy.$Proxy77.clean(Unknown Source)
	at org.jenkinsci.plugins.gitclient.RemoteGitImpl.clean(RemoteGitImpl.java:450)
	at hudson.plugins.git.extensions.impl.CleanCheckout.onCheckoutCompleted(CleanCheckout.java:30)
	at hudson.plugins.git.GitSCM.checkout(GitSCM.java:1193)
	at hudson.scm.SCM.checkout(SCM.java:495)
	at hudson.model.AbstractProject.checkout(AbstractProject.java:1212)
	at hudson.model.AbstractBuild$AbstractBuildExecution.defaultCheckout(AbstractBuild.java:566)
	at jenkins.scm.SCMCheckoutStrategy.checkout(SCMCheckoutStrategy.java:86)
	at hudson.model.AbstractBuild$AbstractBuildExecution.run(AbstractBuild.java:491)
	at hudson.model.Run.execute(Run.java:1737)
	at hudson.matrix.MatrixRun.run(MatrixRun.java:146)
	at hudson.model.ResourceController.execute(ResourceController.java:97)
	at hudson.model.Executor.run(Executor.java:421)
Run condition [Always] enabling perform for step [[]]

gireeshpunathil · 2017-10-23T04:58:35Z

@Trott - thanks. Probably all are root caused by one issue (stale processes in the system from previous runs), and manifest differently based on which code part suffers (g++/java).

jasnell · 2017-10-23T16:51:29Z

doc/api/util.md

@@ -107,30 +107,50 @@ environment variable set, then it will not print anything.
 Multiple comma-separated `section` names may be specified in the `NODE_DEBUG`
 environment variable. For example: `NODE_DEBUG=fs,net,tls`.

-## util.deprecate(function, string)
+## util.deprecate(fn, msg[, code])


nit: s/fn/function, s/msg/message

Not looking for a debate here, but do want to make sure we have broad agreement and perhaps something that needs an entry in the style guide or something:

If I'm not mistaken, we try to use the same argument name in the docs as we use for the variables in the source code.

I believe there are two reasons, but correction welcome. First, it makes the reading of source code along with docs easier. Second, it allows the mapping of documentation variable names to variable names in stack traces possible.

If I'm right about that, then we can not use function as it is a keyword and not a valid variable name. We use fn and msg elsewhere in the docs. I'm inclined to leave them. They are pretty self-explanatory, and get an explanation immediately below anyway.

(More here: #20489.)

jasnell · 2017-10-23T16:52:28Z

lib/internal/util.js

@@ -18,6 +18,10 @@ function objectToString(o) {
  return Object.prototype.toString.call(o);
 }

+// Keep a list of deprecation codes that have been warned on so we only warn on
+// each one once.
+const codesWarned = {};


ooo nice, this crosses a long standing todo off my list, thank you!

Trott · 2017-10-30T05:34:38Z

This is semver-major so needs one more @nodejs/tsc to land. Ping TSC...

mcollina

LGTM

cjihrig · 2017-10-30T14:31:17Z

lib/util.js

-    const maybeCustomInspect = value[customInspectSymbol] || value.inspect;
+    let maybeCustom = value[customInspectSymbol];
+
+    if (!maybeCustom && value.inspect && value.inspect !== exports.inspect &&


Isn't the && value.inspect part unnecessary? The two remaining checks would cover that case, and I'd think would be faster.

@cjihrig Indeed, that appears to be the case. I've removed it. Thanks.

In test-util-inspect, apply ESLint exception for accessor-pairs rule narrowly. It had been applied to nearly the whole file, but is only needed for two lines.

Improve documentation for `util.deprecate()`. In particular, provide complete function signature, document arguments, and document return value.

If another function has already emitted the deprecation warning with the same code as the warning that is about to be emitted, do not emit the warning.

Change documentation-only deprecation for custom inspection using `object.inspect` property to a runtime deprecation. Refs: nodejs#15549

Trott · 2017-11-12T00:43:06Z

Latest changes CI: https://ci.nodejs.org/job/node-test-pull-request/11364/

jasnell · 2017-11-12T01:36:37Z

Still LGTM

In test-util-inspect, apply ESLint exception for accessor-pairs rule narrowly. It had been applied to nearly the whole file, but is only needed for two lines. PR-URL: nodejs#16393 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

Improve documentation for `util.deprecate()`. In particular, provide complete function signature, document arguments, and document return value. PR-URL: nodejs#16393 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

If another function has already emitted the deprecation warning with the same code as the warning that is about to be emitted, do not emit the warning. This is a breaking change. Previously, different functions could emit the same deprecation warning multiple times. This was a known bug rather than a feature, but this change is being treated as a breaking change out of caution. Identical deprecation warnings should not be emitted. PR-URL: nodejs#16393 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

Change documentation-only deprecation for custom inspection using `object.inspect` property to a runtime deprecation. This is a breaking change. Custom inspection via `object.inspect` is deprecated because there is a more robust Symbol-based alternative to `.inspect` and the custom inspection via `object.inspect` feature means that people can accidentally break `console.log()` simply by attaching a `.inspect` property to their objects. Note that since this is a deprecation, the custom inspection will still work. The breaking change is simply the printing of a warning which could alarm users, break tests or other things that might be dependent on specific output, etc. PR-URL: nodejs#16393 Ref: nodejs#15549 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

Trott · 2017-11-17T06:03:18Z

Landed in 60698c2, ccc87eb, 07d39a2, and 617e3e9.

Trott · 2017-11-17T06:10:50Z

Argh, I probably should have run CITGM. Sorry about that. Here we go: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1078/

Trott · 2017-11-17T07:25:54Z

CITGM looks OK to me, but I sure wouldn't mind a more CITGM-experienced pair of eyes looking at it. Basically, I looked at what packages failed, and compared that to the failures in the previous several runs. If I didn't see mine adding anything to the list, I shrugged it off. Where I did see mine adding a package or two, I checked the errors and it was usually something unrelated like an install failure.

Trott · 2017-11-17T07:26:14Z

@nodejs/citgm ^^^^^

Trott added notable-change PRs with changes that should be highlighted in changelogs. semver-major PRs that contain breaking changes and should be released in the next major version. util Issues and PRs related to the built-in util module. labels Oct 22, 2017

Trott mentioned this pull request Oct 22, 2017

Deprecate object.inspect for custom inspection #15549

Closed

addaleax reviewed Oct 22, 2017

View reviewed changes

Trott force-pushed the runtime-deprecation-custom-inspect branch from c028092 to f5ced4f Compare October 22, 2017 22:03

refack mentioned this pull request Oct 22, 2017

util: allow to disable only custom inspect "named" method #16266

Closed

4 tasks

vsemozhetbyt reviewed Oct 22, 2017

View reviewed changes

BridgeAR reviewed Oct 22, 2017

View reviewed changes

Trott force-pushed the runtime-deprecation-custom-inspect branch from f5ced4f to 74f9c7c Compare October 22, 2017 22:39

vsemozhetbyt reviewed Oct 22, 2017

View reviewed changes

Trott force-pushed the runtime-deprecation-custom-inspect branch from 77e02ee to 61e875a Compare October 23, 2017 04:35

jasnell reviewed Oct 23, 2017

View reviewed changes

jasnell approved these changes Oct 23, 2017

View reviewed changes

Trott modified the milestone: 9.0.0 Oct 30, 2017

mcollina approved these changes Oct 30, 2017

View reviewed changes

cjihrig approved these changes Oct 30, 2017

View reviewed changes

Trott added 3 commits November 11, 2017 15:06

test: apply eslint exceptions narrowly

0b09032

In test-util-inspect, apply ESLint exception for accessor-pairs rule narrowly. It had been applied to nearly the whole file, but is only needed for two lines.

doc: improve documentation for util.deprecate()

77038aa

Improve documentation for `util.deprecate()`. In particular, provide complete function signature, document arguments, and document return value.

util: emit deprecation code only once

04ffd99

If another function has already emitted the deprecation warning with the same code as the warning that is about to be emitted, do not emit the warning.

Trott force-pushed the runtime-deprecation-custom-inspect branch 2 times, most recently from 2907af3 to 3916830 Compare November 11, 2017 23:45

util: runtime deprecation for custom .inspect()

90cccae

Change documentation-only deprecation for custom inspection using `object.inspect` property to a runtime deprecation. Refs: nodejs#15549

Trott force-pushed the runtime-deprecation-custom-inspect branch from 3916830 to 90cccae Compare November 12, 2017 00:34

Trott closed this Nov 17, 2017

chentsulin mentioned this pull request Sep 16, 2018

use util.inspect.custom instead of Object.inspect bottenderjs/messaging-apis#393

Merged

Trott deleted the runtime-deprecation-custom-inspect branch January 13, 2022 22:47

Deprecate object.inspect for custom inspection #16393

Deprecate object.inspect for custom inspection #16393

Conversation

Trott commented Oct 22, 2017

Checklist

Affected core subsystem(s)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Trott Oct 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Trott Oct 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

refack commented Oct 22, 2017

Trott commented Oct 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Trott Oct 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

refack commented Oct 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Trott commented Oct 23, 2017

Trott commented Oct 23, 2017

Trott commented Oct 23, 2017

Trott commented Oct 23, 2017

gireeshpunathil commented Oct 23, 2017

Trott commented Oct 23, 2017 • edited Loading

Trott commented Oct 23, 2017 • edited Loading

Trott commented Oct 23, 2017

Trott commented Oct 23, 2017

gireeshpunathil commented Oct 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Trott commented Oct 30, 2017

mcollina left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Trott commented Nov 12, 2017

jasnell commented Nov 12, 2017

Trott commented Nov 17, 2017

Trott commented Nov 17, 2017

Trott commented Nov 17, 2017

Trott commented Nov 17, 2017

Trott Oct 22, 2017 •

edited

Loading

Trott Oct 22, 2017 •

edited

Loading

Trott Oct 22, 2017 •

edited

Loading

Trott commented Oct 23, 2017 •

edited

Loading

Trott commented Oct 23, 2017 •

edited

Loading