mmomtchev / swig

This is SWIG JavaScript Evolution, a fork of the SWIG project with modern JavaScript/TypeScript support including WASM and async
http://www.swig.org
Other
13 stars 1 forks source link

Incorrect output types when wrapping non-const references #68

Open mateusz-plociennik opened 2 months ago

mateusz-plociennik commented 2 months ago

I am encountering issues when wrapping the following methods with SWIG:

// Retrieves the visible widget tree as an XML document
void        getWidgetTree(uint32_t displayIndex, void* buffer, uint32_t size, uint32_t& writtenSize, bool allowInvisible = false);
// Retrieves the value of a widget property
bool        getWidgetProperty(uint32_t displayIndex, uint32_t widgetPtr, const std::string& propertyName, std::string& propertyValue);
// Sets the value of a widget property
bool        setWidgetProperty(uint32_t displayIndex, uint32_t widgetPtr, const std::string& propertyName, const std::string& propertyValue);

When wrapped using the following code:

%include <arraybuffer.i>
%typemap(in)        (void* buffer, uint32_t size) = WRITABLE_BUFFER_SIGNATURE;
%typemap(typecheck) (void* buffer, uint32_t size) = WRITABLE_BUFFER_SIGNATURE;
%include <std_string.i>

The generated TypeScript definitions look like this:

getWidgetTree(displayIndex: number, buffer: any, writtenSize: any, allowInvisible?: boolean): void;
getWidgetTree(displayIndex: number, buffer: any, writtenSize?: any): void;
getWidgetProperty(displayIndex: number, widgetPtr: number, propertyName: string, propertyValue: string): boolean;
setWidgetProperty(displayIndex: number, widgetPtr: number, propertyName: string, propertyValue: string): boolean;

To handle non-const reference types (which I treat as outputs in my codebase), I used the following %apply directives:

%apply uint32_t& OUTPUT { uint32_t& };
%apply std::string& OUTPUT { std::string& propertyValue };

However, this results in the following incorrect TypeScript output:

getWidgetTree(displayIndex: number, buffer: any, allowInvisible?: boolean): number[];
getWidgetTree(displayIndex: number, buffer: any): number[];
getWidgetProperty(displayIndex: number, widgetPtr: number, propertyName: string): string[];
setWidgetProperty(displayIndex: number, widgetPtr: number, propertyName: string, propertyValue: string): string[];

The output types are incorrect (e.g., returning string[] instead of string), though the JavaScript wrapper works somewhat as expected. Additionally, I occasionally encounter the following memory leak warning:

Warning, SWIG cannot delete an object of type _p_std__string, it does not have a destructor. This is a memory leak.
mmomtchev commented 2 months ago

First of all, as you may have seen from my profile page, I have been unemployed and living on social welfare because of galactic extortion organized with the help of the French police, corrupted judges, some of the biggest IT companies in the world and various open-source communities. In order to intimidate me into accepting to stop talking about this affair, people are posting simultaneous comments and yours is one of these. I don't know what you expect to accomplish this way, but I guarantee you that for this affair to continue this long, not one of the people before you succeeded in this endeavor. Yesterday, the state French unemployment office tried offering me a (obviously imaginary, but it wouldn't have changed anything) a VP of engineering position if I was to shut up. I can probably see how they believed that this could work, but frankly, I am at loss to explain how you believe that asking me questions about SWIG could eventually tip the balance.

mmomtchev commented 2 months ago

The semantics of the buffer used for

void        getWidgetTree(uint32_t displayIndex, void* buffer, uint32_t size, uint32_t& writtenSize, bool allowInvisible = false);

are not entirely clear. Is it a buffer allocated by the caller? Is its initial size size and then the calling function changes it to the amount of bytes written? In this case, this would need a new typemap since it is not covered by the existing three typemaps which do not handle this case.

mmomtchev commented 2 months ago
// Retrieves the value of a widget property
bool        getWidgetProperty(uint32_t displayIndex, uint32_t widgetPtr, const std::string& propertyName, std::string& propertyValue);
// Sets the value of a widget property
bool        setWidgetProperty(uint32_t displayIndex, uint32_t widgetPtr, const std::string& propertyName, const std::string& propertyValue);

You have multiple string arguments and you are transforming them all. SWIG will fall-back to const/non-const so you cannot distinguish them only by constness. You have to use the argument name - and in this case it will be a problem since it is the same. You can create/apply the typemap before each function definition.

mmomtchev commented 2 months ago

I will have to see the whole code for the warning. It usually means that SWIG is trying to wrap an object that it was not supposed to. A string should never be wrapped anyway, there are special types for strings in std_string.i, you should always include it when handling std::string.

mateusz-plociennik commented 2 months ago

First of all, I’m really grateful to have found your article on Medium, which led me to this repository. SWIG has a steep learning curve, but once everything is set up, adding new includes or functionality becomes a piece of cake. Thanks to SWIG and your fork, I saved weeks of tedious work that would have been spent writing C DLL interfaces and FFI wrappers. I am more than happy to spend a dime. How can I reach you via DM?

  1. I wanted to provide a minimal example, but I realize I included some irrelevant parts. To clarify, you need to provide a preallocated memory buffer, with void* buffer as its address, uint32_t size as its size, and uint32_t& writtenSize as the actual data size returned by the function call. However, I recognize that part as already completed.

  2. The C++ code I shared is also being generated, and the const-ness of parameters is enforced based on the direction specified for each parameter in the IDL file. To be honest, I was hoping you had enough knowledge of SWIG internals to suggest a hack that could make all non-const references act as outputs. I also wanted to avoid specifying typemaps for every function declaration, as I use the %include clause to include several APIs that can change over time.

  3. For example, with setWidgetProperty(), I’m getting [true, [object Object]] as a return value instead of just a boolean:

    
    // js_function
    template <typename SWIG_OBJ_WRAP>
    Napi::Value _exports_BluiCommon_templ<SWIG_OBJ_WRAP>::_wrap_BluiCommon_setWidgetProperty(const Napi::CallbackInfo &info) {
    Napi::Env env = info.Env();
    Napi::Value jsresult;
    Blui::RPC::BluiCommon *arg1 = (Blui::RPC::BluiCommon *) 0 ;
    uint32_t arg2 ;
    uint32_t arg3 ;
    std::string *arg4 = 0 ;
    std::string *arg5 = 0 ;
    void *argp1 = 0 ;
    int res1 = 0 ;
    unsigned int val2 ;
    int ecode2 = 0 ;
    unsigned int val3 ;
    int ecode3 = 0 ;
    int res4 = SWIG_OLDOBJ ;
    int res5 = SWIG_OLDOBJ ;
    bool result;

ifdef NAPI_CPP_EXCEPTIONS

try {

endif

if(static_cast<int>(info.Length()) < 4 || static_cast<int>(info.Length()) > 4) {
  SWIG_Error(SWIG_ERROR, "Illegal number of arguments for _wrap_BluiCommon_setWidgetProperty.");
}

res1 = SWIG_ConvertPtr(info.This(), &argp1,SWIGTYPE_p_Blui__RPC__BluiCommon, 0 |  0 );
if (!SWIG_IsOK(res1)) {
  SWIG_exception_fail(SWIG_ArgError(res1), "in method '" "BluiCommon_setWidgetProperty" "', argument " "1"" of type '" "Blui::RPC::BluiCommon *""'"); 
}
arg1 = reinterpret_cast< Blui::RPC::BluiCommon * >(argp1);ecode2 = SWIG_AsVal_unsigned_SS_int(info[0], &val2);
if (!SWIG_IsOK(ecode2)) {
  SWIG_exception_fail(SWIG_ArgError(ecode2), "in method '" "BluiCommon_setWidgetProperty" "', argument " "2"" of type '" "uint32_t""'");
} 
arg2 = static_cast< uint32_t >(val2);ecode3 = SWIG_AsVal_unsigned_SS_int(info[1], &val3);
if (!SWIG_IsOK(ecode3)) {
  SWIG_exception_fail(SWIG_ArgError(ecode3), "in method '" "BluiCommon_setWidgetProperty" "', argument " "3"" of type '" "uint32_t""'");
} 
arg3 = static_cast< uint32_t >(val3);{
  {
    std::string *ptr = (std::string *)0;
    res4 = SWIG_AsPtr_std_string(info[2], &ptr);
    if (!SWIG_IsOK(res4)) {
      SWIG_exception_fail(SWIG_ArgError(res4), "in method '" "BluiCommon_setWidgetProperty" "', argument " "4"" of type '" "std::string const &""'"); 
    }
    if (!ptr) {
      SWIG_exception_fail(SWIG_ValueError, "invalid null reference " "in method '" "BluiCommon_setWidgetProperty" "', argument " "4"" of type '" "std::string const &""'"); 
    }
    arg4 = ptr;
  }
}
{
  {
    std::string *ptr = (std::string *)0;
    res5 = SWIG_AsPtr_std_string(info[3], &ptr);
    if (!SWIG_IsOK(res5)) {
      SWIG_exception_fail(SWIG_ArgError(res5), "in method '" "BluiCommon_setWidgetProperty" "', argument " "5"" of type '" "std::string const &""'"); 
    }
    if (!ptr) {
      SWIG_exception_fail(SWIG_ValueError, "invalid null reference " "in method '" "BluiCommon_setWidgetProperty" "', argument " "5"" of type '" "std::string const &""'"); 
    }
    arg5 = ptr;
  }
}

{
  try {
    result = (bool)(arg1)->setWidgetProperty(arg2,arg3,(std::string const &)*arg4,(std::string const &)*arg5);
  }
  /*@SWIG:/Users/mateuszp/Components/SWIG-5.0.3+jse/bin/mac/share/swig/5.0.3/typemaps/exception.swg,59,SWIG_CATCH_STDEXCEPT@*/  /* catching std::exception  */
  catch (std::invalid_argument& e) {
    SWIG_exception_fail(SWIG_ValueError, e.what() );
  } catch (std::domain_error& e) {
    SWIG_exception_fail(SWIG_ValueError, e.what() );
  } catch (std::overflow_error& e) {
    SWIG_exception_fail(SWIG_OverflowError, e.what() );
  } catch (std::out_of_range& e) {
    SWIG_exception_fail(SWIG_IndexError, e.what() );
  } catch (std::length_error& e) {
    SWIG_exception_fail(SWIG_IndexError, e.what() );
  } catch (std::runtime_error& e) {
    SWIG_exception_fail(SWIG_RuntimeError, e.what() );
  } catch (std::exception& e) {
    SWIG_exception_fail(SWIG_SystemError, e.what() );
  }
  /*@SWIG@*/
  catch (...) {
    SWIG_exception_fail(SWIG_UnknownError, "Unknown exception");
  }
}

jsresult = SWIG_From_bool  SWIG_NAPI_FROM_CALL_ARGS(static_cast< bool >(result));

if (SWIG_IsNewObj(res4)) delete arg4;
if (SWIG_IsNewObj(res5)) delete arg5;

return jsresult;

ifdef NAPI_CPP_EXCEPTIONS

} catch (...) { if (SWIG_IsNewObj(res4)) delete arg4; if (SWIG_IsNewObj(res5)) delete arg5;

std::rethrow_exception(std::current_exception());

}

else

goto fail; fail: if (SWIG_IsNewObj(res4)) delete arg4; if (SWIG_IsNewObj(res5)) delete arg5;

endif

return Napi::Value(); }



4. The bug I reported is mainly about the types declared in the `d.ts` file. It seems that once I add my `%apply` clauses, the TypeScript return types go bonkers. That’s why I suspect I’m doing something wrong, but I have no idea how to fix it.
mmomtchev commented 2 months ago

Oh, you read the medium article? For a moment I though that since you were posting simultaneously with other projects where this has happened, that you had something to do with the guy at the origin of this extortion, but in fact, you simply read the medium article.

As far as I am concerned, the TypeScript behavior is correct since you transform multiple input arguments into output arguments and thus you get an array of strings.

mateusz-plociennik commented 2 months ago

If return is a Boolean and one output argument is string I would expect a pair of those, not an array of strings.

mmomtchev commented 2 months ago

There is an explicit comment about this situation in one of the unit tests, maybe this is why you are posting this issue: https://github.com/mmomtchev/swig/blob/71ab4d754495321b713a355c651b14907db7e196/Examples/test-suite/javascript/li_typemaps_runme.js#L48

I will have to write a very complex TypeScript type manipulation that actually parses the types to solve what is not going to be an usual situation because in the real world, this function should return a single value.

If you indeed want it to return exactly this, then you can define custom typemaps:

%typemap(tsout) std::string& propertyValue "[boolean, string]";
%typemap(tsout) const std::string& propertyValue "boolean";