Argument clinic: add support for creating method aliases

@AlexWaygood

Feature or enhancement

Proposal:

Before argument clinic, creating method aliases in C extension classes was very easy: simply add another MethodDef for the same function:

    {"replace",     _PyCFunction_CAST(datetime_replace),      METH_VARARGS | METH_KEYWORDS},
    {"__replace__", _PyCFunction_CAST(datetime_replace),      METH_VARARGS | METH_KEYWORDS},

This is very cheap and convenient. However, with argument clinic we cannot quite do this. If I do

    DATETIME_DATETIME_REPLACE_METHODDEF
    {"__replace__", _PyCFunction_CAST(datetime_datetime_replace), METH_FASTCALL | METH_KEYWORDS},

I have to manually copy the flags from the generated MethodDef into the one I wrote myself. This works now, but can break later if clinic generates a MethodDef with different flags. It is possible to work-around by copying the function, but this adds a maintenance burden and generates argument parsing twice, leading to code bloat.

It would be nice to have some syntax to tell argument clinic to generate the alias MethodDef. It could look like this:

/*[clinic input]
alias datetime.date.__replace__ = datetime.date.replace
[clinic start generated code]*/

which would generate:

#define DATETIME_DATETIME___REPLACE___METHODDEF    \
    {"__replace__", _PyCFunction_CAST(datetime_datetime_replace), METH_FASTCALL|METH_KEYWORDS, \
     datetime_datetime_replace__doc__},

that can then be used as usual:

    DATETIME_DATETIME_REPLACE_METHODDEF
    DATETIME_DATETIME___REPLACE___METHODDEF

This is presently a problem for code.__replace__, which is an alias for generated code.replace¹. I also ran into this in #112921, where I convert datetime.*.replace methods to use argument clinic.

CC @AlexWaygood @larryhastings @erlend-aasland

Has this already been discussed elsewhere?

No response given

Links to previous discussion of this feature:

No response

https://github.com/python/cpython/blob/893c9ccf48eacb02fa6ae93632f2d0cb6778dbb6/Objects/codeobject.c#L2180 ↩

How about a @ directive?

/*[clinic input]
@aliases __replace__ _replace
datetime.date.replace
   args
   ...
[clinic start generated code]*/

Alternatively:

/*[clinic input]
@alias __replace__ as datetime_replace_alias
datetime.date.replace as datetime_replace
    args
    ...
[clinic start generated code]*/

Possibly this is a dumb question -- how would this differ from the existing clone feature?

Possibly this is a dumb question -- how would this differ from the existing clone feature?

It only creates a new PyMethodDef entry. There is no function declaration, etc.

@eltoder

Got it. That'll be something to make sure we're explicit about in the devguide docs.

It is a similar concept to cloning though, so I guess I lean towards something like @eltoder's proposed syntax (something involving an assignment) rather than an @ directive?

@eltoder

Got it. That'll be something to make sure we're explicit about in the devguide docs.

Yeah, absolutely.

It is a similar concept to cloning though, so I guess I lean towards something like @eltoder's proposed syntax (something involving an assignment) rather than an @ directive?

Yes, it is similar to cloning. No matter what syntax we land on, I think we need to be able to support the as notation. That is, either:

/*[clinic input]
alias datetime.date.__replace__ = datetime.date.replace as date___replace__
[clinic start generated code]*/

... or a variant of #113270 (comment).

I think the decorator syntax could work and be a very compact and readable notation:

/*[clinic input]
@alias("datetime.date.__replace__")
datetime.date.replace as datetime_replace
    args
    ...
[clinic start generated code]*/

To be honest, I think the ideal syntax is just the assignment like in Python:

/*[clinic input]
datetime.date.__replace__ = datetime.date.replace
[clinic start generated code]*/

and the cloning feature would be something like:

/*[clinic input]
datetime.date.bar from datetime.date.foo
[clinic start generated code]*/

since it isn't an assignment. But that's probably too late to change. We can go with something like this instead:

/*[clinic input]
datetime.date.__replace__ alias datetime.date.replace
[clinic start generated code]*/

In this case, we do not need to support as: there's no C implementation for the alias and the one for the target (right hand side) should have been declared in its own definition.

Actually, I take it back about as. It is needed to give the C macro and the python alias different names. Though to be consistent with the cloning syntax, as should go before =:

/*[clinic input]
alias datetime.date.__replace__ as date___replace__ = datetime.date.replace
[clinic start generated code]*/

or

/*[clinic input]
datetime.date.__replace__ as date___replace__ alias datetime.date.replace
[clinic start generated code]*/

Yeah, I think we need to support custom macro names with the as syntax.

Let's compare alternatives proposed so far:

Syntax

alias directive:

/*[clinic input]
# Declare the 'replace' function.
datetime.date.replace
   args
[clinic start generated code]*/


/*[clinic input]
# Declare the alias '__replace__' in a separate clinic input block.
alias datetime.date.__replace__ as date__replace__ = datetime.date.replace
[clinic start generated code]*/

modify state_modulename_name to support the alias syntax:

/*[clinic input]
# Declare the 'replace' function and its alias '__replace__'
# in a single block, on one line.
datetime.date.__replace__ as date__replace__ alias datetime.date.replace
   args
[clinic start generated code]*/

@alias annotation:

/*[clinic input]
# Declare the 'replace' function and its alias '__replace__'
# in a single block, on two lines.
@alias __replace__ as date__replace__
datetime.date.replace
   args
[clinic start generated code]*/

I find 2. hard to read, and I dare not to think of how hard it would be to interpret such a line if you involve cloning as well 😄

Implementation

We'd need a new def directive_alias() function in DSLParser. We'd have to refactor state_modulename_name() so we could easily reuse the as and = syntax. directive_alias will need to perform lookups similar to the cloning feature of state_modulename_name(), so it will probably end up with a complexity similar to that function.
state_modulename_name() is already fairly complex, so I would prefer to refactor it prior to adding support for the alias clause. The implementation itself will probably be a smaller diff than 1., but there will be a lot of syntax corner cases to deal with (= then alias then as? as then = then alias? etc.), both in the implementation and not to mention test suite.
We'd need a new def at_alias() function in DSLParser. We already did some refactoring regarding the as "keyword" (parse_function_names()); we could probably reuse that to implement as for at_alias(). Contrary to 2., we would not need to perform lookups to find the original declaration; we can just store the alias in the parser object and pick it up again when we enter the parsing state machine.

I believe 3. will be easiest to implement, but I may be wrong.

Proof-of-concept implementation of 3.: https://github.com/erlend-aasland/cpython/pull/new/clinic/alias

I agree that 2 seems hard to read, so 1 or 3 are better options. 3 seems like the most compact syntax and easier to implement. The only thing to note is that if you use short names instead of qualified names (i.e. @alias __replace__ instead of @alias datetime.date.__replace__) you cannot create an alias in another class. This is occasionally handy, for example, to expose a method from a base class in a derived class, e.g.

class Base:
    def foo(self): ...

class Derived(Base):
    base_foo = Base.foo

    @override
    def foo(self): ...

However, I don't currently have a case like this for argument clinic.

This is occasionally handy, for example, to expose a method from a base class in a derived class, e.g.

This would require the base class and the target method to be declared using Argument Clinic. It would also require that the base class and the derived class is implemented in the same file.

This would require the base class and the target method to be declared using Argument Clinic. It would also require that the base class and the derived class is implemented in the same file.

This is the case, for example, in the datetime module, so it is not unrealistic. But I don't have a use case for cross-class aliases at the moment.

Proof-of-concept implementation of 3.: https://github.com/erlend-aasland/cpython/pull/new/clinic/alias

You can make DSLParser.alias a list. Then you can declare multiple aliases by repeating @alias directives.

You can make DSLParser.alias a list. Then you can declare multiple aliases by repeating @alias directives.

I thought about that, but let's not over-design this from the start. If the need arises, it is fairly trivial to allow multiple aliases as a future enhancement. We developed the keyword-only/positional-only deprecation system in Argument Clinic with similar enhancement iterations.

But I don't have a use case for cross-class aliases at the moment.

If there is no use-case, we don't need to implement support for it :) If the need arises, we can deal with it in a follow-up issue.

I thought about that, but let's not over-design this from the start. If the need arises, it is fairly trivial to allow multiple aliases as a future enhancement. We developed the keyword-only/positional-only deprecation system in Argument Clinic with similar enhancement iterations.

Sure, but seems like an odd limitation. Having a list will actually make the code simpler since you can remove the "Called @alias twice!" check and turn two other ifs into fors without any other changes.

Having a list will actually make the code simpler [...]

I suggest to wait with that statement until tests have been added ;)

I suggest to wait with that statement until tests have been added ;)

You'll have to write a test for using two alias directives either way :)

I forgot one thing: we need to generate a new docstring signature for the alias, which means we need to duplicate the docstring definition as well.

@AlexWaygood

I forgot one thing: we need to generate a new docstring signature for the alias, which means we need to duplicate the docstring definition as well.

^^ @AlexWaygood: this implies we need to bring back our old plan of refactoring the docstring generation. In our previous attempt, we moved the complete docstring generation to the Function object, but approach was rejected: see #107840. IIRC, we briefly discussed having a generator class; perhaps that approach can be revived; so a DocstringGenerator class. Let's create an issue for it.

@erlend-aasland

@erlend-aasland looks like github is missing a yak-shaving emoji :-)

You could also just use clone, then have the implementation of the clone call the implementation of the original. Compilers are smart these days--calling a static function with identical arguments will get optimized away, so it won't be any slower. Assuming the number of places where we have these duplicate method defs is small, I'd probably prefer this clone approach, as this solves the problem with adding new syntax / implementation / conceptual load to Clinic.

Thanks for chiming in, Larry. Yeah, that's probably the best option.

@erlend-aasland

You could also just use clone, then have the implementation of the clone call the implementation of the original. Compilers are smart these days--calling a static function with identical arguments will get optimized away, so it won't be any slower.

Compilers will optimize it by just inlining the static function. This leads to code bloat as the argument parsing code, which can be substantial, and the function implementation will be duplicated in the executable. You can see this for yourself, for example: https://godbolt.org/z/GfMjfes8c. Notice how datetime_time_replace and datetime_time_replace2 are duplicated in the output. This means we are getting a bigger executable and slower compilation. It is much inferior to just creating a second PyMethodDef pointing to the same function, as was a common practice before argument clinic.

The alias feature itself seems very simple. @erlend-aasland had most of it implemented in less than a day.

@erlend-aasland

@erlend-aasland Are you still working on this?

@erlend-aasland

@erlend-aasland Are you still working on this?

Not actively, so feel free to pick it up. But I do think we need to do more refactorings (as previously mentioned) in order to pull this off nicely. My branch is a very limited variant of this feature.

Navigation Menu

Repository navigation

Argument clinic: add support for creating method aliases #113270

Feature or enhancement

Proposal:

Has this already been discussed elsewhere?

Links to previous discussion of this feature:

Syntax

Implementation

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

Argument clinic: add support for creating method aliases #113270

Description

Feature or enhancement

Proposal:

Has this already been discussed elsewhere?

Links to previous discussion of this feature:

Footnotes

Activity

erlend-aasland commented on Dec 19, 2023

erlend-aasland commented on Dec 19, 2023

AlexWaygood commented on Dec 19, 2023

erlend-aasland commented on Dec 19, 2023

AlexWaygood commented on Dec 19, 2023

erlend-aasland commented on Dec 19, 2023

eltoder commented on Dec 19, 2023

eltoder commented on Dec 19, 2023

erlend-aasland commented on Dec 19, 2023

Syntax

Implementation

erlend-aasland commented on Dec 19, 2023

eltoder commented on Dec 19, 2023

erlend-aasland commented on Dec 19, 2023

eltoder commented on Dec 19, 2023

erlend-aasland commented on Dec 19, 2023

eltoder commented on Dec 19, 2023

erlend-aasland commented on Dec 20, 2023

eltoder commented on Dec 20, 2023

erlend-aasland commented on Dec 20, 2023

erlend-aasland commented on Dec 20, 2023

eltoder commented on Dec 20, 2023

larryhastings commented on Dec 30, 2023

erlend-aasland commented on Dec 30, 2023

eltoder commented on Dec 30, 2023

eltoder commented on Jan 31, 2024

erlend-aasland commented on Jan 31, 2024

Metadata

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

Issue actions