gh-122943: Rework support of var-positional parameter in Argument Clinic

[serhiy-storchaka]

Move creation of a tuple for var-positional parameter out of _PyArg_UnpackKeywordsWithVararg().
Merge _PyArg_UnpackKeywordsWithVararg() with _PyArg_UnpackKeywords(). Add a new parameter in _PyArg_UnpackKeywords().

The "parameters" and "converters" attributes of ParseArgsCodeGen no longer contain the var-positional parameter. It is now available as the "varpos" attribute. Optimize code generation for var-positional parameter and reuse the same generating code for functions with and without keyword parameters.

• Issue: Re-work support of var-positional parameters in Argument Clinic #122943

[serhiy-storchaka]

You can see that support of var-positional parameters only needs modifying few lines of code rather than adding a 150-line function (which contains bugs).

[erlend-aasland]

Good job!

[serhiy-storchaka]

There are now conflicts with #126064.

I am going to merge this PR after resolving conflicts to avoid future conflicts.

[skirpichev]

@serhiy-storchaka, @erlend-aasland, I did port of #126064 on top of this pr. Diff follows (only Tools/clinic stuff included):

diff --git a/Tools/clinic/libclinic/clanguage.py b/Tools/clinic/libclinic/clanguage.py
index 32d2c045b0..32aba81ab8 100644
--- a/Tools/clinic/libclinic/clanguage.py
+++ b/Tools/clinic/libclinic/clanguage.py
@@ -15,7 +15,7 @@
     Module, Class, Function, Parameter,
     permute_optional_groups,
     GETTER, SETTER, METHOD_INIT)
-from libclinic.converters import self_converter
+from libclinic.converters import defining_class_converter, self_converter
 from libclinic.parse_args import ParseArgsCodeGen
 if TYPE_CHECKING:
     from libclinic.app import Clinic
@@ -396,6 +396,12 @@ def render_function(
         first_optional = len(selfless)
         positional = selfless and selfless[-1].is_positional_only()
         has_option_groups = False
+        requires_defining_class = (len(selfless)
+                                   and isinstance(selfless[0].converter,
+                                                  defining_class_converter))
+        pass_vararg_directly = (all(p.is_positional_only() or p.is_vararg()
+                                    for p in selfless)
+                                and not requires_defining_class)
 
         # offset i by -1 because first_optional needs to ignore self
         for i, p in enumerate(parameters, -1):
@@ -404,6 +410,9 @@ def render_function(
             if (i != -1) and (p.default is not unspecified):
                 first_optional = min(first_optional, i)
 
+            if p.is_vararg() and not pass_vararg_directly:
+                data.cleanup.append(f"Py_XDECREF({c.parser_name});")
+
             # insert group variable
             group = p.group
             if last_group != group:
@@ -415,6 +424,11 @@ def render_function(
                     data.impl_parameters.append("int " + group_name)
                     has_option_groups = True
 
+            if p.is_vararg() and pass_vararg_directly:
+                data.impl_arguments.append('nvararg')
+                data.impl_parameters.append('Py_ssize_t nargs')
+                p.converter.type = 'PyObject *const *'
+
             c.render(p, data)
 
         if has_option_groups and (not positional):
diff --git a/Tools/clinic/libclinic/converter.py b/Tools/clinic/libclinic/converter.py
index 7d74331c73..86853bb4fb 100644
--- a/Tools/clinic/libclinic/converter.py
+++ b/Tools/clinic/libclinic/converter.py
@@ -296,10 +296,7 @@ def _render_non_self(
             data.post_parsing.append('/* Post parse cleanup for ' + name + ' */\n' + post_parsing.rstrip() + '\n')
 
         # cleanup
-        if parameter.is_vararg():
-            cleanup = f"Py_XDECREF({self.parser_name});"
-        else:
-            cleanup = self.cleanup()
+        cleanup = self.cleanup()
         if cleanup:
             data.cleanup.append('/* Cleanup for ' + name + ' */\n' + cleanup.rstrip() + "\n")
 
diff --git a/Tools/clinic/libclinic/parse_args.py b/Tools/clinic/libclinic/parse_args.py
index edf13e9f95..10bee51e09 100644
--- a/Tools/clinic/libclinic/parse_args.py
+++ b/Tools/clinic/libclinic/parse_args.py
@@ -524,6 +524,11 @@ def parse_pos_only(self) -> None:
 
         parser_code = []
         max_args = NO_VARARG if self.varpos else self.max_pos
+        if self.varpos:
+            self.declarations = f"Py_ssize_t nvararg = {nargs} - {self.max_pos};"
+        else:
+            self.declarations = ""
+
         if self.limited_capi:
             if nargs != 'nargs':
                 nargs_def = f'Py_ssize_t nargs = {nargs};'
@@ -594,7 +599,11 @@ def parse_pos_only(self) -> None:
             if has_optional:
                 parser_code.append("skip_optional:")
             if self.varpos:
-                parser_code.append(libclinic.normalize_snippet(self._parse_vararg(), indent=4))
+                if min(self.pos_only, self.min_pos) >= self.max_pos:
+                    start = f'args + {self.max_pos}' if self.max_pos else 'args'
+                    parser_code.append(libclinic.normalize_snippet(f"{self.varpos.converter.parser_name} = {start};", indent=4))
+                else:
+                    parser_code.append(libclinic.normalize_snippet(self._parse_vararg(), indent=4))
         else:
             for parameter in self.parameters:
                 parameter.converter.use_converter()
@@ -619,7 +628,7 @@ def parse_pos_only(self) -> None:
                         goto exit;
                     }}
                     """, indent=4)]
-        self.parser_body(*parser_code)
+        self.parser_body(*parser_code, declarations=self.declarations)
 
     def parse_general(self, clang: CLanguage) -> None:
         parsearg: str | None

I did basic tests for affected modules (e.g. test_math or test_set), run also test_clinic. Seems working.

I admit, this looks as a temporary solution. On pros, reverting #126064 will introduce performance regressions, e.g. for math.gcd/lcm/hypot and in some set methods.

[serhiy-storchaka]

This is not so easy. Well, I managed to resolve conflicts. This PR now reverts #126064 and #126235 changes, then add converters for var-positional parameter. The result is not so bad as I was afraid, but the PR is now larger. This is not the final result. More changes will follow.

Note that the gc and _testclinic modules again use a tuple representation. Now tuple and array converters can be used for var-positional parameter. object is temporary an alias of tuple to minimize the diff. I'll change this in the following PR.

[skirpichev]

Well, I managed to resolve conflicts.

See above. It's possible to resolve conflicts without regressions.

This PR now reverts #126064 and #126235 changes

Why revert the second pr?

[serhiy-storchaka]

Why revert the second pr?

To apply the changes in other way. Don't worry, there is no regression in the math module. But the generated code is now different, and the hand-written code is different, because parameter names and order are different.

[skirpichev]

Don't worry, there is no regression in the math module.

It's in set methods.

[erlend-aasland]

Do we really need to revert the clinic input in Modules/? I don't see the benefit of all this extra churn.

[serhiy-storchaka]

gcmodule is better with a tuple. Test modules now contain tests for both convertors and also for no-fastapi. There is nothing redundant.

[erlend-aasland]

gcmodule is better with a tuple.

Why is it "better" to revert the recently added optimisation? What about the math module changes? The set module changes? None of this seems needed at all.

[skirpichev]

Why is it "better" to revert the recently added optimisation?

It's not reverted. And now I agreed with Serhiy, the gc module is better with the tuple converter. This will not add some overhead.

What about the math module changes? The set module changes?

Now all this seems to be restored. There should be no performance regressions.

As it was noted above, I think, print() might be changed to use the new array converter. This will add some performance boost and also address #90370. Change seems to be small enough.