Merge pull request #2949 from rjtobin/posonly_opt

Positional-only code generation optimization

Cython/Compiler/Nodes.py

@@ -3661,6 +3661,7 @@ class DefNodeWrapper(FuncDefNode):
         positional_args = []
         required_kw_only_args = []
         optional_kw_only_args = []
+        num_pos_only_args = 0
         for arg in args:
             if arg.is_generic:
                 if arg.default:
@@ -3673,6 +3674,8 @@ class DefNodeWrapper(FuncDefNode):
                     required_kw_only_args.append(arg)
                 elif not arg.is_self_arg and not arg.is_type_arg:
                     positional_args.append(arg)
+                if arg.pos_only:
+                    num_pos_only_args += 1
 
         # sort required kw-only args before optional ones to avoid special
         # cases in the unpacking code
@@ -3691,10 +3694,11 @@ class DefNodeWrapper(FuncDefNode):
 
         code.putln('{')
         all_args = tuple(positional_args) + tuple(kw_only_args)
+        non_posonly_args = [arg for arg in all_args if not arg.pos_only]
         code.putln("static PyObject **%s[] = {%s};" % (
             Naming.pykwdlist_cname,
             ','.join(['&%s' % code.intern_identifier(arg.name)
-                      for arg in all_args if not arg.pos_only] + ['0'])))
+                      for arg in non_posonly_args] + ['0'])))
 
         # Before being converted and assigned to the target variables,
         # borrowed references to all unpacked argument values are
@@ -3706,13 +3710,41 @@ class DefNodeWrapper(FuncDefNode):
         # was passed for them.
         self.generate_argument_values_setup_code(all_args, code)
 
+        # If all args are positional-only, we can raise an error
+        # straight away if we receive a non-empty kw-dict.
+        # This requires a PyDict_Size call.  This call is wasteful
+        # for functions which do accept kw-args, so we do not generate
+        # the PyDict_Size call unless all args are positional-only.
+        accept_kwd_args = non_posonly_args or self.starstar_arg
+        if accept_kwd_args:
+            kw_unpacking_condition = Naming.kwds_cname
+        else:
+            kw_unpacking_condition = "%s && PyDict_Size(%s) > 0" % (
+                Naming.kwds_cname, Naming.kwds_cname)
+
         # --- optimised code when we receive keyword arguments
         code.putln("if (%s(%s)) {" % (
             (self.num_required_kw_args > 0) and "likely" or "unlikely",
-            Naming.kwds_cname))
-        self.generate_keyword_unpacking_code(
-            min_positional_args, max_positional_args,
-            has_fixed_positional_count, has_kw_only_args, all_args, argtuple_error_label, code)
+            kw_unpacking_condition))
+
+        if accept_kwd_args:
+            self.generate_keyword_unpacking_code(
+                min_positional_args, max_positional_args,
+                has_fixed_positional_count, has_kw_only_args, all_args, argtuple_error_label, code)
+        else:
+            # Here we do not accept kw-args but we are passed a non-empty kw-dict.
+            # We call ParseOptionalKeywords which will raise an appropriate error if
+            # the kw-args dict passed is non-empty (which it will be, since kw_unpacking_condition is true)
+            code.globalstate.use_utility_code(
+                UtilityCode.load_cached("ParseKeywords", "FunctionArguments.c"))
+            code.putln('if (likely(__Pyx_ParseOptionalKeywords(%s, %s, %s, %s, %s, "%s") < 0)) %s' % (
+                Naming.kwds_cname,
+                Naming.pykwdlist_cname,
+                self.starstar_arg and self.starstar_arg.entry.cname or '0',
+                'values',
+                0,
+                self.name,
+                code.error_goto(self.pos)))
 
         # --- optimised code when we do not receive any keyword arguments
         if (self.num_required_kw_args and min_positional_args > 0) or min_positional_args == max_positional_args:
@@ -3877,18 +3909,42 @@ class DefNodeWrapper(FuncDefNode):
     def generate_keyword_unpacking_code(self, min_positional_args, max_positional_args,
                                         has_fixed_positional_count,
                                         has_kw_only_args, all_args, argtuple_error_label, code):
+        # First we count how many arguments must be passed as positional
+        num_required_posonly_args = num_pos_only_args = 0
+        for i, arg in enumerate(all_args):
+            if arg.pos_only:
+                num_pos_only_args += 1
+                if not arg.default:
+                    num_required_posonly_args += 1
+
         code.putln('Py_ssize_t kw_args;')
         code.putln('const Py_ssize_t pos_args = PyTuple_GET_SIZE(%s);' % Naming.args_cname)
         # copy the values from the args tuple and check that it's not too long
         code.putln('switch (pos_args) {')
         if self.star_arg:
             code.putln('default:')
-        for i in range(max_positional_args-1, -1, -1):
+
+        for i in range(max_positional_args-1, num_required_posonly_args-1, -1):
             code.put('case %2d: ' % (i+1))
             code.putln("values[%d] = PyTuple_GET_ITEM(%s, %d);" % (
                 i, Naming.args_cname, i))
             code.putln('CYTHON_FALLTHROUGH;')
-        code.putln('case  0: break;')
+        if num_required_posonly_args > 0:
+            code.put('case %2d: ' % num_required_posonly_args)
+            for i in range(num_required_posonly_args-1, -1, -1):
+                code.putln("values[%d] = PyTuple_GET_ITEM(%s, %d);" % (
+                    i, Naming.args_cname, i))
+            code.putln('break;')
+        for i in range(num_required_posonly_args-2, -1, -1):
+            code.put('case %2d: ' % (i+1))
+            code.putln('CYTHON_FALLTHROUGH;')
+
+        code.put('case  0: ')
+        if num_required_posonly_args == 0:
+            code.putln('break;')
+        else:
+            # catch-all for not enough pos-only args passed
+            code.put_goto(argtuple_error_label)
         if not self.star_arg:
             code.put('default: ') # more arguments than allowed
             code.put_goto(argtuple_error_label)
@@ -3906,31 +3962,22 @@ class DefNodeWrapper(FuncDefNode):
         code.putln('kw_args = PyDict_Size(%s);' % Naming.kwds_cname)
         if self.num_required_args or max_positional_args > 0:
             last_required_arg = -1
-            last_required_posonly_arg = -1
             for i, arg in enumerate(all_args):
                 if not arg.default:
                     last_required_arg = i
-                if arg.pos_only and not arg.default:
-                    last_required_posonly_arg = i
             if last_required_arg < max_positional_args:
                 last_required_arg = max_positional_args-1
-            if max_positional_args > 0:
+            if max_positional_args > num_pos_only_args:
                 code.putln('switch (pos_args) {')
-            for i, arg in enumerate(all_args[:last_required_arg+1]):
-                if max_positional_args > 0 and i <= max_positional_args:
-                    if i != 0:
+            for i, arg in enumerate(all_args[num_pos_only_args:last_required_arg+1], num_pos_only_args):
+                if max_positional_args > num_pos_only_args and i <= max_positional_args:
+                    if i != num_pos_only_args:
                         code.putln('CYTHON_FALLTHROUGH;')
                     if self.star_arg and i == max_positional_args:
                         code.putln('default:')
                     else:
                         code.putln('case %2d:' % i)
                 pystring_cname = code.intern_identifier(arg.name)
-                if arg.pos_only:
-                    if i == last_required_posonly_arg:
-                        code.put_goto(argtuple_error_label)
-                    elif i == last_required_arg:
-                        code.putln('break;')
-                    continue
                 if arg.default:
                     if arg.kw_only:
                         # optional kw-only args are handled separately below
@@ -3969,7 +4016,7 @@ class DefNodeWrapper(FuncDefNode):
                             self.name, pystring_cname))
                         code.putln(code.error_goto(self.pos))
                         code.putln('}')
-            if max_positional_args > 0:
+            if max_positional_args > num_pos_only_args:
                 code.putln('}')
 
         if has_kw_only_args:
@@ -3989,7 +4036,6 @@ class DefNodeWrapper(FuncDefNode):
         # ParseOptionalKeywords() needs to know how many of the arguments
         # that could be passed as keywords have in fact been passed as
         # positional args.
-        num_pos_only_args = self.num_posonly_args
         if num_pos_only_args > 0:
             # There are positional-only arguments which we don't want to count,
             # since they cannot be keyword arguments.  Subtract the number of
@@ -4033,14 +4079,17 @@ class DefNodeWrapper(FuncDefNode):
     def generate_optional_kwonly_args_unpacking_code(self, all_args, code):
         optional_args = []
         first_optional_arg = -1
+        num_posonly_args = 0
         for i, arg in enumerate(all_args):
+            if arg.pos_only:
+                num_posonly_args += 1
             if not arg.kw_only or not arg.default:
                 continue
             if not optional_args:
                 first_optional_arg = i
             optional_args.append(arg.name)
-        if self.num_posonly_args > 0:
-            posonly_correction = '-%d' % self.num_posonly_args
+        if num_posonly_args > 0:
+            posonly_correction = '-%d' % num_posonly_args
         else:
             posonly_correction = ''
         if optional_args:

tests/run/posonly.pyx

@@ -2,8 +2,6 @@
 # mode: run
 # tag: posonly
 
-# TODO: remove posonly tag before merge
-
 import cython
 import sys
 import pickle
@@ -124,9 +122,9 @@ def test_use_positional_as_keyword2(a, /, b):
 def test_use_positional_as_keyword3(a, b, /):
     """
     >>> test_use_positional_as_keyword3(1, 2)
-    >>> test_use_positional_as_keyword3(a=1, b=2)
+    >>> test_use_positional_as_keyword3(a=1, b=2) # doctest:+ELLIPSIS
     Traceback (most recent call last):
-    TypeError: test_use_positional_as_keyword3() takes exactly 2 positional arguments (0 given)
+    TypeError: test_use_positional_as_keyword3() got an unexpected keyword argument '...'
     """
 
 def test_positional_only_and_arg_invalid_calls(a, b, /, c):
@@ -266,7 +264,7 @@ class TestPosonlyMethods(object):
     Got type error
     >>> TestPosonlyMethods().f(1, b=2)
     Traceback (most recent call last):
-    TypeError: f() takes exactly 3 positional arguments (2 given)
+    TypeError: f() got an unexpected keyword argument 'b'
     """
     def f(self, a, b, /):
         return a, b
@@ -365,9 +363,9 @@ def test_serialization1(a, b, /):
     >>> unpickled_posonly = pickle.loads(pickled_posonly)
     >>> unpickled_posonly(1, 2)
     (1, 2)
-    >>> unpickled_posonly(a=1, b=2)
+    >>> unpickled_posonly(a=1, b=2) # doctest: +ELLIPSIS
     Traceback (most recent call last):
-    TypeError: test_serialization1() takes exactly 2 positional arguments (0 given)
+    TypeError: test_serialization1() got an unexpected keyword argument '...'
     """
     return (a, b)
 
@@ -496,3 +494,65 @@ def f_call_one_optional_kwd(a,/,*,b=2):
     (1, 3)
     """
     return (a,b)
+
+def f_call_posonly_stararg(a,/,*args):
+    """
+    >>> f_call_posonly_stararg(1)
+    (1, ())
+    >>> f_call_posonly_stararg(1, 2, 3, 4)
+    (1, (2, 3, 4))
+    """
+    return (a,args)
+
+def f_call_posonly_kwarg(a,/,**kw):
+    """
+    >>> f_call_posonly_kwarg(1)
+    (1, {})
+    >>> all_args = f_call_posonly_kwarg(1, b=2, c=3, d=4)
+    >>> all_args == (1, {'b': 2, 'c': 3, 'd': 4}) or all_args
+    True
+    """
+    return (a,kw)
+
+def f_call_posonly_stararg_kwarg(a,/,*args,**kw):
+    """
+    >>> f_call_posonly_stararg_kwarg(1)
+    (1, (), {})
+    >>> f_call_posonly_stararg_kwarg(1, 2)
+    (1, (2,), {})
+    >>> all_args = f_call_posonly_stararg_kwarg(1, b=3, c=4)
+    >>> all_args == (1, (), {'b': 3, 'c': 4}) or all_args
+    True
+    >>> all_args = f_call_posonly_stararg_kwarg(1, 2, b=3, c=4)
+    >>> all_args == (1, (2,), {'b': 3, 'c': 4}) or all_args
+    True
+    """
+    return (a,args,kw)
+
+def test_empty_kwargs(a, b, /):
+    """
+    >>> test_empty_kwargs(1, 2)
+    (1, 2)
+    >>> test_empty_kwargs(1, 2, **{})
+    (1, 2)
+    >>> test_empty_kwargs(1, 2, **{'c': 3})
+    Traceback (most recent call last):
+    TypeError: test_empty_kwargs() got an unexpected keyword argument 'c'
+    """
+    return (a,b)
+
+cdef class TestExtensionClass:
+    """
+    >>> t = TestExtensionClass()
+    >>> t.f(1,2)
+    (1, 2, 3)
+    >>> t.f(1,2,4)
+    (1, 2, 4)
+    >>> t.f(1, 2, c=4)
+    (1, 2, 4)
+    >>> t.f(1, 2, 5, c=6)
+    Traceback (most recent call last):
+    TypeError: f() got multiple values for keyword argument 'c'
+    """
+    def f(self, a, b, /, c=3):
+        return (a,b,c)