Loader hooks' `next` functions behave unexpectedly when `arguments.length` is longer, even if documented named arguments are correct

[cspotcode]

Version

18.6.0

Platform

Linux redacted 5.4.72-microsoft-standard-WSL2 #1 SMP Wed Oct 28 23:40:43 UTC 2020 x86_64 x86_64 x86_64 GNU/Linux

Subsystem

loaders

What steps will reproduce the bug?

https://github.com/cspotcode/repros/tree/node-chaining-bug

Passing more than 2 arguments to the next function passed into a --loader hook will cause unexpected behavior because the implementation of next uses rest args and bases its behavior on arguments.length instead of positionalArg === undefined checks.

How often does it reproduce? Is there a required condition?

always

What is the expected behavior?

No response

What do you see instead?

If one loader hook calls next(url, context, mystery) then the subsequent loader's hook is invoked as resolve(url, context, mystery, next) The mystery value has been passed from one loader to the next. The next argument is at index 3 instead of 2. Additionally, internal behaviors based on arguments.length === 2 may not execute as expected. I see some ctx handling, not 100% sure what it does.

This also happens when mystery is undefined.

This may seem like it's not a bug, since the behavior for passing more than 2 args is unspecified by node's docs. However, there is implicit agreement in JS that functions which are documented to accept named args, not rest args, will ignore any additional args and will not adjust their behavior based on arguments.length.

For example, this unspoken agreement enables code like this to work reliably:

array.filter(lodash.isBoolean); // isBoolean is passed 3 args, but due to unspoken agreement, it will accept and ignore more than 1 arg

Another example where this unspoken agreement to not use arguments.length enables a nice JS pattern:

function doOperation(foo, force = defaultForceValue) {}

const force = shouldForce ? true : undefined; // fallback to default
doOperation(foo, force);

Additional information

Slack thread where this was discussed: https://openjs-foundation.slack.com/archives/C01810KG1EF/p1658853609909249

node v16 release thread; we are hoping to get this fix backported to v16 before 16.17.0 is released.
#44098

I was a bit light on details above, since we've already discussed sufficiently on Slack and @JakobJingleheimer has already agreed to implement the fix. (Thanks!)