Some possible bugs concerning fs.appendFile() and fs.writeFile()

[vsemozhetbyt]

• Version: 7.6.0
• Platform: Windows 7 x64
• Subsystem: fs, test

1. fs.appendFile() and fs.writeFile() can be called without mandatory data parameter, while not throwing any error messages. Thus, the only second parameter serves as data and callback (mandatory as well) parameters at the same time.

const fs = require('fs');

const cb = (err) => { if(err) console.error(err); }

fs.appendFile('append-file.txt', cb);
fs.writeFile('write-file.txt', cb);

This code runs without any errors and produces 2 files with the same content:

> cat append-file.txt
(err) => { if(err) console.error(err); }
> cat write-file.txt
(err) => { if(err) console.error(err); }

It seems this is hardly an expected behavior.

2. test/parallel/test-fs-null-bytes.js tests these functions with wrong parameters scheme, i.e. without mandatory data parameter. This may be not very important for the test aim, but it makes it somehow compromised.

[gireeshpunathil]

Agreed that this is indeed an issue.
Modifying the callback thus:

const cb = (err) => { if(err) console.error(err); else console.log('well done!');}

shows that 'cb' is used as data, as well as the callback.

Looking at the writeFile implementation fs.watchFile I see that the callback and the data being extracted without cohesion between them, as the root case. For example, callback is extracted as the last param, and data is extracted as the second param. No assertion is made to make sure they are discretely different, and mandatory, as the per the API doc:
fs.writeFile(file, data[, options], callback)

[vsemozhetbyt]

PR for the second issue.

[seishun]

@vsemozhetbyt Just curious, how did you find out that test/parallel/test-fs-null-bytes.js is problematic?

[vsemozhetbyt]

@seishun I was trying to eliminate deopts in some fs functions and was going over possible fixes. One of these fixes was to replace this line in the fs.writeFile():
callback = maybeCallback(arguments[arguments.length - 1]);
by this one:
callback = maybeCallback(callback || options);
Then I have built and have run tests and only test/parallel/test-fs-null-bytes.js has failed, so I went at it)

[ghaiklor]

yeah, writeFile doesn't have a check for data.

fs.writeFile = function(path, data, options, callback) {
  callback = maybeCallback(arguments[arguments.length - 1]);
  options = getOptions(options, { encoding: 'utf8', mode: 0o666, flag: 'w' });
  // ...
}

So, in @vsemozhetbyt case, data is a callback and a data at the same time, which leads to converting a function into string and writing it into file as data and calling it as a callback since it's the last one argument from the list.

What if we will add a small check here:

fs.writeFile = function(path, data, options, callback) {
  if (typeof data === 'function') throw new Error('you need to specify data');

  callback = maybeCallback(arguments[arguments.length - 1]);
  options = getOptions(options, { encoding: 'utf8', mode: 0o666, flag: 'w' });
  // ...
}

I believe that order (path: String, data: Buffer) is strict here and we can just check if data is a function or not.

What you think?

UPD: I like @vsemozhetbyt 's approach with maybeCallback(callback || options), though I'm not sure if we can throw an error Calling an asynchronous function without callback is deprecated.. May be better to throw an error with direct message about data argument.

[vsemozhetbyt]

@ghaiklor There is a PR that tries to fix this in a different way, but it has hung a little. Let's see how it will proceed)

[ghaiklor]

@vsemozhetbyt BTW, it can be closed due works as expected, since docs are saying that you need to specify 3 arguments and only options is optional 😸

[vsemozhetbyt]

@ghaiklor Let's have an eccentric method for a change)

[seishun]

Fixed in 208db56.