[PATCH v4 00/13] qapi: static typing conversion, pt5b

[PATCH v4 00/13] qapi: static typing conversion, pt5b

[John Snow]

Hello darkness my old friend; This is part five (b), and focuses on
QAPIDoc in parser.py.

GitLab: https://gitlab.com/jsnow/qemu/-/commits/python-qapi-cleanup-pt5b
CI: https://gitlab.com/jsnow/qemu/-/pipelines/380464863
    Note: intentional trailing whitespace in a QAPI schema test causes
    a warning on the `check-patch` CI test. Ignore it.

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/`

V4:

005/13:[0006] [FC] 'qapi/parser: remove FIXME comment from _append_body_line'
006/13:[down] 'qapi/parser: clarify _end_section() logic'
007/13:[0004] [FC] 'qapi/parser: Introduce NullSection'
010/13:[down] 'qapi/parser: Add FIXME for consolidating JSON-related types'

- Dropped formerly-patch-05, for now.
- Added a new FIXME comment to keep myself honest for pt5c O:-)
- (05) Adjusted a commend and the parser error message
- (06) Changed commit title for 06 ("simplify" => "clarify")
- (07) Adjust comment.

John Snow (13):
  qapi/pylintrc: ignore 'consider-using-f-string' warning
  qapi/gen: use dict.items() to iterate over _modules
  qapi/parser: fix unused check_args_section arguments
  qapi: Add spaces after symbol declaration for consistency
  qapi/parser: remove FIXME comment from _append_body_line
  qapi/parser: clarify _end_section() logic
  qapi/parser: Introduce NullSection
  qapi/parser: add import cycle workaround
  qapi/parser: add type hint annotations (QAPIDoc)
  qapi/parser: Add FIXME for consolidating JSON-related types
  qapi/parser: enable mypy checks
  qapi/parser: Silence too-few-public-methods warning
  qapi/parser: enable pylint checks

 qapi/block-core.json                   |   1 +
 qga/qapi-schema.json                   |   3 +
 scripts/qapi/gen.py                    |   3 +-
 scripts/qapi/mypy.ini                  |   5 -
 scripts/qapi/parser.py                 | 152 ++++++++++++++++---------
 scripts/qapi/pylintrc                  |   4 +-
 tests/qapi-schema/doc-bad-feature.err  |   2 +-
 tests/qapi-schema/doc-empty-symbol.err |   2 +-
 tests/qapi-schema/doc-good.json        |   8 ++
 9 files changed, 114 insertions(+), 66 deletions(-)

-- 
2.31.1

[PATCH v4 01/13] qapi/pylintrc: ignore 'consider-using-f-string' warning

[John Snow]

Pylint 2.11.x adds this warning. We're not yet ready to pursue that
conversion, so silence it for now.

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

diff --git a/scripts/qapi/pylintrc b/scripts/qapi/pylintrc
index c5275d5f59b..5b7dbc58ad8 100644
--- a/scripts/qapi/pylintrc
+++ b/scripts/qapi/pylintrc
@@ -23,6 +23,7 @@ disable=fixme,
         too-many-branches,
         too-many-statements,
         too-many-instance-attributes,
+        consider-using-f-string,
 
 [REPORTS]
 
-- 
2.31.1

[PATCH v4 02/13] qapi/gen: use dict.items() to iterate over _modules

[John Snow]

New pylint warning. I could silence it, but this is the only occurrence
in the entire tree, including everything in iotests/ and python/. Easier
to just change this one instance.

(The warning is emitted in cases where you are fetching the values
anyway, so you may as well just take advantage of the iterator to avoid
redundant lookups.)

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

diff --git a/scripts/qapi/gen.py b/scripts/qapi/gen.py
index ab26d5c937a..2ec1e7b3b68 100644
--- a/scripts/qapi/gen.py
+++ b/scripts/qapi/gen.py
@@ -296,10 +296,9 @@ def _temp_module(self, name: str) -> Iterator[None]:
         self._current_module = old_module
 
     def write(self, output_dir: str, opt_builtins: bool = False) -> None:
-        for name in self._module:
+        for name, (genc, genh) in self._module.items():
             if QAPISchemaModule.is_builtin_module(name) and not opt_builtins:
                 continue
-            (genc, genh) = self._module[name]
             genc.write(output_dir)
             genh.write(output_dir)
 
-- 
2.31.1

[PATCH v4 03/13] 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 f03ba2cfec8..bfd2dbfd9a2 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.31.1

Re: [PATCH v4 03/13] qapi/parser: fix unused check_args_section arguments

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> 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>

In v3, the blank line was where it belongs:

  Fix test output now that the error message is improved.

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

The change looks accidental.  Can tidy up in my tree.

Re: [PATCH v4 03/13] qapi/parser: fix unused check_args_section arguments

[John Snow]

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

On Fri, Oct 1, 2021 at 10:11 AM Markus Armbruster <armbru@redhat.com> wrote:

> John Snow <jsnow@redhat.com> writes:
>
> > 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>
>
> In v3, the blank line was where it belongs:
>
>   Fix test output now that the error message is improved.
>
>   Fixes: e151941d1b
>   Signed-off-by: John Snow <jsnow@redhat.com>
>
> The change looks accidental.  Can tidy up in my tree.
>

Yes please, thank you :)

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

[PATCH v4 04/13] qapi: Add spaces after symbol declaration for consistency

[John Snow]

Several QGA definitions omit a blank line after the symbol
declaration. This works OK currently, but it's the only place where we
do this. Adjust it for consistency.

Future commits may wind up enforcing this formatting.

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

---

This isn't strictly necessary and I don't actually get around to
enforcing it in this series, but I figured I'd share it with the list
anyway. We can just drop this patch but I wanted to see your thoughts.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 qapi/block-core.json            | 1 +
 qga/qapi-schema.json            | 3 +++
 tests/qapi-schema/doc-good.json | 8 ++++++++
 3 files changed, 12 insertions(+)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index 4114f8b6fc3..52a6dae9522 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -3132,6 +3132,7 @@
 
 ##
 # @BlockdevQcow2EncryptionFormat:
+#
 # @aes: AES-CBC with plain64 initialization vectors
 #
 # Since: 2.10
diff --git a/qga/qapi-schema.json b/qga/qapi-schema.json
index c60f5e669d7..94e4aacdcc6 100644
--- a/qga/qapi-schema.json
+++ b/qga/qapi-schema.json
@@ -1140,6 +1140,7 @@
 
 ##
 # @GuestExec:
+#
 # @pid: pid of child process in guest OS
 #
 # Since: 2.5
@@ -1171,6 +1172,7 @@
 
 ##
 # @GuestHostName:
+#
 # @host-name: Fully qualified domain name of the guest OS
 #
 # Since: 2.10
@@ -1197,6 +1199,7 @@
 
 ##
 # @GuestUser:
+#
 # @user: Username
 # @domain: Logon domain (windows only)
 # @login-time: Time of login of this user on the computer. If multiple
diff --git a/tests/qapi-schema/doc-good.json b/tests/qapi-schema/doc-good.json
index a20acffd8b9..86dc25d2bd8 100644
--- a/tests/qapi-schema/doc-good.json
+++ b/tests/qapi-schema/doc-good.json
@@ -53,6 +53,7 @@
 
 ##
 # @Enum:
+#
 # @one: The _one_ {and only}
 #
 # Features:
@@ -67,6 +68,7 @@
 
 ##
 # @Base:
+#
 # @base1:
 # the first member
 ##
@@ -75,6 +77,7 @@
 
 ##
 # @Variant1:
+#
 # A paragraph
 #
 # Another paragraph (but no @var: line)
@@ -91,11 +94,13 @@
 
 ##
 # @Variant2:
+#
 ##
 { 'struct': 'Variant2', 'data': {} }
 
 ##
 # @Object:
+#
 # Features:
 # @union-feat1: a feature
 ##
@@ -109,6 +114,7 @@
 
 ##
 # @Alternate:
+#
 # @i: an integer
 #     @b is undocumented
 #
@@ -126,6 +132,7 @@
 
 ##
 # @cmd:
+#
 # @arg1: the first argument
 #
 # @arg2: the second
@@ -175,6 +182,7 @@
 
 ##
 # @EVT_BOXED:
+#
 # Features:
 # @feat3: a feature
 ##
-- 
2.31.1

[PATCH v4 05/13] qapi/parser: remove FIXME comment from _append_body_line

[John Snow]

True, we do not check the validity of this symbol -- but we don't check
the validity of definition names during parse, either -- that happens
later, during the expr check. I don't want to introduce a dependency on
expr.py:check_name_str here and introduce a cycle.

Instead, rest assured that a documentation block is required for each
definition. This requirement uses the names of each section to ensure
that we fulfilled this requirement.

e.g., let's say that block-core.json has a comment block for
"Snapshot!Info" by accident. We'll see this error message:

In file included from ../../qapi/block.json:8:
../../qapi/block-core.json: In struct 'SnapshotInfo':
../../qapi/block-core.json:38: documentation comment is for 'Snapshot!Info'

That's a pretty decent error message.

Now, let's say that we actually mangle it twice, identically:

../../qapi/block-core.json: In struct 'Snapshot!Info':
../../qapi/block-core.json:38: struct has an invalid name

That's also pretty decent. If we forget to fix it in both places, we'll
just be back to the first error.

Therefore, let's just drop this FIXME and adjust the error message to
not imply a more thorough check than is actually performed.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 scripts/qapi/parser.py                 | 6 ++++--
 tests/qapi-schema/doc-empty-symbol.err | 2 +-
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
index bfd2dbfd9a2..23898ab1dcd 100644
--- a/scripts/qapi/parser.py
+++ b/scripts/qapi/parser.py
@@ -556,9 +556,11 @@ def _append_body_line(self, line):
             if not line.endswith(':'):
                 raise QAPIParseError(self._parser, "line should end with ':'")
             self.symbol = line[1:-1]
-            # FIXME invalid names other than the empty string aren't flagged
+            # Invalid names are not checked here, but the name provided MUST
+            # match the following definition, which *is* validated in expr.py.
             if not self.symbol:
-                raise QAPIParseError(self._parser, "invalid name")
+                raise QAPIParseError(
+                    self._parser, "name required after '@'")
         elif self.symbol:
             # This is a definition documentation block
             if name.startswith('@') and name.endswith(':'):
diff --git a/tests/qapi-schema/doc-empty-symbol.err b/tests/qapi-schema/doc-empty-symbol.err
index 81b90e882a7..aa51be41b2d 100644
--- a/tests/qapi-schema/doc-empty-symbol.err
+++ b/tests/qapi-schema/doc-empty-symbol.err
@@ -1 +1 @@
-doc-empty-symbol.json:4:1: invalid name
+doc-empty-symbol.json:4:1: name required after '@'
-- 
2.31.1

[PATCH v4 06/13] qapi/parser: clarify _end_section() logic

[John Snow]

The "if self._section" clause in end_section is mysterious: In which
circumstances might we end a section when we don't have one?

QAPIDoc always expects there to be a "current section", only except
after a call to end_comment(). This actually *shouldn't* ever be 'None',
so let's remove that logic so I don't wonder why it's like this again in
three months.

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

diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
index 23898ab1dcd..82f1d952b13 100644
--- a/scripts/qapi/parser.py
+++ b/scripts/qapi/parser.py
@@ -718,13 +718,21 @@ def _start_section(self, name=None, indent=0):
         self.sections.append(self._section)
 
     def _end_section(self):
-        if self._section:
-            text = self._section.text = self._section.text.strip()
-            if self._section.name and (not text or text.isspace()):
-                raise QAPIParseError(
-                    self._parser,
-                    "empty doc section '%s'" % self._section.name)
-            self._section = None
+        assert self._section is not None
+
+        text = self._section.text = self._section.text.strip()
+
+        # Only the 'body' section is allowed to have an empty body.
+        # All other sections, including anonymous ones, must have text.
+        if self._section != self.body and not text:
+            # We do not create anonymous sections unless there is
+            # something to put in them; this is a parser bug.
+            assert self._section.name
+            raise QAPIParseError(
+                self._parser,
+                "empty doc section '%s'" % self._section.name)
+
+        self._section = None
 
     def _append_freeform(self, line):
         match = re.match(r'(@\S+:)', line)
-- 
2.31.1

[PATCH v4 07/13] qapi/parser: Introduce NullSection

[John Snow]

Here's the weird bit. QAPIDoc generally expects -- virtually everywhere
-- that it will always have a current section. The sole exception to
this is in the case that end_comment() is called, which leaves us with
*no* section. However, in this case, we also don't expect to actually
ever mutate the comment contents ever again.

NullSection is just a Null-object that allows us to maintain the
invariant that we *always* have a current section, enforced by static
typing -- allowing us to type that field as QAPIDoc.Section instead of
the more ambiguous Optional[QAPIDoc.Section].

end_section is renamed to switch_section and now accepts as an argument
the new section to activate, clarifying that no callers ever just
unilaterally end a section; they only do so when starting a new section.

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

---

For my money: Optional types can be a nuisance because an unfamiliar
reader may wonder in what circumstances the field may be unset. This
makes the condition quite a bit more explicit and statically provable.

Doing it in this way (and not by creating a mutable dummy section) will also
continue to reject (rather noisily) any erroneous attempts to append
additional lines after end_comment() has been called.

Also, this section isn't indexed into .sections[] and isn't really
visible in any way to external users of the class, so it seems like a
harmless and low-cost way to formalize the "life cycle" of a QAPIDoc
parser.

Clean and clear as I can make it, in as few lines as I could muster.

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

diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
index 82f1d952b13..40c5da4b172 100644
--- a/scripts/qapi/parser.py
+++ b/scripts/qapi/parser.py
@@ -478,6 +478,13 @@ def __init__(self, parser, name, indent=0):
         def connect(self, member):
             self.member = member
 
+    class NullSection(Section):
+        """
+        Immutable dummy section for use at the end of a doc block.
+        """
+        def append(self, line):
+            assert False, "Text appended after end_comment() called."
+
     def __init__(self, parser, info):
         # self._parser is used to report errors with QAPIParseError.  The
         # resulting error position depends on the state of the parser.
@@ -525,7 +532,7 @@ def append(self, line):
         self._append_line(line)
 
     def end_comment(self):
-        self._end_section()
+        self._switch_section(QAPIDoc.NullSection(self._parser))
 
     @staticmethod
     def _is_section_tag(name):
@@ -699,9 +706,9 @@ def _start_symbol_section(self, symbols_dict, name, indent):
             raise QAPIParseError(self._parser,
                                  "'%s' parameter name duplicated" % name)
         assert not self.sections
-        self._end_section()
-        self._section = QAPIDoc.ArgSection(self._parser, name, indent)
-        symbols_dict[name] = self._section
+        new_section = QAPIDoc.ArgSection(self._parser, name, indent)
+        self._switch_section(new_section)
+        symbols_dict[name] = new_section
 
     def _start_args_section(self, name, indent):
         self._start_symbol_section(self.args, name, indent)
@@ -713,13 +720,11 @@ def _start_section(self, name=None, indent=0):
         if name in ('Returns', 'Since') and self.has_section(name):
             raise QAPIParseError(self._parser,
                                  "duplicated '%s' section" % name)
-        self._end_section()
-        self._section = QAPIDoc.Section(self._parser, name, indent)
-        self.sections.append(self._section)
-
-    def _end_section(self):
-        assert self._section is not None
+        new_section = QAPIDoc.Section(self._parser, name, indent)
+        self._switch_section(new_section)
+        self.sections.append(new_section)
 
+    def _switch_section(self, new_section):
         text = self._section.text = self._section.text.strip()
 
         # Only the 'body' section is allowed to have an empty body.
@@ -732,7 +737,7 @@ def _end_section(self):
                 self._parser,
                 "empty doc section '%s'" % self._section.name)
 
-        self._section = None
+        self._section = new_section
 
     def _append_freeform(self, line):
         match = re.match(r'(@\S+:)', line)
-- 
2.31.1

[PATCH v4 08/13] qapi/parser: add import cycle workaround

[John Snow]

Adding static types causes a cycle in the QAPI generator:
[schema -> expr -> parser -> schema]. It exists because the QAPIDoc
class needs the names of types defined by the schema module, but the
schema module needs to import both expr.py/parser.py to do its actual
parsing.

Ultimately, the layering violation is that parser.py should not have any
knowledge of specifics of the Schema. QAPIDoc performs double-duty here
both as a parser *and* as a finalized object that is part of the schema.

In this patch, add the offending type hints alongside the workaround to
avoid the cycle becoming a problem at runtime. See
https://mypy.readthedocs.io/en/latest/runtime_troubles.html#import-cycles
for more information on this workaround technique.


I see three ultimate resolutions here:

(1) Just keep this patch and use the TYPE_CHECKING trick to eliminate
    the cycle which is only present during static analysis.

(2) Don't bother to annotate connect_member() et al, give them 'object'
    or 'Any'. I don't particularly like this, because it diminishes the
    usefulness of type hints for documentation purposes. Still, it's an
    extremely quick fix.

(3) Reimplement doc <--> definition correlation directly in schema.py,
    integrating doc fields directly into QAPISchemaMember and relieving
    the QAPIDoc class of the responsibility. Users of the information
    would instead visit the members first and retrieve their
    documentation instead of the inverse operation -- visiting the
    documentation and retrieving their members.

My preference is (3), but in the short-term (1) is the easiest way to
have my cake (strong type hints) and eat it too (Not have import
cycles). Do (1) for now, but plan for (3).


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

diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
index 40c5da4b172..75582ddb003 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,12 @@
 from .source import QAPISourceInfo
 
 
+if TYPE_CHECKING:
+    # pylint: disable=cyclic-import
+    # TODO: Remove cycle. [schema -> expr -> parser -> schema]
+    from .schema import QAPISchemaFeature, QAPISchemaMember
+
+
 # Return value alias for get_expr().
 _ExprValue = Union[List[object], Dict[str, object], str, bool]
 
@@ -473,9 +480,9 @@ def append(self, line):
     class ArgSection(Section):
         def __init__(self, parser, name, indent=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
 
     class NullSection(Section):
@@ -747,14 +754,14 @@ 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"
-- 
2.31.1

[PATCH v4 09/13] qapi/parser: add type hint annotations (QAPIDoc)

[John Snow]

Annotations do not change runtime behavior.
This commit consists of only annotations.

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

diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
index 75582ddb003..73c1c4ef599 100644
--- a/scripts/qapi/parser.py
+++ b/scripts/qapi/parser.py
@@ -37,6 +37,9 @@
     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]
 
@@ -454,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)
@@ -463,7 +467,7 @@ def __init__(self, parser, name=None, indent=0):
             # the expected indent level of the text of this section
             self._indent = indent
 
-        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:
@@ -478,7 +482,8 @@ 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: str, indent: int = 0):
             super().__init__(parser, name, indent)
             self.member: Optional['QAPISchemaMember'] = None
 
@@ -489,35 +494,34 @@ class NullSection(Section):
         """
         Immutable dummy section for use at the end of a doc block.
         """
-        def append(self, line):
+        def append(self, line: str) -> None:
             assert False, "Text appended after end_comment() called."
 
-    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()
-        # a list of Section
-        self.sections = []
+        # dicts mapping parameter/feature names to their ArgSection
+        self.args: Dict[str, QAPIDoc.ArgSection] = OrderedDict()
+        self.features: Dict[str, QAPIDoc.ArgSection] = OrderedDict()
+        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.
 
@@ -538,18 +542,18 @@ def append(self, line):
         line = line[1:]
         self._append_line(line)
 
-    def end_comment(self):
+    def end_comment(self) -> None:
         self._switch_section(QAPIDoc.NullSection(self._parser))
 
     @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.
 
@@ -591,7 +595,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.
 
@@ -637,7 +641,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(':'):
@@ -669,7 +673,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.
 
@@ -705,7 +709,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")
@@ -717,13 +725,14 @@ def _start_symbol_section(self, symbols_dict, name, indent):
         self._switch_section(new_section)
         symbols_dict[name] = new_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)
@@ -731,7 +740,7 @@ def _start_section(self, name=None, indent=0):
         self._switch_section(new_section)
         self.sections.append(new_section)
 
-    def _switch_section(self, new_section):
+    def _switch_section(self, new_section: 'QAPIDoc.Section') -> None:
         text = self._section.text = self._section.text.strip()
 
         # Only the 'body' section is allowed to have an empty body.
@@ -746,7 +755,7 @@ def _switch_section(self, new_section):
 
         self._section = new_section
 
-    def _append_freeform(self, line):
+    def _append_freeform(self, line: str) -> None:
         match = re.match(r'(@\S+:)', line)
         if match:
             raise QAPIParseError(self._parser,
@@ -768,14 +777,16 @@ def connect_feature(self, feature: 'QAPISchemaFeature') -> None:
                                % 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.31.1

[PATCH v4 10/13] qapi/parser: Add FIXME for consolidating JSON-related types

[John Snow]

The fix for this comment is forthcoming in a future commit, but this
will keep me honest. The linting configuration in ./python/setup.cfg
prohibits 'FIXME' comments. A goal of this long-running series is to
move ./scripts/qapi to ./python/qemu/qapi so that the QAPI generator is
regularly type-checked by GitLab CI.

This comment is a time-bomb to force me to address this issue prior to
that step.

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

diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
index 73c1c4ef599..0265b47b955 100644
--- a/scripts/qapi/parser.py
+++ b/scripts/qapi/parser.py
@@ -43,6 +43,10 @@
 # Return value alias for get_expr().
 _ExprValue = Union[List[object], Dict[str, object], str, bool]
 
+# FIXME: Consolidate and centralize definitions for TopLevelExpr,
+# _ExprValue, _JSONValue, and _JSONObject; currently scattered across
+# several modules.
+
 
 class QAPIParseError(QAPISourceError):
     """Error class for all QAPI schema parsing errors."""
-- 
2.31.1

[PATCH v4 11/13] 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.31.1

[PATCH v4 12/13] qapi/parser: Silence too-few-public-methods warning

[John Snow]

Eh. Not worth the fuss today. There are bigger fish to fry.

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

diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
index 0265b47b955..1b006cdc133 100644
--- a/scripts/qapi/parser.py
+++ b/scripts/qapi/parser.py
@@ -461,8 +461,10 @@ class QAPIDoc:
     """
 
     class Section:
+        # pylint: disable=too-few-public-methods
         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)
@@ -498,6 +500,7 @@ class NullSection(Section):
         """
         Immutable dummy section for use at the end of a doc block.
         """
+        # pylint: disable=too-few-public-methods
         def append(self, line: str) -> None:
             assert False, "Text appended after end_comment() called."
 
-- 
2.31.1

[PATCH v4 13/13] 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 5b7dbc58ad8..b259531a726 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.31.1

Re: [PATCH v4 00/13] qapi: static typing conversion, pt5b

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> Hello darkness my old friend; This is part five (b), and focuses on
> QAPIDoc in parser.py.

Series:
Reviewed-by: Markus Armbruster <armbru@redhat.com>

Re: [PATCH v4 00/13] qapi: static typing conversion, pt5b

[Markus Armbruster]

Markus Armbruster <armbru@redhat.com> writes:

> John Snow <jsnow@redhat.com> writes:
>
>> Hello darkness my old friend; This is part five (b), and focuses on
>> QAPIDoc in parser.py.
>
> Series:
> Reviewed-by: Markus Armbruster <armbru@redhat.com>

... and queued.  Thanks!