From 2f53112aad897b6474ed0e577f274b17541be36d Mon Sep 17 00:00:00 2001 From: "Erlend E. Aasland" Date: Tue, 15 Aug 2023 10:48:19 +0200 Subject: [PATCH 1/8] gh-104683: Argument Clinic: Extract parse_names() helper Reduce duplicate code in state_modulename_name() --- Tools/clinic/clinic.py | 51 ++++++++++++++++++------------------------ 1 file changed, 22 insertions(+), 29 deletions(-) diff --git a/Tools/clinic/clinic.py b/Tools/clinic/clinic.py index 1e0303c77087eb..3874fa2a3f290e 100755 --- a/Tools/clinic/clinic.py +++ b/Tools/clinic/clinic.py @@ -1674,18 +1674,8 @@ def render_function( full_name = f.full_name template_dict = {'full_name': full_name} template_dict['name'] = f.displayname - - if f.c_basename: - c_basename = f.c_basename - else: - fields = full_name.split(".") - if fields[-1] == '__new__': - fields.pop() - c_basename = "_".join(fields) - - template_dict['c_basename'] = c_basename - - template_dict['methoddef_name'] = c_basename.upper() + "_METHODDEF" + template_dict['c_basename'] = f.c_basename + template_dict['methoddef_name'] = f.c_basename.upper() + "_METHODDEF" template_dict['docstring'] = self.docstring_for_c_string(f) @@ -2653,7 +2643,7 @@ class Function: name: str module: Module | Clinic cls: Class | None - c_basename: str | None + c_basename: str full_name: str return_converter: CReturnConverter kind: FunctionKind @@ -4840,6 +4830,22 @@ def state_dsl_start(self, line: str) -> None: self.next(self.state_modulename_name, line) + @staticmethod + def parse_names(line: str) -> tuple[str, str]: + left, _, right = line.partition(' as ') + full_name = left.strip() + c_basename = right.strip() + if not c_basename: + fields = full_name.split(".") + if fields[-1] == '__new__': + fields.pop() + c_basename = "_".join(fields) + if not is_legal_py_identifier(full_name): + 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 full_name, c_basename + def state_modulename_name(self, line: str) -> None: # looking for declaration, which establishes the leftmost column # line should be @@ -4862,15 +4868,10 @@ def state_modulename_name(self, line: str) -> None: # are we cloning? before, equals, existing = line.rpartition('=') - c_basename: str | None if equals: - full_name, _, c_basename = before.partition(' as ') - full_name = full_name.strip() - c_basename = c_basename.strip() + full_name, c_basename = self.parse_names(before) existing = existing.strip() - if (is_legal_py_identifier(full_name) and - (not c_basename or is_legal_c_identifier(c_basename)) and - is_legal_py_identifier(existing)): + if is_legal_py_identifier(existing): # we're cloning! fields = [x.strip() for x in existing.split('.')] function_name = fields.pop() @@ -4915,15 +4916,7 @@ def state_modulename_name(self, line: str) -> None: line, _, returns = line.partition('->') returns = returns.strip() - - full_name, _, c_basename = line.partition(' as ') - full_name = full_name.strip() - c_basename = c_basename.strip() or None - - if not is_legal_py_identifier(full_name): - fail(f"Illegal function name: {full_name!r}") - if c_basename and not is_legal_c_identifier(c_basename): - fail(f"Illegal C basename: {c_basename!r}") + full_name, c_basename = self.parse_names(line) return_converter = None if returns: From 76b637aa43d84263a88ef41ed1ea0e2c7bdd57c4 Mon Sep 17 00:00:00 2001 From: "Erlend E. Aasland" Date: Tue, 15 Aug 2023 14:03:09 +0200 Subject: [PATCH 2/8] Ensure a C basename is provided after 'as' --- Tools/clinic/clinic.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/Tools/clinic/clinic.py b/Tools/clinic/clinic.py index 3874fa2a3f290e..2d2cf55a56e9d2 100755 --- a/Tools/clinic/clinic.py +++ b/Tools/clinic/clinic.py @@ -4832,9 +4832,11 @@ def state_dsl_start(self, line: str) -> None: @staticmethod def parse_names(line: str) -> tuple[str, str]: - left, _, right = line.partition(' as ') + left, _as, right = line.partition(' as ') full_name = left.strip() c_basename = right.strip() + if _as and not c_basename: + fail("No C basename provided after 'as' keyword") if not c_basename: fields = full_name.split(".") if fields[-1] == '__new__': From 0243b09295fd596a1a5c2970147d6818513d33d0 Mon Sep 17 00:00:00 2001 From: "Erlend E. Aasland" Date: Tue, 15 Aug 2023 14:09:58 +0200 Subject: [PATCH 3/8] Add test_no_c_basename() --- Lib/test/test_clinic.py | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/Lib/test/test_clinic.py b/Lib/test/test_clinic.py index f067a26d1fb3ae..4adddcb6c052ab 100644 --- a/Lib/test/test_clinic.py +++ b/Lib/test/test_clinic.py @@ -28,7 +28,8 @@ def _make_clinic(*, filename='clinic_tests'): return c -def _expect_failure(tc, parser, code, errmsg, *, filename=None, lineno=None): +def _expect_failure(tc, parser, code, errmsg, *, filename=None, lineno=None, + strip=True): """Helper for the parser tests. tc: unittest.TestCase; passed self in the wrapper @@ -38,7 +39,9 @@ def _expect_failure(tc, parser, code, errmsg, *, filename=None, lineno=None): filename: str, optional filename lineno: int, optional line number """ - code = dedent(code).strip() + code = dedent(code) + if strip: + code = code.strip() errmsg = re.escape(errmsg) with tc.assertRaisesRegex(clinic.ClinicError, errmsg) as cm: parser(code) @@ -828,9 +831,10 @@ def parse_function(self, text, signatures_in_block=2, function_index=1): assert isinstance(s[function_index], clinic.Function) return s[function_index] - def expect_failure(self, block, err, *, filename=None, lineno=None): + def expect_failure(self, block, err, *, + filename=None, lineno=None, strip=True): return _expect_failure(self, self.parse_function, block, err, - filename=filename, lineno=lineno) + filename=filename, lineno=lineno, strip=strip) def checkDocstring(self, fn, expected): self.assertTrue(hasattr(fn, "docstring")) @@ -1491,6 +1495,11 @@ def test_illegal_c_basename(self): err = "Illegal C basename: '935'" self.expect_failure(block, err) + def test_no_c_basename(self): + block = "foo as " + err = "No C basename provided after 'as' keyword" + self.expect_failure(block, err, strip=False) + def test_single_star(self): block = """ module foo From 3f63df95382583dceadae136a7e7f1c80300d7ec Mon Sep 17 00:00:00 2001 From: "Erlend E. Aasland" Date: Tue, 15 Aug 2023 23:03:55 +0200 Subject: [PATCH 4/8] Adjust test --- Lib/test/test_clinic.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Lib/test/test_clinic.py b/Lib/test/test_clinic.py index 76729c8c3e4f79..a5f23138692490 100644 --- a/Lib/test/test_clinic.py +++ b/Lib/test/test_clinic.py @@ -682,7 +682,7 @@ class C "void *" "" foo2 as .illegal. = foo1 [clinic start generated code]*/ """ - err = "Illegal C basename: '.illegal. = foo1'" + err = "Illegal C basename: '.illegal.'" self.expect_failure(block, err, lineno=7) From 2e68f63d39aeedd246ae0acd4151da8177c4fece Mon Sep 17 00:00:00 2001 From: "Erlend E. Aasland" Date: Wed, 16 Aug 2023 13:19:59 +0200 Subject: [PATCH 5/8] Style Co-authored-by: Alex Waygood --- Tools/clinic/clinic.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Tools/clinic/clinic.py b/Tools/clinic/clinic.py index 50a64ce6409757..e5a5937837c0e2 100755 --- a/Tools/clinic/clinic.py +++ b/Tools/clinic/clinic.py @@ -4832,10 +4832,10 @@ def state_dsl_start(self, line: str) -> None: @staticmethod def parse_names(line: str) -> tuple[str, str]: - left, _as, right = line.partition(' as ') + left, as_, right = line.partition(' as ') full_name = left.strip() c_basename = right.strip() - if _as and not c_basename: + if as_ and not c_basename: fail("No C basename provided after 'as' keyword") if not c_basename: fields = full_name.split(".") From a915dc91042a3c329e001023e13ebc98c79303d4 Mon Sep 17 00:00:00 2001 From: "Erlend E. Aasland" Date: Wed, 16 Aug 2023 13:32:17 +0200 Subject: [PATCH 6/8] Use a named tuple --- Tools/clinic/clinic.py | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/Tools/clinic/clinic.py b/Tools/clinic/clinic.py index e5a5937837c0e2..489cb1d2421453 100755 --- a/Tools/clinic/clinic.py +++ b/Tools/clinic/clinic.py @@ -4567,6 +4567,11 @@ class ParamState(enum.IntEnum): RIGHT_SQUARE_AFTER = 6 +class FunctionNames(NamedTuple): + full_name: str + c_basename: str + + class DSLParser: function: Function | None state: StateKeeper @@ -4846,7 +4851,7 @@ def parse_names(line: str) -> tuple[str, str]: 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 full_name, c_basename + return FunctionNames(full_name=full_name, c_basename=c_basename) def update_function_kind(self, fullname: str) -> None: fields = fullname.split('.') From 3735c9580c454b918c2be6604a291b22d78ce5fa Mon Sep 17 00:00:00 2001 From: "Erlend E. Aasland" Date: Wed, 16 Aug 2023 14:33:58 +0200 Subject: [PATCH 7/8] Update Tools/clinic/clinic.py Co-authored-by: Alex Waygood --- 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 489cb1d2421453..9a10caa50449a6 100755 --- a/Tools/clinic/clinic.py +++ b/Tools/clinic/clinic.py @@ -4836,7 +4836,7 @@ def state_dsl_start(self, line: str) -> None: self.next(self.state_modulename_name, line) @staticmethod - def parse_names(line: str) -> tuple[str, str]: + def parse_names(line: str) -> FunctionNames: left, as_, right = line.partition(' as ') full_name = left.strip() c_basename = right.strip() From 2ee034099928753c1ca94f8889bbc0d7f86b3061 Mon Sep 17 00:00:00 2001 From: "Erlend E. Aasland" Date: Wed, 16 Aug 2023 15:00:41 +0200 Subject: [PATCH 8/8] Improved function name Co-authored-by: Alex Waygood --- Tools/clinic/clinic.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/Tools/clinic/clinic.py b/Tools/clinic/clinic.py index 9a10caa50449a6..9f7c47430772f7 100755 --- a/Tools/clinic/clinic.py +++ b/Tools/clinic/clinic.py @@ -4836,7 +4836,7 @@ def state_dsl_start(self, line: str) -> None: self.next(self.state_modulename_name, line) @staticmethod - def parse_names(line: str) -> FunctionNames: + def parse_function_names(line: str) -> FunctionNames: left, as_, right = line.partition(' as ') full_name = left.strip() c_basename = right.strip() @@ -4891,7 +4891,7 @@ def state_modulename_name(self, line: str) -> None: # are we cloning? before, equals, existing = line.rpartition('=') if equals: - full_name, c_basename = self.parse_names(before) + full_name, c_basename = self.parse_function_names(before) existing = existing.strip() if is_legal_py_identifier(existing): # we're cloning! @@ -4939,7 +4939,7 @@ def state_modulename_name(self, line: str) -> None: line, _, returns = line.partition('->') returns = returns.strip() - full_name, c_basename = self.parse_names(line) + full_name, c_basename = self.parse_function_names(line) return_converter = None if returns: