[PATCH v3 00/20] qapi: statically type schema.py

[PATCH v3 00/20] qapi: statically type schema.py

[John Snow]

This is *v3*, for some definitions of "version" and "three".

It's still got some parts that Markus isn't wildly fond of, but I needed
a synchronization part for discussing the remaining nits. (Sorry Markus.)

v3:
 - 01: fixed alphabetization (... sigh)
 - 03: updated docstring, added super calls,
       renamed argument to @defn, reflowed long lines
 - 04: updated commit message
 - 05: updated commit message
 - 06: pushed type hints into later commit
 - 09: removed default argument and stuffed into next patch
 - 10: Added the parts Markus doesn't like (Sorry)
 - 11: updated commit message
 - 12: updated commit message
 - 13: Split into two patches.
       Kept stuff Markus doesn't like (Sorry)
 - 14: New, split from 13.

I know this probably doesn't address everything, so if I didn't change
it, feel free to re-flag so I can keep the feedback in one spot on the
new series.

(Sorry sorry sorry sorry sorry.)

v2:
 - dropped the resolve_type refactoring patch
 - added QAPISchemaDefinition
 - misc bits and pieces.

001/19:[down] 'qapi: sort pylint suppressions'
  New, Markus's suggestion.

002/19:[0001] [FC] 'qapi/schema: add pylint suppressions'
  Added newline, Markus's RB

003/19:[down] 'qapi: create QAPISchemaDefinition'
  New, Markus's suggestion.

004/19:[0002] [FC] 'qapi/schema: declare type for QAPISchemaObjectTypeMember.type'
  Adjusted commit message and comment.

005/19:[down] 'qapi/schema: declare type for QAPISchemaArrayType.element_type'
  Patch renamed; removed @property trick in favor of static type declaration

006/19:[0009] [FC] 'qapi/schema: make c_type() and json_type() abstract methods'
  Use abc.ABC and @abstractmethod

007/19:[0001] [FC] 'qapi/schema: adjust type narrowing for mypy's benefit'
  Adjusted commit message; left in an assertion that is removed later instead.

008/19:[down] 'qapi/schema: add type narrowing to lookup_type()'
  Renamed
  removed type hints which get added later in the series instead
  simplified logic.

009/19:[down] 'qapi/schema: allow resolve_type to be used for built-in types'
  New patch. (Types are added later.)

010/19:[down] 'qapi: use schema.resolve_type instead of schema.lookup_type'
  New patch, replaces old 07/19 "assert schema.lookup_type did not fail"

011/19:[0011] [FC] 'qapi/schema: fix QAPISchemaArrayType.check's call to resolve_type'
  Dramatically simplified.

012/19:[----] [--] 'qapi/schema: assert info is present when necessary'
013/19:[----] [--] 'qapi/schema: split "checked" field into "checking" and "checked"'
  Changed the commit message, but actually struggled finding anything simpler
  than what I already had, owing to the fact that q_empty is a valid construct
  and I can't seem to avoid adding a new state variable here.

014/19:[----] [-C] 'qapi/schema: fix typing for QAPISchemaVariants.tag_member'
  Also unchanged from review, I think this is simplest still...

015/19:[down] 'qapi/schema: assert inner type of QAPISchemaVariants in check_clash()'
  Renamed, changed commit message and comment.

016/19:[----] [--] 'qapi/parser: demote QAPIExpression to Dict[str, Any]'
017/19:[0042] [FC] 'qapi/schema: add type hints'
  Mostly contextual changes.

018/19:[----] [--] 'qapi/schema: turn on mypy strictness'
019/19:[0006] [FC] 'qapi/schema: remove unnecessary asserts'
  Zapped a few more.

John Snow (20):
  qapi: sort pylint suppressions
  qapi/schema: add pylint suppressions
  qapi: create QAPISchemaDefinition
  qapi/schema: declare type for QAPISchemaObjectTypeMember.type
  qapi/schema: declare type for QAPISchemaArrayType.element_type
  qapi/schema: make c_type() and json_type() abstract methods
  qapi/schema: adjust type narrowing for mypy's benefit
  qapi/schema: add type narrowing to lookup_type()
  qapi/schema: assert resolve_type has 'info' and 'what' args on error
  qapi: use schema.resolve_type instead of schema.lookup_type
  qapi/schema: fix QAPISchemaArrayType.check's call to resolve_type
  qapi/schema: assert info is present when necessary
  qapi/schema: split "checked" field into "checking" and "checked"
  qapi/schema: Don't initialize "members" with `None`
  qapi/schema: fix typing for QAPISchemaVariants.tag_member
  qapi/schema: assert inner type of QAPISchemaVariants in check_clash()
  qapi/parser: demote QAPIExpression to Dict[str, Any]
  qapi/schema: add type hints
  qapi/schema: turn on mypy strictness
  qapi/schema: remove unnecessary asserts

 scripts/qapi/introspect.py |   4 +-
 scripts/qapi/mypy.ini      |   5 -
 scripts/qapi/parser.py     |   3 +-
 scripts/qapi/pylintrc      |  11 +-
 scripts/qapi/schema.py     | 794 ++++++++++++++++++++++++-------------
 5 files changed, 534 insertions(+), 283 deletions(-)

-- 
2.43.0

[PATCH v3 01/20] qapi: sort pylint suppressions

[John Snow]

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

diff --git a/scripts/qapi/pylintrc b/scripts/qapi/pylintrc
index 90546df5345..1342412c3cf 100644
--- a/scripts/qapi/pylintrc
+++ b/scripts/qapi/pylintrc
@@ -16,13 +16,13 @@ ignore-patterns=schema.py,
 # --enable=similarities". If you want to run only the classes checker, but have
 # no Warning level messages displayed, use "--disable=all --enable=classes
 # --disable=W".
-disable=fixme,
+disable=consider-using-f-string,
+        fixme,
         missing-docstring,
         too-many-arguments,
         too-many-branches,
-        too-many-statements,
         too-many-instance-attributes,
-        consider-using-f-string,
+        too-many-statements,
         useless-option-value,
 
 [REPORTS]
-- 
2.43.0

[PATCH v3 02/20] qapi/schema: add pylint suppressions

[John Snow]

With this patch, pylint is happy with the file, so enable it in the
configuration.

Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
---
 scripts/qapi/pylintrc  | 5 -----
 scripts/qapi/schema.py | 5 +++++
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/scripts/qapi/pylintrc b/scripts/qapi/pylintrc
index 1342412c3cf..c028a1f9f51 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
diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
index 6a836950a9a..b7830672e57 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
@@ -83,6 +85,7 @@ def c_name(self):
         return c_name(self.name)
 
     def check(self, schema):
+        # pylint: disable=unused-argument
         assert not self._checked
         seen = {}
         for f in self.features:
@@ -117,6 +120,7 @@ def is_implicit(self):
         return not self.info
 
     def visit(self, visitor):
+        # pylint: disable=unused-argument
         assert self._checked
 
     def describe(self):
@@ -135,6 +139,7 @@ def visit_module(self, name):
         pass
 
     def visit_needed(self, entity):
+        # pylint: disable=unused-argument
         # Default to visiting everything
         return True
 
-- 
2.43.0

[PATCH v3 03/20] qapi: create QAPISchemaDefinition

[John Snow]

Include entities don't have names, but we generally expect "entities" to
have names. Reclassify all entities with names as *definitions*, leaving
the nameless include entities as QAPISchemaEntity instances.

This is primarily to help simplify typing around expectations of what
callers expect for properties of an "entity".

Suggested-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: John Snow <jsnow@redhat.com>
---
 scripts/qapi/schema.py | 152 ++++++++++++++++++++++++-----------------
 1 file changed, 88 insertions(+), 64 deletions(-)

diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
index b7830672e57..7f790d44742 100644
--- a/scripts/qapi/schema.py
+++ b/scripts/qapi/schema.py
@@ -55,14 +55,13 @@ def is_present(self):
 
 
 class QAPISchemaEntity:
-    meta: Optional[str] = None
+    """
+    A schema entity.
 
-    def __init__(self, name: str, info, doc, ifcond=None, features=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
+    This is either a directive, such as include, or a definition.
+    The latter uses sub-class `QAPISchemaDefinition`.
+    """
+    def __init__(self, info):
         self._module = None
         # For explicitly defined entities, info points to the (explicit)
         # definition.  For builtins (and their arrays), info is None.
@@ -70,37 +69,20 @@ def __init__(self, name: str, info, doc, ifcond=None, features=None):
         # triggered the implicit definition (there may be more than one
         # such place).
         self.info = info
-        self.doc = doc
-        self._ifcond = ifcond or QAPISchemaIfCond()
-        self.features = features or []
         self._checked = False
 
     def __repr__(self):
-        if self.name is None:
-            return "<%s at 0x%x>" % (type(self).__name__, id(self))
-        return "<%s:%s at 0x%x>" % (type(self).__name__, self.name,
-                                    id(self))
-
-    def c_name(self):
-        return c_name(self.name)
+        return "<%s at 0x%x>" % (type(self).__name__, id(self))
 
     def check(self, schema):
         # pylint: disable=unused-argument
-        assert not self._checked
-        seen = {}
-        for f in self.features:
-            f.check_clash(self.info, seen)
         self._checked = True
 
     def connect_doc(self, doc=None):
-        doc = doc or self.doc
-        if doc:
-            for f in self.features:
-                doc.connect_feature(f)
+        pass
 
     def check_doc(self):
-        if self.doc:
-            self.doc.check()
+        pass
 
     def _set_module(self, schema, info):
         assert self._checked
@@ -111,6 +93,51 @@ def _set_module(self, schema, info):
     def set_module(self, schema):
         self._set_module(schema, self.info)
 
+    def visit(self, visitor):
+        # pylint: disable=unused-argument
+        assert self._checked
+
+
+class QAPISchemaDefinition(QAPISchemaEntity):
+    meta: Optional[str] = None
+
+    def __init__(self, name: str, info, doc, ifcond=None, features=None):
+        assert isinstance(name, str)
+        super().__init__(info)
+        for f in features or []:
+            assert isinstance(f, QAPISchemaFeature)
+            f.set_defined_in(name)
+        self.name = name
+        self.doc = doc
+        self._ifcond = ifcond or QAPISchemaIfCond()
+        self.features = features or []
+
+    def __repr__(self):
+        return "<%s:%s at 0x%x>" % (type(self).__name__, self.name,
+                                    id(self))
+
+    def c_name(self):
+        return c_name(self.name)
+
+    def check(self, schema):
+        assert not self._checked
+        super().check(schema)
+        seen = {}
+        for f in self.features:
+            f.check_clash(self.info, seen)
+
+    def connect_doc(self, doc=None):
+        super().connect_doc(doc)
+        doc = doc or self.doc
+        if doc:
+            for f in self.features:
+                doc.connect_feature(f)
+
+    def check_doc(self):
+        super().check_doc()
+        if self.doc:
+            self.doc.check()
+
     @property
     def ifcond(self):
         assert self._checked
@@ -119,10 +146,6 @@ def ifcond(self):
     def is_implicit(self):
         return not self.info
 
-    def visit(self, visitor):
-        # pylint: disable=unused-argument
-        assert self._checked
-
     def describe(self):
         assert self.meta
         return "%s '%s'" % (self.meta, self.name)
@@ -222,7 +245,7 @@ def visit(self, visitor):
 
 class QAPISchemaInclude(QAPISchemaEntity):
     def __init__(self, sub_module, info):
-        super().__init__(None, info, None)
+        super().__init__(info)
         self._sub_module = sub_module
 
     def visit(self, visitor):
@@ -230,7 +253,7 @@ def visit(self, visitor):
         visitor.visit_include(self._sub_module.name, self.info)
 
 
-class QAPISchemaType(QAPISchemaEntity):
+class QAPISchemaType(QAPISchemaDefinition):
     # Return the C type for common use.
     # For the types we commonly box, this is a pointer type.
     def c_type(self):
@@ -801,7 +824,7 @@ def __init__(self, name, info, typ, ifcond=None):
         super().__init__(name, info, typ, False, ifcond)
 
 
-class QAPISchemaCommand(QAPISchemaEntity):
+class QAPISchemaCommand(QAPISchemaDefinition):
     meta = 'command'
 
     def __init__(self, name, info, doc, ifcond, features,
@@ -872,7 +895,7 @@ def visit(self, visitor):
             self.coroutine)
 
 
-class QAPISchemaEvent(QAPISchemaEntity):
+class QAPISchemaEvent(QAPISchemaDefinition):
     meta = 'event'
 
     def __init__(self, name, info, doc, ifcond, features, arg_type, boxed):
@@ -943,23 +966,24 @@ def __init__(self, fname):
         self.check()
 
     def _def_entity(self, ent):
+        self._entity_list.append(ent)
+
+    def _def_definition(self, defn):
         # Only the predefined types are allowed to not have info
-        assert ent.info or self._predefining
-        self._entity_list.append(ent)
-        if ent.name is None:
-            return
+        assert defn.info or self._predefining
+        self._def_entity(defn)
         # TODO reject names that differ only in '_' vs. '.'  vs. '-',
         # because they're liable to clash in generated C.
-        other_ent = self._entity_dict.get(ent.name)
-        if other_ent:
-            if other_ent.info:
-                where = QAPISourceError(other_ent.info, "previous definition")
+        other_defn = self._entity_dict.get(defn.name)
+        if other_defn:
+            if other_defn.info:
+                where = QAPISourceError(other_defn.info, "previous definition")
                 raise QAPISemError(
-                    ent.info,
-                    "'%s' is already defined\n%s" % (ent.name, where))
+                    defn.info,
+                    "'%s' is already defined\n%s" % (defn.name, where))
             raise QAPISemError(
-                ent.info, "%s is already defined" % other_ent.describe())
-        self._entity_dict[ent.name] = ent
+                defn.info, "%s is already defined" % other_defn.describe())
+        self._entity_dict[defn.name] = defn
 
     def lookup_entity(self, name, typ=None):
         ent = self._entity_dict.get(name)
@@ -1001,7 +1025,7 @@ def _def_include(self, expr: QAPIExpression):
             QAPISchemaInclude(self._make_module(include), expr.info))
 
     def _def_builtin_type(self, name, json_type, c_type):
-        self._def_entity(QAPISchemaBuiltinType(name, json_type, c_type))
+        self._def_definition(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
         # (qapi-builtin-types.[ch]) may be shared by some other
@@ -1027,15 +1051,15 @@ def _def_predefineds(self):
             self._def_builtin_type(*t)
         self.the_empty_object_type = QAPISchemaObjectType(
             'q_empty', None, None, None, None, None, [], None)
-        self._def_entity(self.the_empty_object_type)
+        self._def_definition(self.the_empty_object_type)
 
         qtypes = ['none', 'qnull', 'qnum', 'qstring', 'qdict', 'qlist',
                   'qbool']
         qtype_values = self._make_enum_members(
             [{'name': n} for n in qtypes], None)
 
-        self._def_entity(QAPISchemaEnumType('QType', None, None, None, None,
-                                            qtype_values, 'QTYPE'))
+        self._def_definition(QAPISchemaEnumType(
+            'QType', None, None, None, None, qtype_values, 'QTYPE'))
 
     def _make_features(self, features, info):
         if features is None:
@@ -1057,7 +1081,8 @@ def _make_enum_members(self, values, info):
     def _make_array_type(self, element_type, info):
         name = element_type + 'List'    # reserved by check_defn_name_str()
         if not self.lookup_type(name):
-            self._def_entity(QAPISchemaArrayType(name, info, element_type))
+            self._def_definition(QAPISchemaArrayType(
+                name, info, element_type))
         return name
 
     def _make_implicit_object_type(self, name, info, ifcond, role, members):
@@ -1072,7 +1097,7 @@ def _make_implicit_object_type(self, name, info, ifcond, role, members):
             # later.
             pass
         else:
-            self._def_entity(QAPISchemaObjectType(
+            self._def_definition(QAPISchemaObjectType(
                 name, info, None, ifcond, None, None, members, None))
         return name
 
@@ -1083,7 +1108,7 @@ def _def_enum_type(self, expr: QAPIExpression):
         ifcond = QAPISchemaIfCond(expr.get('if'))
         info = expr.info
         features = self._make_features(expr.get('features'), info)
-        self._def_entity(QAPISchemaEnumType(
+        self._def_definition(QAPISchemaEnumType(
             name, info, expr.doc, ifcond, features,
             self._make_enum_members(data, info), prefix))
 
@@ -1111,7 +1136,7 @@ def _def_struct_type(self, expr: QAPIExpression):
         info = expr.info
         ifcond = QAPISchemaIfCond(expr.get('if'))
         features = self._make_features(expr.get('features'), info)
-        self._def_entity(QAPISchemaObjectType(
+        self._def_definition(QAPISchemaObjectType(
             name, info, expr.doc, ifcond, features, base,
             self._make_members(data, info),
             None))
@@ -1141,7 +1166,7 @@ def _def_union_type(self, expr: QAPIExpression):
                                info)
             for (key, value) in data.items()]
         members: List[QAPISchemaObjectTypeMember] = []
-        self._def_entity(
+        self._def_definition(
             QAPISchemaObjectType(name, info, expr.doc, ifcond, features,
                                  base, members,
                                  QAPISchemaVariants(
@@ -1160,7 +1185,7 @@ def _def_alternate_type(self, expr: QAPIExpression):
                                info)
             for (key, value) in data.items()]
         tag_member = QAPISchemaObjectTypeMember('type', info, 'QType', False)
-        self._def_entity(
+        self._def_definition(
             QAPISchemaAlternateType(
                 name, info, expr.doc, ifcond, features,
                 QAPISchemaVariants(None, info, tag_member, variants)))
@@ -1185,11 +1210,10 @@ def _def_command(self, expr: QAPIExpression):
         if isinstance(rets, list):
             assert len(rets) == 1
             rets = self._make_array_type(rets[0], info)
-        self._def_entity(QAPISchemaCommand(name, info, expr.doc, ifcond,
-                                           features, data, rets,
-                                           gen, success_response,
-                                           boxed, allow_oob, allow_preconfig,
-                                           coroutine))
+        self._def_definition(
+            QAPISchemaCommand(name, info, expr.doc, ifcond, features, data,
+                              rets, gen, success_response, boxed, allow_oob,
+                              allow_preconfig, coroutine))
 
     def _def_event(self, expr: QAPIExpression):
         name = expr['event']
@@ -1202,8 +1226,8 @@ def _def_event(self, expr: QAPIExpression):
             data = self._make_implicit_object_type(
                 name, info, ifcond,
                 'arg', self._make_members(data, info))
-        self._def_entity(QAPISchemaEvent(name, info, expr.doc, ifcond,
-                                         features, data, boxed))
+        self._def_definition(QAPISchemaEvent(name, info, expr.doc, ifcond,
+                                             features, data, boxed))
 
     def _def_exprs(self, exprs):
         for expr in exprs:
-- 
2.43.0

[PATCH v3 04/20] qapi/schema: declare type for QAPISchemaObjectTypeMember.type

[John Snow]

A QAPISchemaObjectTypeMember's type gets resolved only during .check().
We have QAPISchemaObjectTypeMember.__init__() initialize self.type =
None, and .check() assign the actual type.  Using .type before .check()
is wrong, and hopefully crashes due to the value being None.  Works.

However, it makes for awkward typing.  With .type:
Optional[QAPISchemaType], mypy is of course unable to see that it's None
before .check(), and a QAPISchemaType after.  To help it over the hump,
we'd have to assert self.type is not None before all the (valid) uses.
The assertion catches invalid uses, but only at run time; mypy can't
flag them.

Instead, declare .type in .__init__() as QAPISchemaType *without*
initializing it.  Using .type before .check() now certainly crashes,
which is an improvement.  Mypy still can't flag invalid uses, but that's
okay.

Addresses typing errors such as these:

qapi/schema.py:657: error: "None" has no attribute "alternate_qtype"  [attr-defined]
qapi/schema.py:662: error: "None" has no attribute "describe"  [attr-defined]

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 7f790d44742..74a32656249 100644
--- a/scripts/qapi/schema.py
+++ b/scripts/qapi/schema.py
@@ -794,7 +794,7 @@ 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: QAPISchemaType  # set during check()
         self.optional = optional
         self.features = features or []
 
-- 
2.43.0

[PATCH v3 05/20] qapi/schema: declare type for QAPISchemaArrayType.element_type

[John Snow]

A QAPISchemaArrayType's element type gets resolved only during .check().
We have QAPISchemaArrayType.__init__() initialize self.element_type =
None, and .check() assign the actual type.  Using .element_type before
.check() is wrong, and hopefully crashes due to the value being None.
Works.

However, it makes for awkward typing.  With .element_type:
Optional[QAPISchemaType], mypy is of course unable to see that it's None
before .check(), and a QAPISchemaType after.  To help it over the hump,
we'd have to assert self.element_type is not None before all the (valid)
uses.  The assertion catches invalid uses, but only at run time; mypy
can't flag them.

Instead, declare .element_type in .__init__() as QAPISchemaType
*without* initializing it.  Using .element_type before .check() now
certainly crashes, which is an improvement.  Mypy still can't flag
invalid uses, but that's okay.

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 74a32656249..ac34e2781de 100644
--- a/scripts/qapi/schema.py
+++ b/scripts/qapi/schema.py
@@ -389,7 +389,7 @@ 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.element_type: QAPISchemaType
 
     def need_has_if_optional(self):
         # When FOO is an array, we still need has_FOO to distinguish
-- 
2.43.0

[PATCH v3 06/20] qapi/schema: make c_type() and json_type() abstract methods

[John Snow]

These methods should always return a str, it's only the default abstract
implementation that doesn't. They can be marked "abstract", which
requires subclasses to override the method with the proper return type.

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

diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
index ac34e2781de..8abcd4a86cb 100644
--- a/scripts/qapi/schema.py
+++ b/scripts/qapi/schema.py
@@ -16,6 +16,7 @@
 
 # TODO catching name collisions in generated code would be nice
 
+from abc import ABC, abstractmethod
 from collections import OrderedDict
 import os
 import re
@@ -253,9 +254,10 @@ def visit(self, visitor):
         visitor.visit_include(self._sub_module.name, self.info)
 
 
-class QAPISchemaType(QAPISchemaDefinition):
+class QAPISchemaType(QAPISchemaDefinition, ABC):
     # Return the C type for common use.
     # For the types we commonly box, this is a pointer type.
+    @abstractmethod
     def c_type(self):
         pass
 
@@ -267,6 +269,7 @@ def c_param_type(self):
     def c_unboxed_type(self):
         return self.c_type()
 
+    @abstractmethod
     def json_type(self):
         pass
 
-- 
2.43.0

[PATCH v3 07/20] qapi/schema: adjust type narrowing for mypy's benefit

[John Snow]

We already take care to perform some type narrowing for arg_type and
ret_type, but not in a way where mypy can utilize the result once we add
type hints, e.g.:

qapi/schema.py:833: error: Incompatible types in assignment (expression
has type "QAPISchemaType", variable has type
"Optional[QAPISchemaObjectType]") [assignment]

qapi/schema.py:893: error: Incompatible types in assignment (expression
has type "QAPISchemaType", variable has type
"Optional[QAPISchemaObjectType]") [assignment]

A simple change to use a temporary variable helps the medicine go down.

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

diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
index 8abcd4a86cb..043ee7556e6 100644
--- a/scripts/qapi/schema.py
+++ b/scripts/qapi/schema.py
@@ -851,13 +851,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,
@@ -874,8 +875,8 @@ def check(self, schema):
             if self.name not in self.info.pragma.command_returns_exceptions:
                 typ = self.ret_type
                 if isinstance(typ, QAPISchemaArrayType):
-                    typ = self.ret_type.element_type
                     assert typ
+                    typ = typ.element_type
                 if not isinstance(typ, QAPISchemaObjectType):
                     raise QAPISemError(
                         self.info,
@@ -911,13 +912,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(
+            typ = schema.resolve_type(
                 self._arg_type_name, self.info, "event's 'data'")
-            if not isinstance(self.arg_type, QAPISchemaObjectType):
+            if not isinstance(typ, QAPISchemaObjectType):
                 raise QAPISemError(
                     self.info,
                     "event's 'data' cannot take %s"
-                    % self.arg_type.describe())
+                    % typ.describe())
+            self.arg_type = typ
             if self.arg_type.variants and not self.boxed:
                 raise QAPISemError(
                     self.info,
-- 
2.43.0

[PATCH v3 08/20] qapi/schema: add type narrowing to lookup_type()

[John Snow]

This function is a bit hard to type as-is; mypy needs some assertions to
assist with the type narrowing.

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

diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
index 043ee7556e6..e617abb03af 100644
--- a/scripts/qapi/schema.py
+++ b/scripts/qapi/schema.py
@@ -997,7 +997,9 @@ def lookup_entity(self, name, typ=None):
         return ent
 
     def lookup_type(self, name):
-        return self.lookup_entity(name, QAPISchemaType)
+        typ = self.lookup_entity(name, QAPISchemaType)
+        assert typ is None or isinstance(typ, QAPISchemaType)
+        return typ
 
     def resolve_type(self, name, info, what):
         typ = self.lookup_type(name)
-- 
2.43.0

[PATCH v3 09/20] qapi/schema: assert resolve_type has 'info' and 'what' args on error

[John Snow]

resolve_type() is generally used to resolve configuration-provided type
names into type objects, and generally requires valid 'info' and 'what'
parameters.

In some cases, such as with QAPISchemaArrayType.check(), resolve_type
may be used to resolve built-in types and as such will not have an
'info' argument, but also must not fail in this scenario.

Use an assertion to sate mypy that we will indeed have 'info' and 'what'
parameters for the error pathway in resolve_type.

Note: there are only three callsites to resolve_type at present where
"info" is perceived to be possibly None:

    1) QAPISchemaArrayType.check()
    2) QAPISchemaObjectTypeMember.check()
    3) QAPISchemaEvent.check()

    Of those three, only the first actually ever passes None; the other two
    are limited by their base class initializers which accept info=None, but
    neither subclass actually use a None value in practice, currently.

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 e617abb03af..573be7275a6 100644
--- a/scripts/qapi/schema.py
+++ b/scripts/qapi/schema.py
@@ -1004,6 +1004,7 @@ def lookup_type(self, name):
     def resolve_type(self, name, info, what):
         typ = self.lookup_type(name)
         if not typ:
+            assert info and what  # built-in types must not fail lookup
             if callable(what):
                 what = what(info)
             raise QAPISemError(
-- 
2.43.0

[PATCH v3 10/20] qapi: use schema.resolve_type instead of schema.lookup_type

[John Snow]

the function lookup_type() is capable of returning None, but some
callers aren't prepared for that and assume it will always succeed. For
static type analysis purposes, this creates problems at those callsites.

Modify resolve_type() - which already cannot ever return None - to allow
'info' and 'what' parameters to be elided, which infers that the type
lookup is a built-in type lookup.

Change several callsites to use the new form of resolve_type to avoid
the more laborious strictly-typed error-checking scaffolding:

  ret = lookup_type("foo")
  assert ret is not None
  typ: QAPISchemaType = ret

which can be replaced with the simpler:

  typ = resolve_type("foo")

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

diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
index 67c7d89aae0..c38df61a6d5 100644
--- a/scripts/qapi/introspect.py
+++ b/scripts/qapi/introspect.py
@@ -227,10 +227,10 @@ def _use_type(self, typ: QAPISchemaType) -> str:
 
         # Map the various integer types to plain int
         if typ.json_type() == 'int':
-            typ = self._schema.lookup_type('int')
+            typ = self._schema.resolve_type('int')
         elif (isinstance(typ, QAPISchemaArrayType) and
               typ.element_type.json_type() == 'int'):
-            typ = self._schema.lookup_type('intList')
+            typ = self._schema.resolve_type('intList')
         # Add type to work queue if new
         if typ not in self._used_types:
             self._used_types.append(typ)
diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
index 573be7275a6..074897ee4fb 100644
--- a/scripts/qapi/schema.py
+++ b/scripts/qapi/schema.py
@@ -650,8 +650,7 @@ def check(self, schema, seen):
                     "discriminator '%s' is not a member of %s"
                     % (self._tag_name, base))
             # Here we do:
-            base_type = schema.lookup_type(self.tag_member.defined_in)
-            assert base_type
+            base_type = schema.resolve_type(self.tag_member.defined_in)
             if not base_type.is_implicit():
                 base = "base type '%s'" % self.tag_member.defined_in
             if not isinstance(self.tag_member.type, QAPISchemaEnumType):
@@ -1001,7 +1000,7 @@ def lookup_type(self, name):
         assert typ is None or isinstance(typ, QAPISchemaType)
         return typ
 
-    def resolve_type(self, name, info, what):
+    def resolve_type(self, name, info=None, what=None):
         typ = self.lookup_type(name)
         if not typ:
             assert info and what  # built-in types must not fail lookup
-- 
2.43.0

[PATCH v3 11/20] qapi/schema: fix QAPISchemaArrayType.check's call to resolve_type

[John Snow]

Adjust the expression at the callsite to work around mypy's weak type
introspection that believes this expression can resolve to
QAPISourceInfo; it cannot.

(Fundamentally: self.info only resolves to false in a boolean expression
when it is None; therefore this expression may only ever produce
Optional[str]. mypy does not know that 'info', when it is a
QAPISourceInfo object, cannot ever be false.)

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 074897ee4fb..ae350f64a8f 100644
--- a/scripts/qapi/schema.py
+++ b/scripts/qapi/schema.py
@@ -403,7 +403,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.43.0

[PATCH v3 12/20] qapi/schema: assert info is present when necessary

[John Snow]

QAPISchemaInfo arguments can often be None because built-in definitions
don't have such information.  The type hint can only be
Optional[QAPISchemaInfo] then.  But, mypy gets upset about all the
places where we exploit that it can't actually be None there.  Add
assertions that will help mypy over the hump, to enable adding type
hints in a forthcoming commit.

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 ae350f64a8f..d4d3c3bbcee 100644
--- a/scripts/qapi/schema.py
+++ b/scripts/qapi/schema.py
@@ -758,6 +758,7 @@ def describe(self, info):
             else:
                 assert False
 
+        assert info is not None
         if defined_in != info.defn_name:
             return "%s '%s' of %s '%s'" % (role, self.name, meta, defined_in)
         return "%s '%s'" % (role, self.name)
@@ -848,6 +849,7 @@ def __init__(self, name, info, doc, ifcond, features,
         self.coroutine = coroutine
 
     def check(self, schema):
+        assert self.info is not None
         super().check(schema)
         if self._arg_type_name:
             arg_type = schema.resolve_type(
-- 
2.43.0

[PATCH v3 13/20] qapi/schema: split "checked" field into "checking" and "checked"

[John Snow]

Instead of using the None value for the members field, use a dedicated
"checking" value to detect recursive misconfigurations.

This is intended to assist with subsequent patches which will seek to
remove the "None" value from the members field (which can never be set
legally after the final call to check()) in order to simplify static
typing of that field, by avoiding needing assertions littered at many
callsites.

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

diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
index d4d3c3bbcee..a459016e148 100644
--- a/scripts/qapi/schema.py
+++ b/scripts/qapi/schema.py
@@ -458,19 +458,21 @@ def __init__(self, name, info, doc, ifcond, features,
         self.local_members = local_members
         self.variants = variants
         self.members = None
+        self._checking = False
 
     def check(self, schema):
         # 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).
-        if self.members is not None:
-            # A previous .check() completed: nothing to do
-            return
-        if self._checked:
+        if self._checking:
             # Recursed: C struct contains itself
             raise QAPISemError(self.info,
                                "object %s contains itself" % self.name)
+        if self._checked:
+            # A previous .check() completed: nothing to do
+            return
 
+        self._checking = True
         super().check(schema)
         assert self._checked and self.members is None
 
@@ -495,7 +497,8 @@ def check(self, schema):
             self.variants.check(schema, seen)
             self.variants.check_clash(self.info, seen)
 
-        self.members = members  # mark completed
+        self.members = members
+        self._checking = False  # mark completed
 
     # 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
-- 
2.43.0

[PATCH v3 14/20] qapi/schema: Don't initialize "members" with `None`

[John Snow]

Declare, but don't initialize the "members" field with type
List[QAPISchemaObjectTypeMember].

This simplifies the typing from what would otherwise be
Optional[List[T]] to merely List[T]. This removes the need to add
assertions to several callsites that this value is not None - which it
never will be after the delayed initialization in check() anyway.

The type declaration without initialization trick will cause accidental
uses of this field prior to full initialization to raise an
AttributeError.

(Note that it is valid to have an empty members list, see the internal
q_empty object as an example. For this reason, we cannot use the empty
list as a replacement test for full initialization and instead rely on
the _checked/_checking fields.)

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

diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
index a459016e148..947e7efb1a8 100644
--- a/scripts/qapi/schema.py
+++ b/scripts/qapi/schema.py
@@ -20,7 +20,7 @@
 from collections import OrderedDict
 import os
 import re
-from typing import List, Optional
+from typing import List, Optional, cast
 
 from .common import (
     POINTER_SUFFIX,
@@ -457,7 +457,7 @@ def __init__(self, name, info, doc, ifcond, features,
         self.base = None
         self.local_members = local_members
         self.variants = variants
-        self.members = None
+        self.members: List[QAPISchemaObjectTypeMember]
         self._checking = False
 
     def check(self, schema):
@@ -474,7 +474,7 @@ def check(self, schema):
 
         self._checking = True
         super().check(schema)
-        assert self._checked and self.members is None
+        assert self._checked
 
         seen = OrderedDict()
         if self._base_name:
@@ -491,7 +491,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
+        # List[QAPISchemaObjectTypeMember]. Cast to the narrower type.
+        members = cast(List[QAPISchemaObjectTypeMember], list(seen.values()))
 
         if self.variants:
             self.variants.check(schema, seen)
@@ -524,11 +527,9 @@ def is_implicit(self):
         return self.name.startswith('q_')
 
     def is_empty(self):
-        assert self.members is not None
         return not self.members and not self.variants
 
     def has_conditional_members(self):
-        assert self.members is not None
         return any(m.ifcond.is_present() for m in self.members)
 
     def c_name(self):
-- 
2.43.0

[PATCH v3 15/20] qapi/schema: fix typing for QAPISchemaVariants.tag_member

[John Snow]

There are two related changes here:

(1) We need to perform type narrowing for resolving the type of
    tag_member during check(), and

(2) tag_member is a delayed initialization field, but we can hide it
    behind a property that raises an Exception if it's called too
    early. This simplifies the typing in quite a few places and avoids
    needing to assert that the "tag_member is not None" at a dozen
    callsites, which can be confusing and suggest the wrong thing to a
    drive-by contributor.

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

diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
index 947e7efb1a8..7508e3a4fa6 100644
--- a/scripts/qapi/schema.py
+++ b/scripts/qapi/schema.py
@@ -635,25 +635,39 @@ def __init__(self, tag_name, info, tag_member, variants):
             assert isinstance(v, QAPISchemaVariant)
         self._tag_name = tag_name
         self.info = info
-        self.tag_member = tag_member
+        self._tag_member: Optional[QAPISchemaObjectTypeMember] = tag_member
         self.variants = variants
 
+    @property
+    def tag_member(self) -> 'QAPISchemaObjectTypeMember':
+        if self._tag_member is None:
+            raise RuntimeError(
+                "QAPISchemaVariants has no tag_member property until "
+                "after check() has been run."
+            )
+        return self._tag_member
+
     def set_defined_in(self, name):
         for v in self.variants:
             v.set_defined_in(name)
 
     def check(self, schema, seen):
         if self._tag_name:      # union
-            self.tag_member = seen.get(c_name(self._tag_name))
+            # We need to narrow the member type:
+            tmp = seen.get(c_name(self._tag_name))
+            assert tmp is None or isinstance(tmp, QAPISchemaObjectTypeMember)
+            self._tag_member = tmp
+
             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 self._tag_member or self._tag_name != self._tag_member.name:
                 raise QAPISemError(
                     self.info,
                     "discriminator '%s' is not a member of %s"
                     % (self._tag_name, base))
             # Here we do:
+            assert self.tag_member.defined_in
             base_type = schema.resolve_type(self.tag_member.defined_in)
             if not base_type.is_implicit():
                 base = "base type '%s'" % self.tag_member.defined_in
@@ -673,11 +687,13 @@ def check(self, schema, seen):
                     "discriminator member '%s' of %s must not be conditional"
                     % (self._tag_name, base))
         else:                   # alternate
+            assert self._tag_member
             assert isinstance(self.tag_member.type, QAPISchemaEnumType)
             assert not self.tag_member.optional
             assert not self.tag_member.ifcond.is_present()
         if self._tag_name:      # union
             # branches that are not explicitly covered get an empty type
+            assert self.tag_member.defined_in
             cases = {v.name for v in self.variants}
             for m in self.tag_member.type.members:
                 if m.name not in cases:
-- 
2.43.0

[PATCH v3 16/20] qapi/schema: assert inner type of QAPISchemaVariants in check_clash()

[John Snow]

QAPISchemaVariant's "variants" field is typed as
List[QAPISchemaVariant], where the typing for QAPISchemaVariant allows
its type field to be any QAPISchemaType.

However, QAPISchemaVariant expects that all of its variants contain the
narrower QAPISchemaObjectType. This relationship is enforced at runtime
in QAPISchemaVariants.check(). This relationship is not embedded in the
type system though, so QAPISchemaVariants.check_clash() needs to
re-assert this property in order to call
QAPISchemaVariant.type.check_clash().

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

diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
index 7508e3a4fa6..4d153b216c0 100644
--- a/scripts/qapi/schema.py
+++ b/scripts/qapi/schema.py
@@ -723,7 +723,10 @@ def check(self, schema, seen):
     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
+            # branch do not affect another branch.
+            #
+            # v.type's typing is enforced in check() above.
+            assert isinstance(v.type, QAPISchemaObjectType)
             v.type.check_clash(info, dict(seen))
 
 
-- 
2.43.0

[PATCH v3 17/20] qapi/parser: demote QAPIExpression to Dict[str, Any]

[John Snow]

Dict[str, object] is a stricter type, but with the way that code is
currently arranged, it is infeasible to enforce this strictness.

In particular, although expr.py's entire raison d'être is normalization
and type-checking of QAPI Expressions, that type information is not
"remembered" in any meaningful way by mypy because each individual
expression is not downcast to a specific expression type that holds all
the details of each expression's unique form.

As a result, all of the code in schema.py that deals with actually
creating type-safe specialized structures has no guarantee (myopically)
that the data it is being passed is correct.

There are two ways to solve this:

(1) Re-assert that the incoming data is in the shape we expect it to be, or
(2) Disable type checking for this data.

(1) is appealing to my sense of strictness, but I gotta concede that it
is asinine to re-check the shape of a QAPIExpression in schema.py when
expr.py has just completed that work at length. The duplication of code
and the nightmare thought of needing to update both locations if and
when we change the shape of these structures makes me extremely
reluctant to go down this route.

(2) allows us the chance to miss updating types in the case that types
are updated in expr.py, but it *is* an awful lot simpler and,
importantly, gets us closer to type checking schema.py *at
all*. Something is better than nothing, I'd argue.

So, do the simpler dumber thing and worry about future strictness
improvements later.

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

diff --git a/scripts/qapi/parser.py b/scripts/qapi/parser.py
index 48cd55a38cc..422de616a35 100644
--- a/scripts/qapi/parser.py
+++ b/scripts/qapi/parser.py
@@ -19,6 +19,7 @@
 import re
 from typing import (
     TYPE_CHECKING,
+    Any,
     Dict,
     List,
     Mapping,
@@ -43,7 +44,7 @@
 _ExprValue = Union[List[object], Dict[str, object], str, bool]
 
 
-class QAPIExpression(Dict[str, object]):
+class QAPIExpression(Dict[str, Any]):
     # pylint: disable=too-few-public-methods
     def __init__(self,
                  data: Mapping[str, object],
-- 
2.43.0

[PATCH v3 18/20] qapi/schema: add type hints

[John Snow]

This patch only adds type hints, which aren't utilized at runtime and
don't change the behavior of this module in any way.

In a scant few locations, type hints are removed where no longer
necessary due to inference power from typing all of the rest of
creation; and any type hints that no longer need string quotes are
changed.

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

diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
index 4d153b216c0..319c8475d21 100644
--- a/scripts/qapi/schema.py
+++ b/scripts/qapi/schema.py
@@ -16,11 +16,21 @@
 
 # TODO catching name collisions in generated code would be nice
 
+from __future__ import annotations
+
 from abc import ABC, abstractmethod
 from collections import OrderedDict
 import os
 import re
-from typing import List, Optional, cast
+from typing import (
+    Any,
+    Callable,
+    Dict,
+    List,
+    Optional,
+    Union,
+    cast,
+)
 
 from .common import (
     POINTER_SUFFIX,
@@ -32,26 +42,30 @@
 )
 from .error import QAPIError, QAPISemError, QAPISourceError
 from .expr import check_exprs
-from .parser import QAPIExpression, QAPISchemaParser
+from .parser import QAPIDoc, QAPIExpression, QAPISchemaParser
+from .source import QAPISourceInfo
 
 
 class QAPISchemaIfCond:
-    def __init__(self, ifcond=None):
+    def __init__(
+        self,
+        ifcond: Optional[Union[str, Dict[str, object]]] = None,
+    ) -> None:
         self.ifcond = ifcond
 
-    def _cgen(self):
+    def _cgen(self) -> str:
         return cgen_ifcond(self.ifcond)
 
-    def gen_if(self):
+    def gen_if(self) -> str:
         return gen_if(self._cgen())
 
-    def gen_endif(self):
+    def gen_endif(self) -> str:
         return gen_endif(self._cgen())
 
-    def docgen(self):
+    def docgen(self) -> str:
         return docgen_ifcond(self.ifcond)
 
-    def is_present(self):
+    def is_present(self) -> bool:
         return bool(self.ifcond)
 
 
@@ -62,8 +76,8 @@ class QAPISchemaEntity:
     This is either a directive, such as include, or a definition.
     The latter uses sub-class `QAPISchemaDefinition`.
     """
-    def __init__(self, info):
-        self._module = None
+    def __init__(self, info: Optional[QAPISourceInfo]):
+        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
@@ -72,37 +86,46 @@ def __init__(self, info):
         self.info = info
         self._checked = False
 
-    def __repr__(self):
+    def __repr__(self) -> str:
         return "<%s at 0x%x>" % (type(self).__name__, id(self))
 
-    def check(self, schema):
+    def check(self, schema: QAPISchema) -> None:
         # pylint: disable=unused-argument
         self._checked = True
 
-    def connect_doc(self, doc=None):
+    def connect_doc(self, doc: Optional[QAPIDoc] = None) -> None:
         pass
 
-    def check_doc(self):
+    def check_doc(self) -> None:
         pass
 
-    def _set_module(self, schema, info):
+    def _set_module(
+        self, schema: QAPISchema, info: Optional[QAPISourceInfo]
+    ) -> None:
         assert self._checked
         fname = info.fname if info else QAPISchemaModule.BUILTIN_MODULE_NAME
         self._module = schema.module_by_fname(fname)
         self._module.add_entity(self)
 
-    def set_module(self, schema):
+    def set_module(self, schema: QAPISchema) -> None:
         self._set_module(schema, self.info)
 
-    def visit(self, visitor):
+    def visit(self, visitor: QAPISchemaVisitor) -> None:
         # pylint: disable=unused-argument
         assert self._checked
 
 
 class QAPISchemaDefinition(QAPISchemaEntity):
-    meta: Optional[str] = None
+    meta: str
 
-    def __init__(self, name: str, info, doc, ifcond=None, features=None):
+    def __init__(
+        self,
+        name: str,
+        info: Optional[QAPISourceInfo],
+        doc: Optional[QAPIDoc],
+        ifcond: Optional[QAPISchemaIfCond] = None,
+        features: Optional[List[QAPISchemaFeature]] = None,
+    ):
         assert isinstance(name, str)
         super().__init__(info)
         for f in features or []:
@@ -113,89 +136,147 @@ def __init__(self, name: str, info, doc, ifcond=None, features=None):
         self._ifcond = ifcond or QAPISchemaIfCond()
         self.features = features or []
 
-    def __repr__(self):
+    def __repr__(self) -> str:
         return "<%s:%s at 0x%x>" % (type(self).__name__, self.name,
                                     id(self))
 
-    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
         super().check(schema)
-        seen = {}
+        seen: Dict[str, QAPISchemaMember] = {}
         for f in self.features:
             f.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
         if doc:
             for f in self.features:
                 doc.connect_feature(f)
 
-    def check_doc(self):
+    def check_doc(self) -> None:
         super().check_doc()
         if self.doc:
             self.doc.check()
 
     @property
-    def ifcond(self):
+    def ifcond(self) -> QAPISchemaIfCond:
         assert self._checked
         return self._ifcond
 
-    def is_implicit(self):
+    def is_implicit(self) -> bool:
         return not self.info
 
-    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: str) -> None:
         pass
 
-    def visit_needed(self, entity):
+    def visit_needed(self, entity: QAPISchemaEntity) -> bool:
         # pylint: disable=unused-argument
         # Default to visiting everything
         return True
 
-    def visit_include(self, name, info):
+    def visit_include(self, name: str, info: Optional[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: QAPISchemaIfCond,
+        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: QAPISchemaIfCond,
+        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: QAPISchemaIfCond,
+        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: QAPISchemaIfCond,
+        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: Optional[QAPISourceInfo],
+        ifcond: QAPISchemaIfCond,
+        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, coroutine):
+    def visit_command(
+        self,
+        name: str,
+        info: Optional[QAPISourceInfo],
+        ifcond: QAPISchemaIfCond,
+        features: List[QAPISchemaFeature],
+        arg_type: Optional[QAPISchemaObjectType],
+        ret_type: Optional[QAPISchemaType],
+        gen: bool,
+        success_response: bool,
+        boxed: bool,
+        allow_oob: bool,
+        allow_preconfig: bool,
+        coroutine: bool,
+    ) -> None:
         pass
 
-    def visit_event(self, name, info, ifcond, features, arg_type, boxed):
+    def visit_event(
+        self,
+        name: str,
+        info: Optional[QAPISourceInfo],
+        ifcond: QAPISchemaIfCond,
+        features: List[QAPISchemaFeature],
+        arg_type: Optional[QAPISchemaObjectType],
+        boxed: bool,
+    ) -> None:
         pass
 
 
@@ -203,9 +284,9 @@ class QAPISchemaModule:
 
     BUILTIN_MODULE_NAME = './builtin'
 
-    def __init__(self, name):
+    def __init__(self, name: str):
         self.name = name
-        self._entity_list = []
+        self._entity_list: List[QAPISchemaEntity] = []
 
     @staticmethod
     def is_system_module(name: str) -> bool:
@@ -234,10 +315,10 @@ def is_builtin_module(cls, name: str) -> bool:
         """
         return name == cls.BUILTIN_MODULE_NAME
 
-    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):
@@ -245,11 +326,11 @@ def visit(self, visitor):
 
 
 class QAPISchemaInclude(QAPISchemaEntity):
-    def __init__(self, sub_module, info):
+    def __init__(self, sub_module: QAPISchemaModule, info: QAPISourceInfo):
         super().__init__(info)
         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)
 
@@ -258,22 +339,22 @@ class QAPISchemaType(QAPISchemaDefinition, ABC):
     # Return the C type for common use.
     # For the types we commonly box, this is a pointer type.
     @abstractmethod
-    def c_type(self):
+    def c_type(self) -> str:
         pass
 
     # 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()
 
     @abstractmethod
-    def json_type(self):
+    def json_type(self) -> str:
         pass
 
-    def alternate_qtype(self):
+    def alternate_qtype(self) -> Optional[str]:
         json2qtype = {
             'null':    'QTYPE_QNULL',
             'string':  'QTYPE_QSTRING',
@@ -285,17 +366,17 @@ def alternate_qtype(self):
         }
         return json2qtype.get(self.json_type())
 
-    def doc_type(self):
+    def doc_type(self) -> Optional[str]:
         if self.is_implicit():
             return None
         return self.name
 
-    def need_has_if_optional(self):
+    def need_has_if_optional(self) -> bool:
         # When FOO is a pointer, has_FOO == !!FOO, i.e. has_FOO is redundant.
         # Except for arrays; see QAPISchemaArrayType.need_has_if_optional().
         return not self.c_type().endswith(POINTER_SUFFIX)
 
-    def check(self, schema):
+    def check(self, schema: QAPISchema) -> None:
         super().check(schema)
         for feat in self.features:
             if feat.is_special():
@@ -303,7 +384,7 @@ def check(self, schema):
                     self.info,
                     f"feature '{feat.name}' is not supported for types")
 
-    def describe(self):
+    def describe(self) -> str:
         assert self.meta
         return "%s type '%s'" % (self.meta, self.name)
 
@@ -311,7 +392,7 @@ def describe(self):
 class QAPISchemaBuiltinType(QAPISchemaType):
     meta = 'built-in'
 
-    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',
@@ -319,24 +400,24 @@ def __init__(self, name, json_type, c_type):
         self._json_type_name = json_type
         self._c_type_name = c_type
 
-    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())
 
@@ -344,7 +425,16 @@ def visit(self, visitor):
 class QAPISchemaEnumType(QAPISchemaType):
     meta = 'enum'
 
-    def __init__(self, name, info, doc, ifcond, features, members, prefix):
+    def __init__(
+        self,
+        name: str,
+        info: Optional[QAPISourceInfo],
+        doc: Optional[QAPIDoc],
+        ifcond: Optional[QAPISchemaIfCond],
+        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)
@@ -353,32 +443,32 @@ def __init__(self, name, info, doc, ifcond, features, members, prefix):
         self.members = members
         self.prefix = prefix
 
-    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._def_predefineds()
         return 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,
@@ -388,60 +478,71 @@ def visit(self, visitor):
 class QAPISchemaArrayType(QAPISchemaType):
     meta = 'array'
 
-    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: QAPISchemaType
 
-    def need_has_if_optional(self):
+    def need_has_if_optional(self) -> bool:
         # When FOO is an array, we still need has_FOO to distinguish
         # absent (!has_FOO) from present and empty (has_FOO && !FOO).
         return True
 
-    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) -> QAPISchemaIfCond:
         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[QAPISchemaIfCond],
+        features: Optional[List[QAPISchemaFeature]],
+        base: Optional[str],
+        local_members: List[QAPISchemaObjectTypeMember],
+        variants: Optional[QAPISchemaVariants],
+    ):
         # struct has local_members, optional base, and no variants
         # union has base, variants, and no local_members
         super().__init__(name, info, doc, ifcond, features)
@@ -460,7 +561,7 @@ def __init__(self, name, info, doc, ifcond, features,
         self.members: List[QAPISchemaObjectTypeMember]
         self._checking = False
 
-    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).
@@ -506,14 +607,18 @@ 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: Optional[QAPISourceInfo],
+        seen: Dict[str, QAPISchemaMember],
+    ) -> None:
         assert self._checked
         for m in self.members:
             m.check_clash(info, seen)
         if self.variants:
             self.variants.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():
@@ -521,32 +626,32 @@ def connect_doc(self, doc=None):
         for m in self.local_members:
             m.connect_doc(doc)
 
-    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:
         return not self.members and not self.variants
 
-    def has_conditional_members(self):
+    def has_conditional_members(self) -> bool:
         return any(m.ifcond.is_present() for m in self.members)
 
-    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,
@@ -559,7 +664,15 @@ def visit(self, visitor):
 class QAPISchemaAlternateType(QAPISchemaType):
     meta = 'alternate'
 
-    def __init__(self, name, info, doc, ifcond, features, variants):
+    def __init__(
+        self,
+        name: str,
+        info: QAPISourceInfo,
+        doc: Optional[QAPIDoc],
+        ifcond: Optional[QAPISchemaIfCond],
+        features: List[QAPISchemaFeature],
+        variants: QAPISchemaVariants,
+    ):
         super().__init__(name, info, doc, ifcond, features)
         assert isinstance(variants, QAPISchemaVariants)
         assert variants.tag_member
@@ -567,7 +680,7 @@ def __init__(self, name, info, doc, ifcond, features, variants):
         variants.tag_member.set_defined_in(self.name)
         self.variants = variants
 
-    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
@@ -575,8 +688,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)
             qtype = v.type.alternate_qtype()
@@ -605,26 +718,32 @@ 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],
+    ):
         # Unions pass tag_name but not tag_member.
         # Alternates pass tag_member but not tag_name.
         # After check(), tag_member is always set.
@@ -635,11 +754,11 @@ def __init__(self, tag_name, info, tag_member, variants):
             assert isinstance(v, QAPISchemaVariant)
         self._tag_name = tag_name
         self.info = info
-        self._tag_member: Optional[QAPISchemaObjectTypeMember] = tag_member
+        self._tag_member = tag_member
         self.variants = variants
 
     @property
-    def tag_member(self) -> 'QAPISchemaObjectTypeMember':
+    def tag_member(self) -> QAPISchemaObjectTypeMember:
         if self._tag_member is None:
             raise RuntimeError(
                 "QAPISchemaVariants has no tag_member property until "
@@ -647,11 +766,13 @@ def tag_member(self) -> 'QAPISchemaObjectTypeMember':
             )
         return self._tag_member
 
-    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 self._tag_name:      # union
             # We need to narrow the member type:
             tmp = seen.get(c_name(self._tag_name))
@@ -720,7 +841,11 @@ 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: Optional[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.
@@ -734,18 +859,27 @@ 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[QAPISchemaIfCond] = None,
+    ):
         assert isinstance(name, str)
         self.name = name
         self.info = info
         self.ifcond = ifcond or QAPISchemaIfCond()
-        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(
@@ -754,11 +888,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: Optional[QAPISourceInfo]) -> str:
         role = self.role
         meta = 'type'
         defined_in = self.defined_in
@@ -790,14 +924,20 @@ def describe(self, info):
 class QAPISchemaEnumMember(QAPISchemaMember):
     role = 'value'
 
-    def __init__(self, name, info, ifcond=None, features=None):
+    def __init__(
+        self,
+        name: str,
+        info: Optional[QAPISourceInfo],
+        ifcond: Optional[QAPISchemaIfCond] = None,
+        features: Optional[List[QAPISchemaFeature]] = None,
+    ):
         super().__init__(name, info, ifcond)
         for f in features or []:
             assert isinstance(f, QAPISchemaFeature)
             f.set_defined_in(name)
         self.features = features or []
 
-    def connect_doc(self, doc):
+    def connect_doc(self, doc: Optional[QAPIDoc]) -> None:
         super().connect_doc(doc)
         if doc:
             for f in self.features:
@@ -807,12 +947,20 @@ def connect_doc(self, doc):
 class QAPISchemaFeature(QAPISchemaMember):
     role = 'feature'
 
-    def is_special(self):
+    def is_special(self) -> bool:
         return self.name in ('deprecated', 'unstable')
 
 
 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[QAPISchemaIfCond] = None,
+        features: Optional[List[QAPISchemaFeature]] = None,
+    ):
         super().__init__(name, info, ifcond)
         assert isinstance(typ, str)
         assert isinstance(optional, bool)
@@ -824,19 +972,19 @@ def __init__(self, name, info, typ, optional, ifcond=None, features=None):
         self.optional = optional
         self.features = features or []
 
-    def need_has(self):
+    def need_has(self) -> bool:
         assert self.type
         return self.optional and self.type.need_has_if_optional()
 
-    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:
@@ -846,24 +994,42 @@ 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: QAPISchemaIfCond,
+    ):
         super().__init__(name, info, typ, False, ifcond)
 
 
 class QAPISchemaCommand(QAPISchemaDefinition):
     meta = 'command'
 
-    def __init__(self, name, info, doc, ifcond, features,
-                 arg_type, ret_type,
-                 gen, success_response, boxed, allow_oob, allow_preconfig,
-                 coroutine):
+    def __init__(
+        self,
+        name: str,
+        info: QAPISourceInfo,
+        doc: Optional[QAPIDoc],
+        ifcond: QAPISchemaIfCond,
+        features: List[QAPISchemaFeature],
+        arg_type: Optional[str],
+        ret_type: Optional[str],
+        gen: bool,
+        success_response: bool,
+        boxed: bool,
+        allow_oob: bool,
+        allow_preconfig: bool,
+        coroutine: 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
@@ -871,7 +1037,7 @@ def __init__(self, name, info, doc, ifcond, features,
         self.allow_preconfig = allow_preconfig
         self.coroutine = coroutine
 
-    def check(self, schema):
+    def check(self, schema: QAPISchema) -> None:
         assert self.info is not None
         super().check(schema)
         if self._arg_type_name:
@@ -907,14 +1073,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,
@@ -926,14 +1092,23 @@ def visit(self, visitor):
 class QAPISchemaEvent(QAPISchemaDefinition):
     meta = 'event'
 
-    def __init__(self, name, info, doc, ifcond, features, arg_type, boxed):
+    def __init__(
+        self,
+        name: str,
+        info: QAPISourceInfo,
+        doc: Optional[QAPIDoc],
+        ifcond: QAPISchemaIfCond,
+        features: List[QAPISchemaFeature],
+        arg_type: Optional[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
 
-    def check(self, schema):
+    def check(self, schema: QAPISchema) -> None:
         super().check(schema)
         if self._arg_type_name:
             typ = schema.resolve_type(
@@ -955,14 +1130,14 @@ def check(self, schema):
                     self.info,
                     "conditional event arguments require 'boxed': true")
 
-    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,
@@ -970,7 +1145,7 @@ def visit(self, visitor):
 
 
 class QAPISchema:
-    def __init__(self, fname):
+    def __init__(self, fname: str):
         self.fname = fname
 
         try:
@@ -982,9 +1157,9 @@ def __init__(self, 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, QAPISchemaDefinition] = {}
+        self._module_dict: Dict[str, QAPISchemaModule] = OrderedDict()
         self._schema_dir = os.path.dirname(fname)
         self._make_module(QAPISchemaModule.BUILTIN_MODULE_NAME)
         self._make_module(fname)
@@ -994,10 +1169,10 @@ def __init__(self, fname):
         self._def_exprs(exprs)
         self.check()
 
-    def _def_entity(self, ent):
+    def _def_entity(self, ent: QAPISchemaEntity) -> None:
         self._entity_list.append(ent)
 
-    def _def_definition(self, defn):
+    def _def_definition(self, defn: QAPISchemaDefinition) -> None:
         # Only the predefined types are allowed to not have info
         assert defn.info or self._predefining
         self._def_entity(defn)
@@ -1014,18 +1189,27 @@ def _def_definition(self, defn):
                 defn.info, "%s is already defined" % other_defn.describe())
         self._entity_dict[defn.name] = defn
 
-    def lookup_entity(self, name, typ=None):
+    def lookup_entity(
+        self,
+        name: str,
+        typ: Optional[type] = 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) -> Optional[QAPISchemaType]:
         typ = self.lookup_entity(name, QAPISchemaType)
         assert typ is None or isinstance(typ, QAPISchemaType)
         return typ
 
-    def resolve_type(self, name, info=None, what=None):
+    def resolve_type(
+        self,
+        name: str,
+        info: Optional[QAPISourceInfo] = None,
+        what: Union[None, str, Callable[[QAPISourceInfo], str]] = None,
+    ) -> QAPISchemaType:
         typ = self.lookup_type(name)
         if not typ:
             assert info and what  # built-in types must not fail lookup
@@ -1040,23 +1224,25 @@ def _module_name(self, fname: str) -> str:
             return fname
         return os.path.relpath(fname, self._schema_dir)
 
-    def _make_module(self, fname):
+    def _make_module(self, fname: 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: str) -> QAPISchemaModule:
         name = self._module_name(fname)
         return self._module_dict[name]
 
-    def _def_include(self, expr: QAPIExpression):
+    def _def_include(self, expr: QAPIExpression) -> None:
         include = expr['include']
         assert expr.doc is None
         self._def_entity(
             QAPISchemaInclude(self._make_module(include), expr.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_definition(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
@@ -1064,7 +1250,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'),
@@ -1093,31 +1279,52 @@ def _def_predefineds(self):
         self._def_definition(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: Optional[QAPISourceInfo],
+    ) -> List[QAPISchemaFeature]:
         if features is None:
             return []
         return [QAPISchemaFeature(f['name'], info,
                                   QAPISchemaIfCond(f.get('if')))
                 for f in features]
 
-    def _make_enum_member(self, name, ifcond, features, info):
+    def _make_enum_member(
+        self,
+        name: str,
+        ifcond: Optional[Union[str, Dict[str, Any]]],
+        features: Optional[List[Dict[str, Any]]],
+        info: Optional[QAPISourceInfo],
+    ) -> QAPISchemaEnumMember:
         return QAPISchemaEnumMember(name, info,
                                     QAPISchemaIfCond(ifcond),
                                     self._make_features(features, info))
 
-    def _make_enum_members(self, values, info):
+    def _make_enum_members(
+        self, values: List[Dict[str, Any]], info: Optional[QAPISourceInfo]
+    ) -> List[QAPISchemaEnumMember]:
         return [self._make_enum_member(v['name'], v.get('if'),
                                        v.get('features'), info)
                 for v in values]
 
-    def _make_array_type(self, element_type, info):
+    def _make_array_type(
+        self, element_type: str, info: Optional[QAPISourceInfo]
+    ) -> str:
         name = element_type + 'List'    # reserved by check_defn_name_str()
         if not self.lookup_type(name):
             self._def_definition(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: QAPISchemaIfCond,
+        role: str,
+        members: List[QAPISchemaObjectTypeMember],
+    ) -> Optional[str]:
         if not members:
             return None
         # See also QAPISchemaObjectTypeMember.describe()
@@ -1133,7 +1340,7 @@ 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: QAPIExpression):
+    def _def_enum_type(self, expr: QAPIExpression) -> None:
         name = expr['enum']
         data = expr['data']
         prefix = expr.get('prefix')
@@ -1144,7 +1351,14 @@ def _def_enum_type(self, expr: QAPIExpression):
             name, info, expr.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: Union[List[str], str],
+        ifcond: QAPISchemaIfCond,
+        features: Optional[List[Dict[str, Any]]],
+        info: QAPISourceInfo,
+    ) -> QAPISchemaObjectTypeMember:
         optional = False
         if name.startswith('*'):
             name = name[1:]
@@ -1155,13 +1369,17 @@ 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, Any],
+        info: QAPISourceInfo,
+    ) -> List[QAPISchemaObjectTypeMember]:
         return [self._make_member(key, value['type'],
                                   QAPISchemaIfCond(value.get('if')),
                                   value.get('features'), info)
                 for (key, value) in data.items()]
 
-    def _def_struct_type(self, expr: QAPIExpression):
+    def _def_struct_type(self, expr: QAPIExpression) -> None:
         name = expr['struct']
         base = expr.get('base')
         data = expr['data']
@@ -1173,13 +1391,19 @@ def _def_struct_type(self, expr: QAPIExpression):
             self._make_members(data, info),
             None))
 
-    def _make_variant(self, case, typ, ifcond, info):
+    def _make_variant(
+        self,
+        case: str,
+        typ: str,
+        ifcond: QAPISchemaIfCond,
+        info: QAPISourceInfo,
+    ) -> QAPISchemaVariant:
         if isinstance(typ, list):
             assert len(typ) == 1
             typ = self._make_array_type(typ[0], info)
         return QAPISchemaVariant(case, info, typ, ifcond)
 
-    def _def_union_type(self, expr: QAPIExpression):
+    def _def_union_type(self, expr: QAPIExpression) -> None:
         name = expr['union']
         base = expr['base']
         tag_name = expr['discriminator']
@@ -1204,7 +1428,7 @@ def _def_union_type(self, expr: QAPIExpression):
                                  QAPISchemaVariants(
                                      tag_name, info, None, variants)))
 
-    def _def_alternate_type(self, expr: QAPIExpression):
+    def _def_alternate_type(self, expr: QAPIExpression) -> None:
         name = expr['alternate']
         data = expr['data']
         assert isinstance(data, dict)
@@ -1222,7 +1446,7 @@ def _def_alternate_type(self, expr: QAPIExpression):
                 name, info, expr.doc, ifcond, features,
                 QAPISchemaVariants(None, info, tag_member, variants)))
 
-    def _def_command(self, expr: QAPIExpression):
+    def _def_command(self, expr: QAPIExpression) -> None:
         name = expr['command']
         data = expr.get('data')
         rets = expr.get('returns')
@@ -1247,7 +1471,7 @@ def _def_command(self, expr: QAPIExpression):
                               rets, gen, success_response, boxed, allow_oob,
                               allow_preconfig, coroutine))
 
-    def _def_event(self, expr: QAPIExpression):
+    def _def_event(self, expr: QAPIExpression) -> None:
         name = expr['event']
         data = expr.get('data')
         boxed = expr.get('boxed', False)
@@ -1261,7 +1485,7 @@ def _def_event(self, expr: QAPIExpression):
         self._def_definition(QAPISchemaEvent(name, info, expr.doc, ifcond,
                                              features, data, boxed))
 
-    def _def_exprs(self, exprs):
+    def _def_exprs(self, exprs: List[QAPIExpression]) -> None:
         for expr in exprs:
             if 'enum' in expr:
                 self._def_enum_type(expr)
@@ -1280,7 +1504,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()
@@ -1288,7 +1512,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.43.0

[PATCH v3 19/20] qapi/schema: turn on mypy strictness

[John Snow]

This patch can be rolled in with the previous one once the series is
ready for merge, but for work-in-progress' sake, it's separate here.

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

diff --git a/scripts/qapi/mypy.ini b/scripts/qapi/mypy.ini
index 56e0dfb1327..8109470a031 100644
--- a/scripts/qapi/mypy.ini
+++ b/scripts/qapi/mypy.ini
@@ -2,8 +2,3 @@
 strict = True
 disallow_untyped_calls = False
 python_version = 3.8
-
-[mypy-qapi.schema]
-disallow_untyped_defs = False
-disallow_incomplete_defs = False
-check_untyped_defs = False
-- 
2.43.0

[PATCH v3 20/20] qapi/schema: remove unnecessary asserts

[John Snow]

With strict typing enabled, these runtime statements aren't necessary
anymore; we can prove them statically.

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

diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
index 319c8475d21..fb0c5d6e2c6 100644
--- a/scripts/qapi/schema.py
+++ b/scripts/qapi/schema.py
@@ -126,10 +126,8 @@ def __init__(
         ifcond: Optional[QAPISchemaIfCond] = None,
         features: Optional[List[QAPISchemaFeature]] = None,
     ):
-        assert isinstance(name, str)
         super().__init__(info)
         for f in features or []:
-            assert isinstance(f, QAPISchemaFeature)
             f.set_defined_in(name)
         self.name = name
         self.doc = doc
@@ -171,7 +169,6 @@ def is_implicit(self) -> bool:
         return not self.info
 
     def describe(self) -> str:
-        assert self.meta
         return "%s '%s'" % (self.meta, self.name)
 
 
@@ -385,7 +382,6 @@ def check(self, schema: QAPISchema) -> None:
                     f"feature '{feat.name}' is not supported for types")
 
     def describe(self) -> str:
-        assert self.meta
         return "%s type '%s'" % (self.meta, self.name)
 
 
@@ -394,7 +390,6 @@ class QAPISchemaBuiltinType(QAPISchemaType):
 
     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',
                              'value')
         self._json_type_name = json_type
@@ -437,9 +432,7 @@ def __init__(
     ):
         super().__init__(name, info, doc, ifcond, features)
         for m in members:
-            assert isinstance(m, QAPISchemaEnumMember)
             m.set_defined_in(name)
-        assert prefix is None or isinstance(prefix, str)
         self.members = members
         self.prefix = prefix
 
@@ -482,7 +475,6 @@ 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: QAPISchemaType
 
@@ -527,7 +519,6 @@ def visit(self, visitor: QAPISchemaVisitor) -> None:
                                  self.element_type)
 
     def describe(self) -> str:
-        assert self.meta
         return "%s type ['%s']" % (self.meta, self._element_type_name)
 
 
@@ -547,12 +538,9 @@ def __init__(
         # union has base, variants, and no local_members
         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)
         if variants is not None:
-            assert isinstance(variants, QAPISchemaVariants)
             variants.set_defined_in(name)
         self._base_name = base
         self.base = None
@@ -674,7 +662,6 @@ def __init__(
         variants: QAPISchemaVariants,
     ):
         super().__init__(name, info, doc, ifcond, features)
-        assert isinstance(variants, QAPISchemaVariants)
         assert variants.tag_member
         variants.set_defined_in(name)
         variants.tag_member.set_defined_in(self.name)
@@ -750,8 +737,6 @@ def __init__(
         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)
         self._tag_name = tag_name
         self.info = info
         self._tag_member = tag_member
@@ -865,7 +850,6 @@ def __init__(
         info: Optional[QAPISourceInfo],
         ifcond: Optional[QAPISchemaIfCond] = None,
     ):
-        assert isinstance(name, str)
         self.name = name
         self.info = info
         self.ifcond = ifcond or QAPISchemaIfCond()
@@ -933,7 +917,6 @@ def __init__(
     ):
         super().__init__(name, info, ifcond)
         for f in features or []:
-            assert isinstance(f, QAPISchemaFeature)
             f.set_defined_in(name)
         self.features = features or []
 
@@ -962,10 +945,7 @@ def __init__(
         features: Optional[List[QAPISchemaFeature]] = None,
     ):
         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)
         self._type_name = typ
         self.type: QAPISchemaType  # set during check()
@@ -973,7 +953,6 @@ def __init__(
         self.features = features or []
 
     def need_has(self) -> bool:
-        assert self.type
         return self.optional and self.type.need_has_if_optional()
 
     def check(self, schema: QAPISchema) -> None:
@@ -1024,8 +1003,6 @@ def __init__(
         coroutine: 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: Optional[QAPISchemaObjectType] = None
         self._ret_type_name = ret_type
@@ -1065,7 +1042,6 @@ def check(self, schema: QAPISchema) -> None:
             if self.name not in self.info.pragma.command_returns_exceptions:
                 typ = self.ret_type
                 if isinstance(typ, QAPISchemaArrayType):
-                    assert typ
                     typ = typ.element_type
                 if not isinstance(typ, QAPISchemaObjectType):
                     raise QAPISemError(
@@ -1103,7 +1079,6 @@ def __init__(
         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: Optional[QAPISchemaObjectType] = None
         self.boxed = boxed
-- 
2.43.0

Re: [PATCH v3 08/20] qapi/schema: add type narrowing to lookup_type()

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> This function is a bit hard to type as-is; mypy needs some assertions to
> assist with the type narrowing.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  scripts/qapi/schema.py | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
> index 043ee7556e6..e617abb03af 100644
> --- a/scripts/qapi/schema.py
> +++ b/scripts/qapi/schema.py
> @@ -997,7 +997,9 @@ def lookup_entity(self, name, typ=None):
       def lookup_entity(self, name, typ=None):
           ent = self._entity_dict.get(name)
           if typ and not isinstance(ent, typ):
               return None
>          return ent
>  
>      def lookup_type(self, name):
> -        return self.lookup_entity(name, QAPISchemaType)
> +        typ = self.lookup_entity(name, QAPISchemaType)
> +        assert typ is None or isinstance(typ, QAPISchemaType)
> +        return typ
>  
>      def resolve_type(self, name, info, what):
>          typ = self.lookup_type(name)

I figure the real trouble-maker is .lookup_entity().

When not passed an optional type argument, it returns QAPISchemaEntity.

When passed an optional type argument, it returns that type or None.

Too cute for type hints to express, I guess.

What if we drop .lookup_entity()'s optional argument?  There are just
three callers:

1. .lookup_type(), visible above.  

       def lookup_type(self, name):
           ent = self.lookup_entity(name)
           if isinstance(ent, QAPISchemaType):
               return ent
           return None

    This should permit typing it as -> Optional[QAPISchemaType] without
    further ado.

2. ._make_implicit_object_type() below

   Uses .lookup_type() to check whether the implicit object type already
   exists.  We figure we could

           typ = self.lookup_entity(name)
           if typ:
               assert(isinstance(typ, QAPISchemaObjectType))
               # The implicit object type has multiple users.  This can

3. QAPIDocDirective.run() doesn't pass a type argument, so no change.

Thoughts?

If you'd prefer not to rock the boat for this series, could it still
make sense as a followup?

Re: [PATCH v3 09/20] qapi/schema: assert resolve_type has 'info' and 'what' args on error

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> resolve_type() is generally used to resolve configuration-provided type
> names into type objects, and generally requires valid 'info' and 'what'
> parameters.
>
> In some cases, such as with QAPISchemaArrayType.check(), resolve_type
> may be used to resolve built-in types and as such will not have an
> 'info' argument, but also must not fail in this scenario.
>
> Use an assertion to sate mypy that we will indeed have 'info' and 'what'
> parameters for the error pathway in resolve_type.
>
> Note: there are only three callsites to resolve_type at present where
> "info" is perceived to be possibly None:

Who is the perceiver?  mypy?

>
>     1) QAPISchemaArrayType.check()
>     2) QAPISchemaObjectTypeMember.check()
>     3) QAPISchemaEvent.check()
>
>     Of those three, only the first actually ever passes None; the other two
>     are limited by their base class initializers which accept info=None, but
>     neither subclass actually use a None value in practice, currently.
>
> 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 e617abb03af..573be7275a6 100644
> --- a/scripts/qapi/schema.py
> +++ b/scripts/qapi/schema.py
> @@ -1004,6 +1004,7 @@ def lookup_type(self, name):
>      def resolve_type(self, name, info, what):
>          typ = self.lookup_type(name)
>          if not typ:
> +            assert info and what  # built-in types must not fail lookup
>              if callable(what):
>                  what = what(info)
>              raise QAPISemError(
<

Re: [PATCH v3 13/20] qapi/schema: split "checked" field into "checking" and "checked"

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> Instead of using the None value for the members field, use a dedicated
> "checking" value to detect recursive misconfigurations.
>
> This is intended to assist with subsequent patches which will seek to
> remove the "None" value from the members field (which can never be set
> legally after the final call to check()) in order to simplify static
> typing of that field, by avoiding needing assertions littered at many
> callsites.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  scripts/qapi/schema.py | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
>
> diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
> index d4d3c3bbcee..a459016e148 100644
> --- a/scripts/qapi/schema.py
> +++ b/scripts/qapi/schema.py
> @@ -458,19 +458,21 @@ def __init__(self, name, info, doc, ifcond, features,
>          self.local_members = local_members
>          self.variants = variants
>          self.members = None
> +        self._checking = False
>  
>      def check(self, schema):
>          # 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).
> -        if self.members is not None:
> -            # A previous .check() completed: nothing to do
> -            return
> -        if self._checked:
> +        if self._checking:
>              # Recursed: C struct contains itself
>              raise QAPISemError(self.info,
>                                 "object %s contains itself" % self.name)
> +        if self._checked:
> +            # A previous .check() completed: nothing to do
> +            return

The diff would be easier to read if you could keep the order...  You
don't due to the subtle change of the state predicates.  More on that
below.

>  
> +        self._checking = True
>          super().check(schema)
>          assert self._checked and self.members is None
>  
> @@ -495,7 +497,8 @@ def check(self, schema):
>              self.variants.check(schema, seen)
>              self.variants.check_clash(self.info, seen)
>  
> -        self.members = members  # mark completed
> +        self.members = members
> +        self._checking = False  # mark completed
>  
>      # 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

We .check() entities on after the other.  *Except*
QAPISchemaObjectType.check() "calls another type T's .check() exactly
when the C struct emitted by gen_object() contains that T's C struct".
If the recursion loops, the schema asks for C structs containing
themselves.  To catch this, we have QAPISchemaType objects go through
three states:

1. Not yet checked.

2. Being checked; object.check() is on the call stack.

3. Checked, i.e. object.check() completed.

How to recognize the states before the patch:

1. not ._checked and .members is None

2. ._checked and .members is None

3. ._checked and .members is not None

   Since .members is not None implies .checked, we simply use
   .members is not None.

We go from state 1. to 2. in super().check().

We go from state 2. to 3. at # mark completed.

Note that .checked becomes true well before we finish checking.  This is
admittedly confusing.  If you can think of a less confusing name, ...

The patch's aim is to avoid use of .members, to enable the next patch.

I don't doubt that your solution works, but trying to understand how it
works makes my tired brain go owww!

State invariants (I think):

1. not ._checked and .members is None and not ._checking

2. ._checked and .members is None and ._checking

3. ._checked and .members is not None and not ._checking

We can then recognize states without use of .members:

1. not ._checked and not ._checking

   Since not ._checked implies not .checking, we can simply use
   not ._checked.

2. ._checked and ._checking

   A deliciously confusing predicate, isn't it?

3. ._checked and not ._checking

Deep breath...  alright, here's the stupidest replacement for use of
.members that could possibly work: add a flag, ensure it's True exactly
when .members is not None.  Like this:

diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
index d4d3c3bbce..095831baf2 100644
--- a/scripts/qapi/schema.py
+++ b/scripts/qapi/schema.py
@@ -458,12 +458,13 @@ def __init__(self, name, info, doc, ifcond, features,
         self.local_members = local_members
         self.variants = variants
         self.members = None
+        self._check_complete = False
 
     def check(self, schema):
         # 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).
-        if self.members is not None:
+        if self._check_complete:
             # A previous .check() completed: nothing to do
             return
         if self._checked:
@@ -495,7 +496,8 @@ def check(self, schema):
             self.variants.check(schema, seen)
             self.variants.check_clash(self.info, seen)
 
-        self.members = members  # mark completed
+        self.members = members
+        self._check_complete = True  # mark completed
 
     # 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


Thoughts?

Re: [PATCH v3 14/20] qapi/schema: Don't initialize "members" with `None`

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> Declare, but don't initialize the "members" field with type
> List[QAPISchemaObjectTypeMember].
>
> This simplifies the typing from what would otherwise be
> Optional[List[T]] to merely List[T]. This removes the need to add
> assertions to several callsites that this value is not None - which it
> never will be after the delayed initialization in check() anyway.
>
> The type declaration without initialization trick will cause accidental
> uses of this field prior to full initialization to raise an
> AttributeError.
>
> (Note that it is valid to have an empty members list, see the internal
> q_empty object as an example. For this reason, we cannot use the empty
> list as a replacement test for full initialization and instead rely on
> the _checked/_checking fields.)
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  scripts/qapi/schema.py | 13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)
>
> diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
> index a459016e148..947e7efb1a8 100644
> --- a/scripts/qapi/schema.py
> +++ b/scripts/qapi/schema.py
> @@ -20,7 +20,7 @@
>  from collections import OrderedDict
>  import os
>  import re
> -from typing import List, Optional
> +from typing import List, Optional, cast
>  
>  from .common import (
>      POINTER_SUFFIX,
> @@ -457,7 +457,7 @@ def __init__(self, name, info, doc, ifcond, features,
>          self.base = None
>          self.local_members = local_members
>          self.variants = variants
> -        self.members = None
> +        self.members: List[QAPISchemaObjectTypeMember]
>          self._checking = False
>  
>      def check(self, schema):
> @@ -474,7 +474,7 @@ def check(self, schema):
>  
>          self._checking = True
>          super().check(schema)
> -        assert self._checked and self.members is None
> +        assert self._checked

This asserts state is "2. Being checked:.

The faithful update would be

           assert self._checked and self._checking

Or with my alternative patch

           assert self._checked and not self._check_complete
>  
>          seen = OrderedDict()
>          if self._base_name:
> @@ -491,7 +491,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
> +        # List[QAPISchemaObjectTypeMember]. Cast to the narrower type.

What do you mean by "check_clash is abstract"?

> +        members = cast(List[QAPISchemaObjectTypeMember], list(seen.values()))

Do we actually need this *now*, or only later when we have more type
hints?

>  
>          if self.variants:
>              self.variants.check(schema, seen)
> @@ -524,11 +527,9 @@ def is_implicit(self):
>          return self.name.startswith('q_')
>  
>      def is_empty(self):
> -        assert self.members is not None

This asserts state is "3. Checked".

The faithful update would be

           assert self._checked and not self._checking

Or with my alternative patch

           assert self._check_complete

>          return not self.members and not self.variants
>  
>      def has_conditional_members(self):
> -        assert self.members is not None

Likewise.

>          return any(m.ifcond.is_present() for m in self.members)
>  
>      def c_name(self):

This patch does two things:

1. Replace test of self.members (enabled by the previous patch)

2. Drop initialization of self.members and simplify the typing

Observation, not demand.  Wouldn't *mind* a split, though :)

Re: [PATCH v3 10/20] qapi: use schema.resolve_type instead of schema.lookup_type

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> the function lookup_type() is capable of returning None, but some
> callers aren't prepared for that and assume it will always succeed. For
> static type analysis purposes, this creates problems at those callsites.
>
> Modify resolve_type() - which already cannot ever return None - to allow
> 'info' and 'what' parameters to be elided, which infers that the type
> lookup is a built-in type lookup.
>
> Change several callsites to use the new form of resolve_type to avoid
> the more laborious strictly-typed error-checking scaffolding:
>
>   ret = lookup_type("foo")
>   assert ret is not None
>   typ: QAPISchemaType = ret
>
> which can be replaced with the simpler:
>
>   typ = resolve_type("foo")
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  scripts/qapi/introspect.py | 4 ++--
>  scripts/qapi/schema.py     | 5 ++---
>  2 files changed, 4 insertions(+), 5 deletions(-)
>
> diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
> index 67c7d89aae0..c38df61a6d5 100644
> --- a/scripts/qapi/introspect.py
> +++ b/scripts/qapi/introspect.py
> @@ -227,10 +227,10 @@ def _use_type(self, typ: QAPISchemaType) -> str:
>  
>          # Map the various integer types to plain int
>          if typ.json_type() == 'int':
> -            typ = self._schema.lookup_type('int')
> +            typ = self._schema.resolve_type('int')
>          elif (isinstance(typ, QAPISchemaArrayType) and
>                typ.element_type.json_type() == 'int'):
> -            typ = self._schema.lookup_type('intList')
> +            typ = self._schema.resolve_type('intList')
>          # Add type to work queue if new
>          if typ not in self._used_types:
>              self._used_types.append(typ)
> diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
> index 573be7275a6..074897ee4fb 100644
> --- a/scripts/qapi/schema.py
> +++ b/scripts/qapi/schema.py
> @@ -650,8 +650,7 @@ def check(self, schema, seen):
>                      "discriminator '%s' is not a member of %s"
>                      % (self._tag_name, base))
>              # Here we do:
> -            base_type = schema.lookup_type(self.tag_member.defined_in)
> -            assert base_type
> +            base_type = schema.resolve_type(self.tag_member.defined_in)
>              if not base_type.is_implicit():
>                  base = "base type '%s'" % self.tag_member.defined_in
>              if not isinstance(self.tag_member.type, QAPISchemaEnumType):
> @@ -1001,7 +1000,7 @@ def lookup_type(self, name):
>          assert typ is None or isinstance(typ, QAPISchemaType)
>          return typ
>  
> -    def resolve_type(self, name, info, what):
> +    def resolve_type(self, name, info=None, what=None):
>          typ = self.lookup_type(name)
>          if not typ:
>              assert info and what  # built-in types must not fail lookup

I still dislike this, but I don't think discussing this more will help.
I can either accept it as the price of all your other work, or come up
with my own solution.

Let's focus on the remainder of the series.

Re: [PATCH v3 15/20] qapi/schema: fix typing for QAPISchemaVariants.tag_member

[Markus Armbruster]

Just to help me keep track of things: I explored an alternative solution
in

    Subject: Re: [PATCH 13/19] qapi/schema: fix typing for QAPISchemaVariants.tag_member 
    Message-ID: <87ttnckz8n.fsf@pond.sub.org>

Re: [PATCH v3 08/20] qapi/schema: add type narrowing to lookup_type()

[John Snow]

On Tue, Feb 20, 2024 at 5:39 AM Markus Armbruster <armbru@redhat.com> wrote:
>
> John Snow <jsnow@redhat.com> writes:
>
> > This function is a bit hard to type as-is; mypy needs some assertions to
> > assist with the type narrowing.
> >
> > Signed-off-by: John Snow <jsnow@redhat.com>
> > ---
> >  scripts/qapi/schema.py | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
> > index 043ee7556e6..e617abb03af 100644
> > --- a/scripts/qapi/schema.py
> > +++ b/scripts/qapi/schema.py
> > @@ -997,7 +997,9 @@ def lookup_entity(self, name, typ=None):
>        def lookup_entity(self, name, typ=None):
>            ent = self._entity_dict.get(name)
>            if typ and not isinstance(ent, typ):
>                return None
> >          return ent
> >
> >      def lookup_type(self, name):
> > -        return self.lookup_entity(name, QAPISchemaType)
> > +        typ = self.lookup_entity(name, QAPISchemaType)
> > +        assert typ is None or isinstance(typ, QAPISchemaType)
> > +        return typ
> >
> >      def resolve_type(self, name, info, what):
> >          typ = self.lookup_type(name)
>
> I figure the real trouble-maker is .lookup_entity().
>
> When not passed an optional type argument, it returns QAPISchemaEntity.
>
> When passed an optional type argument, it returns that type or None.
>
> Too cute for type hints to express, I guess.
>
> What if we drop .lookup_entity()'s optional argument?  There are just
> three callers:
>
> 1. .lookup_type(), visible above.
>
>        def lookup_type(self, name):
>            ent = self.lookup_entity(name)
>            if isinstance(ent, QAPISchemaType):
>                return ent
>            return None
>
>     This should permit typing it as -> Optional[QAPISchemaType] without
>     further ado.
>
> 2. ._make_implicit_object_type() below
>
>    Uses .lookup_type() to check whether the implicit object type already
>    exists.  We figure we could
>
>            typ = self.lookup_entity(name)
>            if typ:
>                assert(isinstance(typ, QAPISchemaObjectType))
>                # The implicit object type has multiple users.  This can
>
> 3. QAPIDocDirective.run() doesn't pass a type argument, so no change.
>
> Thoughts?
>
> If you'd prefer not to rock the boat for this series, could it still
> make sense as a followup?

It makes sense as a follow-up, I think. I had other patches in the
past that attempted to un-cuten these functions and make them more
statically solid, but the shifting sands kept making it easier to put
off until later.

Lemme see if I can just tack this on to the end of the series and see
how it behaves...

Re: [PATCH v3 08/20] qapi/schema: add type narrowing to lookup_type()

[John Snow]

On Mon, Mar 11, 2024 at 2:14 PM John Snow <jsnow@redhat.com> wrote:
>
> On Tue, Feb 20, 2024 at 5:39 AM Markus Armbruster <armbru@redhat.com> wrote:
> >
> > John Snow <jsnow@redhat.com> writes:
> >
> > > This function is a bit hard to type as-is; mypy needs some assertions to
> > > assist with the type narrowing.
> > >
> > > Signed-off-by: John Snow <jsnow@redhat.com>
> > > ---
> > >  scripts/qapi/schema.py | 4 +++-
> > >  1 file changed, 3 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
> > > index 043ee7556e6..e617abb03af 100644
> > > --- a/scripts/qapi/schema.py
> > > +++ b/scripts/qapi/schema.py
> > > @@ -997,7 +997,9 @@ def lookup_entity(self, name, typ=None):
> >        def lookup_entity(self, name, typ=None):
> >            ent = self._entity_dict.get(name)
> >            if typ and not isinstance(ent, typ):
> >                return None
> > >          return ent
> > >
> > >      def lookup_type(self, name):
> > > -        return self.lookup_entity(name, QAPISchemaType)
> > > +        typ = self.lookup_entity(name, QAPISchemaType)
> > > +        assert typ is None or isinstance(typ, QAPISchemaType)
> > > +        return typ
> > >
> > >      def resolve_type(self, name, info, what):
> > >          typ = self.lookup_type(name)
> >
> > I figure the real trouble-maker is .lookup_entity().
> >
> > When not passed an optional type argument, it returns QAPISchemaEntity.
> >
> > When passed an optional type argument, it returns that type or None.
> >
> > Too cute for type hints to express, I guess.
> >
> > What if we drop .lookup_entity()'s optional argument?  There are just
> > three callers:
> >
> > 1. .lookup_type(), visible above.
> >
> >        def lookup_type(self, name):
> >            ent = self.lookup_entity(name)
> >            if isinstance(ent, QAPISchemaType):
> >                return ent
> >            return None
> >
> >     This should permit typing it as -> Optional[QAPISchemaType] without
> >     further ado.
> >
> > 2. ._make_implicit_object_type() below
> >
> >    Uses .lookup_type() to check whether the implicit object type already
> >    exists.  We figure we could
> >
> >            typ = self.lookup_entity(name)
> >            if typ:
> >                assert(isinstance(typ, QAPISchemaObjectType))
> >                # The implicit object type has multiple users.  This can
> >
> > 3. QAPIDocDirective.run() doesn't pass a type argument, so no change.
> >
> > Thoughts?
> >
> > If you'd prefer not to rock the boat for this series, could it still
> > make sense as a followup?
>
> It makes sense as a follow-up, I think. I had other patches in the
> past that attempted to un-cuten these functions and make them more
> statically solid, but the shifting sands kept making it easier to put
> off until later.
>
> Lemme see if I can just tack this on to the end of the series and see
> how it behaves...

Oh, I see what you're doing. Well, I think it's fine if you want to,
but it's also fine to keep this "stricter" method. There's also ways
to type it using mypy's @overload which I've monkey'd with in the
past. Dealer's choice, honestly, but I think I'm eager to just get to
the "fully typed" baseline and then worry about changing more stuff.

Re: [PATCH v3 09/20] qapi/schema: assert resolve_type has 'info' and 'what' args on error

[John Snow]

On Tue, Feb 20, 2024 at 5:48 AM Markus Armbruster <armbru@redhat.com> wrote:
>
> John Snow <jsnow@redhat.com> writes:
>
> > resolve_type() is generally used to resolve configuration-provided type
> > names into type objects, and generally requires valid 'info' and 'what'
> > parameters.
> >
> > In some cases, such as with QAPISchemaArrayType.check(), resolve_type
> > may be used to resolve built-in types and as such will not have an
> > 'info' argument, but also must not fail in this scenario.
> >
> > Use an assertion to sate mypy that we will indeed have 'info' and 'what'
> > parameters for the error pathway in resolve_type.
> >
> > Note: there are only three callsites to resolve_type at present where
> > "info" is perceived to be possibly None:
>
> Who is the perceiver?  mypy?

Deep.

Yes.

>
> >
> >     1) QAPISchemaArrayType.check()
> >     2) QAPISchemaObjectTypeMember.check()
> >     3) QAPISchemaEvent.check()
> >
> >     Of those three, only the first actually ever passes None; the other two
> >     are limited by their base class initializers which accept info=None, but
> >     neither subclass actually use a None value in practice, currently.
> >
> > 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 e617abb03af..573be7275a6 100644
> > --- a/scripts/qapi/schema.py
> > +++ b/scripts/qapi/schema.py
> > @@ -1004,6 +1004,7 @@ def lookup_type(self, name):
> >      def resolve_type(self, name, info, what):
> >          typ = self.lookup_type(name)
> >          if not typ:
> > +            assert info and what  # built-in types must not fail lookup
> >              if callable(what):
> >                  what = what(info)
> >              raise QAPISemError(
> <
>

Re: [PATCH v3 10/20] qapi: use schema.resolve_type instead of schema.lookup_type

[John Snow]

On Tue, Feb 20, 2024 at 10:18 AM Markus Armbruster <armbru@redhat.com> wrote:
>
> John Snow <jsnow@redhat.com> writes:
>
> > the function lookup_type() is capable of returning None, but some
> > callers aren't prepared for that and assume it will always succeed. For
> > static type analysis purposes, this creates problems at those callsites.
> >
> > Modify resolve_type() - which already cannot ever return None - to allow
> > 'info' and 'what' parameters to be elided, which infers that the type
> > lookup is a built-in type lookup.
> >
> > Change several callsites to use the new form of resolve_type to avoid
> > the more laborious strictly-typed error-checking scaffolding:
> >
> >   ret = lookup_type("foo")
> >   assert ret is not None
> >   typ: QAPISchemaType = ret
> >
> > which can be replaced with the simpler:
> >
> >   typ = resolve_type("foo")
> >
> > Signed-off-by: John Snow <jsnow@redhat.com>
> > ---
> >  scripts/qapi/introspect.py | 4 ++--
> >  scripts/qapi/schema.py     | 5 ++---
> >  2 files changed, 4 insertions(+), 5 deletions(-)
> >
> > diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
> > index 67c7d89aae0..c38df61a6d5 100644
> > --- a/scripts/qapi/introspect.py
> > +++ b/scripts/qapi/introspect.py
> > @@ -227,10 +227,10 @@ def _use_type(self, typ: QAPISchemaType) -> str:
> >
> >          # Map the various integer types to plain int
> >          if typ.json_type() == 'int':
> > -            typ = self._schema.lookup_type('int')
> > +            typ = self._schema.resolve_type('int')
> >          elif (isinstance(typ, QAPISchemaArrayType) and
> >                typ.element_type.json_type() == 'int'):
> > -            typ = self._schema.lookup_type('intList')
> > +            typ = self._schema.resolve_type('intList')
> >          # Add type to work queue if new
> >          if typ not in self._used_types:
> >              self._used_types.append(typ)
> > diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
> > index 573be7275a6..074897ee4fb 100644
> > --- a/scripts/qapi/schema.py
> > +++ b/scripts/qapi/schema.py
> > @@ -650,8 +650,7 @@ def check(self, schema, seen):
> >                      "discriminator '%s' is not a member of %s"
> >                      % (self._tag_name, base))
> >              # Here we do:
> > -            base_type = schema.lookup_type(self.tag_member.defined_in)
> > -            assert base_type
> > +            base_type = schema.resolve_type(self.tag_member.defined_in)
> >              if not base_type.is_implicit():
> >                  base = "base type '%s'" % self.tag_member.defined_in
> >              if not isinstance(self.tag_member.type, QAPISchemaEnumType):
> > @@ -1001,7 +1000,7 @@ def lookup_type(self, name):
> >          assert typ is None or isinstance(typ, QAPISchemaType)
> >          return typ
> >
> > -    def resolve_type(self, name, info, what):
> > +    def resolve_type(self, name, info=None, what=None):
> >          typ = self.lookup_type(name)
> >          if not typ:
> >              assert info and what  # built-in types must not fail lookup
>
> I still dislike this, but I don't think discussing this more will help.
> I can either accept it as the price of all your other work, or come up
> with my own solution.
>
> Let's focus on the remainder of the series.

You're absolutely more than welcome to take the series and fork it to
help bring it home - as long as it passes type checking at the end, I
won't really mind what happens to it in the interim :)

Sorry if that comes across as being stubborn, it's more the case that
I genuinely don't think I see your point of view, and feel unable to
target it.

(Sorry.)

--js

Re: [PATCH v3 08/20] qapi/schema: add type narrowing to lookup_type()

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> On Mon, Mar 11, 2024 at 2:14 PM John Snow <jsnow@redhat.com> wrote:
>>
>> On Tue, Feb 20, 2024 at 5:39 AM Markus Armbruster <armbru@redhat.com> wrote:
>> >
>> > John Snow <jsnow@redhat.com> writes:
>> >
>> > > This function is a bit hard to type as-is; mypy needs some assertions to
>> > > assist with the type narrowing.
>> > >
>> > > Signed-off-by: John Snow <jsnow@redhat.com>
>> > > ---
>> > >  scripts/qapi/schema.py | 4 +++-
>> > >  1 file changed, 3 insertions(+), 1 deletion(-)
>> > >
>> > > diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
>> > > index 043ee7556e6..e617abb03af 100644
>> > > --- a/scripts/qapi/schema.py
>> > > +++ b/scripts/qapi/schema.py
>> > > @@ -997,7 +997,9 @@ def lookup_entity(self, name, typ=None):
>> >        def lookup_entity(self, name, typ=None):
>> >            ent = self._entity_dict.get(name)
>> >            if typ and not isinstance(ent, typ):
>> >                return None
>> > >          return ent
>> > >
>> > >      def lookup_type(self, name):
>> > > -        return self.lookup_entity(name, QAPISchemaType)
>> > > +        typ = self.lookup_entity(name, QAPISchemaType)
>> > > +        assert typ is None or isinstance(typ, QAPISchemaType)
>> > > +        return typ
>> > >
>> > >      def resolve_type(self, name, info, what):
>> > >          typ = self.lookup_type(name)
>> >
>> > I figure the real trouble-maker is .lookup_entity().
>> >
>> > When not passed an optional type argument, it returns QAPISchemaEntity.
>> >
>> > When passed an optional type argument, it returns that type or None.
>> >
>> > Too cute for type hints to express, I guess.
>> >
>> > What if we drop .lookup_entity()'s optional argument?  There are just
>> > three callers:
>> >
>> > 1. .lookup_type(), visible above.
>> >
>> >        def lookup_type(self, name):
>> >            ent = self.lookup_entity(name)
>> >            if isinstance(ent, QAPISchemaType):
>> >                return ent
>> >            return None
>> >
>> >     This should permit typing it as -> Optional[QAPISchemaType] without
>> >     further ado.
>> >
>> > 2. ._make_implicit_object_type() below
>> >
>> >    Uses .lookup_type() to check whether the implicit object type already
>> >    exists.  We figure we could
>> >
>> >            typ = self.lookup_entity(name)
>> >            if typ:
>> >                assert(isinstance(typ, QAPISchemaObjectType))
>> >                # The implicit object type has multiple users.  This can
>> >
>> > 3. QAPIDocDirective.run() doesn't pass a type argument, so no change.
>> >
>> > Thoughts?
>> >
>> > If you'd prefer not to rock the boat for this series, could it still
>> > make sense as a followup?
>>
>> It makes sense as a follow-up, I think. I had other patches in the
>> past that attempted to un-cuten these functions and make them more
>> statically solid, but the shifting sands kept making it easier to put
>> off until later.
>>
>> Lemme see if I can just tack this on to the end of the series and see
>> how it behaves...
>
> Oh, I see what you're doing. Well, I think it's fine if you want to,
> but it's also fine to keep this "stricter" method. There's also ways
> to type it using mypy's @overload which I've monkey'd with in the
> past. Dealer's choice, honestly, but I think I'm eager to just get to
> the "fully typed" baseline and then worry about changing more stuff.

That's okay.  However, a good part of the typing exercise's benefit is
the pinpointing of needlessly cute code, i.e. code that could be just as
well be less cute.  To actually reap the benefit, we need to make it
less cute.  If we put it off, we risk to forget.  Acceptable if we take
appropriate steps not to forget.

Re: [PATCH v3 09/20] qapi/schema: assert resolve_type has 'info' and 'what' args on error

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> On Tue, Feb 20, 2024 at 5:48 AM Markus Armbruster <armbru@redhat.com> wrote:
>>
>> John Snow <jsnow@redhat.com> writes:
>>
>> > resolve_type() is generally used to resolve configuration-provided type
>> > names into type objects, and generally requires valid 'info' and 'what'
>> > parameters.
>> >
>> > In some cases, such as with QAPISchemaArrayType.check(), resolve_type
>> > may be used to resolve built-in types and as such will not have an
>> > 'info' argument, but also must not fail in this scenario.
>> >
>> > Use an assertion to sate mypy that we will indeed have 'info' and 'what'
>> > parameters for the error pathway in resolve_type.
>> >
>> > Note: there are only three callsites to resolve_type at present where
>> > "info" is perceived to be possibly None:
>>
>> Who is the perceiver?  mypy?
>
> Deep.
>
> Yes.

Recommend active voice: "where mypy preceives @info to be possibly None".

>>
>> >
>> >     1) QAPISchemaArrayType.check()
>> >     2) QAPISchemaObjectTypeMember.check()
>> >     3) QAPISchemaEvent.check()
>> >
>> >     Of those three, only the first actually ever passes None; the other two
>> >     are limited by their base class initializers which accept info=None, but
>> >     neither subclass actually use a None value in practice, currently.
>> >
>> > 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 e617abb03af..573be7275a6 100644
>> > --- a/scripts/qapi/schema.py
>> > +++ b/scripts/qapi/schema.py
>> > @@ -1004,6 +1004,7 @@ def lookup_type(self, name):
>> >      def resolve_type(self, name, info, what):
>> >          typ = self.lookup_type(name)
>> >          if not typ:
>> > +            assert info and what  # built-in types must not fail lookup
>> >              if callable(what):
>> >                  what = what(info)
>> >              raise QAPISemError(
>> <
>>

Re: [PATCH v3 13/20] qapi/schema: split "checked" field into "checking" and "checked"

[John Snow]

On Tue, Feb 20, 2024 at 8:29 AM Markus Armbruster <armbru@redhat.com> wrote:
>
> John Snow <jsnow@redhat.com> writes:
>
> > Instead of using the None value for the members field, use a dedicated
> > "checking" value to detect recursive misconfigurations.
> >
> > This is intended to assist with subsequent patches which will seek to
> > remove the "None" value from the members field (which can never be set
> > legally after the final call to check()) in order to simplify static
> > typing of that field, by avoiding needing assertions littered at many
> > callsites.
> >
> > Signed-off-by: John Snow <jsnow@redhat.com>
> > ---
> >  scripts/qapi/schema.py | 13 ++++++++-----
> >  1 file changed, 8 insertions(+), 5 deletions(-)
> >
> > diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
> > index d4d3c3bbcee..a459016e148 100644
> > --- a/scripts/qapi/schema.py
> > +++ b/scripts/qapi/schema.py
> > @@ -458,19 +458,21 @@ def __init__(self, name, info, doc, ifcond, features,
> >          self.local_members = local_members
> >          self.variants = variants
> >          self.members = None
> > +        self._checking = False
> >
> >      def check(self, schema):
> >          # 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).
> > -        if self.members is not None:
> > -            # A previous .check() completed: nothing to do
> > -            return
> > -        if self._checked:
> > +        if self._checking:
> >              # Recursed: C struct contains itself
> >              raise QAPISemError(self.info,
> >                                 "object %s contains itself" % self.name)
> > +        if self._checked:
> > +            # A previous .check() completed: nothing to do
> > +            return
>
> The diff would be easier to read if you could keep the order...  You
> don't due to the subtle change of the state predicates.  More on that
> below.
>
> >
> > +        self._checking = True
> >          super().check(schema)
> >          assert self._checked and self.members is None
> >
> > @@ -495,7 +497,8 @@ def check(self, schema):
> >              self.variants.check(schema, seen)
> >              self.variants.check_clash(self.info, seen)
> >
> > -        self.members = members  # mark completed
> > +        self.members = members
> > +        self._checking = False  # mark completed
> >
> >      # 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
>
> We .check() entities on after the other.  *Except*
> QAPISchemaObjectType.check() "calls another type T's .check() exactly
> when the C struct emitted by gen_object() contains that T's C struct".
> If the recursion loops, the schema asks for C structs containing
> themselves.  To catch this, we have QAPISchemaType objects go through
> three states:
>
> 1. Not yet checked.
>
> 2. Being checked; object.check() is on the call stack.
>
> 3. Checked, i.e. object.check() completed.
>
> How to recognize the states before the patch:
>
> 1. not ._checked and .members is None
>
> 2. ._checked and .members is None
>
> 3. ._checked and .members is not None
>
>    Since .members is not None implies .checked, we simply use
>    .members is not None.
>
> We go from state 1. to 2. in super().check().
>
> We go from state 2. to 3. at # mark completed.
>
> Note that .checked becomes true well before we finish checking.  This is
> admittedly confusing.  If you can think of a less confusing name, ...

"checking", of course ;)

I won't change it here, but... that's what I'd be drawn to ...

>
> The patch's aim is to avoid use of .members, to enable the next patch.
>
> I don't doubt that your solution works, but trying to understand how it
> works makes my tired brain go owww!
>
> State invariants (I think):
>
> 1. not ._checked and .members is None and not ._checking
>
> 2. ._checked and .members is None and ._checking
>
> 3. ._checked and .members is not None and not ._checking
>
> We can then recognize states without use of .members:
>
> 1. not ._checked and not ._checking
>
>    Since not ._checked implies not .checking, we can simply use
>    not ._checked.
>
> 2. ._checked and ._checking
>
>    A deliciously confusing predicate, isn't it?
>
> 3. ._checked and not ._checking
>
> Deep breath...  alright, here's the stupidest replacement for use of
> .members that could possibly work: add a flag, ensure it's True exactly
> when .members is not None.  Like this:

OK, sure. As long as the next patch also shakes out just fine...

>
> diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
> index d4d3c3bbce..095831baf2 100644
> --- a/scripts/qapi/schema.py
> +++ b/scripts/qapi/schema.py
> @@ -458,12 +458,13 @@ def __init__(self, name, info, doc, ifcond, features,
>          self.local_members = local_members
>          self.variants = variants
>          self.members = None
> +        self._check_complete = False
>
>      def check(self, schema):
>          # 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).
> -        if self.members is not None:
> +        if self._check_complete:
>              # A previous .check() completed: nothing to do
>              return
>          if self._checked:
> @@ -495,7 +496,8 @@ def check(self, schema):
>              self.variants.check(schema, seen)
>              self.variants.check_clash(self.info, seen)
>
> -        self.members = members  # mark completed
> +        self.members = members
> +        self._check_complete = True  # mark completed
>
>      # 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
>
>
> Thoughts?
>

Re: [PATCH v3 14/20] qapi/schema: Don't initialize "members" with `None`

[John Snow]

On Tue, Feb 20, 2024 at 10:03 AM Markus Armbruster <armbru@redhat.com> wrote:
>
> John Snow <jsnow@redhat.com> writes:
>
> > Declare, but don't initialize the "members" field with type
> > List[QAPISchemaObjectTypeMember].
> >
> > This simplifies the typing from what would otherwise be
> > Optional[List[T]] to merely List[T]. This removes the need to add
> > assertions to several callsites that this value is not None - which it
> > never will be after the delayed initialization in check() anyway.
> >
> > The type declaration without initialization trick will cause accidental
> > uses of this field prior to full initialization to raise an
> > AttributeError.
> >
> > (Note that it is valid to have an empty members list, see the internal
> > q_empty object as an example. For this reason, we cannot use the empty
> > list as a replacement test for full initialization and instead rely on
> > the _checked/_checking fields.)
> >
> > Signed-off-by: John Snow <jsnow@redhat.com>
> > ---
> >  scripts/qapi/schema.py | 13 +++++++------
> >  1 file changed, 7 insertions(+), 6 deletions(-)
> >
> > diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
> > index a459016e148..947e7efb1a8 100644
> > --- a/scripts/qapi/schema.py
> > +++ b/scripts/qapi/schema.py
> > @@ -20,7 +20,7 @@
> >  from collections import OrderedDict
> >  import os
> >  import re
> > -from typing import List, Optional
> > +from typing import List, Optional, cast
> >
> >  from .common import (
> >      POINTER_SUFFIX,
> > @@ -457,7 +457,7 @@ def __init__(self, name, info, doc, ifcond, features,
> >          self.base = None
> >          self.local_members = local_members
> >          self.variants = variants
> > -        self.members = None
> > +        self.members: List[QAPISchemaObjectTypeMember]
> >          self._checking = False
> >
> >      def check(self, schema):
> > @@ -474,7 +474,7 @@ def check(self, schema):
> >
> >          self._checking = True
> >          super().check(schema)
> > -        assert self._checked and self.members is None
> > +        assert self._checked
>
> This asserts state is "2. Being checked:.
>
> The faithful update would be
>
>            assert self._checked and self._checking
>
> Or with my alternative patch
>
>            assert self._checked and not self._check_complete
> >
> >          seen = OrderedDict()
> >          if self._base_name:
> > @@ -491,7 +491,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
> > +        # List[QAPISchemaObjectTypeMember]. Cast to the narrower type.
>
> What do you mean by "check_clash is abstract"?
>
> > +        members = cast(List[QAPISchemaObjectTypeMember], list(seen.values()))
>
> Do we actually need this *now*, or only later when we have more type
> hints?
>
> >
> >          if self.variants:
> >              self.variants.check(schema, seen)
> > @@ -524,11 +527,9 @@ def is_implicit(self):
> >          return self.name.startswith('q_')
> >
> >      def is_empty(self):
> > -        assert self.members is not None
>
> This asserts state is "3. Checked".
>
> The faithful update would be
>
>            assert self._checked and not self._checking
>
> Or with my alternative patch
>
>            assert self._check_complete
>
> >          return not self.members and not self.variants
> >
> >      def has_conditional_members(self):
> > -        assert self.members is not None
>
> Likewise.

Do we even need these assertions, though? If members isn't set, it's
gonna crash anyway. I don't think you actually need them at all. I
think it's fine to leave these changes in this patch and to remove the
assertion as it no longer really guards anything.

>
> >          return any(m.ifcond.is_present() for m in self.members)
> >
> >      def c_name(self):
>
> This patch does two things:
>
> 1. Replace test of self.members (enabled by the previous patch)
>
> 2. Drop initialization of self.members and simplify the typing
>
> Observation, not demand.  Wouldn't *mind* a split, though :)
>

I think maybe one of the assertions can be replaced in the previous
patch, but I think everything else does really belong in this patch.

--js

Re: [PATCH v3 14/20] qapi/schema: Don't initialize "members" with `None`

[John Snow]

On Tue, Feb 20, 2024 at 10:03 AM Markus Armbruster <armbru@redhat.com> wrote:
>
> John Snow <jsnow@redhat.com> writes:
>
> > Declare, but don't initialize the "members" field with type
> > List[QAPISchemaObjectTypeMember].
> >
> > This simplifies the typing from what would otherwise be
> > Optional[List[T]] to merely List[T]. This removes the need to add
> > assertions to several callsites that this value is not None - which it
> > never will be after the delayed initialization in check() anyway.
> >
> > The type declaration without initialization trick will cause accidental
> > uses of this field prior to full initialization to raise an
> > AttributeError.
> >
> > (Note that it is valid to have an empty members list, see the internal
> > q_empty object as an example. For this reason, we cannot use the empty
> > list as a replacement test for full initialization and instead rely on
> > the _checked/_checking fields.)
> >
> > Signed-off-by: John Snow <jsnow@redhat.com>
> > ---
> >  scripts/qapi/schema.py | 13 +++++++------
> >  1 file changed, 7 insertions(+), 6 deletions(-)
> >
> > diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
> > index a459016e148..947e7efb1a8 100644
> > --- a/scripts/qapi/schema.py
> > +++ b/scripts/qapi/schema.py
> > @@ -20,7 +20,7 @@
> >  from collections import OrderedDict
> >  import os
> >  import re
> > -from typing import List, Optional
> > +from typing import List, Optional, cast
> >
> >  from .common import (
> >      POINTER_SUFFIX,
> > @@ -457,7 +457,7 @@ def __init__(self, name, info, doc, ifcond, features,
> >          self.base = None
> >          self.local_members = local_members
> >          self.variants = variants
> > -        self.members = None
> > +        self.members: List[QAPISchemaObjectTypeMember]
> >          self._checking = False
> >
> >      def check(self, schema):
> > @@ -474,7 +474,7 @@ def check(self, schema):
> >
> >          self._checking = True
> >          super().check(schema)
> > -        assert self._checked and self.members is None
> > +        assert self._checked
>
> This asserts state is "2. Being checked:.
>
> The faithful update would be
>
>            assert self._checked and self._checking
>
> Or with my alternative patch
>
>            assert self._checked and not self._check_complete
> >
> >          seen = OrderedDict()
> >          if self._base_name:
> > @@ -491,7 +491,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
> > +        # List[QAPISchemaObjectTypeMember]. Cast to the narrower type.
>
> What do you mean by "check_clash is abstract"?
>
> > +        members = cast(List[QAPISchemaObjectTypeMember], list(seen.values()))
>
> Do we actually need this *now*, or only later when we have more type
> hints?

We need it now: to declare members without initializing it, I need to
declare a type. May as well choose the correct one. This assignment
needs to be the correct type.

>
> >
> >          if self.variants:
> >              self.variants.check(schema, seen)
> > @@ -524,11 +527,9 @@ def is_implicit(self):
> >          return self.name.startswith('q_')
> >
> >      def is_empty(self):
> > -        assert self.members is not None
>
> This asserts state is "3. Checked".
>
> The faithful update would be
>
>            assert self._checked and not self._checking
>
> Or with my alternative patch
>
>            assert self._check_complete
>
> >          return not self.members and not self.variants
> >
> >      def has_conditional_members(self):
> > -        assert self.members is not None
>
> Likewise.
>
> >          return any(m.ifcond.is_present() for m in self.members)
> >
> >      def c_name(self):
>
> This patch does two things:
>
> 1. Replace test of self.members (enabled by the previous patch)
>
> 2. Drop initialization of self.members and simplify the typing
>
> Observation, not demand.  Wouldn't *mind* a split, though :)
>

Re: [PATCH v3 14/20] qapi/schema: Don't initialize "members" with `None`

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> On Tue, Feb 20, 2024 at 10:03 AM Markus Armbruster <armbru@redhat.com> wrote:
>>
>> John Snow <jsnow@redhat.com> writes:
>>
>> > Declare, but don't initialize the "members" field with type
>> > List[QAPISchemaObjectTypeMember].
>> >
>> > This simplifies the typing from what would otherwise be
>> > Optional[List[T]] to merely List[T]. This removes the need to add
>> > assertions to several callsites that this value is not None - which it
>> > never will be after the delayed initialization in check() anyway.
>> >
>> > The type declaration without initialization trick will cause accidental
>> > uses of this field prior to full initialization to raise an
>> > AttributeError.
>> >
>> > (Note that it is valid to have an empty members list, see the internal
>> > q_empty object as an example. For this reason, we cannot use the empty
>> > list as a replacement test for full initialization and instead rely on
>> > the _checked/_checking fields.)
>> >
>> > Signed-off-by: John Snow <jsnow@redhat.com>
>> > ---
>> >  scripts/qapi/schema.py | 13 +++++++------
>> >  1 file changed, 7 insertions(+), 6 deletions(-)
>> >
>> > diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
>> > index a459016e148..947e7efb1a8 100644
>> > --- a/scripts/qapi/schema.py
>> > +++ b/scripts/qapi/schema.py
>> > @@ -20,7 +20,7 @@
>> >  from collections import OrderedDict
>> >  import os
>> >  import re
>> > -from typing import List, Optional
>> > +from typing import List, Optional, cast
>> >
>> >  from .common import (
>> >      POINTER_SUFFIX,
>> > @@ -457,7 +457,7 @@ def __init__(self, name, info, doc, ifcond, features,
>> >          self.base = None
>> >          self.local_members = local_members
>> >          self.variants = variants
>> > -        self.members = None
>> > +        self.members: List[QAPISchemaObjectTypeMember]
>> >          self._checking = False
>> >
>> >      def check(self, schema):
>> > @@ -474,7 +474,7 @@ def check(self, schema):
>> >
>> >          self._checking = True
>> >          super().check(schema)
>> > -        assert self._checked and self.members is None
>> > +        assert self._checked
>>
>> This asserts state is "2. Being checked:.
>>
>> The faithful update would be
>>
>>            assert self._checked and self._checking
>>
>> Or with my alternative patch
>>
>>            assert self._checked and not self._check_complete
>> >
>> >          seen = OrderedDict()
>> >          if self._base_name:
>> > @@ -491,7 +491,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
>> > +        # List[QAPISchemaObjectTypeMember]. Cast to the narrower type.
>>
>> What do you mean by "check_clash is abstract"?
>>
>> > +        members = cast(List[QAPISchemaObjectTypeMember], list(seen.values()))
>>
>> Do we actually need this *now*, or only later when we have more type
>> hints?
>>
>> >
>> >          if self.variants:
>> >              self.variants.check(schema, seen)
>> > @@ -524,11 +527,9 @@ def is_implicit(self):
>> >          return self.name.startswith('q_')
>> >
>> >      def is_empty(self):
>> > -        assert self.members is not None
>>
>> This asserts state is "3. Checked".
>>
>> The faithful update would be
>>
>>            assert self._checked and not self._checking
>>
>> Or with my alternative patch
>>
>>            assert self._check_complete
>>
>> >          return not self.members and not self.variants
>> >
>> >      def has_conditional_members(self):
>> > -        assert self.members is not None
>>
>> Likewise.
>
> Do we even need these assertions, though? If members isn't set, it's
> gonna crash anyway. I don't think you actually need them at all. I
> think it's fine to leave these changes in this patch and to remove the
> assertion as it no longer really guards anything.

When I wrote my review comment, my mind was running on "mechanical
transformation" rails: "the faithful update would be".  The "in state 3"
predicate is .members is not None before the patch, and ._check_complete
afterwards.

You're right, the assertion no longer guards.  It can at best aid
understanding the code.  Feel free to drop it.

Would it make sense to explain the transformation of the "in state N"
predicates briefly in the commit message?

>> >          return any(m.ifcond.is_present() for m in self.members)
>> >
>> >      def c_name(self):
>>
>> This patch does two things:
>>
>> 1. Replace test of self.members (enabled by the previous patch)
>>
>> 2. Drop initialization of self.members and simplify the typing
>>
>> Observation, not demand.  Wouldn't *mind* a split, though :)
>>
>
> I think maybe one of the assertions can be replaced in the previous
> patch, but I think everything else does really belong in this patch.

My observation is that this patch could be split in two, not that parts
of it belong to the previous patch.

Re: [PATCH v3 14/20] qapi/schema: Don't initialize "members" with `None`

[John Snow]

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

On Wed, Mar 13, 2024, 2:57 AM Markus Armbruster <armbru@redhat.com> wrote:

> John Snow <jsnow@redhat.com> writes:
>
> > On Tue, Feb 20, 2024 at 10:03 AM Markus Armbruster <armbru@redhat.com>
> wrote:
> >>
> >> John Snow <jsnow@redhat.com> writes:
> >>
> >> > Declare, but don't initialize the "members" field with type
> >> > List[QAPISchemaObjectTypeMember].
> >> >
> >> > This simplifies the typing from what would otherwise be
> >> > Optional[List[T]] to merely List[T]. This removes the need to add
> >> > assertions to several callsites that this value is not None - which it
> >> > never will be after the delayed initialization in check() anyway.
> >> >
> >> > The type declaration without initialization trick will cause
> accidental
> >> > uses of this field prior to full initialization to raise an
> >> > AttributeError.
> >> >
> >> > (Note that it is valid to have an empty members list, see the internal
> >> > q_empty object as an example. For this reason, we cannot use the empty
> >> > list as a replacement test for full initialization and instead rely on
> >> > the _checked/_checking fields.)
> >> >
> >> > Signed-off-by: John Snow <jsnow@redhat.com>
> >> > ---
> >> >  scripts/qapi/schema.py | 13 +++++++------
> >> >  1 file changed, 7 insertions(+), 6 deletions(-)
> >> >
> >> > diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
> >> > index a459016e148..947e7efb1a8 100644
> >> > --- a/scripts/qapi/schema.py
> >> > +++ b/scripts/qapi/schema.py
> >> > @@ -20,7 +20,7 @@
> >> >  from collections import OrderedDict
> >> >  import os
> >> >  import re
> >> > -from typing import List, Optional
> >> > +from typing import List, Optional, cast
> >> >
> >> >  from .common import (
> >> >      POINTER_SUFFIX,
> >> > @@ -457,7 +457,7 @@ def __init__(self, name, info, doc, ifcond,
> features,
> >> >          self.base = None
> >> >          self.local_members = local_members
> >> >          self.variants = variants
> >> > -        self.members = None
> >> > +        self.members: List[QAPISchemaObjectTypeMember]
> >> >          self._checking = False
> >> >
> >> >      def check(self, schema):
> >> > @@ -474,7 +474,7 @@ def check(self, schema):
> >> >
> >> >          self._checking = True
> >> >          super().check(schema)
> >> > -        assert self._checked and self.members is None
> >> > +        assert self._checked
> >>
> >> This asserts state is "2. Being checked:.
> >>
> >> The faithful update would be
> >>
> >>            assert self._checked and self._checking
> >>
> >> Or with my alternative patch
> >>
> >>            assert self._checked and not self._check_complete
> >> >
> >> >          seen = OrderedDict()
> >> >          if self._base_name:
> >> > @@ -491,7 +491,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
> >> > +        # List[QAPISchemaObjectTypeMember]. Cast to the narrower
> type.
> >>
> >> What do you mean by "check_clash is abstract"?
> >>
> >> > +        members = cast(List[QAPISchemaObjectTypeMember],
> list(seen.values()))
> >>
> >> Do we actually need this *now*, or only later when we have more type
> >> hints?
> >>
> >> >
> >> >          if self.variants:
> >> >              self.variants.check(schema, seen)
> >> > @@ -524,11 +527,9 @@ def is_implicit(self):
> >> >          return self.name.startswith('q_')
> >> >
> >> >      def is_empty(self):
> >> > -        assert self.members is not None
> >>
> >> This asserts state is "3. Checked".
> >>
> >> The faithful update would be
> >>
> >>            assert self._checked and not self._checking
> >>
> >> Or with my alternative patch
> >>
> >>            assert self._check_complete
> >>
> >> >          return not self.members and not self.variants
> >> >
> >> >      def has_conditional_members(self):
> >> > -        assert self.members is not None
> >>
> >> Likewise.
> >
> > Do we even need these assertions, though? If members isn't set, it's
> > gonna crash anyway. I don't think you actually need them at all. I
> > think it's fine to leave these changes in this patch and to remove the
> > assertion as it no longer really guards anything.
>
> When I wrote my review comment, my mind was running on "mechanical
> transformation" rails: "the faithful update would be".  The "in state 3"
> predicate is .members is not None before the patch, and ._check_complete
> afterwards.
>
> You're right, the assertion no longer guards.  It can at best aid
> understanding the code.  Feel free to drop it.
>

I did. I felt it was relevant here instead of in the drop assertions patch
(at the end of the series) because it wasn't doing type narrowing, it's
checking for a value that can truly not occur anymore as of this patch.


> Would it make sense to explain the transformation of the "in state N"
> predicates briefly in the commit message?
>

The wha...?

Oh, if you'd like your invariant analysis in the commit message, please be
my guest and add it with my blessing.


> >> >          return any(m.ifcond.is_present() for m in self.members)
> >> >
> >> >      def c_name(self):
> >>
> >> This patch does two things:
> >>
> >> 1. Replace test of self.members (enabled by the previous patch)
> >>
> >> 2. Drop initialization of self.members and simplify the typing
> >>
> >> Observation, not demand.  Wouldn't *mind* a split, though :)
> >>
> >
> > I think maybe one of the assertions can be replaced in the previous
> > patch, but I think everything else does really belong in this patch.
>
> My observation is that this patch could be split in two, not that parts
> of it belong to the previous patch.
>

What I did was change the assertions in the previous patch (including
moving one from this patch backwards one) such that comparisons to None are
nearly eliminated (the two assertions in is_empty and
has_conditional_members remain, however.)

then this patch:

- changes initialization to declaration by adding the type hint (and
removing the None initial value)
- adds the cast to the seen dict
- removes the two assertions

For mypy not to regress mid-series, I *believe* all of those above items
cannot be split. The patch has become pretty small...

(If you remove the None assertions in the two predicate methods, I think
you either run into a type checking problem *or* you just have a commit
that technically has a bug. Either way, it seems most cohesive to defer
that particular change.)

If you believe it is still semantically two changes, you're welcome to
adjust the commit message or attempt to split it yourself, but I don't
think I see what you do, sorry.

--js

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