From 4b84af3470e01a8e2d7ea37e2d4f45f0d3e9f747 Mon Sep 17 00:00:00 2001 From: "Erlend E. Aasland" Date: Mon, 3 Jul 2023 11:58:16 +0200 Subject: [PATCH 1/5] Add ParamState class --- Tools/clinic/clinic.py | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/Tools/clinic/clinic.py b/Tools/clinic/clinic.py index 2e46131d4e336a..b7bf89fb15b021 100755 --- a/Tools/clinic/clinic.py +++ b/Tools/clinic/clinic.py @@ -4292,6 +4292,32 @@ def dedent(self, line): StateKeeper = Callable[[str | None], None] +class ParamState(enum.IntEnum): + # Before we've seen anything. + # Legal transitions: to LEFT_SQUARE_BEFORE or REQUIRED + START = enum.auto() + + # Left square backets before required params. + LEFT_SQUARE_BEFORE = enum.auto() + + # In a group, before required params. + GROUP_BEFORE = enum.auto() + + # Required params, positional-or-keyword or positional-only (we + # don't know yet). Renumber lefft groups! + REQUIRED = enum.auto() + + # Positional-or-keyword or positional-only params that now must have + # default values. + OPTIONAL = enum.auto() + + # In a group, after required params. + GROUP_AFTER = enum.auto() + + # Right square brackets after required params. + RIGHT_SQUARE_AFTER = enum.auto() + + class DSLParser: function: Function | None state: StateKeeper From c5c9e27a0aaaffd4e29eb9fbbb7ec314f9833977 Mon Sep 17 00:00:00 2001 From: "Erlend E. Aasland" Date: Mon, 3 Jul 2023 12:07:16 +0200 Subject: [PATCH 2/5] Use ParamState iso. ints --- Tools/clinic/clinic.py | 85 +++++++++++++++++++----------------------- 1 file changed, 38 insertions(+), 47 deletions(-) diff --git a/Tools/clinic/clinic.py b/Tools/clinic/clinic.py index b7bf89fb15b021..ac44955a03e27b 100755 --- a/Tools/clinic/clinic.py +++ b/Tools/clinic/clinic.py @@ -4293,29 +4293,34 @@ def dedent(self, line): StateKeeper = Callable[[str | None], None] class ParamState(enum.IntEnum): + """Parameter parsing state. + + [ [ a, b, ] c, ] d, e, f=3, [ g, h, [ i ] ] <- line + 01 2 3 4 5 6 <- state transitions + """ # Before we've seen anything. # Legal transitions: to LEFT_SQUARE_BEFORE or REQUIRED - START = enum.auto() + START = 0 # Left square backets before required params. - LEFT_SQUARE_BEFORE = enum.auto() + LEFT_SQUARE_BEFORE = 1 # In a group, before required params. - GROUP_BEFORE = enum.auto() + GROUP_BEFORE = 2 # Required params, positional-or-keyword or positional-only (we # don't know yet). Renumber lefft groups! - REQUIRED = enum.auto() + REQUIRED = 3 # Positional-or-keyword or positional-only params that now must have # default values. - OPTIONAL = enum.auto() + OPTIONAL = 4 # In a group, after required params. - GROUP_AFTER = enum.auto() + GROUP_AFTER = 5 # Right square brackets after required params. - RIGHT_SQUARE_AFTER = enum.auto() + RIGHT_SQUARE_AFTER = 6 class DSLParser: @@ -4355,7 +4360,7 @@ def reset(self) -> None: self.keyword_only = False self.positional_only = False self.group = 0 - self.parameter_state = self.ps_start + self.parameter_state: ParamState = ParamState.START self.seen_positional_with_default = False self.indent = IndentStack() self.kind = CALLABLE @@ -4748,22 +4753,8 @@ def state_modulename_name(self, line): # # These rules are enforced with a single state variable: # "parameter_state". (Previously the code was a miasma of ifs and - # separate boolean state variables.) The states are: - # - # [ [ a, b, ] c, ] d, e, f=3, [ g, h, [ i ] ] <- line - # 01 2 3 4 5 6 <- state transitions - # - # 0: ps_start. before we've seen anything. legal transitions are to 1 or 3. - # 1: ps_left_square_before. left square brackets before required parameters. - # 2: ps_group_before. in a group, before required parameters. - # 3: ps_required. required parameters, positional-or-keyword or positional-only - # (we don't know yet). (renumber left groups!) - # 4: ps_optional. positional-or-keyword or positional-only parameters that - # now must have default values. - # 5: ps_group_after. in a group, after required parameters. - # 6: ps_right_square_after. right square brackets after required parameters. - ps_start, ps_left_square_before, ps_group_before, ps_required, \ - ps_optional, ps_group_after, ps_right_square_after = range(7) + # separate boolean state variables.) The states are defined in the + # ParamState class. def state_parameters_start(self, line: str) -> None: if not self.valid_line(line): @@ -4781,8 +4772,8 @@ def to_required(self): """ Transition to the "required" parameter state. """ - if self.parameter_state != self.ps_required: - self.parameter_state = self.ps_required + if self.parameter_state != ParamState.REQUIRED: + self.parameter_state = ParamState.REQUIRED for p in self.function.parameters.values(): p.group = -p.group @@ -4815,14 +4806,14 @@ def state_parameter(self, line): self.parse_special_symbol(line) return - if self.parameter_state in (self.ps_start, self.ps_required): + if self.parameter_state in (ParamState.START, ParamState.REQUIRED): self.to_required() - elif self.parameter_state == self.ps_left_square_before: - self.parameter_state = self.ps_group_before - elif self.parameter_state == self.ps_group_before: + elif self.parameter_state == ParamState.LEFT_SQUARE_BEFORE: + self.parameter_state = ParamState.GROUP_BEFORE + elif self.parameter_state == ParamState.GROUP_BEFORE: if not self.group: self.to_required() - elif self.parameter_state in (self.ps_group_after, self.ps_optional): + elif self.parameter_state in (ParamState.GROUP_AFTER, ParamState.OPTIONAL): pass else: fail("Function " + self.function.name + " has an unsupported group configuration. (Unexpected state " + str(self.parameter_state) + ".a)") @@ -4885,7 +4876,7 @@ def state_parameter(self, line): name, legacy, kwargs = self.parse_converter(parameter.annotation) if not default: - if self.parameter_state == self.ps_optional: + if self.parameter_state == ParamState.OPTIONAL: fail("Can't have a parameter without a default (" + repr(parameter_name) + ")\nafter a parameter with a default!") if is_vararg: value = NULL @@ -4898,8 +4889,8 @@ def state_parameter(self, line): if is_vararg: fail("Vararg can't take a default value!") - if self.parameter_state == self.ps_required: - self.parameter_state = self.ps_optional + if self.parameter_state == ParamState.REQUIRED: + self.parameter_state = ParamState.OPTIONAL default = default.strip() bad = False ast_input = f"x = {default}" @@ -5023,14 +5014,14 @@ def bad_node(self, node): if isinstance(converter, self_converter): if len(self.function.parameters) == 1: - if (self.parameter_state != self.ps_required): + if (self.parameter_state != ParamState.REQUIRED): fail("A 'self' parameter cannot be marked optional.") if value is not unspecified: fail("A 'self' parameter cannot have a default value.") if self.group: fail("A 'self' parameter cannot be in an optional group.") kind = inspect.Parameter.POSITIONAL_ONLY - self.parameter_state = self.ps_start + self.parameter_state = ParamState.START self.function.parameters.clear() else: fail("A 'self' parameter, if specified, must be the very first thing in the parameter block.") @@ -5038,7 +5029,7 @@ def bad_node(self, node): if isinstance(converter, defining_class_converter): _lp = len(self.function.parameters) if _lp == 1: - if (self.parameter_state != self.ps_required): + if (self.parameter_state != ParamState.REQUIRED): fail("A 'defining_class' parameter cannot be marked optional.") if value is not unspecified: fail("A 'defining_class' parameter cannot have a default value.") @@ -5086,10 +5077,10 @@ def parse_special_symbol(self, symbol): fail("Function " + self.function.name + " uses '*' more than once.") self.keyword_only = True elif symbol == '[': - if self.parameter_state in (self.ps_start, self.ps_left_square_before): - self.parameter_state = self.ps_left_square_before - elif self.parameter_state in (self.ps_required, self.ps_group_after): - self.parameter_state = self.ps_group_after + if self.parameter_state in (ParamState.START, ParamState.LEFT_SQUARE_BEFORE): + self.parameter_state = ParamState.LEFT_SQUARE_BEFORE + elif self.parameter_state in (ParamState.REQUIRED, ParamState.GROUP_AFTER): + self.parameter_state = ParamState.GROUP_AFTER else: fail("Function " + self.function.name + " has an unsupported group configuration. (Unexpected state " + str(self.parameter_state) + ".b)") self.group += 1 @@ -5100,19 +5091,19 @@ def parse_special_symbol(self, symbol): 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 - if self.parameter_state in (self.ps_left_square_before, self.ps_group_before): - self.parameter_state = self.ps_group_before - elif self.parameter_state in (self.ps_group_after, self.ps_right_square_after): - self.parameter_state = self.ps_right_square_after + if self.parameter_state in (ParamState.LEFT_SQUARE_BEFORE, ParamState.GROUP_BEFORE): + self.parameter_state = ParamState.GROUP_BEFORE + elif self.parameter_state in (ParamState.GROUP_AFTER, ParamState.RIGHT_SQUARE_AFTER): + self.parameter_state = ParamState.RIGHT_SQUARE_AFTER else: fail("Function " + self.function.name + " has an unsupported group configuration. (Unexpected state " + str(self.parameter_state) + ".c)") elif symbol == '/': if self.positional_only: fail("Function " + self.function.name + " uses '/' more than once.") self.positional_only = True - # ps_required and ps_optional are allowed here, that allows positional-only without option groups + # REQUIREDD and OPTIONAL are allowed here, that allows positional-only without option groups # to work (and have default values!) - if (self.parameter_state not in (self.ps_required, self.ps_optional, self.ps_right_square_after, self.ps_group_before)) or self.group: + if (self.parameter_state not in (ParamState.REQUIRED, ParamState.OPTIONAL, ParamState.RIGHT_SQUARE_AFTER, ParamState.GROUP_BEFORE)) or self.group: fail("Function " + self.function.name + " has an unsupported group configuration. (Unexpected state " + str(self.parameter_state) + ".d)") if self.keyword_only: fail("Function " + self.function.name + " mixes keyword-only and positional-only parameters, which is unsupported.") From d13e99409691fa26bb9aee5facf9a2076f91f115 Mon Sep 17 00:00:00 2001 From: "Erlend E. Aasland" Date: Mon, 3 Jul 2023 12:46:25 +0200 Subject: [PATCH 3/5] Use patttern matching iso. if ladder --- Tools/clinic/clinic.py | 21 +++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-) diff --git a/Tools/clinic/clinic.py b/Tools/clinic/clinic.py index ac44955a03e27b..8dd8434a58647c 100755 --- a/Tools/clinic/clinic.py +++ b/Tools/clinic/clinic.py @@ -4806,17 +4806,18 @@ def state_parameter(self, line): self.parse_special_symbol(line) return - if self.parameter_state in (ParamState.START, ParamState.REQUIRED): - self.to_required() - elif self.parameter_state == ParamState.LEFT_SQUARE_BEFORE: - self.parameter_state = ParamState.GROUP_BEFORE - elif self.parameter_state == ParamState.GROUP_BEFORE: - if not self.group: + match self.parameter_state: + case ParamState.START | ParamState.REQUIRED: self.to_required() - elif self.parameter_state in (ParamState.GROUP_AFTER, ParamState.OPTIONAL): - pass - else: - fail("Function " + self.function.name + " has an unsupported group configuration. (Unexpected state " + str(self.parameter_state) + ".a)") + case ParamState.LEFT_SQUARE_BEFORE: + self.parameter_state = ParamState.GROUP_BEFORE + case ParamState.GROUP_BEFORE: + if not self.group: + self.to_required() + case ParamState.GROUP_AFTER | ParamState.OPTIONAL: + pass + case st: + fail(f"Function {self.function.name} has an unsupported group configuration. (Unexpected state {st}.a)") # handle "as" for parameters too c_name = None From 1dfe15f3195ee8c60d9eeed152cb3e2df1f3a213 Mon Sep 17 00:00:00 2001 From: "Erlend E. Aasland" Date: Mon, 3 Jul 2023 12:55:31 +0200 Subject: [PATCH 4/5] Cleanup --- Tools/clinic/clinic.py | 41 +++++++++++++++++++++++++---------------- 1 file changed, 25 insertions(+), 16 deletions(-) diff --git a/Tools/clinic/clinic.py b/Tools/clinic/clinic.py index 8dd8434a58647c..9cffc291fa3c2a 100755 --- a/Tools/clinic/clinic.py +++ b/Tools/clinic/clinic.py @@ -4878,7 +4878,8 @@ def state_parameter(self, line): if not default: if self.parameter_state == ParamState.OPTIONAL: - fail("Can't have a parameter without a default (" + repr(parameter_name) + ")\nafter a parameter with a default!") + fail(f"Can't have a parameter without a default ({parameter_name!r})\n" + "after a parameter with a default!") if is_vararg: value = NULL kwargs.setdefault('c_default', "NULL") @@ -5078,12 +5079,13 @@ def parse_special_symbol(self, symbol): fail("Function " + self.function.name + " uses '*' more than once.") self.keyword_only = True elif symbol == '[': - if self.parameter_state in (ParamState.START, ParamState.LEFT_SQUARE_BEFORE): - self.parameter_state = ParamState.LEFT_SQUARE_BEFORE - elif self.parameter_state in (ParamState.REQUIRED, ParamState.GROUP_AFTER): - self.parameter_state = ParamState.GROUP_AFTER - else: - fail("Function " + self.function.name + " has an unsupported group configuration. (Unexpected state " + str(self.parameter_state) + ".b)") + 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 == ']': @@ -5092,20 +5094,27 @@ def parse_special_symbol(self, symbol): 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 - if self.parameter_state in (ParamState.LEFT_SQUARE_BEFORE, ParamState.GROUP_BEFORE): - self.parameter_state = ParamState.GROUP_BEFORE - elif self.parameter_state in (ParamState.GROUP_AFTER, ParamState.RIGHT_SQUARE_AFTER): - self.parameter_state = ParamState.RIGHT_SQUARE_AFTER - else: - fail("Function " + self.function.name + " has an unsupported group configuration. (Unexpected state " + str(self.parameter_state) + ".c)") + 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 - # REQUIREDD and OPTIONAL are allowed here, that allows positional-only without option groups + # REQUIRED and OPTIONAL are allowed here, that allows positional-only without option groups # to work (and have default values!) - if (self.parameter_state not in (ParamState.REQUIRED, ParamState.OPTIONAL, ParamState.RIGHT_SQUARE_AFTER, ParamState.GROUP_BEFORE)) or self.group: - fail("Function " + self.function.name + " has an unsupported group configuration. (Unexpected state " + str(self.parameter_state) + ".d)") + 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 From 46952717742b2653c82b3748533894ceea4db11a Mon Sep 17 00:00:00 2001 From: "Erlend E. Aasland" Date: Mon, 3 Jul 2023 15:30:30 +0200 Subject: [PATCH 5/5] Apply suggestions from code review Co-authored-by: Oleg Iarygin Co-authored-by: Alex Waygood --- Tools/clinic/clinic.py | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/Tools/clinic/clinic.py b/Tools/clinic/clinic.py index 9cffc291fa3c2a..0fe7eaa6b32521 100755 --- a/Tools/clinic/clinic.py +++ b/Tools/clinic/clinic.py @@ -4309,7 +4309,7 @@ class ParamState(enum.IntEnum): GROUP_BEFORE = 2 # Required params, positional-or-keyword or positional-only (we - # don't know yet). Renumber lefft groups! + # don't know yet). Renumber left groups! REQUIRED = 3 # Positional-or-keyword or positional-only params that now must have @@ -4772,7 +4772,7 @@ def to_required(self): """ Transition to the "required" parameter state. """ - if self.parameter_state != ParamState.REQUIRED: + if self.parameter_state is not ParamState.REQUIRED: self.parameter_state = ParamState.REQUIRED for p in self.function.parameters.values(): p.group = -p.group @@ -4877,7 +4877,7 @@ def state_parameter(self, line): name, legacy, kwargs = self.parse_converter(parameter.annotation) if not default: - if self.parameter_state == ParamState.OPTIONAL: + if self.parameter_state is ParamState.OPTIONAL: fail(f"Can't have a parameter without a default ({parameter_name!r})\n" "after a parameter with a default!") if is_vararg: @@ -4891,7 +4891,7 @@ def state_parameter(self, line): if is_vararg: fail("Vararg can't take a default value!") - if self.parameter_state == ParamState.REQUIRED: + if self.parameter_state is ParamState.REQUIRED: self.parameter_state = ParamState.OPTIONAL default = default.strip() bad = False @@ -5016,7 +5016,7 @@ def bad_node(self, node): if isinstance(converter, self_converter): if len(self.function.parameters) == 1: - if (self.parameter_state != ParamState.REQUIRED): + if self.parameter_state is not ParamState.REQUIRED: fail("A 'self' parameter cannot be marked optional.") if value is not unspecified: fail("A 'self' parameter cannot have a default value.") @@ -5031,7 +5031,7 @@ def bad_node(self, node): if isinstance(converter, defining_class_converter): _lp = len(self.function.parameters) if _lp == 1: - if (self.parameter_state != ParamState.REQUIRED): + if self.parameter_state is not ParamState.REQUIRED: fail("A 'defining_class' parameter cannot be marked optional.") if value is not unspecified: fail("A 'defining_class' parameter cannot have a default value.") @@ -5107,12 +5107,12 @@ def parse_special_symbol(self, symbol): self.positional_only = True # REQUIRED and OPTIONAL are allowed here, that allows positional-only without option groups # to work (and have default values!) - allowed = ( + 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: