8000 gh-112205: Support @getter annotation from AC by corona10 · Pull Request #112396 · python/cpython · GitHub
[go: up one dir, main page]
Skip to content

gh-112205: Support @getter annotation from AC #112396

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

Merged
merged 18 commits into from
Nov 30, 2023
Merged

gh-112205: Support @getter annotation from AC #112396

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
72d9018
gh-112205: Support @getter annotation from AC
corona10 Nov 25, 2023
ea83973
Update define macro
corona10 Nov 25, 2023
969adf9
Apply suggestions from code review
corona10 Nov 25, 2023
716b8d9
nit
corona10 Nov 25, 2023
c5fe2b2
Add test case
corona10 Nov 25, 2023
2d9800d
Add test case
corona10 Nov 25, 2023
442c575
Clean up
corona10 Nov 25, 2023
e0fb271
Update Tools/clinic/clinic.py
corona10 Nov 25, 2023
0778eb7
Update logic
corona10 Nov 25, 2023
d5b1751
Update
corona10 Nov 25, 2023
7f4bfa8
Clean up
corona10 Nov 25, 2023
fe46de0
Update Tools/clinic/clinic.py
corona10 Nov 27, 2023
0892d0d
Address code review
corona10 Nov 27, 2023
769f6cd
Address code review
corona10 Nov 28, 2023
dde933e
Address code review
corona10 Nov 28, 2023
d381b5e
Change template key
corona10 Nov 28, 2023
351ba08
Use Py_UNUSED for context
corona10 Nov 28, 2023
f53a035
Update test
corona10 Nov 28, 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
Next Next commit
gh-112205: Support @Getter annotation from AC
  • Loading branch information
@corona10
corona10 committed Nov 25, 2023
commit 72d9018be372ead490d36687187bff390dfaf82d
81 changes: 33 additions & 48 deletions Modules/_io/bufferedio.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@
#include "Python.h"
#include "pycore_bytesobject.h" // _PyBytes_Join()
#include "pycore_call.h" // _PyObject_CallNoArgs()
#include "pycore_critical_section.h" // Py_BEGIN_CRITICAL_SECTION()
#include "pycore_object.h" // _PyObject_GC_UNTRACK()
#include "pycore_pyerrors.h" // _Py_FatalErrorFormat()
#include "pycore_pylifecycle.h" // _Py_IsInterpreterFinalizing()
Expand Down Expand Up @@ -518,25 +517,20 @@ buffered_closed(buffered *self)
return closed;
}

/*[clinic input]
@critical_section
@getter
_io._Buffered.closed
[clinic start generated code]*/

static PyObject *
buffered_closed_get_impl(buffered *self, void *context)
_io__Buffered_closed_get_impl(buffered *self)
/*[clinic end generated code: output=f08ce57290703a1a input=18eddefdfe4a3d2f]*/
{
CHECK_INITIALIZED(self)
return PyObject_GetAttr(self->raw, &_Py_ID(closed));
}

static PyObject *
buffered_closed_get(buffered *self, void *context)
{
PyObject *return_value = NULL;

Py_BEGIN_CRITICAL_SECTION(self);
return_value = buffered_closed_get_impl(self, context);
Py_END_CRITICAL_SECTION();

return return_value;
}

/*[clinic input]
@critical_section
_io._Buffered.close
Expand Down Expand Up @@ -662,44 +656,35 @@ _io__Buffered_writable_impl(buffered *self)
return PyObject_CallMethodNoArgs(self->raw, &_Py_ID(writable));
}


/*[clinic input]
@critical_section
@getter
_io._Buffered.name
[clinic start generated code]*/

static PyObject *
buffered_name_get_impl(buffered *self, void *context)
_io__Buffered_name_get_impl(buffered *self)
/*[clinic end generated code: output=d2adf384051d3d10 input=6b84a0e6126f545e]*/
{
CHECK_INITIALIZED(self)
return PyObject_GetAttr(self->raw, &_Py_ID(name));
}

static PyObject *
buffered_name_get(buffered *self, void *context)
{
PyObject *return_value = NULL;

Py_BEGIN_CRITICAL_SECTION(self);
return_value = buffered_name_get_impl(self, context);
Py_END_CRITICAL_SECTION();

return return_value;
}
/*[clinic input]
@critical_section
@getter
_io._Buffered.mode
[clinic start generated code]*/

static PyObject *
buffered_mode_get_impl(buffered *self, void *context)
_io__Buffered_mode_get_impl(buffered *self)
/*[clinic end generated code: output=0feb205748892fa4 input=0762d5e28542fd8c]*/
{
CHECK_INITIALIZED(self)
return PyObject_GetAttr(self->raw, &_Py_ID(mode));
}

static PyObject *
buffered_mode_get(buffered *self, void *context)
{
PyObject *return_value = NULL;

Py_BEGIN_CRITICAL_SECTION(self);
return_value = buffered_mode_get_impl(self, context);
Py_END_CRITICAL_SECTION();

return return_value;
}

/* Lower-level APIs */

/*[clinic input]
Expand Down Expand Up @@ -2541,9 +2526,9 @@ static PyMemberDef bufferedreader_members[] = {
};

static PyGetSetDef bufferedreader_getset[] = {
{"closed", (getter)buffered_closed_get, NULL, NULL},
{"name", (getter)buffered_name_get, NULL, NULL},
{"mode", (getter)buffered_mode_get, NULL, NULL},
_IO__BUFFERED_CLOSED_GET_GETTERDEF
_IO__BUFFERED_NAME_GET_GETTERDEF
_IO__BUFFERED_MODE_GET_GETTERDEF
{NULL}
};

Expand Down Expand Up @@ -2601,9 +2586,9 @@ static PyMemberDef bufferedwriter_members[] = {
};

static PyGetSetDef bufferedwriter_getset[] = {
{"closed", (getter)buffered_closed_get, NULL, NULL},
{"name", (getter)buffered_name_get, NULL, NULL},
{"mode", (getter)buffered_mode_get, NULL, NULL},
_IO__BUFFERED_CLOSED_GET_GETTERDEF
_IO__BUFFERED_NAME_GET_GETTERDEF
_IO__BUFFERED_MODE_GET_GETTERDEF
{NULL}
};

Expand Down Expand Up @@ -2719,9 +2704,9 @@ static PyMemberDef bufferedrandom_members[] = {
};

static PyGetSetDef bufferedrandom_getset[] = {
{"closed", (getter)buffered_closed_get, NULL, NULL},
{"name", (getter)buffered_name_get, NULL, NULL},
{"mode", (getter)buffered_mode_get, NULL, NULL},
_IO__BUFFERED_CLOSED_GET_GETTERDEF
_IO__BUFFERED_NAME_GET_GETTERDEF
_IO__BUFFERED_MODE_GET_GETTERDEF
{NULL}
};

Expand Down
56 changes: 55 additions & 1 deletion Modules/_io/clinic/bufferedio.c.h
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

44 changes: 40 additions & 4 deletions Tools/clinic/clinic.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -846,6 +846,10 @@ class CLanguage(Language):
static PyObject *
{c_basename}({self_type}{self_name}, PyObject *Py_UNUSED(ignored))
""")
PARSER_PROTOTYPE_GETTER: Final[str] = normalize_snippet("""
static PyObject *
{c_basename}({self_type}{self_name}, void *context)
""")
METH_O_PROTOTYPE: Final[str] = normalize_snippet("""
static PyObject *
{c_basename}({impl_parameters})
Expand All @@ -865,6 +869,10 @@ class CLanguage(Language):
#define {methoddef_name} \
{{"{name}", {methoddef_cast}{c_basename}{methoddef_cast_end}, {methoddef_flags}, {c_basename}__doc__}},
""")
GETTERDEF_PROTOTYPE_DEFINE: Final[str] = normalize_snippet(r"""
#define {methoddef_name} \
{{"{name}", {methoddef_cast}{c_basename}{methoddef_cast_end}, NULL, NULL}},
Copy link
Member Author

Choose a reason for hiding this comment

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

We may need to reform the template for the setter case too. But not sure we should handle this at this PR :)

""")
METHODDEF_PROTOTYPE_IFNDEF: Final[str] = normalize_snippet("""
#ifndef {methoddef_name}
#define {methoddef_name}
Expand Down Expand Up @@ -1161,6 +1169,9 @@ def output_templates(
methoddef_define = self.METHODDEF_PROTOTYPE_DEFINE
if new_or_init and not f.docstring:
docstring_prototype = docstring_definition = ''
elif f.getter:
methoddef_define = self.GETTERDEF_PROTOTYPE_DEFINE
docstring_prototype = docstring_definition = ''
else:
docstring_prototype = self.DOCSTRING_PROTOTYPE_VAR
docstring_definition = self.DOCSTRING_PROTOTYPE_STRVAR
Expand Down Expand Up @@ -1217,7 +1228,11 @@ def parser_body(
parsearg: str | None
if not parameters:
parser_code: list[str] | None
if not requires_defining_class:
if f.getter:
flags = "NULL"
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks strange to me; AFAICS, we don't use flags for getters (methoddef_flags is not included in the template). And if we were to supply a zero default for flags, it would have to be an int literal (0, not NULL).

Ideally we should not have to specify flags here, but a lot of the following code expect it to be defined. I suggest we either set it to the empty string or the zero int literal.

Suggested change
flags = "NULL"
flags = "0"
Suggested change
flags = "NULL"
flags = "" # This should end up unused

@AlexWaygood?

This comment was marked as resolved.

Sign in to view

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah please ignore my previous comment, now I understood what you want to say :)

Copy link
Member Author

Choose a reason for hiding this comment

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

You may already know: the reason I don't use methodef_flags to the template is due to the structure of getset.

cpython/Include/descrobject.h

Lines 11 to 17 in a194938

struct PyGetSetDef {
const char *name;
getter get;
setter set;
const char *doc;
void *closure;
};

Copy link
Contributor

Choose a reason for hiding this comment

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

We're not creating a PyMethodDef struct here, so the template field naming is now confusing. For example, the methoddef_name and methoddef_cast names are just confusing when we're generating a PyGetSetDef struct. I think we should fix the naming sooner than later.

Copy link
Member Author

Choose a reason for hiding this comment

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

We're not creating a PyMethodDef struct here, so the template field naming is now confusing. For example, the methoddef_name and methoddef_cast names are just confusing when we're generating a PyGetSetDef struct. I think we should fix the naming sooner than later.

We need to refactor some of the code while implementing @setter, Can we handle this issue on the @setter PR? or Do you want to handle it as this PR?

Copy link
Member Author

Choose a reason for hiding this comment

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

dde933e
Remove unnecessary template :)

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure my suggested comment is good enough :) Wording it more explicit might be better (cc. @AlexWaygood).

Copy link
Member

Choose a reason for hiding this comment

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

I think this is fine... or, at least, I can't think of anything better :)

Looking at this function gives me a headache; this change doesn't really make my head hurt significantly more tbh :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, the urge to refactor is growing. I suggest we take a stab at it pretty soon, Alex :)

parser_prototype = self.PARSER_PROTOTYPE_GETTER
parser_code = []
elif not requires_defining_class:
# no parameters, METH_NOARGS
flags = "METH_NOARGS"
parser_prototype = self.PARSER_PROTOTYPE_NOARGS
Expand Down Expand Up @@ -1670,6 +1685,8 @@ def parser_body(
methoddef_cast_end = ""
if flags in ('METH_NOARGS', 'METH_O', 'METH_VARARGS'):
methoddef_cast = "(PyCFunction)"
elif f.getter:
methoddef_cast = "(getter)"
elif limited_capi:
methoddef_cast = "(PyCFunction)(void(*)(void))"
else:
Expand Down Expand Up @@ -1928,7 +1945,10 @@ def render_function(
template_dict = {'full_name': full_name}
template_dict['name'] = f.displayname
template_dict['c_basename'] = f.c_basename
template_dict['methoddef_name'] = f.c_basename.upper() + "_METHODDEF"
if f.getter:
template_dict['methoddef_name'] = f.c_basename.upper() + "_GETTERDEF"
else:
template_dict['methoddef_name'] = f.c_basename.upper() + "_METHODDEF"

template_dict['docstring'] = self.docstring_for_c_string(f)

Expand Down Expand Up @@ -2932,6 +2952,7 @@ class FunctionKind(enum.Enum):
CLASS_METHOD = enum.auto()
METHOD_INIT = enum.auto()
METHOD_NEW = enum.auto()
GETTER = enum.auto()

@functools.cached_property
def new_or_init(self) -> bool:
Expand All @@ -2947,6 +2968,7 @@ def __repr__(self) -> str:
CLASS_METHOD: Final = FunctionKind.CLASS_METHOD
METHOD_INIT: Final = FunctionKind.METHOD_INIT
METHOD_NEW: Final = FunctionKind.METHOD_NEW
GETTER: Final = FunctionKind.GETTER

ParamDict = dict[str, "Parameter"]
ReturnConverterType = Callable[..., "CReturnConverter"]
Expand Down Expand Up @@ -2983,6 +3005,7 @@ class Function:
docstring_only: bool = False
critical_section: bool = False
target_critical_section: list[str] = dc.field(default_factory=list)
getter: bool = False

def __post_init__(self) -> None:
self.parent = self.cls or self.module
Expand Down Expand Up @@ -3032,6 +3055,8 @@ def methoddef_flags(self) -> str | None:
flags.append('METH_CLASS')
case FunctionKind.STATIC_METHOD:
flags.append('METH_STATIC')
case FunctionKind.GETTER:
pass
case _ as kind:
assert kind is FunctionKind.CALLABLE, f"unknown kind: {kind!r}"
if self.coexist:
Expand Down Expand Up @@ -4678,7 +4703,7 @@ def parse_arg(self, argname: str, displayname: str, *, limited_capi: bool) -> st
def correct_name_for_self(
f: Function
) -> tuple[str, str]:
if f.kind in (CALLABLE, METHOD_INIT):
if f.kind in (CALLABLE, METHOD_INIT, GETTER):
if f.cls:
return "PyObject *", "self"
return "PyObject *", "module"
Expand Down Expand Up @@ -5140,6 +5165,7 @@ class DSLParser:
preserve_output: bool
critical_section: bool
target_critical_section: list[str]
getter: bool
from_version_re = re.compile(r'([*/]) +\[from +(.+)\]')

def __init__(self, clinic: Clinic) -> None:
Expand Down Expand Up @@ -5176,6 +5202,7 @@ def reset(self) -> None:
self.preserve_output = False
self.critical_section = False
self.target_critical_section = []
self.getter = False

def directive_version(self, required: str) -> None:
global version
Expand Down Expand Up @@ -5310,6 +5337,9 @@ def at_critical_section(self, *args: str) -> None:
self.target_critical_section.extend(args)
self.critical_section = True

def at_getter(self) -> None:
self.getter = True

def at_staticmethod(self) -> None:
if self.kind is not CALLABLE:
fail("Can't set @staticmethod, function is not a normal callable")
Expand Down Expand Up @@ -5427,6 +5457,9 @@ def update_function_kind(self, fullname: str) -> None:
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
elif self.getter and cls:
self.kind = GETTER


def state_modulename_name(self, line: str) -> None:
# looking for declaration, which establishes the leftmost column
Expand Down Expand Up @@ -5500,6 +5533,8 @@ def state_modulename_name(self, line: str) -> None:
line, _, returns = line.partition( 77C1 9;->')
returns = returns.strip()
full_name, c_basename = self.parse_function_names(line)
if self.getter:
c_basename += '_get'

return_converter = None
if returns:
Expand Down Expand Up @@ -5534,7 +5569,8 @@ def state_modulename_name(self, line: str) -> None:
self.function = Function(name=function_name, full_name=full_name, module=module, cls=cls, c_basename=c_basename,
return_converter=return_converter, kind=self.kind, coexist=self.coexist,
critical_section=self.critical_section,
target_critical_section=self.target_critical_section)
target_critical_section=self.target_critical_section,
getter=self.getter)
self.block.signatures.append(self.function)

# insert a self converter automatically
Expand Down
0