[PATCH v3 00/15] qapi: static typing conversion, pt5a

[PATCH v3 00/15] qapi: static typing conversion, pt5a

[John Snow]

This is part five (a), and focuses on QAPISchemaParser in parser.py.
It does not touch QAPIDoc yet, which will be covered next.

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

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: Commit message changed
004: Commit message changed
005/15:[0002] [FC] 'qapi/parser: Assert lexer value is a string'
        - Remove comment
015/15:[0019] [FC] 'qapi/parser: add docstrings'
        - Futz with docstrings based on review from armbru

V2:

001/21:[0024] [FC] 'qapi/parser: Don't try to handle file errors'
002/21:[down] 'qapi: Add test for nonexistent schema file'
003/21:[0008] [FC] 'qapi/source: Remove line number from QAPISourceInfo initializer'
004/21:[0003] [FC] 'qapi/parser: factor parsing routine into method'
005/21:[0002] [FC] 'qapi/parser: Assert lexer value is a string'
006/21:[down] 'qapi/parser: enforce all top-level expressions must be dict in _parse()'
007/21:[----] [--] 'qapi/parser: assert object keys are strings'
008/21:[----] [--] 'qapi/parser: Use @staticmethod where appropriate'
009/21:[down] 'qapi: add must_match helper'
010/21:[down] 'qapi/parser: Fix token membership tests when token can be None'
011/21:[0012] [FC] 'qapi/parser: Rework _check_pragma_list_of_str as a TypeGuard'
012/21:[0019] [FC] 'qapi/parser: add type hint annotations'
013/21:[down] 'qapi/parser: Remove superfluous list comprehension'
014/21:[----] [--] 'qapi/parser: allow 'ch' variable name'
015/21:[0080] [FC] 'qapi/parser: add docstrings'

01:
  - Futzed with the commit message a lot.
  - Added new try/except to QAPISchema() instead of main().
  - Adjusted "let caller handle this error" comment
  - Adjusted test-qapi not to crash.
02:
  - New, add test for nonexistant root schema file.
03:
  - Commit message changes.
  - Rebase changes, removed _column RFC patch that preceded it.
04:
  - Commit message changes.
  - Minor rebase changes (from changed comment in 01)
05:
  - Remove assert message, replace w/ comment
06:
  - Replaces 'qapi/parser: assert get_expr returns object in outer loop'
09:
  - Renamed match_nofail() to must_match()
10:
  - Use tuple() for token membership tests
  - Add test cases to prevent regressions
11:
  - _check => _check_list_str
  - info.pragma => pragma
12:
  - Remove 'Expression' type entirely for now
  - Highlight self.tok as actively taking None type with Union[str, None]
  - Minor rebase confetti.
13:
  - Renamed commit message.
15:
  - Reworked.
  - Note that 'pos' is indeed interface as it is used by the error handlers.

John Snow (15):
  qapi/parser: Don't try to handle file errors
  qapi: Add test for nonexistent schema file
  qapi/source: Remove line number from QAPISourceInfo initializer
  qapi/parser: factor parsing routine into method
  qapi/parser: Assert lexer value is a string
  qapi/parser: enforce all top-level expressions must be dict in
    _parse()
  qapi/parser: assert object keys are strings
  qapi/parser: Use @staticmethod where appropriate
  qapi: add must_match helper
  qapi/parser: Fix token membership tests when token can be None
  qapi/parser: Rework _check_pragma_list_of_str as a TypeGuard
  qapi/parser: add type hint annotations
  qapi/parser: Remove superfluous list comprehension
  qapi/parser: allow 'ch' variable name
  qapi/parser: add docstrings

 scripts/qapi/common.py                        |   8 +-
 scripts/qapi/main.py                          |   6 +-
 scripts/qapi/parser.py                        | 230 +++++++++++++-----
 scripts/qapi/pylintrc                         |   1 +
 scripts/qapi/schema.py                        |  11 +-
 scripts/qapi/source.py                        |  13 +-
 tests/qapi-schema/meson.build                 |   9 +-
 tests/qapi-schema/missing-array-rsqb.err      |   1 +
 tests/qapi-schema/missing-array-rsqb.json     |   1 +
 tests/qapi-schema/missing-array-rsqb.out      |   0
 .../missing-object-member-element.err         |   1 +
 .../missing-object-member-element.json        |   1 +
 .../missing-object-member-element.out         |   0
 tests/qapi-schema/missing-schema.err          |   1 +
 tests/qapi-schema/missing-schema.out          |   0
 tests/qapi-schema/non-objects.err             |   2 +-
 tests/qapi-schema/quoted-structural-chars.err |   2 +-
 tests/qapi-schema/test-qapi.py                |   3 -
 18 files changed, 209 insertions(+), 81 deletions(-)
 create mode 100644 tests/qapi-schema/missing-array-rsqb.err
 create mode 100644 tests/qapi-schema/missing-array-rsqb.json
 create mode 100644 tests/qapi-schema/missing-array-rsqb.out
 create mode 100644 tests/qapi-schema/missing-object-member-element.err
 create mode 100644 tests/qapi-schema/missing-object-member-element.json
 create mode 100644 tests/qapi-schema/missing-object-member-element.out
 create mode 100644 tests/qapi-schema/missing-schema.err
 create mode 100644 tests/qapi-schema/missing-schema.out

-- 
2.30.2

[PATCH v3 01/15] qapi/parser: Don't try to handle file errors

[John Snow]

Fixes: f5d4361cda
Fixes: 52a474180a
Fixes: 46f49468c6

Remove the try/except block that handles file-opening errors in
QAPISchemaParser.__init__() and add one each to
QAPISchemaParser._include() and QAPISchema.__init__() respectively.

This simultaneously fixes the typing of info.fname (f5d4361cda), A
static typing violation in test-qapi (46f49468c6), and a regression of
an error message (52a474180a).


The short-ish version of what motivates this patch is:

- It's hard to write a good error message in the init method,
  because we need to determine the context of our caller to do so.
  It's easier to just let the caller write the message.
- We don't want to allow QAPISourceInfo(None, None, None) to exist. The
  typing introduced by commit f5d4361cda types the 'fname' field as
  (non-optional) str, which was premature until the removal of this
  construct.
- Errors made using such an object are currently incorrect (since
  52a474180a)
- It's not technically a semantic error if we cannot open the schema.
- There are various typing constraints that make mixing these two cases
  undesirable for a single special case.
- test-qapi's code handling an fname of 'None' is now dead, drop it.
  Additionally, Not all QAPIError objects have an 'info' field (since
  46f49468), so deleting this stanza corrects a typing oversight in
  test-qapi introduced by that commit.


Other considerations:

- open() is moved to a 'with' block to ensure file pointers are
  cleaned up deterministically.
- Python 3.3 deprecated IOError and made it a synonym for OSError.
  Avoid the misleading perception these exception handlers are
  narrower than they really are.


The long version:

The error message here is incorrect (since commit 52a474180a):

> python3 qapi-gen.py 'fake.json'
qapi-gen.py: qapi-gen.py: can't read schema file 'fake.json': No such file or directory

In pursuing it, we find that QAPISourceInfo has a special accommodation
for when there's no filename. Meanwhile, the intent when QAPISourceInfo
was typed (f5d4361cda) was non-optional 'str'. This usage was
overlooked.

To remove this, I'd want to avoid having a "fake" QAPISourceInfo
object. I also don't want to explicitly begin accommodating
QAPISourceInfo itself being None, because we actually want to eventually
prove that this can never happen -- We don't want to confuse "The file
isn't open yet" with "This error stems from a definition that wasn't
defined in any file".

(An earlier series tried to create a dummy info object, but it was tough
to prove in review that it worked correctly without creating new
regressions. This patch avoids that distraction. We would like to first
prove that we never raise QAPISemError for any built-in object before we
add "special" info objects. We aren't ready to do that yet.)

So, which way out of the labyrinth?

Here's one way: Don't try to handle errors at a level with "mixed"
semantic contexts; i.e. don't mix inclusion errors (should report a
source line where the include was triggered) and command line errors
(where we specified a file we couldn't read).

Remove the error handling from the initializer of the parser. Pythonic!
Now it's the caller's job to figure out what to do about it. Handle the
error in QAPISchemaParser._include() instead, where we can write a
targeted error message where we are guaranteed to have an 'info' context
to report with.

The root level error can similarly move to QAPISchema.__init__(), where
we know we'll never have an info context to report with, so we use a
more abstract error type.

Now the error looks sensible again:

> python3 qapi-gen.py 'fake.json'
qapi-gen.py: can't read schema file 'fake.json': No such file or directory

With these error cases separated, QAPISourceInfo can be solidified as
never having placeholder arguments that violate our desired types. Clean
up test-qapi along similar lines.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 scripts/qapi/parser.py         | 18 +++++++++---------
 scripts/qapi/schema.py         | 11 +++++++++--
 scripts/qapi/source.py         |  3 ---
 tests/qapi-schema/test-qapi.py |  3 ---
 4 files changed, 18 insertions(+), 17 deletions(-)

diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
index ca5e8e18e00..a53b735e7de 100644
--- a/scripts/qapi/parser.py
+++ b/scripts/qapi/parser.py
@@ -40,15 +40,9 @@ def __init__(self, fname, previously_included=None, incl_info=None):
         previously_included = previously_included or set()
         previously_included.add(os.path.abspath(fname))
 
-        try:
-            fp = open(fname, 'r', encoding='utf-8')
+        # May raise OSError; allow the caller to handle it.
+        with open(fname, 'r', encoding='utf-8') as fp:
             self.src = fp.read()
-        except IOError as e:
-            raise QAPISemError(incl_info or QAPISourceInfo(None, None, None),
-                               "can't read %s file '%s': %s"
-                               % ("include" if incl_info else "schema",
-                                  fname,
-                                  e.strerror))
 
         if self.src == '' or self.src[-1] != '\n':
             self.src += '\n'
@@ -129,7 +123,13 @@ def _include(self, include, info, incl_fname, previously_included):
         if incl_abs_fname in previously_included:
             return None
 
-        return QAPISchemaParser(incl_fname, previously_included, info)
+        try:
+            return QAPISchemaParser(incl_fname, previously_included, info)
+        except OSError as err:
+            raise QAPISemError(
+                info,
+                f"can't read include file '{incl_fname}': {err.strerror}"
+            ) from err
 
     def _check_pragma_list_of_str(self, name, value, info):
         if (not isinstance(value, list)
diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
index 3a4172fb749..d1d27ff7ee8 100644
--- a/scripts/qapi/schema.py
+++ b/scripts/qapi/schema.py
@@ -20,7 +20,7 @@
 from typing import Optional
 
 from .common import POINTER_SUFFIX, c_name
-from .error import QAPISemError, QAPISourceError
+from .error import QAPIError, QAPISemError, QAPISourceError
 from .expr import check_exprs
 from .parser import QAPISchemaParser
 
@@ -849,7 +849,14 @@ def visit(self, visitor):
 class QAPISchema:
     def __init__(self, fname):
         self.fname = fname
-        parser = QAPISchemaParser(fname)
+
+        try:
+            parser = QAPISchemaParser(fname)
+        except OSError as err:
+            raise QAPIError(
+                f"can't read schema file '{fname}': {err.strerror}"
+            ) from err
+
         exprs = check_exprs(parser.exprs)
         self.docs = parser.docs
         self._entity_list = []
diff --git a/scripts/qapi/source.py b/scripts/qapi/source.py
index 03b6ede0828..1ade864d7b9 100644
--- a/scripts/qapi/source.py
+++ b/scripts/qapi/source.py
@@ -10,7 +10,6 @@
 # See the COPYING file in the top-level directory.
 
 import copy
-import sys
 from typing import List, Optional, TypeVar
 
 
@@ -53,8 +52,6 @@ def next_line(self: T) -> T:
         return info
 
     def loc(self) -> str:
-        if self.fname is None:
-            return sys.argv[0]
         ret = self.fname
         if self.line is not None:
             ret += ':%d' % self.line
diff --git a/tests/qapi-schema/test-qapi.py b/tests/qapi-schema/test-qapi.py
index e8db9d09d91..f1c4deb9a51 100755
--- a/tests/qapi-schema/test-qapi.py
+++ b/tests/qapi-schema/test-qapi.py
@@ -128,9 +128,6 @@ def test_and_diff(test_name, dir_name, update):
     try:
         test_frontend(os.path.join(dir_name, test_name + '.json'))
     except QAPIError as err:
-        if err.info.fname is None:
-            print("%s" % err, file=sys.stderr)
-            return 2
         errstr = str(err) + '\n'
         if dir_name:
             errstr = errstr.replace(dir_name + '/', '')
-- 
2.30.2

[PATCH v3 02/15] qapi: Add test for nonexistent schema file

[John Snow]

This tests the error-return pathway introduced in the previous commit.
(Thanks to Paolo for the help with the Meson magic.)

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

---

This went after the previous patch instead of before because prior to
removing the sys.argv[0] bit from QAPISourceInfo, I can't filter the
test to pass the diff. Instead of writing something new to get a better
patch ordering, just add the test after.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 tests/qapi-schema/meson.build        | 7 ++++++-
 tests/qapi-schema/missing-schema.err | 1 +
 tests/qapi-schema/missing-schema.out | 0
 3 files changed, 7 insertions(+), 1 deletion(-)
 create mode 100644 tests/qapi-schema/missing-schema.err
 create mode 100644 tests/qapi-schema/missing-schema.out

diff --git a/tests/qapi-schema/meson.build b/tests/qapi-schema/meson.build
index d7163e6601c..dc448e8f74d 100644
--- a/tests/qapi-schema/meson.build
+++ b/tests/qapi-schema/meson.build
@@ -199,11 +199,16 @@ schemas = [
   'unknown-escape.json',
   'unknown-expr-key.json',
 ]
+schemas = files(schemas)
+
+# Intentionally missing schema file test -- not passed through files():
+schemas += [meson.current_source_dir() / 'missing-schema.json']
 
 # Because people may want to use test-qapi.py from the command line, we
 # are not using the "#! /usr/bin/env python3" trick here.  See
 # docs/devel/build-system.txt
-test('QAPI schema regression tests', python, args: files('test-qapi.py', schemas),
+test('QAPI schema regression tests', python,
+     args: files('test-qapi.py') + schemas,
      env: test_env, suite: ['qapi-schema', 'qapi-frontend'])
 
 diff = find_program('diff')
diff --git a/tests/qapi-schema/missing-schema.err b/tests/qapi-schema/missing-schema.err
new file mode 100644
index 00000000000..b4d9ff1fb2b
--- /dev/null
+++ b/tests/qapi-schema/missing-schema.err
@@ -0,0 +1 @@
+can't read schema file 'missing-schema.json': No such file or directory
diff --git a/tests/qapi-schema/missing-schema.out b/tests/qapi-schema/missing-schema.out
new file mode 100644
index 00000000000..e69de29bb2d
-- 
2.30.2

[PATCH v3 03/15] qapi/source: Remove line number from QAPISourceInfo initializer

[John Snow]

With the QAPISourceInfo(None, None, None) construct gone, there's no
longer any reason to have to specify that a file starts on the first
line. Remove it from the initializer and default it to 1.

Remove the last vestiges where we check for 'line' being unset, that
can't happen, now.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 scripts/qapi/parser.py |  2 +-
 scripts/qapi/source.py | 10 +++-------
 2 files changed, 4 insertions(+), 8 deletions(-)

diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
index a53b735e7de..39dbcc4eacc 100644
--- a/scripts/qapi/parser.py
+++ b/scripts/qapi/parser.py
@@ -47,7 +47,7 @@ def __init__(self, fname, previously_included=None, incl_info=None):
         if self.src == '' or self.src[-1] != '\n':
             self.src += '\n'
         self.cursor = 0
-        self.info = QAPISourceInfo(fname, 1, incl_info)
+        self.info = QAPISourceInfo(fname, incl_info)
         self.line_pos = 0
         self.exprs = []
         self.docs = []
diff --git a/scripts/qapi/source.py b/scripts/qapi/source.py
index 1ade864d7b9..04193cc9643 100644
--- a/scripts/qapi/source.py
+++ b/scripts/qapi/source.py
@@ -31,10 +31,9 @@ def __init__(self) -> None:
 class QAPISourceInfo:
     T = TypeVar('T', bound='QAPISourceInfo')
 
-    def __init__(self, fname: str, line: int,
-                 parent: Optional['QAPISourceInfo']):
+    def __init__(self, fname: str, parent: Optional['QAPISourceInfo']):
         self.fname = fname
-        self.line = line
+        self.line = 1
         self.parent = parent
         self.pragma: QAPISchemaPragma = (
             parent.pragma if parent else QAPISchemaPragma()
@@ -52,10 +51,7 @@ def next_line(self: T) -> T:
         return info
 
     def loc(self) -> str:
-        ret = self.fname
-        if self.line is not None:
-            ret += ':%d' % self.line
-        return ret
+        return f"{self.fname}:{self.line}"
 
     def in_defn(self) -> str:
         if self.defn_name:
-- 
2.30.2

[PATCH v3 04/15] qapi/parser: factor parsing routine into method

[John Snow]

For the sake of keeping __init__ smaller (and treating it more like a
gallery of what state variables we can expect to see), put the actual
parsing action into a parse method. It remains invoked from the init
method to reduce churn.

To accomplish this, @previously_included becomes the private data
member ._included, and the filename is stashed as ._fname.

Add any missing declarations to the init method, and group them by
function so they can be understood quickly at a glance.

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

diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
index 39dbcc4eacc..d620706fffb 100644
--- a/scripts/qapi/parser.py
+++ b/scripts/qapi/parser.py
@@ -37,23 +37,39 @@ def __init__(self, parser, msg):
 class QAPISchemaParser:
 
     def __init__(self, fname, previously_included=None, incl_info=None):
-        previously_included = previously_included or set()
-        previously_included.add(os.path.abspath(fname))
+        self._fname = fname
+        self._included = previously_included or set()
+        self._included.add(os.path.abspath(self._fname))
+        self.src = ''
 
-        # May raise OSError; allow the caller to handle it.
-        with open(fname, 'r', encoding='utf-8') as fp:
-            self.src = fp.read()
-
-        if self.src == '' or self.src[-1] != '\n':
-            self.src += '\n'
+        # Lexer state (see `accept` for details):
+        self.info = QAPISourceInfo(self._fname, incl_info)
+        self.tok = None
+        self.pos = 0
         self.cursor = 0
-        self.info = QAPISourceInfo(fname, incl_info)
+        self.val = None
         self.line_pos = 0
+
+        # Parser output:
         self.exprs = []
         self.docs = []
-        self.accept()
+
+        # Showtime!
+        self._parse()
+
+    def _parse(self):
         cur_doc = None
 
+        # May raise OSError; allow the caller to handle it.
+        with open(self._fname, 'r', encoding='utf-8') as fp:
+            self.src = fp.read()
+        if self.src == '' or self.src[-1] != '\n':
+            self.src += '\n'
+
+        # Prime the lexer:
+        self.accept()
+
+        # Parse until done:
         while self.tok is not None:
             info = self.info
             if self.tok == '#':
@@ -71,12 +87,12 @@ def __init__(self, fname, previously_included=None, incl_info=None):
                 if not isinstance(include, str):
                     raise QAPISemError(info,
                                        "value of 'include' must be a string")
-                incl_fname = os.path.join(os.path.dirname(fname),
+                incl_fname = os.path.join(os.path.dirname(self._fname),
                                           include)
                 self.exprs.append({'expr': {'include': incl_fname},
                                    'info': info})
                 exprs_include = self._include(include, info, incl_fname,
-                                              previously_included)
+                                              self._included)
                 if exprs_include:
                     self.exprs.extend(exprs_include.exprs)
                     self.docs.extend(exprs_include.docs)
-- 
2.30.2

[PATCH v3 05/15] qapi/parser: Assert lexer value is a string

[John Snow]

The type checker can't narrow the type of the token value to string,
because it's only loosely correlated with the return token.

We know that a token of '#' should always have a "str" value.
Add an assertion.

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

diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
index d620706fffb..0bc852eda72 100644
--- a/scripts/qapi/parser.py
+++ b/scripts/qapi/parser.py
@@ -304,6 +304,7 @@ def get_doc(self, info):
         cur_doc = QAPIDoc(self, info)
         self.accept(False)
         while self.tok == '#':
+            assert isinstance(self.val, str)
             if self.val.startswith('##'):
                 # End of doc comment
                 if self.val != '##':
-- 
2.30.2

[PATCH v3 06/15] qapi/parser: enforce all top-level expressions must be dict in _parse()

[John Snow]

Instead of using get_expr nested=False, allow get_expr to always return
any expression. In exchange, add a new error message to the top-level
parser that explains the semantic error: Top-level expressions must
always be JSON objects.

This helps mypy understand the rest of this function which assumes that
get_expr did indeed return a dict.

The exception type changes from QAPIParseError to QAPISemError as a
result, and the error message in two tests now changes.

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

---

Thanks Markus, I like this quite a bit better. I think I swung the
pendulum back too far away from "try not to change anything". This is
cleaner.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 scripts/qapi/parser.py                        | 14 ++++++++------
 tests/qapi-schema/non-objects.err             |  2 +-
 tests/qapi-schema/quoted-structural-chars.err |  2 +-
 3 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
index 0bc852eda72..ffdd4298b6b 100644
--- a/scripts/qapi/parser.py
+++ b/scripts/qapi/parser.py
@@ -78,7 +78,11 @@ def _parse(self):
                     self.docs.append(cur_doc)
                 continue
 
-            expr = self.get_expr(False)
+            expr = self.get_expr()
+            if not isinstance(expr, dict):
+                raise QAPISemError(
+                    info, "top-level expression must be an object")
+
             if 'include' in expr:
                 self.reject_expr_doc(cur_doc)
                 if len(expr) != 1:
@@ -251,7 +255,7 @@ def get_members(self):
             self.accept()
             if key in expr:
                 raise QAPIParseError(self, "duplicate key '%s'" % key)
-            expr[key] = self.get_expr(True)
+            expr[key] = self.get_expr()
             if self.tok == '}':
                 self.accept()
                 return expr
@@ -270,7 +274,7 @@ def get_values(self):
             raise QAPIParseError(
                 self, "expected '{', '[', ']', string, or boolean")
         while True:
-            expr.append(self.get_expr(True))
+            expr.append(self.get_expr())
             if self.tok == ']':
                 self.accept()
                 return expr
@@ -278,9 +282,7 @@ def get_values(self):
                 raise QAPIParseError(self, "expected ',' or ']'")
             self.accept()
 
-    def get_expr(self, nested):
-        if self.tok != '{' and not nested:
-            raise QAPIParseError(self, "expected '{'")
+    def get_expr(self):
         if self.tok == '{':
             self.accept()
             expr = self.get_members()
diff --git a/tests/qapi-schema/non-objects.err b/tests/qapi-schema/non-objects.err
index 3a4ea36966e..23bdb69c711 100644
--- a/tests/qapi-schema/non-objects.err
+++ b/tests/qapi-schema/non-objects.err
@@ -1 +1 @@
-non-objects.json:1:1: expected '{'
+non-objects.json:1: top-level expression must be an object
diff --git a/tests/qapi-schema/quoted-structural-chars.err b/tests/qapi-schema/quoted-structural-chars.err
index 07d1561d1f7..af6c1e173db 100644
--- a/tests/qapi-schema/quoted-structural-chars.err
+++ b/tests/qapi-schema/quoted-structural-chars.err
@@ -1 +1 @@
-quoted-structural-chars.json:1:1: expected '{'
+quoted-structural-chars.json:1: top-level expression must be an object
-- 
2.30.2

[PATCH v3 07/15] qapi/parser: assert object keys are strings

[John Snow]

The single quote token implies the value is a string. Assert this to be
the case, to allow us to write an accurate return type for get_members.

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

diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
index ffdd4298b6b..4959630ce64 100644
--- a/scripts/qapi/parser.py
+++ b/scripts/qapi/parser.py
@@ -249,6 +249,8 @@ def get_members(self):
             raise QAPIParseError(self, "expected string or '}'")
         while True:
             key = self.val
+            assert isinstance(key, str)  # Guaranteed by tok == "'"
+
             self.accept()
             if self.tok != ':':
                 raise QAPIParseError(self, "expected ':'")
-- 
2.30.2

[PATCH v3 08/15] qapi/parser: Use @staticmethod where appropriate

[John Snow]

No self, no thank you!

(Quiets pylint warnings.)

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

diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
index 4959630ce64..7c718661950 100644
--- a/scripts/qapi/parser.py
+++ b/scripts/qapi/parser.py
@@ -130,7 +130,8 @@ def reject_expr_doc(doc):
                 "documentation for '%s' is not followed by the definition"
                 % doc.symbol)
 
-    def _include(self, include, info, incl_fname, previously_included):
+    @staticmethod
+    def _include(include, info, incl_fname, previously_included):
         incl_abs_fname = os.path.abspath(incl_fname)
         # catch inclusion cycle
         inf = info
@@ -151,7 +152,8 @@ def _include(self, include, info, incl_fname, previously_included):
                 f"can't read include file '{incl_fname}': {err.strerror}"
             ) from err
 
-    def _check_pragma_list_of_str(self, name, value, info):
+    @staticmethod
+    def _check_pragma_list_of_str(name, value, info):
         if (not isinstance(value, list)
                 or any([not isinstance(elt, str) for elt in value])):
             raise QAPISemError(
-- 
2.30.2

[PATCH v3 09/15] qapi: add must_match helper

[John Snow]

Mypy cannot generally understand that these regex functions cannot
possibly fail. Add a "must_match" helper that makes this clear for
mypy.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 scripts/qapi/common.py |  8 +++++++-
 scripts/qapi/main.py   |  6 ++----
 scripts/qapi/parser.py | 13 +++++++------
 3 files changed, 16 insertions(+), 11 deletions(-)

diff --git a/scripts/qapi/common.py b/scripts/qapi/common.py
index cbd3fd81d36..6ad1eeb61d4 100644
--- a/scripts/qapi/common.py
+++ b/scripts/qapi/common.py
@@ -12,7 +12,7 @@
 # See the COPYING file in the top-level directory.
 
 import re
-from typing import Optional, Sequence
+from typing import Match, Optional, Sequence
 
 
 #: Magic string that gets removed along with all space to its right.
@@ -210,3 +210,9 @@ def gen_endif(ifcond: Sequence[str]) -> str:
 #endif /* %(cond)s */
 ''', cond=ifc)
     return ret
+
+
+def must_match(pattern: str, string: str) -> Match[str]:
+    match = re.match(pattern, string)
+    assert match is not None
+    return match
diff --git a/scripts/qapi/main.py b/scripts/qapi/main.py
index 703e7ed1ed5..f2ea6e0ce4a 100644
--- a/scripts/qapi/main.py
+++ b/scripts/qapi/main.py
@@ -8,11 +8,11 @@
 """
 
 import argparse
-import re
 import sys
 from typing import Optional
 
 from .commands import gen_commands
+from .common import must_match
 from .error import QAPIError
 from .events import gen_events
 from .introspect import gen_introspect
@@ -22,9 +22,7 @@
 
 
 def invalid_prefix_char(prefix: str) -> Optional[str]:
-    match = re.match(r'([A-Za-z_.-][A-Za-z0-9_.-]*)?', prefix)
-    # match cannot be None, but mypy cannot infer that.
-    assert match is not None
+    match = must_match(r'([A-Za-z_.-][A-Za-z0-9_.-]*)?', prefix)
     if match.end() != len(prefix):
         return prefix[match.end()]
     return None
diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
index 7c718661950..48137d3fbec 100644
--- a/scripts/qapi/parser.py
+++ b/scripts/qapi/parser.py
@@ -18,6 +18,7 @@
 import os
 import re
 
+from .common import must_match
 from .error import QAPISemError, QAPISourceError
 from .source import QAPISourceInfo
 
@@ -238,8 +239,8 @@ def accept(self, skip_comment=True):
             elif not self.tok.isspace():
                 # Show up to next structural, whitespace or quote
                 # character
-                match = re.match('[^[\\]{}:,\\s\'"]+',
-                                 self.src[self.cursor-1:])
+                match = must_match('[^[\\]{}:,\\s\'"]+',
+                                   self.src[self.cursor-1:])
                 raise QAPIParseError(self, "stray '%s'" % match.group(0))
 
     def get_members(self):
@@ -369,7 +370,7 @@ def append(self, line):
             # Strip leading spaces corresponding to the expected indent level
             # Blank lines are always OK.
             if line:
-                indent = re.match(r'\s*', line).end()
+                indent = must_match(r'\s*', line).end()
                 if indent < self._indent:
                     raise QAPIParseError(
                         self._parser,
@@ -505,7 +506,7 @@ def _append_args_line(self, line):
             # from line and replace it with spaces so that 'f' has the
             # same index as it did in the original line and can be
             # handled the same way we will handle following lines.
-            indent = re.match(r'@\S*:\s*', line).end()
+            indent = must_match(r'@\S*:\s*', line).end()
             line = line[indent:]
             if not line:
                 # Line was just the "@arg:" header; following lines
@@ -540,7 +541,7 @@ def _append_features_line(self, line):
             # from line and replace it with spaces so that 'f' has the
             # same index as it did in the original line and can be
             # handled the same way we will handle following lines.
-            indent = re.match(r'@\S*:\s*', line).end()
+            indent = must_match(r'@\S*:\s*', line).end()
             line = line[indent:]
             if not line:
                 # Line was just the "@arg:" header; following lines
@@ -586,7 +587,7 @@ def _append_various_line(self, line):
             # from line and replace it with spaces so that 'f' has the
             # same index as it did in the original line and can be
             # handled the same way we will handle following lines.
-            indent = re.match(r'\S*:\s*', line).end()
+            indent = must_match(r'\S*:\s*', line).end()
             line = line[indent:]
             if not line:
                 # Line was just the "Section:" header; following lines
-- 
2.30.2

[PATCH v3 10/15] qapi/parser: Fix token membership tests when token can be None

[John Snow]

When the token can be None (EOF), we can't use 'x in "abc"' style
membership tests to group types of tokens together, because 'None in
"abc"' is a TypeError.

Easy enough to fix. (Use a tuple: It's neither a static typing error nor
a runtime error to check for None in Tuple[str, ...])

Add tests to prevent a regression. (Note: they cannot be added prior to
this fix, as the unhandled stack trace will not match test output in the
CI system.)

Signed-off-by: John Snow <jsnow@redhat.com>
---
 scripts/qapi/parser.py                               | 5 +++--
 tests/qapi-schema/meson.build                        | 2 ++
 tests/qapi-schema/missing-array-rsqb.err             | 1 +
 tests/qapi-schema/missing-array-rsqb.json            | 1 +
 tests/qapi-schema/missing-array-rsqb.out             | 0
 tests/qapi-schema/missing-object-member-element.err  | 1 +
 tests/qapi-schema/missing-object-member-element.json | 1 +
 tests/qapi-schema/missing-object-member-element.out  | 0
 8 files changed, 9 insertions(+), 2 deletions(-)
 create mode 100644 tests/qapi-schema/missing-array-rsqb.err
 create mode 100644 tests/qapi-schema/missing-array-rsqb.json
 create mode 100644 tests/qapi-schema/missing-array-rsqb.out
 create mode 100644 tests/qapi-schema/missing-object-member-element.err
 create mode 100644 tests/qapi-schema/missing-object-member-element.json
 create mode 100644 tests/qapi-schema/missing-object-member-element.out

diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
index 48137d3fbec..9f980f75139 100644
--- a/scripts/qapi/parser.py
+++ b/scripts/qapi/parser.py
@@ -275,7 +275,7 @@ def get_values(self):
         if self.tok == ']':
             self.accept()
             return expr
-        if self.tok not in "{['tf":
+        if self.tok not in tuple("{['tf"):
             raise QAPIParseError(
                 self, "expected '{', '[', ']', string, or boolean")
         while True:
@@ -294,7 +294,8 @@ def get_expr(self):
         elif self.tok == '[':
             self.accept()
             expr = self.get_values()
-        elif self.tok in "'tf":
+        elif self.tok in tuple("'tf"):
+            assert isinstance(self.val, (str, bool))
             expr = self.val
             self.accept()
         else:
diff --git a/tests/qapi-schema/meson.build b/tests/qapi-schema/meson.build
index dc448e8f74d..9e8f658ce38 100644
--- a/tests/qapi-schema/meson.build
+++ b/tests/qapi-schema/meson.build
@@ -134,9 +134,11 @@ schemas = [
   'indented-expr.json',
   'leading-comma-list.json',
   'leading-comma-object.json',
+  'missing-array-rsqb.json',
   'missing-colon.json',
   'missing-comma-list.json',
   'missing-comma-object.json',
+  'missing-object-member-element.json',
   'missing-type.json',
   'nested-struct-data.json',
   'nested-struct-data-invalid-dict.json',
diff --git a/tests/qapi-schema/missing-array-rsqb.err b/tests/qapi-schema/missing-array-rsqb.err
new file mode 100644
index 00000000000..b5f58b8c12a
--- /dev/null
+++ b/tests/qapi-schema/missing-array-rsqb.err
@@ -0,0 +1 @@
+missing-array-rsqb.json:1:44: expected '{', '[', string, or boolean
diff --git a/tests/qapi-schema/missing-array-rsqb.json b/tests/qapi-schema/missing-array-rsqb.json
new file mode 100644
index 00000000000..7fca1df923c
--- /dev/null
+++ b/tests/qapi-schema/missing-array-rsqb.json
@@ -0,0 +1 @@
+['Daisy,', 'Daisy,', 'Give me your answer',
diff --git a/tests/qapi-schema/missing-array-rsqb.out b/tests/qapi-schema/missing-array-rsqb.out
new file mode 100644
index 00000000000..e69de29bb2d
diff --git a/tests/qapi-schema/missing-object-member-element.err b/tests/qapi-schema/missing-object-member-element.err
new file mode 100644
index 00000000000..c08a3dc307f
--- /dev/null
+++ b/tests/qapi-schema/missing-object-member-element.err
@@ -0,0 +1 @@
+missing-object-member-element.json:1:8: expected '{', '[', string, or boolean
diff --git a/tests/qapi-schema/missing-object-member-element.json b/tests/qapi-schema/missing-object-member-element.json
new file mode 100644
index 00000000000..f52d0106f31
--- /dev/null
+++ b/tests/qapi-schema/missing-object-member-element.json
@@ -0,0 +1 @@
+{'key':
diff --git a/tests/qapi-schema/missing-object-member-element.out b/tests/qapi-schema/missing-object-member-element.out
new file mode 100644
index 00000000000..e69de29bb2d
-- 
2.30.2

[PATCH v3 11/15] qapi/parser: Rework _check_pragma_list_of_str as a TypeGuard

[John Snow]

TypeGuards wont exist in Python proper until 3.10. Ah well. We can hack
up our own by declaring this function to return the type we claim it
checks for and using this to safely downcast object -> List[str].

In so doing, I bring this function under _pragma so it can use the
'info' object in its closure. Having done this, _pragma also now no
longer needs to take a 'self' parameter, so drop it.

To help with line-length, and with the context evident from its new
scope, rename the function to the shorter check_list_str().

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

---

I left (name, value) as args to avoid creating a fully magic "macro",
because this looked simply too weird:

    info.pragma.foobar = check_list_str()

and it looked more reasonable as:

    info.pragma.foobar = check_list_str(name, value)

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

diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
index 9f980f75139..8a58e1228f0 100644
--- a/scripts/qapi/parser.py
+++ b/scripts/qapi/parser.py
@@ -17,6 +17,7 @@
 from collections import OrderedDict
 import os
 import re
+from typing import List
 
 from .common import must_match
 from .error import QAPISemError, QAPISourceError
@@ -154,28 +155,29 @@ def _include(include, info, incl_fname, previously_included):
             ) from err
 
     @staticmethod
-    def _check_pragma_list_of_str(name, value, info):
-        if (not isinstance(value, list)
-                or any([not isinstance(elt, str) for elt in value])):
-            raise QAPISemError(
-                info,
-                "pragma %s must be a list of strings" % name)
+    def _pragma(name, value, info):
+
+        def check_list_str(name, value) -> List[str]:
+            if (not isinstance(value, list) or
+                    any([not isinstance(elt, str) for elt in value])):
+                raise QAPISemError(
+                    info,
+                    "pragma %s must be a list of strings" % name)
+            return value
+
+        pragma = info.pragma
 
-    def _pragma(self, name, value, info):
         if name == 'doc-required':
             if not isinstance(value, bool):
                 raise QAPISemError(info,
                                    "pragma 'doc-required' must be boolean")
-            info.pragma.doc_required = value
+            pragma.doc_required = value
         elif name == 'command-name-exceptions':
-            self._check_pragma_list_of_str(name, value, info)
-            info.pragma.command_name_exceptions = value
+            pragma.command_name_exceptions = check_list_str(name, value)
         elif name == 'command-returns-exceptions':
-            self._check_pragma_list_of_str(name, value, info)
-            info.pragma.command_returns_exceptions = value
+            pragma.command_returns_exceptions = check_list_str(name, value)
         elif name == 'member-name-exceptions':
-            self._check_pragma_list_of_str(name, value, info)
-            info.pragma.member_name_exceptions = value
+            pragma.member_name_exceptions = check_list_str(name, value)
         else:
             raise QAPISemError(info, "unknown pragma '%s'" % name)
 
-- 
2.30.2

[PATCH v3 12/15] qapi/parser: add type hint annotations

[John Snow]

Annotations do not change runtime behavior.
This commit *only* adds annotations.

(Annotations for QAPIDoc are in a forthcoming commit.)

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

diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
index 8a58e1228f0..419e36c8702 100644
--- a/scripts/qapi/parser.py
+++ b/scripts/qapi/parser.py
@@ -17,16 +17,26 @@
 from collections import OrderedDict
 import os
 import re
-from typing import List
+from typing import (
+    Dict,
+    List,
+    Optional,
+    Set,
+    Union,
+)
 
 from .common import must_match
 from .error import QAPISemError, QAPISourceError
 from .source import QAPISourceInfo
 
 
+# Return value alias for get_expr().
+_ExprValue = Union[List[object], Dict[str, object], str, bool]
+
+
 class QAPIParseError(QAPISourceError):
     """Error class for all QAPI schema parsing errors."""
-    def __init__(self, parser, msg):
+    def __init__(self, parser: 'QAPISchemaParser', msg: str):
         col = 1
         for ch in parser.src[parser.line_pos:parser.pos]:
             if ch == '\t':
@@ -38,7 +48,10 @@ def __init__(self, parser, msg):
 
 class QAPISchemaParser:
 
-    def __init__(self, fname, previously_included=None, incl_info=None):
+    def __init__(self,
+                 fname: str,
+                 previously_included: Optional[Set[str]] = None,
+                 incl_info: Optional[QAPISourceInfo] = None):
         self._fname = fname
         self._included = previously_included or set()
         self._included.add(os.path.abspath(self._fname))
@@ -46,20 +59,20 @@ def __init__(self, fname, previously_included=None, incl_info=None):
 
         # Lexer state (see `accept` for details):
         self.info = QAPISourceInfo(self._fname, incl_info)
-        self.tok = None
+        self.tok: Union[None, str] = None
         self.pos = 0
         self.cursor = 0
-        self.val = None
+        self.val: Optional[Union[bool, str]] = None
         self.line_pos = 0
 
         # Parser output:
-        self.exprs = []
-        self.docs = []
+        self.exprs: List[Dict[str, object]] = []
+        self.docs: List[QAPIDoc] = []
 
         # Showtime!
         self._parse()
 
-    def _parse(self):
+    def _parse(self) -> None:
         cur_doc = None
 
         # May raise OSError; allow the caller to handle it.
@@ -125,7 +138,7 @@ def _parse(self):
         self.reject_expr_doc(cur_doc)
 
     @staticmethod
-    def reject_expr_doc(doc):
+    def reject_expr_doc(doc: Optional['QAPIDoc']) -> None:
         if doc and doc.symbol:
             raise QAPISemError(
                 doc.info,
@@ -133,10 +146,14 @@ def reject_expr_doc(doc):
                 % doc.symbol)
 
     @staticmethod
-    def _include(include, info, incl_fname, previously_included):
+    def _include(include: str,
+                 info: QAPISourceInfo,
+                 incl_fname: str,
+                 previously_included: Set[str]
+                 ) -> Optional['QAPISchemaParser']:
         incl_abs_fname = os.path.abspath(incl_fname)
         # catch inclusion cycle
-        inf = info
+        inf: Optional[QAPISourceInfo] = info
         while inf:
             if incl_abs_fname == os.path.abspath(inf.fname):
                 raise QAPISemError(info, "inclusion loop for %s" % include)
@@ -155,9 +172,9 @@ def _include(include, info, incl_fname, previously_included):
             ) from err
 
     @staticmethod
-    def _pragma(name, value, info):
+    def _pragma(name: str, value: object, info: QAPISourceInfo) -> None:
 
-        def check_list_str(name, value) -> List[str]:
+        def check_list_str(name: str, value: object) -> List[str]:
             if (not isinstance(value, list) or
                     any([not isinstance(elt, str) for elt in value])):
                 raise QAPISemError(
@@ -181,7 +198,7 @@ def check_list_str(name, value) -> List[str]:
         else:
             raise QAPISemError(info, "unknown pragma '%s'" % name)
 
-    def accept(self, skip_comment=True):
+    def accept(self, skip_comment: bool = True) -> None:
         while True:
             self.tok = self.src[self.cursor]
             self.pos = self.cursor
@@ -245,8 +262,8 @@ def accept(self, skip_comment=True):
                                    self.src[self.cursor-1:])
                 raise QAPIParseError(self, "stray '%s'" % match.group(0))
 
-    def get_members(self):
-        expr = OrderedDict()
+    def get_members(self) -> Dict[str, object]:
+        expr: Dict[str, object] = OrderedDict()
         if self.tok == '}':
             self.accept()
             return expr
@@ -272,8 +289,8 @@ def get_members(self):
             if self.tok != "'":
                 raise QAPIParseError(self, "expected string")
 
-    def get_values(self):
-        expr = []
+    def get_values(self) -> List[object]:
+        expr: List[object] = []
         if self.tok == ']':
             self.accept()
             return expr
@@ -289,7 +306,8 @@ def get_values(self):
                 raise QAPIParseError(self, "expected ',' or ']'")
             self.accept()
 
-    def get_expr(self):
+    def get_expr(self) -> _ExprValue:
+        expr: _ExprValue
         if self.tok == '{':
             self.accept()
             expr = self.get_members()
@@ -305,7 +323,7 @@ def get_expr(self):
                 self, "expected '{', '[', string, or boolean")
         return expr
 
-    def get_doc(self, info):
+    def get_doc(self, info: QAPISourceInfo) -> List['QAPIDoc']:
         if self.val != '##':
             raise QAPIParseError(
                 self, "junk after '##' at start of documentation comment")
-- 
2.30.2

[PATCH v3 13/15] qapi/parser: Remove superfluous list comprehension

[John Snow]

A generator suffices (and quiets a pylint warning).

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

diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
index 419e36c8702..b0f18c0d176 100644
--- a/scripts/qapi/parser.py
+++ b/scripts/qapi/parser.py
@@ -176,7 +176,7 @@ def _pragma(name: str, value: object, info: QAPISourceInfo) -> None:
 
         def check_list_str(name: str, value: object) -> List[str]:
             if (not isinstance(value, list) or
-                    any([not isinstance(elt, str) for elt in value])):
+                    any(not isinstance(elt, str) for elt in value)):
                 raise QAPISemError(
                     info,
                     "pragma %s must be a list of strings" % name)
-- 
2.30.2

[PATCH v3 14/15] qapi/parser: allow 'ch' variable name

[John Snow]

We can have a two-letter variable name, as a treat.

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

--

I don't want to use 'chr' or 'char', and in context 'ch' works well
enough. I will assume that any possible future uses will also be obvious
enough.

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 88efbf71cb2..c5275d5f59b 100644
--- a/scripts/qapi/pylintrc
+++ b/scripts/qapi/pylintrc
@@ -43,6 +43,7 @@ good-names=i,
            _,
            fp,  # fp = open(...)
            fd,  # fd = os.open(...)
+           ch,
 
 [VARIABLES]
 
-- 
2.30.2

[PATCH v3 15/15] qapi/parser: add docstrings

[John Snow]

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

diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
index b0f18c0d176..06167ed3e0a 100644
--- a/scripts/qapi/parser.py
+++ b/scripts/qapi/parser.py
@@ -47,7 +47,27 @@ def __init__(self, parser: 'QAPISchemaParser', msg: str):
 
 
 class QAPISchemaParser:
+    """
+    Parse QAPI schema source.
 
+    Parse a JSON-esque schema file and process directives.  See
+    qapi-code-gen.txt section "Schema Syntax" for the exact syntax.
+    Grammatical validation is handled later by `expr.check_exprs()`.
+
+    :param fname: Source file name.
+    :param previously_included:
+        The absolute names of previously included source files,
+        if being invoked from another parser.
+    :param incl_info:
+       `QAPISourceInfo` belonging to the parent module.
+       ``None`` implies this is the root module.
+
+    :ivar exprs: Resulting parsed expressions.
+    :ivar docs: Resulting parsed documentation blocks.
+
+    :raise OSError: For problems reading the root schema document.
+    :raise QAPIError: For errors in the schema source.
+    """
     def __init__(self,
                  fname: str,
                  previously_included: Optional[Set[str]] = None,
@@ -73,6 +93,11 @@ def __init__(self,
         self._parse()
 
     def _parse(self) -> None:
+        """
+        Parse the QAPI schema document.
+
+        :return: None. Results are stored in ``.exprs`` and ``.docs``.
+        """
         cur_doc = None
 
         # May raise OSError; allow the caller to handle it.
@@ -199,6 +224,50 @@ def check_list_str(name: str, value: object) -> List[str]:
             raise QAPISemError(info, "unknown pragma '%s'" % name)
 
     def accept(self, skip_comment: bool = True) -> None:
+        """
+        Read and store the next token.
+
+        :param skip_comment:
+            When false, return COMMENT tokens ("#").
+            This is used when reading documentation blocks.
+
+        :return:
+            None. Several instance attributes are updated instead:
+
+            - ``.tok`` represents the token type. See below for values.
+            - ``.info`` describes the token's source location.
+            - ``.val`` is the token's value, if any. See below.
+            - ``.pos`` is the buffer index of the first character of
+              the token.
+
+        * Single-character tokens:
+
+            These are "{", "}", ":", ",", "[", and "]". ``.tok`` holds
+            the single character and ``.val`` is None.
+
+        * Multi-character tokens:
+
+          * COMMENT:
+
+            This token is not normally returned by the lexer, but it can
+            be when ``skip_comment`` is False. ``.tok`` is "#", and
+            ``.val`` is a string including all chars until end-of-line,
+            including the "#" itself.
+
+          * STRING:
+
+            ``.tok`` is "'", the single quote. ``.val`` contains the
+            string, excluding the surrounding quotes.
+
+          * TRUE and FALSE:
+
+            ``.tok`` is either "t" or "f", ``.val`` will be the
+            corresponding bool value.
+
+          * EOF:
+
+            ``.tok`` and ``.val`` will both be None at EOF.
+        """
         while True:
             self.tok = self.src[self.cursor]
             self.pos = self.cursor
-- 
2.30.2

Re: [PATCH v3 00/15] qapi: static typing conversion, pt5a

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> This is part five (a), and focuses on QAPISchemaParser in parser.py.
> It does not touch QAPIDoc yet, which will be covered next.
> 
> gitlab: https://gitlab.com/jsnow/qemu/-/commits/python-qapi-cleanup-pt5a
> 
> 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/`

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

and queued with doc string spacing slightly tweaked to consistently
satisfy PEP 8's two spaces between sentences rule.  Thanks!

Re: [PATCH v3 14/15] qapi/parser: allow 'ch' variable name

[Markus Armbruster]

Uh, one more little thing... this commit message isn't quite right:

John Snow <jsnow@redhat.com> writes:

> We can have a two-letter variable name, as a treat.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
>
> --

This line is not a separator.

>
> I don't want to use 'chr' or 'char', and in context 'ch' works well
> enough. I will assume that any possible future uses will also be obvious
> enough.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---

This one is.

Two S-o-b.  Okay to manually toss everything after the "--" line?

>  scripts/qapi/pylintrc | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/scripts/qapi/pylintrc b/scripts/qapi/pylintrc
> index 88efbf71cb2..c5275d5f59b 100644
> --- a/scripts/qapi/pylintrc
> +++ b/scripts/qapi/pylintrc
> @@ -43,6 +43,7 @@ good-names=i,
>             _,
>             fp,  # fp = open(...)
>             fd,  # fd = os.open(...)
> +           ch,
>  
>  [VARIABLES]

Re: [PATCH v3 14/15] qapi/parser: allow 'ch' variable name

[John Snow]

On 5/20/21 5:36 AM, Markus Armbruster wrote:
> Uh, one more little thing... this commit message isn't quite right:
> 
> John Snow <jsnow@redhat.com> writes:
> 
>> We can have a two-letter variable name, as a treat.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>>
>> --
> 
> This line is not a separator.
> 
>>
>> I don't want to use 'chr' or 'char', and in context 'ch' works well
>> enough. I will assume that any possible future uses will also be obvious
>> enough.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
> 
> This one is.
> 
> Two S-o-b.  Okay to manually toss everything after the "--" line?
> 

Whoops. Script failure because of the typo in the separator I added 
manually. Yes.

```
qapi/parser: allow 'ch' variable name

We can have a two-letter variable name, as a treat.

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

Is fine. Any edits you want to make to my joking, not-that-useful commit 
message is also fine.

>>   scripts/qapi/pylintrc | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/scripts/qapi/pylintrc b/scripts/qapi/pylintrc
>> index 88efbf71cb2..c5275d5f59b 100644
>> --- a/scripts/qapi/pylintrc
>> +++ b/scripts/qapi/pylintrc
>> @@ -43,6 +43,7 @@ good-names=i,
>>              _,
>>              fp,  # fp = open(...)
>>              fd,  # fd = os.open(...)
>> +           ch,
>>   
>>   [VARIABLES]