NAN_MODULE_INIT cast warning on Node v16

[antimeme]

NAN_MODULE_INIT doesn't seem to do its job with recent versions of Node.js. For example, using v16.18.1 I get the following warning:

/.../.cache/node-gyp/16.18.1/include/node/node.h:887:43: warning: cast between incompatible function types from ‘void (*)(Nan::ADDON_REGISTER_FUNCTION_ARGS_TYPE)’ {aka ‘void (*)(v8::Local<v8::Object>)’} to ‘node::addon_register_func’ {aka ‘void (*)(v8::Local<v8::Object>, v8::Local<v8::Value>, void*)’} [-Wcast-function-type] (node::addon_register_func) (regfunc), \ ^ /.../.cache/node-gyp/16.18.1/include/node/node.h:921:3: note: in expansion of macro ‘NODE_MODULE_X’ NODE_MODULE_X(modname, regfunc, NULL, 0) // NOLINT (readability/null_usage) ^~~~~~~~~~~~~ ../source/node/addon.cc:10709:1: note: in expansion of macro ‘NODE_MODULE’ NODE_MODULE(NODE_GYP_MODULE_NAME, InitAddon) ^~~~~~~~~~~

I can fix this by spelling out the signature of InitAddon (needs three arguments: a local v8::Object, a local v8::Value and void *). But this seems to defeat the purpose of NAN altogether, since a different signature is required for earlier Node releases.

Is this project dead?

[antimeme]

In case it's relevant, I'm using version 2.17.0 of NAN in package.json. That's what the front page here claims is the latest version. The same addon code complies without warnings for Node v12.22.10, so it seems the NAN_MODULE_INIT macro just doesn't understand the new signature expected by Node.

[kkoopa]

Thanks for bringing this to attention. I will see if I can get a fix out.

[agnat]

A few quick notes:

1. @antimeme, somewhat surprisingly the right way to fix the warning is in fact to use the three argument version of the signature. That is the canonical form and it will work correctly even with very old node versions.
2. Another safe option is to ignore the warning. The add-on will work fine on all versions including v16.
3. The upstream code in question is rather obscure and delicate and I don't think we can (or should) paper over the warning like we usually do. @kkoopa please ping me for review or if you want to discuss the issue. If I'm not mistaken that's right in the middle of the node dynamic loader and somehow I'm familiar with its tricks...

[kkoopa]

Righto. I had a look now and if I am not mistaken, this appears to be about context-aware modules, which I presume is what @agnat hinted at. Context-aware modules ought to work with NAN, but might require some minor hoop jumping, as NAN does not acknowledge them since they only appeared in Node 11. The Node documentation on addons still shows the old single-argument signature as the standard requirement. Said documentation also suggests using NODE_MODULE_INIT to handle that case, which I presume ought to work fine. NAN_MODULE_INIT only exists to handle an old signature change of the intialization function from when the base class v8::Handle<T> was removed and fully subsumed by the (then) derived class v8::Local<T>.

If my understanding is correct, I agree that this is not something NAN should (or needs to) address and you should be able to just replace NAN_MODULE_INIT and NODE_MODULE with NODE_MODULE_INIT.

[agnat]

I don't think this is a context aware add-on. Looking at the signature in the warning it looks like a standard addon_register_func. See https://github.com/nodejs/node/blob/v16.x/src/node.h#L823-L832.

[kkoopa]

Ah, so it is just the (relatively) new version of GCC giving arguably superfluous warnings about the cast. IIRC, the official way of suppressing that warning is to have an intermediate cast to void * (which is technically wrong by the standards of both C and C++, since data pointers need not necessarily have the same size as code pointers, but is required by POSIX). If so, this is not a problem with NAN, but with Node itself and upstream should consider adding an intermediate cast to void * at L887, since the documentation states that void (*)(v8::Local<v8::Object>) is the signature to use.

[kkoopa]

A local mitigation for suppressing the warning would then be something like NODE_MODULE(foo, (void *)bar).

[agnat]

Ah, so it is just the (relatively) new version of GCC

Yes, pretty sure it's a change in compiler behaviour. Could be flags or a different compiler. I don't know ... doesn't matter. We know the code hasn't changed. There is no new signature. jedi_hand_gesture.gif

arguably superfluous warnings

I have to disagree. I think it's a great warning because that is a bad cast... I mean ... damn.

That thing where add-on devs can skip unused trailing arguments in the init function, this "signature abbreviation" feature? That's what the cast does. Unfortunately, it also opens pandoras box by completely disabling all compile-time signature checks. As it stands, most compilers will quietly accept any function no matter how wrong. The result is a hard run-time crash on require() instead of a more graceful compile-time error. It's not even that difficult to implement the "signature abbreviation" in a type-safe fashion. So, I feel that's the way forward and the cast has to go...

Anyway, it's not NANs business. @antimeme, please report the warning at nodejs/node and maybe mention our discussion here.

[kkoopa]

Yes, I remember running into that warning some years ago. I think it was GCC 8 that begun having it in -Wall by default. I did not care for it then, nor do I now, but then I also try to avoid casting and only do so when necessary. I cannot think of a situation where one would ever do a cast unintentionally (in first-party code). Having stated this, I do agree that the cast is bad.

Anyway, thanks for helping out. It was a long time since I last had to peer into the internals of Node and you saved me from going down the wrong rabbit hole. It is always great to have an expert available for discussion. I will now close this issue since it has been resolved.

[antimeme]

I'd be happy to report this to nodejs/node (with a link here) but I'm not sure I understand the reason this is a problem for Node itself. The impression I get is that the signature expected for the first argument to NODE_MODULE changed from a one argument function before Node v11 to a three argument function afterward. Maybe I've misread that?

Why isn't it the responsibility of NAN_MODULE_INIT to deal with this? My solution has been to avoid using NAN_MODULE_INIT and use the three argument signature. The warnings are gone and no cast is necessary. But if the signature changes in the future or if I try to build for Node versions before v11 my code will fail. I could address this with #ifdef but that seems like the exact purpose for which NAN_MODULE_INIT was created in the first place.

I may just be missing something but the impression I'm getting is that NAN_MODULE_INIT is wrong but is not going to be fixed. Is that right?

[agnat]

The impression I get is that the signature expected for the first argument to NODE_MODULE changed from a one argument function before Node v11 to a three argument function afterward. Maybe I've misread that?

Yes, you misread that. Follow the source code link in my comment above and bonk the blame button. The signature hasn't changed in a long while.

Why isn't it the responsibility of NAN_MODULE_INIT to deal with this?

Because it is an actual issue in the node code base and not a compatibility issue. It is not NANs job to hide actual issues.

I may just be missing something but the impression I'm getting is that NAN_MODULE_INIT is wrong but is not going to be fixed. Is that right?

Yes, you are misreading the issue. No, nothing is wrong with NAN_MODULE_INIT. So, yeah. No fixes on the NAN side.

Hope this helps.

[kkoopa]

Depends on how you look at it. NAN_MODULE_INIT is not wrong from the perspective of Node, cf. in https://github.com/nodejs/node/blob/main/doc/api/addons.md. You will get the exact same warning if you create an addon as described in said documentation.

NAN_MODULE_INIT exists because of the aforementioned modification when the signature changed from void (*)(v8::Handle<v8::Object>) to void (*)(v8::Local<v8::Object>).

The single-argument intializer is the standard one and always has been. While NAN_MODULE_INIT could paper over it to suppress the warning, that is not the proper fix, since it eliminates type safety, more relevant if Node were to ever properly address the root cause. Warnings and errors should arise when passing incompatible function pointers around, so that one does not do so by mistake.

Now, if the single-argument initializer were to be removed from Node, it would be proper to make NAN_MODULE_INIT choose between the single- and three-argument initializer.

[agnat]

That thing where add-on devs can skip unused trailing arguments in the init function, this "signature abbreviation" feature?

@antimeme, that's where you're at. You've been skipping the unused arguments. That's fine. You are using a traditional node feature/hack/quirk. It has always been like that. However, now it issues a warning on some compilers and the node people should take look.

Hope this helps.

[antimeme]

What is the root cause, exactly? Following that source link, NODE_MODULE expands to NODE_MODULE_X which casts regfunc to node::addon_register_func which takes three arguments and has taken three arguments for a long time. So it seems the issue on the Node side is that the documentation (specifically https://github.com/nodejs/node/blob/main/doc/api/addons.md) is wrong?

[kkoopa]

The root cause is the typecast. I would not say the documentation is incorrect. The cast is perfectly legit, but it is still bad, since it allows feeding any old function as the initializer and causes the warning in question.

[agnat]

I'd say the documentation is incomplete. It does not explain the signature options at all, IIRC. It is mostly tutorial style material and lacks actual reference documentation. But yeah... what @kkoopa says

[antimeme]

Either NODE_MODULE requires a function that takes one argument or it requires a function that takes three arguments. Looking through the Node source, I see src/node_binding.cc:528, src/node_binding.cc:642 both of which call it with three arguments and src/node_api.cc:627 which casts it to something for napi. I can't see any place it might be called with one argument. You seem to be arguing that Node actually requires a function of one argument -- or at least you're denying that NAN_MODULE_INIT is wrong to create a function of one argument where a function taking three arguments is required.

The cast may be also be a bug, but it's not this bug. The documentation may also have a bug, but again, that's not this bug. This bug is that NAN_MODULE_INIT generates a function signature with one argument but Node has wanted a function with three arguments for at least nine years, even if compiler warnings didn't make that apparent until recently.

[kkoopa]

If we are going to split hairs, Node requires every addon to supply a registration callback. It can have any number of parameters and any return type. Any returned value will be ignored. The three first parameters may be valid arguments in the registration callback. If so, the first is v8::Local<v8::Object>, the second is v8::Local<v8::Value>, the third is any pointer type.

[agnat]

Either NODE_MODULE requires a function that takes one argument or it requires a function that takes three arguments.

That's not true. There are a number of ways to implement callbacks with "optional arguments". Node implements the pattern using said cast. It exploits the standard C calling convention where arguments are passed using the stack. If the callee doesn't use all arguments on the stack, that's fine. I don't like the pattern because of the pandoras box above. But that's how it is implemented...