From c3ae5149802e31d231c545ed77fad3e7d05fb202 Mon Sep 17 00:00:00 2001
From: LiviaMedeiros <livia@cirno.name>
Date: Mon, 4 Apr 2022 19:07:50 +0800
Subject: [PATCH] fs: make params in writing methods optional

This change allows passing objects as "named parameters":
- `fs.write(fd, buffer[, options], callback)`
- `fs.writeSync(fd, buffer[, options])`
- `filehandle.write(buffer[, options])`

Fixes: https://github.com/nodejs/node/issues/41666

PR-URL: https://github.com/nodejs/node/pull/42601
Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com>
Reviewed-By: Minwoo Jung <nodecorelab@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
---
 doc/api/fs.md                                 |  67 +++++++++++-
 lib/fs.js                                     |  37 +++++--
 lib/internal/fs/promises.js                   |  11 +-
 .../test-fs-promises-write-optional-params.js |  95 ++++++++++++++++
 .../parallel/test-fs-write-optional-params.js | 102 ++++++++++++++++++
 .../test-fs-write-sync-optional-params.js     |  89 +++++++++++++++
 6 files changed, 388 insertions(+), 13 deletions(-)
 create mode 100644 test/parallel/test-fs-promises-write-optional-params.js
 create mode 100644 test/parallel/test-fs-write-optional-params.js
 create mode 100644 test/parallel/test-fs-write-sync-optional-params.js

diff --git a/doc/api/fs.md b/doc/api/fs.md
index 0c9c6a9373d8c1..014d6cbadbea07 100644
--- a/doc/api/fs.md
+++ b/doc/api/fs.md
@@ -608,7 +608,7 @@ added: v10.0.0
 Change the file system timestamps of the object referenced by the {FileHandle}
 then resolves the promise with no arguments upon success.
 
-#### `filehandle.write(buffer[, offset[, length[, position]]])`
+#### `filehandle.write(buffer, offset[, length[, position]])`
 
 <!-- YAML
 added: v10.0.0
@@ -621,7 +621,7 @@ changes:
 
 * `buffer` {Buffer|TypedArray|DataView}
 * `offset` {integer} The start position from within `buffer` where the data
-  to write begins. **Default:** `0`
+  to write begins.
 * `length` {integer} The number of bytes from `buffer` to write. **Default:**
   `buffer.byteLength - offset`
 * `position` {integer|null} The offset from the beginning of the file where the
@@ -646,6 +646,25 @@ On Linux, positional writes do not work when the file is opened in append mode.
 The kernel ignores the position argument and always appends the data to
 the end of the file.
 
+#### `filehandle.write(buffer[, options])`
+
+<!-- YAML
+added: REPLACEME
+-->
+
+* `buffer` {Buffer|TypedArray|DataView}
+* `options` {Object}
+  * `offset` {integer} **Default:** `0`
+  * `length` {integer} **Default:** `buffer.byteLength - offset`
+  * `position` {integer} **Default:** `null`
+* Returns: {Promise}
+
+Write `buffer` to the file.
+
+Similar to the above `filehandle.write` function, this version takes an
+optional `options` object. If no `options` object is specified, it will
+default with the above values.
+
 #### `filehandle.write(string[, position[, encoding]])`
 
 <!-- YAML
@@ -4161,7 +4180,7 @@ This happens when:
 * the file is deleted, followed by a restore
 * the file is renamed and then renamed a second time back to its original name
 
-### `fs.write(fd, buffer[, offset[, length[, position]]], callback)`
+### `fs.write(fd, buffer, offset[, length[, position]], callback)`
 
 <!-- YAML
 added: v0.0.2
@@ -4223,6 +4242,29 @@ On Linux, positional writes don't work when the file is opened in append mode.
 The kernel ignores the position argument and always appends the data to
 the end of the file.
 
+### `fs.write(fd, buffer[, options], callback)`
+
+<!-- YAML
+added: REPLACEME
+-->
+
+* `fd` {integer}
+* `buffer` {Buffer|TypedArray|DataView}
+* `options` {Object}
+  * `offset` {integer} **Default:** `0`
+  * `length` {integer} **Default:** `buffer.byteLength - offset`
+  * `position` {integer} **Default:** `null`
+* `callback` {Function}
+  * `err` {Error}
+  * `bytesWritten` {integer}
+  * `buffer` {Buffer|TypedArray|DataView}
+
+Write `buffer` to the file specified by `fd`.
+
+Similar to the above `fs.write` function, this version takes an
+optional `options` object. If no `options` object is specified, it will
+default with the above values.
+
 ### `fs.write(fd, string[, position[, encoding]], callback)`
 
 <!-- YAML
@@ -5555,7 +5597,7 @@ for more details.
 For detailed information, see the documentation of the asynchronous version of
 this API: [`fs.writeFile()`][].
 
-### `fs.writeSync(fd, buffer[, offset[, length[, position]]])`
+### `fs.writeSync(fd, buffer, offset[, length[, position]])`
 
 <!-- YAML
 added: v0.1.21
@@ -5586,6 +5628,23 @@ changes:
 For detailed information, see the documentation of the asynchronous version of
 this API: [`fs.write(fd, buffer...)`][].
 
+### `fs.writeSync(fd, buffer[, options])`
+
+<!-- YAML
+added: REPLACEME
+-->
+
+* `fd` {integer}
+* `buffer` {Buffer|TypedArray|DataView}
+* `options` {Object}
+  * `offset` {integer} **Default:** `0`
+  * `length` {integer} **Default:** `buffer.byteLength - offset`
+  * `position` {integer} **Default:** `null`
+* Returns: {number} The number of bytes written.
+
+For detailed information, see the documentation of the asynchronous version of
+this API: [`fs.write(fd, buffer...)`][].
+
 ### `fs.writeSync(fd, string[, position[, encoding]])`
 
 <!-- YAML
diff --git a/lib/fs.js b/lib/fs.js
index f8b8e569283bb6..ceb0e189678140 100644
--- a/lib/fs.js
+++ b/lib/fs.js
@@ -630,7 +630,7 @@ function read(fd, buffer, offsetOrOptions, length, position, callback) {
     ({
       offset = 0,
       length = buffer.byteLength - offset,
-      position = null
+      position = null,
     } = params ?? ObjectCreate(null));
   }
 
@@ -701,7 +701,7 @@ function readSync(fd, buffer, offset, length, position) {
     ({
       offset = 0,
       length = buffer.byteLength - offset,
-      position = null
+      position = null,
     } = options);
   }
 
@@ -797,7 +797,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 {number} [offset]
+ * @param {number | object} [offsetOrOptions]
  * @param {number} [length]
  * @param {number | null} [position]
  * @param {(
@@ -807,7 +807,7 @@ function readvSync(fd, buffers, position) {
  *   ) => any} callback
  * @returns {void}
  */
-function write(fd, buffer, offset, length, position, callback) {
+function write(fd, buffer, offsetOrOptions, length, position, callback) {
   function wrapper(err, written) {
     // Retain a reference to buffer so that it can't be GC'ed too soon.
     callback(err, written || 0, buffer);
@@ -815,8 +815,18 @@ function write(fd, buffer, offset, length, position, callback) {
 
   fd = getValidatedFd(fd);
 
+  let offset = offsetOrOptions;
   if (isArrayBufferView(buffer)) {
     callback = maybeCallback(callback || position || length || offset);
+
+    if (typeof offset === 'object') {
+      ({
+        offset = 0,
+        length = buffer.byteLength - offset,
+        position = null,
+      } = offsetOrOptions ?? ObjectCreate(null));
+    }
+
     if (offset == null || typeof offset === 'function') {
       offset = 0;
     } else {
@@ -862,16 +872,27 @@ ObjectDefineProperty(write, internalUtil.customPromisifyArgs,
  * specified `fd` (file descriptor).
  * @param {number} fd
  * @param {Buffer | TypedArray | DataView | string} buffer
- * @param {number} [offset]
- * @param {number} [length]
- * @param {number | null} [position]
+ * @param {{
+ *   offset?: number;
+ *   length?: number;
+ *   position?: number | null;
+ *   }} [offsetOrOptions]
  * @returns {number}
  */
-function writeSync(fd, buffer, offset, length, position) {
+function writeSync(fd, buffer, offsetOrOptions, length, position) {
   fd = getValidatedFd(fd);
   const ctx = {};
   let result;
+
+  let offset = offsetOrOptions;
   if (isArrayBufferView(buffer)) {
+    if (typeof offset === 'object') {
+      ({
+        offset = 0,
+        length = buffer.byteLength - offset,
+        position = null
+      } = offsetOrOptions ?? ObjectCreate(null));
+    }
     if (position === undefined)
       position = null;
     if (offset == null) {
diff --git a/lib/internal/fs/promises.js b/lib/internal/fs/promises.js
index 4c861b17dc2389..7b7377292d55b2 100644
--- a/lib/internal/fs/promises.js
+++ b/lib/internal/fs/promises.js
@@ -517,11 +517,20 @@ async function readv(handle, buffers, position) {
   return { bytesRead, buffers };
 }
 
-async function write(handle, buffer, offset, length, position) {
+async function write(handle, buffer, offsetOrOptions, length, position) {
   if (buffer?.byteLength === 0)
     return { bytesWritten: 0, buffer };
 
+  let offset = offsetOrOptions;
   if (isArrayBufferView(buffer)) {
+    if (typeof offset === 'object') {
+      ({
+        offset = 0,
+        length = buffer.byteLength - offset,
+        position = null
+      } = offsetOrOptions ?? ObjectCreate(null));
+    }
+
     if (offset == null) {
       offset = 0;
     } else {
diff --git a/test/parallel/test-fs-promises-write-optional-params.js b/test/parallel/test-fs-promises-write-optional-params.js
new file mode 100644
index 00000000000000..b130c491541800
--- /dev/null
+++ b/test/parallel/test-fs-promises-write-optional-params.js
@@ -0,0 +1,95 @@
+'use strict';
+
+const common = require('../common');
+
+// This test ensures that filehandle.write accepts "named parameters" object
+// and doesn't interpret objects as strings
+
+const assert = require('assert');
+const fsPromises = require('fs').promises;
+const path = require('path');
+const tmpdir = require('../common/tmpdir');
+
+tmpdir.refresh();
+
+const dest = path.resolve(tmpdir.path, 'tmp.txt');
+const buffer = Buffer.from('zyx');
+
+async function testInvalid(dest, expectedCode, ...params) {
+  let fh;
+  try {
+    fh = await fsPromises.open(dest, 'w+');
+    await assert.rejects(
+      fh.write(...params),
+      { code: expectedCode });
+  } finally {
+    await fh?.close();
+  }
+}
+
+async function testValid(dest, buffer, options) {
+  let fh;
+  try {
+    fh = await fsPromises.open(dest, 'w+');
+    const writeResult = await fh.write(buffer, options);
+    const writeBufCopy = Uint8Array.prototype.slice.call(writeResult.buffer);
+
+    const readResult = await fh.read(buffer, options);
+    const readBufCopy = Uint8Array.prototype.slice.call(readResult.buffer);
+
+    assert.ok(writeResult.bytesWritten >= readResult.bytesRead);
+    if (options.length !== undefined && options.length !== null) {
+      assert.strictEqual(writeResult.bytesWritten, options.length);
+    }
+    if (options.offset === undefined || options.offset === 0) {
+      assert.deepStrictEqual(writeBufCopy, readBufCopy);
+    }
+    assert.deepStrictEqual(writeResult.buffer, readResult.buffer);
+  } finally {
+    await fh?.close();
+  }
+}
+
+(async () => {
+  // Test if first argument is not wrongly interpreted as ArrayBufferView|string
+  for (const badBuffer of [
+    undefined, null, true, 42, 42n, Symbol('42'), NaN, [], () => {},
+    Promise.resolve(new Uint8Array(1)),
+    {},
+    { buffer: 'amNotParam' },
+    { string: 'amNotParam' },
+    { buffer: new Uint8Array(1).buffer },
+    new Date(),
+    new String('notPrimitive'),
+    { toString() { return 'amObject'; } },
+    { [Symbol.toPrimitive]: (hint) => 'amObject' },
+  ]) {
+    await testInvalid(dest, 'ERR_INVALID_ARG_TYPE', badBuffer, {});
+  }
+
+  // First argument (buffer or string) is mandatory
+  await testInvalid(dest, 'ERR_INVALID_ARG_TYPE');
+
+  // Various invalid options
+  await testInvalid(dest, 'ERR_OUT_OF_RANGE', buffer, { length: 5 });
+  await testInvalid(dest, 'ERR_OUT_OF_RANGE', buffer, { offset: 5 });
+  await testInvalid(dest, 'ERR_OUT_OF_RANGE', buffer, { length: 1, offset: 3 });
+  await testInvalid(dest, 'ERR_OUT_OF_RANGE', buffer, { length: -1 });
+  await testInvalid(dest, 'ERR_OUT_OF_RANGE', buffer, { offset: -1 });
+  await testInvalid(dest, 'ERR_INVALID_ARG_TYPE', buffer, { offset: false });
+  await testInvalid(dest, 'ERR_INVALID_ARG_TYPE', buffer, { offset: true });
+
+  // Test compatibility with filehandle.read counterpart
+  for (const options of [
+    {},
+    { length: 1 },
+    { position: 5 },
+    { length: 1, position: 5 },
+    { length: 1, position: -1, offset: 2 },
+    { length: null },
+    { position: null },
+    { offset: 1 },
+  ]) {
+    await testValid(dest, buffer, options);
+  }
+})().then(common.mustCall());
diff --git a/test/parallel/test-fs-write-optional-params.js b/test/parallel/test-fs-write-optional-params.js
new file mode 100644
index 00000000000000..21732114153c0c
--- /dev/null
+++ b/test/parallel/test-fs-write-optional-params.js
@@ -0,0 +1,102 @@
+'use strict';
+
+const common = require('../common');
+
+// This test ensures that fs.write accepts "named parameters" object
+// and doesn't interpret objects as strings
+
+const assert = require('assert');
+const fs = require('fs');
+const path = require('path');
+const tmpdir = require('../common/tmpdir');
+const util = require('util');
+
+tmpdir.refresh();
+
+const destInvalid = path.resolve(tmpdir.path, 'rwopt_invalid');
+const buffer = Buffer.from('zyx');
+
+function testInvalidCb(fd, expectedCode, buffer, options, callback) {
+  assert.throws(
+    () => fs.write(fd, buffer, options, common.mustNotCall()),
+    { code: expectedCode }
+  );
+  callback(0);
+}
+
+function testValidCb(buffer, options, index, callback) {
+  const dest = path.resolve(tmpdir.path, `rwopt_valid_${index}`);
+  fs.open(dest, 'w+', common.mustSucceed((fd) => {
+    fs.write(fd, buffer, options, common.mustSucceed((bytesWritten, bufferWritten) => {
+      const writeBufCopy = Uint8Array.prototype.slice.call(bufferWritten);
+
+      fs.read(fd, buffer, options, common.mustSucceed((bytesRead, bufferRead) => {
+        const readBufCopy = Uint8Array.prototype.slice.call(bufferRead);
+
+        assert.ok(bytesWritten >= bytesRead);
+        if (options.length !== undefined && options.length !== null) {
+          assert.strictEqual(bytesWritten, options.length);
+        }
+        if (options.offset === undefined || options.offset === 0) {
+          assert.deepStrictEqual(writeBufCopy, readBufCopy);
+        }
+        assert.deepStrictEqual(bufferWritten, bufferRead);
+        fs.close(fd, common.mustSucceed(callback));
+      }));
+    }));
+  }));
+}
+
+// Promisify to reduce flakiness
+const testInvalid = util.promisify(testInvalidCb);
+const testValid = util.promisify(testValidCb);
+
+async function runTests(fd) {
+  // Test if first argument is not wrongly interpreted as ArrayBufferView|string
+  for (const badBuffer of [
+    undefined, null, true, 42, 42n, Symbol('42'), NaN, [], () => {},
+    Promise.resolve(new Uint8Array(1)),
+    {},
+    { buffer: 'amNotParam' },
+    { string: 'amNotParam' },
+    { buffer: new Uint8Array(1).buffer },
+    new Date(),
+    new String('notPrimitive'),
+    { [Symbol.toPrimitive]: (hint) => 'amObject' },
+
+    // TODO(LiviaMedeiros): add the following after DEP0162 EOL
+    // { toString() { return 'amObject'; } },
+  ]) {
+    await testInvalid(fd, 'ERR_INVALID_ARG_TYPE', badBuffer, {});
+  }
+
+  // First argument (buffer or string) is mandatory
+  await testInvalid(fd, 'ERR_INVALID_ARG_TYPE', undefined, undefined);
+
+  // Various invalid options
+  await testInvalid(fd, 'ERR_OUT_OF_RANGE', buffer, { length: 5 });
+  await testInvalid(fd, 'ERR_OUT_OF_RANGE', buffer, { offset: 5 });
+  await testInvalid(fd, 'ERR_OUT_OF_RANGE', buffer, { length: 1, offset: 3 });
+  await testInvalid(fd, 'ERR_OUT_OF_RANGE', buffer, { length: -1 });
+  await testInvalid(fd, 'ERR_OUT_OF_RANGE', buffer, { offset: -1 });
+  await testInvalid(fd, 'ERR_INVALID_ARG_TYPE', buffer, { offset: false });
+  await testInvalid(fd, 'ERR_INVALID_ARG_TYPE', buffer, { offset: true });
+
+  // Test compatibility with fs.read counterpart
+  for (const [ index, options ] of [
+    {},
+    { length: 1 },
+    { position: 5 },
+    { length: 1, position: 5 },
+    { length: 1, position: -1, offset: 2 },
+    { length: null },
+    { position: null },
+    { offset: 1 },
+  ].entries()) {
+    await testValid(buffer, options, index);
+  }
+}
+
+fs.open(destInvalid, 'w+', common.mustSucceed(async (fd) => {
+  runTests(fd).then(common.mustCall(() => fs.close(fd, common.mustSucceed())));
+}));
diff --git a/test/parallel/test-fs-write-sync-optional-params.js b/test/parallel/test-fs-write-sync-optional-params.js
new file mode 100644
index 00000000000000..80dfa51e0ba152
--- /dev/null
+++ b/test/parallel/test-fs-write-sync-optional-params.js
@@ -0,0 +1,89 @@
+'use strict';
+
+require('../common');
+
+// This test ensures that fs.writeSync accepts "named parameters" object
+// and doesn't interpret objects as strings
+
+const assert = require('assert');
+const fs = require('fs');
+const path = require('path');
+const tmpdir = require('../common/tmpdir');
+
+tmpdir.refresh();
+
+const dest = path.resolve(tmpdir.path, 'tmp.txt');
+const buffer = Buffer.from('zyx');
+
+function testInvalid(dest, expectedCode, ...bufferAndOptions) {
+  let fd;
+  try {
+    fd = fs.openSync(dest, 'w+');
+    assert.throws(
+      () => fs.writeSync(fd, ...bufferAndOptions),
+      { code: expectedCode });
+  } finally {
+    if (fd != null) fs.closeSync(fd);
+  }
+}
+
+function testValid(dest, buffer, options) {
+  let fd;
+  try {
+    fd = fs.openSync(dest, 'w+');
+    const bytesWritten = fs.writeSync(fd, buffer, options);
+    const bytesRead = fs.readSync(fd, buffer, options);
+
+    assert.ok(bytesWritten >= bytesRead);
+    if (options.length !== undefined && options.length !== null) {
+      assert.strictEqual(bytesWritten, options.length);
+    }
+  } finally {
+    if (fd != null) fs.closeSync(fd);
+  }
+}
+
+{
+  // Test if second argument is not wrongly interpreted as string or options
+  for (const badBuffer of [
+    undefined, null, true, 42, 42n, Symbol('42'), NaN, [], () => {},
+    {},
+    { buffer: 'amNotParam' },
+    { string: 'amNotParam' },
+    { buffer: new Uint8Array(1) },
+    { buffer: new Uint8Array(1).buffer },
+    Promise.resolve(new Uint8Array(1)),
+    new Date(),
+    new String('notPrimitive'),
+    { toString() { return 'amObject'; } },
+    { [Symbol.toPrimitive]: (hint) => 'amObject' },
+  ]) {
+    testInvalid(dest, 'ERR_INVALID_ARG_TYPE', badBuffer);
+  }
+
+  // First argument (buffer or string) is mandatory
+  testInvalid(dest, 'ERR_INVALID_ARG_TYPE');
+
+  // Various invalid options
+  testInvalid(dest, 'ERR_OUT_OF_RANGE', buffer, { length: 5 });
+  testInvalid(dest, 'ERR_OUT_OF_RANGE', buffer, { offset: 5 });
+  testInvalid(dest, 'ERR_OUT_OF_RANGE', buffer, { length: 1, offset: 3 });
+  testInvalid(dest, 'ERR_OUT_OF_RANGE', buffer, { length: -1 });
+  testInvalid(dest, 'ERR_OUT_OF_RANGE', buffer, { offset: -1 });
+  testInvalid(dest, 'ERR_INVALID_ARG_TYPE', buffer, { offset: false });
+  testInvalid(dest, 'ERR_INVALID_ARG_TYPE', buffer, { offset: true });
+
+  // Test compatibility with fs.readSync counterpart with reused options
+  for (const options of [
+    {},
+    { length: 1 },
+    { position: 5 },
+    { length: 1, position: 5 },
+    { length: 1, position: -1, offset: 2 },
+    { length: null },
+    { position: null },
+    { offset: 1 },
+  ]) {
+    testValid(dest, buffer, options);
+  }
+}