From 446bdb60899d761d5c4d5d1bf53affc7278f54d4 Mon Sep 17 00:00:00 2001
From: Darshan Sen <raisinten@gmail.com>
Date: Wed, 27 Jan 2021 18:53:03 +0530
Subject: [PATCH] fs: change default value of position in read and readSync

Since the docs mention that `position` in `read` and `readSync` should
be an `integer` or a `bigint`, the functions should rather accept what
is actually fed into `read`, i.e., `-1` instead of any nullish value.
---
 doc/api/fs.md                        | 12 ++++++++---
 lib/fs.js                            | 22 ++++++++++++++++----
 test/parallel/test-fs-promisified.js |  2 +-
 test/parallel/test-fs-read-type.js   | 30 +++++++++++++++++++++++++++-
 test/parallel/test-fs-readfile-fd.js |  2 +-
 5 files changed, 58 insertions(+), 10 deletions(-)

diff --git a/doc/api/fs.md b/doc/api/fs.md
index e11c057802e763..6b8e65e1739d4a 100644
--- a/doc/api/fs.md
+++ b/doc/api/fs.md
@@ -2704,6 +2704,9 @@ directory and subsequent read operations.
 <!-- YAML
 added: v0.0.2
 changes:
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs/node/pull/37101
+    description: Runtime description.
   - version: v10.10.0
     pr-url: https://github.com/nodejs/node/pull/22150
     description: The `buffer` parameter can now be any `TypedArray`, or a
@@ -2722,8 +2725,8 @@ changes:
 * `offset` {integer} The position in `buffer` to write the data to.
 * `length` {integer}  The number of bytes to read.
 * `position` {integer|bigint} Specifies where to begin reading from in the
-  file. If `position` is `null` or `-1 `, data will be read from the current
-  file position, and the file position will be updated. If `position` is an
+  file. If `position` is `-1 `, data will be read from the current file
+  position, and the file position will be updated. If `position` is any other
   integer, the file position will be unchanged.
 * `callback` {Function}
   * `err` {Error}
@@ -2746,6 +2749,9 @@ added:
  - v13.11.0
  - v12.17.0
 changes:
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs/node/pull/37101
+    description: Runtime description.
   - version:
      - v13.11.0
      - v12.17.0
@@ -2758,7 +2764,7 @@ changes:
   * `buffer` {Buffer|TypedArray|DataView} **Default:** `Buffer.alloc(16384)`
   * `offset` {integer} **Default:** `0`
   * `length` {integer} **Default:** `buffer.length`
-  * `position` {integer|bigint} **Default:** `null`
+  * `position` {integer|bigint} **Default:** `-1`
 * `callback` {Function}
   * `err` {Error}
   * `bytesRead` {integer}
diff --git a/lib/fs.js b/lib/fs.js
index d396d3a80c99c0..0dbee09a9eb55c 100644
--- a/lib/fs.js
+++ b/lib/fs.js
@@ -534,7 +534,7 @@ function read(fd, buffer, offset, length, position, callback) {
       buffer = Buffer.alloc(16384),
       offset = 0,
       length = buffer.length,
-      position
+      position = -1,
     } = options);
   }
 
@@ -562,8 +562,13 @@ function read(fd, buffer, offset, length, position, callback) {
 
   validateOffsetLengthRead(offset, length, buffer.byteLength);
 
-  if (position == null)
+  if (position === null) {
+    process.emitWarning(
+      `The provided ${position} is not a valid position, and is supported ` +
+      'in the fs module solely for compatibility.',
+      'DeprecationWarning', 'DEP01149');
     position = -1;
+  }
 
   validatePosition(position, 'position');
 
@@ -592,7 +597,11 @@ function readSync(fd, buffer, offset, length, position) {
     // Assume fs.read(fd, buffer, options)
     const options = offset || {};
 
-    ({ offset = 0, length = buffer.length, position } = options);
+    ({
+      offset = 0,
+      length = buffer.length,
+      position = -1,
+    } = options);
   }
 
   validateBuffer(buffer);
@@ -616,8 +625,13 @@ function readSync(fd, buffer, offset, length, position) {
 
   validateOffsetLengthRead(offset, length, buffer.byteLength);
 
-  if (position == null)
+  if (position === null) {
+    process.emitWarning(
+      `The provided ${position} is not a valid position, and is supported ` +
+      'in the fs module solely for compatibility.',
+      'DeprecationWarning', 'DEP0149');
     position = -1;
+  }
 
   validatePosition(position, 'position');
 
diff --git a/test/parallel/test-fs-promisified.js b/test/parallel/test-fs-promisified.js
index 0744f62fd17fa5..f24123694fc12f 100644
--- a/test/parallel/test-fs-promisified.js
+++ b/test/parallel/test-fs-promisified.js
@@ -11,7 +11,7 @@ const exists = promisify(fs.exists);
 
 {
   const fd = fs.openSync(__filename, 'r');
-  read(fd, Buffer.alloc(1024), 0, 1024, null).then(common.mustCall((obj) => {
+  read(fd, Buffer.alloc(1024), 0, 1024, -1).then(common.mustCall((obj) => {
     assert.strictEqual(typeof obj.bytesRead, 'number');
     assert(obj.buffer instanceof Buffer);
     fs.closeSync(fd);
diff --git a/test/parallel/test-fs-read-type.js b/test/parallel/test-fs-read-type.js
index 5635fe0a662c64..819374fc85d235 100644
--- a/test/parallel/test-fs-read-type.js
+++ b/test/parallel/test-fs-read-type.js
@@ -8,7 +8,6 @@ const filepath = fixtures.path('x.txt');
 const fd = fs.openSync(filepath, 'r');
 const expected = 'xyz\n';
 
-
 // Error must be thrown with string
 assert.throws(
   () => fs.read(fd, expected.length, 0, 'utf-8', common.mustNotCall()),
@@ -90,6 +89,21 @@ assert.throws(() => {
   });
 });
 
+{
+  fs.read(fd,
+          Buffer.allocUnsafe(expected.length),
+          0,
+          expected.length,
+          null,
+          common.mustCall());
+  common.expectWarning(
+    'DeprecationWarning',
+    'The provided null is not a valid position, and is supported ' +
+    'in the fs module solely for compatibility.',
+    'DEP01149',
+  );
+}
+
 [0.5, 2 ** 53, 2n ** 63n].forEach((value) => {
   assert.throws(() => {
     fs.read(fd,
@@ -210,6 +224,20 @@ assert.throws(() => {
   });
 });
 
+{
+  fs.readSync(fd,
+              Buffer.allocUnsafe(expected.length),
+              0,
+              expected.length,
+              null);
+  common.expectWarning(
+    'DeprecationWarning',
+    'The provided null is not a valid position, and is supported ' +
+    'in the fs module solely for compatibility.',
+    'DEP01149',
+  );
+}
+
 [0.5, 2 ** 53, 2n ** 63n].forEach((value) => {
   assert.throws(() => {
     fs.readSync(fd,
diff --git a/test/parallel/test-fs-readfile-fd.js b/test/parallel/test-fs-readfile-fd.js
index d7bf11c2d8d671..7097e67bc5b1d2 100644
--- a/test/parallel/test-fs-readfile-fd.js
+++ b/test/parallel/test-fs-readfile-fd.js
@@ -79,7 +79,7 @@ function tempFdSync(callback) {
       const buf = Buffer.alloc(5);
 
       // Read only five bytes, so that the position moves to five.
-      fs.read(fd, buf, 0, 5, null, common.mustSucceed((bytes) => {
+      fs.read(fd, buf, 0, 5, -1, common.mustSucceed((bytes) => {
         assert.strictEqual(bytes, 5);
         assert.deepStrictEqual(buf.toString(), 'Hello');