fs: .write(options) support

[LiviaMedeiros]

What is the problem this feature will solve?

In fs subsystem, there are forms of .read methods, accepting Object as argument: filehandle.read, fs.read, fs.readSync.
They allow to skip optional offset and/or length, and to use convenient reusable objects, e.g. const webpSize = {buffer: new Uint32Array(1), position: 0x4}
However, .write counterparts don't support that. So instead of using "named arguments" and reusing the same objects (e.g. await fh.write(webpSize); after modifying buffer contents), we have to write a wrapper function or something like:

await fh.write(webpSize.buffer, undefined, undefined, webpSize.position);
// or emulate single-passable-variable:
const _webpSizeA = [webpSize.buffer, ...[,,], webpSize.position];
await fh.write(..._webpSizeA);

...which doesn't feel right.

What is the feature you are proposing to solve the problem?

Adding:
filehandle.write(options) to Promises API (FileHandle class)
fs.write(fd, options, callback) to Callback API
fs.writeSync(fd, options) to Synchronous API
Skipping options would do the same as passing {} which means buffer === undefined, so making them optional is redundant.
Doing nothing or "writing 0 bytes" by default contradicts with async reading functions: they create 16KB buffer.

Rough patch for Promises API:

diff --git a/lib/internal/fs/promises.js b/lib/internal/fs/promises.js
index 6a0ce4d05c6b..8d534b0256bd 100644
--- a/lib/internal/fs/promises.js
+++ b/lib/internal/fs/promises.js
@@ -562,7 +562,18 @@ async function readv(handle, buffers, position) {
   return { bytesRead, buffers };
 }
 
-async function write(handle, buffer, offset, length, position) {
+async function write(handle, bufferOrOptions, offset, length, position) {
+  let buffer = bufferOrOptions;
+  if (!isArrayBufferView(buffer)) {
+    validateBuffer(bufferOrOptions?.buffer);
+    ({
+      buffer,
+      offset = 0,
+      length = buffer.byteLength,
+      position = null
+    } = bufferOrOptions);
+  }
+
   if (buffer?.byteLength === 0)
     return { bytesWritten: 0, buffer };

Of course, this will break current code, because if buffer isn't an ArrayBufferView, Node.js assumes filehandle.write(string[, position[, encoding]]).
Or it implements this: "If buffer is a plain object, it must have an own (not inherited) toString function property."?
Or this: "If string is not a string, or an object with an own toString function property, the promise is rejected with an error."?

So here goes the important part: current code is already broken.

Correctly handled error:

import {open} from 'fs/promises';
const fh = await open('/dev/null', 'r+');
const obj = {[Symbol.toPrimitive]: hint => 'iAmNotToString'};
fh.write(obj).then(console.log); // should not work according to docs

$ ./node -v && ./node ./tst.mjs
v18.0.0-pre
node:internal/errors:464
    ErrorCaptureStackTrace(err);
    ^

TypeError [ERR_INVALID_ARG_TYPE]: The "buffer" argument must be of type string or an instance of Buffer, TypedArray, or DataView. Received an instance of Object
    at write (node:internal/fs/promises:586:3)
    at fsCall (node:internal/fs/promises:365:18)
    at FileHandle.write (node:internal/fs/promises:190:12)
    at file:///home/livia/nodejs/node/tst.mjs:4:4 {
  code: 'ERR_INVALID_ARG_TYPE'
}

Again but with toString:

import {open} from 'fs/promises';
const fh = await open('/dev/null', 'r+');
const obj = {toString: () => 'writeMe'}; // const obj = {toString() {return 'writeMe';}};
fh.write(obj).then(console.log); // should return: { bytesWritten: 7, buffer: 'writeMe' }

$ ./node -v && ./node ./tst.mjs
v18.0.0-pre
./node[3418]: ../src/string_bytes.cc:319:static size_t node::StringBytes::Write(v8::Isolate*, char*, size_t, v8::Local<v8::Value>, node::encoding, int*): Assertion `val->IsString() == true' failed.
 1: 0x5610cba98420 node::Abort() [./node]
 2: 0x5610cba984a5  [./node]
 3: 0x5610cbb9b98e node::StringBytes::Write(v8::Isolate*, char*, unsigned long, v8::Local<v8::Value>, node::encoding, int*) [./node]
 4: 0x5610cbaa7e58  [./node]
 5: 0x5610cbcfdda4 v8::internal::FunctionCallbackArguments::Call(v8::internal::CallHandlerInfo) [./node]
 6: 0x5610cbcfe6b2  [./node]
 7: 0x5610cbcfebaf v8::internal::Builtin_HandleApiCall(int, unsigned long*, v8::internal::Isolate*) [./node]
 8: 0x5610cc635479  [./node]
Aborted

Same outcome in v16.13.1.

While ".toString() must be an own property, not inherited" rule might successfully prevent from writing random [object Object]s everywhere, it also makes stringifying objects barely usable:

// .write won't accept these:
let url = new URL('https://example.com');
let date = new Date();

// and people will be encouraged to:
url.toString = url.toString;
date.toString = Date.prototype.toString;

// but why not just:
url = url.toString();
await fh.write(date.toString());

Rather than fixing this and allowing weird and potentially dangerous implicit .toString() calls, I suggest to use it as opportunity to change behaviour for object to "named arguments", because it won't break too much.

Thank you.

What alternatives have you considered?

Additional segregation between methods for buffers and strings, e.g. by introducing .writeString, .writeStringSync
Considerable, but devastatingly breaking change.

[aduh95]

Hi @LiviaMedeiros, would you be interested sending a PR implementing this? I'm not sure we can get rid of the toString special casing – that depends how widespread its usage is in the ecosystem (well if it is indeed broken, we can assume no one is using it) – but the rest sounds feasible.

[LiviaMedeiros]

Yes, I'll probably work on this; but it still requires further confirmation if it's safe to change.

Supporting both named arguments and "stringable" objects would look ambiguous for their intersection:

// this one might be interpreted as string by documented logic, but it shouldn't
const dataSlice = {
  buffer: new Uint8Array(4),
  position: 0x40,
  toString() {
    return String.fromCharCode(...this.buffer);
  }
}

// this one won't be interpreted as string because it inherits .toString() from prototype
const someText = new String('someText');

// what is it and would it be obvious enough?
const sameText = Object.assign(someText, {
  buffer: ['iAmNotBuffer', 'justSomeAssignedData']
});

After proper testing, it seems that .writeSync from Synchronous API has the same issue, but .write from Callback API works as documented.

Promises API:

node/lib/internal/fs/promises.js

Lines 586 to 589 in a8afe26

	validateStringAfterArrayBufferView(buffer, 'buffer');
	validateEncoding(buffer, length);
	const bytesWritten = (await binding.writeString(handle.fd, buffer, offset,
	length, kUsePromises)) || 0;

Synchronous API:

node/lib/fs.js

Lines 876 to 882 in a8afe26

	validateStringAfterArrayBufferView(buffer, 'buffer');
	validateEncoding(buffer, length);
	if (offset === undefined)
	offset = null;
	result = binding.writeString(fd, buffer, offset, length,
	undefined, ctx);

Callback API:

node/lib/fs.js

Lines 824 to 842 in a8afe26

	validateStringAfterArrayBufferView(buffer, 'buffer');
	if (typeof position !== 'function') {
	if (typeof offset === 'function') {
	position = offset;
	offset = null;
	} else {
	position = length;
	}
	length = 'utf8';
	}
	const str = String(buffer);
	validateEncoding(str, length);
	callback = maybeCallback(position);
	const req = new FSReqCallback();
	req.oncomplete = wrapper;
	return binding.writeString(fd, str, offset, length, req);

validateStringAfterArrayBufferView just checks if it's a string or an object with ownProperty function .toString
Callback version also performs type conversion before sending it to binding.writeString

Would it make sense to rework only Promises and Synchronous versions, and keep Callback "as is" for now?