src: Add an operator to retrieve raw napi_callback_info from CallbackInfo

[JckXia]

Resolves #1234, and also complete unit test coverage for #1048

[mhdawson]

LGTM

[mhdawson]

@JckXia thanks! Landed in 55bd08e

[lovell]

There's a new compilation error with sharp that occurs on Windows only when using node-addon-api v5.1.0 that does not happen with v5.0.0, and this PR seems to be the most relevant change that might have triggered it.

D:\a\sharp\sharp\src\metadata.cc(279,32): error C2666: 'Napi::CallbackInfo::operator []': 2 overloads have similar conversions [D:\a\sharp\sharp\build\sharp-win32-ia32.vcxproj]
D:\a\sharp\sharp\node_modules\node-addon-api\napi-inl.h(3477,34): message : could be 'const Napi::Value Napi::CallbackInfo::operator [](size_t) const' (compiling source file ..\src\metadata.cc) [D:\a\sharp\sharp\build\sharp-win32-ia32.vcxproj]
D:\a\sharp\sharp\src\metadata.cc(279,32): message : or       'built-in C++ operator[(napi_callback_info, int)' [D:\a\sharp\sharp\build\sharp-win32-ia32.vcxproj]
D:\a\sharp\sharp\src\metadata.cc(279,32): message : while trying to match the argument list '(const Napi::CallbackInfo, int)' [D:\a\sharp\sharp\build\sharp-win32-ia32.vcxproj]

https://github.com/lovell/sharp/actions/runs/3924810417/jobs/6709314064

The code that triggers this is relatively straightforward (and hasn't changed):

Napi::Value metadata(const Napi::CallbackInfo& info) {
  ...
  Napi::Object options = info[0].As<Napi::Object>(); // compilation error occurs here

https://github.com/lovell/sharp/blob/flow/src/metadata.cc#L279

I'm still investigating this, but can anyone spot anything wrong with the way sharp is using node-addon-api here that this change might have uncovered?

[JckXia]

Hi @lovell, I believe this particular issue is indeed related to this PR. I dug a little around C2666 on Stackoverflow.

From what I understand, by introducing this conversion operator for napi_callback_info, the MSVC compiler needs to choose between info.operator napi_callback_info()[int] (built-in), and info.operator[](size_t) (custom operator overload). In either variants, 1 step of implicit conversion is required. In the first variant, we need to implicitly convert Napi::CallbackInfo into napi_callback_info, and in the second variant, we need to convert the literal 0 , which is an int into an size_t, thus the ambiguity. A workaround could be to cast the literal into a "size_t" before passing it to info, but I think this is a bug that warrants a change from our side.

[lovell]

@JckXia Thank you very much for investigating, I agree with your findings.

The docs do say the [] operator is for size_t only, so technically sharp has always been wrong (but lucky).

https://github.com/nodejs/node-addon-api/blob/main/doc/callbackinfo.md#operator-

I've unpinned via commit lovell/sharp@bdc50e1 and everything is compiling happily again.

Hopefully someone might be able to work out why the existing node-addon-api tests, which all appear to use literal int to access CallbackInfo, didn't pick up on this. It's possible this PR in its current form might be considered a semver major change.

[JckXia]

Yep I agree @lovell, and thank you for catching this! We have a windows ci pipeline running but for some reason the checks wasn't able to flag the error. Will bring up this issue in our weekly meetings.

[tobias-klein]

I am getting this exact error in the latest Windows CI builds of node-sword-interface.

[mhdawson]

@JckXia do you have any suggestions for a good way to fix the issue? Trying to understand if we should think about backing out the change (although that would potentially be SemVer major as well)

[JckXia]

@mhdawson I think there might be a few options we can explore.

1. Remove the implicit conversion operator and declare the removal change a SemVer-major. Then, instead of using an implicit conversion operator, we can create an accessor function like GetCbInfo. This way people that need the raw napi_callback_info (hopefully not many since it's a fairly new change) still have a means of accessing that pointer. We also get to keep the existing API for [] overload, since a LOT of people (including us) are passing literals into info. The downside is that it seems to break conventions as we have been using implicit conversion operators to translate C++ NAPI objects to C node-API objects. But looking at std::string, there is an explicit getter c_str() function to acquire the C string equivalent makes me think if we should reconsider our approach.

2. Change the parameter type of the CallbackInfo::operator[](size_t index) to be int instead. I believe this way we should be able to avoid the ambiguity in question. But I don't know if this will be a breaking change for anyone because I don't understand all the nuances between size_t and int other than that size_t can potentially represent a greater range of values.

3. Remove the implicit conversion operator completely, but then people who might need the raw napi_callback_info is now out of luck if they had already written code around being able to access this pointer.

On a side note, I couldn't reproduce this error locally using MSVC 2019 C:\Program Files (x86)\Microsoft Visual Studio\2019\Community\MSBuild\Current\Bin\MSBuild.exe. Don't really know why that is. Going to take a deeper dive tomorrow.

[mhdawson]

@JckXia thanks for the outline. I don't know the nuances of the implicit conversions well enough to know of the top of my head but my first thought might be that changing from size_t to int could affect people who had declared index as a size_t.

Let us know what you figure out in the deeper dive.