Node: Better assertion message for C++ binding argument checks

[joyeecheung]

Refs: https://github.com/nodejs/node/issues/20325

At the moment we simply use something like

CHECK(args[1]->IsString());

for guarding against incorrect usage of the C++ bindings, which produces a message that looks like this:

../src/node_contextify.cc:631:static void node::contextify::ContextifyScript::New(const FunctionCallbackInfo<v8::Value> &): Assertion `args[1]->IsString()' failed.

This is not exactly useful for users who don't know much about our internals. A better message would be something like this (ideally we could phrase it better but that would mean more C++ logic):

2nd argument passed to `process.binding('contextify').ContextifyScript` is not a String.

(DEP0111): process.binding() is deprecated.
You are likely using code that accesses Node.js internals incorrectly.

<JS stack>

<C++ stack>

Which could be generated with a macro like this (or we could pass the 2nd bit if we don't want to write a formatter in C++):

CHECK_BINDING_ARG(args, 1, String, contextify, ContextifyScript);

We could also use UNLIKELY() when implementing the macro for compiler hints.

[bnoordhuis]

CHECKs are for impossible conditions. I don't see the need for complicating the code base because a few npm modules do stupid things.

The plan is to start printing deprecation warnings for process.binding('whatever') in the near future, right? That seems like the simplest way forward.

[joyeecheung]

The plan is to start printing deprecation warnings for process.binding('whatever') in the near future, right? That seems like the simplest way forward.

Even if we start runtime-deprecating process.binding, it will probably take a long time until we can actually remove it. Is that possible in, say, within 3 years?

[fhinkel]

Should this remain open?

[joyeecheung]

Until we have a concrete plan to deprecate process.binding at all, I think this should remain open.