lib: consistent this in callbacks

[Trott]

This change normalizes a few situations where a callback might be invoked with this set to undefined in one case, but null in a very similar case.

First commit:

test: refactor test-fs-stat

* add `use strict'
* change checks that `this` is mapped to `global` in sloppy mode to
  checks that `this` is `undefined`
* modify arguments to assertions to match docs (actual first, expected
  second)
* add blank line below `common` declaration per test writing guide
* use `assert.ifError()` as appropriate

Second commit:

fs: invoke callbacks with undefined context

Many callbacks appear to be invoked with `this` set to `undefined`
including `fs.stat()`, `fs.lstat()`, and `fs.fstat()`.

However, some such as `fs.open()` and `fs.mkdtemp()` invoke their
callbacks with `this` set to `null`.

Use a consistent way to invoke callbacks to guarantee consistency.

Third commit:

test: check `this` value for `nextTick()`

Depending on how many arguments are provided, `nextTick()` may run its
callback with `this` set to `null` or not. Add assertions for
both cases.

Fourth commit:

process: make `this` value consistent

The value of `this` for callbacks of `nextTick()` can vary depending on
the number of arguments. Make it consistent.

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• tests and/or benchmarks are included
• commit message follows commit guidelines

Affected core subsystem(s)

test process lib fs

[Trott]

Needs a CITGM run and some benchmarks too, although I imagine the consistency aspect overrides anything the benchmarks show unless it's a severe perf hit.

[mscdex]

This is the kind of thing we definitely need to benchmark... both here and nextTick() especially because in both scenarios a different type of value is being used with the spread operator (special arguments vs array).

[Trott]

The current next-tick benchmarks do not test the code modified here. I'll add a commit to include this code path in the benchmarks.

[refack]

There are a few other combinations that should be benchmarked:

1. cb.bind(undefined)
2. function (...args) { cb(...args) }
3. (...args) => cb(...args)

I'll try to check some.

[Trott]

OK, I went back to .apply(undefined, arguments) which probably won't correct as many edge cases, but is better than the current .apply(null, arguments) in that regard and doesn't have a negative perf hit.

[Trott]

@mscdex I updated the process.nextTick() benchmark code in e15b625.

Running the resulting benchmarks shows no significant performance difference.

Command I ran to generate data:

$ node benchmark/compare.js --old ./node-master --new ./node-strictify --filter next-tick --filter args  process | tee compare.csv

Command I ran to analyze it:

$ cat compare.csv | Rscript benchmark/compare.R

Result:

                                              improvement confidence   p.value
 process/next-tick-breadth-args.js millions=2     -0.48 %            0.3246212
 process/next-tick-depth-args.js millions=12       0.42 %            0.1323853

Increasing the number of runs might help decrease the p-values a bit, but so far, it seems like the change does not significantly impact performance.

[Trott]

@nodejs/ctc This is semver-major so needs at least two CTC approvals.

[Trott]

CI: https://ci.nodejs.org/job/node-test-pull-request/9525/

[mcollina]

LGTM

[refack]

There are a few other combinations that should be benchmarked:

1. cb.bind(undefined)
2. function (...args) { cb(...args) }
3. (...args) => cb(...args)

I'll try to check some.

[Trott]

Going to rebase and force push so I can check the fs benchmarks. I don't expect that to turn anything up, but we'll see.

[Trott]

On the nextTick() benchmarks, I can't help but notice that the previous benchmarks tested up to three arguments, and that's exactly how many arguments were fast-pathed by the switch statement. In other words, the implementation and the benchmark matched each other in a way that suggests that we may have been coding to the particulars of the benchmark. It might be worth benchmarking without the switch statement to get an idea of the perf difference now, but that's for another PR.