Automate positional arguments in clinic code

[nineteendo]

Feature or enhancement

Problem:

It looks like we often forget to mark arguments as positional only in clinic input:

cpython/Modules/posixmodule.c

Lines 5470 to 5476 in 8fc953f

	/*[clinic input]
	os._path_normpath
	path: object
	Basic path normalization.
	[clinic start generated code]*/

This causes a lot of overhead with no practical benefit (this function is even private):

cpython/Modules/clinic/posixmodule.c.h

Lines 2257 to 2304 in 8fc953f

	#define OS__PATH_NORMPATH_METHODDEF \
	{"_path_normpath", _PyCFunction_CAST(os__path_normpath), METH_FASTCALL|METH_KEYWORDS, os__path_normpath__doc__},
	static PyObject *
	os__path_normpath_impl(PyObject *module, PyObject *path);
	static PyObject *
	os__path_normpath(PyObject *module, PyObject *const *args, Py_ssize_t nargs, PyObject *kwnames)
	{
	PyObject *return_value = NULL;
	#if defined(Py_BUILD_CORE) && !defined(Py_BUILD_CORE_MODULE)
	#define NUM_KEYWORDS 1
	static struct {
	PyGC_Head _this_is_not_used;
	PyObject_VAR_HEAD
	PyObject *ob_item[NUM_KEYWORDS];
	} _kwtuple = {
	.ob_base = PyVarObject_HEAD_INIT(&PyTuple_Type, NUM_KEYWORDS)
	.ob_item = { &_Py_ID(path), },
	};
	#undef NUM_KEYWORDS
	#define KWTUPLE (&_kwtuple.ob_base.ob_base)
	#else // !Py_BUILD_CORE
	# define KWTUPLE NULL
	#endif // !Py_BUILD_CORE
	static const char * const _keywords[] = {"path", NULL};
	static _PyArg_Parser _parser = {
	.keywords = _keywords,
	.fname = "_path_normpath",
	.kwtuple = KWTUPLE,
	};
	#undef KWTUPLE
	PyObject *argsbuf[1];
	PyObject *path;
	args = _PyArg_UnpackKeywords(args, nargs, NULL, kwnames, &_parser, 1, 1, 0, argsbuf);
	if (!args) {
	goto exit;
	}
	path = args[0];
	return_value = os__path_normpath_impl(module, path);
	exit:
	return return_value;
	}

If we wrote this instead:

/*[clinic input]
os._path_normpath

    path: object
    /

Basic path normalization.
[clinic start generated code]*/

The overhead is gone:

#define OS__PATH_NORMPATH_METHODDEF    \
    {"_path_normpath", (PyCFunction)os__path_normpath, METH_O, os__path_normpath__doc__},

No, I didn't forget to include the rest of the code, this is all the code!

Proposal:

Instead of manually updating all clinic input, we should modify clinic.py to do this automatically for functions without keyword arguments. If the old behaviour is desired, you should explicitly tell clinic.py that you want to support keyword arguments at the cost of performance:

/*[clinic input]
os._path_normpath

    /
    path: object

Basic path normalization.
[clinic start generated code]*/

Has this already been discussed elsewhere?

This is a minor feature, which does not need previous discussion elsewhere

Links to previous discussion of this feature:

No response

[nineteendo]

cc @erlend-aasland, @eendebakpt

[nineteendo]

Alex, you should probably also add the performance label, as this could greatly improve CPython's performance. :)

[AlexWaygood]

Alex, you should probably also add the performance label, as this could greatly improve CPython's performance. :)

Unfortunately I don't think we'd see any immediate performance gains, as we'd have to undergo a deprecation period of at least two release cycles before making any parameters that are currently positional-or-keyword positional-only :/

[nineteendo]

Damn that's really unfortunate. ;( Could we do this deprecation as soon as possible with a list of all affected functions?

[erlend-aasland]

This would break a lot of code. I don't think this is a good idea.

[nineteendo]

But which code? I'm talking about functions with only required arguments.
All I can think of is usage with functools.partial() which wouldn't apply to functions with a single argument!

[AlexWaygood]

Could we do this deprecation as soon as possible with a list of all affected functions?

We'd need to approach such a deprecation extremely carefully. Each function changed would have to be evaluated separately to see if the tradeoff would be worth it for users of CPython. For each function, we'd have to consider:

• How common is it to pass a keyword argument to the parameter(s) we're considering changing? If it's common, and/or if it makes sense to be able to pass it by keyword, it may not be worth the disruption
• How much of a speedup are we talking for that function?
• How likely is it that that function would be used in performance-critical areas of a codebase, such that speeding up the function would offer a meaningful and noticeable improvement for users of CPython?

This isn't to say that we shouldn't make any changes here, necessarily. If there are builtin functions where it really makes no sense to pass an argument by keyword, it would speed the function up a lot to make a parameter positional-only, and we can't really find any uses in the wild of people passing it by keyword — for those functions, I think we could definitely consider starting a deprecation cycle so that we can eventually make them positional-only. We definitely can't do them all at once, though, I'm afraid :-)

[nineteendo]

We'd need to approach such a deprecation extremely carefully. Each function changed would have to be evaluated separately to see if the tradeoff would be worth it for users of CPython.

I agree, though we should use an approach that minimizes the required work (like only evaluating functions with 2 or more
required arguments and skipping infrequently used modules).

How common is it to pass a keyword argument to the parameter(s) we're considering changing? If it's common, and/or if it makes sense to be able to pass it by keyword, it may not be worth the disruption

Won't this be mostly used for functions with readability problems or an unintuitive order, and usage with something like functools.partial() (which doesn't apply to functions with a single argument)?

We definitely can't do them all at once, though, I'm afraid

That's fine, but it's important that we discuss this.

[AlexWaygood]

Won't this be mostly used for functions with readability problems or an unintuitive order, and usage with something like functools.partial()

It's pretty hard to say in the abstract, which is why we'd need to consider each function individually before making a decision on whether to start the deprecation cycle :-) but anything that would require people to change code that was previously working counts as a backwards-compatibility breakage, so is subject to the policy in PEP-387

[nineteendo]

Could we consider automatically updating all current clinic input to explicitely allow keyword arguments like in the proposal, and make arguments positional only by default if no keywords are provided?
Or would that lead to too many merge conflicts?

[nineteendo]

I also think we should start with the private functions (starting with an underscore).
For those functions we just need to check if they're not exported as public functions.
(And of course don't break code in the stdlib).

[AlexWaygood]

Could we consider automatically updating all current clinic input to explicitely allow keyword arguments like in the proposal, and make arguments positional only by default if no keywords are provided? Or would that lead to too many merge conflicts?

I'm not sure I fully understand what you're proposing here, sorry

[nineteendo]

OK, so we simply convert this code:

 /*[clinic input] 
 os._path_normpath 
  
     path: object 
  
 Basic path normalization. 
 [clinic start generated code]*/ 

To this:

 /*[clinic input] 
 os._path_normpath 
  
     /
     path: object 
  
 Basic path normalization. 
 [clinic start generated code]*/ 

That's it. Oops, that's not supported:

Error in file 'Modules/posixmodule.c' on line 5473:
Function '_path_normpath' has an unsupported group configuration. (Unexpected state 0.d)