Skip to content

Argument Clinic: Remove METHOD_INIT and METHOD_NEW function kinds #23

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Argument Clinic: Remove METHOD_INIT and METHOD_NEW function kinds #23

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
637dc72
Argument Clinic: Remove METHOD_INIT and METHOD_NEW function kinds
erlend-aasland Aug 17, 2023
cf0d224
return annotation
erlend-aasland Aug 17, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Lib/test/test_clinic.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3507,7 +3507,7 @@ def test_Function_and_Parameter_reprs(self):
c_basename=None,
full_name='foofoo',
return_converter=clinic.init_return_converter(),
kind=clinic.FunctionKind.METHOD_INIT,
kind=clinic.FunctionKind.CALLABLE,
coexist=False
)
self.assertEqual(repr(function), "<clinic.Function 'foo'>")
Expand Down
70 changes: 31 additions & 39 deletions Tools/clinic/clinic.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -998,7 +998,7 @@ def output_templates(

has_option_groups = parameters and (parameters[0].group or parameters[-1].group)
default_return_converter = f.return_converter.type == 'PyObject *'
new_or_init = f.kind.new_or_init
new_or_init = f.new_or_init

vararg: int | str = NO_VARARG
pos_only = min_pos = max_pos = min_kw_only = pseudo_args = 0
Expand Down Expand Up @@ -1405,7 +1405,7 @@ def parser_body(
if new_or_init:
methoddef_define = ''

if f.kind is METHOD_NEW:
if f.name == "__new__":
parser_prototype = self.PARSER_PROTOTYPE_KEYWORD
else:
return_value_declaration = "int return_value = -1;"
Expand Down Expand Up @@ -1629,7 +1629,7 @@ def render_function(
last_group = 0
first_optional = len(selfless)
positional = selfless and selfless[-1].is_positional_only()
new_or_init = f.kind.new_or_init
new_or_init = f.new_or_init
has_option_groups = False

# offset i by -1 because first_optional needs to ignore self
Expand Down Expand Up @@ -2604,12 +2604,6 @@ class FunctionKind(enum.Enum):
CALLABLE = enum.auto()
STATIC_METHOD = enum.auto()
CLASS_METHOD = enum.auto()
METHOD_INIT = enum.auto()
METHOD_NEW = enum.auto()

@functools.cached_property
def new_or_init(self) -> bool:
return self in {FunctionKind.METHOD_INIT, FunctionKind.METHOD_NEW}

def __repr__(self) -> str:
return f"<clinic.FunctionKind.{self.name}>"
Expand All @@ -2619,8 +2613,6 @@ def __repr__(self) -> str:
CALLABLE: Final = FunctionKind.CALLABLE
STATIC_METHOD: Final = FunctionKind.STATIC_METHOD
CLASS_METHOD: Final = FunctionKind.CLASS_METHOD
METHOD_INIT: Final = FunctionKind.METHOD_INIT
METHOD_NEW: Final = FunctionKind.METHOD_NEW

ParamDict = dict[str, "Parameter"]
ReturnConverterType = Callable[..., "CReturnConverter"]
Expand Down Expand Up @@ -2661,10 +2653,14 @@ def __post_init__(self) -> None:
self.self_converter: self_converter | None = None
self.__render_parameters__: list[Parameter] | None = None

@functools.cached_property
def new_or_init(self) -> bool:
return self.name in ("__new__", "__init__")
Comment on lines 2655 to +2658
Copy link

@AlexWaygood AlexWaygood Aug 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't need to be a cached_property anymore; we can just assign it eagerly in __post_init__:

Suggested change
@functools.cached_property
def new_or_init(self) -> bool:
return self.name in ("__new__", "__init__")
self.new_or_init = self.name in ("__new__", "__init__")

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, that's fine. But what do you think of this refactor?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I did not mean to be dismissive! Thanks for the suggestion :)

Copy link

@AlexWaygood AlexWaygood Aug 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, you're right; the overall concept is more important!

I think it looks good. Conceptually, I think these aren't really distinct categories of methods; they're subcategories of (respectively) CALLABLE and CLASS_METHOD. So we're simplifying some code while also making it conceptually cleaner, which is nice.

The benefit doesn't seem huge to me right now, but I'll trust you when you say that this clears the way for further refactorings :)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll try to formulate my motivation for doing this. I just need to find time to jot it down :)


@functools.cached_property
def displayname(self) -> str:
"""Pretty-printable name."""
if self.kind.new_or_init:
if self.new_or_init:
assert isinstance(self.cls, Class)
return self.cls.name
else:
Expand All @@ -2673,7 +2669,7 @@ def displayname(self) -> str:
@functools.cached_property
def fulldisplayname(self) -> str:
parent: Class | Module | Clinic | None
if self.kind.new_or_init:
if self.new_or_init:
parent = getattr(self.cls, "parent", None)
else:
parent = self.parent
Expand All @@ -2696,7 +2692,7 @@ def render_parameters(self) -> list[Parameter]:

@property
def methoddef_flags(self) -> str | None:
if self.kind.new_or_init:
if self.new_or_init:
return None
flags = []
match self.kind:
Expand Down Expand Up @@ -4127,21 +4123,21 @@ def parse_arg(self, argname: str, displayname: str) -> str | None:
def correct_name_for_self(
f: Function
) -> tuple[str, str]:
if f.kind in (CALLABLE, METHOD_INIT):
if f.kind is CALLABLE or f.name == "__init__":
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if f.kind is CALLABLE or f.name == "__init__":
if f.kind is CALLABLE:

if f.cls:
return "PyObject *", "self"
return "PyObject *", "module"
if f.kind is STATIC_METHOD:
return "void *", "null"
if f.kind in (CLASS_METHOD, METHOD_NEW):
if f.kind is CLASS_METHOD:
return "PyTypeObject *", "type"
raise AssertionError(f"Unhandled type of function f: {f.kind!r}")

def required_type_for_self_for_parser(
f: Function
) -> str | None:
type, _ = correct_name_for_self(f)
if f.kind in (METHOD_INIT, METHOD_NEW, STATIC_METHOD, CLASS_METHOD):
if f.kind in (STATIC_METHOD, CLASS_METHOD) or f.name == "__init__":
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if f.kind in (STATIC_METHOD, CLASS_METHOD) or f.name == "__init__":
if f.kind in (STATIC_METHOD, CLASS_METHOD) or f.new_or_init:

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reads slightly nicer.

return type
return None

Expand All @@ -4165,13 +4161,13 @@ def pre_render(self) -> None:

kind = self.function.kind

if kind is STATIC_METHOD or kind.new_or_init:
if kind is STATIC_METHOD or f.new_or_init:
self.show_in_signature = False

# tp_new (METHOD_NEW) functions are of type newfunc:
# tp_new functions are of type newfunc:
# typedef PyObject *(*newfunc)(PyTypeObject *, PyObject *, PyObject *);
#
# tp_init (METHOD_INIT) functions are of type initproc:
# tp_init functions are of type initproc:
# typedef int (*initproc)(PyObject *, PyObject *, PyObject *);
#
# All other functions generated by Argument Clinic are stored in
Expand All @@ -4189,18 +4185,18 @@ def pre_render(self) -> None:
#
# * The type of the first parameter to the impl will always be of self.type.
#
# * If the function is neither tp_new (METHOD_NEW) nor tp_init (METHOD_INIT):
# * If the function is neither tp_new nor tp_init:
# * The type of the first parameter to the parsing function is also self.type.
# This means that if you step into the parsing function, your "self" parameter
# is of the correct type, which may make debugging more pleasant.
#
# * Else if the function is tp_new (METHOD_NEW):
# * Else if the function is tp_new:
# * The type of the first parameter to the parsing function is "PyTypeObject *",
# so the type signature of the function call is an exact match.
# * If self.type != "PyTypeObject *", we cast the first parameter to self.type
# in the impl call.
#
# * Else if the function is tp_init (METHOD_INIT):
# * Else if the function is tp_init:
# * The type of the first parameter to the parsing function is "PyObject *",
# so the type signature of the function call is an exact match.
# * If self.type != "PyObject *", we cast the first parameter to self.type
Expand Down Expand Up @@ -4233,11 +4229,10 @@ def render(self, parameter: Parameter, data: CRenderData) -> None:
def set_template_dict(self, template_dict: TemplateDict) -> None:
template_dict['self_name'] = self.name
template_dict['self_type'] = self.parser_type
kind = self.function.kind
cls = self.function.cls

if kind.new_or_init and cls and cls.typedef:
if kind is METHOD_NEW:
if self.function.new_or_init and cls and cls.typedef:
if self.function.name == "__new__":
type_check = (
'({0} == base_tp || {0}->tp_init == base_tp->tp_init)'
).format(self.name)
Expand Down Expand Up @@ -4862,11 +4857,9 @@ def update_function_kind(self, fullname: str) -> None:
if name == '__new__':
if (self.kind is not CLASS_METHOD) or (not cls):
fail("'__new__' must be a class method!")
self.kind = METHOD_NEW
elif name == '__init__':
if (self.kind is not CALLABLE) or (not cls):
fail("'__init__' must be a normal method, not a class or static method!")
self.kind = METHOD_INIT

def state_modulename_name(self, line: str) -> None:
# looking for declaration, which establishes the leftmost column
Expand Down Expand Up @@ -4920,16 +4913,15 @@ def state_modulename_name(self, line: str) -> None:
"c_basename": c_basename,
"docstring": "",
}
if not (existing_function.kind is self.kind and
# Allow __new__ or __init__ methods.
if existing_function.new_or_init:
overrides["kind"] = self.kind
# Future enhancement: allow custom return converters
overrides["return_converter"] = CReturnConverter()
elif not (existing_function.kind is self.kind and
existing_function.coexist == self.coexist):
# Allow __new__ or __init__ methods.
if existing_function.kind.new_or_init:
overrides["kind"] = self.kind
# Future enhancement: allow custom return converters
overrides["return_converter"] = CReturnConverter()
else:
fail("'kind' of function and cloned function don't match! "
"(@classmethod/@staticmethod/@coexist)")
fail("'kind' of function and cloned function don't match! "
"(@classmethod/@staticmethod/@coexist)")
function = existing_function.copy(**overrides)
self.function = function
self.block.signatures.append(function)
Expand Down Expand Up @@ -4965,7 +4957,7 @@ def state_modulename_name(self, line: str) -> None:
module, cls = self.clinic._module_and_class(fields)

self.update_function_kind(full_name)
if self.kind is METHOD_INIT and not return_converter:
if function_name == "__init__" and not return_converter:
return_converter = init_return_converter()

if not return_converter:
Expand Down Expand Up @@ -5729,7 +5721,7 @@ def format_docstring_parameters(params: list[Parameter]) -> str:
def format_docstring(self) -> str:
assert self.function is not None
f = self.function
if f.kind.new_or_init and not f.docstring:
if f.new_or_init and not f.docstring:
# don't render a docstring at all, no signature, nothing.
return f.docstring

Expand Down