[PATCH 00/25] qapi: static typing conversion, pt6

[PATCH 00/25] qapi: static typing conversion, pt6

[John Snow]

based-on: <20200922223525.4085762-1-jsnow@redhat.com>
          [PATCH 00/26] qapi: static typing conversion, pt5

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

Part 6 (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 schema.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 (25):
  qapi/schema: add Visitable mixin
  qapi/schema.py: Move meta-type into class instances
  qapi/schema.py: add assert in stub methods
  qapi/schema.py: constrain QAPISchemaObjectType base type
  qapi/schema.py: constrain QAPISchemaObjectTypeMember arg_type type
  qapi/schema.py: constrain QAPISchemaEvent arg_type type
  qapi/schema.py: constrain tag_member type
  qapi/schema.py: Allow alternate_type to assert
  qapi/schema.py: remove superfluous assert
  qapi/schema.py: Add assertion to ifcond property
  qapi/schema.py: Constrain type of QAPISchemaObjectType members field
  qapi/schema.py: remove 'and' from non-bool rvalue expressions
  qapi/schema.py: Test type of self.ret_type instead of local temp
  qapi/schema.py: Assert variants of an object are also objects
  qapi/schema.py: add type hint annotations
  qapi/schema.py: enable checking
  qapi: Disable similarity checks in pylint entirely
  qapi/schema.py: Add pylint warning suppressions
  qapi/schema.py: Convert several methods to classmethods
  qapi/schema.py: Replace one-letter variable names
  qapi/schema.py: disable pylint line limit
  qapi/schema.py: Ignore unused argument for check()
  qapi/schema.py: enable pylint checks
  qapi/schema.py: Add module docstring
  qapi/schema.py: Use python3 style super()

 scripts/qapi/mypy.ini  |   6 -
 scripts/qapi/pylintrc  |   6 +-
 scripts/qapi/schema.py | 848 +++++++++++++++++++++++++++--------------
 3 files changed, 557 insertions(+), 303 deletions(-)

-- 
2.26.2

[PATCH 01/25] qapi/schema: add Visitable mixin

[John Snow]

Python doesn't have anything quite exactly like Traits, Interfaces, or
Mixins; but we can approximate it.

Add a 'Visitable' class that enforces a type signature for the visit()
method; this way, mypy is ensuring that even for otherwise unrelated
classes that do not inherit from a common base, this signature will
always be forced to be compatible.

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

diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
index 51af0449f5..55434f5c82 100644
--- a/scripts/qapi/schema.py
+++ b/scripts/qapi/schema.py
@@ -25,7 +25,13 @@
 from .parser import QAPISchemaParser
 
 
-class QAPISchemaEntity:
+class Visitable:
+    """Abstract duck that suggests a class is visitable."""
+    def visit(self, visitor: 'QAPISchemaVisitor') -> None:
+        raise NotImplementedError
+
+
+class QAPISchemaEntity(Visitable):
     meta: Optional[str] = None
 
     def __init__(self, name: str, info, doc, ifcond=None, features=None):
@@ -136,7 +142,7 @@ def visit_event(self, name, info, ifcond, features, arg_type, boxed):
         pass
 
 
-class QAPISchemaModule:
+class QAPISchemaModule(Visitable):
     def __init__(self, name):
         self.name = name
         self._entity_list = []
@@ -812,7 +818,7 @@ def visit(self, visitor):
             self.arg_type, self.boxed)
 
 
-class QAPISchema:
+class QAPISchema(Visitable):
     def __init__(self, fname):
         self.fname = fname
         parser = QAPISchemaParser(fname)
-- 
2.26.2

[PATCH 02/25] qapi/schema.py: Move meta-type into class instances

[John Snow]

We are using meta as a class variable, but union types use this as a
mutable value which changes the value for the class, not the instance.

Use the empty string as the default/empty value for ease of typing. It
is still false-ish, so it will work just fine.

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

diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
index 55434f5c82..0201b42095 100644
--- a/scripts/qapi/schema.py
+++ b/scripts/qapi/schema.py
@@ -17,7 +17,6 @@
 import os
 import re
 from collections import OrderedDict
-from typing import Optional
 
 from .common import c_name, POINTER_SUFFIX
 from .error import QAPISourceError, QAPISemError
@@ -32,8 +31,6 @@ def visit(self, visitor: 'QAPISchemaVisitor') -> None:
 
 
 class QAPISchemaEntity(Visitable):
-    meta: Optional[str] = None
-
     def __init__(self, name: str, info, doc, ifcond=None, features=None):
         assert name is None or isinstance(name, str)
         for f in features or []:
@@ -51,6 +48,15 @@ def __init__(self, name: str, info, doc, ifcond=None, features=None):
         self._ifcond = ifcond or []
         self.features = features or []
         self._checked = False
+        self._meta = ''
+
+    @property
+    def meta(self) -> str:
+        return self._meta
+
+    @meta.setter
+    def meta(self, value: str) -> None:
+        self._meta = value
 
     def c_name(self):
         return c_name(self.name)
@@ -212,8 +218,6 @@ def describe(self):
 
 
 class QAPISchemaBuiltinType(QAPISchemaType):
-    meta = 'built-in'
-
     def __init__(self, name, json_type, c_type):
         super().__init__(name, None, None)
         assert not c_type or isinstance(c_type, str)
@@ -221,6 +225,7 @@ def __init__(self, name, json_type, c_type):
                              'value')
         self._json_type_name = json_type
         self._c_type_name = c_type
+        self._meta = 'built-in'
 
     def c_name(self):
         return self.name
@@ -245,8 +250,6 @@ def visit(self, visitor):
 
 
 class QAPISchemaEnumType(QAPISchemaType):
-    meta = 'enum'
-
     def __init__(self, name, info, doc, ifcond, features, members, prefix):
         super().__init__(name, info, doc, ifcond, features)
         for m in members:
@@ -255,6 +258,7 @@ def __init__(self, name, info, doc, ifcond, features, members, prefix):
         assert prefix is None or isinstance(prefix, str)
         self.members = members
         self.prefix = prefix
+        self._meta = 'enum'
 
     def check(self, schema):
         super().check(schema)
@@ -289,13 +293,12 @@ def visit(self, visitor):
 
 
 class QAPISchemaArrayType(QAPISchemaType):
-    meta = 'array'
-
     def __init__(self, name, info, element_type):
         super().__init__(name, info, None)
         assert isinstance(element_type, str)
         self._element_type_name = element_type
         self.element_type = None
+        self._meta = 'array'
 
     def check(self, schema):
         super().check(schema)
@@ -344,7 +347,7 @@ def __init__(self, name, info, doc, ifcond, features,
         # flat union has base, variants, and no local_members
         # simple union has local_members, variants, and no base
         super().__init__(name, info, doc, ifcond, features)
-        self.meta = 'union' if variants else 'struct'
+        self._meta = 'union' if variants else 'struct'
         assert base is None or isinstance(base, str)
         for m in local_members:
             assert isinstance(m, QAPISchemaObjectTypeMember)
@@ -456,8 +459,6 @@ def visit(self, visitor):
 
 
 class QAPISchemaAlternateType(QAPISchemaType):
-    meta = 'alternate'
-
     def __init__(self, name, info, doc, ifcond, features, variants):
         super().__init__(name, info, doc, ifcond, features)
         assert isinstance(variants, QAPISchemaVariants)
@@ -465,6 +466,7 @@ def __init__(self, name, info, doc, ifcond, features, variants):
         variants.set_defined_in(name)
         variants.tag_member.set_defined_in(self.name)
         self.variants = variants
+        self._meta = 'alternate'
 
     def check(self, schema):
         super().check(schema)
@@ -716,8 +718,6 @@ def __init__(self, name, info, typ, ifcond=None):
 
 
 class QAPISchemaCommand(QAPISchemaEntity):
-    meta = 'command'
-
     def __init__(self, name, info, doc, ifcond, features,
                  arg_type, ret_type,
                  gen, success_response, boxed, allow_oob, allow_preconfig):
@@ -733,6 +733,7 @@ def __init__(self, name, info, doc, ifcond, features,
         self.boxed = boxed
         self.allow_oob = allow_oob
         self.allow_preconfig = allow_preconfig
+        self._meta = 'command'
 
     def check(self, schema):
         super().check(schema)
@@ -779,14 +780,13 @@ def visit(self, visitor):
 
 
 class QAPISchemaEvent(QAPISchemaEntity):
-    meta = 'event'
-
     def __init__(self, name, info, doc, ifcond, features, arg_type, boxed):
         super().__init__(name, info, doc, ifcond, features)
         assert not arg_type or isinstance(arg_type, str)
         self._arg_type_name = arg_type
         self.arg_type = None
         self.boxed = boxed
+        self._meta = 'event'
 
     def check(self, schema):
         super().check(schema)
-- 
2.26.2

[PATCH 03/25] qapi/schema.py: add assert in stub methods

[John Snow]

Instead of pass (an implicit return none), use raise NotImplementedError
to mark a function as abstract -- one that doesn't return. This allows
us to correctly type the stub.

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

diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
index 0201b42095..a53631e660 100644
--- a/scripts/qapi/schema.py
+++ b/scripts/qapi/schema.py
@@ -176,8 +176,8 @@ def visit(self, visitor):
 class QAPISchemaType(QAPISchemaEntity):
     # Return the C type for common use.
     # For the types we commonly box, this is a pointer type.
-    def c_type(self):
-        pass
+    def c_type(self) -> str:
+        raise NotImplementedError()
 
     # Return the C type to be used in a parameter list.
     def c_param_type(self):
@@ -187,8 +187,8 @@ def c_param_type(self):
     def c_unboxed_type(self):
         return self.c_type()
 
-    def json_type(self):
-        pass
+    def json_type(self) -> str:
+        raise NotImplementedError()
 
     def alternate_qtype(self):
         json2qtype = {
-- 
2.26.2

[PATCH 04/25] qapi/schema.py: constrain QAPISchemaObjectType base type

[John Snow]

Re-order check slightly so we can provide a stronger guarantee on the
typing of the base field.

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

diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
index a53631e660..3aa842be08 100644
--- a/scripts/qapi/schema.py
+++ b/scripts/qapi/schema.py
@@ -378,14 +378,14 @@ def check(self, schema):
 
         seen = OrderedDict()
         if self._base_name:
-            self.base = schema.resolve_type(self._base_name, self.info,
-                                            "'base'")
-            if (not isinstance(self.base, QAPISchemaObjectType)
-                    or self.base.variants):
+            base = schema.resolve_type(self._base_name, self.info, "'base'")
+            if (not isinstance(base, QAPISchemaObjectType)
+                    or base.variants):
                 raise QAPISemError(
                     self.info,
                     "'base' requires a struct type, %s isn't"
-                    % self.base.describe())
+                    % base.describe())
+            self.base = base
             self.base.check(schema)
             self.base.check_clash(self.info, seen)
         for m in self.local_members:
-- 
2.26.2

[PATCH 05/25] qapi/schema.py: constrain QAPISchemaObjectTypeMember arg_type type

[John Snow]

Re-order the check method slightly in order to provide stronger typing
for the arg_type field.

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

diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
index 3aa842be08..c9a62711c2 100644
--- a/scripts/qapi/schema.py
+++ b/scripts/qapi/schema.py
@@ -738,13 +738,14 @@ def __init__(self, name, info, doc, ifcond, features,
     def check(self, schema):
         super().check(schema)
         if self._arg_type_name:
-            self.arg_type = schema.resolve_type(
+            arg_type = schema.resolve_type(
                 self._arg_type_name, self.info, "command's 'data'")
-            if not isinstance(self.arg_type, QAPISchemaObjectType):
+            if not isinstance(arg_type, QAPISchemaObjectType):
                 raise QAPISemError(
                     self.info,
                     "command's 'data' cannot take %s"
-                    % self.arg_type.describe())
+                    % arg_type.describe())
+            self.arg_type = arg_type
             if self.arg_type.variants and not self.boxed:
                 raise QAPISemError(
                     self.info,
-- 
2.26.2

[PATCH 06/25] qapi/schema.py: constrain QAPISchemaEvent arg_type type

[John Snow]

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

diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
index c9a62711c2..aaf20f776b 100644
--- a/scripts/qapi/schema.py
+++ b/scripts/qapi/schema.py
@@ -792,13 +792,14 @@ def __init__(self, name, info, doc, ifcond, features, arg_type, boxed):
     def check(self, schema):
         super().check(schema)
         if self._arg_type_name:
-            self.arg_type = schema.resolve_type(
+            arg_type = schema.resolve_type(
                 self._arg_type_name, self.info, "event's 'data'")
-            if not isinstance(self.arg_type, QAPISchemaObjectType):
+            if not isinstance(arg_type, QAPISchemaObjectType):
                 raise QAPISemError(
                     self.info,
                     "event's 'data' cannot take %s"
-                    % self.arg_type.describe())
+                    % arg_type.describe())
+            self.arg_type = arg_type
             if self.arg_type.variants and not self.boxed:
                 raise QAPISemError(
                     self.info,
-- 
2.26.2

[PATCH 07/25] qapi/schema.py: constrain tag_member type

[John Snow]

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

diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
index aaf20f776b..15ff441660 100644
--- a/scripts/qapi/schema.py
+++ b/scripts/qapi/schema.py
@@ -546,15 +546,18 @@ def set_defined_in(self, name):
 
     def check(self, schema, seen):
         if not self.tag_member:  # flat union
-            self.tag_member = seen.get(c_name(self._tag_name))
+            tag_member = seen.get(c_name(self._tag_name))
             base = "'base'"
             # Pointing to the base type when not implicit would be
             # nice, but we don't know it here
-            if not self.tag_member or self._tag_name != self.tag_member.name:
+            if not tag_member or self._tag_name != tag_member.name:
                 raise QAPISemError(
                     self.info,
                     "discriminator '%s' is not a member of %s"
                     % (self._tag_name, base))
+
+            assert isinstance(tag_member, QAPISchemaObjectTypeMember)
+            self.tag_member = tag_member
             # Here we do:
             base_type = schema.lookup_type(self.tag_member.defined_in)
             assert base_type
-- 
2.26.2

[PATCH 08/25] qapi/schema.py: Allow alternate_type to assert

[John Snow]

It is generally nicer to just let things fail, because it makes the
static type hints less infected with Optional[T], where a future
programmer using the library has to wonder what that means.

Let errors be errors.

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

diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
index 15ff441660..a84d8549a4 100644
--- a/scripts/qapi/schema.py
+++ b/scripts/qapi/schema.py
@@ -199,7 +199,7 @@ def alternate_qtype(self):
             'boolean': 'QTYPE_QBOOL',
             'object':  'QTYPE_QDICT'
         }
-        return json2qtype.get(self.json_type())
+        return json2qtype[self.json_type()]
 
     def doc_type(self):
         if self.is_implicit():
@@ -480,8 +480,9 @@ def check(self, schema):
         types_seen = {}
         for v in self.variants.variants:
             v.check_clash(self.info, seen)
-            qtype = v.type.alternate_qtype()
-            if not qtype:
+            try:
+                qtype = v.type.alternate_qtype()
+            except KeyError:
                 raise QAPISemError(
                     self.info,
                     "%s cannot use %s"
-- 
2.26.2

[PATCH 09/25] qapi/schema.py: remove superfluous assert

[John Snow]

We'll actually get a better error message by just letting the dictionary
lookup fail.

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

diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
index a84d8549a4..b35f741c6f 100644
--- a/scripts/qapi/schema.py
+++ b/scripts/qapi/schema.py
@@ -892,7 +892,6 @@ def _make_module(self, fname):
 
     def module_by_fname(self, fname):
         name = self._module_name(fname)
-        assert name in self._module_dict
         return self._module_dict[name]
 
     def _def_include(self, expr, info, doc):
-- 
2.26.2

[PATCH 10/25] qapi/schema.py: Add assertion to ifcond property

[John Snow]

ifcond's initialization allows a fairly confusing type, but we want to
assert that after the QAPISchema is built, this always returns a
List[str]. Add an assertion to allow us to say that.

(Note: Technically, I only assert that it's a list -- but type hints
that will be added later will make it clear that the only possible list
we could have here is a list[str], and this assertion is sufficient to
prove the point to mypy.)

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

diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
index b35f741c6f..62b1a7e890 100644
--- a/scripts/qapi/schema.py
+++ b/scripts/qapi/schema.py
@@ -88,7 +88,7 @@ def set_module(self, schema):
 
     @property
     def ifcond(self):
-        assert self._checked
+        assert self._checked and isinstance(self._ifcond, list)
         return self._ifcond
 
     def is_implicit(self):
-- 
2.26.2

[PATCH 11/25] qapi/schema.py: Constrain type of QAPISchemaObjectType members field

[John Snow]

This must always be QAPISchemaObjectTypeMember, and we do check and
enforce this -- but because the seen dict is invariant and check methods
need to operate in terms of a more abstract type, we need to tell the
type system to believe us.

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

diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
index 62b1a7e890..57343a1002 100644
--- a/scripts/qapi/schema.py
+++ b/scripts/qapi/schema.py
@@ -17,6 +17,7 @@
 import os
 import re
 from collections import OrderedDict
+from typing import cast, List
 
 from .common import c_name, POINTER_SUFFIX
 from .error import QAPISourceError, QAPISemError
@@ -391,7 +392,10 @@ def check(self, schema):
         for m in self.local_members:
             m.check(schema)
             m.check_clash(self.info, seen)
-        members = seen.values()
+
+        # check_clash is abstract, but local_members is asserted to be
+        # Sequence[QAPISchemaObjectTypeMember]. Cast to the narrower type.
+        members = cast(List[QAPISchemaObjectTypeMember], list(seen.values()))
 
         if self.variants:
             self.variants.check(schema, seen)
-- 
2.26.2

[PATCH 12/25] qapi/schema.py: remove 'and' from non-bool rvalue expressions

[John Snow]

There's nothing inherently wrong with using 'and' in this manner, but
the type returned is more loose than you might expect. Any value can be
false-ish, so mypy assumes that the type of the expression must be the
Union of both types, because a type can always be false-ish.

To tighten the static analysis type, we have to use the slightly
clunkier but more formally correct ternary.

Fixes these errors:

qapi/schema.py:103: error: Argument 1 to "module_by_fname" of "QAPISchema" has
    incompatible type "Union[QAPISourceInfo, str]"; expected "Optional[str]"

qapi/schema.py:380: error: Argument 3 to "resolve_type" of "QAPISchema" has
    incompatible type "Union[QAPISourceInfo, str]";
    expected "Union[str, Callable[[Optional[QAPISourceInfo]], str], None]"

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

diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
index 57343a1002..943f234ee2 100644
--- a/scripts/qapi/schema.py
+++ b/scripts/qapi/schema.py
@@ -81,7 +81,7 @@ def check_doc(self):
 
     def _set_module(self, schema, info):
         assert self._checked
-        self._module = schema.module_by_fname(info and info.fname)
+        self._module = schema.module_by_fname(info.fname if info else None)
         self._module.add_entity(self)
 
     def set_module(self, schema):
@@ -305,7 +305,7 @@ def check(self, schema):
         super().check(schema)
         self.element_type = schema.resolve_type(
             self._element_type_name, self.info,
-            self.info and self.info.defn_meta)
+            self.info.defn_meta if self.info else None)
         assert not isinstance(self.element_type, QAPISchemaArrayType)
 
     def set_module(self, schema):
-- 
2.26.2

[PATCH 13/25] qapi/schema.py: Test type of self.ret_type instead of local temp

[John Snow]

This is obscure: If we test the type on a local copy instead of the
stored state AFTER the assignment, mypy does not constrain the type of
the copy. If we test on the stored state, it works out fine.

Corrects this warning:

qapi/schema.py:887: error: "QAPISchemaType" has no attribute "element_type"

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

diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
index 943f234ee2..09c7ceab41 100644
--- a/scripts/qapi/schema.py
+++ b/scripts/qapi/schema.py
@@ -764,7 +764,7 @@ def check(self, schema):
                 self._ret_type_name, self.info, "command's 'returns'")
             if self.name not in self.info.pragma.returns_whitelist:
                 typ = self.ret_type
-                if isinstance(typ, QAPISchemaArrayType):
+                if isinstance(self.ret_type, QAPISchemaArrayType):
                     typ = self.ret_type.element_type
                     assert typ
                 if not isinstance(typ, QAPISchemaObjectType):
-- 
2.26.2

[PATCH 14/25] qapi/schema.py: Assert variants of an object are also objects

[John Snow]

The 'seen' dictionaries are invariant types and require the most
abstracted type to maintain a consistent interface with other
check_clash implementations.

In this case, we happen to know (and require) that they will be object
types, so add a runtime assertion to constrain that type.

Corrects this warning:
qapi/schema.py:718: error: Item "QAPISchemaType" of "Optional[QAPISchemaType]"
    has no attribute "check_clash"

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

diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
index 09c7ceab41..b502eee9cb 100644
--- a/scripts/qapi/schema.py
+++ b/scripts/qapi/schema.py
@@ -620,6 +620,7 @@ def check_clash(self, info, seen):
         for v in self.variants:
             # Reset seen map for each variant, since qapi names from one
             # branch do not affect another branch
+            assert isinstance(v.type, QAPISchemaObjectType)
             v.type.check_clash(info, dict(seen))
 
 
-- 
2.26.2

[PATCH 15/25] qapi/schema.py: add type hint annotations

[John Snow]

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

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

diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
index b502eee9cb..8907bec0b5 100644
--- a/scripts/qapi/schema.py
+++ b/scripts/qapi/schema.py
@@ -14,15 +14,27 @@
 
 # TODO catching name collisions in generated code would be nice
 
-import os
-import re
 from collections import OrderedDict
-from typing import cast, List
+import re
+import os
+from typing import (
+    Any,
+    Callable,
+    Dict,
+    List,
+    Optional,
+    Type,
+    TypeVar,
+    Union,
+    cast,
+    overload,
+)
 
 from .common import c_name, POINTER_SUFFIX
 from .error import QAPISourceError, QAPISemError
 from .expr import check_exprs
-from .parser import QAPISchemaParser
+from .parser import QAPIDoc, QAPISchemaParser, ParsedExpression
+from .source import QAPISourceInfo
 
 
 class Visitable:
@@ -32,13 +44,18 @@ def visit(self, visitor: 'QAPISchemaVisitor') -> None:
 
 
 class QAPISchemaEntity(Visitable):
-    def __init__(self, name: str, info, doc, ifcond=None, features=None):
+    def __init__(self,
+                 name: str,
+                 info: Optional[QAPISourceInfo],
+                 doc: Optional[QAPIDoc],
+                 ifcond: Optional[Union[List[str], 'QAPISchemaType']] = None,
+                 features: Optional[List['QAPISchemaFeature']] = None):
         assert name is None or isinstance(name, str)
         for f in features or []:
             assert isinstance(f, QAPISchemaFeature)
             f.set_defined_in(name)
         self.name = name
-        self._module = None
+        self._module: Optional[QAPISchemaModule] = None
         # For explicitly defined entities, info points to the (explicit)
         # definition.  For builtins (and their arrays), info is None.
         # For implicitly defined entities, info points to a place that
@@ -59,105 +76,151 @@ def meta(self) -> str:
     def meta(self, value: str) -> None:
         self._meta = value
 
-    def c_name(self):
+    def c_name(self) -> str:
         return c_name(self.name)
 
-    def check(self, schema):
+    def check(self, schema: 'QAPISchema') -> None:
         assert not self._checked
-        seen = {}
+        seen: Dict[str, 'QAPISchemaMember'] = {}
         for f in self.features:
             f.check_clash(self.info, seen)
         self._checked = True
 
-    def connect_doc(self, doc=None):
+    def connect_doc(self, doc: Optional[QAPIDoc] = None) -> None:
         doc = doc or self.doc
         if doc:
             for f in self.features:
                 doc.connect_feature(f)
 
-    def check_doc(self):
+    def check_doc(self) -> None:
         if self.doc:
             self.doc.check()
 
-    def _set_module(self, schema, info):
+    def _set_module(self,
+                    schema: 'QAPISchema',
+                    info: Optional[QAPISourceInfo]) -> None:
         assert self._checked
         self._module = schema.module_by_fname(info.fname if info else None)
         self._module.add_entity(self)
 
-    def set_module(self, schema):
+    def set_module(self, schema: 'QAPISchema') -> None:
         self._set_module(schema, self.info)
 
     @property
-    def ifcond(self):
+    def ifcond(self) -> List[str]:
         assert self._checked and isinstance(self._ifcond, list)
         return self._ifcond
 
-    def is_implicit(self):
+    def is_implicit(self) -> bool:
         return not self.info
 
-    def visit(self, visitor):
+    def visit(self, visitor: 'QAPISchemaVisitor') -> None:
         assert self._checked
 
-    def describe(self):
+    def describe(self) -> str:
         assert self.meta
         return "%s '%s'" % (self.meta, self.name)
 
 
 class QAPISchemaVisitor:
-    def visit_begin(self, schema):
+    def visit_begin(self, schema: 'QAPISchema') -> None:
         pass
 
-    def visit_end(self):
+    def visit_end(self) -> None:
         pass
 
-    def visit_module(self, name):
+    def visit_module(self, name: Optional[str]) -> None:
         pass
 
-    def visit_needed(self, entity):
+    def visit_needed(self, entity: QAPISchemaEntity) -> bool:
         # Default to visiting everything
         return True
 
-    def visit_include(self, name, info):
+    def visit_include(self, name: str, info: QAPISourceInfo) -> None:
         pass
 
-    def visit_builtin_type(self, name, info, json_type):
+    def visit_builtin_type(self, name: str,
+                           info: Optional[QAPISourceInfo],
+                           json_type: str) -> None:
         pass
 
-    def visit_enum_type(self, name, info, ifcond, features, members, prefix):
+    def visit_enum_type(self,
+                        name: str,
+                        info: Optional[QAPISourceInfo],
+                        ifcond: List[str],
+                        features: List['QAPISchemaFeature'],
+                        members: List['QAPISchemaEnumMember'],
+                        prefix: Optional[str]) -> None:
         pass
 
-    def visit_array_type(self, name, info, ifcond, element_type):
+    def visit_array_type(self,
+                         name: str,
+                         info: Optional[QAPISourceInfo],
+                         ifcond: List[str],
+                         element_type: 'QAPISchemaType') -> None:
         pass
 
-    def visit_object_type(self, name, info, ifcond, features,
-                          base, members, variants):
+    def visit_object_type(self,
+                          name: str,
+                          info: Optional[QAPISourceInfo],
+                          ifcond: List[str],
+                          features: List['QAPISchemaFeature'],
+                          base: Optional['QAPISchemaObjectType'],
+                          members: List['QAPISchemaObjectTypeMember'],
+                          variants: Optional['QAPISchemaVariants']) -> None:
         pass
 
-    def visit_object_type_flat(self, name, info, ifcond, features,
-                               members, variants):
+    def visit_object_type_flat(self,
+                               name: str,
+                               info: Optional[QAPISourceInfo],
+                               ifcond: List[str],
+                               features: List['QAPISchemaFeature'],
+                               members: List['QAPISchemaObjectTypeMember'],
+                               variants: Optional['QAPISchemaVariants'],
+                               ) -> None:
         pass
 
-    def visit_alternate_type(self, name, info, ifcond, features, variants):
+    def visit_alternate_type(self,
+                             name: str,
+                             info: QAPISourceInfo,
+                             ifcond: List[str],
+                             features: List['QAPISchemaFeature'],
+                             variants: 'QAPISchemaVariants') -> None:
         pass
 
-    def visit_command(self, name, info, ifcond, features,
-                      arg_type, ret_type, gen, success_response, boxed,
-                      allow_oob, allow_preconfig):
+    def visit_command(self,
+                      name: str,
+                      info: QAPISourceInfo,
+                      ifcond: List[str],
+                      features: List['QAPISchemaFeature'],
+                      arg_type: 'QAPISchemaObjectType',
+                      ret_type: Optional['QAPISchemaType'],
+                      gen: bool,
+                      success_response: bool,
+                      boxed: bool,
+                      allow_oob: bool,
+                      allow_preconfig: bool) -> None:
         pass
 
-    def visit_event(self, name, info, ifcond, features, arg_type, boxed):
+    def visit_event(self,
+                    name: str,
+                    info: QAPISourceInfo,
+                    ifcond: List[str],
+                    features: List['QAPISchemaFeature'],
+                    arg_type: 'QAPISchemaObjectType',
+                    boxed: bool) -> None:
         pass
 
 
 class QAPISchemaModule(Visitable):
-    def __init__(self, name):
+    def __init__(self, name: Optional[str]):
         self.name = name
-        self._entity_list = []
+        self._entity_list: List[QAPISchemaEntity] = []
 
-    def add_entity(self, ent):
+    def add_entity(self, ent: QAPISchemaEntity) -> None:
         self._entity_list.append(ent)
 
-    def visit(self, visitor):
+    def visit(self, visitor: QAPISchemaVisitor) -> None:
         visitor.visit_module(self.name)
         for entity in self._entity_list:
             if visitor.visit_needed(entity):
@@ -165,11 +228,11 @@ def visit(self, visitor):
 
 
 class QAPISchemaInclude(QAPISchemaEntity):
-    def __init__(self, sub_module, info):
+    def __init__(self, sub_module: QAPISchemaModule, info: QAPISourceInfo):
         super().__init__(None, info, None)
         self._sub_module = sub_module
 
-    def visit(self, visitor):
+    def visit(self, visitor: QAPISchemaVisitor) -> None:
         super().visit(visitor)
         visitor.visit_include(self._sub_module.name, self.info)
 
@@ -181,17 +244,17 @@ def c_type(self) -> str:
         raise NotImplementedError()
 
     # Return the C type to be used in a parameter list.
-    def c_param_type(self):
+    def c_param_type(self) -> str:
         return self.c_type()
 
     # Return the C type to be used where we suppress boxing.
-    def c_unboxed_type(self):
+    def c_unboxed_type(self) -> str:
         return self.c_type()
 
     def json_type(self) -> str:
         raise NotImplementedError()
 
-    def alternate_qtype(self):
+    def alternate_qtype(self) -> str:
         json2qtype = {
             'null':    'QTYPE_QNULL',
             'string':  'QTYPE_QSTRING',
@@ -202,24 +265,24 @@ def alternate_qtype(self):
         }
         return json2qtype[self.json_type()]
 
-    def doc_type(self):
+    def doc_type(self) -> Optional[str]:
         if self.is_implicit():
             return None
         return self.name
 
-    def check(self, schema):
+    def check(self, schema: 'QAPISchema') -> None:
         QAPISchemaEntity.check(self, schema)
         if 'deprecated' in [f.name for f in self.features]:
             raise QAPISemError(
                 self.info, "feature 'deprecated' is not supported for types")
 
-    def describe(self):
+    def describe(self) -> str:
         assert self.meta
         return "%s type '%s'" % (self.meta, self.name)
 
 
 class QAPISchemaBuiltinType(QAPISchemaType):
-    def __init__(self, name, json_type, c_type):
+    def __init__(self, name: str, json_type: str, c_type: str):
         super().__init__(name, None, None)
         assert not c_type or isinstance(c_type, str)
         assert json_type in ('string', 'number', 'int', 'boolean', 'null',
@@ -228,30 +291,37 @@ def __init__(self, name, json_type, c_type):
         self._c_type_name = c_type
         self._meta = 'built-in'
 
-    def c_name(self):
+    def c_name(self) -> str:
         return self.name
 
-    def c_type(self):
+    def c_type(self) -> str:
         return self._c_type_name
 
-    def c_param_type(self):
+    def c_param_type(self) -> str:
         if self.name == 'str':
             return 'const ' + self._c_type_name
         return self._c_type_name
 
-    def json_type(self):
+    def json_type(self) -> str:
         return self._json_type_name
 
-    def doc_type(self):
+    def doc_type(self) -> str:
         return self.json_type()
 
-    def visit(self, visitor):
+    def visit(self, visitor: QAPISchemaVisitor) -> None:
         super().visit(visitor)
         visitor.visit_builtin_type(self.name, self.info, self.json_type())
 
 
 class QAPISchemaEnumType(QAPISchemaType):
-    def __init__(self, name, info, doc, ifcond, features, members, prefix):
+    def __init__(self,
+                 name: str,
+                 info: Optional[QAPISourceInfo],
+                 doc: Optional[QAPIDoc],
+                 ifcond: Optional[List[str]],
+                 features: Optional[List['QAPISchemaFeature']],
+                 members: List['QAPISchemaEnumMember'],
+                 prefix: Optional[str]):
         super().__init__(name, info, doc, ifcond, features)
         for m in members:
             assert isinstance(m, QAPISchemaEnumMember)
@@ -261,32 +331,32 @@ def __init__(self, name, info, doc, ifcond, features, members, prefix):
         self.prefix = prefix
         self._meta = 'enum'
 
-    def check(self, schema):
+    def check(self, schema: 'QAPISchema') -> None:
         super().check(schema)
-        seen = {}
+        seen: Dict[str, 'QAPISchemaMember'] = {}
         for m in self.members:
             m.check_clash(self.info, seen)
 
-    def connect_doc(self, doc=None):
+    def connect_doc(self, doc: Optional[QAPIDoc] = None) -> None:
         super().connect_doc(doc)
         doc = doc or self.doc
         for m in self.members:
             m.connect_doc(doc)
 
-    def is_implicit(self):
+    def is_implicit(self) -> bool:
         # See QAPISchema._make_implicit_enum_type() and ._def_predefineds()
         return self.name.endswith('Kind') or self.name == 'QType'
 
-    def c_type(self):
+    def c_type(self) -> str:
         return c_name(self.name)
 
-    def member_names(self):
+    def member_names(self) -> List[str]:
         return [m.name for m in self.members]
 
-    def json_type(self):
+    def json_type(self) -> str:
         return 'string'
 
-    def visit(self, visitor):
+    def visit(self, visitor: QAPISchemaVisitor) -> None:
         super().visit(visitor)
         visitor.visit_enum_type(
             self.name, self.info, self.ifcond, self.features,
@@ -294,56 +364,65 @@ def visit(self, visitor):
 
 
 class QAPISchemaArrayType(QAPISchemaType):
-    def __init__(self, name, info, element_type):
+    def __init__(self, name: str,
+                 info: Optional[QAPISourceInfo],
+                 element_type: str):
         super().__init__(name, info, None)
         assert isinstance(element_type, str)
         self._element_type_name = element_type
-        self.element_type = None
+        self.element_type: Optional[QAPISchemaType] = None
         self._meta = 'array'
 
-    def check(self, schema):
+    def check(self, schema: 'QAPISchema') -> None:
         super().check(schema)
         self.element_type = schema.resolve_type(
             self._element_type_name, self.info,
             self.info.defn_meta if self.info else None)
         assert not isinstance(self.element_type, QAPISchemaArrayType)
 
-    def set_module(self, schema):
+    def set_module(self, schema: 'QAPISchema') -> None:
         self._set_module(schema, self.element_type.info)
 
     @property
-    def ifcond(self):
+    def ifcond(self) -> List[str]:
         assert self._checked
         return self.element_type.ifcond
 
-    def is_implicit(self):
+    def is_implicit(self) -> bool:
         return True
 
-    def c_type(self):
+    def c_type(self) -> str:
         return c_name(self.name) + POINTER_SUFFIX
 
-    def json_type(self):
+    def json_type(self) -> str:
         return 'array'
 
-    def doc_type(self):
+    def doc_type(self) -> Optional[str]:
         elt_doc_type = self.element_type.doc_type()
         if not elt_doc_type:
             return None
         return 'array of ' + elt_doc_type
 
-    def visit(self, visitor):
+    def visit(self, visitor: QAPISchemaVisitor) -> None:
         super().visit(visitor)
         visitor.visit_array_type(self.name, self.info, self.ifcond,
                                  self.element_type)
 
-    def describe(self):
+    def describe(self) -> str:
         assert self.meta
         return "%s type ['%s']" % (self.meta, self._element_type_name)
 
 
 class QAPISchemaObjectType(QAPISchemaType):
-    def __init__(self, name, info, doc, ifcond, features,
-                 base, local_members, variants):
+    def __init__(self,
+                 name: str,
+                 info: Optional[QAPISourceInfo],
+                 doc: Optional[QAPIDoc],
+                 ifcond: Optional['QAPISchemaType'],
+                 features: Optional[List['QAPISchemaFeature']],
+                 base: Optional[str],
+                 local_members: List['QAPISchemaObjectTypeMember'],
+                 variants: Optional['QAPISchemaVariants']):
         # struct has local_members, optional base, and no variants
         # flat union has base, variants, and no local_members
         # simple union has local_members, variants, and no base
@@ -357,12 +436,12 @@ def __init__(self, name, info, doc, ifcond, features,
             assert isinstance(variants, QAPISchemaVariants)
             variants.set_defined_in(name)
         self._base_name = base
-        self.base = None
+        self.base: Optional[QAPISchemaObjectType] = None
         self.local_members = local_members
         self.variants = variants
-        self.members = None
+        self.members: Optional[List[QAPISchemaObjectTypeMember]] = None
 
-    def check(self, schema):
+    def check(self, schema: 'QAPISchema') -> None:
         # This calls another type T's .check() exactly when the C
         # struct emitted by gen_object() contains that T's C struct
         # (pointers don't count).
@@ -377,7 +456,7 @@ def check(self, schema):
         super().check(schema)
         assert self._checked and self.members is None
 
-        seen = OrderedDict()
+        seen: Dict[str, 'QAPISchemaMember'] = OrderedDict()
         if self._base_name:
             base = schema.resolve_type(self._base_name, self.info, "'base'")
             if (not isinstance(base, QAPISchemaObjectType)
@@ -406,13 +485,15 @@ def check(self, schema):
     # Check that the members of this type do not cause duplicate JSON members,
     # and update seen to track the members seen so far. Report any errors
     # on behalf of info, which is not necessarily self.info
-    def check_clash(self, info, seen):
+    def check_clash(self,
+                    info: QAPISourceInfo,
+                    seen: Dict[str, 'QAPISchemaMember']) -> None:
         assert self._checked
         assert not self.variants       # not implemented
         for m in self.members:
             m.check_clash(info, seen)
 
-    def connect_doc(self, doc=None):
+    def connect_doc(self, doc: Optional[QAPIDoc] = None) -> None:
         super().connect_doc(doc)
         doc = doc or self.doc
         if self.base and self.base.is_implicit():
@@ -421,7 +502,7 @@ def connect_doc(self, doc=None):
             m.connect_doc(doc)
 
     @property
-    def ifcond(self):
+    def ifcond(self) -> List[str]:
         assert self._checked
         if isinstance(self._ifcond, QAPISchemaType):
             # Simple union wrapper type inherits from wrapped type;
@@ -429,30 +510,30 @@ def ifcond(self):
             return self._ifcond.ifcond
         return self._ifcond
 
-    def is_implicit(self):
+    def is_implicit(self) -> bool:
         # See QAPISchema._make_implicit_object_type(), as well as
         # _def_predefineds()
         return self.name.startswith('q_')
 
-    def is_empty(self):
+    def is_empty(self) -> bool:
         assert self.members is not None
         return not self.members and not self.variants
 
-    def c_name(self):
+    def c_name(self) -> str:
         assert self.name != 'q_empty'
         return super().c_name()
 
-    def c_type(self):
+    def c_type(self) -> str:
         assert not self.is_implicit()
         return c_name(self.name) + POINTER_SUFFIX
 
-    def c_unboxed_type(self):
+    def c_unboxed_type(self) -> str:
         return c_name(self.name)
 
-    def json_type(self):
+    def json_type(self) -> str:
         return 'object'
 
-    def visit(self, visitor):
+    def visit(self, visitor: QAPISchemaVisitor) -> None:
         super().visit(visitor)
         visitor.visit_object_type(
             self.name, self.info, self.ifcond, self.features,
@@ -463,7 +544,13 @@ def visit(self, visitor):
 
 
 class QAPISchemaAlternateType(QAPISchemaType):
-    def __init__(self, name, info, doc, ifcond, features, variants):
+    def __init__(self,
+                 name: str,
+                 info: QAPISourceInfo,
+                 doc: QAPIDoc,
+                 ifcond: Optional[List[str]],
+                 features: List['QAPISchemaFeature'],
+                 variants: 'QAPISchemaVariants'):
         super().__init__(name, info, doc, ifcond, features)
         assert isinstance(variants, QAPISchemaVariants)
         assert variants.tag_member
@@ -472,7 +559,7 @@ def __init__(self, name, info, doc, ifcond, features, variants):
         self.variants = variants
         self._meta = 'alternate'
 
-    def check(self, schema):
+    def check(self, schema: 'QAPISchema') -> None:
         super().check(schema)
         self.variants.tag_member.check(schema)
         # Not calling self.variants.check_clash(), because there's nothing
@@ -480,8 +567,8 @@ def check(self, schema):
         self.variants.check(schema, {})
         # Alternate branch names have no relation to the tag enum values;
         # so we have to check for potential name collisions ourselves.
-        seen = {}
-        types_seen = {}
+        seen: Dict[str, QAPISchemaMember] = {}
+        types_seen: Dict[str, str] = {}
         for v in self.variants.variants:
             v.check_clash(self.info, seen)
             try:
@@ -511,26 +598,30 @@ def check(self, schema):
                         % (v.describe(self.info), types_seen[qt]))
                 types_seen[qt] = v.name
 
-    def connect_doc(self, doc=None):
+    def connect_doc(self, doc: Optional[QAPIDoc] = None) -> None:
         super().connect_doc(doc)
         doc = doc or self.doc
         for v in self.variants.variants:
             v.connect_doc(doc)
 
-    def c_type(self):
+    def c_type(self) -> str:
         return c_name(self.name) + POINTER_SUFFIX
 
-    def json_type(self):
+    def json_type(self) -> str:
         return 'value'
 
-    def visit(self, visitor):
+    def visit(self, visitor: QAPISchemaVisitor) -> None:
         super().visit(visitor)
         visitor.visit_alternate_type(
             self.name, self.info, self.ifcond, self.features, self.variants)
 
 
 class QAPISchemaVariants:
-    def __init__(self, tag_name, info, tag_member, variants):
+    def __init__(self,
+                 tag_name: Optional[str],
+                 info: QAPISourceInfo,
+                 tag_member: Optional['QAPISchemaObjectTypeMember'],
+                 variants: List['QAPISchemaVariant']):
         # Flat unions pass tag_name but not tag_member.
         # Simple unions and alternates pass tag_member but not tag_name.
         # After check(), tag_member is always set, and tag_name remains
@@ -545,11 +636,13 @@ def __init__(self, tag_name, info, tag_member, variants):
         self.tag_member = tag_member
         self.variants = variants
 
-    def set_defined_in(self, name):
+    def set_defined_in(self, name: str) -> None:
         for v in self.variants:
             v.set_defined_in(name)
 
-    def check(self, schema, seen):
+    def check(self,
+              schema: 'QAPISchema',
+              seen: Dict[str, 'QAPISchemaMember']) -> None:
         if not self.tag_member:  # flat union
             tag_member = seen.get(c_name(self._tag_name))
             base = "'base'"
@@ -616,7 +709,9 @@ def check(self, schema, seen):
                         % (v.describe(self.info), v.type.describe()))
                 v.type.check(schema)
 
-    def check_clash(self, info, seen):
+    def check_clash(self,
+                    info: QAPISourceInfo,
+                    seen: Dict[str, 'QAPISchemaMember']) -> None:
         for v in self.variants:
             # Reset seen map for each variant, since qapi names from one
             # branch do not affect another branch
@@ -628,18 +723,22 @@ class QAPISchemaMember:
     """ Represents object members, enum members and features """
     role = 'member'
 
-    def __init__(self, name, info, ifcond=None):
+    def __init__(self, name: str,
+                 info: Optional[QAPISourceInfo],
+                 ifcond: Optional[List[str]] = None):
         assert isinstance(name, str)
         self.name = name
         self.info = info
         self.ifcond = ifcond or []
-        self.defined_in = None
+        self.defined_in: Optional[str] = None
 
-    def set_defined_in(self, name):
+    def set_defined_in(self, name: str) -> None:
         assert not self.defined_in
         self.defined_in = name
 
-    def check_clash(self, info, seen):
+    def check_clash(self,
+                    info: Optional[QAPISourceInfo],
+                    seen: Dict[str, 'QAPISchemaMember']) -> None:
         cname = c_name(self.name)
         if cname in seen:
             raise QAPISemError(
@@ -648,11 +747,11 @@ def check_clash(self, info, seen):
                 % (self.describe(info), seen[cname].describe(info)))
         seen[cname] = self
 
-    def connect_doc(self, doc):
+    def connect_doc(self, doc: Optional[QAPIDoc]) -> None:
         if doc:
             doc.connect_member(self)
 
-    def describe(self, info):
+    def describe(self, info: QAPISourceInfo) -> str:
         role = self.role
         defined_in = self.defined_in
         assert defined_in
@@ -692,7 +791,13 @@ class QAPISchemaFeature(QAPISchemaMember):
 
 
 class QAPISchemaObjectTypeMember(QAPISchemaMember):
-    def __init__(self, name, info, typ, optional, ifcond=None, features=None):
+    def __init__(self,
+                 name: str,
+                 info: QAPISourceInfo,
+                 typ: str,
+                 optional: bool,
+                 ifcond: Optional[List[str]] = None,
+                 features: Optional[List[QAPISchemaFeature]] = None):
         super().__init__(name, info, ifcond)
         assert isinstance(typ, str)
         assert isinstance(optional, bool)
@@ -700,19 +805,19 @@ def __init__(self, name, info, typ, optional, ifcond=None, features=None):
             assert isinstance(f, QAPISchemaFeature)
             f.set_defined_in(name)
         self._type_name = typ
-        self.type = None
+        self.type: Optional[QAPISchemaType] = None
         self.optional = optional
         self.features = features or []
 
-    def check(self, schema):
+    def check(self, schema: 'QAPISchema') -> None:
         assert self.defined_in
         self.type = schema.resolve_type(self._type_name, self.info,
                                         self.describe)
-        seen = {}
+        seen: Dict[str, QAPISchemaMember] = {}
         for f in self.features:
             f.check_clash(self.info, seen)
 
-    def connect_doc(self, doc):
+    def connect_doc(self, doc: Optional[QAPIDoc]) -> None:
         super().connect_doc(doc)
         if doc:
             for f in self.features:
@@ -722,21 +827,35 @@ def connect_doc(self, doc):
 class QAPISchemaVariant(QAPISchemaObjectTypeMember):
     role = 'branch'
 
-    def __init__(self, name, info, typ, ifcond=None):
+    def __init__(self,
+                 name: str,
+                 info: QAPISourceInfo,
+                 typ: str,
+                 ifcond: Optional[List[str]] = None):
         super().__init__(name, info, typ, False, ifcond)
 
 
 class QAPISchemaCommand(QAPISchemaEntity):
-    def __init__(self, name, info, doc, ifcond, features,
-                 arg_type, ret_type,
-                 gen, success_response, boxed, allow_oob, allow_preconfig):
+    def __init__(self,
+                 name: str,
+                 info: QAPISourceInfo,
+                 doc: QAPIDoc,
+                 ifcond: Optional[List[str]],
+                 features: List[QAPISchemaFeature],
+                 arg_type: str,
+                 ret_type: Optional[str],
+                 gen: bool,
+                 success_response: bool,
+                 boxed: bool,
+                 allow_oob: bool,
+                 allow_preconfig: bool):
         super().__init__(name, info, doc, ifcond, features)
         assert not arg_type or isinstance(arg_type, str)
         assert not ret_type or isinstance(ret_type, str)
         self._arg_type_name = arg_type
-        self.arg_type = None
+        self.arg_type: Optional[QAPISchemaObjectType] = None
         self._ret_type_name = ret_type
-        self.ret_type = None
+        self.ret_type: Optional[QAPISchemaType] = None
         self.gen = gen
         self.success_response = success_response
         self.boxed = boxed
@@ -744,7 +863,7 @@ def __init__(self, name, info, doc, ifcond, features,
         self.allow_preconfig = allow_preconfig
         self._meta = 'command'
 
-    def check(self, schema):
+    def check(self, schema: 'QAPISchema') -> None:
         super().check(schema)
         if self._arg_type_name:
             arg_type = schema.resolve_type(
@@ -774,14 +893,14 @@ def check(self, schema):
                         "command's 'returns' cannot take %s"
                         % self.ret_type.describe())
 
-    def connect_doc(self, doc=None):
+    def connect_doc(self, doc: Optional[QAPIDoc] = None) -> None:
         super().connect_doc(doc)
         doc = doc or self.doc
         if doc:
             if self.arg_type and self.arg_type.is_implicit():
                 self.arg_type.connect_doc(doc)
 
-    def visit(self, visitor):
+    def visit(self, visitor: QAPISchemaVisitor) -> None:
         super().visit(visitor)
         visitor.visit_command(
             self.name, self.info, self.ifcond, self.features,
@@ -790,15 +909,22 @@ def visit(self, visitor):
 
 
 class QAPISchemaEvent(QAPISchemaEntity):
-    def __init__(self, name, info, doc, ifcond, features, arg_type, boxed):
+    def __init__(self,
+                 name: str,
+                 info: QAPISourceInfo,
+                 doc: QAPIDoc,
+                 ifcond: Optional[List[str]],
+                 features: List[QAPISchemaFeature],
+                 arg_type: str,
+                 boxed: bool):
         super().__init__(name, info, doc, ifcond, features)
         assert not arg_type or isinstance(arg_type, str)
         self._arg_type_name = arg_type
-        self.arg_type = None
+        self.arg_type: Optional[QAPISchemaObjectType] = None
         self.boxed = boxed
         self._meta = 'event'
 
-    def check(self, schema):
+    def check(self, schema: 'QAPISchema') -> None:
         super().check(schema)
         if self._arg_type_name:
             arg_type = schema.resolve_type(
@@ -815,29 +941,32 @@ def check(self, schema):
                     "event's 'data' can take %s only with 'boxed': true"
                     % self.arg_type.describe())
 
-    def connect_doc(self, doc=None):
+    def connect_doc(self, doc: Optional[QAPIDoc] = None) -> None:
         super().connect_doc(doc)
         doc = doc or self.doc
         if doc:
             if self.arg_type and self.arg_type.is_implicit():
                 self.arg_type.connect_doc(doc)
 
-    def visit(self, visitor):
+    def visit(self, visitor: QAPISchemaVisitor) -> None:
         super().visit(visitor)
         visitor.visit_event(
             self.name, self.info, self.ifcond, self.features,
             self.arg_type, self.boxed)
 
 
+_EntityType = TypeVar('_EntityType', bound=QAPISchemaEntity)
+
+
 class QAPISchema(Visitable):
-    def __init__(self, fname):
+    def __init__(self, fname: str):
         self.fname = fname
         parser = QAPISchemaParser(fname)
         exprs = check_exprs(parser.exprs)
         self.docs = parser.docs
-        self._entity_list = []
-        self._entity_dict = {}
-        self._module_dict = OrderedDict()
+        self._entity_list: List[QAPISchemaEntity] = []
+        self._entity_dict: Dict[str, QAPISchemaEntity] = {}
+        self._module_dict: Dict[str, QAPISchemaModule] = OrderedDict()
         self._schema_dir = os.path.dirname(fname)
         self._make_module(None)  # built-ins
         self._make_module(fname)
@@ -847,7 +976,7 @@ def __init__(self, fname):
         self._def_exprs(exprs)
         self.check()
 
-    def _def_entity(self, ent):
+    def _def_entity(self, ent: QAPISchemaEntity) -> None:
         # Only the predefined types are allowed to not have info
         assert ent.info or self._predefining
         self._entity_list.append(ent)
@@ -866,16 +995,33 @@ def _def_entity(self, ent):
                 ent.info, "%s is already defined" % other_ent.describe())
         self._entity_dict[ent.name] = ent
 
-    def lookup_entity(self, name, typ=None):
+    @overload
+    def lookup_entity(self, name: str,
+                      typ: None = None) -> Optional[QAPISchemaEntity]: ...
+
+    @overload
+    def lookup_entity(self, name: str,
+                      typ: Type[_EntityType]) -> Optional[_EntityType]: ...
+
+    def lookup_entity(self,
+                      name: str,
+                      typ: Optional[Type[QAPISchemaEntity]] = None,
+                      ) -> Optional[QAPISchemaEntity]:
         ent = self._entity_dict.get(name)
         if typ and not isinstance(ent, typ):
             return None
         return ent
 
-    def lookup_type(self, name):
+    def lookup_type(self, name: str) -> QAPISchemaType:
         return self.lookup_entity(name, QAPISchemaType)
 
-    def resolve_type(self, name, info, what):
+    def resolve_type(self,
+                     name: str,
+                     info: Optional[QAPISourceInfo],
+                     what: Optional[
+                         Union[str, Callable[[Optional[QAPISourceInfo]], str]]
+                     ],
+                     ) -> QAPISchemaType:
         typ = self.lookup_type(name)
         if not typ:
             if callable(what):
@@ -884,27 +1030,33 @@ def resolve_type(self, name, info, what):
                 info, "%s uses unknown type '%s'" % (what, name))
         return typ
 
-    def _module_name(self, fname):
+    def _module_name(self, fname: Optional[str]) -> Optional[str]:
         if fname is None:
             return None
         return os.path.relpath(fname, self._schema_dir)
 
-    def _make_module(self, fname):
+    def _make_module(self, fname: Optional[str]) -> QAPISchemaModule:
         name = self._module_name(fname)
         if name not in self._module_dict:
             self._module_dict[name] = QAPISchemaModule(name)
         return self._module_dict[name]
 
-    def module_by_fname(self, fname):
+    def module_by_fname(self, fname: Optional[str]) -> QAPISchemaModule:
         name = self._module_name(fname)
         return self._module_dict[name]
 
-    def _def_include(self, expr, info, doc):
+    def _def_include(self,
+                     expr: Dict[str, Any],
+                     info: QAPISourceInfo,
+                     doc: Optional[QAPIDoc]) -> None:
         include = expr['include']
         assert doc is None
         self._def_entity(QAPISchemaInclude(self._make_module(include), info))
 
-    def _def_builtin_type(self, name, json_type, c_type):
+    def _def_builtin_type(self,
+                          name: str,
+                          json_type: str,
+                          c_type: str) -> None:
         self._def_entity(QAPISchemaBuiltinType(name, json_type, c_type))
         # Instantiating only the arrays that are actually used would
         # be nice, but we can't as long as their generated code
@@ -912,7 +1064,7 @@ def _def_builtin_type(self, name, json_type, c_type):
         # schema.
         self._make_array_type(name, None)
 
-    def _def_predefineds(self):
+    def _def_predefineds(self) -> None:
         for t in [('str',    'string',  'char' + POINTER_SUFFIX),
                   ('number', 'number',  'double'),
                   ('int',    'int',     'int64_t'),
@@ -941,17 +1093,26 @@ def _def_predefineds(self):
         self._def_entity(QAPISchemaEnumType('QType', None, None, None, None,
                                             qtype_values, 'QTYPE'))
 
-    def _make_features(self, features, info):
+    def _make_features(self,
+                       features: Optional[List[Dict[str, Any]]],
+                       info: QAPISourceInfo) -> List[QAPISchemaFeature]:
         if features is None:
             return []
         return [QAPISchemaFeature(f['name'], info, f.get('if'))
                 for f in features]
 
-    def _make_enum_members(self, values, info):
+    def _make_enum_members(self,
+                           values: List[Dict[str, Any]],
+                           info: Optional[QAPISourceInfo],
+                           ) -> List[QAPISchemaEnumMember]:
         return [QAPISchemaEnumMember(v['name'], info, v.get('if'))
                 for v in values]
 
-    def _make_implicit_enum_type(self, name, info, ifcond, values):
+    def _make_implicit_enum_type(self,
+                                 name: str,
+                                 info: QAPISourceInfo,
+                                 ifcond: Optional[List[str]],
+                                 values: List[Dict[str, Any]]) -> str:
         # See also QAPISchemaObjectTypeMember.describe()
         name = name + 'Kind'    # reserved by check_defn_name_str()
         self._def_entity(QAPISchemaEnumType(
@@ -960,13 +1121,21 @@ def _make_implicit_enum_type(self, name, info, ifcond, values):
             None))
         return name
 
-    def _make_array_type(self, element_type, info):
+    def _make_array_type(self,
+                         element_type: str,
+                         info: QAPISourceInfo) -> str:
         name = element_type + 'List'    # reserved by check_defn_name_str()
         if not self.lookup_type(name):
             self._def_entity(QAPISchemaArrayType(name, info, element_type))
         return name
 
-    def _make_implicit_object_type(self, name, info, ifcond, role, members):
+    def _make_implicit_object_type(self,
+                                   name: str,
+                                   info: QAPISourceInfo,
+                                   ifcond: Optional[QAPISchemaType],
+                                   role: str,
+                                   members: List[QAPISchemaObjectTypeMember],
+                                   ) -> Optional[str]:
         if not members:
             return None
         # See also QAPISchemaObjectTypeMember.describe()
@@ -990,7 +1159,10 @@ def _make_implicit_object_type(self, name, info, ifcond, role, members):
                 name, info, None, ifcond, None, None, members, None))
         return name
 
-    def _def_enum_type(self, expr, info, doc):
+    def _def_enum_type(self,
+                       expr: Dict[str, Any],
+                       info: QAPISourceInfo,
+                       doc: QAPIDoc) -> None:
         name = expr['enum']
         data = expr['data']
         prefix = expr.get('prefix')
@@ -1000,7 +1172,12 @@ def _def_enum_type(self, expr, info, doc):
             name, info, doc, ifcond, features,
             self._make_enum_members(data, info), prefix))
 
-    def _make_member(self, name, typ, ifcond, features, info):
+    def _make_member(self,
+                     name: str,
+                     typ: str,
+                     ifcond: Optional[List[str]],
+                     features: Optional[List[Dict[str, Any]]],
+                     info: QAPISourceInfo) -> QAPISchemaObjectTypeMember:
         optional = False
         if name.startswith('*'):
             name = name[1:]
@@ -1011,12 +1188,18 @@ def _make_member(self, name, typ, ifcond, features, info):
         return QAPISchemaObjectTypeMember(name, info, typ, optional, ifcond,
                                           self._make_features(features, info))
 
-    def _make_members(self, data, info):
+    def _make_members(self,
+                      data: Dict[str, Dict[str, Any]],
+                      info: QAPISourceInfo,
+                      ) -> List[QAPISchemaObjectTypeMember]:
         return [self._make_member(key, value['type'], value.get('if'),
                                   value.get('features'), info)
                 for (key, value) in data.items()]
 
-    def _def_struct_type(self, expr, info, doc):
+    def _def_struct_type(self,
+                         expr: Dict[str, Any],
+                         info: QAPISourceInfo,
+                         doc: QAPIDoc) -> None:
         name = expr['struct']
         base = expr.get('base')
         data = expr['data']
@@ -1027,10 +1210,18 @@ def _def_struct_type(self, expr, info, doc):
             self._make_members(data, info),
             None))
 
-    def _make_variant(self, case, typ, ifcond, info):
+    def _make_variant(self,
+                      case: str,
+                      typ: str,
+                      ifcond: Optional[List[str]],
+                      info: QAPISourceInfo) -> QAPISchemaVariant:
         return QAPISchemaVariant(case, info, typ, ifcond)
 
-    def _make_simple_variant(self, case, typ, ifcond, info):
+    def _make_simple_variant(self,
+                             case: str,
+                             typ: str,
+                             ifcond: Optional[List[str]],
+                             info: QAPISourceInfo) -> QAPISchemaVariant:
         if isinstance(typ, list):
             assert len(typ) == 1
             typ = self._make_array_type(typ[0], info)
@@ -1039,7 +1230,10 @@ def _make_simple_variant(self, case, typ, ifcond, info):
             'wrapper', [self._make_member('data', typ, None, None, info)])
         return QAPISchemaVariant(case, info, typ, ifcond)
 
-    def _def_union_type(self, expr, info, doc):
+    def _def_union_type(self,
+                        expr: Dict[str, Any],
+                        info: QAPISourceInfo,
+                        doc: QAPIDoc) -> None:
         name = expr['union']
         data = expr['data']
         base = expr.get('base')
@@ -1070,7 +1264,10 @@ def _def_union_type(self, expr, info, doc):
                                  QAPISchemaVariants(
                                      tag_name, info, tag_member, variants)))
 
-    def _def_alternate_type(self, expr, info, doc):
+    def _def_alternate_type(self,
+                            expr: Dict[str, Any],
+                            info: QAPISourceInfo,
+                            doc: QAPIDoc) -> None:
         name = expr['alternate']
         data = expr['data']
         ifcond = expr.get('if')
@@ -1084,7 +1281,10 @@ def _def_alternate_type(self, expr, info, doc):
                                     QAPISchemaVariants(
                                         None, info, tag_member, variants)))
 
-    def _def_command(self, expr, info, doc):
+    def _def_command(self,
+                     expr: Dict[str, Any],
+                     info: QAPISourceInfo,
+                     doc: QAPIDoc) -> None:
         name = expr['command']
         data = expr.get('data')
         rets = expr.get('returns')
@@ -1107,7 +1307,10 @@ def _def_command(self, expr, info, doc):
                                            gen, success_response,
                                            boxed, allow_oob, allow_preconfig))
 
-    def _def_event(self, expr, info, doc):
+    def _def_event(self,
+                   expr: Dict[str, Any],
+                   info: QAPISourceInfo,
+                   doc: QAPIDoc) -> None:
         name = expr['event']
         data = expr.get('data')
         boxed = expr.get('boxed', False)
@@ -1120,7 +1323,7 @@ def _def_event(self, expr, info, doc):
         self._def_entity(QAPISchemaEvent(name, info, doc, ifcond, features,
                                          data, boxed))
 
-    def _def_exprs(self, exprs):
+    def _def_exprs(self, exprs: List[ParsedExpression]) -> None:
         for expr_elem in exprs:
             expr = expr_elem.expr
             info = expr_elem.info
@@ -1142,7 +1345,7 @@ def _def_exprs(self, exprs):
             else:
                 assert False
 
-    def check(self):
+    def check(self) -> None:
         for ent in self._entity_list:
             ent.check(self)
             ent.connect_doc()
@@ -1150,7 +1353,7 @@ def check(self):
         for ent in self._entity_list:
             ent.set_module(self)
 
-    def visit(self, visitor):
+    def visit(self, visitor: QAPISchemaVisitor) -> None:
         visitor.visit_begin(self)
         for mod in self._module_dict.values():
             mod.visit(visitor)
-- 
2.26.2

[PATCH 16/25] qapi/schema.py: enable checking

[John Snow]

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

diff --git a/scripts/qapi/mypy.ini b/scripts/qapi/mypy.ini
index 20ab509946..df60c18de1 100644
--- a/scripts/qapi/mypy.ini
+++ b/scripts/qapi/mypy.ini
@@ -1,10 +1,4 @@
 [mypy]
 strict = True
 strict_optional = False
-disallow_untyped_calls = False
 python_version = 3.6
-
-[mypy-qapi.schema]
-disallow_untyped_defs = False
-disallow_incomplete_defs = False
-check_untyped_defs = False
-- 
2.26.2

[PATCH 17/25] qapi: Disable similarity checks in pylint entirely

[John Snow]

The pylint similarity checks cannot distinguish parameter lists from
other code; with the QAPISchemaVisitor interface having long lists of
parameters, these similarity checks fire off in a way that's difficult
to disable in a targeted way without littering the code with pylint
pragmas.

There is a change request filed to be able to ignore parameter lists,
see: https://github.com/PyCQA/pylint/issues/3619

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 5091a08f12..fb444e93bb 100644
--- a/scripts/qapi/pylintrc
+++ b/scripts/qapi/pylintrc
@@ -18,6 +18,7 @@ ignore-patterns=schema.py,
 # --disable=W".
 disable=fixme,
         missing-docstring,
+        similarities, # See https://github.com/PyCQA/pylint/issues/3619
         too-many-arguments,
         too-many-branches,
         too-many-statements,
-- 
2.26.2

[PATCH 18/25] qapi/schema.py: Add pylint warning suppressions

[John Snow]

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

diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
index 8907bec0b5..61238c0686 100644
--- a/scripts/qapi/schema.py
+++ b/scripts/qapi/schema.py
@@ -39,6 +39,8 @@
 
 class Visitable:
     """Abstract duck that suggests a class is visitable."""
+    # pylint: disable=too-few-public-methods
+
     def visit(self, visitor: 'QAPISchemaVisitor') -> None:
         raise NotImplementedError
 
@@ -133,6 +135,7 @@ def visit_module(self, name: Optional[str]) -> None:
         pass
 
     def visit_needed(self, entity: QAPISchemaEntity) -> bool:
+        # pylint: disable=unused-argument, no-self-use
         # Default to visiting everything
         return True
 
-- 
2.26.2

[PATCH 19/25] qapi/schema.py: Convert several methods to classmethods

[John Snow]

If they don't use self and nothing that extends them needs self either,
they can be classmethods.

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

diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
index 61238c0686..2d23ce04eb 100644
--- a/scripts/qapi/schema.py
+++ b/scripts/qapi/schema.py
@@ -1096,7 +1096,8 @@ def _def_predefineds(self) -> None:
         self._def_entity(QAPISchemaEnumType('QType', None, None, None, None,
                                             qtype_values, 'QTYPE'))
 
-    def _make_features(self,
+    @classmethod
+    def _make_features(cls,
                        features: Optional[List[Dict[str, Any]]],
                        info: QAPISourceInfo) -> List[QAPISchemaFeature]:
         if features is None:
@@ -1104,7 +1105,8 @@ def _make_features(self,
         return [QAPISchemaFeature(f['name'], info, f.get('if'))
                 for f in features]
 
-    def _make_enum_members(self,
+    @classmethod
+    def _make_enum_members(cls,
                            values: List[Dict[str, Any]],
                            info: Optional[QAPISourceInfo],
                            ) -> List[QAPISchemaEnumMember]:
@@ -1213,7 +1215,8 @@ def _def_struct_type(self,
             self._make_members(data, info),
             None))
 
-    def _make_variant(self,
+    @classmethod
+    def _make_variant(cls,
                       case: str,
                       typ: str,
                       ifcond: Optional[List[str]],
-- 
2.26.2

[PATCH 20/25] qapi/schema.py: Replace one-letter variable names

[John Snow]

I hope you like butter, because here comes the churn!

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

diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
index 2d23ce04eb..a0e047c735 100644
--- a/scripts/qapi/schema.py
+++ b/scripts/qapi/schema.py
@@ -53,9 +53,11 @@ def __init__(self,
                  ifcond: Optional[Union[List[str], 'QAPISchemaType']] = None,
                  features: Optional[List['QAPISchemaFeature']] = None):
         assert name is None or isinstance(name, str)
-        for f in features or []:
-            assert isinstance(f, QAPISchemaFeature)
-            f.set_defined_in(name)
+
+        for feature in features or []:
+            assert isinstance(feature, QAPISchemaFeature)
+            feature.set_defined_in(name)
+
         self.name = name
         self._module: Optional[QAPISchemaModule] = None
         # For explicitly defined entities, info points to the (explicit)
@@ -84,15 +86,15 @@ def c_name(self) -> str:
     def check(self, schema: 'QAPISchema') -> None:
         assert not self._checked
         seen: Dict[str, 'QAPISchemaMember'] = {}
-        for f in self.features:
-            f.check_clash(self.info, seen)
+        for feature in self.features:
+            feature.check_clash(self.info, seen)
         self._checked = True
 
     def connect_doc(self, doc: Optional[QAPIDoc] = None) -> None:
         doc = doc or self.doc
         if doc:
-            for f in self.features:
-                doc.connect_feature(f)
+            for feature in self.features:
+                doc.connect_feature(feature)
 
     def check_doc(self) -> None:
         if self.doc:
@@ -326,9 +328,9 @@ def __init__(self,
                  members: List['QAPISchemaEnumMember'],
                  prefix: Optional[str]):
         super().__init__(name, info, doc, ifcond, features)
-        for m in members:
-            assert isinstance(m, QAPISchemaEnumMember)
-            m.set_defined_in(name)
+        for member in members:
+            assert isinstance(member, QAPISchemaEnumMember)
+            member.set_defined_in(name)
         assert prefix is None or isinstance(prefix, str)
         self.members = members
         self.prefix = prefix
@@ -337,14 +339,14 @@ def __init__(self,
     def check(self, schema: 'QAPISchema') -> None:
         super().check(schema)
         seen: Dict[str, 'QAPISchemaMember'] = {}
-        for m in self.members:
-            m.check_clash(self.info, seen)
+        for member in self.members:
+            member.check_clash(self.info, seen)
 
     def connect_doc(self, doc: Optional[QAPIDoc] = None) -> None:
         super().connect_doc(doc)
         doc = doc or self.doc
-        for m in self.members:
-            m.connect_doc(doc)
+        for member in self.members:
+            member.connect_doc(doc)
 
     def is_implicit(self) -> bool:
         # See QAPISchema._make_implicit_enum_type() and ._def_predefineds()
@@ -432,9 +434,9 @@ def __init__(self,
         super().__init__(name, info, doc, ifcond, features)
         self._meta = 'union' if variants else 'struct'
         assert base is None or isinstance(base, str)
-        for m in local_members:
-            assert isinstance(m, QAPISchemaObjectTypeMember)
-            m.set_defined_in(name)
+        for member in local_members:
+            assert isinstance(member, QAPISchemaObjectTypeMember)
+            member.set_defined_in(name)
         if variants is not None:
             assert isinstance(variants, QAPISchemaVariants)
             variants.set_defined_in(name)
@@ -471,9 +473,9 @@ def check(self, schema: 'QAPISchema') -> None:
             self.base = base
             self.base.check(schema)
             self.base.check_clash(self.info, seen)
-        for m in self.local_members:
-            m.check(schema)
-            m.check_clash(self.info, seen)
+        for member in self.local_members:
+            member.check(schema)
+            member.check_clash(self.info, seen)
 
         # check_clash is abstract, but local_members is asserted to be
         # Sequence[QAPISchemaObjectTypeMember]. Cast to the narrower type.
@@ -493,16 +495,16 @@ def check_clash(self,
                     seen: Dict[str, 'QAPISchemaMember']) -> None:
         assert self._checked
         assert not self.variants       # not implemented
-        for m in self.members:
-            m.check_clash(info, seen)
+        for member in self.members:
+            member.check_clash(info, seen)
 
     def connect_doc(self, doc: Optional[QAPIDoc] = None) -> None:
         super().connect_doc(doc)
         doc = doc or self.doc
         if self.base and self.base.is_implicit():
             self.base.connect_doc(doc)
-        for m in self.local_members:
-            m.connect_doc(doc)
+        for member in self.local_members:
+            member.connect_doc(doc)
 
     @property
     def ifcond(self) -> List[str]:
@@ -572,40 +574,43 @@ def check(self, schema: 'QAPISchema') -> None:
         # so we have to check for potential name collisions ourselves.
         seen: Dict[str, QAPISchemaMember] = {}
         types_seen: Dict[str, str] = {}
-        for v in self.variants.variants:
-            v.check_clash(self.info, seen)
+
+        for variant in self.variants.variants:
+            variant.check_clash(self.info, seen)
+
             try:
-                qtype = v.type.alternate_qtype()
+                qtype = variant.type.alternate_qtype()
             except KeyError:
-                raise QAPISemError(
-                    self.info,
-                    "%s cannot use %s"
-                    % (v.describe(self.info), v.type.describe()))
+                msg = "{} cannot use {}".format(
+                    variant.describe(self.info), variant.type.describe())
+                raise QAPISemError(self.info, msg) from None
+
             conflicting = set([qtype])
             if qtype == 'QTYPE_QSTRING':
-                if isinstance(v.type, QAPISchemaEnumType):
-                    for m in v.type.members:
-                        if m.name in ['on', 'off']:
+                if isinstance(variant.type, QAPISchemaEnumType):
+                    for member in variant.type.members:
+                        if member.name in ['on', 'off']:
                             conflicting.add('QTYPE_QBOOL')
-                        if re.match(r'[-+0-9.]', m.name):
+                        if re.match(r'[-+0-9.]', member.name):
                             # lazy, could be tightened
                             conflicting.add('QTYPE_QNUM')
                 else:
                     conflicting.add('QTYPE_QNUM')
                     conflicting.add('QTYPE_QBOOL')
-            for qt in conflicting:
-                if qt in types_seen:
-                    raise QAPISemError(
-                        self.info,
-                        "%s can't be distinguished from '%s'"
-                        % (v.describe(self.info), types_seen[qt]))
-                types_seen[qt] = v.name
+
+            for qtype in conflicting:
+                if qtype in types_seen:
+                    msg = "{} can't be distinguished from '{}'".format(
+                        variant.describe(self.info), types_seen[qtype])
+                    raise QAPISemError(self.info, msg)
+
+                types_seen[qtype] = variant.name
 
     def connect_doc(self, doc: Optional[QAPIDoc] = None) -> None:
         super().connect_doc(doc)
         doc = doc or self.doc
-        for v in self.variants.variants:
-            v.connect_doc(doc)
+        for variant in self.variants.variants:
+            variant.connect_doc(doc)
 
     def c_type(self) -> str:
         return c_name(self.name) + POINTER_SUFFIX
@@ -632,16 +637,16 @@ def __init__(self,
         assert bool(tag_member) != bool(tag_name)
         assert (isinstance(tag_name, str) or
                 isinstance(tag_member, QAPISchemaObjectTypeMember))
-        for v in variants:
-            assert isinstance(v, QAPISchemaVariant)
+        for variant in variants:
+            assert isinstance(variant, QAPISchemaVariant)
         self._tag_name = tag_name
         self.info = info
         self.tag_member = tag_member
         self.variants = variants
 
     def set_defined_in(self, name: str) -> None:
-        for v in self.variants:
-            v.set_defined_in(name)
+        for variant in self.variants:
+            variant.set_defined_in(name)
 
     def check(self,
               schema: 'QAPISchema',
@@ -686,40 +691,41 @@ def check(self,
         if self._tag_name:    # flat union
             # branches that are not explicitly covered get an empty type
             cases = {v.name for v in self.variants}
-            for m in self.tag_member.type.members:
-                if m.name not in cases:
-                    v = QAPISchemaVariant(m.name, self.info,
-                                          'q_empty', m.ifcond)
-                    v.set_defined_in(self.tag_member.defined_in)
-                    self.variants.append(v)
+            for member in self.tag_member.type.members:
+                if member.name not in cases:
+                    variant = QAPISchemaVariant(member.name, self.info,
+                                                'q_empty', member.ifcond)
+                    variant.set_defined_in(self.tag_member.defined_in)
+                    self.variants.append(variant)
         if not self.variants:
             raise QAPISemError(self.info, "union has no branches")
-        for v in self.variants:
-            v.check(schema)
+        for variant in self.variants:
+            variant.check(schema)
             # Union names must match enum values; alternate names are
             # checked separately. Use 'seen' to tell the two apart.
             if seen:
-                if v.name not in self.tag_member.type.member_names():
+                if variant.name not in self.tag_member.type.member_names():
                     raise QAPISemError(
                         self.info,
                         "branch '%s' is not a value of %s"
-                        % (v.name, self.tag_member.type.describe()))
-                if (not isinstance(v.type, QAPISchemaObjectType)
-                        or v.type.variants):
+                        % (variant.name, self.tag_member.type.describe()))
+                if (not isinstance(variant.type, QAPISchemaObjectType)
+                        or variant.type.variants):
                     raise QAPISemError(
                         self.info,
-                        "%s cannot use %s"
-                        % (v.describe(self.info), v.type.describe()))
-                v.type.check(schema)
+                        "%s cannot use %s" % (
+                            variant.describe(self.info),
+                            variant.type.describe()))
+                variant.type.check(schema)
 
     def check_clash(self,
                     info: QAPISourceInfo,
                     seen: Dict[str, 'QAPISchemaMember']) -> None:
-        for v in self.variants:
+        for variant in self.variants:
             # Reset seen map for each variant, since qapi names from one
             # branch do not affect another branch
-            assert isinstance(v.type, QAPISchemaObjectType)
-            v.type.check_clash(info, dict(seen))
+            assert isinstance(variant.type, QAPISchemaObjectType)
+            variant.type.check_clash(info, dict(seen))
 
 
 class QAPISchemaMember:
@@ -804,9 +810,9 @@ def __init__(self,
         super().__init__(name, info, ifcond)
         assert isinstance(typ, str)
         assert isinstance(optional, bool)
-        for f in features or []:
-            assert isinstance(f, QAPISchemaFeature)
-            f.set_defined_in(name)
+        for feature in features or []:
+            assert isinstance(feature, QAPISchemaFeature)
+            feature.set_defined_in(name)
         self._type_name = typ
         self.type: Optional[QAPISchemaType] = None
         self.optional = optional
@@ -817,14 +823,14 @@ def check(self, schema: 'QAPISchema') -> None:
         self.type = schema.resolve_type(self._type_name, self.info,
                                         self.describe)
         seen: Dict[str, QAPISchemaMember] = {}
-        for f in self.features:
-            f.check_clash(self.info, seen)
+        for feature in self.features:
+            feature.check_clash(self.info, seen)
 
     def connect_doc(self, doc: Optional[QAPIDoc]) -> None:
         super().connect_doc(doc)
         if doc:
-            for f in self.features:
-                doc.connect_feature(f)
+            for feature in self.features:
+                doc.connect_feature(feature)
 
 
 class QAPISchemaVariant(QAPISchemaObjectTypeMember):
@@ -1068,22 +1074,22 @@ def _def_builtin_type(self,
         self._make_array_type(name, None)
 
     def _def_predefineds(self) -> None:
-        for t in [('str',    'string',  'char' + POINTER_SUFFIX),
-                  ('number', 'number',  'double'),
-                  ('int',    'int',     'int64_t'),
-                  ('int8',   'int',     'int8_t'),
-                  ('int16',  'int',     'int16_t'),
-                  ('int32',  'int',     'int32_t'),
-                  ('int64',  'int',     'int64_t'),
-                  ('uint8',  'int',     'uint8_t'),
-                  ('uint16', 'int',     'uint16_t'),
-                  ('uint32', 'int',     'uint32_t'),
-                  ('uint64', 'int',     'uint64_t'),
-                  ('size',   'int',     'uint64_t'),
-                  ('bool',   'boolean', 'bool'),
-                  ('any',    'value',   'QObject' + POINTER_SUFFIX),
-                  ('null',   'null',    'QNull' + POINTER_SUFFIX)]:
-            self._def_builtin_type(*t)
+        for args in (('str',    'string',  'char' + POINTER_SUFFIX),
+                     ('number', 'number',  'double'),
+                     ('int',    'int',     'int64_t'),
+                     ('int8',   'int',     'int8_t'),
+                     ('int16',  'int',     'int16_t'),
+                     ('int32',  'int',     'int32_t'),
+                     ('int64',  'int',     'int64_t'),
+                     ('uint8',  'int',     'uint8_t'),
+                     ('uint16', 'int',     'uint16_t'),
+                     ('uint32', 'int',     'uint32_t'),
+                     ('uint64', 'int',     'uint64_t'),
+                     ('size',   'int',     'uint64_t'),
+                     ('bool',   'boolean', 'bool'),
+                     ('any',    'value',   'QObject' + POINTER_SUFFIX),
+                     ('null',   'null',    'QNull' + POINTER_SUFFIX)):
+            self._def_builtin_type(*args)
         self.the_empty_object_type = QAPISchemaObjectType(
             'q_empty', None, None, None, None, None, [], None)
         self._def_entity(self.the_empty_object_type)
-- 
2.26.2

[PATCH 21/25] qapi/schema.py: disable pylint line limit

[John Snow]

It's a big file, but there's really nothing in here that doesn't belong
in here -- and I don't think it's large enough to justify the
one-module-per-class approach.

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

diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
index a0e047c735..271befea1c 100644
--- a/scripts/qapi/schema.py
+++ b/scripts/qapi/schema.py
@@ -12,6 +12,8 @@
 # This work is licensed under the terms of the GNU GPL, version 2.
 # See the COPYING file in the top-level directory.
 
+# pylint: disable=too-many-lines ¯\_(ツ)_/¯
+
 # TODO catching name collisions in generated code would be nice
 
 from collections import OrderedDict
-- 
2.26.2

[PATCH 22/25] qapi/schema.py: Ignore unused argument for check()

[John Snow]

This is an interface with a default implementation. Pylint doesn't have
enough context to be aware of this.

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

diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
index 271befea1c..6ecbc2aa6b 100644
--- a/scripts/qapi/schema.py
+++ b/scripts/qapi/schema.py
@@ -86,6 +86,7 @@ def c_name(self) -> str:
         return c_name(self.name)
 
     def check(self, schema: 'QAPISchema') -> None:
+        # pylint: disable=unused-argument
         assert not self._checked
         seen: Dict[str, 'QAPISchemaMember'] = {}
         for feature in self.features:
-- 
2.26.2

[PATCH 23/25] qapi/schema.py: enable pylint checks

[John Snow]

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

diff --git a/scripts/qapi/pylintrc b/scripts/qapi/pylintrc
index fb444e93bb..539e5f65a0 100644
--- a/scripts/qapi/pylintrc
+++ b/scripts/qapi/pylintrc
@@ -1,10 +1,5 @@
 [MASTER]
 
-# Add files or directories matching the regex patterns to the ignore list.
-# The regex matches against base names, not paths.
-ignore-patterns=schema.py,
-
-
 [MESSAGES CONTROL]
 
 # Disable the message, report, category or checker with the given id(s). You
-- 
2.26.2

[PATCH 24/25] qapi/schema.py: Add module docstring

[John Snow]

Add some microdocumentation that gives a nice file-level overview of
this 1300+ line file.

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

diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
index 6ecbc2aa6b..baafe3babf 100644
--- a/scripts/qapi/schema.py
+++ b/scripts/qapi/schema.py
@@ -1,7 +1,37 @@
 # -*- coding: utf-8 -*-
-#
-# QAPI schema internal representation
-#
+"""
+QAPI Schema internal representation.
+
+The `QAPISchema` represents the fully realized QAPI schema. It builds a
+listing of `QAPISchemaEntity` objects that are each associated with a
+`QAPISchemaModule`.
+
+`QAPISchemaMember` represents a single member of a collection of
+members, see the subclasses thereof for users. `QAPISchemaVariants` is a
+simple collection of `QAPISchemaVariant`.
+
+The `QAPISchemaVisitor` can be extended and passed to QAPISchema.visit
+to iterate over a schema and perform code generation tasks.
+
+The Python class hierarchy at a glance:
+
+`QAPISchemaEntity`
+  `QAPISchemaInclude`
+  `QAPISchemaCommand`
+  `QAPISchemaEvent`
+  `QAPISchemaType`
+    `QAPISchemaBuiltinType`
+    `QAPISchemaEnumType`
+    `QAPISchemaArrayType`
+    `QAPISchemaObjectType`
+    `QAPISchemaAlternateType`
+
+`QAPISchemaMember`
+  `QAPISchemaEnumMember`
+  `QAPISchemaFeature`
+  `QAPISchemaObjectTypeMember`
+    `QAPISchemaVariant`
+"""
 # Copyright (c) 2015-2019 Red Hat Inc.
 #
 # Authors:
-- 
2.26.2

[PATCH 25/25] qapi/schema.py: Use python3 style super()

[John Snow]

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

diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
index baafe3babf..6b47ca26e0 100644
--- a/scripts/qapi/schema.py
+++ b/scripts/qapi/schema.py
@@ -309,7 +309,7 @@ def doc_type(self) -> Optional[str]:
         return self.name
 
     def check(self, schema: 'QAPISchema') -> None:
-        QAPISchemaEntity.check(self, schema)
+        super().check(schema)
         if 'deprecated' in [f.name for f in self.features]:
             raise QAPISemError(
                 self.info, "feature 'deprecated' is not supported for types")
-- 
2.26.2

Re: [PATCH 00/25] qapi: static typing conversion, pt6

[John Snow]

On 9/22/20 6:44 PM, John Snow wrote:
> based-on: <20200922223525.4085762-1-jsnow@redhat.com>
>            [PATCH 00/26] qapi: static typing conversion, pt5
> 
> Hi, this series adds static type hints to the QAPI module.
> This is the final part, part six!
> 
> Part 6 (Everything):
>      https://gitlab.com/jsnow/qemu/-/tree/python-qapi-cleanup-pt6
> 

Note: I have rebased and updated the pt6 branch. I am currently working 
on respinning pt2, which now contains the introspect.py patches that 
were formerly in pt1.

Thanks,
--js

> - 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 schema.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 (25):
>    qapi/schema: add Visitable mixin
>    qapi/schema.py: Move meta-type into class instances
>    qapi/schema.py: add assert in stub methods
>    qapi/schema.py: constrain QAPISchemaObjectType base type
>    qapi/schema.py: constrain QAPISchemaObjectTypeMember arg_type type
>    qapi/schema.py: constrain QAPISchemaEvent arg_type type
>    qapi/schema.py: constrain tag_member type
>    qapi/schema.py: Allow alternate_type to assert
>    qapi/schema.py: remove superfluous assert
>    qapi/schema.py: Add assertion to ifcond property
>    qapi/schema.py: Constrain type of QAPISchemaObjectType members field
>    qapi/schema.py: remove 'and' from non-bool rvalue expressions
>    qapi/schema.py: Test type of self.ret_type instead of local temp
>    qapi/schema.py: Assert variants of an object are also objects
>    qapi/schema.py: add type hint annotations
>    qapi/schema.py: enable checking
>    qapi: Disable similarity checks in pylint entirely
>    qapi/schema.py: Add pylint warning suppressions
>    qapi/schema.py: Convert several methods to classmethods
>    qapi/schema.py: Replace one-letter variable names
>    qapi/schema.py: disable pylint line limit
>    qapi/schema.py: Ignore unused argument for check()
>    qapi/schema.py: enable pylint checks
>    qapi/schema.py: Add module docstring
>    qapi/schema.py: Use python3 style super()
> 
>   scripts/qapi/mypy.ini  |   6 -
>   scripts/qapi/pylintrc  |   6 +-
>   scripts/qapi/schema.py | 848 +++++++++++++++++++++++++++--------------
>   3 files changed, 557 insertions(+), 303 deletions(-)
>