Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

gh-104683: Argument clinic: remove some unnecessary uses of self.next() in the DSLParser #107635

Closed
wants to merge 1 commit into from

Conversation

AlexWaygood
Copy link
Member

@AlexWaygood AlexWaygood commented Aug 4, 2023

Many state_foo methods on the DSLParser class in clinic.py are called only indirectly via the next() method:

cpython/Tools/clinic/clinic.py

Lines 4645 to 4652 in 407d7fd

def next(
self,
state: StateKeeper,
line: str | None = None
) -> None:
self.state = state
if line is not None:
self.state(line)

The reason for this is that the DSLParser parses argument-clinic input a line at a time; but state often persists across lines. (Example: a docstring can span multiple lines; by saving the "state" at the end of one line, the DSLParser is able to resume parsing with the same state when it begins parsing the next line, and therefore knows that it's in the middle of a docstring.)

However, there are two state_foo methods on the DSLParser class that are arguably badly named, and shouldn't be called via self.next(), even though they both currently are:

  • state_modulename_name
  • state_parameter_docstring_start

Both of these are examples of states that are impossible to span multiple lines. A modulename declaration cannot span multiple lines; nor can the start of a parameter docstring. As such, calling these methods via self.next() is needless indirection, and can be removed.

It is provable that both of these states cannot span multiple lines, due to the fact that neither state_modulename_name nor state_parameter_docstring_start ever exit early before setting self.state to something new (by calling self.next).


def state_modulename_name(self, line: str) -> None:
def parse_modulename_name(self, line: str) -> None:
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I renamed this function to distinguish it from the state_foo functions, all of which must only ever be called indirectly via self.next()

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think of parse_function_declaration?

Suggested change
def parse_modulename_name(self, line: str) -> None:
def parse_function_declaration(self, line: str) -> None:

Tools/clinic/clinic.py Show resolved Hide resolved
@erlend-aasland
Copy link
Contributor

I'm not sure (yet) this is a step in the right direction. By consolidating stages in the state machine, debugging it becomes harder. For example, if I add this debug print:

diff --git a/Tools/clinic/clinic.py b/Tools/clinic/clinic.py
index 6c5a2c7c85..b9d47d2e0e 100755
--- a/Tools/clinic/clinic.py
+++ b/Tools/clinic/clinic.py
@@ -4652,6 +4652,7 @@ def next(
             line: str | None = None
     ) -> None:
         self.state = state
+        print(f"--> {state.__name__:25} {line=!r}")
         if line is not None:
             self.state(line)
 

I get this:

$ cat test.c
/*[clinic input]
module m
m.func
    a: int
[clinic start generated code]*/

static PyObject *
m_func_impl(PyObject *module, int a)
/*[clinic end generated code: output=188ac0e0fa832273 input=e686625fcf7cad0e]*/
$ python3.12 Tools/clinic/clinic.py test.c
--> state_modulename_name     line='m.func'
--> state_parameters_start    line=None
--> state_parameter           line='    a: int'

However, with this PR, I now get:

$ python3.12 Tools/clinic/clinic.py test.c
--> state_parameters_start    line=None
--> state_parameter           line='    a: int'

@AlexWaygood
Copy link
Member Author

AlexWaygood commented Aug 4, 2023

Right, that's a very good point, and I think exposes that my mental model of the state machine here was subtly wrong. I think there may be another solution to the "But wait, these branches are unreachable!" problem I came up against in Argument-Clinic#14; I'll have a play around.

@AlexWaygood AlexWaygood marked this pull request as draft August 4, 2023 16:39
@AlexWaygood AlexWaygood closed this Aug 4, 2023
@AlexWaygood AlexWaygood deleted the unnecessary-next-calls branch August 4, 2023 17:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants