lib: prefer call() over apply() if argument list is not array

[LiviaMedeiros]

All three of FunctionPrototypeCall, FunctionPrototypeApply, and ReflectApply are used across codebase without clear guidelines on when each should be preferred.
The inconsistency gets to the point where identical lines within same file may use different approach:

node/lib/internal/fs/utils.js

Lines 494 to 498 in b8b4350

	function BigIntStats(dev, mode, nlink, uid, gid, rdev, blksize,
	ino, size, blocks,
	atimeNs, mtimeNs, ctimeNs, birthtimeNs) {
	ReflectApply(StatsBase, this, [dev, mode, nlink, uid, gid, rdev, blksize,
	ino, size, blocks]);

node/lib/internal/fs/utils.js

Lines 522 to 526 in b8b4350

	function Stats(dev, mode, nlink, uid, gid, rdev, blksize,
	ino, size, blocks,
	atimeMs, mtimeMs, ctimeMs, birthtimeMs) {
	FunctionPrototypeCall(StatsBase, this, dev, mode, nlink, uid, gid, rdev,
	blksize, ino, size, blocks);

This PR brings a bit more consistency by using FunctionPrototypeCall whenever the argument list is not an already predefined array.

In terms of performance, currently apply with short predefined array of arguments is almost as fast as call with predefined variables (i.e. not ...args nor args[0], args[1], ...), while call is 2-3 times faster than apply that allocates new array for each call.

This probably can become a linter rule.

[nodejs-github-bot]

Review requested:

• @nodejs/crypto
• @nodejs/loaders
• @nodejs/net

[codecov]

Codecov Report

❌ Patch coverage is 98.66667% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 88.55%. Comparing base (93fc880) to head (0eee566).
⚠️ Report is 15 commits behind head on main.

Files with missing lines	Patch %	Lines
lib/internal/tls/wrap.js	80.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #60796      +/-   ##
==========================================
+ Coverage   88.53%   88.55%   +0.01%     
==========================================
  Files         703      703              
  Lines      208260   208252       -8     
  Branches    40156    40166      +10     
==========================================
+ Hits       184393   184421      +28     
+ Misses      15881    15832      -49     
- Partials     7986     7999      +13     

Files with missing lines	Coverage Δ
lib/assert.js	98.23% <100.00%> (ø)
lib/dgram.js	97.45% <100.00%> (-0.01%)	⬇️
lib/fs.js	98.18% <100.00%> (+<0.01%)	⬆️
lib/internal/child_process.js	95.08% <100.00%> (-0.01%)	⬇️
lib/internal/cluster/child.js	98.38% <100.00%> (+<0.01%)	⬆️
lib/internal/cluster/worker.js	100.00% <100.00%> (ø)
lib/internal/crypto/cipher.js	97.94% <100.00%> (ø)
lib/internal/crypto/hash.js	98.93% <100.00%> (ø)
lib/internal/crypto/sig.js	95.73% <100.00%> (-0.02%)	⬇️
lib/internal/crypto/webcrypto.js	96.37% <100.00%> (+<0.01%)	⬆️
... and 13 more

... and 37 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[Renegade334]

As an aside, it'd be interesting to know whether there's any significant performance difference between the bound FunctionPrototypeApply and the native ReflectApply.

[nodejs-github-bot]

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

[aduh95]

This probably can become a linter rule.

Using the following:

diff --git a/lib/eslint.config_partial.mjs b/lib/eslint.config_partial.mjs
index b919708a978..457ae91a987 100644
--- a/lib/eslint.config_partial.mjs
+++ b/lib/eslint.config_partial.mjs
@@ -40,2 +40,10 @@ const noRestrictedSyntax = [
   },
+  {
+    selector: "CallExpression:matches([callee.type='Identifier'][callee.name='FunctionPrototypeApply'], [callee.type='MemberExpression'][callee.property.type='Identifier'][callee.property.name='apply'][arguments.length=2])",
+    message: 'Use `ReflectApply` instead of %Function.prototype.apply%',
+  },
+  {
+    selector: "CallExpression[callee.type='Identifier'][callee.name='ReflectApply'][arguments.2.type='ArrayExpression']",
+    message: 'Use `FunctionPrototypeCall` to avoid creating an ad-hoc array',
+  },
 ];

I see there are still 33 reported "problems" in lib/. I guess that can be addressed in a followup

[aduh95]

@Renegade334 added backport-requested-v24.x PRs awaiting manual backport to the v24.x-staging branch.

Why?

[Renegade334]

Why?

Doesn't land, is all.

[aduh95]

Why?

Doesn't land, is all.

It's probably too soon to assess, the staging branch is many commits behind

[LiviaMedeiros]

As an aside, it'd be interesting to know whether there's any significant performance difference between the bound FunctionPrototypeApply and the native ReflectApply.

From a brief comparison on main and v24.11.1, their performance as primordials using --expose-internals looks exactly the same. Follow-up, moving away from FunctionPrototypeApply purely for consistency, probably wouldn't hurt.

Also for the opposite (FunctionPrototypeCall(something, something, ...args)), the only occurrence i've found was:

node/lib/internal/webstreams/util.js

Lines 169 to 178 in b8b4350

	// This implements "invoke a callback function type" for callback functions that return a promise.
	// See https://webidl.spec.whatwg.org/#es-invoking-callback-functions
	async function invokePromiseCallback(fn, thisArg, ...args) {
	return FunctionPrototypeCall(fn, thisArg, ...args);
	}
	function createPromiseCallback(name, fn, thisArg) {
	validateFunction(fn, name);
	return (...args) => invokePromiseCallback(fn, thisArg, ...args);
	}

I see there are still 33 reported "problems" in lib/. I guess that can be addressed in a followup

Thanks! At least some of them come from specific files that had prototype primordials removed and restricted:

node/lib/eslint.config_partial.mjs

Lines 513 to 527 in b8b4350

	'lib/_http_*.js',
	'lib/_tls_*.js',
	'lib/http.js',
	'lib/http2.js',
	'lib/internal/http.js',
	'lib/internal/http2/*.js',
	'lib/tls.js',
	'lib/zlib.js',
	],
	rules: {
	'no-restricted-syntax': [
	...noRestrictedSyntax,
	{
	selector: 'VariableDeclarator:has(.init[name="primordials"]) Identifier[name=/Prototype[A-Z]/]:not([name=/^(Object|Reflect)(Get|Set)PrototypeOf$/])',
	message: 'Do not use prototype primordials in this file.',

AFAICT it would be okay to suppress this rule or use .call() instead of primordial, but it's better to be separate PR with benchmark CI.

[nodejs-github-bot]

Landed in 46dc5d7