fs: add read(buffer[, options]) versions

[LiviaMedeiros]

Refs: #42601 (writing counterparts)

This PR does:

• Adding fs.read(fd, buffer[, options], callback)
• Adding filehandle.read(buffer[, options])
• Aligning fs.readSync code (changes in UB corner cases) ¹ (offloaded due to potential breakage)

These changes would allow to:

• Align writing methods with uniform buffer[, options] signature ²
• Align signatures of reading methods
• Which also means not only read-write interoperability within each of three APIs, but also between any of them
• Achieve a bit more consistent UB behaviour, when documentation implies similarity ("For detailed information, see the documentation of the asynchronous version of this API")
• Have it fully completed without waiting for DEP0162 EOL
• Harden fs.readSync typechecking ³
• (optional) Introduce params⁴ signatures later, if needed ⁵
• (optional) Deprecate and remove params signatures later, if strongly needed ⁶
• (opinionated) Maybe make usage of fs.read and fh.read less error-prone ⁶

Why current behaviour is questionable

Default buffer in fs.read and filehandle.read is Buffer.alloc(16384). It's a pretty reasonable value for positioned argument (i.e. if it's used only when all further arguments are unset as well), but it's a footgun when length is defined.

#!/usr/bin/env node
import {open} from 'fs/promises';
async function readSubstring(filepath, len = 0, pos = 0) {
	const fh = await open(filepath, 'r');
	const res = await fh.read({ length: Number(len), position: Number(pos) });
	const str = String(res.buffer);
	return str;
}
const substr = await readSubstring(...process.argv.slice(2));
console.log(substr);

$ echo 123456789 > /tmp/tst
$ readsubstr.mjs /tmp/tst 3 5
678
$ readsubstr.mjs /tmp/tst 3 5 | wc -c
16385

Without reading the docs properly, I'd totes assume that it allocates length + offset||0 number of bytes by default.
Because of that, buffer has to be allocated explicitly. Which makes [buffer[, options]] signature more reasonable and less misleading.

Footnotes

1. Changes in UB are about interpreting false/null/undefined values as invalid or default. I think, the best way here is to interpret typeof offsetOrOptions === 'object' as indicator of attempt to use "named params" version. null means default values. false and undefined means offset === 0. Everything else goes to offset's integer validation.
   Exception: Callback API methods will still check the number of arguments as well, to keep callback strictly positioned. ↩

2. Refs: https://github.com/nodejs/node/pull/42601#discussion_r846095440 ↩

3. Current implementation of fs.readSync interprets any value as options object. For example, passing a primitive string will use its length property without any warning. ↩

4. I use the following terminology here: options == { offset, length, position }, params == { buffer, ...options } == { buffer, offset, length, position } ↩

5. Theoretical reason to include buffer is its strong connection to offset and length. If we specify nonzero offset, we usually imply a non-arbitrary buffer. However, I don't have strong arguments nor good examples; and it can be worked around anyway. ↩

6. See "Why current behaviour is questionable" foldablie ↩ ↩²

[LiviaMedeiros]

Offloaded fs.readSync changes to a separate PR.

I'm still confused about offset === null and options.offset === null case.

• In lib: fix fs.read when passing null value #32479, setting any option to null was prohibited, but the test was bugged.
  As for today, position == null || position === -1 has a special meaning ("read at current position and update it"). length is silently coerced to integer. offset is either coerced to 0 or interpreted as default options (in readSync version).

• Later, test: add coverage for uncovered if statement in fs.read function(callback and promise api) #35918 introduced opposite (passing) test for { offset: null }.
  This test is changed in this PR, original version won't pass.

• In test: improve fs coverage #38517, in particular test: improve fs coverage #38517 (comment), the original test was removed as de-facto not working.

I believe that the intention behind original test was purely about default params (so, for example, it won't override position it default value for it was 0).
Is there something else, and should or not offset === null throw?

My personal preference in a context of this PR is to prohibit it to avoid any intersection with options object, purely because typeof null === 'object'.
However, if supporting nullable offset is the desired behaviour, it can be worked around to avoid any breaking changes here.

[LiviaMedeiros]

In current state or this PR, breaking changes for fs.readSync are reverted, and handling of offset === null cases should be compatible with master branch. I'm doing brief rechecks with this.

If there are still any signs of breaking changes, please do not hesitate to point them out.
I believe that it can be kept within semver-minor PRs that contain new features and should be released in the next minor version. without functionality loss.

[nodejs-github-bot]

Commit Queue failed

- Loading data for nodejs/node/pull/42768
✔  Done loading data for nodejs/node/pull/42768
----------------------------------- PR info ------------------------------------
Title      fs: add read(buffer[, options]) versions (#42768)
   ⚠  Could not retrieve the email or name of the PR author's from user's GitHub profile!
Branch     LiviaMedeiros:optional-read-signature -> nodejs:master
Labels     fs, semver-minor, author ready, needs-ci, commit-queue-squash
Commits    16
 - fs: add read(buffer, options) signatures
 - squash: make offsetOrOptions more consistent
 - squash: add doc entries
 - squash: fix fs.read()
 - squash: expand fs-read-optional test
 - squash: expand fs-read-promises-optional test
 - squash: improve tests
 - squash: fixup
 - squash: fixup
 - squash: inline null coalescing
 - squash: revert changes in fs.readSync()
 - squash: disallow offset === false
 - squash: fixup
 - squash: allow offset === null
 - squash: fix tests being other way round
 - squash: adjust tests
Committers 1
 - LiviaMedeiros 
PR-URL: https://github.com/nodejs/node/pull/42768
Refs: https://github.com/nodejs/node/pull/42601
Reviewed-By: Antoine du Hamel 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/42768
Refs: https://github.com/nodejs/node/pull/42601
Reviewed-By: Antoine du Hamel 
--------------------------------------------------------------------------------
   ℹ  This PR was created on Sun, 17 Apr 2022 17:15:02 GMT
   ✔  Approvals: 1
   ✔  - Antoine du Hamel (@aduh95) (TSC): https://github.com/nodejs/node/pull/42768#pullrequestreview-950452626
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2022-04-29T02:55:25Z: https://ci.nodejs.org/job/node-test-pull-request/43764/
- Querying data for job/node-test-pull-request/43764/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  No git cherry-pick in progress
   ✔  No git am in progress
   ✔  No git rebase in progress
--------------------------------------------------------------------------------
- Bringing origin/master up to date...
From https://github.com/nodejs/node
 * branch                  master     -> FETCH_HEAD
✔  origin/master is now up-to-date
- Downloading patch for 42768
From https://github.com/nodejs/node
 * branch                  refs/pull/42768/merge -> FETCH_HEAD
✔  Fetched commits as 916a13a8a36d..ab0b534874af
--------------------------------------------------------------------------------
Auto-merging lib/fs.js
[master 4b272000d4] fs: add read(buffer, options) signatures
 Author: LiviaMedeiros 
 Date: Fri Apr 15 22:01:15 2022 +0800
 3 files changed, 37 insertions(+), 30 deletions(-)
Auto-merging lib/fs.js
CONFLICT (content): Merge conflict in lib/fs.js
error: could not apply aec64cae9d... squash: make offsetOrOptions more consistent
hint: After resolving the conflicts, mark them with
hint: "git add/rm ", then run
hint: "git cherry-pick --continue".
hint: You can instead skip this commit with "git cherry-pick --skip".
hint: To abort and get back to the state before "git cherry-pick",
hint: run "git cherry-pick --abort".
   ✖  Failed to apply patches

https://github.com/nodejs/node/actions/runs/2259461302

[LiviaMedeiros]

Rebasing this atm

[aduh95]

Landed in 57678e5

[LiviaMedeiros]

Oh, thanks! 😄