[JS] Use CanCastAsInteger in JSC and V8

[mmomtchev]

As everything is a floating point number in JS, use CanCastAsInteger to identify integer values.
Additionally, treat an OverflowError as a TypeError when overloading to allow (some form) of overloading between integers and floating point values.

[ojwb]

I'm wondering about what I'd actually want to happen when wrapping a C++ API with overloads between double and integer types.

One pattern that comes to mind is the two overloads provide the same functionality and essentially exist to avoid forcing the caller to convert an integer to a double if they have an integer to start with, so if we already have a double (as we do in SWIG/Javascript) just calling the double overload seems ressonable, and doing work to decide if that double could be passed as an integer seems like unnecessary complication.

The other realistic pattern I came up with was serialisation, e.g:

send(int i);
send(double d);

If whatever takes the serialised data expects a particular type in a particular context we need exact control over which overload gets called and to wrap this usably for Javascript you'd used %rename to wrap as e.g. send_int() and send_double(). If the serialised data is self-describing, we're mostly back in the first situation and calling the double overload is simpler and more predictable.

I wondered what SWIG/Lua does since Lua only recently gained an actual integer type which SWIG doesn't yet know about that (#1150), but hacking up a quick overload_numeric_runme.lua it seems Lua overload handling is rather broken so it's not a helpful case study.

I'm not sure if any other target languages are "double-only".

@wsfulton What do you think?

[ojwb]

Not exactly the same situation admittedly, but some other target languages have a double type but no float and AFAIK we don't try to resolve float vs double overloads for any of them based on whether the passed value is exactly representable as a float.

[wsfulton]

SWIG has a mechanism for handling overloading of numeric types described in https://swig.org/Doc4.1/SWIGPlus.html#SWIGPlus_overloaded_methods. Javascript does not seem to implement the type precedence via the typecheck typemap for this to work. Once that is fixed, this patch is most likely not needed.

I do note that Python uses a SWIG_PYTHON_CAST_MODE macro which then enables CanCastAsInteger as a (minor) part of Python's -castmode implementation for implicit type casting.

v8 has the Value::IsInt32() function to determine if a Javascript number is an integer and could be utilised as follows:

diff --git a/Lib/javascript/v8/javascriptprimtypes.swg b/Lib/javascript/v8/javascriptprimtypes.swg
index 8ed571df1..ede171909 100644
--- a/Lib/javascript/v8/javascriptprimtypes.swg
+++ b/Lib/javascript/v8/javascriptprimtypes.swg
@@ -44,6 +44,9 @@ int SWIG_AsVal_dec(int)(SWIGV8_VALUE valRef, int* val)
   if (!valRef->IsNumber()) {
     return SWIG_TypeError;
   }
+  if (!valRef->IsInt32()) {
+    return SWIG_TypeError;
+  }
   if(val) *val = SWIGV8_INTEGER_VALUE(valRef);
 
   return SWIG_OK;

While the internal representation of scripting languages might be by double only or not, there are usually methods to work out if the type is a double or an integer like IsInt32(). I didn't look for jsc nor napi equivalents, but hopefully there are some.

With the above patch and if the C++ methods are declared in the correct order (that the missing dispatch ranking would fix), then overloading with integer and double works. For example:

void send(int i) { std::cout << "int " << i << std::endl; }
void send(double i) { std::cout << "double " << i << std::endl; }

Above will call int overload with send(123) and will call the double overload with send(123.4). But with the following:

void send(double i) { std::cout << "double " << i << std::endl; }
void send(int i) { std::cout << "int " << i << std::endl; }

only the double overload will be used. Once the typecheck typemaps are used in the Javascript implementation it won't matter what order the c++ code is declared in.

overload_numeric is a relevant testcase for testing an implementation, but it could do with extending by adding in a different order of the over methods to better test the cast ranking.
overload_bool is another test that could be used.

[mmomtchev]

Ok, I will consider adding the typecheck typemaps but this is a substantial modification as it has to be made across the three backends (JSC, V8, NAPI). Let's first merge the current PRs to avoid huge conflicts.

I am afraid that there is no generic way to find if a V8 number is an integer except to use CanCastAsInteger. Still, for cases where there are simple primitives, I will use them.

[wsfulton]

Okay, let's come back to this after #2545 is merged.

[mmomtchev]

@wsfulton There is one major problem with adding typecheck typemaps to JS - they will have to be mandatory. This means that a new SWIG release that adds typecheck typemaps will break every single user project that uses custom typemaps. Is this ok?

[wsfulton]

The typecheck typemaps are fundamental to overloading, it'll have to be okay. We do make changes that may require extra work after a release especially when fixing bugs like this. It will of course need documenting with some helpful advice added to the changes file.