[PATCH 00/26] qapi: static typing conversion, pt5

[PATCH 00/26] qapi: static typing conversion, pt5

[John Snow]

based-on: <20200922210101.4081073-1-jsnow@redhat.com>
          [PATCH 0/6] qapi: static typing conversion, pt4

Hi, this series adds static type hints to the QAPI module.
This is part five!

Part 5: https://gitlab.com/jsnow/qemu/-/tree/python-qapi-cleanup-pt5
Everything: https://gitlab.com/jsnow/qemu/-/tree/python-qapi-cleanup-pt6

- Requires Python 3.6+
- Requires mypy 0.770 or newer (for type analysis only)
- Requires pylint 2.6.0 or newer (for lint checking only)

This part of the series focuses on just parser.py.

Type hints are added in patches that add *only* type hints and change no
other behavior. Any necessary changes to behavior to accommodate typing
are split out into their own tiny patches.

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

John Snow (26):
  qapi/parser.py: refactor parsing routine into method
  qapi/parser.py: group variable declarations in __init__
  qapi/parser.py: use 'with' statement for opening files
  qapi/source.py: Add default arguments to QAPISourceInfo
  qapi/parser.py: start source info at line 0
  qapi/parser.py: raise QAPIParseError during file opening
  qapi/parser.py: fully remove 'null' constant
  qapi/parser.py: Assert lexer value is a string
  qapi/parser.py: assert get_expr returns object in outer loop
  qapi/parser.py: assert object keys are strings
  qapi/parser.py: Convert several methods to @classmethod
  qapi/parser.py: add casts to pragma checks
  qapi/parser.py: add type hint annotations
  qapi/parser.py: add docstrings
  qapi/parser.py: add ParsedExpression type
  qapi/pragma.py: Move QAPISchemaPragma into its own module
  qapi/pragma.py: Move pragma parsing out of parser.py
  qapi/parser.py: Modify _include() to use parser state
  qapi/parser.py: add parent argument
  qapi/parser.py: remove unused check_args_section arguments
  qapi/parser.py: QAPIDoc: convert @staticmethod to @classmethod
  qapi/parser.py: add type hint annotations (QAPIDoc)
  qapi/parser.py: enable mypy checks
  qapi/parser.py: remove one and two-letter variables
  qapi/parser.py: Silence too-few-public-methods warning
  qapi/parser.py: enable pylint checks

 scripts/qapi/error.py                     |   8 +-
 scripts/qapi/expr.py                      |  56 ++--
 scripts/qapi/mypy.ini                     |   5 -
 scripts/qapi/parser.py                    | 322 +++++++++++++---------
 scripts/qapi/pragma.py                    |  68 +++++
 scripts/qapi/pylintrc                     |   3 +-
 scripts/qapi/schema.py                    |   6 +-
 scripts/qapi/source.py                    |  22 +-
 tests/qapi-schema/leading-comma-list.err  |   2 +-
 tests/qapi-schema/trailing-comma-list.err |   2 +-
 10 files changed, 301 insertions(+), 193 deletions(-)
 create mode 100644 scripts/qapi/pragma.py

-- 
2.26.2

[PATCH 01/26] qapi/parser.py: refactor parsing routine into method

[John Snow]

For the sake of keeping __init__ easier to reason about, with fewer
variables in play.

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

diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
index 8be4570c31..55117f5754 100644
--- a/scripts/qapi/parser.py
+++ b/scripts/qapi/parser.py
@@ -46,28 +46,34 @@ class QAPIDocError(QAPIError):
 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.cursor = 0
+        self.info = QAPISourceInfo(self._fname, 1, incl_info)
+        self.line_pos = 0
+        self.exprs = []
+        self.docs = []
 
         try:
-            fp = open(fname, 'r', encoding='utf-8')
+            fp = open(self._fname, 'r', encoding='utf-8')
             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,
+                                  self._fname,
                                   e.strerror))
+        self._parse()
 
+    def _parse(self):
+        cur_doc = None
+
+        # Prime the lexer:
         if self.src == '' or self.src[-1] != '\n':
             self.src += '\n'
-        self.cursor = 0
-        self.info = QAPISourceInfo(fname, 1, incl_info)
-        self.line_pos = 0
-        self.exprs = []
-        self.docs = []
         self.accept()
-        cur_doc = None
 
         while self.tok is not None:
             info = self.info
@@ -86,12 +92,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.26.2

[PATCH 02/26] qapi/parser.py: group variable declarations in __init__

[John Snow]

Add missing declarations and group them by function so they're easier to
understand at a distance.

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

diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
index 55117f5754..b2984c8ff0 100644
--- a/scripts/qapi/parser.py
+++ b/scripts/qapi/parser.py
@@ -50,12 +50,19 @@ def __init__(self, fname, previously_included=None, incl_info=None):
         self._included = previously_included or set()
         self._included.add(os.path.abspath(self._fname))
 
+        # Lexer state (see `accept` for details):
+        self.tok = None
+        self.pos = 0
         self.cursor = 0
+        self.val = None
         self.info = QAPISourceInfo(self._fname, 1, incl_info)
         self.line_pos = 0
+
+        # Parser output:
         self.exprs = []
         self.docs = []
 
+        # Showtime!
         try:
             fp = open(self._fname, 'r', encoding='utf-8')
             self.src = fp.read()
-- 
2.26.2

[PATCH 03/26] qapi/parser.py: use 'with' statement for opening files

[John Snow]

Minor, this prevents file descriptor leaks on error pathways.

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

diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
index b2984c8ff0..d0f35fe03e 100644
--- a/scripts/qapi/parser.py
+++ b/scripts/qapi/parser.py
@@ -64,8 +64,8 @@ def __init__(self, fname, previously_included=None, incl_info=None):
 
         # Showtime!
         try:
-            fp = open(self._fname, 'r', encoding='utf-8')
-            self.src = fp.read()
+            with open(self._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"
-- 
2.26.2

[PATCH 04/26] qapi/source.py: Add default arguments to QAPISourceInfo

[John Snow]

The parser tries to create an object as QAPISourceInfo(None, None,
None). Add some defaults to QAPISourceInfo such that we don't need to
pass explicit as many explicit nothings.

Having the defaults nearby the code in source.py also helps remind us
that they might be unset.

Using a numerical 0 to mean "no line" is nicer to type than using None
for the same.

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

diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
index d0f35fe03e..db4e9ae872 100644
--- a/scripts/qapi/parser.py
+++ b/scripts/qapi/parser.py
@@ -67,7 +67,7 @@ def __init__(self, fname, previously_included=None, incl_info=None):
             with open(self._fname, 'r', encoding='utf-8') as fp:
                 self.src = fp.read()
         except IOError as e:
-            raise QAPISemError(incl_info or QAPISourceInfo(None, None, None),
+            raise QAPISemError(incl_info or QAPISourceInfo(None),
                                "can't read %s file '%s': %s"
                                % ("include" if incl_info else "schema",
                                   self._fname,
diff --git a/scripts/qapi/source.py b/scripts/qapi/source.py
index ba991d798f..d1de9e36b1 100644
--- a/scripts/qapi/source.py
+++ b/scripts/qapi/source.py
@@ -30,7 +30,10 @@ def __init__(self) -> None:
 class QAPISourceInfo:
     T = TypeVar('T', bound='QAPISourceInfo')
 
-    def __init__(self: T, fname: str, line: int, parent: Optional[T]):
+    def __init__(self: T,
+                 fname: str,
+                 line: int = 0,
+                 parent: Optional[T] = None):
         self.fname = fname
         self.line = line
         self.parent = parent
@@ -53,7 +56,7 @@ def loc(self) -> str:
         if self.fname is None:
             return sys.argv[0]
         ret = self.fname
-        if self.line is not None:
+        if self.line:
             ret += ':%d' % self.line
         return ret
 
-- 
2.26.2

[PATCH 05/26] qapi/parser.py: start source info at line 0

[John Snow]

Before we have any actual line context, we still have a file context. We
can use this to report errors without mentioning a specific line.

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

diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
index db4e9ae872..fc0b1516cc 100644
--- a/scripts/qapi/parser.py
+++ b/scripts/qapi/parser.py
@@ -55,7 +55,7 @@ def __init__(self, fname, previously_included=None, incl_info=None):
         self.pos = 0
         self.cursor = 0
         self.val = None
-        self.info = QAPISourceInfo(self._fname, 1, incl_info)
+        self.info = QAPISourceInfo(self._fname, parent=incl_info)
         self.line_pos = 0
 
         # Parser output:
@@ -78,6 +78,7 @@ def _parse(self):
         cur_doc = None
 
         # Prime the lexer:
+        self.info.line += 1
         if self.src == '' or self.src[-1] != '\n':
             self.src += '\n'
         self.accept()
-- 
2.26.2

[PATCH 06/26] qapi/parser.py: raise QAPIParseError during file opening

[John Snow]

Now that we can have exception contexts that don't specify a specific
line, we can use that context to raise errors when opening the file.

If we don't have a parent context, we can simply use our own.

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

diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
index fc0b1516cc..5a7233bc76 100644
--- a/scripts/qapi/parser.py
+++ b/scripts/qapi/parser.py
@@ -67,11 +67,13 @@ def __init__(self, fname, previously_included=None, incl_info=None):
             with open(self._fname, 'r', encoding='utf-8') as fp:
                 self.src = fp.read()
         except IOError as e:
-            raise QAPISemError(incl_info or QAPISourceInfo(None),
-                               "can't read %s file '%s': %s"
-                               % ("include" if incl_info else "schema",
-                                  self._fname,
-                                  e.strerror))
+            msg = "can't read {kind:s} file '{fname:s}': {errmsg:s}".format(
+                kind='include' if incl_info else 'schema',
+                fname=self._fname,
+                errmsg=e.strerror
+            )
+            context = incl_info or self.info
+            raise QAPIParseError(context, msg) from e
         self._parse()
 
     def _parse(self):
-- 
2.26.2

[PATCH 07/26] qapi/parser.py: fully remove 'null' constant

[John Snow]

Based on the docs, we don't support the null constant, and the code
agrees. There's a few remnants where callers check .tok for 'n', and
these can be removed.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 scripts/qapi/parser.py                    | 8 ++++----
 tests/qapi-schema/leading-comma-list.err  | 2 +-
 tests/qapi-schema/trailing-comma-list.err | 2 +-
 3 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
index 5a7233bc76..78355ca93f 100644
--- a/scripts/qapi/parser.py
+++ b/scripts/qapi/parser.py
@@ -274,9 +274,9 @@ def get_values(self):
         if self.tok == ']':
             self.accept()
             return expr
-        if self.tok not in "{['tfn":
+        if self.tok not in "{['tf":
             raise self._parse_error(
-                "expected '{', '[', ']', string, boolean or 'null'")
+                "expected '{', '[', ']', string, or boolean")
         while True:
             expr.append(self.get_expr(True))
             if self.tok == ']':
@@ -295,12 +295,12 @@ def get_expr(self, nested):
         elif self.tok == '[':
             self.accept()
             expr = self.get_values()
-        elif self.tok in "'tfn":
+        elif self.tok in "'tf":
             expr = self.val
             self.accept()
         else:
             raise self._parse_error(
-                "expected '{', '[', string, boolean or 'null'")
+                "expected '{', '[', string, or boolean")
         return expr
 
     def _get_doc(self, info):
diff --git a/tests/qapi-schema/leading-comma-list.err b/tests/qapi-schema/leading-comma-list.err
index 76eed2b5b3..0725d6529f 100644
--- a/tests/qapi-schema/leading-comma-list.err
+++ b/tests/qapi-schema/leading-comma-list.err
@@ -1 +1 @@
-leading-comma-list.json:2:13: expected '{', '[', ']', string, boolean or 'null'
+leading-comma-list.json:2:13: expected '{', '[', ']', string, or boolean
diff --git a/tests/qapi-schema/trailing-comma-list.err b/tests/qapi-schema/trailing-comma-list.err
index ad2f2d7c97..bb5f8c3c90 100644
--- a/tests/qapi-schema/trailing-comma-list.err
+++ b/tests/qapi-schema/trailing-comma-list.err
@@ -1 +1 @@
-trailing-comma-list.json:2:36: expected '{', '[', string, boolean or 'null'
+trailing-comma-list.json:2:36: expected '{', '[', string, or boolean
-- 
2.26.2

[PATCH 08/26] qapi/parser.py: Assert lexer value is a string

[John Snow]

The type checker can't constrain the token value to string in this case,
because it's only loosely correlated with the return token, which is
"stringly typed".

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 78355ca93f..6774b6c736 100644
--- a/scripts/qapi/parser.py
+++ b/scripts/qapi/parser.py
@@ -312,6 +312,7 @@ def _get_doc(self, info):
         cur_doc = QAPIDoc(info)
         self.accept(False)
         while self.tok == '#':
+            assert isinstance(self.val, str), "Expected str value"
             if self.val.startswith('##'):
                 # End of doc comment
                 if self.val != '##':
-- 
2.26.2

[PATCH 09/26] qapi/parser.py: assert get_expr returns object in outer loop

[John Snow]

get_expr can return many things, depending on where it is used. In the
outer parsing loop, we expect and require it to return an object.

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 6774b6c736..1bc33e85ea 100644
--- a/scripts/qapi/parser.py
+++ b/scripts/qapi/parser.py
@@ -94,6 +94,9 @@ def _parse(self):
                 continue
 
             expr = self.get_expr(False)
+            if not isinstance(expr, dict):
+                raise QAPISemError(info, "Expecting object statement")
+
             if 'include' in expr:
                 self.reject_expr_doc(cur_doc)
                 if len(expr) != 1:
-- 
2.26.2

[PATCH 10/26] qapi/parser.py: assert object keys are strings

[John Snow]

Since values can also be other data types, add an assertion to ensure
we're dealing with strings.

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 1bc33e85ea..756c904257 100644
--- a/scripts/qapi/parser.py
+++ b/scripts/qapi/parser.py
@@ -256,6 +256,8 @@ def get_members(self):
             raise self._parse_error("expected string or '}'")
         while True:
             key = self.val
+            assert isinstance(key, str), f"expected str, got {type(key)!s}"
+
             self.accept()
             if self.tok != ':':
                 raise self._parse_error("expected ':'")
-- 
2.26.2

[PATCH 11/26] qapi/parser.py: Convert several methods to @classmethod

[John Snow]

It's usually nicer to keep static methods as class methods -- this
allows them to call other class methods, to be subclassed and extended,
etc.

Meanwhile, any method that doesn't utilize `self` can be a class method.

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

diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
index 756c904257..75a693a9d7 100644
--- a/scripts/qapi/parser.py
+++ b/scripts/qapi/parser.py
@@ -139,15 +139,16 @@ def _parse(self):
     def _parse_error(self, msg: str) -> QAPIParseError:
         return QAPIParseError.make(self, msg)
 
-    @staticmethod
-    def reject_expr_doc(doc):
+    @classmethod
+    def reject_expr_doc(cls, doc):
         if doc and doc.symbol:
             raise QAPISemError(
                 doc.info,
                 "documentation for '%s' is not followed by the definition"
                 % doc.symbol)
 
-    def _include(self, include, info, incl_fname, previously_included):
+    @classmethod
+    def _include(cls, include, info, incl_fname, previously_included):
         incl_abs_fname = os.path.abspath(incl_fname)
         # catch inclusion cycle
         inf = info
@@ -162,7 +163,8 @@ def _include(self, include, info, incl_fname, previously_included):
 
         return QAPISchemaParser(incl_fname, previously_included, info)
 
-    def _pragma(self, name, value, info):
+    @classmethod
+    def _pragma(cls, name, value, info):
         if name == 'doc-required':
             if not isinstance(value, bool):
                 raise QAPISemError(info,
-- 
2.26.2

[PATCH 12/26] qapi/parser.py: add casts to pragma checks

[John Snow]

This kind of type checking at runtime is not something mypy can
introspect, so add a do-nothing cast to help mypy out.

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

diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
index 75a693a9d7..9a1007f779 100644
--- a/scripts/qapi/parser.py
+++ b/scripts/qapi/parser.py
@@ -17,7 +17,7 @@
 import os
 import re
 from collections import OrderedDict
-from typing import Type, TypeVar
+from typing import List, Type, TypeVar, cast
 
 from .error import QAPIError, QAPISourceError, QAPISemError
 from .source import QAPISourceInfo
@@ -176,14 +176,14 @@ def _pragma(cls, name, value, info):
                 raise QAPISemError(
                     info,
                     "pragma returns-whitelist must be a list of strings")
-            info.pragma.returns_whitelist = value
+            info.pragma.returns_whitelist = cast(List[str], value)
         elif name == 'name-case-whitelist':
             if (not isinstance(value, list)
                     or any([not isinstance(elt, str) for elt in value])):
                 raise QAPISemError(
                     info,
                     "pragma name-case-whitelist must be a list of strings")
-            info.pragma.name_case_whitelist = value
+            info.pragma.name_case_whitelist = cast(List[str], value)
         else:
             raise QAPISemError(info, "unknown pragma '%s'" % name)
 
-- 
2.26.2

[PATCH 13/26] qapi/parser.py: add type hint annotations

[John Snow]

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

Annotations for QAPIDoc are in a later commit.

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

diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
index 9a1007f779..d9aae4ddb7 100644
--- a/scripts/qapi/parser.py
+++ b/scripts/qapi/parser.py
@@ -14,18 +14,35 @@
 # This work is licensed under the terms of the GNU GPL, version 2.
 # See the COPYING file in the top-level directory.
 
+
+from collections import OrderedDict
 import os
 import re
-from collections import OrderedDict
-from typing import List, Type, TypeVar, cast
+from typing import (
+    Any,
+    Dict,
+    List,
+    Optional,
+    Set,
+    Type,
+    TypeVar,
+    Union,
+    cast,
+)
 
 from .error import QAPIError, QAPISourceError, QAPISemError
 from .source import QAPISourceInfo
 
 
+Expression = Dict[str, Any]
+_Value = Union[List[object], 'OrderedDict[str, object]', str, bool]
+# Necessary imprecision: mypy does not (yet?) support recursive types;
+# so we must stub out that recursion with 'object'.
+# Note, we do not support numerics or null in this parser.
+
+
 class QAPIParseError(QAPISourceError):
     """Error class for all QAPI schema parsing errors."""
-
     T = TypeVar('T', bound='QAPIParseError')
 
     @classmethod
@@ -45,22 +62,25 @@ class QAPIDocError(QAPIError):
 
 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))
 
         # Lexer state (see `accept` for details):
-        self.tok = None
+        self.tok: Optional[str] = None
         self.pos = 0
         self.cursor = 0
-        self.val = None
+        self.val: Optional[Union[bool, str]] = None
         self.info = QAPISourceInfo(self._fname, parent=incl_info)
         self.line_pos = 0
 
         # Parser output:
-        self.exprs = []
-        self.docs = []
+        self.exprs: List[Expression] = []
+        self.docs: List[QAPIDoc] = []
 
         # Showtime!
         try:
@@ -76,7 +96,7 @@ def __init__(self, fname, previously_included=None, incl_info=None):
             raise QAPIParseError(context, msg) from e
         self._parse()
 
-    def _parse(self):
+    def _parse(self) -> None:
         cur_doc = None
 
         # Prime the lexer:
@@ -140,7 +160,7 @@ def _parse_error(self, msg: str) -> QAPIParseError:
         return QAPIParseError.make(self, msg)
 
     @classmethod
-    def reject_expr_doc(cls, doc):
+    def reject_expr_doc(cls, doc: Optional['QAPIDoc']) -> None:
         if doc and doc.symbol:
             raise QAPISemError(
                 doc.info,
@@ -148,7 +168,12 @@ def reject_expr_doc(cls, doc):
                 % doc.symbol)
 
     @classmethod
-    def _include(cls, include, info, incl_fname, previously_included):
+    def _include(cls,
+                 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
@@ -164,7 +189,10 @@ def _include(cls, include, info, incl_fname, previously_included):
         return QAPISchemaParser(incl_fname, previously_included, info)
 
     @classmethod
-    def _pragma(cls, name, value, info):
+    def _pragma(cls,
+                name: str,
+                value: object,
+                info: QAPISourceInfo) -> None:
         if name == 'doc-required':
             if not isinstance(value, bool):
                 raise QAPISemError(info,
@@ -187,7 +215,7 @@ def _pragma(cls, name, value, info):
         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
@@ -249,8 +277,8 @@ def accept(self, skip_comment=True):
                                  self.src[self.cursor-1:])
                 raise self._parse_error("stray '%s'" % match.group(0))
 
-    def get_members(self):
-        expr = OrderedDict()
+    def get_members(self) -> 'OrderedDict[str, object]':
+        expr: 'OrderedDict[str, object]' = OrderedDict()
         if self.tok == '}':
             self.accept()
             return expr
@@ -276,8 +304,8 @@ def get_members(self):
             if self.tok != "'":
                 raise self._parse_error("expected string")
 
-    def get_values(self):
-        expr = []
+    def get_values(self) -> List[object]:
+        expr: List[object] = []
         if self.tok == ']':
             self.accept()
             return expr
@@ -293,7 +321,8 @@ def get_values(self):
                 raise self._parse_error("expected ',' or ']'")
             self.accept()
 
-    def get_expr(self, nested):
+    def get_expr(self, nested: bool = False) -> _Value:
+        expr: _Value
         if self.tok != '{' and not nested:
             raise self._parse_error("expected '{'")
         if self.tok == '{':
@@ -310,7 +339,7 @@ def get_expr(self, nested):
                 "expected '{', '[', string, or boolean")
         return expr
 
-    def _get_doc(self, info):
+    def _get_doc(self, info: QAPISourceInfo) -> List['QAPIDoc']:
         if self.val != '##':
             raise self._parse_error(
                 "junk after '##' at start of documentation comment")
@@ -342,7 +371,7 @@ def _get_doc(self, info):
 
         raise self._parse_error("documentation comment must end with '##'")
 
-    def get_doc(self, info):
+    def get_doc(self, info: QAPISourceInfo) -> List['QAPIDoc']:
         try:
             return self._get_doc(info)
         except QAPIDocError as err:
-- 
2.26.2

[PATCH 14/26] qapi/parser.py: add docstrings

[John Snow]

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

diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
index d9aae4ddb7..490436b48a 100644
--- a/scripts/qapi/parser.py
+++ b/scripts/qapi/parser.py
@@ -61,7 +61,15 @@ class QAPIDocError(QAPIError):
 
 
 class QAPISchemaParser:
+    """
+    Performs parsing of a QAPI schema source file.
 
+    :param fname: Path to the source file
+    :param previously_included: Set of absolute paths of previously included
+                                source files; these will not be parsed again.
+    :param incl_info: QAPISourceInfo for the parent document;
+                      Can be None for the parent document.
+    """
     def __init__(self,
                  fname: str,
                  previously_included: Optional[Set[str]] = None,
@@ -97,6 +105,10 @@ def __init__(self,
         self._parse()
 
     def _parse(self) -> None:
+        """
+        Parse the QAPI Schema Document.
+        Build self.exprs, self.docs
+        """
         cur_doc = None
 
         # Prime the lexer:
@@ -216,6 +228,32 @@ def _pragma(cls,
             raise QAPISemError(info, "unknown pragma '%s'" % name)
 
     def accept(self, skip_comment: bool = True) -> None:
+        """
+        Read the next lexeme.
+
+        - `tok` is the current lexeme/token type.
+          It will always be a single char in `"[]{},:'tf#"`.
+        - `pos` is the position of the first character in the lexeme.
+        - `cursor` is the position of the next character.
+        - `val` is the value of the lexeme.
+
+        Single-char lexemes:
+          LBRACE, RBRACE, COLON, COMMA, LSQB, RSQB:
+            `tok` holds the single-char value of the lexeme.
+
+        Multi-char lexemes:
+          COMMENT - `tok` is `'#'`.
+                    `val` is a string including all chars until end-of-line.
+                          (The '#' is excluded.)
+          STRING  - `tok` is `"'"`.
+                    `val` is the string, excluding the quotes.
+          TRUE    - `tok` is `"t"`. `val` is `True`.
+          FALSE   - `tok` is `"f"`. `val` is `False`.
+
+        NEWLINE and SPACE lexemes are consumed by the lexer directly.
+        `line_pos` and `info` are advanced when NEWLINE is encountered.
+        `tok` is set to `None` upon reaching EOF.
+        """
         while True:
             self.tok = self.src[self.cursor]
             self.pos = self.cursor
-- 
2.26.2

[PATCH 15/26] qapi/parser.py: add ParsedExpression type

[John Snow]

This is an immutable, named, typed tuple; it's nicer than arbitrary
dicts for passing data around when using strict typing.

Turn parser.exprs into a list of ParsedExpressions instead, and adjust
expr.py to match.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 scripts/qapi/expr.py   | 56 +++++++++++++++---------------------------
 scripts/qapi/parser.py | 32 +++++++++++++-----------
 scripts/qapi/schema.py |  6 ++---
 3 files changed, 41 insertions(+), 53 deletions(-)

diff --git a/scripts/qapi/expr.py b/scripts/qapi/expr.py
index cfd342aa04..f2059c505c 100644
--- a/scripts/qapi/expr.py
+++ b/scripts/qapi/expr.py
@@ -46,7 +46,7 @@
 
 from .common import c_name
 from .error import QAPISemError
-from .parser import QAPIDoc
+from .parser import ParsedExpression
 from .source import QAPISourceInfo
 
 
@@ -517,7 +517,7 @@ class ExpressionType(str, Enum):
 }
 
 
-def check_exprs(exprs: List[_JSObject]) -> List[_JSObject]:
+def check_exprs(exprs: List[ParsedExpression]) -> List[ParsedExpression]:
     """
     Validate and normalize a list of parsed QAPI schema expressions. [RW]
 
@@ -526,49 +526,33 @@ def check_exprs(exprs: List[_JSObject]) -> List[_JSObject]:
 
     :param exprs: The list of expressions to normalize/validate.
     """
-    for expr_elem in exprs:
-        # Expression
-        assert isinstance(expr_elem['expr'], dict)
-        expr: Expression = expr_elem['expr']
-        for key in expr.keys():
-            assert isinstance(key, str)
-
-        # QAPISourceInfo
-        assert isinstance(expr_elem['info'], QAPISourceInfo)
-        info: QAPISourceInfo = expr_elem['info']
-
-        # Optional[QAPIDoc]
-        tmp = expr_elem.get('doc')
-        assert tmp is None or isinstance(tmp, QAPIDoc)
-        doc: Optional[QAPIDoc] = tmp
-
+    for expr in exprs:
         for kind in ExpressionType:
-            if kind in expr:
+            if kind in expr.expr:
                 meta = kind
                 break
         else:
-            raise QAPISemError(info, "expression is missing metatype")
+            raise QAPISemError(expr.info, "expression is missing metatype")
 
         if meta == ExpressionType.INCLUDE:
             continue
 
-        name = cast(str, expr[meta])  # asserted right below:
-        check_name_is_str(name, info, "'%s'" % meta.value)
-        info.set_defn(meta.value, name)
-        check_defn_name_str(name, info, meta.value)
+        name = cast(str, expr.expr[meta])  # asserted right below:
+        check_name_is_str(name, expr.info, "'%s'" % meta.value)
+        expr.info.set_defn(meta.value, name)
+        check_defn_name_str(name, expr.info, meta.value)
 
-        if doc:
-            if doc.symbol != name:
-                raise QAPISemError(
-                    info, "documentation comment is for '%s'" % doc.symbol)
-            doc.check_expr(expr)
-        elif info.pragma.doc_required:
-            raise QAPISemError(info,
-                               "documentation comment required")
+        if expr.doc:
+            if expr.doc.symbol != name:
+                msg = f"documentation comment is for '{expr.doc.symbol}'"
+                raise QAPISemError(expr.info, msg)
+            expr.doc.check_expr(expr.expr)
+        elif expr.info.pragma.doc_required:
+            raise QAPISemError(expr.info, "documentation comment required")
 
-        _CHECK_FN[meta](expr, info)
-        check_if(expr, info, meta.value)
-        check_features(expr.get('features'), info)
-        check_flags(expr, info)
+        _CHECK_FN[meta](expr.expr, expr.info)
+        check_if(expr.expr, expr.info, meta.value)
+        check_features(expr.expr.get('features'), expr.info)
+        check_flags(expr.expr, expr.info)
 
     return exprs
diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
index 490436b48a..f65afa4eb2 100644
--- a/scripts/qapi/parser.py
+++ b/scripts/qapi/parser.py
@@ -19,9 +19,8 @@
 import os
 import re
 from typing import (
-    Any,
-    Dict,
     List,
+    NamedTuple,
     Optional,
     Set,
     Type,
@@ -34,13 +33,18 @@
 from .source import QAPISourceInfo
 
 
-Expression = Dict[str, Any]
 _Value = Union[List[object], 'OrderedDict[str, object]', str, bool]
 # Necessary imprecision: mypy does not (yet?) support recursive types;
 # so we must stub out that recursion with 'object'.
 # Note, we do not support numerics or null in this parser.
 
 
+class ParsedExpression(NamedTuple):
+    expr: 'OrderedDict[str, object]'
+    info: QAPISourceInfo
+    doc: Optional['QAPIDoc']
+
+
 class QAPIParseError(QAPISourceError):
     """Error class for all QAPI schema parsing errors."""
     T = TypeVar('T', bound='QAPIParseError')
@@ -87,7 +91,7 @@ def __init__(self,
         self.line_pos = 0
 
         # Parser output:
-        self.exprs: List[Expression] = []
+        self.exprs: List[ParsedExpression] = []
         self.docs: List[QAPIDoc] = []
 
         # Showtime!
@@ -139,8 +143,7 @@ def _parse(self) -> None:
                                        "value of 'include' must be a string")
                 incl_fname = os.path.join(os.path.dirname(self._fname),
                                           include)
-                self.exprs.append({'expr': {'include': incl_fname},
-                                   'info': info})
+                self._add_expr(OrderedDict({'include': incl_fname}), info)
                 exprs_include = self._include(include, info, incl_fname,
                                               self._included)
                 if exprs_include:
@@ -157,20 +160,21 @@ def _parse(self) -> None:
                 for name, value in pragma.items():
                     self._pragma(name, value, info)
             else:
-                expr_elem = {'expr': expr,
-                             'info': info}
-                if cur_doc:
-                    if not cur_doc.symbol:
-                        raise QAPISemError(
-                            cur_doc.info, "definition documentation required")
-                    expr_elem['doc'] = cur_doc
-                self.exprs.append(expr_elem)
+                if cur_doc and not cur_doc.symbol:
+                    raise QAPISemError(
+                        cur_doc.info, "definition documentation required")
+                self._add_expr(expr, info, cur_doc)
             cur_doc = None
         self.reject_expr_doc(cur_doc)
 
     def _parse_error(self, msg: str) -> QAPIParseError:
         return QAPIParseError.make(self, msg)
 
+    def _add_expr(self, expr: 'OrderedDict[str, object]',
+                  info: QAPISourceInfo,
+                  doc: Optional['QAPIDoc'] = None) -> None:
+        self.exprs.append(ParsedExpression(expr, info, doc))
+
     @classmethod
     def reject_expr_doc(cls, doc: Optional['QAPIDoc']) -> None:
         if doc and doc.symbol:
diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
index 121d8488d2..51af0449f5 100644
--- a/scripts/qapi/schema.py
+++ b/scripts/qapi/schema.py
@@ -1106,9 +1106,9 @@ def _def_event(self, expr, info, doc):
 
     def _def_exprs(self, exprs):
         for expr_elem in exprs:
-            expr = expr_elem['expr']
-            info = expr_elem['info']
-            doc = expr_elem.get('doc')
+            expr = expr_elem.expr
+            info = expr_elem.info
+            doc = expr_elem.doc
             if 'enum' in expr:
                 self._def_enum_type(expr, info, doc)
             elif 'struct' in expr:
-- 
2.26.2

[PATCH 16/26] qapi/pragma.py: Move QAPISchemaPragma into its own module

[John Snow]

Signed-off-by: John Snow <jsnow@redhat.com>
---
 scripts/qapi/pragma.py | 25 +++++++++++++++++++++++++
 scripts/qapi/source.py | 15 ++-------------
 2 files changed, 27 insertions(+), 13 deletions(-)
 create mode 100644 scripts/qapi/pragma.py

diff --git a/scripts/qapi/pragma.py b/scripts/qapi/pragma.py
new file mode 100644
index 0000000000..7f3db9ab87
--- /dev/null
+++ b/scripts/qapi/pragma.py
@@ -0,0 +1,25 @@
+#
+# QAPI pragma information
+#
+# Copyright (c) 2020 John Snow, for Red Hat Inc.
+#
+# Authors:
+#  John Snow <jsnow@redhat.com>
+#
+# This work is licensed under the terms of the GNU GPL, version 2.
+# See the COPYING file in the top-level directory.
+
+from typing import List
+
+
+class QAPISchemaPragma:
+    # Replace with @dataclass in Python 3.7+
+    # pylint: disable=too-few-public-methods
+
+    def __init__(self) -> None:
+        # Are documentation comments required?
+        self.doc_required = False
+        # Whitelist of commands allowed to return a non-dictionary
+        self.returns_whitelist: List[str] = []
+        # Whitelist of entities allowed to violate case conventions
+        self.name_case_whitelist: List[str] = []
diff --git a/scripts/qapi/source.py b/scripts/qapi/source.py
index d1de9e36b1..fe1424be03 100644
--- a/scripts/qapi/source.py
+++ b/scripts/qapi/source.py
@@ -11,20 +11,9 @@
 
 import copy
 import sys
-from typing import List, Optional, TypeVar
+from typing import Optional, TypeVar
 
-
-class QAPISchemaPragma:
-    # Replace with @dataclass in Python 3.7+
-    # pylint: disable=too-few-public-methods
-
-    def __init__(self) -> None:
-        # Are documentation comments required?
-        self.doc_required = False
-        # Whitelist of commands allowed to return a non-dictionary
-        self.returns_whitelist: List[str] = []
-        # Whitelist of entities allowed to violate case conventions
-        self.name_case_whitelist: List[str] = []
+from .pragma import QAPISchemaPragma
 
 
 class QAPISourceInfo:
-- 
2.26.2

[PATCH 17/26] qapi/pragma.py: Move pragma parsing out of parser.py

[John Snow]

parser.py is a JSON parser at heart and shouldn't necessarily understand
what it is parsing on a semantic level. Move pragma parsing to pragma.py,
and leave the parser a little more happily ignorant.

Note: the type annotation in error.py now creates a cyclic import, because
error -> source -> pragma -> error. Use the magical mypy constant TYPE_CHECKING
to avoid this cycle at runtime. pylint dislikes this cycle still, but it can
be safely ignored.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 scripts/qapi/error.py  |  8 +++---
 scripts/qapi/parser.py | 41 ++++---------------------------
 scripts/qapi/pragma.py | 55 +++++++++++++++++++++++++++++++++++++-----
 3 files changed, 59 insertions(+), 45 deletions(-)

diff --git a/scripts/qapi/error.py b/scripts/qapi/error.py
index ab6a0f6271..be5fd24218 100644
--- a/scripts/qapi/error.py
+++ b/scripts/qapi/error.py
@@ -11,9 +11,11 @@
 # This work is licensed under the terms of the GNU GPL, version 2.
 # See the COPYING file in the top-level directory.
 
-from typing import Optional
+from typing import Optional, TYPE_CHECKING
 
-from .source import QAPISourceInfo
+if TYPE_CHECKING:
+    # pylint: disable=cyclic-import
+    from .source import QAPISourceInfo
 
 
 class QAPIError(Exception):
@@ -23,7 +25,7 @@ class QAPIError(Exception):
 class QAPISourceError(QAPIError):
     """Error class for all exceptions identifying a source location."""
     def __init__(self,
-                 info: QAPISourceInfo,
+                 info: 'QAPISourceInfo',
                  msg: str,
                  col: Optional[int] = None):
         super().__init__()
diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
index f65afa4eb2..5b3a9b5da8 100644
--- a/scripts/qapi/parser.py
+++ b/scripts/qapi/parser.py
@@ -26,10 +26,10 @@
     Type,
     TypeVar,
     Union,
-    cast,
 )
 
 from .error import QAPIError, QAPISourceError, QAPISemError
+from .pragma import PragmaError
 from .source import QAPISourceInfo
 
 
@@ -151,14 +151,10 @@ def _parse(self) -> None:
                     self.docs.extend(exprs_include.docs)
             elif "pragma" in expr:
                 self.reject_expr_doc(cur_doc)
-                if len(expr) != 1:
-                    raise QAPISemError(info, "invalid 'pragma' directive")
-                pragma = expr['pragma']
-                if not isinstance(pragma, dict):
-                    raise QAPISemError(
-                        info, "value of 'pragma' must be an object")
-                for name, value in pragma.items():
-                    self._pragma(name, value, info)
+                try:
+                    info.pragma.parse(expr)
+                except PragmaError as err:
+                    raise QAPISemError(info, str(err)) from err
             else:
                 if cur_doc and not cur_doc.symbol:
                     raise QAPISemError(
@@ -204,33 +200,6 @@ def _include(cls,
 
         return QAPISchemaParser(incl_fname, previously_included, info)
 
-    @classmethod
-    def _pragma(cls,
-                name: str,
-                value: object,
-                info: QAPISourceInfo) -> None:
-        if name == 'doc-required':
-            if not isinstance(value, bool):
-                raise QAPISemError(info,
-                                   "pragma 'doc-required' must be boolean")
-            info.pragma.doc_required = value
-        elif name == 'returns-whitelist':
-            if (not isinstance(value, list)
-                    or any([not isinstance(elt, str) for elt in value])):
-                raise QAPISemError(
-                    info,
-                    "pragma returns-whitelist must be a list of strings")
-            info.pragma.returns_whitelist = cast(List[str], value)
-        elif name == 'name-case-whitelist':
-            if (not isinstance(value, list)
-                    or any([not isinstance(elt, str) for elt in value])):
-                raise QAPISemError(
-                    info,
-                    "pragma name-case-whitelist must be a list of strings")
-            info.pragma.name_case_whitelist = cast(List[str], value)
-        else:
-            raise QAPISemError(info, "unknown pragma '%s'" % name)
-
     def accept(self, skip_comment: bool = True) -> None:
         """
         Read the next lexeme.
diff --git a/scripts/qapi/pragma.py b/scripts/qapi/pragma.py
index 7f3db9ab87..03ba3cac90 100644
--- a/scripts/qapi/pragma.py
+++ b/scripts/qapi/pragma.py
@@ -9,17 +9,60 @@
 # This work is licensed under the terms of the GNU GPL, version 2.
 # See the COPYING file in the top-level directory.
 
-from typing import List
+from typing import Mapping, Sequence
+
+from .error import QAPIError
+
+
+class PragmaError(QAPIError):
+    """For errors relating to Pragma validation."""
 
 
 class QAPISchemaPragma:
-    # Replace with @dataclass in Python 3.7+
-    # pylint: disable=too-few-public-methods
-
     def __init__(self) -> None:
         # Are documentation comments required?
         self.doc_required = False
         # Whitelist of commands allowed to return a non-dictionary
-        self.returns_whitelist: List[str] = []
+        self.returns_whitelist: Sequence[str] = tuple()
         # Whitelist of entities allowed to violate case conventions
-        self.name_case_whitelist: List[str] = []
+        self.name_case_whitelist: Sequence[str] = tuple()
+
+    def _add_doc_required(self, value: object) -> None:
+        if not isinstance(value, bool):
+            raise PragmaError("pragma 'doc-required' must be boolean")
+        self.doc_required = value
+
+    def _add_returns_whitelist(self, value: object) -> None:
+        if (not isinstance(value, list)
+                or any([not isinstance(elt, str) for elt in value])):
+            raise PragmaError(
+                "pragma returns-whitelist must be a list of strings")
+        self.returns_whitelist = tuple(value)
+
+    def _add_name_case_whitelist(self, value: object) -> None:
+        if (not isinstance(value, list)
+                or any([not isinstance(elt, str) for elt in value])):
+            raise PragmaError(
+                "pragma name-case-whitelist must be a list of strings")
+        self.name_case_whitelist = tuple(value)
+
+    def add(self, name: str, value: object) -> None:
+        if name == 'doc-required':
+            self._add_doc_required(value)
+        elif name == 'returns-whitelist':
+            self._add_returns_whitelist(value)
+        elif name == 'name-case-whitelist':
+            self._add_name_case_whitelist(value)
+        else:
+            raise PragmaError(f"unknown pragma '{name}'")
+
+    def parse(self, expression: Mapping[str, object]) -> None:
+        if expression.keys() != {'pragma'}:
+            raise PragmaError("invalid 'pragma' directive")
+
+        body = expression['pragma']
+        if not isinstance(body, dict):
+            raise PragmaError("value of 'pragma' must be an object")
+
+        for name, value in body.items():
+            self.add(name, value)
-- 
2.26.2

[PATCH 18/26] qapi/parser.py: Modify _include() to use parser state

[John Snow]

It doesn't need to take quite so many arguments.

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

diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
index 5b3a9b5da8..77067b2f5d 100644
--- a/scripts/qapi/parser.py
+++ b/scripts/qapi/parser.py
@@ -144,8 +144,7 @@ def _parse(self) -> None:
                 incl_fname = os.path.join(os.path.dirname(self._fname),
                                           include)
                 self._add_expr(OrderedDict({'include': incl_fname}), info)
-                exprs_include = self._include(include, info, incl_fname,
-                                              self._included)
+                exprs_include = self._include(include, incl_fname)
                 if exprs_include:
                     self.exprs.extend(exprs_include.exprs)
                     self.docs.extend(exprs_include.docs)
@@ -179,26 +178,22 @@ def reject_expr_doc(cls, doc: Optional['QAPIDoc']) -> None:
                 "documentation for '%s' is not followed by the definition"
                 % doc.symbol)
 
-    @classmethod
-    def _include(cls,
-                 include: str,
-                 info: QAPISourceInfo,
-                 incl_fname: str,
-                 previously_included: Set[str]
-                 ) -> Optional['QAPISchemaParser']:
+    def _include(self, include: str,
+                 incl_fname: str) -> Optional['QAPISchemaParser']:
         incl_abs_fname = os.path.abspath(incl_fname)
+
         # catch inclusion cycle
-        inf = info
+        inf = self.info
         while inf:
             if incl_abs_fname == os.path.abspath(inf.fname):
-                raise QAPISemError(info, "inclusion loop for %s" % include)
+                raise QAPISemError(self.info, f"inclusion loop for {include}")
             inf = inf.parent
 
         # skip multiple include of the same file
-        if incl_abs_fname in previously_included:
+        if incl_abs_fname in self._included:
             return None
 
-        return QAPISchemaParser(incl_fname, previously_included, info)
+        return QAPISchemaParser(incl_fname, self._included, self.info)
 
     def accept(self, skip_comment: bool = True) -> None:
         """
-- 
2.26.2

[PATCH 19/26] qapi/parser.py: add parent argument

[John Snow]

Instead of passing previously_included and info separately, we can pass
the parent parser itself. This cuts down on the number of parameters to
pass when creating a parser; and makes it easier to add new shared data
members between parent and child.

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

diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
index 77067b2f5d..fa0ddad922 100644
--- a/scripts/qapi/parser.py
+++ b/scripts/qapi/parser.py
@@ -68,26 +68,23 @@ class QAPISchemaParser:
     """
     Performs parsing of a QAPI schema source file.
 
-    :param fname: Path to the source file
-    :param previously_included: Set of absolute paths of previously included
-                                source files; these will not be parsed again.
-    :param incl_info: QAPISourceInfo for the parent document;
-                      Can be None for the parent document.
+    :param fname:  Path to the source file
+    :param parent: Parent parser, if this is an included file.
     """
-    def __init__(self,
-                 fname: str,
-                 previously_included: Optional[Set[str]] = None,
-                 incl_info: Optional[QAPISourceInfo] = None):
+    def __init__(self, fname: str,
+                 parent: Optional['QAPISchemaParser'] = None):
         self._fname = fname
-        self._included = previously_included or set()
+        self._included: Set[str] = parent._included if parent else set()
         self._included.add(os.path.abspath(self._fname))
+        parent_info = parent.info if parent else None
 
         # Lexer state (see `accept` for details):
         self.tok: Optional[str] = None
         self.pos = 0
         self.cursor = 0
         self.val: Optional[Union[bool, str]] = None
-        self.info = QAPISourceInfo(self._fname, parent=incl_info)
+        self.info: QAPISourceInfo = QAPISourceInfo(self._fname,
+                                                   parent=parent_info)
         self.line_pos = 0
 
         # Parser output:
@@ -100,11 +97,11 @@ def __init__(self,
                 self.src = fp.read()
         except IOError as e:
             msg = "can't read {kind:s} file '{fname:s}': {errmsg:s}".format(
-                kind='include' if incl_info else 'schema',
+                kind='include' if parent else 'schema',
                 fname=self._fname,
                 errmsg=e.strerror
             )
-            context = incl_info or self.info
+            context = parent_info if parent_info else self.info
             raise QAPIParseError(context, msg) from e
         self._parse()
 
@@ -193,7 +190,7 @@ def _include(self, include: str,
         if incl_abs_fname in self._included:
             return None
 
-        return QAPISchemaParser(incl_fname, self._included, self.info)
+        return QAPISchemaParser(incl_fname, self)
 
     def accept(self, skip_comment: bool = True) -> None:
         """
-- 
2.26.2

[PATCH 20/26] qapi/parser.py: remove unused check_args_section arguments

[John Snow]

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

diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
index fa0ddad922..a3403d4017 100644
--- a/scripts/qapi/parser.py
+++ b/scripts/qapi/parser.py
@@ -650,7 +650,7 @@ def check_expr(self, expr):
 
     def check(self):
 
-        def check_args_section(args, info, what):
+        def check_args_section(args):
             bogus = [name for name, section in args.items()
                      if not section.member]
             if bogus:
@@ -661,5 +661,5 @@ def check_args_section(args, info, what):
                        "', '".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)
+        check_args_section(self.features)
-- 
2.26.2

[PATCH 21/26] qapi/parser.py: QAPIDoc: convert @staticmethod to @classmethod

[John Snow]

For consistency: replace @staticmethod with @classmethod.

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

diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
index a3403d4017..f5f40ffa16 100644
--- a/scripts/qapi/parser.py
+++ b/scripts/qapi/parser.py
@@ -465,8 +465,8 @@ def append(self, line):
     def end_comment(self):
         self._end_section()
 
-    @staticmethod
-    def _is_section_tag(name):
+    @classmethod
+    def _is_section_tag(cls, name):
         return name in ('Returns:', 'Since:',
                         # those are often singular or plural
                         'Note:', 'Notes:',
-- 
2.26.2

[PATCH 22/26] qapi/parser.py: add type hint annotations (QAPIDoc)

[John Snow]

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

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

diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
index f5f40ffa16..cdb64ffc22 100644
--- a/scripts/qapi/parser.py
+++ b/scripts/qapi/parser.py
@@ -23,6 +23,7 @@
     NamedTuple,
     Optional,
     Set,
+    TYPE_CHECKING,
     Type,
     TypeVar,
     Union,
@@ -32,6 +33,10 @@
 from .pragma import PragmaError
 from .source import QAPISourceInfo
 
+if TYPE_CHECKING:
+    # pylint: disable=cyclic-import
+    from .schema import QAPISchemaMember, QAPISchemaFeature
+
 
 _Value = Union[List[object], 'OrderedDict[str, object]', str, bool]
 # Necessary imprecision: mypy does not (yet?) support recursive types;
@@ -405,43 +410,43 @@ class QAPIDoc:
     """
 
     class Section:
-        def __init__(self, name=None):
+        def __init__(self, name: Optional[str] = None):
             # optional section name (argument/member or section name)
             self.name = name
             self.text = ''
 
-        def append(self, line):
+        def append(self, line: str) -> None:
             self.text += line.rstrip() + '\n'
 
     class ArgSection(Section):
-        def __init__(self, name):
+        def __init__(self, name: Optional[str] = None):
             super().__init__(name)
-            self.member = None
+            self.member: Optional['QAPISchemaMember'] = None
 
-        def connect(self, member):
+        def connect(self, member: 'QAPISchemaMember') -> None:
             self.member = member
 
-    def __init__(self, info):
+    def __init__(self, info: QAPISourceInfo):
         self.info = info
-        self.symbol = None
+        self.symbol: Optional[str] = None
         self.body = QAPIDoc.Section()
         # dict mapping parameter name to ArgSection
-        self.args = OrderedDict()
-        self.features = OrderedDict()
+        self.args: 'OrderedDict[str, QAPIDoc.ArgSection]' = OrderedDict()
+        self.features: 'OrderedDict[str, QAPIDoc.ArgSection]' = OrderedDict()
         # a list of Section
-        self.sections = []
+        self.sections: List[QAPIDoc.Section] = []
         # the current section
         self._section = self.body
         self._append_line = self._append_body_line
 
-    def has_section(self, name):
+    def has_section(self, name: str) -> bool:
         """Return True if we have a section with this name."""
         for i in self.sections:
             if i.name == name:
                 return True
         return False
 
-    def append(self, line):
+    def append(self, line: str) -> None:
         """
         Parse a comment line and add it to the documentation.
 
@@ -462,18 +467,18 @@ def append(self, line):
         line = line[1:]
         self._append_line(line)
 
-    def end_comment(self):
+    def end_comment(self) -> None:
         self._end_section()
 
     @classmethod
-    def _is_section_tag(cls, name):
+    def _is_section_tag(cls, 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.
 
@@ -513,7 +518,7 @@ def _append_body_line(self, line):
             # This is a free-form documentation block
             self._append_freeform(line.strip())
 
-    def _append_args_line(self, line):
+    def _append_args_line(self, line: str) -> None:
         """
         Process a line of documentation text in an argument section.
 
@@ -546,7 +551,7 @@ def _append_args_line(self, line):
 
         self._append_freeform(line.strip())
 
-    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(':'):
@@ -565,7 +570,7 @@ def _append_features_line(self, line):
 
         self._append_freeform(line.strip())
 
-    def _append_various_line(self, line):
+    def _append_various_line(self, line: str) -> None:
         """
         Process a line of documentation text in an additional section.
 
@@ -591,7 +596,10 @@ def _append_various_line(self, line):
 
         self._append_freeform(line)
 
-    def _start_symbol_section(self, symbols_dict, name):
+    def _start_symbol_section(
+            self,
+            symbols_dict: 'OrderedDict[str, QAPIDoc.ArgSection]',
+            name: str) -> None:
         # FIXME invalid names other than the empty string aren't flagged
         if not name:
             raise QAPIDocError("invalid parameter name")
@@ -602,20 +610,20 @@ def _start_symbol_section(self, symbols_dict, name):
         self._section = QAPIDoc.ArgSection(name)
         symbols_dict[name] = self._section
 
-    def _start_args_section(self, name):
+    def _start_args_section(self, name: str) -> None:
         self._start_symbol_section(self.args, name)
 
-    def _start_features_section(self, name):
+    def _start_features_section(self, name: str) -> None:
         self._start_symbol_section(self.features, name)
 
-    def _start_section(self, name=None):
+    def _start_section(self, name: Optional[str] = None) -> None:
         if name in ('Returns', 'Since') and self.has_section(name):
             raise QAPIDocError("duplicated '%s' section" % name)
         self._end_section()
         self._section = QAPIDoc.Section(name)
         self.sections.append(self._section)
 
-    def _end_section(self):
+    def _end_section(self) -> None:
         if self._section:
             text = self._section.text = self._section.text.strip()
             if self._section.name and (not text or text.isspace()):
@@ -623,34 +631,34 @@ def _end_section(self):
                     "empty doc section '%s'" % self._section.name)
             self._section = None
 
-    def _append_freeform(self, line):
+    def _append_freeform(self, line: str) -> None:
         match = re.match(r'(@\S+:)', line)
         if match:
             raise QAPIDocError("'%s' not allowed in free-form documentation"
                                % 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(member.name)
         self.args[member.name].connect(member)
 
-    def connect_feature(self, feature):
+    def connect_feature(self, feature: 'QAPISchemaFeature') -> None:
         if feature.name not in self.features:
             raise QAPISemError(feature.info,
                                "feature '%s' lacks documentation"
                                % feature.name)
         self.features[feature.name].connect(feature)
 
-    def check_expr(self, expr):
+    def check_expr(self, expr: 'OrderedDict[str, object]') -> 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_args_section(args):
+    def check(self) -> None:
+        def check_args_section(
+                args: 'OrderedDict[str, QAPIDoc.ArgSection]') -> None:
             bogus = [name for name, section in args.items()
                      if not section.member]
             if bogus:
-- 
2.26.2

[PATCH 23/26] qapi/parser.py: enable mypy checks

[John Snow]

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 4d341c6b2d..20ab509946 100644
--- a/scripts/qapi/mypy.ini
+++ b/scripts/qapi/mypy.ini
@@ -4,11 +4,6 @@ strict_optional = False
 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.26.2

[PATCH 24/26] qapi/parser.py: remove one and two-letter variables

[John Snow]

Standard pylint complaint: use more descriptibe variable names. OK,
fine.

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

diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
index cdb64ffc22..818f8bc580 100644
--- a/scripts/qapi/parser.py
+++ b/scripts/qapi/parser.py
@@ -57,8 +57,8 @@ class QAPIParseError(QAPISourceError):
     @classmethod
     def make(cls: Type[T], parser: 'QAPISchemaParser', msg: str) -> T:
         col = 1
-        for ch in parser.src[parser.line_pos:parser.pos]:
-            if ch == '\t':
+        for char in parser.src[parser.line_pos:parser.pos]:
+            if char == '\t':
                 col = (col + 7) % 8 + 1
             else:
                 col += 1
@@ -100,14 +100,16 @@ def __init__(self, fname: str,
         try:
             with open(self._fname, 'r', encoding='utf-8') as fp:
                 self.src = fp.read()
-        except IOError as e:
+        except IOError as err:
             msg = "can't read {kind:s} file '{fname:s}': {errmsg:s}".format(
                 kind='include' if parent else 'schema',
                 fname=self._fname,
-                errmsg=e.strerror
+                errmsg=err.strerror
             )
             context = parent_info if parent_info else self.info
-            raise QAPIParseError(context, msg) from e
+            raise QAPIParseError(context, msg) from err
+
+        # Showtime!
         self._parse()
 
     def _parse(self) -> None:
@@ -245,25 +247,25 @@ def accept(self, skip_comment: bool = True) -> None:
                 string = ''
                 esc = False
                 while True:
-                    ch = self.src[self.cursor]
+                    char = self.src[self.cursor]
                     self.cursor += 1
-                    if ch == '\n':
+                    if char == '\n':
                         raise self._parse_error("missing terminating \"'\"")
                     if esc:
                         # Note: we recognize only \\ because we have
                         # no use for funny characters in strings
-                        if ch != '\\':
-                            raise self._parse_error(f"unknown escape \\{ch}")
+                        if char != '\\':
+                            raise self._parse_error(f"unknown escape \\{char}")
                         esc = False
-                    elif ch == '\\':
+                    elif char == '\\':
                         esc = True
                         continue
-                    elif ch == "'":
+                    elif char == "'":
                         self.val = string
                         return
-                    if ord(ch) < 32 or ord(ch) >= 127:
+                    if ord(char) < 32 or ord(char) >= 127:
                         raise self._parse_error("funny character in string")
-                    string += ch
+                    string += char
             elif self.src.startswith('true', self.pos):
                 self.val = True
                 self.cursor += 3
-- 
2.26.2

[PATCH 25/26] qapi/parser.py: Silence too-few-public-methods warning

[John Snow]

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 818f8bc580..e3481b02f2 100644
--- a/scripts/qapi/parser.py
+++ b/scripts/qapi/parser.py
@@ -412,6 +412,8 @@ class QAPIDoc:
     """
 
     class Section:
+        # pylint: disable=too-few-public-methods
+
         def __init__(self, name: Optional[str] = None):
             # optional section name (argument/member or section name)
             self.name = name
-- 
2.26.2

[PATCH 26/26] qapi/parser.py: enable pylint checks

[John Snow]

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 88efbf71cb..5091a08f12 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.26.2

Re: [PATCH 00/26] qapi: static typing conversion, pt5

[John Snow]

On 9/22/20 6:34 PM, John Snow wrote:
> based-on: <20200922210101.4081073-1-jsnow@redhat.com>

^ Copy-paste malfunction

based-on: <20200922212115.4084301-1-jsnow@redhat.com>

>            [PATCH 0/6] qapi: static typing conversion, pt4
> 
> Hi, this series adds static type hints to the QAPI module.
> This is part five!
> 
> Part 5: https://gitlab.com/jsnow/qemu/-/tree/python-qapi-cleanup-pt5
> Everything: https://gitlab.com/jsnow/qemu/-/tree/python-qapi-cleanup-pt6
> 
> - Requires Python 3.6+
> - Requires mypy 0.770 or newer (for type analysis only)
> - Requires pylint 2.6.0 or newer (for lint checking only)
> 
> This part of the series focuses on just parser.py.
> 
> Type hints are added in patches that add *only* type hints and change no
> other behavior. Any necessary changes to behavior to accommodate typing
> are split out into their own tiny patches.
> 
> Every commit should pass with:
>   - flake8 qapi/
>   - pylint --rcfile=qapi/pylintrc qapi/
>   - mypy --config-file=qapi/mypy.ini qapi/
> 
> John Snow (26):
>    qapi/parser.py: refactor parsing routine into method
>    qapi/parser.py: group variable declarations in __init__
>    qapi/parser.py: use 'with' statement for opening files
>    qapi/source.py: Add default arguments to QAPISourceInfo
>    qapi/parser.py: start source info at line 0
>    qapi/parser.py: raise QAPIParseError during file opening
>    qapi/parser.py: fully remove 'null' constant
>    qapi/parser.py: Assert lexer value is a string
>    qapi/parser.py: assert get_expr returns object in outer loop
>    qapi/parser.py: assert object keys are strings
>    qapi/parser.py: Convert several methods to @classmethod
>    qapi/parser.py: add casts to pragma checks
>    qapi/parser.py: add type hint annotations
>    qapi/parser.py: add docstrings
>    qapi/parser.py: add ParsedExpression type
>    qapi/pragma.py: Move QAPISchemaPragma into its own module
>    qapi/pragma.py: Move pragma parsing out of parser.py
>    qapi/parser.py: Modify _include() to use parser state
>    qapi/parser.py: add parent argument
>    qapi/parser.py: remove unused check_args_section arguments
>    qapi/parser.py: QAPIDoc: convert @staticmethod to @classmethod
>    qapi/parser.py: add type hint annotations (QAPIDoc)
>    qapi/parser.py: enable mypy checks
>    qapi/parser.py: remove one and two-letter variables
>    qapi/parser.py: Silence too-few-public-methods warning
>    qapi/parser.py: enable pylint checks
> 
>   scripts/qapi/error.py                     |   8 +-
>   scripts/qapi/expr.py                      |  56 ++--
>   scripts/qapi/mypy.ini                     |   5 -
>   scripts/qapi/parser.py                    | 322 +++++++++++++---------
>   scripts/qapi/pragma.py                    |  68 +++++
>   scripts/qapi/pylintrc                     |   3 +-
>   scripts/qapi/schema.py                    |   6 +-
>   scripts/qapi/source.py                    |  22 +-
>   tests/qapi-schema/leading-comma-list.err  |   2 +-
>   tests/qapi-schema/trailing-comma-list.err |   2 +-
>   10 files changed, 301 insertions(+), 193 deletions(-)
>   create mode 100644 scripts/qapi/pragma.py
>