From 230b85f2133246321e761476882bd50f55d05195 Mon Sep 17 00:00:00 2001 From: "Erlend E. Aasland" Date: Tue, 4 Jul 2023 00:56:40 +0200 Subject: [PATCH 1/7] Annotate --- 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 311f0a1a56a038..59eba16a9583c0 100755 --- a/Tools/clinic/clinic.py +++ b/Tools/clinic/clinic.py @@ -5146,7 +5146,9 @@ def parse_converter( "Annotations must be either a name, a function call, or a string." ) - def parse_special_symbol(self, symbol): + def parse_special_symbol(self, symbol: str): + assert isinstance(self.function, Function) + if symbol == '*': if self.keyword_only: fail("Function " + self.function.name + " uses '*' more than once.") From 98273d45629ba7a826a90249493a4dc23e126d4f Mon Sep 17 00:00:00 2001 From: "Erlend E. Aasland" Date: Tue, 4 Jul 2023 01:30:57 +0200 Subject: [PATCH 2/7] Refactor: extract functions --- Tools/clinic/clinic.py | 124 ++++++++++++++++++++++++----------------- 1 file changed, 73 insertions(+), 51 deletions(-) diff --git a/Tools/clinic/clinic.py b/Tools/clinic/clinic.py index 59eba16a9583c0..f3ec62388f3d2e 100755 --- a/Tools/clinic/clinic.py +++ b/Tools/clinic/clinic.py @@ -5146,59 +5146,81 @@ def parse_converter( "Annotations must be either a name, a function call, or a string." ) + def parse_star(self, function: Function) -> None: + if self.keyword_only: + fail(f"Function {function.name} uses '*' more than once.") + self.keyword_only = True + + def parse_open_bracket(self, function: Function) -> None: + match self.parameter_state: + case ParamState.START | ParamState.LEFT_SQUARE_BEFORE: + self.parameter_state = ParamState.LEFT_SQUARE_BEFORE + case ParamState.REQUIRED | ParamState.GROUP_AFTER: + self.parameter_state = ParamState.GROUP_AFTER + case st: + fail(f"Function {function.name} has an unsupported group configuration. " + f"(Unexpected state {st}.b)") + self.group += 1 + self.function.docstring_only = True + + def parse_close_bracket(self, function: Function) -> None: + if not self.group: + fail(f"Function {function.name} has a ] without a matching [.") + if not any(p.group == self.group for p in function.parameters.values()): + fail(f"Function {function.name} has an empty group.\n" + "All groups must contain at least one parameter.") + self.group -= 1 + match self.parameter_state: + case ParamState.LEFT_SQUARE_BEFORE | ParamState.GROUP_BEFORE: + self.parameter_state = ParamState.GROUP_BEFORE + case ParamState.GROUP_AFTER | ParamState.RIGHT_SQUARE_AFTER: + self.parameter_state = ParamState.RIGHT_SQUARE_AFTER + case st: + fail(f"Function {function.name} has an unsupported group configuration. " + f"(Unexpected state {st}.c)") + + def parse_slash(self, function: Function) -> None: + if self.positional_only: + fail(f"Function {function.name} uses '/' more than once.") + self.positional_only = True + # REQUIRED and OPTIONAL are allowed here, that allows positional-only + # without option groups to work (and have default values!) + allowed = { + ParamState.REQUIRED, + ParamState.OPTIONAL, + ParamState.RIGHT_SQUARE_AFTER, + ParamState.GROUP_BEFORE, + } + if (self.parameter_state not in allowed) or self.group: + fail(f"Function {function.name} has an unsupported group configuration. " + f"(Unexpected state {self.parameter_state}.d)") + if self.keyword_only: + fail(f"Function {function.name} mixes keyword-only and " + "positional-only parameters, which is unsupported.") + # fixup preceding parameters + for p in self.function.parameters.values(): + if p.is_vararg(): + continue + if (p.kind != inspect.Parameter.POSITIONAL_OR_KEYWORD and + not isinstance(p.converter, self_converter) + ): + fail(f"Function {function.name} mixes keyword-only and " + "positional-only parameters, which is unsupported.") + p.kind = inspect.Parameter.POSITIONAL_ONLY + def parse_special_symbol(self, symbol: str): assert isinstance(self.function, Function) - - if symbol == '*': - if self.keyword_only: - fail("Function " + self.function.name + " uses '*' more than once.") - self.keyword_only = True - elif symbol == '[': - match self.parameter_state: - case ParamState.START | ParamState.LEFT_SQUARE_BEFORE: - self.parameter_state = ParamState.LEFT_SQUARE_BEFORE - case ParamState.REQUIRED | ParamState.GROUP_AFTER: - self.parameter_state = ParamState.GROUP_AFTER - case st: - fail(f"Function {self.function.name} has an unsupported group configuration. (Unexpected state {st}.b)") - self.group += 1 - self.function.docstring_only = True - elif symbol == ']': - if not self.group: - fail("Function " + self.function.name + " has a ] without a matching [.") - if not any(p.group == self.group for p in self.function.parameters.values()): - fail("Function " + self.function.name + " has an empty group.\nAll groups must contain at least one parameter.") - self.group -= 1 - match self.parameter_state: - case ParamState.LEFT_SQUARE_BEFORE | ParamState.GROUP_BEFORE: - self.parameter_state = ParamState.GROUP_BEFORE - case ParamState.GROUP_AFTER | ParamState.RIGHT_SQUARE_AFTER: - self.parameter_state = ParamState.RIGHT_SQUARE_AFTER - case st: - fail(f"Function {self.function.name} has an unsupported group configuration. (Unexpected state {st}.c)") - elif symbol == '/': - if self.positional_only: - fail("Function " + self.function.name + " uses '/' more than once.") - self.positional_only = True - # REQUIRED and OPTIONAL are allowed here, that allows positional-only without option groups - # to work (and have default values!) - allowed = { - ParamState.REQUIRED, - ParamState.OPTIONAL, - ParamState.RIGHT_SQUARE_AFTER, - ParamState.GROUP_BEFORE, - } - if (self.parameter_state not in allowed) or self.group: - fail(f"Function {self.function.name} has an unsupported group configuration. (Unexpected state {self.parameter_state}.d)") - if self.keyword_only: - fail("Function " + self.function.name + " mixes keyword-only and positional-only parameters, which is unsupported.") - # fixup preceding parameters - for p in self.function.parameters.values(): - if p.is_vararg(): - continue - if (p.kind != inspect.Parameter.POSITIONAL_OR_KEYWORD and not isinstance(p.converter, self_converter)): - fail("Function " + self.function.name + " mixes keyword-only and positional-only parameters, which is unsupported.") - p.kind = inspect.Parameter.POSITIONAL_ONLY + func = self.function + + match symbol: + case '*': + self.parse_star(func) + case '[': + self.parse_open_bracket(func) + case ']': + self.parse_close_bracket(func) + case '/': + self.parse_slash(func) def state_parameter_docstring_start(self, line: str | None) -> None: self.parameter_docstring_indent = len(self.indent.margin) From 6994d098094dea49b75cda580303adf3d0dbb9cc Mon Sep 17 00:00:00 2001 From: "Erlend E. Aasland" Date: Mon, 17 Jul 2023 22:08:42 +0200 Subject: [PATCH 3/7] Fix mypy warnings --- 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 f3ec62388f3d2e..e88a34267d8f79 100755 --- a/Tools/clinic/clinic.py +++ b/Tools/clinic/clinic.py @@ -5161,7 +5161,7 @@ def parse_open_bracket(self, function: Function) -> None: fail(f"Function {function.name} has an unsupported group configuration. " f"(Unexpected state {st}.b)") self.group += 1 - self.function.docstring_only = True + function.docstring_only = True def parse_close_bracket(self, function: Function) -> None: if not self.group: @@ -5198,7 +5198,7 @@ def parse_slash(self, function: Function) -> None: fail(f"Function {function.name} mixes keyword-only and " "positional-only parameters, which is unsupported.") # fixup preceding parameters - for p in self.function.parameters.values(): + for p in function.parameters.values(): if p.is_vararg(): continue if (p.kind != inspect.Parameter.POSITIONAL_OR_KEYWORD and From 8e36b5111558a5a8d888740639da1c493dc47bbd Mon Sep 17 00:00:00 2001 From: "Erlend E. Aasland" Date: Mon, 17 Jul 2023 22:12:49 +0200 Subject: [PATCH 4/7] Remove parse_special_symbol() --- Tools/clinic/clinic.py | 28 ++++++++++------------------ 1 file changed, 10 insertions(+), 18 deletions(-) diff --git a/Tools/clinic/clinic.py b/Tools/clinic/clinic.py index e88a34267d8f79..60bcc2464b7766 100755 --- a/Tools/clinic/clinic.py +++ b/Tools/clinic/clinic.py @@ -4865,10 +4865,16 @@ def state_parameter(self, line: str | None) -> None: return line = line.lstrip() - - if line in ('*', '/', '[', ']'): - self.parse_special_symbol(line) - return + func = self.function + match line: + case '*': + return self.parse_star(func) + case '[': + return self.parse_open_bracket(func) + case ']': + return self.parse_close_bracket(func) + case '/': + return self.parse_slash(func) match self.parameter_state: case ParamState.START | ParamState.REQUIRED: @@ -5208,20 +5214,6 @@ def parse_slash(self, function: Function) -> None: "positional-only parameters, which is unsupported.") p.kind = inspect.Parameter.POSITIONAL_ONLY - def parse_special_symbol(self, symbol: str): - assert isinstance(self.function, Function) - func = self.function - - match symbol: - case '*': - self.parse_star(func) - case '[': - self.parse_open_bracket(func) - case ']': - self.parse_close_bracket(func) - case '/': - self.parse_slash(func) - def state_parameter_docstring_start(self, line: str | None) -> None: self.parameter_docstring_indent = len(self.indent.margin) assert self.indent.depth == 3 From 1a08f1312cdfd3cace20518bbf030583e563c040 Mon Sep 17 00:00:00 2001 From: Alex Waygood Date: Mon, 17 Jul 2023 23:16:31 +0200 Subject: [PATCH 5/7] Address code review: exhaustive match --- Tools/clinic/clinic.py | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/Tools/clinic/clinic.py b/Tools/clinic/clinic.py index 60bcc2464b7766..426619cad2d026 100755 --- a/Tools/clinic/clinic.py +++ b/Tools/clinic/clinic.py @@ -4868,13 +4868,18 @@ def state_parameter(self, line: str | None) -> None: func = self.function match line: case '*': - return self.parse_star(func) + self.parse_star(func) case '[': - return self.parse_open_bracket(func) + self.parse_open_bracket(func) case ']': - return self.parse_close_bracket(func) + self.parse_close_bracket(func) case '/': - return self.parse_slash(func) + self.parse_slash(func) + case param: + self.parse_parameter(param) + + def parse_parameter(self, line: str) -> None: + assert self.function is not None match self.parameter_state: case ParamState.START | ParamState.REQUIRED: From deb05a27448aa4fa99e3bedbd24f486f21170aee Mon Sep 17 00:00:00 2001 From: "Erlend E. Aasland" Date: Mon, 17 Jul 2023 23:32:21 +0200 Subject: [PATCH 6/7] Address review --- Tools/clinic/clinic.py | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/Tools/clinic/clinic.py b/Tools/clinic/clinic.py index 426619cad2d026..91516704df84f6 100755 --- a/Tools/clinic/clinic.py +++ b/Tools/clinic/clinic.py @@ -4864,15 +4864,14 @@ def state_parameter(self, line: str | None) -> None: self.parameter_continuation = line[:-1] return - line = line.lstrip() func = self.function - match line: + match line.lstrip(): case '*': self.parse_star(func) case '[': - self.parse_open_bracket(func) + self.parse_opening_square_bracket(func) case ']': - self.parse_close_bracket(func) + self.parse_closing_square_bracket(func) case '/': self.parse_slash(func) case param: @@ -5158,11 +5157,13 @@ def parse_converter( ) def parse_star(self, function: Function) -> None: + """Parse keyword-only parameter marker '*'.""" if self.keyword_only: fail(f"Function {function.name} uses '*' more than once.") self.keyword_only = True - def parse_open_bracket(self, function: Function) -> None: + def parse_opening_square_bracket(self, function: Function) -> None: + """Parse opening parameter group symbol '['.""" match self.parameter_state: case ParamState.START | ParamState.LEFT_SQUARE_BEFORE: self.parameter_state = ParamState.LEFT_SQUARE_BEFORE @@ -5174,7 +5175,8 @@ def parse_open_bracket(self, function: Function) -> None: self.group += 1 function.docstring_only = True - def parse_close_bracket(self, function: Function) -> None: + def parse_closing_square_bracket(self, function: Function) -> None: + """Parse closing parameter group symbol '['.""" if not self.group: fail(f"Function {function.name} has a ] without a matching [.") if not any(p.group == self.group for p in function.parameters.values()): @@ -5191,6 +5193,7 @@ def parse_close_bracket(self, function: Function) -> None: f"(Unexpected state {st}.c)") def parse_slash(self, function: Function) -> None: + """Parse positional-only parameter marker '/'.""" if self.positional_only: fail(f"Function {function.name} uses '/' more than once.") self.positional_only = True From b0ae589410eaf0f90e611794e30e537450387ef8 Mon Sep 17 00:00:00 2001 From: "Erlend E. Aasland" Date: Mon, 17 Jul 2023 23:46:10 +0200 Subject: [PATCH 7/7] Apply suggestions from code review 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 91516704df84f6..bff8935df13bc6 100755 --- a/Tools/clinic/clinic.py +++ b/Tools/clinic/clinic.py @@ -5176,7 +5176,7 @@ def parse_opening_square_bracket(self, function: Function) -> None: function.docstring_only = True def parse_closing_square_bracket(self, function: Function) -> None: - """Parse closing parameter group symbol '['.""" + """Parse closing parameter group symbol ']'.""" if not self.group: fail(f"Function {function.name} has a ] without a matching [.") if not any(p.group == self.group for p in function.parameters.values()): @@ -5215,7 +5215,7 @@ def parse_slash(self, function: Function) -> None: for p in function.parameters.values(): if p.is_vararg(): continue - if (p.kind != inspect.Parameter.POSITIONAL_OR_KEYWORD and + if (p.kind is not inspect.Parameter.POSITIONAL_OR_KEYWORD and not isinstance(p.converter, self_converter) ): fail(f"Function {function.name} mixes keyword-only and "