fs: remove coercion to string in writing methods

Moves DEP0162 to End-of-Life. PR-URL: #42796 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
nodejs · Sep 12, 2022 · 950a441 · 950a441
1 parent 047cd61
commit 950a441
Show file tree

Hide file tree

Showing 7 changed files with 51 additions and 91 deletions.
diff --git a/doc/api/deprecations.md b/doc/api/deprecations.md
@@ -3131,6 +3131,9 @@ resources and not the actual references.
 
 <!-- YAML
 changes:
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs/node/pull/42796
+    description: End-of-Life.
   - version: v18.0.0
     pr-url: https://github.com/nodejs/node/pull/42607
     description: Runtime deprecation.
@@ -3141,7 +3144,7 @@ changes:
     description: Documentation-only deprecation.
 -->
 
-Type: Runtime
+Type: End-of-Life
 
 Implicit coercion of objects with own `toString` property, passed as second
 parameter in [`fs.write()`][], [`fs.writeFile()`][], [`fs.appendFile()`][],

diff --git a/doc/api/fs.md b/doc/api/fs.md
@@ -4534,6 +4534,10 @@ default with the above values.
 <!-- YAML
 added: v0.11.5
 changes:
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs/node/pull/42796
+    description: Passing to the `string` parameter an object with an own
+                 `toString` function is no longer supported.
   - version: v17.8.0
     pr-url: https://github.com/nodejs/node/pull/42149
     description: Passing to the `string` parameter an object with an own
@@ -4560,16 +4564,16 @@ changes:
 -->
 
 * `fd` {integer}
-* `string` {string|Object}
+* `string` {string}
 * `position` {integer|null} **Default:** `null`
 * `encoding` {string} **Default:** `'utf8'`
 * `callback` {Function}
   * `err` {Error}
   * `written` {integer}
   * `string` {string}
 
-Write `string` to the file specified by `fd`. If `string` is not a string, or an
-object with an own `toString` function property, then an exception is thrown.
+Write `string` to the file specified by `fd`. If `string` is not a string,
+an exception is thrown.
 
 `position` refers to the offset from the beginning of the file where this data
 should be written. If `typeof position !== 'number'` the data will be written at
@@ -4602,6 +4606,10 @@ details.
 <!-- YAML
 added: v0.1.29
 changes:
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs/node/pull/42796
+    description: Passing to the `string` parameter an object with an own
+                 `toString` function is no longer supported.
   - version: v18.0.0
     pr-url: https://github.com/nodejs/node/pull/41678
     description: Passing an invalid callback to the `callback` argument
@@ -4650,7 +4658,7 @@ changes:
 -->
 
 * `file` {string|Buffer|URL|integer} filename or file descriptor
-* `data` {string|Buffer|TypedArray|DataView|Object}
+* `data` {string|Buffer|TypedArray|DataView}
 * `options` {Object|string}
   * `encoding` {string|null} **Default:** `'utf8'`
   * `mode` {integer} **Default:** `0o666`
@@ -5840,6 +5848,10 @@ this API: [`fs.utimes()`][].
 <!-- YAML
 added: v0.1.29
 changes:
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs/node/pull/42796
+    description: Passing to the `data` parameter an object with an own
+                 `toString` function is no longer supported.
   - version: v17.8.0
     pr-url: https://github.com/nodejs/node/pull/42149
     description: Passing to the `data` parameter an object with an own
@@ -5865,7 +5877,7 @@ changes:
 -->
 
 * `file` {string|Buffer|URL|integer} filename or file descriptor
-* `data` {string|Buffer|TypedArray|DataView|Object}
+* `data` {string|Buffer|TypedArray|DataView}
 * `options` {Object|string}
   * `encoding` {string|null} **Default:** `'utf8'`
   * `mode` {integer} **Default:** `0o666`

diff --git a/lib/fs.js b/lib/fs.js
@@ -39,7 +39,6 @@ const {
   ReflectApply,
   SafeMap,
   SafeSet,
-  String,
   StringPrototypeCharCodeAt,
   StringPrototypeIndexOf,
   StringPrototypeSlice,
@@ -84,7 +83,6 @@ const { FSReqCallback } = binding;
 const { toPathIfFileURL } = require('internal/url');
 const {
   customPromisifyArgs: kCustomPromisifyArgsSymbol,
-  deprecate,
   kEmptyObject,
   promisify: {
     custom: kCustomPromisifiedSymbol,
@@ -123,7 +121,6 @@ const {
   validateRmOptionsSync,
   validateRmdirOptions,
   validateStringAfterArrayBufferView,
-  validatePrimitiveStringAfterArrayBufferView,
   warnOnNonPortableTemplate
 } = require('internal/fs/utils');
 const {
@@ -171,11 +168,6 @@ const isWindows = process.platform === 'win32';
 const isOSX = process.platform === 'darwin';
 
 
-const showStringCoercionDeprecation = deprecate(
-  () => {},
-  'Implicit coercion of objects with own toString property is deprecated.',
-  'DEP0162'
-);
 function showTruncateDeprecation() {
   if (truncateWarn) {
     process.emitWarning(
@@ -813,7 +805,7 @@ function readvSync(fd, buffers, position) {
 /**
  * Writes `buffer` to the specified `fd` (file descriptor).
  * @param {number} fd
- * @param {Buffer | TypedArray | DataView | string | object} buffer
+ * @param {Buffer | TypedArray | DataView | string} buffer
  * @param {number | object} [offsetOrOptions]
  * @param {number} [length]
  * @param {number | null} [position]
@@ -861,9 +853,6 @@ function write(fd, buffer, offsetOrOptions, length, position, callback) {
   }
 
   validateStringAfterArrayBufferView(buffer, 'buffer');
-  if (typeof buffer !== 'string') {
-    showStringCoercionDeprecation();
-  }
 
   if (typeof position !== 'function') {
     if (typeof offset === 'function') {
@@ -875,7 +864,7 @@ function write(fd, buffer, offsetOrOptions, length, position, callback) {
     length = 'utf8';
   }
 
-  const str = String(buffer);
+  const str = buffer;
   validateEncoding(str, length);
   callback = maybeCallback(position);
 
@@ -926,7 +915,7 @@ function writeSync(fd, buffer, offsetOrOptions, length, position) {
     result = binding.writeBuffer(fd, buffer, offset, length, position,
                                  undefined, ctx);
   } else {
-    validatePrimitiveStringAfterArrayBufferView(buffer, 'buffer');
+    validateStringAfterArrayBufferView(buffer, 'buffer');
     validateEncoding(buffer, length);
 
     if (offset === undefined)
@@ -2154,7 +2143,7 @@ function writeAll(fd, isUserFd, buffer, offset, length, signal, callback) {
 /**
  * Asynchronously writes data to the file.
  * @param {string | Buffer | URL | number} path
- * @param {string | Buffer | TypedArray | DataView | object} data
+ * @param {string | Buffer | TypedArray | DataView} data
  * @param {{
  *   encoding?: string | null;
  *   mode?: number;
@@ -2171,10 +2160,7 @@ function writeFile(path, data, options, callback) {
 
   if (!isArrayBufferView(data)) {
     validateStringAfterArrayBufferView(data, 'data');
-    if (typeof data !== 'string') {
-      showStringCoercionDeprecation();
-    }
-    data = Buffer.from(String(data), options.encoding || 'utf8');
+    data = Buffer.from(data, options.encoding || 'utf8');
   }
 
   if (isFd(path)) {
@@ -2201,7 +2187,7 @@ function writeFile(path, data, options, callback) {
 /**
  * Synchronously writes data to the file.
  * @param {string | Buffer | URL | number} path
- * @param {string | Buffer | TypedArray | DataView | object} data
+ * @param {string | Buffer | TypedArray | DataView} data
  * @param {{
  *   encoding?: string | null;
  *   mode?: number;
@@ -2214,10 +2200,7 @@ function writeFileSync(path, data, options) {
 
   if (!isArrayBufferView(data)) {
     validateStringAfterArrayBufferView(data, 'data');
-    if (typeof data !== 'string') {
-      showStringCoercionDeprecation();
-    }
-    data = Buffer.from(String(data), options.encoding || 'utf8');
+    data = Buffer.from(data, options.encoding || 'utf8');
   }
 
   const flag = options.flag || 'w';

diff --git a/lib/internal/fs/promises.js b/lib/internal/fs/promises.js
@@ -66,7 +66,7 @@ const {
   validateOffsetLengthWrite,
   validateRmOptions,
   validateRmdirOptions,
-  validatePrimitiveStringAfterArrayBufferView,
+  validateStringAfterArrayBufferView,
   warnOnNonPortableTemplate,
 } = require('internal/fs/utils');
 const { opendir } = require('internal/fs/dir');
@@ -608,7 +608,7 @@ async function write(handle, buffer, offsetOrOptions, length, position) {
     return { bytesWritten, buffer };
   }
 
-  validatePrimitiveStringAfterArrayBufferView(buffer, 'buffer');
+  validateStringAfterArrayBufferView(buffer, 'buffer');
   validateEncoding(buffer, length);
   const bytesWritten = (await binding.writeString(handle.fd, buffer, offset,
                                                   length, kUsePromises)) || 0;
@@ -838,7 +838,7 @@ async function writeFile(path, data, options) {
   const flag = options.flag || 'w';
 
   if (!isArrayBufferView(data) && !isCustomIterable(data)) {
-    validatePrimitiveStringAfterArrayBufferView(data, 'data');
+    validateStringAfterArrayBufferView(data, 'data');
     data = Buffer.from(data, options.encoding || 'utf8');
   }
 

diff --git a/lib/internal/fs/utils.js b/lib/internal/fs/utils.js
@@ -14,7 +14,6 @@ const {
   MathMin,
   MathRound,
   ObjectIs,
-  ObjectPrototypeHasOwnProperty,
   ObjectSetPrototypeOf,
   ReflectApply,
   ReflectOwnKeys,
@@ -874,27 +873,6 @@ const getValidMode = hideStackFrames((mode, type) => {
 });
 
 const validateStringAfterArrayBufferView = hideStackFrames((buffer, name) => {
-  if (typeof buffer === 'string') {
-    return;
-  }
-
-  if (
-    typeof buffer === 'object' &&
-    buffer !== null &&
-    typeof buffer.toString === 'function' &&
-    ObjectPrototypeHasOwnProperty(buffer, 'toString')
-  ) {
-    return;
-  }
-
-  throw new ERR_INVALID_ARG_TYPE(
-    name,
-    ['string', 'Buffer', 'TypedArray', 'DataView'],
-    buffer
-  );
-});
-
-const validatePrimitiveStringAfterArrayBufferView = hideStackFrames((buffer, name) => {
   if (typeof buffer !== 'string') {
     throw new ERR_INVALID_ARG_TYPE(
       name,
@@ -958,6 +936,5 @@ module.exports = {
   validateRmOptionsSync,
   validateRmdirOptions,
   validateStringAfterArrayBufferView,
-  validatePrimitiveStringAfterArrayBufferView,
   warnOnNonPortableTemplate
 };
diff --git a/test/parallel/test-fs-write-file-sync.js b/test/parallel/test-fs-write-file-sync.js
@@ -102,20 +102,20 @@ tmpdir.refresh();
   assert.strictEqual(content, 'hello world!');
 }
 
-// Test writeFileSync with an object with an own toString function
+// Test writeFileSync with an invalid input
 {
-  // Runtime deprecated by DEP0162
-  common.expectWarning('DeprecationWarning',
-                       'Implicit coercion of objects with own toString property is deprecated.',
-                       'DEP0162');
-  const file = path.join(tmpdir.path, 'testWriteFileSyncStringify.txt');
-  const data = {
-    toString() {
-      return 'hello world!';
-    }
-  };
-
-  fs.writeFileSync(file, data, { encoding: 'utf8', flag: 'a' });
-  const content = fs.readFileSync(file, { encoding: 'utf8' });
-  assert.strictEqual(content, String(data));
+  const file = path.join(tmpdir.path, 'testWriteFileSyncInvalid.txt');
+  for (const data of [
+    false, 5, {}, [], null, undefined, true, 5n, () => {}, Symbol(), new Map(),
+    new String('notPrimitive'),
+    { [Symbol.toPrimitive]: (hint) => 'amObject' },
+    { toString() { return 'amObject'; } },
+    Promise.resolve('amPromise'),
+    common.mustNotCall(),
+  ]) {
+    assert.throws(
+      () => fs.writeFileSync(file, data, { encoding: 'utf8', flag: 'a' }),
+      { code: 'ERR_INVALID_ARG_TYPE' }
+    );
+  }
 }
diff --git a/test/parallel/test-fs-write.js b/test/parallel/test-fs-write.js
@@ -33,7 +33,6 @@ const fn = path.join(tmpdir.path, 'write.txt');
 const fn2 = path.join(tmpdir.path, 'write2.txt');
 const fn3 = path.join(tmpdir.path, 'write3.txt');
 const fn4 = path.join(tmpdir.path, 'write4.txt');
-const fn5 = path.join(tmpdir.path, 'write5.txt');
 const expected = 'ümlaut.';
 const constants = fs.constants;
 
@@ -127,23 +126,6 @@ fs.open(fn3, 'w', 0o644, common.mustSucceed((fd) => {
 }));
 
 
-// Test write with an object with an own toString function
-// Runtime deprecated by DEP0162
-common.expectWarning('DeprecationWarning',
-                     'Implicit coercion of objects with own toString property is deprecated.',
-                     'DEP0162');
-fs.open(fn4, 'w', 0o644, common.mustSucceed((fd) => {
-  const done = common.mustSucceed((written) => {
-    assert.strictEqual(written, Buffer.byteLength(expected));
-    fs.closeSync(fd);
-  });
-
-  const data = {
-    toString() { return expected; }
-  };
-  fs.write(fd, data, done);
-}));
-
 [false, 'test', {}, [], null, undefined].forEach((i) => {
   assert.throws(
     () => fs.write(i, common.mustNotCall()),
@@ -162,9 +144,12 @@ fs.open(fn4, 'w', 0o644, common.mustSucceed((fd) => {
 });
 
 [
-  false, 5, {}, [], null, undefined,
+  false, 5, {}, [], null, undefined, true, 5n, () => {}, Symbol(), new Map(),
   new String('notPrimitive'),
   { [Symbol.toPrimitive]: (hint) => 'amObject' },
+  { toString() { return 'amObject'; } },
+  Promise.resolve('amPromise'),
+  common.mustNotCall(),
 ].forEach((data) => {
   assert.throws(
     () => fs.write(1, data, common.mustNotCall()),
@@ -184,7 +169,7 @@ fs.open(fn4, 'w', 0o644, common.mustSucceed((fd) => {
 
 {
   // Regression test for https://github.com/nodejs/node/issues/38168
-  const fd = fs.openSync(fn5, 'w');
+  const fd = fs.openSync(fn4, 'w');
 
   assert.throws(
     () => fs.writeSync(fd, 'abc', 0, 'hex'),