Fix error in JsProxy_Call kwargs handling (#1033)

docs/changelog.md

@@ -42,6 +42,9 @@
 ### Fixed
 - getattr and dir on JsProxy now report consistent results and include all names defined on the Python dictionary backing JsProxy. [#1017](https://github.com/iodide-project/pyodide/pull/1017)
 - `JsProxy.__bool__` now produces more consistent results: both `bool(window)` and `bool(zero-arg-callback)` were `False` but now are `True`. Conversely, `bool(empty_js_set)` and `bool(empty_js_map)` were `True` but now are `False`. [#1061](https://github.com/iodide-project/pyodide/pull/1061)
+- When calling a javascript function from Python without keyword arguments, Pyodide no longer passes a `PyProxy`-wrapped `NULL` pointer as the last argument.
+[#1033](https://github.com/iodide-project/pyodide/pull/1033)
+
 
 ## Version 0.16.1
 *December 25, 2020*

src/core/error_handling.h

@@ -136,6 +136,13 @@ log_error(char* msg);
     }                                                                          \
   } while (0)
 
+#define FAIL_IF_NONZERO(num)                                                   \
+  do {                                                                         \
+    if ((num) != 0) {                                                          \
+      FAIL();                                                                  \
+    }                                                                          \
+  } while (0)
+
 #define FAIL_IF_ERR_OCCURRED()                                                 \
   do {                                                                         \
     if (PyErr_Occurred()) {                                                    \

src/core/hiwire.c

@@ -377,6 +377,11 @@ EM_JS_NUM(bool, hiwire_is_function, (JsRef idobj), {
   // clang-format on
 });
 
+EM_JS_NUM(bool, hiwire_function_supports_kwargs, (JsRef idfunc), {
+  let funcstr = Module.hiwire.get_value(idfunc).toString();
+  return Module.function_supports_kwargs(funcstr);
+});
+
 EM_JS_NUM(bool, hiwire_is_promise, (JsRef idobj), {
   // clang-format off
   let obj = Module.hiwire.get_value(idobj);

src/core/hiwire.h

@@ -449,6 +449,9 @@ hiwire_get_bool(JsRef idobj);
 bool
 hiwire_is_function(JsRef idobj);
 
+bool
+hiwire_function_supports_kwargs(JsRef idfunc);
+
 /**
  * Returns true if the object is a promise.
  */

src/core/jsproxy.c

@@ -33,6 +33,8 @@ JsBoundMethod_cnew(JsRef this_, const char* name);
 typedef struct
 {
   PyObject_HEAD
+  vectorcallfunc vectorcall;
+  int supports_kwargs; // -1 : don't know. 0 : no, 1 : yes
   JsRef js;
   PyObject* bytes;
   bool awaited; // for promises
@@ -128,41 +130,87 @@ JsProxy_SetAttr(PyObject* self, PyObject* attr, PyObject* pyvalue)
   return success ? 0 : -1;
 }
 
+#define JsProxy_JSREF(x) (((JsProxy*)x)->js)
+#define JsProxy_SUPPORTS_KWARGS(x) (((JsProxy*)x)->supports_kwargs)
+
 static PyObject*
-JsProxy_Call(PyObject* self, PyObject* args, PyObject* kwargs)
-{
+JsProxy_Vectorcall(PyObject* self,
+                   PyObject* const* args,
+                   size_t nargsf,
+                   PyObject* kwnames)
+{
+  bool kwargs = false;
+  if (kwnames != NULL) {
+    // There were kwargs? But maybe kwnames is the empty tuple?
+    PyObject* kwname = PyTuple_GetItem(kwnames, 0); /* borrowed!*/
+    // Clear IndexError
+    PyErr_Clear();
+    if (kwname != NULL) {
+      kwargs = true;
+      if (JsProxy_SUPPORTS_KWARGS(self) == -1) {
+        JsProxy_SUPPORTS_KWARGS(self) =
+          hiwire_function_supports_kwargs(JsProxy_JSREF(self));
+        if (JsProxy_SUPPORTS_KWARGS(self) == -1) {
+          // if it's still -1, hiwire_function_supports_kwargs threw an error.
+          return NULL;
+        }
+      }
+    }
+    if (kwargs && !JsProxy_SUPPORTS_KWARGS(self)) {
+      // We have kwargs but function doesn't support them. Raise error.
+      const char* kwname_utf8 = PyUnicode_AsUTF8(kwname);
+      PyErr_Format(PyExc_TypeError,
+                   "jsproxy got an unexpected keyword argument '%s'",
+                   kwname_utf8);
+      return NULL;
+    }
+  }
+
+  // Recursion error?
+  FAIL_IF_NONZERO(Py_EnterRecursiveCall(" in JsProxy_Vectorcall"));
   bool success = false;
   JsRef idargs = NULL;
-  JsRef idarg = NULL;
   JsRef idkwargs = NULL;
+  JsRef idarg = NULL;
   JsRef idresult = NULL;
-  // result:
-  PyObject* pyresult;
-  Py_ssize_t nargs = PyTuple_Size(args);
 
+  Py_ssize_t nargs = PyVectorcall_NARGS(nargsf);
   idargs = hiwire_array();
+  FAIL_IF_NULL(idargs);
   for (Py_ssize_t i = 0; i < nargs; ++i) {
-    idarg = python2js(PyTuple_GET_ITEM(args, i));
+    idarg = python2js(args[i]);
     FAIL_IF_NULL(idarg);
     FAIL_IF_MINUS_ONE(hiwire_push_array(idargs, idarg));
     hiwire_CLEAR(idarg);
   }
 
-  if (PyDict_Size(kwargs)) {
-    idkwargs = python2js(kwargs);
+  if (kwargs) {
+    // store kwargs into an object which we'll use as the last argument.
+    idkwargs = hiwire_object();
+    FAIL_IF_NULL(idkwargs);
+    Py_ssize_t nkwargs = PyTuple_Size(kwnames);
+    for (Py_ssize_t i = 0, k = nargsf; i < nkwargs; ++i, ++k) {
+      PyObject* name = PyTuple_GET_ITEM(kwnames, i); /* borrowed! */
+      const char* name_utf8 = PyUnicode_AsUTF8(name);
+      idarg = python2js(args[k]);
+      FAIL_IF_NULL(idarg);
+      FAIL_IF_MINUS_ONE(hiwire_set_member_string(idkwargs, name_utf8, idarg));
+      hiwire_CLEAR(idarg);
+    }
     FAIL_IF_MINUS_ONE(hiwire_push_array(idargs, idkwargs));
   }
 
-  idresult = hiwire_call(JsProxy_REF(self), idargs);
+  idresult = hiwire_call(JsProxy_JSREF(self), idargs);
   FAIL_IF_NULL(idresult);
-  pyresult = js2python(idresult);
+  PyObject* pyresult = js2python(idresult);
   FAIL_IF_NULL(pyresult);
 
   success = true;
 finally:
+  Py_LeaveRecursiveCall(/* " in JsProxy_Vectorcall" */);
   hiwire_CLEAR(idargs);
-  hiwire_CLEAR(idarg);
   hiwire_CLEAR(idkwargs);
+  hiwire_CLEAR(idarg);
   hiwire_CLEAR(idresult);
   if (!success) {
     Py_CLEAR(pyresult);
@@ -563,12 +611,13 @@ static PyTypeObject JsProxyType = {
   .tp_name = "JsProxy",
   .tp_basicsize = sizeof(JsProxy),
   .tp_dealloc = (destructor)JsProxy_dealloc,
-  .tp_call = JsProxy_Call,
+  .tp_call = PyVectorcall_Call,
+  .tp_vectorcall_offset = offsetof(JsProxy, vectorcall),
   .tp_getattro = JsProxy_GetAttr,
   .tp_setattro = JsProxy_SetAttr,
   .tp_as_async = &JsProxy_asyncMethods,
   .tp_richcompare = JsProxy_RichCompare,
-  .tp_flags = Py_TPFLAGS_DEFAULT,
+  .tp_flags = Py_TPFLAGS_DEFAULT | _Py_TPFLAGS_HAVE_VECTORCALL,
   .tp_doc = "A proxy to make a Javascript object behave like a Python object",
   .tp_methods = JsProxy_Methods,
   .tp_getset = JsProxy_GetSet,
@@ -585,8 +634,10 @@ JsProxy_cnew(JsRef idobj)
 {
   JsProxy* self;
   self = (JsProxy*)JsProxyType.tp_alloc(&JsProxyType, 0);
+  self->vectorcall = JsProxy_Vectorcall;
   self->js = hiwire_incref(idobj);
   self->bytes = NULL;
+  self->supports_kwargs = -1; // don't know
   self->awaited = false;
   return (PyObject*)self;
 }

src/pyodide.js

@@ -365,6 +365,95 @@ globalThis.languagePluginLoader = new Promise((resolve, reject) => {
     return Module.runPython(code);
   };
 
+  Module.function_supports_kwargs = function(funcstr) {
+    // This is basically a finite state machine (except for paren counting)
+    // Start at beginning of argspec
+    let idx = funcstr.indexOf("(") + 1;
+    // States:
+    // START_ARG -- Start of an argument. We leave this state when we see a non
+    // whitespace character.
+    //    If the first nonwhitespace character we see is `{` this is an object
+    //    destructuring argument. Else it's not. When we see non whitespace goto
+    //    state ARG and set `arg_is_obj_dest` true if it's "{", else false.
+    // ARG -- we're in the middle of an argument. Count parens. On comma, if
+    // parens_depth === 0 goto state START_ARG, on quote set
+    //      set quote_start and goto state QUOTE.
+    // QUOTE -- We're in a quote. Record quote_start in quote_start and look for
+    // a matching end quote.
+    //    On end quote, goto state ARGS. If we see "\\" goto state QUOTE_ESCAPE.
+    // QUOTE_ESCAPE -- unconditionally goto state QUOTE.
+    // If we see a ) when parens_depth === 0, return arg_is_obj_dest.
+    let START_ARG = 1;
+    let ARG = 2;
+    let QUOTE = 3;
+    let QUOTE_ESCAPE = 4;
+    let paren_depth = 0;
+    let arg_start = 0;
+    let arg_is_obj_dest = false;
+    let quote_start = undefined;
+    let state = START_ARG;
+    // clang-format off
+    for (i = idx; i < funcstr.length; i++) {
+      let x = funcstr[i];
+      if(state === QUOTE){
+        switch(x){
+          case quote_start:
+            // found match, go back to ARG
+            state = ARG;
+            continue;
+          case "\\":
+            state = QUOTE_ESCAPE;
+            continue;
+          default:
+            continue;
+        }
+      }
+      if(state === QUOTE_ESCAPE){
+        state = QUOTE;
+        continue;
+      }
+      // Skip whitespace.
+      if(x === " " || x === "\n" || x === "\t"){
+        continue;
+      }
+      if(paren_depth === 0){
+        if(x === ")" && state !== QUOTE && state !== QUOTE_ESCAPE){
+          // We hit closing brace which ends argspec.
+          // We have to handle this up here in case argspec ends in a trailing comma
+          // (if we're in state START_ARG, the next check would clobber arg_is_obj_dest).
+          return arg_is_obj_dest;
+        }
+        if(x === ","){
+          state = START_ARG;
+          continue;
+        }
+        // otherwise fall through
+      }
+      if(state === START_ARG){
+        // Nonwhitespace character in START_ARG so now we're in state arg.
+        state = ARG;
+        arg_is_obj_dest = x === "{";
+        // don't continue, fall through to next switch
+      }
+      switch(x){
+        case "[": case "{": case "(":
+          paren_depth ++;
+          continue;
+        case "]": case "}": case ")":
+          paren_depth--;
+          continue;
+        case "'": case '"': case '\`':
+          state = QUOTE;
+          quote_start = x;
+          continue;
+      }
+    }
+    // Correct exit is paren_depth === 0 && x === ")" test above.
+    throw new Error("Assertion failure: this is a logic error in \
+                     hiwire_function_supports_kwargs");
+    // clang-format on
+  };
+
   Module.locateFile = (path) => baseURL + path;
   Module.postRun = async () => {
     Module.version = Module.pyodide_py.__version__;