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

[PATCH v3 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/379678153
    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/`

V3:

001/13:[down] 'qapi/pylintrc: ignore 'consider-using-f-string' warning'
002/13:[down] 'qapi/gen: use dict.items() to iterate over _modules'
003/13:[----] [--] 'qapi/parser: fix unused check_args_section arguments'
004/13:[down] 'qapi: Add spaces after symbol declaration for consistency'
005/13:[down] 'qapi/parser: improve detection of '@symbol:' preface'
006/13:[down] 'qapi/parser: remove FIXME comment from _append_body_line'
007/13:[down] 'qapi/parser: Simplify _end_section()'
008/13:[down] 'qapi/parser: Introduce NullSection'
009/13:[down] 'qapi/parser: add import cycle workaround'
010/13:[0028] [FC] 'qapi/parser: add type hint annotations (QAPIDoc)'
011/13:[----] [--] 'qapi/parser: enable mypy checks'
012/13:[0003] [FC] 'qapi/parser: Silence too-few-public-methods warning'
013/13:[----] [--] 'qapi/parser: enable pylint checks'

- Rebased.
- Add patches 1 & 2 for newer pylint versions
- Add patches 4-6 because I was in the neighborhood
- Patches 7 & 8 rewritten from the corpse of the prior fix/workaround
- Patch 9 split out separately from what is now patch 10
- Update patch 12 to accommodate new 7/8

Notes:
- Patches 4-6 aren't crucial and can be omitted from staging if it makes
  the difference between a respin or not. I think I will definitely want
  patches 5-6 eventually anyway, though. (Or something to replace 'em.)
- Patch 10 and 11 can be squashed, if desired

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: improve detection of '@symbol:' preface
  qapi/parser: remove FIXME comment from _append_body_line
  qapi/parser: Simplify _end_section()
  qapi/parser: Introduce NullSection
  qapi/parser: add import cycle workaround
  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

 qapi/block-core.json                          |   1 +
 qga/qapi-schema.json                          |   3 +
 scripts/qapi/gen.py                           |   3 +-
 scripts/qapi/mypy.ini                         |   5 -
 scripts/qapi/parser.py                        | 161 +++++++++++-------
 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 +
 .../doc-whitespace-leading-symbol.err         |   1 +
 .../doc-whitespace-leading-symbol.json        |   6 +
 .../doc-whitespace-leading-symbol.out         |   0
 .../doc-whitespace-trailing-symbol.err        |   1 +
 .../doc-whitespace-trailing-symbol.json       |   6 +
 .../doc-whitespace-trailing-symbol.out        |   0
 tests/qapi-schema/meson.build                 |   2 +
 16 files changed, 134 insertions(+), 71 deletions(-)
 create mode 100644 tests/qapi-schema/doc-whitespace-leading-symbol.err
 create mode 100644 tests/qapi-schema/doc-whitespace-leading-symbol.json
 create mode 100644 tests/qapi-schema/doc-whitespace-leading-symbol.out
 create mode 100644 tests/qapi-schema/doc-whitespace-trailing-symbol.err
 create mode 100644 tests/qapi-schema/doc-whitespace-trailing-symbol.json
 create mode 100644 tests/qapi-schema/doc-whitespace-trailing-symbol.out

-- 
2.31.1

[PATCH v3 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 v3 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 v3 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

[PATCH v3 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 v3 05/13] qapi/parser: improve detection of '@symbol:' preface

[John Snow]

Leading and trailing whitespace are now discarded, addressing the FIXME
comment. A new error is raised to detect this accidental case.

Parsing for args sections is left alone here; the 'name' variable is
moved into the only block where it is used.

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

---

Tangentially related to delinting in that removing 'FIXME' comments is a
goal for pylint. My goal is to allow 'TODO' to be checked in, but
'FIXME' should be fixed prior to inclusion.

Arbitrary, but that's life for you.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 scripts/qapi/parser.py                              | 13 ++++++++-----
 tests/qapi-schema/doc-whitespace-leading-symbol.err |  1 +
 .../qapi-schema/doc-whitespace-leading-symbol.json  |  6 ++++++
 tests/qapi-schema/doc-whitespace-leading-symbol.out |  0
 .../qapi-schema/doc-whitespace-trailing-symbol.err  |  1 +
 .../qapi-schema/doc-whitespace-trailing-symbol.json |  6 ++++++
 .../qapi-schema/doc-whitespace-trailing-symbol.out  |  0
 tests/qapi-schema/meson.build                       |  2 ++
 8 files changed, 24 insertions(+), 5 deletions(-)
 create mode 100644 tests/qapi-schema/doc-whitespace-leading-symbol.err
 create mode 100644 tests/qapi-schema/doc-whitespace-leading-symbol.json
 create mode 100644 tests/qapi-schema/doc-whitespace-leading-symbol.out
 create mode 100644 tests/qapi-schema/doc-whitespace-trailing-symbol.err
 create mode 100644 tests/qapi-schema/doc-whitespace-trailing-symbol.json
 create mode 100644 tests/qapi-schema/doc-whitespace-trailing-symbol.out

diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
index bfd2dbfd9a2..2f93a752f66 100644
--- a/scripts/qapi/parser.py
+++ b/scripts/qapi/parser.py
@@ -549,18 +549,21 @@ def _append_body_line(self, line):
 
         Else, append the line to the current section.
         """
-        name = line.split(' ', 1)[0]
-        # FIXME not nice: things like '#  @foo:' and '# @foo: ' aren't
-        # recognized, and get silently treated as ordinary text
-        if not self.symbol and not self.body.text and line.startswith('@'):
-            if not line.endswith(':'):
+        stripped = line.strip()
+
+        if not self.symbol and not self.body.text and stripped.startswith('@'):
+            if not stripped.endswith(':'):
                 raise QAPIParseError(self._parser, "line should end with ':'")
+            if not stripped == line:
+                raise QAPIParseError(
+                    self._parser, "extra whitespace around symbol declaration")
             self.symbol = line[1:-1]
             # FIXME invalid names other than the empty string aren't flagged
             if not self.symbol:
                 raise QAPIParseError(self._parser, "invalid name")
         elif self.symbol:
             # This is a definition documentation block
+            name = line.split(' ', 1)[0]
             if name.startswith('@') and name.endswith(':'):
                 self._append_line = self._append_args_line
                 self._append_args_line(line)
diff --git a/tests/qapi-schema/doc-whitespace-leading-symbol.err b/tests/qapi-schema/doc-whitespace-leading-symbol.err
new file mode 100644
index 00000000000..785468b90e2
--- /dev/null
+++ b/tests/qapi-schema/doc-whitespace-leading-symbol.err
@@ -0,0 +1 @@
+doc-whitespace-leading-symbol.json:4:1: extra whitespace around symbol declaration
diff --git a/tests/qapi-schema/doc-whitespace-leading-symbol.json b/tests/qapi-schema/doc-whitespace-leading-symbol.json
new file mode 100644
index 00000000000..128c781bec9
--- /dev/null
+++ b/tests/qapi-schema/doc-whitespace-leading-symbol.json
@@ -0,0 +1,6 @@
+# Documentation for expression has leading whitespace
+
+##
+#  @leading-whitespace:
+##
+{ 'command': 'leading-whitespace', 'data': {'a': 'int'} }
diff --git a/tests/qapi-schema/doc-whitespace-leading-symbol.out b/tests/qapi-schema/doc-whitespace-leading-symbol.out
new file mode 100644
index 00000000000..e69de29bb2d
diff --git a/tests/qapi-schema/doc-whitespace-trailing-symbol.err b/tests/qapi-schema/doc-whitespace-trailing-symbol.err
new file mode 100644
index 00000000000..fe583b38008
--- /dev/null
+++ b/tests/qapi-schema/doc-whitespace-trailing-symbol.err
@@ -0,0 +1 @@
+doc-whitespace-trailing-symbol.json:4:1: extra whitespace around symbol declaration
diff --git a/tests/qapi-schema/doc-whitespace-trailing-symbol.json b/tests/qapi-schema/doc-whitespace-trailing-symbol.json
new file mode 100644
index 00000000000..da706c3d176
--- /dev/null
+++ b/tests/qapi-schema/doc-whitespace-trailing-symbol.json
@@ -0,0 +1,6 @@
+# Documentation for expression has extra whitespace
+
+##
+# @trailing-whitespace:  
+##
+{ 'command': 'trailing-whitespace', 'data': {'a': 'int'} }
diff --git a/tests/qapi-schema/doc-whitespace-trailing-symbol.out b/tests/qapi-schema/doc-whitespace-trailing-symbol.out
new file mode 100644
index 00000000000..e69de29bb2d
diff --git a/tests/qapi-schema/meson.build b/tests/qapi-schema/meson.build
index 6187efbd58f..64ffbd1b3d4 100644
--- a/tests/qapi-schema/meson.build
+++ b/tests/qapi-schema/meson.build
@@ -82,6 +82,8 @@ schemas = [
   'doc-missing.json',
   'doc-no-symbol.json',
   'doc-undoc-feature.json',
+  'doc-whitespace-leading-symbol.json',
+  'doc-whitespace-trailing-symbol.json',
   'double-type.json',
   'duplicate-key.json',
   'empty.json',
-- 
2.31.1

[PATCH v3 06/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 2f93a752f66..52748e8e462 100644
--- a/scripts/qapi/parser.py
+++ b/scripts/qapi/parser.py
@@ -558,9 +558,11 @@ def _append_body_line(self, line):
                 raise QAPIParseError(
                     self._parser, "extra whitespace around symbol declaration")
             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.
             if not self.symbol:
-                raise QAPIParseError(self._parser, "invalid name")
+                raise QAPIParseError(
+                    self._parser, "doc symbol name cannot be blank")
         elif self.symbol:
             # This is a definition documentation block
             name = line.split(' ', 1)[0]
diff --git a/tests/qapi-schema/doc-empty-symbol.err b/tests/qapi-schema/doc-empty-symbol.err
index 81b90e882a7..a4981ee28ea 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: doc symbol name cannot be blank
-- 
2.31.1

[PATCH v3 07/13] qapi/parser: Simplify _end_section()

[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 52748e8e462..1fdc5bc7056 100644
--- a/scripts/qapi/parser.py
+++ b/scripts/qapi/parser.py
@@ -721,13 +721,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 v3 08/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 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 1fdc5bc7056..123fc2f099c 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):
+        """
+        Empty section that signifies the end of a doc block.
+        """
+        def append(self, line):
+            assert False, "BUG: 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):
@@ -702,9 +709,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)
@@ -716,13 +723,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.
@@ -735,7 +740,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 v3 09/13] qapi/parser: add import cycle workaround

[John Snow]

There is a cycle that exists 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.

I see three paths here:

(1) Just 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.

I prefer (3), but (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). See also:
https://mypy.readthedocs.io/en/latest/runtime_troubles.html#import-cycles

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 123fc2f099c..30b1d98df0b 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):
@@ -750,14 +757,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 v3 10/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 30b1d98df0b..8a846079207 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):
         """
         Empty section that signifies the end of a doc block.
         """
-        def append(self, line):
+        def append(self, line: str) -> None:
             assert False, "BUG: 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.
 
@@ -594,7 +598,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.
 
@@ -640,7 +644,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(':'):
@@ -672,7 +676,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.
 
@@ -708,7 +712,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")
@@ -720,13 +728,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)
@@ -734,7 +743,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.
@@ -749,7 +758,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,
@@ -771,14 +780,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 v3 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 v3 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 8a846079207..7511eedaa35 100644
--- a/scripts/qapi/parser.py
+++ b/scripts/qapi/parser.py
@@ -457,8 +457,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)
@@ -494,6 +496,7 @@ class NullSection(Section):
         """
         Empty section that signifies the end of a doc block.
         """
+        # pylint: disable=too-few-public-methods
         def append(self, line: str) -> None:
             assert False, "BUG: Text appended after end_comment() called."
 
-- 
2.31.1

[PATCH v3 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 v3 05/13] qapi/parser: improve detection of '@symbol:' preface

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> Leading and trailing whitespace are now discarded, addressing the FIXME
> comment. A new error is raised to detect this accidental case.
>
> Parsing for args sections is left alone here; the 'name' variable is
> moved into the only block where it is used.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
>
> ---
>
> Tangentially related to delinting in that removing 'FIXME' comments is a
> goal for pylint. My goal is to allow 'TODO' to be checked in, but
> 'FIXME' should be fixed prior to inclusion.
>
> Arbitrary, but that's life for you.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  scripts/qapi/parser.py                              | 13 ++++++++-----
>  tests/qapi-schema/doc-whitespace-leading-symbol.err |  1 +
>  .../qapi-schema/doc-whitespace-leading-symbol.json  |  6 ++++++
>  tests/qapi-schema/doc-whitespace-leading-symbol.out |  0
>  .../qapi-schema/doc-whitespace-trailing-symbol.err  |  1 +
>  .../qapi-schema/doc-whitespace-trailing-symbol.json |  6 ++++++
>  .../qapi-schema/doc-whitespace-trailing-symbol.out  |  0
>  tests/qapi-schema/meson.build                       |  2 ++
>  8 files changed, 24 insertions(+), 5 deletions(-)
>  create mode 100644 tests/qapi-schema/doc-whitespace-leading-symbol.err
>  create mode 100644 tests/qapi-schema/doc-whitespace-leading-symbol.json
>  create mode 100644 tests/qapi-schema/doc-whitespace-leading-symbol.out
>  create mode 100644 tests/qapi-schema/doc-whitespace-trailing-symbol.err
>  create mode 100644 tests/qapi-schema/doc-whitespace-trailing-symbol.json
>  create mode 100644 tests/qapi-schema/doc-whitespace-trailing-symbol.out
>
> diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
> index bfd2dbfd9a2..2f93a752f66 100644
> --- a/scripts/qapi/parser.py
> +++ b/scripts/qapi/parser.py
> @@ -549,18 +549,21 @@ def _append_body_line(self, line):
>  
>          Else, append the line to the current section.
>          """
> -        name = line.split(' ', 1)[0]
> -        # FIXME not nice: things like '#  @foo:' and '# @foo: ' aren't
> -        # recognized, and get silently treated as ordinary text
> -        if not self.symbol and not self.body.text and line.startswith('@'):
> -            if not line.endswith(':'):
> +        stripped = line.strip()
> +
> +        if not self.symbol and not self.body.text and stripped.startswith('@'):
> +            if not stripped.endswith(':'):
>                  raise QAPIParseError(self._parser, "line should end with ':'")
> +            if not stripped == line:
> +                raise QAPIParseError(
> +                    self._parser, "extra whitespace around symbol declaration")

This rejects both leading and trailing whitespace.  Rejecting leading
whitespace is good.  Rejecting trailing whitespace feels a bit pedantic,
and it might not extend to the related case I'll point out below.

Have you considered a regexp instead?  Say

           match = re.match(r'(\s*)@([^:]*)(:?)(\s*)(.*)$', line)

Then match.group(n) is

     n=1  leading whitespace, if any
     n=2  symbol
     n=3  trailing colon, if any
     n=4  trailing whitespace, if any
     n=5  trailing text, if any

Omit the subgroups you don't need.

>              self.symbol = line[1:-1]
>              # FIXME invalid names other than the empty string aren't flagged
>              if not self.symbol:
>                  raise QAPIParseError(self._parser, "invalid name")
>          elif self.symbol:
>              # This is a definition documentation block
> +            name = line.split(' ', 1)[0]
>              if name.startswith('@') and name.endswith(':'):
>                  self._append_line = self._append_args_line
>                  self._append_args_line(line)

Same issue here, and in _append_args_line().  To reproduce, I hacked up
doc-good.json like so

    diff --git a/tests/qapi-schema/doc-good.json b/tests/qapi-schema/doc-good.json
    index 86dc25d2bd..977fcbad48 100644
    --- a/tests/qapi-schema/doc-good.json
    +++ b/tests/qapi-schema/doc-good.json
    @@ -133,7 +133,7 @@
     ##
     # @cmd:
     #
    -# @arg1: the first argument
    +#  @arg1: the first argument
     #
     # @arg2: the second
     #        argument

and got

    $ PYTHONPATH=/work/armbru/qemu/scripts python3 /work/armbru/qemu/tests/qapi-schema/test-qapi.py -d tests/qapi-schema doc-good.json
    doc-good FAIL
    --- tests/qapi-schema/doc-good.out
    +++ 
    @@ -149,12 +149,12 @@
     == Another subsection
     doc symbol=cmd
         body=
    -
    -    arg=arg1
    -the first argument
    +@arg1: the first argument
         arg=arg2
     the second
     argument
    +    arg=arg1
    +
         arg=arg3

         feature=cmd-feat1

[...]

Re: [PATCH v3 06/13] qapi/parser: remove FIXME comment from _append_body_line

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> 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 2f93a752f66..52748e8e462 100644
> --- a/scripts/qapi/parser.py
> +++ b/scripts/qapi/parser.py
> @@ -558,9 +558,11 @@ def _append_body_line(self, line):
>                  raise QAPIParseError(
>                      self._parser, "extra whitespace around symbol declaration")
>              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.
>              if not self.symbol:
> -                raise QAPIParseError(self._parser, "invalid name")
> +                raise QAPIParseError(
> +                    self._parser, "doc symbol name cannot be blank")

"blank" feels like "empty or just whitespace" to me.  We actually mean
"empty".

What about "name required after @"?

>          elif self.symbol:
>              # This is a definition documentation block
>              name = line.split(' ', 1)[0]
> diff --git a/tests/qapi-schema/doc-empty-symbol.err b/tests/qapi-schema/doc-empty-symbol.err
> index 81b90e882a7..a4981ee28ea 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: doc symbol name cannot be blank

Re: [PATCH v3 08/13] qapi/parser: Introduce NullSection

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> 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 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 1fdc5bc7056..123fc2f099c 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):
> +        """
> +        Empty section that signifies the end of a doc block.

What about "Dummy section for use at the end of a doc block"?

> +        """
> +        def append(self, line):
> +            assert False, "BUG: Text appended after end_comment() called."

How can a failing assertion *not* be a bug?

> +
>      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):
> @@ -702,9 +709,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)
> @@ -716,13 +723,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.
> @@ -735,7 +740,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)

Feels clearer, thanks!

Re: [PATCH v3 09/13] qapi/parser: add import cycle workaround

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> There is a cycle that exists in the QAPI generator: [schema -> expr ->

"There is" or "there will be once we add strong type hints"?

> 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.
>
> I see three paths here:
>
> (1) Just 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.
>
> I prefer (3), but (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). See also:
> https://mypy.readthedocs.io/en/latest/runtime_troubles.html#import-cycles
>
> 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 123fc2f099c..30b1d98df0b 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):
> @@ -750,14 +757,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"

This adds just the type hints that cause the cycle.  I like that,
because it illustrates the cycle.  Would be nice if the commit message
mentioned this, perhaps

  I prefer (3), but (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). Also add the type hints that cause the cycle right away to
  illustrate. See also:
  https://mypy.readthedocs.io/en/latest/runtime_troubles.html#import-cycles

Slightly nicer, I think, would be swapping this and the next patch.
Then that one's commit message needs to say something like "except for a
few problematic ones, which the next commit will add".  Worthwhile?  Up
to you.

Re: [PATCH v3 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.

Looks basically ready except for PATCH 5, which we can chop off (I
think), so you don't have to respin the whole series.

"Basically" means we may want to tweak a few commit messages /
comments.  Happy to do that myself.

Re: [PATCH v3 08/13] qapi/parser: Introduce NullSection

[John Snow]

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

On Thu, Sep 30, 2021 at 5:35 AM Markus Armbruster <armbru@redhat.com> wrote:

> John Snow <jsnow@redhat.com> writes:
>
> > 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 dummy section) will also
>

("And not by creating a [mutable] dummy section" ... but this wasn't
destined for the git log anyway.)


> > 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 1fdc5bc7056..123fc2f099c 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):
> > +        """
> > +        Empty section that signifies the end of a doc block.
>
> What about "Dummy section for use at the end of a doc block"?
>
>
Sure. "Immutable dummy section for use at the end of a doc block."


> > +        """
> > +        def append(self, line):
> > +            assert False, "BUG: Text appended after end_comment()
> called."
>
> How can a failing assertion *not* be a bug?
>
>
Haha. I'll drop the prefix. (I'll update my branch with these tiny edits
and you can decide what you'd like to do with that.)


> > +
> >      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):
> > @@ -702,9 +709,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)
> > @@ -716,13 +723,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.
> > @@ -735,7 +740,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)
>
> Feels clearer, thanks!
>
>
Relieved you think so O:-)

--js

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

Re: [PATCH v3 09/13] qapi/parser: add import cycle workaround

[John Snow]

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

On Thu, Sep 30, 2021 at 5:45 AM Markus Armbruster <armbru@redhat.com> wrote:

> John Snow <jsnow@redhat.com> writes:
>
> > There is a cycle that exists in the QAPI generator: [schema -> expr ->
>
> "There is" or "there will be once we add strong type hints"?
>
>
"There exists in my mind-palace a cycle where, ..."

(Will adjust the commit message.)


> > 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.
> >
> > I see three paths here:
> >
> > (1) Just 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.
> >
> > I prefer (3), but (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). See also:
> >
> https://mypy.readthedocs.io/en/latest/runtime_troubles.html#import-cycles
> >
> > 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 123fc2f099c..30b1d98df0b 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):
> > @@ -750,14 +757,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"
>
> This adds just the type hints that cause the cycle.  I like that,
> because it illustrates the cycle.  Would be nice if the commit message
> mentioned this, perhaps
>
>   I prefer (3), but (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). Also add the type hints that cause the cycle right away to
>   illustrate. See also:
>
> https://mypy.readthedocs.io/en/latest/runtime_troubles.html#import-cycles
>
> Slightly nicer, I think, would be swapping this and the next patch.
> Then that one's commit message needs to say something like "except for a
> few problematic ones, which the next commit will add".  Worthwhile?  Up
> to you.
>
>
Doing it the other way around means you can't squash the mypy patch into
the bulk-type-hints patch, but I think the git log usefulness is not better
or worse either way around. (Reviewer usefulness is maybe a ship that has
sailed, by now?)

--js

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

Re: [PATCH v3 06/13] qapi/parser: remove FIXME comment from _append_body_line

[John Snow]

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

On Thu, Sep 30, 2021 at 4:47 AM Markus Armbruster <armbru@redhat.com> wrote:

> John Snow <jsnow@redhat.com> writes:
>
> > 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 2f93a752f66..52748e8e462 100644
> > --- a/scripts/qapi/parser.py
> > +++ b/scripts/qapi/parser.py
> > @@ -558,9 +558,11 @@ def _append_body_line(self, line):
> >                  raise QAPIParseError(
> >                      self._parser, "extra whitespace around symbol
> declaration")
> >              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.
> >              if not self.symbol:
> > -                raise QAPIParseError(self._parser, "invalid name")
> > +                raise QAPIParseError(
> > +                    self._parser, "doc symbol name cannot be blank")
>
> "blank" feels like "empty or just whitespace" to me.  We actually mean
> "empty".
>
> What about "name required after @"?
>
>
Sure, yeah. Updated.


> >          elif self.symbol:
> >              # This is a definition documentation block
> >              name = line.split(' ', 1)[0]
> > diff --git a/tests/qapi-schema/doc-empty-symbol.err
> b/tests/qapi-schema/doc-empty-symbol.err
> > index 81b90e882a7..a4981ee28ea 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: doc symbol name cannot be blank
>
>

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

Re: [PATCH v3 05/13] qapi/parser: improve detection of '@symbol:' preface

[John Snow]

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

On Thu, Sep 30, 2021 at 4:42 AM Markus Armbruster <armbru@redhat.com> wrote:

> John Snow <jsnow@redhat.com> writes:
>
> > Leading and trailing whitespace are now discarded, addressing the FIXME
> > comment. A new error is raised to detect this accidental case.
> >
> > Parsing for args sections is left alone here; the 'name' variable is
> > moved into the only block where it is used.
> >
> > Signed-off-by: John Snow <jsnow@redhat.com>
> >
> > ---
> >
> > Tangentially related to delinting in that removing 'FIXME' comments is a
> > goal for pylint. My goal is to allow 'TODO' to be checked in, but
> > 'FIXME' should be fixed prior to inclusion.
> >
> > Arbitrary, but that's life for you.
> >
> > Signed-off-by: John Snow <jsnow@redhat.com>
> > ---
> >  scripts/qapi/parser.py                              | 13 ++++++++-----
> >  tests/qapi-schema/doc-whitespace-leading-symbol.err |  1 +
> >  .../qapi-schema/doc-whitespace-leading-symbol.json  |  6 ++++++
> >  tests/qapi-schema/doc-whitespace-leading-symbol.out |  0
> >  .../qapi-schema/doc-whitespace-trailing-symbol.err  |  1 +
> >  .../qapi-schema/doc-whitespace-trailing-symbol.json |  6 ++++++
> >  .../qapi-schema/doc-whitespace-trailing-symbol.out  |  0
> >  tests/qapi-schema/meson.build                       |  2 ++
> >  8 files changed, 24 insertions(+), 5 deletions(-)
> >  create mode 100644 tests/qapi-schema/doc-whitespace-leading-symbol.err
> >  create mode 100644 tests/qapi-schema/doc-whitespace-leading-symbol.json
> >  create mode 100644 tests/qapi-schema/doc-whitespace-leading-symbol.out
> >  create mode 100644 tests/qapi-schema/doc-whitespace-trailing-symbol.err
> >  create mode 100644 tests/qapi-schema/doc-whitespace-trailing-symbol.json
> >  create mode 100644 tests/qapi-schema/doc-whitespace-trailing-symbol.out
> >
> > diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
> > index bfd2dbfd9a2..2f93a752f66 100644
> > --- a/scripts/qapi/parser.py
> > +++ b/scripts/qapi/parser.py
> > @@ -549,18 +549,21 @@ def _append_body_line(self, line):
> >
> >          Else, append the line to the current section.
> >          """
> > -        name = line.split(' ', 1)[0]
> > -        # FIXME not nice: things like '#  @foo:' and '# @foo: ' aren't
> > -        # recognized, and get silently treated as ordinary text
> > -        if not self.symbol and not self.body.text and
> line.startswith('@'):
> > -            if not line.endswith(':'):
> > +        stripped = line.strip()
> > +
> > +        if not self.symbol and not self.body.text and
> stripped.startswith('@'):
> > +            if not stripped.endswith(':'):
> >                  raise QAPIParseError(self._parser, "line should end
> with ':'")
> > +            if not stripped == line:
> > +                raise QAPIParseError(
> > +                    self._parser, "extra whitespace around symbol
> declaration")
>
> This rejects both leading and trailing whitespace.  Rejecting leading
> whitespace is good.  Rejecting trailing whitespace feels a bit pedantic,
> and it might not extend to the related case I'll point out below.
>
>
err'd on the conservative side. Wasn't sure how permissive we really wanted
to be.


> Have you considered a regexp instead?  Say
>
>            match = re.match(r'(\s*)@([^:]*)(:?)(\s*)(.*)$', line)
>
> Then match.group(n) is
>
>      n=1  leading whitespace, if any
>      n=2  symbol
>      n=3  trailing colon, if any
>      n=4  trailing whitespace, if any
>      n=5  trailing text, if any
>
> Omit the subgroups you don't need.
>
>
Sensible, for a more comprehensive refactoring.


> >              self.symbol = line[1:-1]
> >              # FIXME invalid names other than the empty string aren't
> flagged
> >              if not self.symbol:
> >                  raise QAPIParseError(self._parser, "invalid name")
> >          elif self.symbol:
> >              # This is a definition documentation block
> > +            name = line.split(' ', 1)[0]
> >              if name.startswith('@') and name.endswith(':'):
> >                  self._append_line = self._append_args_line
> >                  self._append_args_line(line)
>
> Same issue here, and in _append_args_line().  To reproduce, I hacked up
> doc-good.json like so
>
>     diff --git a/tests/qapi-schema/doc-good.json
> b/tests/qapi-schema/doc-good.json
>     index 86dc25d2bd..977fcbad48 100644
>     --- a/tests/qapi-schema/doc-good.json
>     +++ b/tests/qapi-schema/doc-good.json
>     @@ -133,7 +133,7 @@
>      ##
>      # @cmd:
>      #
>     -# @arg1: the first argument
>     +#  @arg1: the first argument
>      #
>      # @arg2: the second
>      #        argument
>
> and got
>
>     $ PYTHONPATH=/work/armbru/qemu/scripts python3
> /work/armbru/qemu/tests/qapi-schema/test-qapi.py -d tests/qapi-schema
> doc-good.json
>     doc-good FAIL
>     --- tests/qapi-schema/doc-good.out
>     +++
>     @@ -149,12 +149,12 @@
>      == Another subsection
>      doc symbol=cmd
>          body=
>     -
>     -    arg=arg1
>     -the first argument
>     +@arg1: the first argument
>          arg=arg2
>      the second
>      argument
>     +    arg=arg1
>     +
>          arg=arg3
>
>          feature=cmd-feat1
>
> [...]
>
>
OK, more time in the oven with this one, and I will tackle it separately
and later. Possibly as part of my sphinx-docs work I want to get to soon.
We may drop it from this series to avoid holding it up.

(The FIXME again keeps me honest here ... !)

Thanks for the reviews!
--js

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