From 851f63caa98e62054d51e597384417d551643adc Mon Sep 17 00:00:00 2001 From: "Erlend E. Aasland" Date: Mon, 3 Jul 2023 13:50:20 +0200 Subject: [PATCH 01/14] gh-106359: Fix corner case bugs in Argument Clinic converter parser DSLParser.parse_converter() could return unusable kwdicts in some rare cases --- Lib/test/test_clinic.py | 23 ++++++++++------------- Tools/clinic/clinic.py | 17 +++++++++-------- 2 files changed, 19 insertions(+), 21 deletions(-) diff --git a/Lib/test/test_clinic.py b/Lib/test/test_clinic.py index a6ce9c2570cb62..108dc15c83356d 100644 --- a/Lib/test/test_clinic.py +++ b/Lib/test/test_clinic.py @@ -797,21 +797,18 @@ def test_other_bizarre_things_in_annotations_fail(self): Error on line 0: Annotations must be either a name, a function call, or a string. """ - - s = self.parse_function_should_fail( - 'module os\nos.access\n path: {"some": "dictionary"}' - ) - self.assertEqual(s, expected_failure_message) - - s = self.parse_function_should_fail( - 'module os\nos.access\n path: ["list", "of", "strings"]' + dataset = ( + 'module os\nos.access\n path: {"some": "dictionary"}', + 'module os\nos.access\n path: ["list", "of", "strings"]', + 'module os\nos.access\n path: (x for x in range(42))', + 'module fo\nfo.barbaz\n o: bool(**{None: "bang!"})', + 'module fo\nfo.barbaz -> bool(**{None: "bang!"})', ) - self.assertEqual(s, expected_failure_message) - s = self.parse_function_should_fail( - 'module os\nos.access\n path: (x for x in range(42))' - ) - self.assertEqual(s, expected_failure_message) + for fn in dataset: + with self.subTest(fn=fn): + actual = self.parse_function_should_fail(fn) + self.assertEqual(actual, expected_failure_message) def test_unused_param(self): block = self.parse(""" diff --git a/Tools/clinic/clinic.py b/Tools/clinic/clinic.py index 2e46131d4e336a..a7a4d3dc93d1a5 100755 --- a/Tools/clinic/clinic.py +++ b/Tools/clinic/clinic.py @@ -5033,10 +5033,11 @@ def bad_node(self, node): key = f"{parameter_name}_as_{c_name}" if c_name else parameter_name self.function.parameters[key] = p - KwargDict = dict[str | None, Any] + KwargDict = dict[str, Any] @staticmethod def parse_converter(annotation: ast.expr | None) -> tuple[str, bool, KwargDict]: + msg = "Annotations must be either a name, a function call, or a string." match annotation: case ast.Constant(value=str() as value): return value, True, {} @@ -5044,15 +5045,15 @@ def parse_converter(annotation: ast.expr | None) -> tuple[str, bool, KwargDict]: return name, False, {} case ast.Call(func=ast.Name(name)): symbols = globals() - kwargs = { - node.arg: eval_ast_expr(node.value, symbols) - for node in annotation.keywords - } + kwargs: dict[str, Any] = {} + for node in annotation.keywords: + if not isinstance(node.arg, str): + fail(msg) + kwargs[node.arg] = eval_ast_expr(node.value, symbols) + print(kwargs) return name, False, kwargs case _: - fail( - "Annotations must be either a name, a function call, or a string." - ) + fail(msg) def parse_special_symbol(self, symbol): if symbol == '*': From bb879ccebd9fe5ba5fb2bbb8168c550c00d6a5dd Mon Sep 17 00:00:00 2001 From: "Erlend E. Aasland" Date: Mon, 3 Jul 2023 13:57:57 +0200 Subject: [PATCH 02/14] Move KwargDict up and name it slightly better --- Tools/clinic/clinic.py | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/Tools/clinic/clinic.py b/Tools/clinic/clinic.py index a7a4d3dc93d1a5..eaf4f95cb6351a 100755 --- a/Tools/clinic/clinic.py +++ b/Tools/clinic/clinic.py @@ -4291,6 +4291,7 @@ def dedent(self, line): StateKeeper = Callable[[str | None], None] +ConverterArgs = dict[str, Any] class DSLParser: function: Function | None @@ -5033,10 +5034,10 @@ def bad_node(self, node): key = f"{parameter_name}_as_{c_name}" if c_name else parameter_name self.function.parameters[key] = p - KwargDict = dict[str, Any] - @staticmethod - def parse_converter(annotation: ast.expr | None) -> tuple[str, bool, KwargDict]: + def parse_converter( + annotation: ast.expr | None + ) -> tuple[str, bool, ConverterArgs]: msg = "Annotations must be either a name, a function call, or a string." match annotation: case ast.Constant(value=str() as value): @@ -5045,7 +5046,7 @@ def parse_converter(annotation: ast.expr | None) -> tuple[str, bool, KwargDict]: return name, False, {} case ast.Call(func=ast.Name(name)): symbols = globals() - kwargs: dict[str, Any] = {} + kwargs: ConverterArgs = {} for node in annotation.keywords: if not isinstance(node.arg, str): fail(msg) From 07abdd5af4d9381f8ab6fde3e861847b3ec5aa04 Mon Sep 17 00:00:00 2001 From: "Erlend E. Aasland" Date: Mon, 3 Jul 2023 14:02:20 +0200 Subject: [PATCH 03/14] Remove debug print --- Tools/clinic/clinic.py | 1 - 1 file changed, 1 deletion(-) diff --git a/Tools/clinic/clinic.py b/Tools/clinic/clinic.py index eaf4f95cb6351a..46c25b1402fe7f 100755 --- a/Tools/clinic/clinic.py +++ b/Tools/clinic/clinic.py @@ -5051,7 +5051,6 @@ def parse_converter( if not isinstance(node.arg, str): fail(msg) kwargs[node.arg] = eval_ast_expr(node.value, symbols) - print(kwargs) return name, False, kwargs case _: fail(msg) From 50097489632191c0c63db2007872af95a2145028 Mon Sep 17 00:00:00 2001 From: "Erlend E. Aasland" Date: Mon, 3 Jul 2023 14:06:25 +0200 Subject: [PATCH 04/14] Add NEWS --- .../Tools-Demos/2023-07-03-14-06-19.gh-issue-106359.RfJuR0.rst | 1 + 1 file changed, 1 insertion(+) create mode 100644 Misc/NEWS.d/next/Tools-Demos/2023-07-03-14-06-19.gh-issue-106359.RfJuR0.rst diff --git a/Misc/NEWS.d/next/Tools-Demos/2023-07-03-14-06-19.gh-issue-106359.RfJuR0.rst b/Misc/NEWS.d/next/Tools-Demos/2023-07-03-14-06-19.gh-issue-106359.RfJuR0.rst new file mode 100644 index 00000000000000..f7d1087e275781 --- /dev/null +++ b/Misc/NEWS.d/next/Tools-Demos/2023-07-03-14-06-19.gh-issue-106359.RfJuR0.rst @@ -0,0 +1 @@ +Fix bizarre bugs. From 288d8ca4f47e1e0e8a108413e54687230d78b115 Mon Sep 17 00:00:00 2001 From: "Erlend E. Aasland" Date: Mon, 3 Jul 2023 14:16:54 +0200 Subject: [PATCH 05/14] Address review: try with a slightly more accurate error message --- Lib/test/test_clinic.py | 27 ++++++++++++++----- ...-07-03-14-06-19.gh-issue-106359.RfJuR0.rst | 3 ++- Tools/clinic/clinic.py | 7 ++--- 3 files changed, 27 insertions(+), 10 deletions(-) diff --git a/Lib/test/test_clinic.py b/Lib/test/test_clinic.py index 108dc15c83356d..fc91498a690a21 100644 --- a/Lib/test/test_clinic.py +++ b/Lib/test/test_clinic.py @@ -797,18 +797,33 @@ def test_other_bizarre_things_in_annotations_fail(self): Error on line 0: Annotations must be either a name, a function call, or a string. """ + + s = self.parse_function_should_fail( + 'module os\nos.access\n path: {"some": "dictionary"}' + ) + self.assertEqual(s, expected_failure_message) + + s = self.parse_function_should_fail( + 'module os\nos.access\n path: ["list", "of", "strings"]' + ) + self.assertEqual(s, expected_failure_message) + + s = self.parse_function_should_fail( + 'module os\nos.access\n path: (x for x in range(42))' + ) + self.assertEqual(s, expected_failure_message) + + def test_bizarre_parseable_annotations(self): + msg = "Annotation dicts must have str keys" dataset = ( - 'module os\nos.access\n path: {"some": "dictionary"}', - 'module os\nos.access\n path: ["list", "of", "strings"]', - 'module os\nos.access\n path: (x for x in range(42))', 'module fo\nfo.barbaz\n o: bool(**{None: "bang!"})', 'module fo\nfo.barbaz -> bool(**{None: "bang!"})', ) - for fn in dataset: with self.subTest(fn=fn): - actual = self.parse_function_should_fail(fn) - self.assertEqual(actual, expected_failure_message) + out = self.parse_function_should_fail(fn) + self.assertTrue(out.startswith("Error on line 0")) + self.assertIn(msg, out) def test_unused_param(self): block = self.parse(""" diff --git a/Misc/NEWS.d/next/Tools-Demos/2023-07-03-14-06-19.gh-issue-106359.RfJuR0.rst b/Misc/NEWS.d/next/Tools-Demos/2023-07-03-14-06-19.gh-issue-106359.RfJuR0.rst index f7d1087e275781..39c57d7b3e5f3e 100644 --- a/Misc/NEWS.d/next/Tools-Demos/2023-07-03-14-06-19.gh-issue-106359.RfJuR0.rst +++ b/Misc/NEWS.d/next/Tools-Demos/2023-07-03-14-06-19.gh-issue-106359.RfJuR0.rst @@ -1 +1,2 @@ -Fix bizarre bugs. +Argument Clinic now explicitly require that keys are string if the provided +parameter annotation is a dict. diff --git a/Tools/clinic/clinic.py b/Tools/clinic/clinic.py index 46c25b1402fe7f..060f53a7abbc1e 100755 --- a/Tools/clinic/clinic.py +++ b/Tools/clinic/clinic.py @@ -5038,7 +5038,6 @@ def bad_node(self, node): def parse_converter( annotation: ast.expr | None ) -> tuple[str, bool, ConverterArgs]: - msg = "Annotations must be either a name, a function call, or a string." match annotation: case ast.Constant(value=str() as value): return value, True, {} @@ -5049,11 +5048,13 @@ def parse_converter( kwargs: ConverterArgs = {} for node in annotation.keywords: if not isinstance(node.arg, str): - fail(msg) + fail("Annotation dicts must have str keys") kwargs[node.arg] = eval_ast_expr(node.value, symbols) return name, False, kwargs case _: - fail(msg) + fail( + "Annotations must be either a name, a function call, or a string." + ) def parse_special_symbol(self, symbol): if symbol == '*': From 7efc3b4156047c0e3983900303620b551ee135ce Mon Sep 17 00:00:00 2001 From: "Erlend E. Aasland" Date: Mon, 3 Jul 2023 14:18:33 +0200 Subject: [PATCH 06/14] kwarg splat --- Lib/test/test_clinic.py | 2 +- Tools/clinic/clinic.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/Lib/test/test_clinic.py b/Lib/test/test_clinic.py index fc91498a690a21..67f5f55922615d 100644 --- a/Lib/test/test_clinic.py +++ b/Lib/test/test_clinic.py @@ -814,7 +814,7 @@ def test_other_bizarre_things_in_annotations_fail(self): self.assertEqual(s, expected_failure_message) def test_bizarre_parseable_annotations(self): - msg = "Annotation dicts must have str keys" + msg = "Cannot use a kwarg splat in a function-call annotation" dataset = ( 'module fo\nfo.barbaz\n o: bool(**{None: "bang!"})', 'module fo\nfo.barbaz -> bool(**{None: "bang!"})', diff --git a/Tools/clinic/clinic.py b/Tools/clinic/clinic.py index 060f53a7abbc1e..47038f98aabd75 100755 --- a/Tools/clinic/clinic.py +++ b/Tools/clinic/clinic.py @@ -5048,7 +5048,7 @@ def parse_converter( kwargs: ConverterArgs = {} for node in annotation.keywords: if not isinstance(node.arg, str): - fail("Annotation dicts must have str keys") + fail("Cannot use a kwarg splat in a function-call annotation") kwargs[node.arg] = eval_ast_expr(node.value, symbols) return name, False, kwargs case _: From 5ac85539ca1cd86a86cbecb3cec256970c8a5c84 Mon Sep 17 00:00:00 2001 From: "Erlend E. Aasland" Date: Mon, 3 Jul 2023 14:24:28 +0200 Subject: [PATCH 07/14] Address review: make the test slightly more accurate Co-authored-by: Alex Waygood --- Lib/test/test_clinic.py | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/Lib/test/test_clinic.py b/Lib/test/test_clinic.py index 67f5f55922615d..f7e6df24ecd94f 100644 --- a/Lib/test/test_clinic.py +++ b/Lib/test/test_clinic.py @@ -814,7 +814,10 @@ def test_other_bizarre_things_in_annotations_fail(self): self.assertEqual(s, expected_failure_message) def test_bizarre_parseable_annotations(self): - msg = "Cannot use a kwarg splat in a function-call annotation" + expected_error_msg = ( + "Error on line 0\n" + "Cannot use a kwarg splat in a function-call annotation" + ) dataset = ( 'module fo\nfo.barbaz\n o: bool(**{None: "bang!"})', 'module fo\nfo.barbaz -> bool(**{None: "bang!"})', @@ -822,8 +825,7 @@ def test_bizarre_parseable_annotations(self): for fn in dataset: with self.subTest(fn=fn): out = self.parse_function_should_fail(fn) - self.assertTrue(out.startswith("Error on line 0")) - self.assertIn(msg, out) + self.assertEqual(out, expected_error_msg) def test_unused_param(self): block = self.parse(""" From 5d88ab6ccb71995487b80d728ffd7d062b632fbb Mon Sep 17 00:00:00 2001 From: "Erlend E. Aasland" Date: Mon, 3 Jul 2023 14:28:40 +0200 Subject: [PATCH 08/14] Apply suggestions from code review Co-authored-by: Alex Waygood --- Lib/test/test_clinic.py | 2 +- .../2023-07-03-14-06-19.gh-issue-106359.RfJuR0.rst | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/Lib/test/test_clinic.py b/Lib/test/test_clinic.py index f7e6df24ecd94f..7232bc64d5fffb 100644 --- a/Lib/test/test_clinic.py +++ b/Lib/test/test_clinic.py @@ -813,7 +813,7 @@ def test_other_bizarre_things_in_annotations_fail(self): ) self.assertEqual(s, expected_failure_message) - def test_bizarre_parseable_annotations(self): + def test_kwarg_splats_disallowed_in_function_call_annotations(self): expected_error_msg = ( "Error on line 0\n" "Cannot use a kwarg splat in a function-call annotation" diff --git a/Misc/NEWS.d/next/Tools-Demos/2023-07-03-14-06-19.gh-issue-106359.RfJuR0.rst b/Misc/NEWS.d/next/Tools-Demos/2023-07-03-14-06-19.gh-issue-106359.RfJuR0.rst index 39c57d7b3e5f3e..e696ff6de173e3 100644 --- a/Misc/NEWS.d/next/Tools-Demos/2023-07-03-14-06-19.gh-issue-106359.RfJuR0.rst +++ b/Misc/NEWS.d/next/Tools-Demos/2023-07-03-14-06-19.gh-issue-106359.RfJuR0.rst @@ -1,2 +1,2 @@ -Argument Clinic now explicitly require that keys are string if the provided -parameter annotation is a dict. +Argument Clinic now explicitly forbids "kwarg splats" in function calls used as +annotations From 2a3e35f756cc27e5f987841d9d76c0b627576f8e Mon Sep 17 00:00:00 2001 From: "Erlend E. Aasland" Date: Mon, 3 Jul 2023 14:29:05 +0200 Subject: [PATCH 09/14] Update Misc/NEWS.d/next/Tools-Demos/2023-07-03-14-06-19.gh-issue-106359.RfJuR0.rst --- .../Tools-Demos/2023-07-03-14-06-19.gh-issue-106359.RfJuR0.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Misc/NEWS.d/next/Tools-Demos/2023-07-03-14-06-19.gh-issue-106359.RfJuR0.rst b/Misc/NEWS.d/next/Tools-Demos/2023-07-03-14-06-19.gh-issue-106359.RfJuR0.rst index e696ff6de173e3..5ef9b23c6d15b0 100644 --- a/Misc/NEWS.d/next/Tools-Demos/2023-07-03-14-06-19.gh-issue-106359.RfJuR0.rst +++ b/Misc/NEWS.d/next/Tools-Demos/2023-07-03-14-06-19.gh-issue-106359.RfJuR0.rst @@ -1,2 +1,2 @@ Argument Clinic now explicitly forbids "kwarg splats" in function calls used as -annotations +annotations. From ec602b56c4e26027d2d04090b4c3314c9472e014 Mon Sep 17 00:00:00 2001 From: Alex Waygood Date: Mon, 3 Jul 2023 13:31:08 +0100 Subject: [PATCH 10/14] fix lint --- .../Tools-Demos/2023-07-03-14-06-19.gh-issue-106359.RfJuR0.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Misc/NEWS.d/next/Tools-Demos/2023-07-03-14-06-19.gh-issue-106359.RfJuR0.rst b/Misc/NEWS.d/next/Tools-Demos/2023-07-03-14-06-19.gh-issue-106359.RfJuR0.rst index 5ef9b23c6d15b0..600c265391ec5b 100644 --- a/Misc/NEWS.d/next/Tools-Demos/2023-07-03-14-06-19.gh-issue-106359.RfJuR0.rst +++ b/Misc/NEWS.d/next/Tools-Demos/2023-07-03-14-06-19.gh-issue-106359.RfJuR0.rst @@ -1,2 +1,2 @@ -Argument Clinic now explicitly forbids "kwarg splats" in function calls used as +Argument Clinic now explicitly forbids "kwarg splats" in function calls used as annotations. From 8fcb6188e147262c04ed7a8f6b952160dc98bf11 Mon Sep 17 00:00:00 2001 From: "Erlend E. Aasland" Date: Mon, 3 Jul 2023 14:43:06 +0200 Subject: [PATCH 11/14] Fix test --- Lib/test/test_clinic.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Lib/test/test_clinic.py b/Lib/test/test_clinic.py index 7232bc64d5fffb..7561ade90f5fec 100644 --- a/Lib/test/test_clinic.py +++ b/Lib/test/test_clinic.py @@ -815,8 +815,8 @@ def test_other_bizarre_things_in_annotations_fail(self): def test_kwarg_splats_disallowed_in_function_call_annotations(self): expected_error_msg = ( - "Error on line 0\n" - "Cannot use a kwarg splat in a function-call annotation" + "Error on line 0:\n" + "Cannot use a kwarg splat in a function-call annotation\n" ) dataset = ( 'module fo\nfo.barbaz\n o: bool(**{None: "bang!"})', From 67425eb68dac259f809ce37a0048be1f380ddecb Mon Sep 17 00:00:00 2001 From: "Erlend E. Aasland" Date: Mon, 3 Jul 2023 14:47:58 +0200 Subject: [PATCH 12/14] Add a few more tests --- Lib/test/test_clinic.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/Lib/test/test_clinic.py b/Lib/test/test_clinic.py index 7561ade90f5fec..7961f080c9c0c5 100644 --- a/Lib/test/test_clinic.py +++ b/Lib/test/test_clinic.py @@ -821,6 +821,8 @@ def test_kwarg_splats_disallowed_in_function_call_annotations(self): dataset = ( 'module fo\nfo.barbaz\n o: bool(**{None: "bang!"})', 'module fo\nfo.barbaz -> bool(**{None: "bang!"})', + 'module fo\nfo.barbaz -> bool(**{bang: None})', + 'module fo\nfo.barbaz\n o: bool(**{bang: None})', ) for fn in dataset: with self.subTest(fn=fn): From 277c01be13d748176d68073514899c8b18d884ae Mon Sep 17 00:00:00 2001 From: "Erlend E. Aasland" Date: Mon, 3 Jul 2023 14:48:45 +0200 Subject: [PATCH 13/14] Don't just use None --- 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 7961f080c9c0c5..063d08e5af3e5d 100644 --- a/Lib/test/test_clinic.py +++ b/Lib/test/test_clinic.py @@ -821,7 +821,7 @@ def test_kwarg_splats_disallowed_in_function_call_annotations(self): dataset = ( 'module fo\nfo.barbaz\n o: bool(**{None: "bang!"})', 'module fo\nfo.barbaz -> bool(**{None: "bang!"})', - 'module fo\nfo.barbaz -> bool(**{bang: None})', + 'module fo\nfo.barbaz -> bool(**{bang: 42})', 'module fo\nfo.barbaz\n o: bool(**{bang: None})', ) for fn in dataset: From 99ad510198c23ec71a47dbf099e3224e556f071b Mon Sep 17 00:00:00 2001 From: "Erlend E. Aasland" Date: Mon, 3 Jul 2023 14:49:16 +0200 Subject: [PATCH 14/14] quotes --- Lib/test/test_clinic.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Lib/test/test_clinic.py b/Lib/test/test_clinic.py index 063d08e5af3e5d..fab2d2bb0bdbe4 100644 --- a/Lib/test/test_clinic.py +++ b/Lib/test/test_clinic.py @@ -821,8 +821,8 @@ def test_kwarg_splats_disallowed_in_function_call_annotations(self): dataset = ( 'module fo\nfo.barbaz\n o: bool(**{None: "bang!"})', 'module fo\nfo.barbaz -> bool(**{None: "bang!"})', - 'module fo\nfo.barbaz -> bool(**{bang: 42})', - 'module fo\nfo.barbaz\n o: bool(**{bang: None})', + 'module fo\nfo.barbaz -> bool(**{"bang": 42})', + 'module fo\nfo.barbaz\n o: bool(**{"bang": None})', ) for fn in dataset: with self.subTest(fn=fn):