emscripten-core / emscripten

Emscripten: An LLVM-to-WebAssembly Compiler
Other
25.45k stars 3.25k forks source link

Overloads, bound types, and native types confusion #20117

Open fraguada opened 11 months ago

fraguada commented 11 months ago

Version of emscripten/emsdk: emcc (Emscripten gcc/clang-like replacement + linker emulating GNU ld) 3.1.30 (cfe2bdfe2692457cb5f5770672f6e5ccb3ffc2f2) clang version 16.0.0 (https://github.com/llvm/llvm-project 800f0f1546b2352ba42a4777149afb13cb874fcd) Target: wasm32-unknown-emscripten Thread model: posix InstalledDir: /emsdk/upstream/bin

Issue I am trying to take advantage of overloading a series of methods that have the same return type (void) but different arguments. Historically we've avoided this because when we first started using emscripten, it was not supported. Revisiting it now, it seems it is almost working, but I have a case where it is not.

Our project links our OpenNURBS library with some c++ bindings, and generates wasm. This has been working well, in general. There are several situations where it would be nice to use overloaded functions.

Here is an example where I am trying to use overloaded functions: https://github.com/mcneel/rhino3dm/blob/dev/src/bindings/bnd_pointcloud.h#L51

The methods I would like to overload (before I was calling them Add1, Add2, etc):

  void Add(ON_3dPoint point);
  void Add(ON_3dPoint point, double value);
  void Add(ON_3dPoint point, ON_3dVector normal);
  void Add(ON_3dPoint point, BND_Color color);
  void Add(ON_3dPoint point, ON_3dVector normal, BND_Color color);

As you see, there are arguments with built in types (double), native types (ON_3dPoint, ON_3dVector), and bound types (BND_Color).

The methods are bound:

class_<BND_PointCloud, base<BND_GeometryBase>>("PointCloud")
    .constructor<>()
    // ... //
    .function("add", select_overload<void(ON_3dPoint)>(&BND_PointCloud::Add))
    .function("add", select_overload<void(ON_3dPoint, double)>(&BND_PointCloud::Add) )
    .function("add", select_overload<void(ON_3dPoint, ON_3dVector)>(&BND_PointCloud::Add) 
    .function("add", select_overload<void(ON_3dPoint, BND_Color)>(&BND_PointCloud::Add) )
    .function("add", select_overload<void(ON_3dPoint, ON_3dVector, BND_Color)>(&BND_PointCloud::Add) )
    // ... //
    ;

The issue is specifically between

When trying to use the add method that takes ON_3dPoint, ON_3dVector OR ON_3dPoint, double, the code goes to the method that uses ON_3dPoint, BND_Color. These all have 2 args, where the second arg is what is changing.

When trying to debug, I notice that the overload table only has 3 methods, when in actuality there are 5 (although the array las a length of 4):

image

So it seems that the method taking ON_3dPoint, ON_3dVector and ON_3dPoint, double is somehow skipped and thus we run the wrong method and get an exception because of the wrong type.

The specific error:

TypeError: Cannot convert "undefined" to int
    at checkAssertions (rhino3dm.module.js:6644:17)
    at Object.toWireType (rhino3dm.module.js:6658:11)
    at __emval_as (rhino3dm.module.js:7110:38)
    at int emscripten::val::as<int>() int emscripten::val::as<int>() const (val.h:544)
    at Binding_to_ON_Color(emscripten::val Binding_to_ON_Color(emscripten::val const&) (bnd_color.cpp:49)
    at BND_PointCloud::Add(ON_3dPoint, BND_PointCloud::Add(ON_3dPoint, emscripten::val) (bnd_pointcloud.cpp:248)
    at emscripten::internal::MethodInvoker<void (BND_PointCloud::*)(ON_3dPoint, emscripten::val), void, BND_PointCloud*, ON_3dPoint, emscripten::val>::invoke(void (BND_PointCloud::* const&)(ON_3dPoint, emscripten::val), BND_PointCloud*, ON_3dPoint*, emscripten::internal::MethodInvoker<void (BND_PointCloud::*)(ON_3dPoint, emscripten::val), void, BND_PointCloud*, ON_3dPoint, emscripten::val>::invoke(void (BND_PointCloud::* const&)(ON_3dPoint, emscripten::val), BND_PointCloud*, ON_3dPoint*, emscripten::_EM_VAL*) (bind.h:600)
    at PointCloud.PointCloud$add (eval at new_ (rhino3dm.module.js:6032:27), <anonymous>:11:1)
    at proto.<computed> [as add] (rhino3dm.module.js:5529:68)
    at script.js:12:8

The reason it is Cannot convert "undefined" to int I suppose is because the type it is expecting is a color, and it is trying to access a color channel which is int.

What could I do to further debug this issue, or to provide any information that might help others understand what is going on? I have seem older issues that deal with overloading, and their progress sounded promising, but it seems that there are still issues with the overloading code. Any suggestions would be greatly appreciated!

brendandahl commented 11 months ago

Overloading is only partially supported and it only works on the number of arguments not the types. We could probably support this, but there's no efficient way to do this in JS.

brendandahl commented 11 months ago

I thought we had an assertion to verify that we don't try to overload the same method twice with the same argument count, but I'm not seeing anything.

fraguada commented 11 months ago

@brendandahl thank you for the confirmation!

I couldn't find any documentation related to that so thought I was doing something wrong. Indeed I noticed that we were passing by

return proto[methodName].overloadTable[arguments.length].apply(this, arguments);

and thought that most likely it is just going by number of args.

For now I will go back to our previous strategy of naming each method something different.

I would definitely add a vote to be able to overload with the same number of arguments, with different argument types if there isn't too much of an overhead on the resulting wasm.

fraguada commented 11 months ago

I see the assertion now, but for overloaded constructors: Cannot register multiple constructors with identical number of parameters

BindingError: Cannot register multiple constructors with identical number of parameters (1) for class 'Point3dList'! Overload resolution is currently only performed using the parameter count, not actual type info!

frank-pian commented 10 months ago

I see that opencascade.js overloads the constructor using a different class name. But I can only bind the same class once, @donalffons can you tell us exactly how it is done.