Deprecate object.inspect for custom inspection

doc/api/deprecations.md

@@ -703,7 +703,7 @@ Type: Runtime
 <a id="DEP0079"></a>
 ### DEP0079: Custom inspection function on Objects via .inspect()
 
-Type: Documentation-only
+Type: Runtime
 
 Using a property named `inspect` on an object to specify a custom inspection
 function for [`util.inspect()`][] is deprecated. Use [`util.inspect.custom`][]

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])
 <!-- YAML
 added: v0.8.0
+changes:
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs/node/pull/16393
+    description: Deprecation warnings are only emitted once for each code.
 -->
 
-The `util.deprecate()` method wraps the given `function` or class in such a way that
-it is marked as deprecated.
+* `fn` {Function} The function that is being deprecated.
+* `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.
+* Returns: {Function} The deprecated function wrapped to emit a warning.
+
+The `util.deprecate()` method wraps `fn` (which may be a function or class) in
+such a way that it is marked as deprecated.
 
-<!-- eslint-disable prefer-rest-params -->
 ```js
 const util = require('util');
 
-exports.puts = util.deprecate(function() {
-  for (let i = 0, len = arguments.length; i < len; ++i) {
-    process.stdout.write(arguments[i] + '\n');
-  }
-}, 'util.puts: Use console.log instead');
+exports.obsoleteFunction = util.deprecate(function() {
+  // Do something here.
+}, 'obsoleteFunction() is deprecated. Use newShinyFunction() instead.');
 ```
 
 When called, `util.deprecate()` will return a function that will emit a
-`DeprecationWarning` using the `process.on('warning')` event. By default,
-this warning will be emitted and printed to `stderr` exactly once, the first
-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 the returned function is
+called. After the warning is emitted, the wrapped function is called without
+emitting a warning.
+
+If the same optional `code` is supplied in multiple calls to `util.deprecate()`,
+the warning will be emitted only once for that `code`.
+
+```js
+const util = require('util');
+
+const fn1 = util.deprecate(someFunction, someMessage, 'DEP0001');
+const fn2 = util.deprecate(someOtherFunction, someOtherMessage, 'DEP0001');
+fn1(); // emits a deprecation warning with code DEP0001
+fn2(); // does not emit a deprecation warning because it has the same code
+```
 
 If either the `--no-deprecation` or `--no-warnings` command line flags are
 used, or if the `process.noDeprecation` property is set to `true` *prior* to
@@ -1213,4 +1233,5 @@ Deprecated predecessor of `console.log`.
 [Internationalization]: intl.html
 [WHATWG Encoding Standard]: https://encoding.spec.whatwg.org/
 [constructor]: https://developer.mozilla.org/en/JavaScript/Reference/Global_Objects/Object/constructor
+[list of deprecated APIS]: deprecations.html#deprecations_list_of_deprecated_apis
 [semantically incompatible]: https://github.com/nodejs/node/issues/4179

lib/internal/util.js

@@ -23,6 +23,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 = {};
+
 // Mark that a method should not be used.
 // Returns a modified function which warns once by default.
 // If --no-deprecation is set, then it is a no-op.
@@ -46,7 +50,10 @@ function deprecate(fn, msg, code) {
     if (!warned) {
       warned = true;
       if (code !== undefined) {
-        process.emitWarning(msg, 'DeprecationWarning', code, deprecated);
+        if (!codesWarned[code]) {
+          process.emitWarning(msg, 'DeprecationWarning', code, deprecated);
+          codesWarned[code] = true;
+        }
       } else {
         process.emitWarning(msg, 'DeprecationWarning', deprecated);
       }

lib/util.js

@@ -428,14 +428,23 @@ function formatValue(ctx, value, recurseTimes, ln) {
   // Provide a hook for user-specified inspect functions.
   // Check that value is an object with an inspect function on it
   if (ctx.customInspect) {
-    const maybeCustomInspect = value[customInspectSymbol] || value.inspect;
+    let maybeCustom = value[customInspectSymbol];
+
+    if (!maybeCustom && value.inspect !== exports.inspect &&
+        typeof value.inspect === 'function') {
+      maybeCustom = deprecate(
+        value.inspect,
+        'Custom inspection function on Objects via .inspect() is deprecated',
+        'DEP0079'
+      );
+    }
 
-    if (typeof maybeCustomInspect === 'function' &&
+    if (typeof maybeCustom === 'function' &&
         // Filter out the util module, its inspect function is special
-        maybeCustomInspect !== exports.inspect &&
+        maybeCustom !== exports.inspect &&
         // Also filter out any prototype objects using the circular check.
         !(value.constructor && value.constructor.prototype === value)) {
-      const ret = maybeCustomInspect.call(value, recurseTimes, ctx);
+      const ret = maybeCustom.call(value, recurseTimes, ctx);
 
       // If the custom inspection method returned `this`, don't go into
       // infinite recursion.

test/parallel/test-util-deprecate.js

@@ -0,0 +1,57 @@
+'use strict';
+
+require('../common');
+
+// Tests basic functionality of util.deprecate().
+
+const assert = require('assert');
+const util = require('util');
+
+const expectedWarnings = new Map();
+
+// Emits deprecation only once if same function is called.
+{
+  const msg = 'fhqwhgads';
+  const fn = util.deprecate(() => {}, msg);
+  expectedWarnings.set(msg, { code: undefined, count: 1 });
+  fn();
+  fn();
+}
+
+// Emits deprecation twice for different functions.
+{
+  const msg = 'sterrance';
+  const fn1 = util.deprecate(() => {}, msg);
+  const fn2 = util.deprecate(() => {}, msg);
+  expectedWarnings.set(msg, { code: undefined, count: 2 });
+  fn1();
+  fn2();
+}
+
+// Emits deprecation only once if optional code is the same, even for different
+// functions.
+{
+  const msg = 'cannonmouth';
+  const code = 'deprecatesque';
+  const fn1 = util.deprecate(() => {}, msg, code);
+  const fn2 = util.deprecate(() => {}, msg, code);
+  expectedWarnings.set(msg, { code, count: 1 });
+  fn1();
+  fn2();
+  fn1();
+  fn2();
+}
+
+process.on('warning', (warning) => {
+  assert.strictEqual(warning.name, 'DeprecationWarning');
+  assert.ok(expectedWarnings.has(warning.message));
+  const expected = expectedWarnings.get(warning.message);
+  assert.strictEqual(warning.code, expected.code);
+  expected.count = expected.count - 1;
+  if (expected.count === 0)
+    expectedWarnings.delete(warning.message);
+});
+
+process.on('exit', () => {
+  assert.deepStrictEqual(expectedWarnings, new Map());
+});

test/parallel/test-util-inspect-deprecated.js

@@ -0,0 +1,18 @@
+'use strict';
+const common = require('../common');
+
+// Test that deprecation warning for custom inspection via the `.inspect()`
+// property (on the target object) is emitted once and only once.
+
+const util = require('util');
+
+{
+  const target = { inspect: () => 'Fhqwhgads' };
+  // `common.expectWarning` will expect the warning exactly one time only
+  common.expectWarning(
+    'DeprecationWarning',
+    'Custom inspection function on Objects via .inspect() is deprecated'
+  );
+  util.inspect(target);  // should emit deprecation warning
+  util.inspect(target);  // should not emit deprecation warning
+}

test/parallel/test-util-inspect.js

@@ -26,8 +26,6 @@ const JSStream = process.binding('js_stream').JSStream;
 const util = require('util');
 const vm = require('vm');
 
-/* eslint-disable accessor-pairs */
-
 assert.strictEqual(util.inspect(1), '1');
 assert.strictEqual(util.inspect(false), 'false');
 assert.strictEqual(util.inspect(''), "''");
@@ -280,6 +278,7 @@ assert.strictEqual(
     '{ readwrite: [Getter/Setter] }');
 
   assert.strictEqual(
+    // eslint-disable-next-line accessor-pairs
     util.inspect({ set writeonly(val) {} }),
     '{ writeonly: [Setter] }');
 
@@ -476,7 +475,7 @@ assert.strictEqual(util.inspect(-0), '-0');
     }
   });
   const setter = Object.create(null, {
-    b: {
+    b: { // eslint-disable-line accessor-pairs
       set: function() {}
     }
   });
@@ -1151,4 +1150,9 @@ if (typeof Symbol !== 'undefined') {
 }
 
 assert.doesNotThrow(() => util.inspect(process));
-/* eslint-enable accessor-pairs */
+
+// Setting custom inspect property to a non-function should do nothing.
+{
+  const obj = { inspect: 'fhqwhgads' };
+  assert.strictEqual(util.inspect(obj), "{ inspect: 'fhqwhgads' }");
+}