[PATCH v2 0/6] qapi: static typing conversion, pt5b

[PATCH v2 0/6] qapi: static typing conversion, pt5b

[John Snow]

This is part five (b), and focuses on QAPIDoc in parser.py.

gitlab: https://gitlab.com/jsnow/qemu/-/commits/python-qapi-cleanup-pt5b

Requirements:
- Python 3.6+
- mypy >= 0.770
- pylint >= 2.6.0 (2.7.0+ when using Python 3.9+)

Every commit should pass with:
 - `isort -c qapi/`
 - `flake8 qapi/`
 - `pylint --rcfile=qapi/pylintrc qapi/`
 - `mypy --config-file=qapi/mypy.ini qapi/`

V2:
 - Changed patch 01 to fix error message.
 - Add a TODO for fixing the cycle in patch 03.
 - Changed some commit messages, patch names

John Snow (6):
  qapi/parser: fix unused check_args_section arguments
  qapi/parser: Allow empty QAPIDoc Sections
  qapi/parser: add type hint annotations (QAPIDoc)
  qapi/parser: enable mypy checks
  qapi/parser: Silence too-few-public-methods warning
  qapi/parser: enable pylint checks

 scripts/qapi/mypy.ini                 |  5 --
 scripts/qapi/parser.py                | 98 +++++++++++++++++----------
 scripts/qapi/pylintrc                 |  3 +-
 tests/qapi-schema/doc-bad-feature.err |  2 +-
 4 files changed, 64 insertions(+), 44 deletions(-)

-- 
2.30.2

[PATCH v2 1/6] qapi/parser: fix unused check_args_section arguments

[John Snow]

Pylint informs us we're not using these arguments. Oops, it's
right. Correct the error message and remove the remaining unused
parameter.

Fix test output now that the error message is improved.

Fixes: e151941d1b
Signed-off-by: John Snow <jsnow@redhat.com>
---
 scripts/qapi/parser.py                | 16 +++++++++-------
 tests/qapi-schema/doc-bad-feature.err |  2 +-
 2 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
index 06167ed3e0a..b6a5e661215 100644
--- a/scripts/qapi/parser.py
+++ b/scripts/qapi/parser.py
@@ -753,16 +753,18 @@ def check_expr(self, expr):
 
     def check(self):
 
-        def check_args_section(args, info, what):
+        def check_args_section(args, what):
             bogus = [name for name, section in args.items()
                      if not section.member]
             if bogus:
                 raise QAPISemError(
                     self.info,
-                    "documented member%s '%s' %s not exist"
-                    % ("s" if len(bogus) > 1 else "",
-                       "', '".join(bogus),
-                       "do" if len(bogus) > 1 else "does"))
+                    "documented %s%s '%s' %s not exist" % (
+                        what,
+                        "s" if len(bogus) > 1 else "",
+                        "', '".join(bogus),
+                        "do" if len(bogus) > 1 else "does"
+                    ))
 
-        check_args_section(self.args, self.info, 'members')
-        check_args_section(self.features, self.info, 'features')
+        check_args_section(self.args, 'member')
+        check_args_section(self.features, 'feature')
diff --git a/tests/qapi-schema/doc-bad-feature.err b/tests/qapi-schema/doc-bad-feature.err
index e4c62adfa3e..49d1746c3d1 100644
--- a/tests/qapi-schema/doc-bad-feature.err
+++ b/tests/qapi-schema/doc-bad-feature.err
@@ -1 +1 @@
-doc-bad-feature.json:3: documented member 'a' does not exist
+doc-bad-feature.json:3: documented feature 'a' does not exist
-- 
2.30.2

[PATCH v2 2/6] qapi/parser: Allow empty QAPIDoc Sections

[John Snow]

It simplifies the typing to say that _section is always a
QAPIDoc.Section().

To accommodate this change, we must allow for this object to evaluate to
False for functions like _end_section which behave differently based on
whether or not a Section has been started.

Signed-off-by: John Snow <jsnow@redhat.com>

---

Probably a better fix is to restructure the code to prevent empty
sections from being "ended", but that seems like a bigger whale than
what I'm after at the immediate moment.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 scripts/qapi/parser.py | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
index b6a5e661215..3ddde318376 100644
--- a/scripts/qapi/parser.py
+++ b/scripts/qapi/parser.py
@@ -456,6 +456,9 @@ def __init__(self, parser, name=None, indent=0):
             # the expected indent level of the text of this section
             self._indent = indent
 
+        def __bool__(self) -> bool:
+            return bool(self.name or self.text)
+
         def append(self, line):
             # Strip leading spaces corresponding to the expected indent level
             # Blank lines are always OK.
@@ -722,7 +725,7 @@ def _end_section(self):
                 raise QAPIParseError(
                     self._parser,
                     "empty doc section '%s'" % self._section.name)
-            self._section = None
+            self._section = QAPIDoc.Section(self._parser)
 
     def _append_freeform(self, line):
         match = re.match(r'(@\S+:)', line)
-- 
2.30.2

[PATCH v2 3/6] qapi/parser: add type hint annotations (QAPIDoc)

[John Snow]

Annotations do not change runtime behavior.

This commit adds mostly annotations, but uses a TYPE_CHECKING runtime
check to conditionally import dependencies, which only triggers during
runs of mypy.

Signed-off-by: John Snow <jsnow@redhat.com>

---

TopLevelExpr, an idea from previous drafts, makes a return here in order
to give a semantic meaning to check_expr(). The type is intended to be
used more in forthcoming commits (pt5c), but I opted to include it now
instead of creating yet-another Dict[str, object] type hint that I would
forget to change later.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 scripts/qapi/parser.py | 77 ++++++++++++++++++++++++++----------------
 1 file changed, 48 insertions(+), 29 deletions(-)

diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
index 3ddde318376..b1e2fa5c577 100644
--- a/scripts/qapi/parser.py
+++ b/scripts/qapi/parser.py
@@ -18,6 +18,7 @@
 import os
 import re
 from typing import (
+    TYPE_CHECKING,
     Dict,
     List,
     Optional,
@@ -30,6 +31,15 @@
 from .source import QAPISourceInfo
 
 
+if TYPE_CHECKING:
+    # pylint: disable=cyclic-import
+    # TODO: Remove cycle. [schema -> expr -> parser -> schema]
+    from .schema import QAPISchemaFeature, QAPISchemaMember
+
+
+#: Represents a single Top Level QAPI schema expression.
+TopLevelExpr = Dict[str, object]
+
 # Return value alias for get_expr().
 _ExprValue = Union[List[object], Dict[str, object], str, bool]
 
@@ -447,7 +457,8 @@ class QAPIDoc:
     """
 
     class Section:
-        def __init__(self, parser, name=None, indent=0):
+        def __init__(self, parser: QAPISchemaParser,
+                     name: Optional[str] = None, indent: int = 0):
             # parser, for error messages about indentation
             self._parser = parser
             # optional section name (argument/member or section name)
@@ -459,7 +470,7 @@ def __init__(self, parser, name=None, indent=0):
         def __bool__(self) -> bool:
             return bool(self.name or self.text)
 
-        def append(self, line):
+        def append(self, line: str) -> None:
             # Strip leading spaces corresponding to the expected indent level
             # Blank lines are always OK.
             if line:
@@ -474,39 +485,40 @@ def append(self, line):
             self.text += line.rstrip() + '\n'
 
     class ArgSection(Section):
-        def __init__(self, parser, name, indent=0):
+        def __init__(self, parser: QAPISchemaParser,
+                     name: Optional[str] = None, indent: int = 0):
             super().__init__(parser, name, indent)
-            self.member = None
+            self.member: Optional['QAPISchemaMember'] = None
 
-        def connect(self, member):
+        def connect(self, member: 'QAPISchemaMember') -> None:
             self.member = member
 
-    def __init__(self, parser, info):
+    def __init__(self, parser: QAPISchemaParser, info: QAPISourceInfo):
         # self._parser is used to report errors with QAPIParseError.  The
         # resulting error position depends on the state of the parser.
         # It happens to be the beginning of the comment.  More or less
         # servicable, but action at a distance.
         self._parser = parser
         self.info = info
-        self.symbol = None
+        self.symbol: Optional[str] = None
         self.body = QAPIDoc.Section(parser)
         # dict mapping parameter name to ArgSection
-        self.args = OrderedDict()
-        self.features = OrderedDict()
+        self.args: Dict[str, QAPIDoc.ArgSection] = OrderedDict()
+        self.features: Dict[str, QAPIDoc.ArgSection] = OrderedDict()
         # a list of Section
-        self.sections = []
+        self.sections: List[QAPIDoc.Section] = []
         # the current section
         self._section = self.body
         self._append_line = self._append_body_line
 
-    def has_section(self, name):
+    def has_section(self, name: str) -> bool:
         """Return True if we have a section with this name."""
         for i in self.sections:
             if i.name == name:
                 return True
         return False
 
-    def append(self, line):
+    def append(self, line: str) -> None:
         """
         Parse a comment line and add it to the documentation.
 
@@ -527,18 +539,18 @@ def append(self, line):
         line = line[1:]
         self._append_line(line)
 
-    def end_comment(self):
+    def end_comment(self) -> None:
         self._end_section()
 
     @staticmethod
-    def _is_section_tag(name):
+    def _is_section_tag(name: str) -> bool:
         return name in ('Returns:', 'Since:',
                         # those are often singular or plural
                         'Note:', 'Notes:',
                         'Example:', 'Examples:',
                         'TODO:')
 
-    def _append_body_line(self, line):
+    def _append_body_line(self, line: str) -> None:
         """
         Process a line of documentation text in the body section.
 
@@ -578,7 +590,7 @@ def _append_body_line(self, line):
             # This is a free-form documentation block
             self._append_freeform(line)
 
-    def _append_args_line(self, line):
+    def _append_args_line(self, line: str) -> None:
         """
         Process a line of documentation text in an argument section.
 
@@ -624,7 +636,7 @@ def _append_args_line(self, line):
 
         self._append_freeform(line)
 
-    def _append_features_line(self, line):
+    def _append_features_line(self, line: str) -> None:
         name = line.split(' ', 1)[0]
 
         if name.startswith('@') and name.endswith(':'):
@@ -656,7 +668,7 @@ def _append_features_line(self, line):
 
         self._append_freeform(line)
 
-    def _append_various_line(self, line):
+    def _append_various_line(self, line: str) -> None:
         """
         Process a line of documentation text in an additional section.
 
@@ -692,7 +704,11 @@ def _append_various_line(self, line):
 
         self._append_freeform(line)
 
-    def _start_symbol_section(self, symbols_dict, name, indent):
+    def _start_symbol_section(
+            self,
+            symbols_dict: Dict[str, 'QAPIDoc.ArgSection'],
+            name: str,
+            indent: int) -> None:
         # FIXME invalid names other than the empty string aren't flagged
         if not name:
             raise QAPIParseError(self._parser, "invalid parameter name")
@@ -704,13 +720,14 @@ def _start_symbol_section(self, symbols_dict, name, indent):
         self._section = QAPIDoc.ArgSection(self._parser, name, indent)
         symbols_dict[name] = self._section
 
-    def _start_args_section(self, name, indent):
+    def _start_args_section(self, name: str, indent: int) -> None:
         self._start_symbol_section(self.args, name, indent)
 
-    def _start_features_section(self, name, indent):
+    def _start_features_section(self, name: str, indent: int) -> None:
         self._start_symbol_section(self.features, name, indent)
 
-    def _start_section(self, name=None, indent=0):
+    def _start_section(self, name: Optional[str] = None,
+                       indent: int = 0) -> None:
         if name in ('Returns', 'Since') and self.has_section(name):
             raise QAPIParseError(self._parser,
                                  "duplicated '%s' section" % name)
@@ -718,7 +735,7 @@ def _start_section(self, name=None, indent=0):
         self._section = QAPIDoc.Section(self._parser, name, indent)
         self.sections.append(self._section)
 
-    def _end_section(self):
+    def _end_section(self) -> None:
         if self._section:
             text = self._section.text = self._section.text.strip()
             if self._section.name and (not text or text.isspace()):
@@ -727,7 +744,7 @@ def _end_section(self):
                     "empty doc section '%s'" % self._section.name)
             self._section = QAPIDoc.Section(self._parser)
 
-    def _append_freeform(self, line):
+    def _append_freeform(self, line: str) -> None:
         match = re.match(r'(@\S+:)', line)
         if match:
             raise QAPIParseError(self._parser,
@@ -735,28 +752,30 @@ def _append_freeform(self, line):
                                  % match.group(1))
         self._section.append(line)
 
-    def connect_member(self, member):
+    def connect_member(self, member: 'QAPISchemaMember') -> None:
         if member.name not in self.args:
             # Undocumented TODO outlaw
             self.args[member.name] = QAPIDoc.ArgSection(self._parser,
                                                         member.name)
         self.args[member.name].connect(member)
 
-    def connect_feature(self, feature):
+    def connect_feature(self, feature: 'QAPISchemaFeature') -> None:
         if feature.name not in self.features:
             raise QAPISemError(feature.info,
                                "feature '%s' lacks documentation"
                                % feature.name)
         self.features[feature.name].connect(feature)
 
-    def check_expr(self, expr):
+    def check_expr(self, expr: TopLevelExpr) -> None:
         if self.has_section('Returns') and 'command' not in expr:
             raise QAPISemError(self.info,
                                "'Returns:' is only valid for commands")
 
-    def check(self):
+    def check(self) -> None:
 
-        def check_args_section(args, what):
+        def check_args_section(
+                args: Dict[str, QAPIDoc.ArgSection], what: str
+        ) -> None:
             bogus = [name for name, section in args.items()
                      if not section.member]
             if bogus:
-- 
2.30.2

[PATCH v2 4/6] qapi/parser: enable mypy checks

[John Snow]

Signed-off-by: John Snow <jsnow@redhat.com>

---

As always, this can be merged with the previous commit.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 scripts/qapi/mypy.ini | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/scripts/qapi/mypy.ini b/scripts/qapi/mypy.ini
index 54ca4483d6d..66253564297 100644
--- a/scripts/qapi/mypy.ini
+++ b/scripts/qapi/mypy.ini
@@ -3,11 +3,6 @@ strict = True
 disallow_untyped_calls = False
 python_version = 3.6
 
-[mypy-qapi.parser]
-disallow_untyped_defs = False
-disallow_incomplete_defs = False
-check_untyped_defs = False
-
 [mypy-qapi.schema]
 disallow_untyped_defs = False
 disallow_incomplete_defs = False
-- 
2.30.2

[PATCH v2 5/6] qapi/parser: Silence too-few-public-methods warning

[John Snow]

Eh. Two properties, a bool method and a public method are non-trivial
enough for me. (Especially in typed python!)

Signed-off-by: John Snow <jsnow@redhat.com>
---
 scripts/qapi/parser.py | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
index b1e2fa5c577..618e6e5b0dd 100644
--- a/scripts/qapi/parser.py
+++ b/scripts/qapi/parser.py
@@ -459,6 +459,8 @@ class QAPIDoc:
     class Section:
         def __init__(self, parser: QAPISchemaParser,
                      name: Optional[str] = None, indent: int = 0):
+            # pylint: disable=too-few-public-methods
+
             # parser, for error messages about indentation
             self._parser = parser
             # optional section name (argument/member or section name)
-- 
2.30.2

[PATCH v2 6/6] qapi/parser: enable pylint checks

[John Snow]

Signed-off-by: John Snow <jsnow@redhat.com>

---

This can be merged with the previous commit, if desired.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 scripts/qapi/pylintrc | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/scripts/qapi/pylintrc b/scripts/qapi/pylintrc
index c5275d5f59b..1a633b2b88e 100644
--- a/scripts/qapi/pylintrc
+++ b/scripts/qapi/pylintrc
@@ -2,8 +2,7 @@
 
 # Add files or directories matching the regex patterns to the ignore list.
 # The regex matches against base names, not paths.
-ignore-patterns=parser.py,
-                schema.py,
+ignore-patterns=schema.py,
 
 
 [MESSAGES CONTROL]
-- 
2.30.2

Re: [PATCH v2 0/6] qapi: static typing conversion, pt5b

[John Snow]

On 5/20/21 6:57 PM, John Snow wrote:
> This is part five (b), and focuses on QAPIDoc in parser.py.
> 
> gitlab: https://gitlab.com/jsnow/qemu/-/commits/python-qapi-cleanup-pt5b
> 
> Requirements:
> - Python 3.6+
> - mypy >= 0.770
> - pylint >= 2.6.0 (2.7.0+ when using Python 3.9+)
> 
> Every commit should pass with:
>   - `isort -c qapi/`
>   - `flake8 qapi/`
>   - `pylint --rcfile=qapi/pylintrc qapi/`
>   - `mypy --config-file=qapi/mypy.ini qapi/`
> 
> V2:
>   - Changed patch 01 to fix error message.
>   - Add a TODO for fixing the cycle in patch 03.
>   - Changed some commit messages, patch names
> 
> John Snow (6):
>    qapi/parser: fix unused check_args_section arguments
>    qapi/parser: Allow empty QAPIDoc Sections
>    qapi/parser: add type hint annotations (QAPIDoc)
>    qapi/parser: enable mypy checks
>    qapi/parser: Silence too-few-public-methods warning
>    qapi/parser: enable pylint checks
> 
>   scripts/qapi/mypy.ini                 |  5 --
>   scripts/qapi/parser.py                | 98 +++++++++++++++++----------
>   scripts/qapi/pylintrc                 |  3 +-
>   tests/qapi-schema/doc-bad-feature.err |  2 +-
>   4 files changed, 64 insertions(+), 44 deletions(-)
> 

 From memory, where we left off was:

- What is our plan for eliminating the cycle in QAPIDoc?
- What's the actual situation with these "empty" sections?

And my answer to both problems as of today is:

... I'm not really sure yet, but I have a lot of preliminary work 
building up on implementing a cross-referenceable QAPI domain for 
sphinx, which might necessitate some heavier changes to how QAPIDoc 
information is parsed and stored.

I'd like to cover fixing both design problems of QAPIDoc at that time if 
you'll let me sweep the dirt under the mat until then. I can add FIXME 
comments to the code -- at the moment, the configuration of ./python/ 
does not tolerate TODO nor FIXME comments, and I do intend to move 
./scripts/qapi to ./python/qemu/qapi once we finish typing it, so you 
can be assured that I'll have to address those to do the thing I want.

In the meantime it means a blemish in the form of TYPE_CHECKING, but it 
lets us get on with everything else in the meantime.

Worst case scenario: A meteorite pierces my skull and the work goes 
unfinished. parser.py type checks but has some FIXME comments jangling 
around that Someone:tm: has to fix, but the Heat Death Of The Universe 
occurs first. Nobody has any energy left to be dissatisfied with the way 
things wound up.

--js

Re: [PATCH v2 2/6] qapi/parser: Allow empty QAPIDoc Sections

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> It simplifies the typing to say that _section is always a
> QAPIDoc.Section().

If you say so....

> To accommodate this change, we must allow for this object to evaluate to
> False for functions like _end_section which behave differently based on
> whether or not a Section has been started.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
>
> ---
>
> Probably a better fix is to restructure the code to prevent empty
> sections from being "ended", but that seems like a bigger whale than
> what I'm after at the immediate moment.

Do we have a TODO comment for that?

>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  scripts/qapi/parser.py | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
> index b6a5e661215..3ddde318376 100644
> --- a/scripts/qapi/parser.py
> +++ b/scripts/qapi/parser.py
> @@ -456,6 +456,9 @@ def __init__(self, parser, name=None, indent=0):
>              # the expected indent level of the text of this section
>              self._indent = indent
>  
> +        def __bool__(self) -> bool:
> +            return bool(self.name or self.text)
> +
>          def append(self, line):
>              # Strip leading spaces corresponding to the expected indent level
>              # Blank lines are always OK.

Overriding __bool__() is the minimally invasive compensation for the
next hunk's replacement of None by a QAPIDoc.Section

However, I'm wary of overriding __bool__().  It creates a difference
between "if object:" and "if object is not None:".  Gives me a queasy
feeling, as shortening the latter to the former is pretty much
automatic.

A boring .is_empty() would avoid that, but we'd have to adjust "if S" to
"if S.is_empty()" wherever we changed S from Optional[Section] to
Section.  Which S could be affected?

The following variables get assigned Section or ArgSection:

    QAPIDoc.body
    QAPIDoc._section
    QAPIDoc.args[NAME]
    QAPIDoc.features[NAME]

.body, .args[NAME] and .features[NAME] are never None I believe.

._section is also assigned None, in ._end_section().  It remains None
until the next ._start*_section().

The only use of .section that doesn't dot into it is in ._end_section().
That's the only spot to adjust.

Confirm by testing: in all of "make check", Section.__bool__() is only
ever called from QAPIDoc._end_section().  Checked by sticking
traceback.print_stack() into .__bool__().

> @@ -722,7 +725,7 @@ def _end_section(self):
>                  raise QAPIParseError(
>                      self._parser,
>                      "empty doc section '%s'" % self._section.name)
> -            self._section = None
> +            self._section = QAPIDoc.Section(self._parser)
>  
>      def _append_freeform(self, line):
>          match = re.match(r'(@\S+:)', line)

Replacing None by QAPIDoc.Section doesn't just simplify the typing!  It
also creates a bogus "additional section" (in the sense of QAPIDoc's
class comment) after any section.  Works, because the .start*_section()
all call .end_section() first, .end_section() does nothing for empty
sections, and the bogus sections remain empty, unless we somehow screw
up the code to add contents to them.  Such screwups are now possible,
whereas before they would crash.

This correctness argument isn't obvious, and therefore should be made in
the commit message.

Re: [PATCH v2 3/6] qapi/parser: add type hint annotations (QAPIDoc)

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> Annotations do not change runtime behavior.
>
> This commit adds mostly annotations, but uses a TYPE_CHECKING runtime
> check to conditionally import dependencies, which only triggers during
> runs of mypy.

Please add a reference to
https://mypy.readthedocs.io/en/latest/runtime_troubles.html#import-cycles

> Signed-off-by: John Snow <jsnow@redhat.com>
>
> ---
>
> TopLevelExpr, an idea from previous drafts, makes a return here in order
> to give a semantic meaning to check_expr(). The type is intended to be
> used more in forthcoming commits (pt5c), but I opted to include it now
> instead of creating yet-another Dict[str, object] type hint that I would
> forget to change later.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  scripts/qapi/parser.py | 77 ++++++++++++++++++++++++++----------------
>  1 file changed, 48 insertions(+), 29 deletions(-)
>
> diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
> index 3ddde318376..b1e2fa5c577 100644
> --- a/scripts/qapi/parser.py
> +++ b/scripts/qapi/parser.py
> @@ -18,6 +18,7 @@
>  import os
>  import re
>  from typing import (
> +    TYPE_CHECKING,
>      Dict,
>      List,
>      Optional,
> @@ -30,6 +31,15 @@
>  from .source import QAPISourceInfo
>  
>  
> +if TYPE_CHECKING:
> +    # pylint: disable=cyclic-import
> +    # TODO: Remove cycle. [schema -> expr -> parser -> schema]
> +    from .schema import QAPISchemaFeature, QAPISchemaMember
> +
> +
> +#: Represents a single Top Level QAPI schema expression.
> +TopLevelExpr = Dict[str, object]

Related: _ExprValue below, and _JSONObject in expr.py.  The latter's
comment gives the best rationale (except I don't get "the purpose of
this module is to interrogate that type").

I think we'd like to have

* A recursive type for JSON value (in our bastardized version of JSON)

  This is Union of bool, str, List[Self], Dict[str, Self].  It's what
  .get_expr() returns.

  Since mypy can't do recursive, we approximate with _ExprValue.

* A recursive type for JSON object

  This is the Dict branch of the above.  It's what check_keys() &
  friends take as argument.

  We approximate with _JSONObject.

  Same for the List branch would make sense if we had a use for the
  type.

* A recursive type for TOP-LEVEL-EXPR

  Actually the same type as the previous one, to be used only for the
  schema's top-level expressions.  It's the elements of
  QAPISchemaParser.exprs[], and what check_exprs() takes as argument.

  We approximate with TopLevelExpr, but so far use it only for
  check_exprs().

  Doesn't really improve type checking, but may serve as documentation.

Shouldn't these types be all defined in one place, namely right here?
Bonus: we need to explain the mypy sadness just once then.

Shouldn't their names be more systematic?  _ExprValue, _JSONObject and
TopLevelExpr hardly suggest any relation...

> +
>  # Return value alias for get_expr().
>  _ExprValue = Union[List[object], Dict[str, object], str, bool]
>  
> @@ -447,7 +457,8 @@ class QAPIDoc:
>      """
>  
>      class Section:
> -        def __init__(self, parser, name=None, indent=0):
> +        def __init__(self, parser: QAPISchemaParser,
> +                     name: Optional[str] = None, indent: int = 0):
>              # parser, for error messages about indentation
>              self._parser = parser
>              # optional section name (argument/member or section name)
> @@ -459,7 +470,7 @@ def __init__(self, parser, name=None, indent=0):
>          def __bool__(self) -> bool:
>              return bool(self.name or self.text)
>  
> -        def append(self, line):
> +        def append(self, line: str) -> None:
>              # Strip leading spaces corresponding to the expected indent level
>              # Blank lines are always OK.
>              if line:
> @@ -474,39 +485,40 @@ def append(self, line):
>              self.text += line.rstrip() + '\n'
>  
>      class ArgSection(Section):
> -        def __init__(self, parser, name, indent=0):
> +        def __init__(self, parser: QAPISchemaParser,
> +                     name: Optional[str] = None, indent: int = 0):

Why not name: str?  All callers pass a str argument...

>              super().__init__(parser, name, indent)
> -            self.member = None
> +            self.member: Optional['QAPISchemaMember'] = None

I guess you need to quote 'QAPISchemaMember', because we actually import
it only if TYPE_CHECKING.  More of the same below.

>  
> -        def connect(self, member):
> +        def connect(self, member: 'QAPISchemaMember') -> None:
>              self.member = member
>  
> -    def __init__(self, parser, info):
> +    def __init__(self, parser: QAPISchemaParser, info: QAPISourceInfo):
>          # self._parser is used to report errors with QAPIParseError.  The
>          # resulting error position depends on the state of the parser.
>          # It happens to be the beginning of the comment.  More or less
>          # servicable, but action at a distance.
>          self._parser = parser
>          self.info = info
> -        self.symbol = None
> +        self.symbol: Optional[str] = None
>          self.body = QAPIDoc.Section(parser)
>          # dict mapping parameter name to ArgSection
> -        self.args = OrderedDict()
> -        self.features = OrderedDict()
> +        self.args: Dict[str, QAPIDoc.ArgSection] = OrderedDict()
> +        self.features: Dict[str, QAPIDoc.ArgSection] = OrderedDict()
>          # a list of Section
> -        self.sections = []
> +        self.sections: List[QAPIDoc.Section] = []
>          # the current section
>          self._section = self.body
>          self._append_line = self._append_body_line
>  
> -    def has_section(self, name):
> +    def has_section(self, name: str) -> bool:
>          """Return True if we have a section with this name."""
>          for i in self.sections:
>              if i.name == name:
>                  return True
>          return False
>  
> -    def append(self, line):
> +    def append(self, line: str) -> None:
>          """
>          Parse a comment line and add it to the documentation.
>  
> @@ -527,18 +539,18 @@ def append(self, line):
>          line = line[1:]
>          self._append_line(line)
>  
> -    def end_comment(self):
> +    def end_comment(self) -> None:
>          self._end_section()
>  
>      @staticmethod
> -    def _is_section_tag(name):
> +    def _is_section_tag(name: str) -> bool:
>          return name in ('Returns:', 'Since:',
>                          # those are often singular or plural
>                          'Note:', 'Notes:',
>                          'Example:', 'Examples:',
>                          'TODO:')
>  
> -    def _append_body_line(self, line):
> +    def _append_body_line(self, line: str) -> None:
>          """
>          Process a line of documentation text in the body section.
>  
> @@ -578,7 +590,7 @@ def _append_body_line(self, line):
>              # This is a free-form documentation block
>              self._append_freeform(line)
>  
> -    def _append_args_line(self, line):
> +    def _append_args_line(self, line: str) -> None:
>          """
>          Process a line of documentation text in an argument section.
>  
> @@ -624,7 +636,7 @@ def _append_args_line(self, line):
>  
>          self._append_freeform(line)
>  
> -    def _append_features_line(self, line):
> +    def _append_features_line(self, line: str) -> None:
>          name = line.split(' ', 1)[0]
>  
>          if name.startswith('@') and name.endswith(':'):
> @@ -656,7 +668,7 @@ def _append_features_line(self, line):
>  
>          self._append_freeform(line)
>  
> -    def _append_various_line(self, line):
> +    def _append_various_line(self, line: str) -> None:
>          """
>          Process a line of documentation text in an additional section.
>  
> @@ -692,7 +704,11 @@ def _append_various_line(self, line):
>  
>          self._append_freeform(line)
>  
> -    def _start_symbol_section(self, symbols_dict, name, indent):
> +    def _start_symbol_section(
> +            self,
> +            symbols_dict: Dict[str, 'QAPIDoc.ArgSection'],

The need to quote this within the very class that defines it is
annoying.

> +            name: str,
> +            indent: int) -> None:
>          # FIXME invalid names other than the empty string aren't flagged
>          if not name:
>              raise QAPIParseError(self._parser, "invalid parameter name")
> @@ -704,13 +720,14 @@ def _start_symbol_section(self, symbols_dict, name, indent):
>          self._section = QAPIDoc.ArgSection(self._parser, name, indent)
>          symbols_dict[name] = self._section
>  
> -    def _start_args_section(self, name, indent):
> +    def _start_args_section(self, name: str, indent: int) -> None:
>          self._start_symbol_section(self.args, name, indent)
>  
> -    def _start_features_section(self, name, indent):
> +    def _start_features_section(self, name: str, indent: int) -> None:
>          self._start_symbol_section(self.features, name, indent)
>  
> -    def _start_section(self, name=None, indent=0):
> +    def _start_section(self, name: Optional[str] = None,
> +                       indent: int = 0) -> None:
>          if name in ('Returns', 'Since') and self.has_section(name):
>              raise QAPIParseError(self._parser,
>                                   "duplicated '%s' section" % name)
> @@ -718,7 +735,7 @@ def _start_section(self, name=None, indent=0):
>          self._section = QAPIDoc.Section(self._parser, name, indent)
>          self.sections.append(self._section)
>  
> -    def _end_section(self):
> +    def _end_section(self) -> None:
>          if self._section:
>              text = self._section.text = self._section.text.strip()
>              if self._section.name and (not text or text.isspace()):
> @@ -727,7 +744,7 @@ def _end_section(self):
>                      "empty doc section '%s'" % self._section.name)
>              self._section = QAPIDoc.Section(self._parser)
>  
> -    def _append_freeform(self, line):
> +    def _append_freeform(self, line: str) -> None:
>          match = re.match(r'(@\S+:)', line)
>          if match:
>              raise QAPIParseError(self._parser,
> @@ -735,28 +752,30 @@ def _append_freeform(self, line):
>                                   % match.group(1))
>          self._section.append(line)
>  
> -    def connect_member(self, member):
> +    def connect_member(self, member: 'QAPISchemaMember') -> None:
>          if member.name not in self.args:
>              # Undocumented TODO outlaw
>              self.args[member.name] = QAPIDoc.ArgSection(self._parser,
>                                                          member.name)
>          self.args[member.name].connect(member)
>  
> -    def connect_feature(self, feature):
> +    def connect_feature(self, feature: 'QAPISchemaFeature') -> None:
>          if feature.name not in self.features:
>              raise QAPISemError(feature.info,
>                                 "feature '%s' lacks documentation"
>                                 % feature.name)
>          self.features[feature.name].connect(feature)
>  
> -    def check_expr(self, expr):
> +    def check_expr(self, expr: TopLevelExpr) -> None:
>          if self.has_section('Returns') and 'command' not in expr:
>              raise QAPISemError(self.info,
>                                 "'Returns:' is only valid for commands")
>  
> -    def check(self):
> +    def check(self) -> None:
>  
> -        def check_args_section(args, what):
> +        def check_args_section(
> +                args: Dict[str, QAPIDoc.ArgSection], what: str
> +        ) -> None:

This is the fourth use of Dict[str, QAPIDoc.ArgSection].  Still fine,
but if we acquire even more, we should consider giving it a name.

>              bogus = [name for name, section in args.items()
>                       if not section.member]
>              if bogus:

Re: [PATCH v2 0/6] qapi: static typing conversion, pt5b

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> This is part five (b), and focuses on QAPIDoc in parser.py.
>
> gitlab: https://gitlab.com/jsnow/qemu/-/commits/python-qapi-cleanup-pt5b
>
> Requirements:
> - Python 3.6+
> - mypy >= 0.770
> - pylint >= 2.6.0 (2.7.0+ when using Python 3.9+)
>
> Every commit should pass with:
>  - `isort -c qapi/`
>  - `flake8 qapi/`
>  - `pylint --rcfile=qapi/pylintrc qapi/`
>  - `mypy --config-file=qapi/mypy.ini qapi/`

Review complete.  Let's discuss my findings, decide what needs to be
addressed now, then see whether I can do it myself or need you to
respin.

Re: [PATCH v2 2/6] qapi/parser: Allow empty QAPIDoc Sections

[John Snow]

[-- Attachment #1: Type: text/plain, Size: 6089 bytes --]

On Tue, Sep 7, 2021 at 4:28 AM Markus Armbruster <armbru@redhat.com> wrote:

> John Snow <jsnow@redhat.com> writes:
>
> > It simplifies the typing to say that _section is always a
> > QAPIDoc.Section().
>
> If you say so....
>
>
I mean, I thought so at the time. I have an aversion to making optional
types and then littering isinstance() checks in places. I like keeping as
much of the typing statically provable as I can without adding runtime
checks. I'm re-evaluating this patch now, and I'll see if there's anything
I can do, but it's hard to predict differences in matters of taste and
style.

One thing an Optional[T] class instance variable can do that might be
helpful is to remind users that they need to check to see if it's present
or not. If I see a path to eliminate those checks, though, I will generally
always take it - eliminates the need to check everywhere else and puts all
the information you need into the static typing. As a habit, I prefer that
when feasible.

> To accommodate this change, we must allow for this object to evaluate to
> > False for functions like _end_section which behave differently based on
> > whether or not a Section has been started.
> >
> > Signed-off-by: John Snow <jsnow@redhat.com>
> >
> > ---
> >
> > Probably a better fix is to restructure the code to prevent empty
> > sections from being "ended", but that seems like a bigger whale than
> > what I'm after at the immediate moment.
>
> Do we have a TODO comment for that?
>
>
Nope. I'll either add one or just fix the root issue, because I want to go
back to writing the cross-referenceable QAPI symbol plugin for sphinx now.
At the time I thought I'd get to fixing some of the issues raised by pt5 a
lot sooner, but.


> >
> > Signed-off-by: John Snow <jsnow@redhat.com>
> > ---
> >  scripts/qapi/parser.py | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
> > index b6a5e661215..3ddde318376 100644
> > --- a/scripts/qapi/parser.py
> > +++ b/scripts/qapi/parser.py
> > @@ -456,6 +456,9 @@ def __init__(self, parser, name=None, indent=0):
> >              # the expected indent level of the text of this section
> >              self._indent = indent
> >
> > +        def __bool__(self) -> bool:
> > +            return bool(self.name or self.text)
> > +
> >          def append(self, line):
> >              # Strip leading spaces corresponding to the expected indent
> level
> >              # Blank lines are always OK.
>
> Overriding __bool__() is the minimally invasive compensation for the
> next hunk's replacement of None by a QAPIDoc.Section
>
> However, I'm wary of overriding __bool__().  It creates a difference
> between "if object:" and "if object is not None:".  Gives me a queasy
> feeling, as shortening the latter to the former is pretty much
> automatic.
>
>
That's just Python, though. If collections are empty, they're falsey. If
QAPIDoc is a collection of documentation lines and we have no lines,
shouldn't that also be falsey?

Nah, I get the apprehension. It's not a strictly linear collection and I
didn't check *every* last field, just name and text. It's kind of a hack
because I'm trying to paper over some deeper(?) problem(??), but it felt
safe because the static typing means we're not likely to confuse the two
cases -- we don't need to distinguish "Absent" from "empty". Or at least,
after this patch we don't need to, anymore. (We'll always have a section,
so an empty section versus no section makes no difference.)

A boring .is_empty() would avoid that, but we'd have to adjust "if S" to
> "if S.is_empty()" wherever we changed S from Optional[Section] to
> Section.  Which S could be affected?
>
>
Feels deeply against the grain of how Python is written to go out of your
way to create an .is_empty() function.


> The following variables get assigned Section or ArgSection:
>
>     QAPIDoc.body
>     QAPIDoc._section
>     QAPIDoc.args[NAME]
>     QAPIDoc.features[NAME]
>
> .body, .args[NAME] and .features[NAME] are never None I believe.
>
> ._section is also assigned None, in ._end_section().  It remains None
> until the next ._start*_section().
>
> The only use of .section that doesn't dot into it is in ._end_section().
> That's the only spot to adjust.
>
> Confirm by testing: in all of "make check", Section.__bool__() is only
> ever called from QAPIDoc._end_section().  Checked by sticking
> traceback.print_stack() into .__bool__().
>
>
If that's the only caller, isn't it then perfectly safe to just use the
patch as written?


> > @@ -722,7 +725,7 @@ def _end_section(self):
> >                  raise QAPIParseError(
> >                      self._parser,
> >                      "empty doc section '%s'" % self._section.name)
> > -            self._section = None
> > +            self._section = QAPIDoc.Section(self._parser)
> >
> >      def _append_freeform(self, line):
> >          match = re.match(r'(@\S+:)', line)
>
> Replacing None by QAPIDoc.Section doesn't just simplify the typing!  It
> also creates a bogus "additional section" (in the sense of QAPIDoc's
> class comment) after any section.  Works, because the .start*_section()
> all call .end_section() first, .end_section() does nothing for empty
> sections, and the bogus sections remain empty, unless we somehow screw
> up the code to add contents to them.  Such screwups are now possible,
> whereas before they would crash.
>
> This correctness argument isn't obvious, and therefore should be made in
> the commit message.
>
>
I'll try to suss out the real problem, because I am not sure how to justify
the hypothetical cases where we might add content to a bogus section,
because I do not fully understand the circumstances in which the bogus
sections are generated. I seem to recall we already *have* them for some
reason, and it's caused by some whitespace issues (in some cases), but
writing tests or proving it doesn't cause a hypothetical future breakage is
just not something I know how to write a commit message for.

So, time to detour into QAPIDoc parsing.

--js

[-- Attachment #2: Type: text/html, Size: 8662 bytes --]

Re: [PATCH v2 3/6] qapi/parser: add type hint annotations (QAPIDoc)

[John Snow]

[-- Attachment #1: Type: text/plain, Size: 15968 bytes --]

On Tue, Sep 7, 2021 at 6:44 AM Markus Armbruster <armbru@redhat.com> wrote:

> John Snow <jsnow@redhat.com> writes:
>
> > Annotations do not change runtime behavior.
> >
> > This commit adds mostly annotations, but uses a TYPE_CHECKING runtime
> > check to conditionally import dependencies, which only triggers during
> > runs of mypy.
>
> Please add a reference to
> https://mypy.readthedocs.io/en/latest/runtime_troubles.html#import-cycles
>
>
OK.


> > Signed-off-by: John Snow <jsnow@redhat.com>
> >
> > ---
> >
> > TopLevelExpr, an idea from previous drafts, makes a return here in order
> > to give a semantic meaning to check_expr(). The type is intended to be
> > used more in forthcoming commits (pt5c), but I opted to include it now
> > instead of creating yet-another Dict[str, object] type hint that I would
> > forget to change later.
> >
> > Signed-off-by: John Snow <jsnow@redhat.com>
> > ---
> >  scripts/qapi/parser.py | 77 ++++++++++++++++++++++++++----------------
> >  1 file changed, 48 insertions(+), 29 deletions(-)
> >
> > diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
> > index 3ddde318376..b1e2fa5c577 100644
> > --- a/scripts/qapi/parser.py
> > +++ b/scripts/qapi/parser.py
> > @@ -18,6 +18,7 @@
> >  import os
> >  import re
> >  from typing import (
> > +    TYPE_CHECKING,
> >      Dict,
> >      List,
> >      Optional,
> > @@ -30,6 +31,15 @@
> >  from .source import QAPISourceInfo
> >
> >
> > +if TYPE_CHECKING:
> > +    # pylint: disable=cyclic-import
> > +    # TODO: Remove cycle. [schema -> expr -> parser -> schema]
>

WRT this todo: you mentioned that you'd prefer having some idea or plan for
how to eliminate the cycle in order to let this band-aid fly. How about
adding a 'doc' member to e.g. QAPISchemaFeature and doing the connection
entirely inside of schema.py, and dropping connect_member() and
connect_feature()?

Would that be serviceable?


> > +    from .schema import QAPISchemaFeature, QAPISchemaMember
> > +
> > +
> > +#: Represents a single Top Level QAPI schema expression.
> > +TopLevelExpr = Dict[str, object]
>
> Related: _ExprValue below, and _JSONObject in expr.py.  The latter's
> comment gives the best rationale (except I don't get "the purpose of
> this module is to interrogate that type").
>
>
in expr.py, the purpose of that module (expr) is explicitly to interrogate
(check, validate) the shape of arbitrary JSON objects. I am saying that a
more strict definition specifically here in expr.py is not necessary
because the entire purpose of expr.py is to, at runtime, verify the shape
of any such variables that might be annotated that way. I am drawing some
distinction to introspect.py, where we're generating that data ourselves --
the stronger types are more viable there, because we know what they are
already.
(again, sorry about mypy's lack of recursive typing, I hate it too, I
promise)


> I think we'd like to have
>
> * A recursive type for JSON value (in our bastardized version of JSON)
>
>   This is Union of bool, str, List[Self], Dict[str, Self].  It's what
>   .get_expr() returns.
>
>   Since mypy can't do recursive, we approximate with _ExprValue.
>
> * A recursive type for JSON object
>
>   This is the Dict branch of the above.  It's what check_keys() &
>   friends take as argument.
>
>   We approximate with _JSONObject.
>
>   Same for the List branch would make sense if we had a use for the
>   type.
>
> * A recursive type for TOP-LEVEL-EXPR
>
>   Actually the same type as the previous one, to be used only for the
>   schema's top-level expressions.  It's the elements of
>   QAPISchemaParser.exprs[], and what check_exprs() takes as argument.
>
>   We approximate with TopLevelExpr, but so far use it only for
>   check_exprs().
>
>   Doesn't really improve type checking, but may serve as documentation.
>
>
That's the intended effect here -- to help highlight which functions
operate on those top-level expressions, and which might be invoked in more
arbitrary cases.
Consider also a hypothetical TOP-LEVEL-EXPR that is actually a bona-fide
object with additional metadata, too. In these cases, the type really will
be different.


> Shouldn't these types be all defined in one place, namely right here?
> Bonus: we need to explain the mypy sadness just once then.
>
> Shouldn't their names be more systematic?  _ExprValue, _JSONObject and
> TopLevelExpr hardly suggest any relation...
>
>
I drop _JSONObject in pt5c, it was a stop-gap. For the rest, I'll see about
a more rigorous consolidation now that we're this far in.

pt5c was intended as a "cleanup" step that did some of that consolidation
of nearly-redundant types; but I wanted all of parser.py under the mypy gun
*first*.
Will you take a raincheck here and we'll focus on the consolidation in the
next series? I agree it's worth doing.

(I can add a 'FIXME' that will 100% need to be fixed before I move
scripts/qapi under python/qemu/qapi -- the linter config there prohibits
them, so you can be sure I can't ignore it.)


> > +
> >  # Return value alias for get_expr().
> >  _ExprValue = Union[List[object], Dict[str, object], str, bool]
> >
> > @@ -447,7 +457,8 @@ class QAPIDoc:
> >      """
> >
> >      class Section:
> > -        def __init__(self, parser, name=None, indent=0):
> > +        def __init__(self, parser: QAPISchemaParser,
> > +                     name: Optional[str] = None, indent: int = 0):
> >              # parser, for error messages about indentation
> >              self._parser = parser
> >              # optional section name (argument/member or section name)
> > @@ -459,7 +470,7 @@ def __init__(self, parser, name=None, indent=0):
> >          def __bool__(self) -> bool:
> >              return bool(self.name or self.text)
> >
> > -        def append(self, line):
> > +        def append(self, line: str) -> None:
> >              # Strip leading spaces corresponding to the expected indent
> level
> >              # Blank lines are always OK.
> >              if line:
> > @@ -474,39 +485,40 @@ def append(self, line):
> >              self.text += line.rstrip() + '\n'
> >
> >      class ArgSection(Section):
> > -        def __init__(self, parser, name, indent=0):
> > +        def __init__(self, parser: QAPISchemaParser,
> > +                     name: Optional[str] = None, indent: int = 0):
>
> Why not name: str?  All callers pass a str argument...
>
>
Maybe a holdover from when I was trying to stick to LSP for initializers. I
think I have since learned that mypy will only whine about LSP if you hold
pointers to the initializer to be invoked in factory functions. I'll fix it
and we'll worry about the LSP warnings if they ever decide to show up.


> >              super().__init__(parser, name, indent)
> > -            self.member = None
> > +            self.member: Optional['QAPISchemaMember'] = None
>
> I guess you need to quote 'QAPISchemaMember', because we actually import
> it only if TYPE_CHECKING.  More of the same below.
>
>
Yep. Future Python versions may be better about this (You don't have to
quote any types, ever -- they're parsed in a later pass) but we just don't
have that luxury yet.

See https://www.python.org/dev/peps/pep-0563/
Also see
https://www.reddit.com/r/Python/comments/muyz5h/pep_563_getting_rolled_back_from_python_310/
for some discussion on why this feature was not made the default for 3.10

(TLDR: It breaks a library called pydantic that largely does exactly what
expr.py does, using type annotations to define the valid shapes of objects.
I'd love to use it in QAPI, actually, ... a discussion for yet another day.)


> >
> > -        def connect(self, member):
> > +        def connect(self, member: 'QAPISchemaMember') -> None:
> >              self.member = member
> >
> > -    def __init__(self, parser, info):
> > +    def __init__(self, parser: QAPISchemaParser, info: QAPISourceInfo):
> >          # self._parser is used to report errors with QAPIParseError.
> The
> >          # resulting error position depends on the state of the parser.
> >          # It happens to be the beginning of the comment.  More or less
> >          # servicable, but action at a distance.
> >          self._parser = parser
> >          self.info = info
> > -        self.symbol = None
> > +        self.symbol: Optional[str] = None
> >          self.body = QAPIDoc.Section(parser)
> >          # dict mapping parameter name to ArgSection
> > -        self.args = OrderedDict()
> > -        self.features = OrderedDict()
> > +        self.args: Dict[str, QAPIDoc.ArgSection] = OrderedDict()
> > +        self.features: Dict[str, QAPIDoc.ArgSection] = OrderedDict()
> >          # a list of Section
> > -        self.sections = []
> > +        self.sections: List[QAPIDoc.Section] = []
> >          # the current section
> >          self._section = self.body
> >          self._append_line = self._append_body_line
> >
> > -    def has_section(self, name):
> > +    def has_section(self, name: str) -> bool:
> >          """Return True if we have a section with this name."""
> >          for i in self.sections:
> >              if i.name == name:
> >                  return True
> >          return False
> >
> > -    def append(self, line):
> > +    def append(self, line: str) -> None:
> >          """
> >          Parse a comment line and add it to the documentation.
> >
> > @@ -527,18 +539,18 @@ def append(self, line):
> >          line = line[1:]
> >          self._append_line(line)
> >
> > -    def end_comment(self):
> > +    def end_comment(self) -> None:
> >          self._end_section()
> >
> >      @staticmethod
> > -    def _is_section_tag(name):
> > +    def _is_section_tag(name: str) -> bool:
> >          return name in ('Returns:', 'Since:',
> >                          # those are often singular or plural
> >                          'Note:', 'Notes:',
> >                          'Example:', 'Examples:',
> >                          'TODO:')
> >
> > -    def _append_body_line(self, line):
> > +    def _append_body_line(self, line: str) -> None:
> >          """
> >          Process a line of documentation text in the body section.
> >
> > @@ -578,7 +590,7 @@ def _append_body_line(self, line):
> >              # This is a free-form documentation block
> >              self._append_freeform(line)
> >
> > -    def _append_args_line(self, line):
> > +    def _append_args_line(self, line: str) -> None:
> >          """
> >          Process a line of documentation text in an argument section.
> >
> > @@ -624,7 +636,7 @@ def _append_args_line(self, line):
> >
> >          self._append_freeform(line)
> >
> > -    def _append_features_line(self, line):
> > +    def _append_features_line(self, line: str) -> None:
> >          name = line.split(' ', 1)[0]
> >
> >          if name.startswith('@') and name.endswith(':'):
> > @@ -656,7 +668,7 @@ def _append_features_line(self, line):
> >
> >          self._append_freeform(line)
> >
> > -    def _append_various_line(self, line):
> > +    def _append_various_line(self, line: str) -> None:
> >          """
> >          Process a line of documentation text in an additional section.
> >
> > @@ -692,7 +704,11 @@ def _append_various_line(self, line):
> >
> >          self._append_freeform(line)
> >
> > -    def _start_symbol_section(self, symbols_dict, name, indent):
> > +    def _start_symbol_section(
> > +            self,
> > +            symbols_dict: Dict[str, 'QAPIDoc.ArgSection'],
>
> The need to quote this within the very class that defines it is
> annoying.
>

Yep. Python has weird scoping rules for nested classes. In my own hobby
code I've largely started avoiding ever using them, because they don't seem
to be worth the trouble.

Long story short: the name isn't available yet because at the time this
annotation is evaluated, we're still in the middle of defining it. I wish
there was a syntax for $thisclass for a nascent self-reference, but alas.

(Yes, I dislike it too. Nothing I can do about it, to my knowledge.)


>
> > +            name: str,
> > +            indent: int) -> None:
> >          # FIXME invalid names other than the empty string aren't flagged
> >          if not name:
> >              raise QAPIParseError(self._parser, "invalid parameter name")
> > @@ -704,13 +720,14 @@ def _start_symbol_section(self, symbols_dict,
> name, indent):
> >          self._section = QAPIDoc.ArgSection(self._parser, name, indent)
> >          symbols_dict[name] = self._section
> >
> > -    def _start_args_section(self, name, indent):
> > +    def _start_args_section(self, name: str, indent: int) -> None:
> >          self._start_symbol_section(self.args, name, indent)
> >
> > -    def _start_features_section(self, name, indent):
> > +    def _start_features_section(self, name: str, indent: int) -> None:
> >          self._start_symbol_section(self.features, name, indent)
> >
> > -    def _start_section(self, name=None, indent=0):
> > +    def _start_section(self, name: Optional[str] = None,
> > +                       indent: int = 0) -> None:
> >          if name in ('Returns', 'Since') and self.has_section(name):
> >              raise QAPIParseError(self._parser,
> >                                   "duplicated '%s' section" % name)
> > @@ -718,7 +735,7 @@ def _start_section(self, name=None, indent=0):
> >          self._section = QAPIDoc.Section(self._parser, name, indent)
> >          self.sections.append(self._section)
> >
> > -    def _end_section(self):
> > +    def _end_section(self) -> None:
> >          if self._section:
> >              text = self._section.text = self._section.text.strip()
> >              if self._section.name and (not text or text.isspace()):
> > @@ -727,7 +744,7 @@ def _end_section(self):
> >                      "empty doc section '%s'" % self._section.name)
> >              self._section = QAPIDoc.Section(self._parser)
> >
> > -    def _append_freeform(self, line):
> > +    def _append_freeform(self, line: str) -> None:
> >          match = re.match(r'(@\S+:)', line)
> >          if match:
> >              raise QAPIParseError(self._parser,
> > @@ -735,28 +752,30 @@ def _append_freeform(self, line):
> >                                   % match.group(1))
> >          self._section.append(line)
> >
> > -    def connect_member(self, member):
> > +    def connect_member(self, member: 'QAPISchemaMember') -> None:
> >          if member.name not in self.args:
> >              # Undocumented TODO outlaw
> >              self.args[member.name] = QAPIDoc.ArgSection(self._parser,
> >                                                          member.name)
> >          self.args[member.name].connect(member)
> >
> > -    def connect_feature(self, feature):
> > +    def connect_feature(self, feature: 'QAPISchemaFeature') -> None:
> >          if feature.name not in self.features:
> >              raise QAPISemError(feature.info,
> >                                 "feature '%s' lacks documentation"
> >                                 % feature.name)
> >          self.features[feature.name].connect(feature)
> >
> > -    def check_expr(self, expr):
> > +    def check_expr(self, expr: TopLevelExpr) -> None:
> >          if self.has_section('Returns') and 'command' not in expr:
> >              raise QAPISemError(self.info,
> >                                 "'Returns:' is only valid for commands")
> >
> > -    def check(self):
> > +    def check(self) -> None:
> >
> > -        def check_args_section(args, what):
> > +        def check_args_section(
> > +                args: Dict[str, QAPIDoc.ArgSection], what: str
> > +        ) -> None:
>
> This is the fourth use of Dict[str, QAPIDoc.ArgSection].  Still fine,
> but if we acquire even more, we should consider giving it a name.
>
>
If they share something in common that makes it name-able, sure. For now:
eh?


> >              bogus = [name for name, section in args.items()
> >                       if not section.member]
> >              if bogus:
>
>

[-- Attachment #2: Type: text/html, Size: 22830 bytes --]

Re: [PATCH v2 3/6] qapi/parser: add type hint annotations (QAPIDoc)

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> On Tue, Sep 7, 2021 at 6:44 AM Markus Armbruster <armbru@redhat.com> wrote:
>
>> John Snow <jsnow@redhat.com> writes:
>>
>> > Annotations do not change runtime behavior.
>> >
>> > This commit adds mostly annotations, but uses a TYPE_CHECKING runtime
>> > check to conditionally import dependencies, which only triggers during
>> > runs of mypy.
>>
>> Please add a reference to
>> https://mypy.readthedocs.io/en/latest/runtime_troubles.html#import-cycles
>>
>>
> OK.
>
>
>> > Signed-off-by: John Snow <jsnow@redhat.com>
>> >
>> > ---
>> >
>> > TopLevelExpr, an idea from previous drafts, makes a return here in order
>> > to give a semantic meaning to check_expr(). The type is intended to be
>> > used more in forthcoming commits (pt5c), but I opted to include it now
>> > instead of creating yet-another Dict[str, object] type hint that I would
>> > forget to change later.
>> >
>> > Signed-off-by: John Snow <jsnow@redhat.com>
>> > ---
>> >  scripts/qapi/parser.py | 77 ++++++++++++++++++++++++++----------------
>> >  1 file changed, 48 insertions(+), 29 deletions(-)
>> >
>> > diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
>> > index 3ddde318376..b1e2fa5c577 100644
>> > --- a/scripts/qapi/parser.py
>> > +++ b/scripts/qapi/parser.py
>> > @@ -18,6 +18,7 @@
>> >  import os
>> >  import re
>> >  from typing import (
>> > +    TYPE_CHECKING,
>> >      Dict,
>> >      List,
>> >      Optional,
>> > @@ -30,6 +31,15 @@
>> >  from .source import QAPISourceInfo
>> >
>> >
>> > +if TYPE_CHECKING:
>> > +    # pylint: disable=cyclic-import
>> > +    # TODO: Remove cycle. [schema -> expr -> parser -> schema]
>>
>
> WRT this todo: you mentioned that you'd prefer having some idea or plan for
> how to eliminate the cycle in order to let this band-aid fly. How about
> adding a 'doc' member to e.g. QAPISchemaFeature and doing the connection
> entirely inside of schema.py, and dropping connect_member() and
> connect_feature()?
>
> Would that be serviceable?

I guess it would.  One way to find out.

>> > +    from .schema import QAPISchemaFeature, QAPISchemaMember
>> > +
>> > +
>> > +#: Represents a single Top Level QAPI schema expression.
>> > +TopLevelExpr = Dict[str, object]
>>
>> Related: _ExprValue below, and _JSONObject in expr.py.  The latter's
>> comment gives the best rationale (except I don't get "the purpose of
>> this module is to interrogate that type").
>>
>>
> in expr.py, the purpose of that module (expr) is explicitly to interrogate
> (check, validate) the shape of arbitrary JSON objects. I am saying that a
> more strict definition specifically here in expr.py is not necessary
> because the entire purpose of expr.py is to, at runtime, verify the shape
> of any such variables that might be annotated that way. I am drawing some
> distinction to introspect.py, where we're generating that data ourselves --
> the stronger types are more viable there, because we know what they are
> already.

Let me try to write a clearer comment:

    # Deserialized JSON objects as returned by the parser.
    # This is a actually Dict[str, _JSONValue], where _JSONValue is
    # Union[bool, str, List[Self], Dict[str, Self]].  Since mypy lacks
    # recursive types, we can't define _JSONValue, and use object
    # instead.  Sad.
    _JSONObject = Dict[str, object]

> (again, sorry about mypy's lack of recursive typing, I hate it too, I
> promise)

We got to play the hand we've been dealt.

>> I think we'd like to have
>>
>> * A recursive type for JSON value (in our bastardized version of JSON)
>>
>>   This is Union of bool, str, List[Self], Dict[str, Self].  It's what
>>   .get_expr() returns.
>>
>>   Since mypy can't do recursive, we approximate with _ExprValue.
>>
>> * A recursive type for JSON object
>>
>>   This is the Dict branch of the above.  It's what check_keys() &
>>   friends take as argument.
>>
>>   We approximate with _JSONObject.
>>
>>   Same for the List branch would make sense if we had a use for the
>>   type.
>>
>> * A recursive type for TOP-LEVEL-EXPR
>>
>>   Actually the same type as the previous one, to be used only for the
>>   schema's top-level expressions.  It's the elements of
>>   QAPISchemaParser.exprs[], and what check_exprs() takes as argument.
>>
>>   We approximate with TopLevelExpr, but so far use it only for
>>   check_exprs().
>>
>>   Doesn't really improve type checking, but may serve as documentation.
>>
>>
> That's the intended effect here -- to help highlight which functions
> operate on those top-level expressions, and which might be invoked in more
> arbitrary cases.
> Consider also a hypothetical TOP-LEVEL-EXPR that is actually a bona-fide
> object with additional metadata, too. In these cases, the type really will
> be different.
>
>
>> Shouldn't these types be all defined in one place, namely right here?
>> Bonus: we need to explain the mypy sadness just once then.
>>
>> Shouldn't their names be more systematic?  _ExprValue, _JSONObject and
>> TopLevelExpr hardly suggest any relation...
>>
>>
> I drop _JSONObject in pt5c, it was a stop-gap. For the rest, I'll see about
> a more rigorous consolidation now that we're this far in.
>
> pt5c was intended as a "cleanup" step that did some of that consolidation
> of nearly-redundant types; but I wanted all of parser.py under the mypy gun
> *first*.
> Will you take a raincheck here and we'll focus on the consolidation in the
> next series? I agree it's worth doing.

Works for me.

> (I can add a 'FIXME' that will 100% need to be fixed before I move
> scripts/qapi under python/qemu/qapi -- the linter config there prohibits
> them, so you can be sure I can't ignore it.)

Makes sense.