From aac1a76cbfcfe12f6dd5433ff595bc0963deb3f4 Mon Sep 17 00:00:00 2001 From: "Erlend E. Aasland" Date: Fri, 2 Feb 2024 14:44:29 +0100 Subject: [PATCH 1/9] gh-114258: Refactor Argument Clinic function name parser Yak-shaving in preparation for adding support for slots. - factor out return converter resolver - factor out cloning --- Tools/clinic/clinic.py | 189 +++++++++++++++++++++-------------------- 1 file changed, 97 insertions(+), 92 deletions(-) diff --git a/Tools/clinic/clinic.py b/Tools/clinic/clinic.py index 770878a3f8d2c7..d2dae31b8075d2 100755 --- a/Tools/clinic/clinic.py +++ b/Tools/clinic/clinic.py @@ -5124,8 +5124,7 @@ def state_dsl_start(self, line: str) -> None: self.next(self.state_modulename_name, line) - @staticmethod - def parse_function_names(line: str) -> FunctionNames: + def parse_function_names(self, line: str) -> FunctionNames: left, as_, right = line.partition(' as ') full_name = left.strip() c_basename = right.strip() @@ -5140,28 +5139,99 @@ def parse_function_names(line: str) -> FunctionNames: fail(f"Illegal function name: {full_name!r}") if not is_legal_c_identifier(c_basename): fail(f"Illegal C basename: {c_basename!r}") - return FunctionNames(full_name=full_name, c_basename=c_basename) + names = FunctionNames(full_name=full_name, c_basename=c_basename) + self.normalize_function_kind(names.full_name) + return names - def update_function_kind(self, fullname: str) -> None: + def normalize_function_kind(self, fullname: str) -> None: + # Fetch the method name and possibly class. fields = fullname.split('.') name = fields.pop() _, cls = self.clinic._module_and_class(fields) + + # Check special method requirements. if name in unsupported_special_methods: fail(f"{name!r} is a special method and cannot be converted to Argument Clinic!") + if name == '__init__' and (self.kind is not CALLABLE or not cls): + fail(f"{name!r} must be a normal method; got '{self.kind}'!") + if name == '__new__' and (self.kind is not CLASS_METHOD or not cls): + fail("'__new__' must be a class method!") + if self.kind in {GETTER, SETTER}: + if not cls: + fail("@getter and @setter must be methods") + # Normalise self.kind. if name == '__new__': - if (self.kind is CLASS_METHOD) and cls: - self.kind = METHOD_NEW - else: - fail("'__new__' must be a class method!") + self.kind = METHOD_NEW elif name == '__init__': - if (self.kind is CALLABLE) and cls: - self.kind = METHOD_INIT + self.kind = METHOD_INIT + + def resolve_return_converter(self, full_name: str, forced_converter): + if forced_converter: + if self.kind in {GETTER, SETTER}: + fail(f"@{self.kind.name.lower()} method cannot define a return type") + ast_input = f"def x() -> {forced_converter}: pass" + try: + module_node = ast.parse(ast_input) + except SyntaxError: + fail(f"Badly formed annotation for {full_name!r}: {forced_converter!r}") + function_node = module_node.body[0] + assert isinstance(function_node, ast.FunctionDef) + try: + name, legacy, kwargs = self.parse_converter(function_node.returns) + if legacy: + fail(f"Legacy converter {name!r} not allowed as a return converter") + if name not in return_converters: + fail(f"No available return converter called {name!r}") + return return_converters[name](**kwargs) + except ValueError: + fail(f"Badly formed annotation for {full_name!r}: {forced_converter!r}") + + if self.kind is METHOD_INIT: + return init_return_converter() + return CReturnConverter() + + def parse_cloned_function(self, names: FunctionNames, existing: str) -> None: + full_name, c_basename = names + fields = [x.strip() for x in existing.split('.')] + function_name = fields.pop() + module, cls = self.clinic._module_and_class(fields) + + for existing_function in (cls or module).functions: + if existing_function.name == function_name: + break + else: + print(f"{cls=}, {module=}, {existing=}", file=sys.stderr) + print(f"{(cls or module).functions=}", file=sys.stderr) + fail(f"Couldn't find existing function {existing!r}!") + + fields = [x.strip() for x in full_name.split('.')] + function_name = fields.pop() + module, cls = self.clinic._module_and_class(fields) + + overrides: dict[str, Any] = { + "name": function_name, + "full_name": full_name, + "module": module, + "cls": cls, + "c_basename": c_basename, + "docstring": "", + } + if 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( - "'__init__' must be a normal method; " - f"got '{self.kind}'!" - ) + 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) + (cls or module).functions.append(function) + self.next(self.state_function_docstring) def state_modulename_name(self, line: str) -> None: # looking for declaration, which establishes the leftmost column @@ -5186,96 +5256,31 @@ def state_modulename_name(self, line: str) -> None: # are we cloning? before, equals, existing = line.rpartition('=') if equals: - full_name, c_basename = self.parse_function_names(before) + names = self.parse_function_names(before) existing = existing.strip() if is_legal_py_identifier(existing): # we're cloning! - fields = [x.strip() for x in existing.split('.')] - function_name = fields.pop() - module, cls = self.clinic._module_and_class(fields) - - for existing_function in (cls or module).functions: - if existing_function.name == function_name: - break - else: - print(f"{cls=}, {module=}, {existing=}", file=sys.stderr) - print(f"{(cls or module).functions=}", file=sys.stderr) - fail(f"Couldn't find existing function {existing!r}!") - - fields = [x.strip() for x in full_name.split('.')] - function_name = fields.pop() - module, cls = self.clinic._module_and_class(fields) - - self.update_function_kind(full_name) - overrides: dict[str, Any] = { - "name": function_name, - "full_name": full_name, - "module": module, - "cls": cls, - "c_basename": c_basename, - "docstring": "", - } - if 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)") - function = existing_function.copy(**overrides) - self.function = function - self.block.signatures.append(function) - (cls or module).functions.append(function) - self.next(self.state_function_docstring) - return + return self.parse_cloned_function(names, existing) line, _, returns = line.partition('->') returns = returns.strip() full_name, c_basename = self.parse_function_names(line) - return_converter = None - if returns: - if self.kind in {GETTER, SETTER}: - fail(f"@{self.kind.name.lower()} method cannot define a return type") - ast_input = f"def x() -> {returns}: pass" - try: - module_node = ast.parse(ast_input) - except SyntaxError: - fail(f"Badly formed annotation for {full_name!r}: {returns!r}") - function_node = module_node.body[0] - assert isinstance(function_node, ast.FunctionDef) - try: - name, legacy, kwargs = self.parse_converter(function_node.returns) - if legacy: - fail(f"Legacy converter {name!r} not allowed as a return converter") - if name not in return_converters: - fail(f"No available return converter called {name!r}") - return_converter = return_converters[name](**kwargs) - except ValueError: - fail(f"Badly formed annotation for {full_name!r}: {returns!r}") - fields = [x.strip() for x in full_name.split('.')] function_name = fields.pop() module, cls = self.clinic._module_and_class(fields) - if self.kind in {GETTER, SETTER}: - if not cls: - fail("@getter and @setter must be methods") - - self.update_function_kind(full_name) - if self.kind is METHOD_INIT and not return_converter: - return_converter = init_return_converter() - - if not return_converter: - return_converter = CReturnConverter() - - 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) + self.function = Function( + name=function_name, + full_name=full_name, + module=module, cls=cls, + c_basename=c_basename, + return_converter=self.resolve_return_converter(full_name, returns), + kind=self.kind, + coexist=self.coexist, + critical_section=self.critical_section, + target_critical_section=self.target_critical_section + ) self.block.signatures.append(self.function) # insert a self converter automatically From 8cbecadca072a5013aaf8aee49888a6af679980a Mon Sep 17 00:00:00 2001 From: "Erlend E. Aasland" Date: Fri, 2 Feb 2024 21:00:15 +0100 Subject: [PATCH 2/9] Fixed most type errors --- Tools/clinic/clinic.py | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/Tools/clinic/clinic.py b/Tools/clinic/clinic.py index d2dae31b8075d2..18a35d47acfa1b 100755 --- a/Tools/clinic/clinic.py +++ b/Tools/clinic/clinic.py @@ -5166,7 +5166,9 @@ def normalize_function_kind(self, fullname: str) -> None: elif name == '__init__': self.kind = METHOD_INIT - def resolve_return_converter(self, full_name: str, forced_converter): + def resolve_return_converter( + self, full_name: str, forced_converter: str + ) -> CReturnConverter: if forced_converter: if self.kind in {GETTER, SETTER}: fail(f"@{self.kind.name.lower()} method cannot define a return type") @@ -5273,7 +5275,8 @@ def state_modulename_name(self, line: str) -> None: self.function = Function( name=function_name, full_name=full_name, - module=module, cls=cls, + module=module, + cls=cls, c_basename=c_basename, return_converter=self.resolve_return_converter(full_name, returns), kind=self.kind, From 8850002ff05b559f2571b24d553b1656baa17350 Mon Sep 17 00:00:00 2001 From: "Erlend E. Aasland" Date: Sat, 3 Feb 2024 00:50:40 +0100 Subject: [PATCH 3/9] Also refactor out 'add function' --- Tools/clinic/clinic.py | 33 ++++++++++++++++++++------------- 1 file changed, 20 insertions(+), 13 deletions(-) diff --git a/Tools/clinic/clinic.py b/Tools/clinic/clinic.py index 18a35d47acfa1b..db4f2f76bf9625 100755 --- a/Tools/clinic/clinic.py +++ b/Tools/clinic/clinic.py @@ -5272,7 +5272,7 @@ def state_modulename_name(self, line: str) -> None: function_name = fields.pop() module, cls = self.clinic._module_and_class(fields) - self.function = Function( + func = Function( name=function_name, full_name=full_name, module=module, @@ -5284,21 +5284,28 @@ def state_modulename_name(self, line: str) -> None: critical_section=self.critical_section, target_critical_section=self.target_critical_section ) - self.block.signatures.append(self.function) + self.add_function(func) - # insert a self converter automatically - type, name = correct_name_for_self(self.function) - kwargs = {} - if cls and type == "PyObject *": - kwargs['type'] = cls.typedef - sc = self.function.self_converter = self_converter(name, name, self.function, **kwargs) - p_self = Parameter(name, inspect.Parameter.POSITIONAL_ONLY, - function=self.function, converter=sc) - self.function.parameters[name] = p_self - - (cls or module).functions.append(self.function) self.next(self.state_parameters_start) + def add_function(self, func: Function) -> None: + # Insert a self converter automatically. + tp, name = correct_name_for_self(func) + kwargs = {} + if func.cls and tp == "PyObject *": + kwargs['type'] = func.cls.typedef + func.self_converter = self_converter(name, name, func, **kwargs) + func.parameters[name] = Parameter( + name, + inspect.Parameter.POSITIONAL_ONLY, + function=func, + converter=func.self_converter + ) + + self.block.signatures.append(func) + self.function = func + (func.cls or func.module).functions.append(func) + # Now entering the parameters section. The rules, formally stated: # # * All lines must be indented with spaces only. From ea48efad65bb35a50ee214121b2f147a309224af Mon Sep 17 00:00:00 2001 From: "Erlend E. Aasland" Date: Sat, 3 Feb 2024 00:59:12 +0100 Subject: [PATCH 4/9] Only parse function names if needed --- Tools/clinic/clinic.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Tools/clinic/clinic.py b/Tools/clinic/clinic.py index db4f2f76bf9625..2c26a68e2cf412 100755 --- a/Tools/clinic/clinic.py +++ b/Tools/clinic/clinic.py @@ -5258,10 +5258,10 @@ def state_modulename_name(self, line: str) -> None: # are we cloning? before, equals, existing = line.rpartition('=') if equals: - names = self.parse_function_names(before) existing = existing.strip() if is_legal_py_identifier(existing): # we're cloning! + names = self.parse_function_names(before) return self.parse_cloned_function(names, existing) line, _, returns = line.partition('->') From 580a1261a488806b0901f4aea4048cbb6b595526 Mon Sep 17 00:00:00 2001 From: "Erlend E. Aasland" Date: Sat, 3 Feb 2024 01:02:39 +0100 Subject: [PATCH 5/9] Nit --- Tools/clinic/clinic.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/Tools/clinic/clinic.py b/Tools/clinic/clinic.py index 2c26a68e2cf412..b1053dbb3c8e7c 100755 --- a/Tools/clinic/clinic.py +++ b/Tools/clinic/clinic.py @@ -5267,6 +5267,7 @@ def state_modulename_name(self, line: str) -> None: line, _, returns = line.partition('->') returns = returns.strip() full_name, c_basename = self.parse_function_names(line) + return_converter = self.resolve_return_converter(full_name, returns) fields = [x.strip() for x in full_name.split('.')] function_name = fields.pop() @@ -5278,7 +5279,7 @@ def state_modulename_name(self, line: str) -> None: module=module, cls=cls, c_basename=c_basename, - return_converter=self.resolve_return_converter(full_name, returns), + return_converter=return_converter, kind=self.kind, coexist=self.coexist, critical_section=self.critical_section, From ed32a6573213026394250f3bba77a3e2095a805b Mon Sep 17 00:00:00 2001 From: "Erlend E. Aasland" Date: Sat, 3 Feb 2024 02:14:54 +0100 Subject: [PATCH 6/9] Please mypy --- Tools/clinic/clinic.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Tools/clinic/clinic.py b/Tools/clinic/clinic.py index b1053dbb3c8e7c..0e8301ed74a599 100755 --- a/Tools/clinic/clinic.py +++ b/Tools/clinic/clinic.py @@ -5292,7 +5292,7 @@ def state_modulename_name(self, line: str) -> None: def add_function(self, func: Function) -> None: # Insert a self converter automatically. tp, name = correct_name_for_self(func) - kwargs = {} + kwargs: dict[str, Any] = {} if func.cls and tp == "PyObject *": kwargs['type'] = func.cls.typedef func.self_converter = self_converter(name, name, func, **kwargs) From 52cc6b2e95a8a64e2499da23f26e6856a6471a90 Mon Sep 17 00:00:00 2001 From: "Erlend E. Aasland" Date: Mon, 5 Feb 2024 01:36:54 +0100 Subject: [PATCH 7/9] Update Tools/clinic/clinic.py --- Tools/clinic/clinic.py | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/Tools/clinic/clinic.py b/Tools/clinic/clinic.py index 0e8301ed74a599..2a4ac7fb0c2801 100755 --- a/Tools/clinic/clinic.py +++ b/Tools/clinic/clinic.py @@ -5292,10 +5292,11 @@ def state_modulename_name(self, line: str) -> None: def add_function(self, func: Function) -> None: # Insert a self converter automatically. tp, name = correct_name_for_self(func) - kwargs: dict[str, Any] = {} if func.cls and tp == "PyObject *": - kwargs['type'] = func.cls.typedef - func.self_converter = self_converter(name, name, func, **kwargs) + func.self_converter = self_converter(name, name, func, + type=func.cls.typedef) + else: + func.self_converter = self_converter(name, name, func) func.parameters[name] = Parameter( name, inspect.Parameter.POSITIONAL_ONLY, From be244f23633ff1e9da8e9f7975fa5b2cd973cc73 Mon Sep 17 00:00:00 2001 From: "Erlend E. Aasland" Date: Mon, 5 Feb 2024 22:50:04 +0100 Subject: [PATCH 8/9] Address Nikita's review --- Tools/clinic/clinic.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/Tools/clinic/clinic.py b/Tools/clinic/clinic.py index eede0e1ab01cef..9bf4788ddaadf3 100755 --- a/Tools/clinic/clinic.py +++ b/Tools/clinic/clinic.py @@ -5158,9 +5158,8 @@ def normalize_function_kind(self, fullname: str) -> None: fail(f"{name!r} must be a normal method; got '{self.kind}'!") if name == '__new__' and (self.kind is not CLASS_METHOD or not cls): fail("'__new__' must be a class method!") - if self.kind in {GETTER, SETTER}: - if not cls: - fail("@getter and @setter must be methods") + if self.kind in {GETTER, SETTER} and not cls: + fail("@getter and @setter must be methods") # Normalise self.kind. if name == '__new__': @@ -5200,8 +5199,9 @@ def parse_cloned_function(self, names: FunctionNames, existing: str) -> None: fields = [x.strip() for x in existing.split('.')] function_name = fields.pop() module, cls = self.clinic._module_and_class(fields) + parent = module or cls - for existing_function in (cls or module).functions: + for existing_function in parent.functions: if existing_function.name == function_name: break else: From 2af5aae833c08f8b8986198ad2788a1ecc7e380d Mon Sep 17 00:00:00 2001 From: "Erlend E. Aasland" Date: Mon, 5 Feb 2024 23:04:42 +0100 Subject: [PATCH 9/9] Oops --- Tools/clinic/clinic.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Tools/clinic/clinic.py b/Tools/clinic/clinic.py index d666a8cfafb7df..e63596c3007940 100755 --- a/Tools/clinic/clinic.py +++ b/Tools/clinic/clinic.py @@ -5199,7 +5199,7 @@ def parse_cloned_function(self, names: FunctionNames, existing: str) -> None: fields = [x.strip() for x in existing.split('.')] function_name = fields.pop() module, cls = self.clinic._module_and_class(fields) - parent = module or cls + parent = cls or module for existing_function in parent.functions: if existing_function.name == function_name: