[PATCH v2 00/19] qapi: statically type schema.py

[PATCH v2 00/19] qapi: statically type schema.py

[John Snow]

Hi! This branch has some comical name like python-qapi-cleanup-pt6-v2
but, simply, it finishes what I started and statically types all of the
QAPI generator code.

Patches 1-16 fix minor nits or typing issues,
Patch 17 adds type hints with no runtime changes,
Patch 18 turns on type checking with mypy,
Patch 19 drops auxiliary asserts that are no longer needed.

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 (19):
  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: allow resolve_type to be used for built-in types
  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: 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     | 773 +++++++++++++++++++++++++------------
 5 files changed, 523 insertions(+), 273 deletions(-)

-- 
2.43.0

[PATCH v2 01/19] 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..78b63af4df6 100644
--- a/scripts/qapi/pylintrc
+++ b/scripts/qapi/pylintrc
@@ -16,14 +16,14 @@ 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,
         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,
+        fixme,
 
 [REPORTS]
 
-- 
2.43.0

Re: [PATCH v2 01/19] qapi: sort pylint suppressions

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> 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..78b63af4df6 100644
> --- a/scripts/qapi/pylintrc
> +++ b/scripts/qapi/pylintrc
> @@ -16,14 +16,14 @@ 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,
>          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,
> +        fixme,
>  
>  [REPORTS]

Any particular reason for putting fixme last?

Re: [PATCH v2 01/19] qapi: sort pylint suppressions

[John Snow]

On Mon, Jan 15, 2024 at 7:18 AM Markus Armbruster <armbru@redhat.com> wrote:
>
> John Snow <jsnow@redhat.com> writes:
>
> > 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..78b63af4df6 100644
> > --- a/scripts/qapi/pylintrc
> > +++ b/scripts/qapi/pylintrc
> > @@ -16,14 +16,14 @@ 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,
> >          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,
> > +        fixme,
> >
> >  [REPORTS]
>
> Any particular reason for putting fixme last?
>

Uhh. You know, I have no idea? I swore I just used emacs to sort the
lines, but ... lol? You'd think I'd notice that this wasn't in
alphabetical order, but...

*cough* sorry

Re: [PATCH v2 01/19] qapi: sort pylint suppressions

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> On Mon, Jan 15, 2024 at 7:18 AM Markus Armbruster <armbru@redhat.com> wrote:
>>
>> John Snow <jsnow@redhat.com> writes:
>>
>> > 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..78b63af4df6 100644
>> > --- a/scripts/qapi/pylintrc
>> > +++ b/scripts/qapi/pylintrc
>> > @@ -16,14 +16,14 @@ 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,
>> >          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,
>> > +        fixme,
>> >
>> >  [REPORTS]
>>
>> Any particular reason for putting fixme last?
>>
>
> Uhh. You know, I have no idea? I swore I just used emacs to sort the
> lines, but ... lol? You'd think I'd notice that this wasn't in
> alphabetical order, but...
>
> *cough* sorry

No worries, happens to the best of us :)

[PATCH v2 02/19] 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 78b63af4df6..ecf1d56356b 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 v2 03/19] 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 | 117 ++++++++++++++++++++++++-----------------
 1 file changed, 70 insertions(+), 47 deletions(-)

diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
index b7830672e57..e39ed972a80 100644
--- a/scripts/qapi/schema.py
+++ b/scripts/qapi/schema.py
@@ -55,14 +55,14 @@ def is_present(self):
 
 
 class QAPISchemaEntity:
-    meta: Optional[str] = None
+    """
+    QAPISchemaEntity represents all schema entities.
 
-    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
+    Notably, this includes both named and un-named entities, such as
+    include directives. Entities with names are represented by the
+    more specific sub-class `QAPISchemaDefinition` instead.
+    """
+    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,14 +70,50 @@ 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._checked = False
+
+    def __repr__(self):
+        return "<%s at 0x%x>" % (type(self).__name__, id(self))
+
+    def check(self, schema):
+        # pylint: disable=unused-argument
+        self._checked = True
+
+    def connect_doc(self, doc=None):
+        pass
+
+    def check_doc(self):
+        pass
+
+    def _set_module(self, schema, info):
+        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):
+        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 []
-        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))
 
@@ -102,15 +138,6 @@ def check_doc(self):
         if self.doc:
             self.doc.check()
 
-    def _set_module(self, schema, info):
-        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):
-        self._set_module(schema, self.info)
-
     @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,11 +966,12 @@ def __init__(self, fname):
         self.check()
 
     def _def_entity(self, ent):
+        self._entity_list.append(ent)
+
+    def _def_definition(self, ent):
         # 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
+        self._def_entity(ent)
         # 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)
@@ -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,7 @@ 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 +1096,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 +1107,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 +1135,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 +1165,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 +1184,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 +1209,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 +1225,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

Re: [PATCH v2 03/19] qapi: create QAPISchemaDefinition

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> 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 | 117 ++++++++++++++++++++++++-----------------
>  1 file changed, 70 insertions(+), 47 deletions(-)
>
> diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
> index b7830672e57..e39ed972a80 100644
> --- a/scripts/qapi/schema.py
> +++ b/scripts/qapi/schema.py
> @@ -55,14 +55,14 @@ def is_present(self):
>  
>  
>  class QAPISchemaEntity:
> -    meta: Optional[str] = None
> +    """
> +    QAPISchemaEntity represents all schema entities.
>  
> -    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
> +    Notably, this includes both named and un-named entities, such as
> +    include directives. Entities with names are represented by the
> +    more specific sub-class `QAPISchemaDefinition` instead.
> +    """

Hmm.  What about:

       """
       A schema entity.

       This is either a directive, such as include, or a definition.
       The latter use 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,14 +70,50 @@ 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._checked = False
> +
> +    def __repr__(self):
> +        return "<%s at 0x%x>" % (type(self).__name__, id(self))
> +
> +    def check(self, schema):
> +        # pylint: disable=unused-argument
> +        self._checked = True
> +
> +    def connect_doc(self, doc=None):
> +        pass
> +
> +    def check_doc(self):
> +        pass
> +
> +    def _set_module(self, schema, info):
> +        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):
> +        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 []
> -        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))
>  
> @@ -102,15 +138,6 @@ def check_doc(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)

       def check_doc(self):
>          if self.doc:
>              self.doc.check()

No super().FOO()?

>  
> -    def _set_module(self, schema, info):
> -        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):
> -        self._set_module(schema, self.info)
> -
>      @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,11 +966,12 @@ def __init__(self, fname):
>          self.check()
>  
>      def _def_entity(self, ent):
> +        self._entity_list.append(ent)
> +
> +    def _def_definition(self, ent):

Name the argument @defn instead of @ent?

>          # 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
> +        self._def_entity(ent)
>          # 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)
> @@ -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'))

The long identifiers squeeze the (also long) argument list against the
right margin.  What about:

           self._def_definition(QAPISchemaEnumType(
               'QType', None, None, None, None, qtype_values, 'QTYPE'))

or

           self._def_definition(
               QAPISchemaEnumType('QType', None, None, None, None,
                                  qtype_values, 'QTYPE'))

We already use the former style elsewhere, visible below.

You add one in the latter style in the second to last hunk.

Pick one style and stick ot it?

>  
>      def _make_features(self, features, info):
>          if features is None:
> @@ -1057,7 +1081,7 @@ 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))

               self._def_definition(QAPISchemaArrayType(
                   name, info, element_type))

or

               self._def_definition(
                   QAPISchemaArrayType(name, info, element_type))

>          return name
>  
>      def _make_implicit_object_type(self, name, info, ifcond, role, members):
> @@ -1072,7 +1096,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 +1107,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 +1135,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 +1165,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 +1184,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 +1209,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 +1225,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:

Slightly more code, but with slightly fewer conditionals.  Feels a bit
cleaner.

Re: [PATCH v2 03/19] qapi: create QAPISchemaDefinition

[John Snow]

On Mon, Jan 15, 2024 at 8:16 AM Markus Armbruster <armbru@redhat.com> wrote:
>
> John Snow <jsnow@redhat.com> writes:
>
> > 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 | 117 ++++++++++++++++++++++++-----------------
> >  1 file changed, 70 insertions(+), 47 deletions(-)
> >
> > diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
> > index b7830672e57..e39ed972a80 100644
> > --- a/scripts/qapi/schema.py
> > +++ b/scripts/qapi/schema.py
> > @@ -55,14 +55,14 @@ def is_present(self):
> >
> >
> >  class QAPISchemaEntity:
> > -    meta: Optional[str] = None
> > +    """
> > +    QAPISchemaEntity represents all schema entities.
> >
> > -    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
> > +    Notably, this includes both named and un-named entities, such as
> > +    include directives. Entities with names are represented by the
> > +    more specific sub-class `QAPISchemaDefinition` instead.
> > +    """
>
> Hmm.  What about:
>
>        """
>        A schema entity.
>
>        This is either a directive, such as include, or a definition.
>        The latter use sub-class `QAPISchemaDefinition`.
>        """
>

Sure. Key point was just the cookie crumb to the sub-class.

> > +    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,14 +70,50 @@ 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._checked = False
> > +
> > +    def __repr__(self):
> > +        return "<%s at 0x%x>" % (type(self).__name__, id(self))
> > +
> > +    def check(self, schema):
> > +        # pylint: disable=unused-argument
> > +        self._checked = True
> > +
> > +    def connect_doc(self, doc=None):
> > +        pass
> > +
> > +    def check_doc(self):
> > +        pass
> > +
> > +    def _set_module(self, schema, info):
> > +        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):
> > +        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 []
> > -        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))
> >
> > @@ -102,15 +138,6 @@ def check_doc(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)
>
>        def check_doc(self):
> >          if self.doc:
> >              self.doc.check()
>
> No super().FOO()?
>

Ah, just an oversight. It worked out because the super method doesn't
do anything anyway. check() and connect_doc() should also use the
super call, probably.

> >
> > -    def _set_module(self, schema, info):
> > -        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):
> > -        self._set_module(schema, self.info)
> > -
> >      @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,11 +966,12 @@ def __init__(self, fname):
> >          self.check()
> >
> >      def _def_entity(self, ent):
> > +        self._entity_list.append(ent)
> > +
> > +    def _def_definition(self, ent):
>
> Name the argument @defn instead of @ent?
>

OK. (Was aiming for less diffstat, but yes.)

> >          # 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
> > +        self._def_entity(ent)
> >          # 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)
> > @@ -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'))
>
> The long identifiers squeeze the (also long) argument list against the
> right margin.  What about:
>
>            self._def_definition(QAPISchemaEnumType(
>                'QType', None, None, None, None, qtype_values, 'QTYPE'))

This is fine to my eyes.

>
> or
>
>            self._def_definition(
>                QAPISchemaEnumType('QType', None, None, None, None,
>                                   qtype_values, 'QTYPE'))
>
> We already use the former style elsewhere, visible below.
>
> You add one in the latter style in the second to last hunk.
>
> Pick one style and stick ot it?

Yeah. I might try to run the black formatter in the end and just stick
to that, if you don't mind a bit of churn in exchange for having it be
a bit more mindless. It would be a big hassle to run it at the
beginning of the series now, though... but I'll fix this instance for
now.

>
> >
> >      def _make_features(self, features, info):
> >          if features is None:
> > @@ -1057,7 +1081,7 @@ 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))
>
>                self._def_definition(QAPISchemaArrayType(
>                    name, info, element_type))
>
> or
>
>                self._def_definition(
>                    QAPISchemaArrayType(name, info, element_type))
>

OK. (79 columns too long for ya?)

> >          return name
> >
> >      def _make_implicit_object_type(self, name, info, ifcond, role, members):
> > @@ -1072,7 +1096,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 +1107,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 +1135,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 +1165,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 +1184,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 +1209,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 +1225,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:
>
> Slightly more code, but with slightly fewer conditionals.  Feels a bit
> cleaner.
>

Probably a few more asserts and things that can come out, too. It's
nicer for static types at the expense of more OO boilerplate.

Re: [PATCH v2 03/19] qapi: create QAPISchemaDefinition

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> On Mon, Jan 15, 2024 at 8:16 AM Markus Armbruster <armbru@redhat.com> wrote:
>>
>> John Snow <jsnow@redhat.com> writes:
>>
>> > 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 | 117 ++++++++++++++++++++++++-----------------
>> >  1 file changed, 70 insertions(+), 47 deletions(-)
>> >
>> > diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
>> > index b7830672e57..e39ed972a80 100644
>> > --- a/scripts/qapi/schema.py
>> > +++ b/scripts/qapi/schema.py
>> > @@ -55,14 +55,14 @@ def is_present(self):
>> >
>> >
>> >  class QAPISchemaEntity:
>> > -    meta: Optional[str] = None
>> > +    """
>> > +    QAPISchemaEntity represents all schema entities.
>> >
>> > -    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
>> > +    Notably, this includes both named and un-named entities, such as
>> > +    include directives. Entities with names are represented by the
>> > +    more specific sub-class `QAPISchemaDefinition` instead.
>> > +    """
>>
>> Hmm.  What about:
>>
>>        """
>>        A schema entity.
>>
>>        This is either a directive, such as include, or a definition.
>>        The latter use sub-class `QAPISchemaDefinition`.

Or is it "uses"?  Not a native speaker...

>>        """
>>
>
> Sure. Key point was just the cookie crumb to the sub-class.
>
>> > +    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,14 +70,50 @@ 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._checked = False
>> > +
>> > +    def __repr__(self):
>> > +        return "<%s at 0x%x>" % (type(self).__name__, id(self))
>> > +
>> > +    def check(self, schema):
>> > +        # pylint: disable=unused-argument
>> > +        self._checked = True
>> > +
>> > +    def connect_doc(self, doc=None):
>> > +        pass
>> > +
>> > +    def check_doc(self):
>> > +        pass
>> > +
>> > +    def _set_module(self, schema, info):
>> > +        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):
>> > +        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 []
>> > -        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))
>> >
>> > @@ -102,15 +138,6 @@ def check_doc(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)
>>
>>        def check_doc(self):
>> >          if self.doc:
>> >              self.doc.check()
>>
>> No super().FOO()?
>
> Ah, just an oversight. It worked out because the super method doesn't
> do anything anyway. check() and connect_doc() should also use the
> super call, probably.

Yes, please; it's a good habit.

>> >
>> > -    def _set_module(self, schema, info):
>> > -        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):
>> > -        self._set_module(schema, self.info)
>> > -
>> >      @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,11 +966,12 @@ def __init__(self, fname):
>> >          self.check()
>> >
>> >      def _def_entity(self, ent):
>> > +        self._entity_list.append(ent)
>> > +
>> > +    def _def_definition(self, ent):
>>
>> Name the argument @defn instead of @ent?
>
> OK. (Was aiming for less diffstat, but yes.)

Yes, the churn from the rename is annoying.  More annoying than the now
odd name?  Not sure.

>> >          # 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
>> > +        self._def_entity(ent)
>> >          # 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)
>> > @@ -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'))
>>
>> The long identifiers squeeze the (also long) argument list against the
>> right margin.  What about:
>>
>>            self._def_definition(QAPISchemaEnumType(
>>                'QType', None, None, None, None, qtype_values, 'QTYPE'))
>
> This is fine to my eyes.
>
>>
>> or
>>
>>            self._def_definition(
>>                QAPISchemaEnumType('QType', None, None, None, None,
>>                                   qtype_values, 'QTYPE'))
>>
>> We already use the former style elsewhere, visible below.
>>
>> You add one in the latter style in the second to last hunk.
>>
>> Pick one style and stick ot it?
>
> Yeah. I might try to run the black formatter in the end and just stick
> to that, if you don't mind a bit of churn in exchange for having it be
> a bit more mindless. It would be a big hassle to run it at the
> beginning of the series now, though... but I'll fix this instance for
> now.

I gave black a quick try a few months ago: the churn is massive.
Not sure it's worth it.

>> >      def _make_features(self, features, info):
>> >          if features is None:
>> > @@ -1057,7 +1081,7 @@ 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))
>>
>>                self._def_definition(QAPISchemaArrayType(
>>                    name, info, element_type))
>>
>> or
>>
>>                self._def_definition(
>>                    QAPISchemaArrayType(name, info, element_type))
>>
>
> OK. (79 columns too long for ya?)

I generally aim for 70, accept 75 without thought, and longer when the
alternative looks worse.  Deeply indented lines get a bit of extra
leeway.

>> >          return name
>> >
>> >      def _make_implicit_object_type(self, name, info, ifcond, role, members):
>> > @@ -1072,7 +1096,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 +1107,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 +1135,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 +1165,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 +1184,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 +1209,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 +1225,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:
>>
>> Slightly more code, but with slightly fewer conditionals.  Feels a bit
>> cleaner.
>>
>
> Probably a few more asserts and things that can come out, too. It's
> nicer for static types at the expense of more OO boilerplate.

Let's do it.

Re: [PATCH v2 03/19] qapi: create QAPISchemaDefinition

[John Snow]

On Tue, Jan 16, 2024 at 2:22 AM Markus Armbruster <armbru@redhat.com> wrote:
>
> John Snow <jsnow@redhat.com> writes:
>
> > On Mon, Jan 15, 2024 at 8:16 AM Markus Armbruster <armbru@redhat.com> wrote:
> >>
> >> John Snow <jsnow@redhat.com> writes:
> >>
> >> > 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 | 117 ++++++++++++++++++++++++-----------------
> >> >  1 file changed, 70 insertions(+), 47 deletions(-)
> >> >
> >> > diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
> >> > index b7830672e57..e39ed972a80 100644
> >> > --- a/scripts/qapi/schema.py
> >> > +++ b/scripts/qapi/schema.py
> >> > @@ -55,14 +55,14 @@ def is_present(self):
> >> >
> >> >
> >> >  class QAPISchemaEntity:
> >> > -    meta: Optional[str] = None
> >> > +    """
> >> > +    QAPISchemaEntity represents all schema entities.
> >> >
> >> > -    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
> >> > +    Notably, this includes both named and un-named entities, such as
> >> > +    include directives. Entities with names are represented by the
> >> > +    more specific sub-class `QAPISchemaDefinition` instead.
> >> > +    """
> >>
> >> Hmm.  What about:
> >>
> >>        """
> >>        A schema entity.
> >>
> >>        This is either a directive, such as include, or a definition.
> >>        The latter use sub-class `QAPISchemaDefinition`.
>
> Or is it "uses"?  Not a native speaker...

American English: collective nouns are treated as singular ("Congress
has passed a law")
British English: collective nouns are treated as plural ("Parliament
have enacted a new regulation")

>
> >>        """
> >>
> >
> > Sure. Key point was just the cookie crumb to the sub-class.
> >
> >> > +    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,14 +70,50 @@ 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._checked = False
> >> > +
> >> > +    def __repr__(self):
> >> > +        return "<%s at 0x%x>" % (type(self).__name__, id(self))
> >> > +
> >> > +    def check(self, schema):
> >> > +        # pylint: disable=unused-argument
> >> > +        self._checked = True
> >> > +
> >> > +    def connect_doc(self, doc=None):
> >> > +        pass
> >> > +
> >> > +    def check_doc(self):
> >> > +        pass
> >> > +
> >> > +    def _set_module(self, schema, info):
> >> > +        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):
> >> > +        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 []
> >> > -        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))
> >> >
> >> > @@ -102,15 +138,6 @@ def check_doc(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)
> >>
> >>        def check_doc(self):
> >> >          if self.doc:
> >> >              self.doc.check()
> >>
> >> No super().FOO()?
> >
> > Ah, just an oversight. It worked out because the super method doesn't
> > do anything anyway. check() and connect_doc() should also use the
> > super call, probably.
>
> Yes, please; it's a good habit.
>
> >> >
> >> > -    def _set_module(self, schema, info):
> >> > -        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):
> >> > -        self._set_module(schema, self.info)
> >> > -
> >> >      @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,11 +966,12 @@ def __init__(self, fname):
> >> >          self.check()
> >> >
> >> >      def _def_entity(self, ent):
> >> > +        self._entity_list.append(ent)
> >> > +
> >> > +    def _def_definition(self, ent):
> >>
> >> Name the argument @defn instead of @ent?
> >
> > OK. (Was aiming for less diffstat, but yes.)
>
> Yes, the churn from the rename is annoying.  More annoying than the now
> odd name?  Not sure.
>
> >> >          # 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
> >> > +        self._def_entity(ent)
> >> >          # 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)
> >> > @@ -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'))
> >>
> >> The long identifiers squeeze the (also long) argument list against the
> >> right margin.  What about:
> >>
> >>            self._def_definition(QAPISchemaEnumType(
> >>                'QType', None, None, None, None, qtype_values, 'QTYPE'))
> >
> > This is fine to my eyes.
> >
> >>
> >> or
> >>
> >>            self._def_definition(
> >>                QAPISchemaEnumType('QType', None, None, None, None,
> >>                                   qtype_values, 'QTYPE'))
> >>
> >> We already use the former style elsewhere, visible below.
> >>
> >> You add one in the latter style in the second to last hunk.
> >>
> >> Pick one style and stick ot it?
> >
> > Yeah. I might try to run the black formatter in the end and just stick
> > to that, if you don't mind a bit of churn in exchange for having it be
> > a bit more mindless. It would be a big hassle to run it at the
> > beginning of the series now, though... but I'll fix this instance for
> > now.
>
> I gave black a quick try a few months ago: the churn is massive.
> Not sure it's worth it.
>

You can reduce the churn:

black --line-length=79 --skip-string-normalization schema.py

It still churns a lot, but it's a lot less. I like not having to think
about the formatting, but we can worry about that after this series.

> >> >      def _make_features(self, features, info):
> >> >          if features is None:
> >> > @@ -1057,7 +1081,7 @@ 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))
> >>
> >>                self._def_definition(QAPISchemaArrayType(
> >>                    name, info, element_type))
> >>
> >> or
> >>
> >>                self._def_definition(
> >>                    QAPISchemaArrayType(name, info, element_type))
> >>
> >
> > OK. (79 columns too long for ya?)
>
> I generally aim for 70, accept 75 without thought, and longer when the
> alternative looks worse.  Deeply indented lines get a bit of extra
> leeway.
>
> >> >          return name
> >> >
> >> >      def _make_implicit_object_type(self, name, info, ifcond, role, members):
> >> > @@ -1072,7 +1096,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 +1107,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 +1135,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 +1165,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 +1184,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 +1209,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 +1225,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:
> >>
> >> Slightly more code, but with slightly fewer conditionals.  Feels a bit
> >> cleaner.
> >>
> >
> > Probably a few more asserts and things that can come out, too. It's
> > nicer for static types at the expense of more OO boilerplate.
>
> Let's do it.
>

[PATCH v2 04/19] qapi/schema: declare type for QAPISchemaObjectTypeMember.type

[John Snow]

declare, but don't initialize the type of "type" to be QAPISchemaType -
and allow the value to be initialized during check(). This creates a
form of delayed initialization for QAPISchemaType objects where the
static typing only represents the fully-realized object, which occurs
after check() has been called.

This avoids the need for several "assert type is not None" statements
littered throughout the code by asserting it "will always be set."

Note that the static typing information for this object will be
incorrect prior to check() being called. If this field is accessed
before it is initialized in check(), you'll be treated to an
AttributeError exception.

Fixes stuff like this:

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 e39ed972a80..48a51dcd188 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

Re: [PATCH v2 04/19] qapi/schema: declare type for QAPISchemaObjectTypeMember.type

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> declare, but don't initialize the type of "type" to be QAPISchemaType -

Declare

> and allow the value to be initialized during check(). This creates a
> form of delayed initialization for QAPISchemaType objects where the
> static typing only represents the fully-realized object, which occurs
> after check() has been called.
>
> This avoids the need for several "assert type is not None" statements
> littered throughout the code by asserting it "will always be set."

Uh, I find "will always be set" confusing.

I think you mean "cannot be None".

But "be set" can also be read as "will have a value".

It actually doesn't exist until .check(), and once it exists, it cannot
be None.

> Note that the static typing information for this object will be
> incorrect prior to check() being called. If this field is accessed
> before it is initialized in check(), you'll be treated to an
> AttributeError exception.

We could now enter a philosophical debate on whether the static typing
is actually incorrect.  Clearly v: T asserts that the value of @v is
always a T, and the type checker proves this.  Does it also assert that
@v exists?  The type checker doesn't care, and that's a feature.

Maybe start with the problem, and then develop the solution:

  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.

> Fixes stuff like this:
>
> 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 e39ed972a80..48a51dcd188 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 []

Re: [PATCH v2 04/19] qapi/schema: declare type for QAPISchemaObjectTypeMember.type

[John Snow]

On Mon, Jan 15, 2024 at 8:53 AM Markus Armbruster <armbru@redhat.com> wrote:
>
> John Snow <jsnow@redhat.com> writes:
>
> > declare, but don't initialize the type of "type" to be QAPISchemaType -
>
> Declare
>
> > and allow the value to be initialized during check(). This creates a
> > form of delayed initialization for QAPISchemaType objects where the
> > static typing only represents the fully-realized object, which occurs
> > after check() has been called.
> >
> > This avoids the need for several "assert type is not None" statements
> > littered throughout the code by asserting it "will always be set."
>
> Uh, I find "will always be set" confusing.
>
> I think you mean "cannot be None".

Yuh

>
> But "be set" can also be read as "will have a value".

...yuh

>
> It actually doesn't exist until .check(), and once it exists, it cannot
> be None.
>
> > Note that the static typing information for this object will be
> > incorrect prior to check() being called. If this field is accessed
> > before it is initialized in check(), you'll be treated to an
> > AttributeError exception.
>
> We could now enter a philosophical debate on whether the static typing
> is actually incorrect.  Clearly v: T asserts that the value of @v is
> always a T, and the type checker proves this.  Does it also assert that
> @v exists?  The type checker doesn't care, and that's a feature.

*clutches TAPL and cries*

>
> Maybe start with the problem, and then develop the solution:
>
>   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.
>

OK.

--js

> > Fixes stuff like this:
> >
> > 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 e39ed972a80..48a51dcd188 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 []
>

[PATCH v2 05/19] qapi/schema: declare type for QAPISchemaArrayType.element_type

[John Snow]

This field should always be present and defined after check() is
called. Declare the property but allow its initialization to be delayed
until check() so that it can be typed without the use of `Optional`.

This helps simplify typing by avoiding the need to interrogate the value
for None at multiple callsites; the overwhelming majority of uses assume
a fully-initialized object.

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 48a51dcd188..e45d9545eda 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

Re: [PATCH v2 05/19] qapi/schema: declare type for QAPISchemaArrayType.element_type

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> This field should always be present and defined after check() is
> called. Declare the property but allow its initialization to be delayed
> until check() so that it can be typed without the use of `Optional`.
>
> This helps simplify typing by avoiding the need to interrogate the value
> for None at multiple callsites; the overwhelming majority of uses assume
> a fully-initialized object.

If you like my version of the previous patch's commit message, we could
reuse it here.

> 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 48a51dcd188..e45d9545eda 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

Re: [PATCH v2 05/19] qapi/schema: declare type for QAPISchemaArrayType.element_type

[John Snow]

On Mon, Jan 15, 2024 at 8:59 AM Markus Armbruster <armbru@redhat.com> wrote:
>
> John Snow <jsnow@redhat.com> writes:
>
> > This field should always be present and defined after check() is
> > called. Declare the property but allow its initialization to be delayed
> > until check() so that it can be typed without the use of `Optional`.
> >
> > This helps simplify typing by avoiding the need to interrogate the value
> > for None at multiple callsites; the overwhelming majority of uses assume
> > a fully-initialized object.
>
> If you like my version of the previous patch's commit message, we could
> reuse it here.

Sure, I tweaked and re-used it.

--js

>
> > 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 48a51dcd188..e45d9545eda 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
>

[PATCH v2 06/19] 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 | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
index e45d9545eda..8e25dd35562 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,10 +254,11 @@ 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.
-    def c_type(self):
+    @abstractmethod
+    def c_type(self) -> str:
         pass
 
     # Return the C type to be used in a parameter list.
@@ -267,7 +269,8 @@ def c_param_type(self):
     def c_unboxed_type(self):
         return self.c_type()
 
-    def json_type(self):
+    @abstractmethod
+    def json_type(self) -> str:
         pass
 
     def alternate_qtype(self):
-- 
2.43.0

Re: [PATCH v2 06/19] qapi/schema: make c_type() and json_type() abstract methods

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> 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 | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
> index e45d9545eda..8e25dd35562 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,10 +254,11 @@ 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.
> -    def c_type(self):
> +    @abstractmethod
> +    def c_type(self) -> str:

You additionally add the type hint.  Suggest to either mention it in the
commit message, or add it only together with the other type hints in
PATCH 17.

>          pass
>  
>      # Return the C type to be used in a parameter list.
> @@ -267,7 +269,8 @@ def c_param_type(self):
>      def c_unboxed_type(self):
>          return self.c_type()
>  
> -    def json_type(self):
> +    @abstractmethod
> +    def json_type(self) -> str:

Likewise.

>          pass
>  
>      def alternate_qtype(self):

Re: [PATCH v2 06/19] qapi/schema: make c_type() and json_type() abstract methods

[John Snow]

On Mon, Jan 15, 2024 at 9:03 AM Markus Armbruster <armbru@redhat.com> wrote:
>
> John Snow <jsnow@redhat.com> writes:
>
> > 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 | 9 ++++++---
> >  1 file changed, 6 insertions(+), 3 deletions(-)
> >
> > diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
> > index e45d9545eda..8e25dd35562 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,10 +254,11 @@ 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.
> > -    def c_type(self):
> > +    @abstractmethod
> > +    def c_type(self) -> str:
>
> You additionally add the type hint.  Suggest to either mention it in the
> commit message, or add it only together with the other type hints in
> PATCH 17.

Okie-dokey. (moved type hints to the big patch)

>
> >          pass
> >
> >      # Return the C type to be used in a parameter list.
> > @@ -267,7 +269,8 @@ def c_param_type(self):
> >      def c_unboxed_type(self):
> >          return self.c_type()
> >
> > -    def json_type(self):
> > +    @abstractmethod
> > +    def json_type(self) -> str:
>
> Likewise.
>
> >          pass
> >
> >      def alternate_qtype(self):
>

[PATCH v2 07/19] 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 8e25dd35562..775766c0135 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 v2 08/19] 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 775766c0135..66a78f28fd4 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 v2 09/19] qapi/schema: allow resolve_type to be used for built-in types

[John Snow]

allow resolve_type to be used for both built-in and user-specified
type definitions. In the event that the type cannot be resolved, assert
that 'info' and 'what' were both provided in order to create a usable
QAPISemError.

In practice, 'info' will only be None for built-in definitions, which
*should not fail* type lookup.

As a convenience, allow the 'what' and 'info' parameters to be elided
entirely so that it can be used as a can-not-fail version of
lookup_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 actually use it in practice.

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

diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
index 66a78f28fd4..a77b51d1b96 100644
--- a/scripts/qapi/schema.py
+++ b/scripts/qapi/schema.py
@@ -1001,9 +1001,10 @@ 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
             if callable(what):
                 what = what(info)
             raise QAPISemError(
-- 
2.43.0

Re: [PATCH v2 09/19] qapi/schema: allow resolve_type to be used for built-in types

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> allow resolve_type to be used for both built-in and user-specified
> type definitions. In the event that the type cannot be resolved, assert
> that 'info' and 'what' were both provided in order to create a usable
> QAPISemError.
>
> In practice, 'info' will only be None for built-in definitions, which
> *should not fail* type lookup.
>
> As a convenience, allow the 'what' and 'info' parameters to be elided
> entirely so that it can be used as a can-not-fail version of
> lookup_type.

The convenience remains unused until the next patch.  It should be added
there.

> 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;

Yes.  More below.

>                                                               the other two
>     are limited by their base class initializers which accept info=None, but

They do?

>     neither actually use it in practice.
>
> Signed-off-by: John Snow <jsnow@redhat.com>

Hmm.

We look up types by name in two ways:

1. Failure is a semantic error

   Use .resolve_type(), passing real @info and @what.

   Users:

   * QAPISchemaArrayType.check() resolving the element type

     Fine print: when the array type is built-in, we pass None @info and
     @what.  The built-in array type's element type must exist for
     .resolve_type() to work.  This commit changes .resolve_type() to
     assert it does.

   * QAPISchemaObjectType.check() resolving the base type

   * QAPISchemaObjectTypeMember.check() resolving the member type

   * QAPISchemaCommand.check() resolving argument type (if named) and
     return type (which is always named).

   * QAPISchemaEvent.check() resolving argument type (if named).

   Note all users are in .check() methods.  That's where type named get
   resolved.

2. Handle failure

   Use .lookup_type(), which returns None when the named type doesn't
   exist.

   Users:

   * QAPISchemaVariants.check(), to look up the base type containing the
     tag member for error reporting purposes.  Failure would be a
     programming error.

   * .resolve_type(), which handles failure as semantic error

   * ._make_array_type(), which uses it as "type exists already"
      predicate.

   * QAPISchemaGenIntrospectVisitor._use_type(), to look up certain
     built-in types.  Failure would be a programming error.

The next commit switches the uses where failure would be a programming
error from .lookup_type() to .resolve_type() without @info and @what, so
failure trips its assertion.  I don't like it, because it overloads
.resolve_type() to serve two rather different use cases:

1. Failure is a semantic error; pass @info and @what

2. Failure is a programming error; don't pass @info and what

The odd one out is of course QAPISchemaArrayType.check(), which wants to
use 1. for the user's types and 2. for built-in types.  Let's ignore it
for a second.

I prefer to do 2. like typ = .lookup_type(); assert typ.  We can factor
this out into its own helper if that helps (pardon the pun).

Back to QAPISchemaArrayType.check().  Its need to resolve built-in
element types, which have no info, necessitates .resolve_type() taking
Optional[QAPISourceInfo].  This might bother you.  It doesn't bother me,
unless it leads to mypy complications I can't see.

We can simply leave it as is.  Adding the assertion to .resolve_type()
is fine.

Ot we complicate QAPISchemaArrayType.check() to simplify
.resolve_type()'s typing, roughly like this:

            if self.info:
                self.element_type = schema.resolve_type(
                    self._element_type_name,
                    self.info, self.info.defn_meta)
            else:               # built-in type
                self.element_type = schema.lookup_type(
                    self._element_type_name)
                assert self.element_type

Not sure it's worth the trouble.  Thoughts?

> ---
>  scripts/qapi/schema.py | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
> index 66a78f28fd4..a77b51d1b96 100644
> --- a/scripts/qapi/schema.py
> +++ b/scripts/qapi/schema.py
> @@ -1001,9 +1001,10 @@ 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
>              if callable(what):
>                  what = what(info)
>              raise QAPISemError(

Re: [PATCH v2 09/19] qapi/schema: allow resolve_type to be used for built-in types

[John Snow]

On Tue, Jan 16, 2024 at 6:09 AM Markus Armbruster <armbru@redhat.com> wrote:
>
> John Snow <jsnow@redhat.com> writes:
>
> > allow resolve_type to be used for both built-in and user-specified
> > type definitions. In the event that the type cannot be resolved, assert
> > that 'info' and 'what' were both provided in order to create a usable
> > QAPISemError.
> >
> > In practice, 'info' will only be None for built-in definitions, which
> > *should not fail* type lookup.
> >
> > As a convenience, allow the 'what' and 'info' parameters to be elided
> > entirely so that it can be used as a can-not-fail version of
> > lookup_type.
>
> The convenience remains unused until the next patch.  It should be added
> there.

Okie-ducky.

>
> > 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;
>
> Yes.  More below.

Scary...

>
> >                                                               the other two
> >     are limited by their base class initializers which accept info=None, but
>
> They do?
>

In the case of QAPISchemaObjectTypeMember, the parent class
QAPISchemaMember allows initialization with info=None. I can't fully
trace all of the callsites, but one of them at least is in types.py:

>     enum_members = members + [QAPISchemaEnumMember('_MAX', None)]

which necessitates, for now, info-less QAPISchemaEnumMember, which
necessitates info-less QAPISchemaMember. There are others, etc.

> >     neither actually use it in practice.
> >
> > Signed-off-by: John Snow <jsnow@redhat.com>
>
> Hmm.

Scary.

>
> We look up types by name in two ways:
>
> 1. Failure is a semantic error
>
>    Use .resolve_type(), passing real @info and @what.
>
>    Users:
>
>    * QAPISchemaArrayType.check() resolving the element type
>
>      Fine print: when the array type is built-in, we pass None @info and
>      @what.  The built-in array type's element type must exist for
>      .resolve_type() to work.  This commit changes .resolve_type() to
>      assert it does.
>
>    * QAPISchemaObjectType.check() resolving the base type
>
>    * QAPISchemaObjectTypeMember.check() resolving the member type
>
>    * QAPISchemaCommand.check() resolving argument type (if named) and
>      return type (which is always named).
>
>    * QAPISchemaEvent.check() resolving argument type (if named).
>
>    Note all users are in .check() methods.  That's where type named get
>    resolved.
>
> 2. Handle failure
>
>    Use .lookup_type(), which returns None when the named type doesn't
>    exist.
>
>    Users:
>
>    * QAPISchemaVariants.check(), to look up the base type containing the
>      tag member for error reporting purposes.  Failure would be a
>      programming error.
>
>    * .resolve_type(), which handles failure as semantic error
>
>    * ._make_array_type(), which uses it as "type exists already"
>       predicate.
>
>    * QAPISchemaGenIntrospectVisitor._use_type(), to look up certain
>      built-in types.  Failure would be a programming error.
>
> The next commit switches the uses where failure would be a programming
> error from .lookup_type() to .resolve_type() without @info and @what, so
> failure trips its assertion.  I don't like it, because it overloads
> .resolve_type() to serve two rather different use cases:
>
> 1. Failure is a semantic error; pass @info and @what
>
> 2. Failure is a programming error; don't pass @info and what
>
> The odd one out is of course QAPISchemaArrayType.check(), which wants to
> use 1. for the user's types and 2. for built-in types.  Let's ignore it
> for a second.

"Let's ignore what motivated this patch" aww...

>
> I prefer to do 2. like typ = .lookup_type(); assert typ.  We can factor
> this out into its own helper if that helps (pardon the pun).
>
> Back to QAPISchemaArrayType.check().  Its need to resolve built-in
> element types, which have no info, necessitates .resolve_type() taking
> Optional[QAPISourceInfo].  This might bother you.  It doesn't bother me,
> unless it leads to mypy complications I can't see.

Well, with this patch I allowed it to take Optional[QAPISourceInfo] -
just keep in mind that QAPISemError *requires* an info object, even
though the typing there is also Optional[QAPISourceInfo] ... It will
assert that info is present in __str__.

Actually, I'd love to change that too - and make it fully required -
but since built-in types have no info, there's too many places I'd
need to change to enforce this as a static type.

Still.

>
> We can simply leave it as is.  Adding the assertion to .resolve_type()
> is fine.
>
> Ot we complicate QAPISchemaArrayType.check() to simplify
> .resolve_type()'s typing, roughly like this:
>
>             if self.info:
>                 self.element_type = schema.resolve_type(
>                     self._element_type_name,
>                     self.info, self.info.defn_meta)
>             else:               # built-in type
>                 self.element_type = schema.lookup_type(
>                     self._element_type_name)
>                 assert self.element_type
>
> Not sure it's worth the trouble.  Thoughts?

I suppose it's your call, ultimately. This patch exists primarily to
help in two places:

(A) QAPISchemaArrayType.check(), as you've noticed, because it uses
the same path for both built-in and user-defined types. This is the
only place in the code where this occurs *at the moment*, but I can't
predict the future.

(B) Calls to lookup_type in introspect.py which look up built-in types
and must-not-fail. It was cumbersome in the old patchset, but this one
makes it simpler.

I suppose at the moment, having the assert directly in resolve_type
just means we get to use the same helper/pathway for both user-defined
and built-in types, which matches the infrastructure we already have,
which doesn't differentiate between the two. (By which I mean, all of
the Schema classes are not split into built-in and user-defined types,
so it is invisible to the type system.)

I could add conditional logic to the array check, and leave the
lookup_type calls in introspect.py being a little cumbersome - my main
concern with that solution is that I might be leaving a nasty
booby-trap in the future if someone wants to add a new built-in type
or something gets refactored to share more code pathways. Maybe that's
not fully rational, but it's why I went the way I did.

(P.S. I still violently want to create an info object that represents
built-in definitions so I can just get rid of all the
Optional[QAPISourceInfo] types from everywhere. I know I tried to do
it before and you vetoed it, but the desire lives on in my heart.)

>
> > ---
> >  scripts/qapi/schema.py | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
> > index 66a78f28fd4..a77b51d1b96 100644
> > --- a/scripts/qapi/schema.py
> > +++ b/scripts/qapi/schema.py
> > @@ -1001,9 +1001,10 @@ 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
> >              if callable(what):
> >                  what = what(info)
> >              raise QAPISemError(
>

Re: [PATCH v2 09/19] qapi/schema: allow resolve_type to be used for built-in types

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> On Tue, Jan 16, 2024 at 6:09 AM Markus Armbruster <armbru@redhat.com> wrote:
>>
>> John Snow <jsnow@redhat.com> writes:
>>
>> > allow resolve_type to be used for both built-in and user-specified
>> > type definitions. In the event that the type cannot be resolved, assert
>> > that 'info' and 'what' were both provided in order to create a usable
>> > QAPISemError.
>> >
>> > In practice, 'info' will only be None for built-in definitions, which
>> > *should not fail* type lookup.
>> >
>> > As a convenience, allow the 'what' and 'info' parameters to be elided
>> > entirely so that it can be used as a can-not-fail version of
>> > lookup_type.
>>
>> The convenience remains unused until the next patch.  It should be added
>> there.
>
> Okie-ducky.
>
>>
>> > 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;
>>
>> Yes.  More below.
>
> Scary...

I know...

>> >                                                               the other two
>> >     are limited by their base class initializers which accept info=None, but
>>
>> They do?
>
> In the case of QAPISchemaObjectTypeMember, the parent class
> QAPISchemaMember allows initialization with info=None. I can't fully
> trace all of the callsites, but one of them at least is in types.py:
>
>>     enum_members = members + [QAPISchemaEnumMember('_MAX', None)]

I see.

We may want to do the _MAX thingy differently.  Not now.

> which necessitates, for now, info-less QAPISchemaEnumMember, which
> necessitates info-less QAPISchemaMember. There are others, etc.

Overriding an inherited attribute of type Optional[T] so it's
non-optional T makes mypy unhappy?

>> >     neither actually use it in practice.
>> >
>> > Signed-off-by: John Snow <jsnow@redhat.com>
>>
>> Hmm.
>
> Scary.
>
>>
>> We look up types by name in two ways:
>>
>> 1. Failure is a semantic error
>>
>>    Use .resolve_type(), passing real @info and @what.
>>
>>    Users:
>>
>>    * QAPISchemaArrayType.check() resolving the element type
>>
>>      Fine print: when the array type is built-in, we pass None @info and
>>      @what.  The built-in array type's element type must exist for
>>      .resolve_type() to work.  This commit changes .resolve_type() to
>>      assert it does.
>>
>>    * QAPISchemaObjectType.check() resolving the base type
>>
>>    * QAPISchemaObjectTypeMember.check() resolving the member type
>>
>>    * QAPISchemaCommand.check() resolving argument type (if named) and
>>      return type (which is always named).
>>
>>    * QAPISchemaEvent.check() resolving argument type (if named).
>>
>>    Note all users are in .check() methods.  That's where type named get
>>    resolved.
>>
>> 2. Handle failure
>>
>>    Use .lookup_type(), which returns None when the named type doesn't
>>    exist.
>>
>>    Users:
>>
>>    * QAPISchemaVariants.check(), to look up the base type containing the
>>      tag member for error reporting purposes.  Failure would be a
>>      programming error.
>>
>>    * .resolve_type(), which handles failure as semantic error
>>
>>    * ._make_array_type(), which uses it as "type exists already"
>>       predicate.
>>
>>    * QAPISchemaGenIntrospectVisitor._use_type(), to look up certain
>>      built-in types.  Failure would be a programming error.
>>
>> The next commit switches the uses where failure would be a programming
>> error from .lookup_type() to .resolve_type() without @info and @what, so
>> failure trips its assertion.  I don't like it, because it overloads
>> .resolve_type() to serve two rather different use cases:
>>
>> 1. Failure is a semantic error; pass @info and @what
>>
>> 2. Failure is a programming error; don't pass @info and what
>>
>> The odd one out is of course QAPISchemaArrayType.check(), which wants to
>> use 1. for the user's types and 2. for built-in types.  Let's ignore it
>> for a second.
>
> "Let's ignore what motivated this patch" aww...

Just for a second, I swear!

>> I prefer to do 2. like typ = .lookup_type(); assert typ.  We can factor
>> this out into its own helper if that helps (pardon the pun).
>>
>> Back to QAPISchemaArrayType.check().  Its need to resolve built-in
>> element types, which have no info, necessitates .resolve_type() taking
>> Optional[QAPISourceInfo].  This might bother you.  It doesn't bother me,
>> unless it leads to mypy complications I can't see.
>
> Well, with this patch I allowed it to take Optional[QAPISourceInfo] -
> just keep in mind that QAPISemError *requires* an info object, even
> though the typing there is also Optional[QAPISourceInfo] ... It will
> assert that info is present in __str__.
>
> Actually, I'd love to change that too - and make it fully required -
> but since built-in types have no info, there's too many places I'd
> need to change to enforce this as a static type.
>
> Still.

Invariant: no error reports for built-in types.

Checked since forever by asserting info is not None, exploiting the fact
that info is None exactly for built-in types.

This makes info: Optional[QAPISourceInfo] by design.

Works.

Specializing it to just QAPISourceInfo moves the assertion check from
run time to compile time.  Might give a nice feeling, but I don't think
it's practical everywhere, and it doesn't really matter anyway.

Using a special value of QAPISourceInfo instead of None would also get
rid of the Optional, along with the potential of checking at compile
time.  Good trade *if* it simplifies the code.  See also the very end of
my reply.

>> We can simply leave it as is.  Adding the assertion to .resolve_type()
>> is fine.
>>
>> Ot we complicate QAPISchemaArrayType.check() to simplify
>> .resolve_type()'s typing, roughly like this:
>>
>>             if self.info:
>>                 self.element_type = schema.resolve_type(
>>                     self._element_type_name,
>>                     self.info, self.info.defn_meta)
>>             else:               # built-in type
>>                 self.element_type = schema.lookup_type(
>>                     self._element_type_name)
>>                 assert self.element_type
>>
>> Not sure it's worth the trouble.  Thoughts?
>
> I suppose it's your call, ultimately. This patch exists primarily to
> help in two places:
>
> (A) QAPISchemaArrayType.check(), as you've noticed, because it uses
> the same path for both built-in and user-defined types. This is the
> only place in the code where this occurs *at the moment*, but I can't
> predict the future.
>
> (B) Calls to lookup_type in introspect.py which look up built-in types
> and must-not-fail. It was cumbersome in the old patchset, but this one
> makes it simpler.
>
> I suppose at the moment, having the assert directly in resolve_type
> just means we get to use the same helper/pathway for both user-defined
> and built-in types, which matches the infrastructure we already have,
> which doesn't differentiate between the two. (By which I mean, all of
> the Schema classes are not split into built-in and user-defined types,
> so it is invisible to the type system.)

Yes.

> I could add conditional logic to the array check, and leave the
> lookup_type calls in introspect.py being a little cumbersome - my main
> concern with that solution is that I might be leaving a nasty
> booby-trap in the future if someone wants to add a new built-in type
> or something gets refactored to share more code pathways. Maybe that's
> not fully rational, but it's why I went the way I did.

In my mind, .resolve_type() is strictly for resolving types during
semantic analysis: look up a type by name, report an error if it doesn't
exist.

Before this patch:

(A) QAPISchemaArrayType.check() works.  The invariant check is buried
somewhat deep, in QAPISourceError.

(B) introspect.py works.  The invariant is not checked there.

(C) QAPISchemaVariants.check() works.  A rather losely related invariant
is checked there: the tag member's type exists.

This patch conflates two changes.

One, it adds an invariant check right to .resolve_type().  Impact:

    (A) Adds an invariant check closer to the surface.

    (B) Not touched.

    (C) Not touched.

No objection.

Two, it defaults .resolve_type()'s arguments to None.  Belongs to the
next patch.

The next patch overloads .resolve_type() to serve two use cases,
1. failure is a semantic error, and 2. failure is a programming error.
The first kind passes the arguments, the second doesn't.  Impact:

    (A) Not touched.

    (B) Adds invariant checking, in the callee.

    (C) Pushes the invariant checking into the callee.

I don't like overloading .resolve_type() this way.  Again: in my mind,
it's strictly for resolving the user's type names in semantic analysis.

If I drop this patch and the next one, mypy complains

    scripts/qapi/schema.py:1219: error: Argument 1 has incompatible type "QAPISourceInfo | None"; expected "QAPISourceInfo"  [arg-type]
    scripts/qapi/introspect.py:230: error: Incompatible types in assignment (expression has type "QAPISchemaType | None", variable has type "QAPISchemaType")  [assignment]
    scripts/qapi/introspect.py:233: error: Incompatible types in assignment (expression has type "QAPISchemaType | None", variable has type "QAPISchemaType")  [assignment]

Retaining the assertion added in this patch takes care of the first one.

To get rid of the two in introspect.py, we need to actually check the
invariant:

diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
index 67c7d89aae..4679b1bc2c 100644
--- a/scripts/qapi/introspect.py
+++ b/scripts/qapi/introspect.py
@@ -227,10 +227,14 @@ 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')
+            type_int = self._schema.lookup_type('int')
+            assert type_int
+            typ = type_int
         elif (isinstance(typ, QAPISchemaArrayType) and
               typ.element_type.json_type() == 'int'):
-            typ = self._schema.lookup_type('intList')
+            type_intList = self._schema.lookup_type('intList')
+            assert type_intList
+            typ = type_intList
         # Add type to work queue if new
         if typ not in self._used_types:
             self._used_types.append(typ)

Straightforward enough, although with a bit of notational overhead.

We use t = .lookup_type(...); assert t in three places then.  Feel free
to factor it out into a new helper.

> (P.S. I still violently want to create an info object that represents
> built-in definitions so I can just get rid of all the
> Optional[QAPISourceInfo] types from everywhere. I know I tried to do
> it before and you vetoed it, but the desire lives on in my heart.)

Once everything is properly typed, the cost and benefit of such a change
should be more clearly visible.

For now, let's try to type what we have, unless what we have complicates
typing too much.

[...]

Re: [PATCH v2 09/19] qapi/schema: allow resolve_type to be used for built-in types

[John Snow]

On Mon, Jan 22, 2024 at 8:12 AM Markus Armbruster <armbru@redhat.com> wrote:
>
> John Snow <jsnow@redhat.com> writes:
>
> > On Tue, Jan 16, 2024 at 6:09 AM Markus Armbruster <armbru@redhat.com> wrote:
> >>
> >> John Snow <jsnow@redhat.com> writes:
> >>
> >> > allow resolve_type to be used for both built-in and user-specified
> >> > type definitions. In the event that the type cannot be resolved, assert
> >> > that 'info' and 'what' were both provided in order to create a usable
> >> > QAPISemError.
> >> >
> >> > In practice, 'info' will only be None for built-in definitions, which
> >> > *should not fail* type lookup.
> >> >
> >> > As a convenience, allow the 'what' and 'info' parameters to be elided
> >> > entirely so that it can be used as a can-not-fail version of
> >> > lookup_type.
> >>
> >> The convenience remains unused until the next patch.  It should be added
> >> there.
> >
> > Okie-ducky.
> >
> >>
> >> > 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;
> >>
> >> Yes.  More below.
> >
> > Scary...
>
> I know...
>
> >> >                                                               the other two
> >> >     are limited by their base class initializers which accept info=None, but
> >>
> >> They do?
> >
> > In the case of QAPISchemaObjectTypeMember, the parent class
> > QAPISchemaMember allows initialization with info=None. I can't fully
> > trace all of the callsites, but one of them at least is in types.py:
> >
> >>     enum_members = members + [QAPISchemaEnumMember('_MAX', None)]
>
> I see.
>
> We may want to do the _MAX thingy differently.  Not now.
>
> > which necessitates, for now, info-less QAPISchemaEnumMember, which
> > necessitates info-less QAPISchemaMember. There are others, etc.
>
> Overriding an inherited attribute of type Optional[T] so it's
> non-optional T makes mypy unhappy?

Yeah, it considers it to be improper OO - it remembers only the
broadest type from the base class, which is Optional[T]. We aren't
overriding the property itself, we've just redefined a different
initializer, which doesn't carry through to the actual object.

(i.e. the initializer takes a T, the core object has an Optional[T],
there's no problem - but the field remains Optional[T].)

>
> >> >     neither actually use it in practice.
> >> >
> >> > Signed-off-by: John Snow <jsnow@redhat.com>
> >>
> >> Hmm.
> >
> > Scary.
> >
> >>
> >> We look up types by name in two ways:
> >>
> >> 1. Failure is a semantic error
> >>
> >>    Use .resolve_type(), passing real @info and @what.
> >>
> >>    Users:
> >>
> >>    * QAPISchemaArrayType.check() resolving the element type
> >>
> >>      Fine print: when the array type is built-in, we pass None @info and
> >>      @what.  The built-in array type's element type must exist for
> >>      .resolve_type() to work.  This commit changes .resolve_type() to
> >>      assert it does.
> >>
> >>    * QAPISchemaObjectType.check() resolving the base type
> >>
> >>    * QAPISchemaObjectTypeMember.check() resolving the member type
> >>
> >>    * QAPISchemaCommand.check() resolving argument type (if named) and
> >>      return type (which is always named).
> >>
> >>    * QAPISchemaEvent.check() resolving argument type (if named).
> >>
> >>    Note all users are in .check() methods.  That's where type named get
> >>    resolved.
> >>
> >> 2. Handle failure
> >>
> >>    Use .lookup_type(), which returns None when the named type doesn't
> >>    exist.
> >>
> >>    Users:
> >>
> >>    * QAPISchemaVariants.check(), to look up the base type containing the
> >>      tag member for error reporting purposes.  Failure would be a
> >>      programming error.
> >>
> >>    * .resolve_type(), which handles failure as semantic error
> >>
> >>    * ._make_array_type(), which uses it as "type exists already"
> >>       predicate.
> >>
> >>    * QAPISchemaGenIntrospectVisitor._use_type(), to look up certain
> >>      built-in types.  Failure would be a programming error.
> >>
> >> The next commit switches the uses where failure would be a programming
> >> error from .lookup_type() to .resolve_type() without @info and @what, so
> >> failure trips its assertion.  I don't like it, because it overloads
> >> .resolve_type() to serve two rather different use cases:
> >>
> >> 1. Failure is a semantic error; pass @info and @what
> >>
> >> 2. Failure is a programming error; don't pass @info and what
> >>
> >> The odd one out is of course QAPISchemaArrayType.check(), which wants to
> >> use 1. for the user's types and 2. for built-in types.  Let's ignore it
> >> for a second.
> >
> > "Let's ignore what motivated this patch" aww...
>
> Just for a second, I swear!
>
> >> I prefer to do 2. like typ = .lookup_type(); assert typ.  We can factor
> >> this out into its own helper if that helps (pardon the pun).
> >>
> >> Back to QAPISchemaArrayType.check().  Its need to resolve built-in
> >> element types, which have no info, necessitates .resolve_type() taking
> >> Optional[QAPISourceInfo].  This might bother you.  It doesn't bother me,
> >> unless it leads to mypy complications I can't see.
> >
> > Well, with this patch I allowed it to take Optional[QAPISourceInfo] -
> > just keep in mind that QAPISemError *requires* an info object, even
> > though the typing there is also Optional[QAPISourceInfo] ... It will
> > assert that info is present in __str__.
> >
> > Actually, I'd love to change that too - and make it fully required -
> > but since built-in types have no info, there's too many places I'd
> > need to change to enforce this as a static type.
> >
> > Still.
>
> Invariant: no error reports for built-in types.
>
> Checked since forever by asserting info is not None, exploiting the fact
> that info is None exactly for built-in types.
>
> This makes info: Optional[QAPISourceInfo] by design.
>
> Works.
>
> Specializing it to just QAPISourceInfo moves the assertion check from
> run time to compile time.  Might give a nice feeling, but I don't think
> it's practical everywhere, and it doesn't really matter anyway.
>
> Using a special value of QAPISourceInfo instead of None would also get
> rid of the Optional, along with the potential of checking at compile
> time.  Good trade *if* it simplifies the code.  See also the very end of
> my reply.
>
> >> We can simply leave it as is.  Adding the assertion to .resolve_type()
> >> is fine.
> >>
> >> Ot we complicate QAPISchemaArrayType.check() to simplify
> >> .resolve_type()'s typing, roughly like this:
> >>
> >>             if self.info:
> >>                 self.element_type = schema.resolve_type(
> >>                     self._element_type_name,
> >>                     self.info, self.info.defn_meta)
> >>             else:               # built-in type
> >>                 self.element_type = schema.lookup_type(
> >>                     self._element_type_name)
> >>                 assert self.element_type
> >>
> >> Not sure it's worth the trouble.  Thoughts?
> >
> > I suppose it's your call, ultimately. This patch exists primarily to
> > help in two places:
> >
> > (A) QAPISchemaArrayType.check(), as you've noticed, because it uses
> > the same path for both built-in and user-defined types. This is the
> > only place in the code where this occurs *at the moment*, but I can't
> > predict the future.
> >
> > (B) Calls to lookup_type in introspect.py which look up built-in types
> > and must-not-fail. It was cumbersome in the old patchset, but this one
> > makes it simpler.
> >
> > I suppose at the moment, having the assert directly in resolve_type
> > just means we get to use the same helper/pathway for both user-defined
> > and built-in types, which matches the infrastructure we already have,
> > which doesn't differentiate between the two. (By which I mean, all of
> > the Schema classes are not split into built-in and user-defined types,
> > so it is invisible to the type system.)
>
> Yes.
>
> > I could add conditional logic to the array check, and leave the
> > lookup_type calls in introspect.py being a little cumbersome - my main
> > concern with that solution is that I might be leaving a nasty
> > booby-trap in the future if someone wants to add a new built-in type
> > or something gets refactored to share more code pathways. Maybe that's
> > not fully rational, but it's why I went the way I did.
>
> In my mind, .resolve_type() is strictly for resolving types during
> semantic analysis: look up a type by name, report an error if it doesn't
> exist.
>
> Before this patch:
>
> (A) QAPISchemaArrayType.check() works.  The invariant check is buried
> somewhat deep, in QAPISourceError.
>
> (B) introspect.py works.  The invariant is not checked there.
>
> (C) QAPISchemaVariants.check() works.  A rather losely related invariant
> is checked there: the tag member's type exists.
>
> This patch conflates two changes.
>
> One, it adds an invariant check right to .resolve_type().  Impact:
>
>     (A) Adds an invariant check closer to the surface.
>
>     (B) Not touched.
>
>     (C) Not touched.
>
> No objection.
>
> Two, it defaults .resolve_type()'s arguments to None.  Belongs to the
> next patch.
>
> The next patch overloads .resolve_type() to serve two use cases,
> 1. failure is a semantic error, and 2. failure is a programming error.
> The first kind passes the arguments, the second doesn't.  Impact:
>
>     (A) Not touched.
>
>     (B) Adds invariant checking, in the callee.
>
>     (C) Pushes the invariant checking into the callee.
>
> I don't like overloading .resolve_type() this way.  Again: in my mind,
> it's strictly for resolving the user's type names in semantic analysis.
>
> If I drop this patch and the next one, mypy complains
>
>     scripts/qapi/schema.py:1219: error: Argument 1 has incompatible type "QAPISourceInfo | None"; expected "QAPISourceInfo"  [arg-type]
>     scripts/qapi/introspect.py:230: error: Incompatible types in assignment (expression has type "QAPISchemaType | None", variable has type "QAPISchemaType")  [assignment]
>     scripts/qapi/introspect.py:233: error: Incompatible types in assignment (expression has type "QAPISchemaType | None", variable has type "QAPISchemaType")  [assignment]
>
> Retaining the assertion added in this patch takes care of the first one.
>
> To get rid of the two in introspect.py, we need to actually check the
> invariant:
>
> diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
> index 67c7d89aae..4679b1bc2c 100644
> --- a/scripts/qapi/introspect.py
> +++ b/scripts/qapi/introspect.py
> @@ -227,10 +227,14 @@ 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')
> +            type_int = self._schema.lookup_type('int')
> +            assert type_int
> +            typ = type_int
>          elif (isinstance(typ, QAPISchemaArrayType) and
>                typ.element_type.json_type() == 'int'):
> -            typ = self._schema.lookup_type('intList')
> +            type_intList = self._schema.lookup_type('intList')
> +            assert type_intList
> +            typ = type_intList
>          # Add type to work queue if new
>          if typ not in self._used_types:
>              self._used_types.append(typ)
>
> Straightforward enough, although with a bit of notational overhead.
>
> We use t = .lookup_type(...); assert t in three places then.  Feel free
> to factor it out into a new helper.
>
> > (P.S. I still violently want to create an info object that represents
> > built-in definitions so I can just get rid of all the
> > Optional[QAPISourceInfo] types from everywhere. I know I tried to do
> > it before and you vetoed it, but the desire lives on in my heart.)
>
> Once everything is properly typed, the cost and benefit of such a change
> should be more clearly visible.
>
> For now, let's try to type what we have, unless what we have complicates
> typing too much.

Re: [PATCH v2 09/19] qapi/schema: allow resolve_type to be used for built-in types

[John Snow]

On Mon, Jan 22, 2024 at 8:12 AM Markus Armbruster <armbru@redhat.com> wrote:
>
> John Snow <jsnow@redhat.com> writes:
>
> > On Tue, Jan 16, 2024 at 6:09 AM Markus Armbruster <armbru@redhat.com> wrote:
> >>
> >> John Snow <jsnow@redhat.com> writes:
> >>
> >> > allow resolve_type to be used for both built-in and user-specified
> >> > type definitions. In the event that the type cannot be resolved, assert
> >> > that 'info' and 'what' were both provided in order to create a usable
> >> > QAPISemError.
> >> >
> >> > In practice, 'info' will only be None for built-in definitions, which
> >> > *should not fail* type lookup.
> >> >
> >> > As a convenience, allow the 'what' and 'info' parameters to be elided
> >> > entirely so that it can be used as a can-not-fail version of
> >> > lookup_type.
> >>
> >> The convenience remains unused until the next patch.  It should be added
> >> there.
> >
> > Okie-ducky.
> >
> >>
> >> > 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;
> >>
> >> Yes.  More below.
> >
> > Scary...
>
> I know...
>
> >> >                                                               the other two
> >> >     are limited by their base class initializers which accept info=None, but
> >>
> >> They do?
> >
> > In the case of QAPISchemaObjectTypeMember, the parent class
> > QAPISchemaMember allows initialization with info=None. I can't fully
> > trace all of the callsites, but one of them at least is in types.py:
> >
> >>     enum_members = members + [QAPISchemaEnumMember('_MAX', None)]
>
> I see.
>
> We may want to do the _MAX thingy differently.  Not now.
>
> > which necessitates, for now, info-less QAPISchemaEnumMember, which
> > necessitates info-less QAPISchemaMember. There are others, etc.
>
> Overriding an inherited attribute of type Optional[T] so it's
> non-optional T makes mypy unhappy?
>
> >> >     neither actually use it in practice.
> >> >
> >> > Signed-off-by: John Snow <jsnow@redhat.com>
> >>
> >> Hmm.
> >
> > Scary.
> >
> >>
> >> We look up types by name in two ways:
> >>
> >> 1. Failure is a semantic error
> >>
> >>    Use .resolve_type(), passing real @info and @what.
> >>
> >>    Users:
> >>
> >>    * QAPISchemaArrayType.check() resolving the element type
> >>
> >>      Fine print: when the array type is built-in, we pass None @info and
> >>      @what.  The built-in array type's element type must exist for
> >>      .resolve_type() to work.  This commit changes .resolve_type() to
> >>      assert it does.
> >>
> >>    * QAPISchemaObjectType.check() resolving the base type
> >>
> >>    * QAPISchemaObjectTypeMember.check() resolving the member type
> >>
> >>    * QAPISchemaCommand.check() resolving argument type (if named) and
> >>      return type (which is always named).
> >>
> >>    * QAPISchemaEvent.check() resolving argument type (if named).
> >>
> >>    Note all users are in .check() methods.  That's where type named get
> >>    resolved.
> >>
> >> 2. Handle failure
> >>
> >>    Use .lookup_type(), which returns None when the named type doesn't
> >>    exist.
> >>
> >>    Users:
> >>
> >>    * QAPISchemaVariants.check(), to look up the base type containing the
> >>      tag member for error reporting purposes.  Failure would be a
> >>      programming error.
> >>
> >>    * .resolve_type(), which handles failure as semantic error
> >>
> >>    * ._make_array_type(), which uses it as "type exists already"
> >>       predicate.
> >>
> >>    * QAPISchemaGenIntrospectVisitor._use_type(), to look up certain
> >>      built-in types.  Failure would be a programming error.
> >>
> >> The next commit switches the uses where failure would be a programming
> >> error from .lookup_type() to .resolve_type() without @info and @what, so
> >> failure trips its assertion.  I don't like it, because it overloads
> >> .resolve_type() to serve two rather different use cases:
> >>
> >> 1. Failure is a semantic error; pass @info and @what
> >>
> >> 2. Failure is a programming error; don't pass @info and what
> >>
> >> The odd one out is of course QAPISchemaArrayType.check(), which wants to
> >> use 1. for the user's types and 2. for built-in types.  Let's ignore it
> >> for a second.
> >
> > "Let's ignore what motivated this patch" aww...
>
> Just for a second, I swear!
>
> >> I prefer to do 2. like typ = .lookup_type(); assert typ.  We can factor
> >> this out into its own helper if that helps (pardon the pun).
> >>
> >> Back to QAPISchemaArrayType.check().  Its need to resolve built-in
> >> element types, which have no info, necessitates .resolve_type() taking
> >> Optional[QAPISourceInfo].  This might bother you.  It doesn't bother me,
> >> unless it leads to mypy complications I can't see.
> >
> > Well, with this patch I allowed it to take Optional[QAPISourceInfo] -
> > just keep in mind that QAPISemError *requires* an info object, even
> > though the typing there is also Optional[QAPISourceInfo] ... It will
> > assert that info is present in __str__.
> >
> > Actually, I'd love to change that too - and make it fully required -
> > but since built-in types have no info, there's too many places I'd
> > need to change to enforce this as a static type.
> >
> > Still.
>
> Invariant: no error reports for built-in types.
>
> Checked since forever by asserting info is not None, exploiting the fact
> that info is None exactly for built-in types.
>
> This makes info: Optional[QAPISourceInfo] by design.
>
> Works.
>
> Specializing it to just QAPISourceInfo moves the assertion check from
> run time to compile time.  Might give a nice feeling, but I don't think
> it's practical everywhere, and it doesn't really matter anyway.
>
> Using a special value of QAPISourceInfo instead of None would also get
> rid of the Optional, along with the potential of checking at compile
> time.  Good trade *if* it simplifies the code.  See also the very end of
> my reply.
>
> >> We can simply leave it as is.  Adding the assertion to .resolve_type()
> >> is fine.
> >>
> >> Ot we complicate QAPISchemaArrayType.check() to simplify
> >> .resolve_type()'s typing, roughly like this:
> >>
> >>             if self.info:
> >>                 self.element_type = schema.resolve_type(
> >>                     self._element_type_name,
> >>                     self.info, self.info.defn_meta)
> >>             else:               # built-in type
> >>                 self.element_type = schema.lookup_type(
> >>                     self._element_type_name)
> >>                 assert self.element_type
> >>
> >> Not sure it's worth the trouble.  Thoughts?
> >
> > I suppose it's your call, ultimately. This patch exists primarily to
> > help in two places:
> >
> > (A) QAPISchemaArrayType.check(), as you've noticed, because it uses
> > the same path for both built-in and user-defined types. This is the
> > only place in the code where this occurs *at the moment*, but I can't
> > predict the future.
> >
> > (B) Calls to lookup_type in introspect.py which look up built-in types
> > and must-not-fail. It was cumbersome in the old patchset, but this one
> > makes it simpler.
> >
> > I suppose at the moment, having the assert directly in resolve_type
> > just means we get to use the same helper/pathway for both user-defined
> > and built-in types, which matches the infrastructure we already have,
> > which doesn't differentiate between the two. (By which I mean, all of
> > the Schema classes are not split into built-in and user-defined types,
> > so it is invisible to the type system.)
>
> Yes.
>
> > I could add conditional logic to the array check, and leave the
> > lookup_type calls in introspect.py being a little cumbersome - my main
> > concern with that solution is that I might be leaving a nasty
> > booby-trap in the future if someone wants to add a new built-in type
> > or something gets refactored to share more code pathways. Maybe that's
> > not fully rational, but it's why I went the way I did.
>
> In my mind, .resolve_type() is strictly for resolving types during
> semantic analysis: look up a type by name, report an error if it doesn't
> exist.
>

In my mind, it's a function which must not return None, which makes it
useful. If it has different failure modes for different arguments,
that doesn't matter much to me. Assertions for programmer errors and
QAPISemError for semantic errors seems fine.

> Before this patch:
>
> (A) QAPISchemaArrayType.check() works.  The invariant check is buried
> somewhat deep, in QAPISourceError.
>

It also completely obscures what has actually failed with a pretty
unreadable error. It's a programmer error, sure, but I'm a programmer
and I hate being inconvenienced. (I have tripped on this bomb multiple
times while writing this series.)

> (B) introspect.py works.  The invariant is not checked there.
>
> (C) QAPISchemaVariants.check() works.  A rather losely related invariant
> is checked there: the tag member's type exists.
>
> This patch conflates two changes.
>
> One, it adds an invariant check right to .resolve_type().  Impact:
>
>     (A) Adds an invariant check closer to the surface.
>
>     (B) Not touched.
>
>     (C) Not touched.
>
> No objection.
>

OK, so I'll just keep the single new assert for this patch ...

> Two, it defaults .resolve_type()'s arguments to None.  Belongs to the
> next patch.
>
> The next patch overloads .resolve_type() to serve two use cases,
> 1. failure is a semantic error, and 2. failure is a programming error.
> The first kind passes the arguments, the second doesn't.  Impact:
>
>     (A) Not touched.
>
>     (B) Adds invariant checking, in the callee.
>
>     (C) Pushes the invariant checking into the callee.
>
> I don't like overloading .resolve_type() this way.  Again: in my mind,
> it's strictly for resolving the user's type names in semantic analysis.

It's already not *strictly* used for that, though, because of (C) in
particular. We have a lot less *goop* at the callsite by just teaching
resolve_type to understand which case it is being used for and
adapting it to raise the correct error in response (Assertion for
programmer failure, QAPISemError for semantic error.)

>
> If I drop this patch and the next one, mypy complains
>
>     scripts/qapi/schema.py:1219: error: Argument 1 has incompatible type "QAPISourceInfo | None"; expected "QAPISourceInfo"  [arg-type]
>     scripts/qapi/introspect.py:230: error: Incompatible types in assignment (expression has type "QAPISchemaType | None", variable has type "QAPISchemaType")  [assignment]
>     scripts/qapi/introspect.py:233: error: Incompatible types in assignment (expression has type "QAPISchemaType | None", variable has type "QAPISchemaType")  [assignment]
>
> Retaining the assertion added in this patch takes care of the first one.
>

Yep.

> To get rid of the two in introspect.py, we need to actually check the
> invariant:
>
> diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
> index 67c7d89aae..4679b1bc2c 100644
> --- a/scripts/qapi/introspect.py
> +++ b/scripts/qapi/introspect.py
> @@ -227,10 +227,14 @@ 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')
> +            type_int = self._schema.lookup_type('int')
> +            assert type_int
> +            typ = type_int
>          elif (isinstance(typ, QAPISchemaArrayType) and
>                typ.element_type.json_type() == 'int'):
> -            typ = self._schema.lookup_type('intList')
> +            type_intList = self._schema.lookup_type('intList')
> +            assert type_intList
> +            typ = type_intList
>          # Add type to work queue if new
>          if typ not in self._used_types:
>              self._used_types.append(typ)
>
> Straightforward enough, although with a bit of notational overhead.

Yeah. It's goopy. I don't like the goop.

In my mind:

(1) resolve_type: idiomatic python for type resolution; Exception one
way or another if we fail.
(2) lookup_type: C-brained type resolution, return None if we fail and
make it the caller's problem to perform due diligence.

>
> We use t = .lookup_type(...); assert t in three places then.  Feel free
> to factor it out into a new helper.
>

It'd cut down on the goop. Not convinced we need yet-another-helper (I
even dropped my patch refactoring these because I decided it wasn't
worth it), but if you would *really* like to maintain some semantic
difference between lookup/resolve beyond the return type, I'll
probably go this route because I think it makes callsites the
cleanest.

> > (P.S. I still violently want to create an info object that represents
> > built-in definitions so I can just get rid of all the
> > Optional[QAPISourceInfo] types from everywhere. I know I tried to do
> > it before and you vetoed it, but the desire lives on in my heart.)
>
> Once everything is properly typed, the cost and benefit of such a change
> should be more clearly visible.
>
> For now, let's try to type what we have, unless what we have complicates
> typing too much.
>

Yes, it just would help sweep all of the dirt into a more consolidated
location. Trying to audit when and where info can be None takes more
brain cycles than I'd prefer. I'm not advocating for it to happen in
this series, I am just advocating for it to happen.

> [...]
>

[PATCH v2 10/19] qapi: use schema.resolve_type instead of schema.lookup_type

[John Snow]

lookup_type() is capable of returning None, but some callers aren't
prepared for that and assume it will always succeed.

Use the must-not-fail variant resolve_type() instead, which guarantees
that the return type will not be None.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 scripts/qapi/introspect.py | 4 ++--
 scripts/qapi/schema.py     | 3 +--
 2 files changed, 3 insertions(+), 4 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 a77b51d1b96..35638c7708a 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):
-- 
2.43.0

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

[John Snow]

Adjust the expression at the callsite to eliminate weak type
introspection that believes this value can resolve to QAPISourceInfo; it
cannot.

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 35638c7708a..43af756ed47 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

Re: [PATCH v2 11/19] qapi/schema: fix QAPISchemaArrayType.check's call to resolve_type

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> Adjust the expression at the callsite to eliminate weak type
> introspection that believes this value can resolve to QAPISourceInfo; it
> cannot.

What do you mean by "weak type introspection"?  mypy being underpowered?

> 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 35638c7708a..43af756ed47 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):

Re: [PATCH v2 11/19] qapi/schema: fix QAPISchemaArrayType.check's call to resolve_type

[John Snow]

On Tue, Jan 16, 2024 at 7:17 AM Markus Armbruster <armbru@redhat.com> wrote:
>
> John Snow <jsnow@redhat.com> writes:
>
> > Adjust the expression at the callsite to eliminate weak type
> > introspection that believes this value can resolve to QAPISourceInfo; it
> > cannot.
>
> What do you mean by "weak type introspection"?  mypy being underpowered?

Yeah, s'what I meant.

>
> > 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 35638c7708a..43af756ed47 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):
>

Re: [PATCH v2 11/19] qapi/schema: fix QAPISchemaArrayType.check's call to resolve_type

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> On Tue, Jan 16, 2024 at 7:17 AM Markus Armbruster <armbru@redhat.com> wrote:
>>
>> John Snow <jsnow@redhat.com> writes:
>>
>> > Adjust the expression at the callsite to eliminate weak type
>> > introspection that believes this value can resolve to QAPISourceInfo; it
>> > cannot.
>>
>> What do you mean by "weak type introspection"?  mypy being underpowered?
>
> Yeah, s'what I meant.

Should be easy to clarify: mention mypy.

[PATCH v2 12/19] qapi/schema: assert info is present when necessary

[John Snow]

QAPISchemaInfo instances are sometimes defined as an Optional
field/argument because built-in definitions don't *have* a source
definition. As a consequence, there are a few places where we need to
assert that it's present because the root entity definition can only
enforce that it is "Optional".

Signed-off-by: John Snow <jsnow@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 scripts/qapi/schema.py | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
index 43af756ed47..eefa58a798b 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

Re: [PATCH v2 12/19] qapi/schema: assert info is present when necessary

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> QAPISchemaInfo instances are sometimes defined as an Optional
> field/argument because built-in definitions don't *have* a source
> definition. As a consequence, there are a few places where we need to
> assert that it's present because the root entity definition can only
> enforce that it is "Optional".

Well, we need to assert to help mypy over the hump.  But that's later in
this series.  Just like "enforce that is is 'Optional'".  My point is:
the commit message is less than clear unless the reader already knows
where we're going.

Here's my try:

  QAPISchemaInfo arguments can often be None because build-in
  definitions don't have such information.  The type hint can only be
  Optional[QAPISchemaInfo] then.  But then 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 the
  type hints.

>
> Signed-off-by: John Snow <jsnow@redhat.com>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>  scripts/qapi/schema.py | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
> index 43af756ed47..eefa58a798b 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(

[PATCH v2 13/19] qapi/schema: split "checked" field into "checking" and "checked"

[John Snow]

differentiate between "actively in the process of checking" and
"checking has completed". This allows us to clean up the types of some
internal fields such as QAPISchemaObjectType's members field which
currently uses "None" as a test for determining if check has been run
already or not.

This simplifies the typing from a cumbersome Optional[List[T]] to merely
a List[T], which is more pythonic: it is safe to iterate over an empty
list with "for x in []" whereas with an Optional[List[T]] you have to
rely on the more cumbersome "if L: for x in L: ..."

Note that it is valid to have an empty members list, see the internal
q_empty object as an example.

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

diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
index eefa58a798b..0d9a70ab4cb 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,22 +457,24 @@ 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):
         # 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
+        assert self._checked and not self.members
 
         seen = OrderedDict()
         if self._base_name:
@@ -489,13 +491,17 @@ 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)
             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

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

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> differentiate between "actively in the process of checking" and
> "checking has completed". This allows us to clean up the types of some
> internal fields such as QAPISchemaObjectType's members field which
> currently uses "None" as a test for determining if check has been run
> already or not.
>
> This simplifies the typing from a cumbersome Optional[List[T]] to merely
> a List[T], which is more pythonic: it is safe to iterate over an empty
> list with "for x in []" whereas with an Optional[List[T]] you have to
> rely on the more cumbersome "if L: for x in L: ..."

Does this cumbersome form exist?

> Note that it is valid to have an empty members list, see the internal
> q_empty object as an example.

Yes.

.members becomes valid only in .check().  Before the patch, .__init__()
initializes it to None, and .check() sets it to the real value.  We use
assert .members is not None to catch invalid use.  We can also hope
invalid use without an assert would crash.  for m in .members would.

We've seen this pattern before: PATCH 4+5.  There, we change .__init__()
to declare the attribute without initializing it.  Use before it becomes
valid now certainly crashes, which is an improvement.  Why can't we do
the same here?

> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  scripts/qapi/schema.py | 24 +++++++++++++++---------
>  1 file changed, 15 insertions(+), 9 deletions(-)
>
> diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
> index eefa58a798b..0d9a70ab4cb 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,22 +457,24 @@ 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):
>          # 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
> +        assert self._checked and not self.members
>  
>          seen = OrderedDict()
>          if self._base_name:
> @@ -489,13 +491,17 @@ 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)
>              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

I think you missed these:

       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)

The assertions no longer work.  I figure you want to assert .checked
instead.

Consider splitting the patch: first add .checking, and replace use of
.members by use of .checking where possible.  Then change .members.  The
split may or may not be easier to describe and digest.  Use your
judgement.

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

[John Snow]

On Tue, Jan 16, 2024 at 9:58 AM Markus Armbruster <armbru@redhat.com> wrote:
>
> John Snow <jsnow@redhat.com> writes:
>
> > differentiate between "actively in the process of checking" and
> > "checking has completed". This allows us to clean up the types of some
> > internal fields such as QAPISchemaObjectType's members field which
> > currently uses "None" as a test for determining if check has been run
> > already or not.
> >
> > This simplifies the typing from a cumbersome Optional[List[T]] to merely
> > a List[T], which is more pythonic: it is safe to iterate over an empty
> > list with "for x in []" whereas with an Optional[List[T]] you have to
> > rely on the more cumbersome "if L: for x in L: ..."
>
> Does this cumbersome form exist?

I thought it should, but I suppose it shouldn't given that you have
been relying on None to cause a crash.

Here's where that pattern *would* be used if None was legitimate:

qapi/schema.py:604: error: Item "None" of
"Optional[List[QAPISchemaObjectTypeMember]]" has no attribute
"__iter__" (not iterable)  [union-attr]
qapi/schema.py:626: error: Item "None" of
"Optional[List[QAPISchemaObjectTypeMember]]" has no attribute
"__iter__" (not iterable)  [union-attr]
qapi/gen.py:122: error: Item "None" of
"Optional[List[QAPISchemaObjectTypeMember]]" has no attribute
"__iter__" (not iterable)  [union-attr]
qapi/commands.py:68: error: Item "None" of
"Optional[List[QAPISchemaObjectTypeMember]]" has no attribute
"__iter__" (not iterable)  [union-attr]
qapi/events.py:60: error: Item "None" of
"Optional[List[QAPISchemaObjectTypeMember]]" has no attribute
"__iter__" (not iterable)  [union-attr]

but, I suppose your argument is that it just literally never is. So
never mind the callsite argument - though mypy still wants its
guarantee that it will never be None here.

>
> > Note that it is valid to have an empty members list, see the internal
> > q_empty object as an example.
>
> Yes.
>
> .members becomes valid only in .check().  Before the patch, .__init__()
> initializes it to None, and .check() sets it to the real value.  We use
> assert .members is not None to catch invalid use.  We can also hope
> invalid use without an assert would crash.  for m in .members would.
>

Mmm, I see. (I very often just literally don't account for you relying
on invalid types being load-bearing ... Seeing a stack trace where
you're told that "None" does not have such-and-such property usually
feels about as helpful as getting kicked out of a moving car on the
freeway.)

> We've seen this pattern before: PATCH 4+5.  There, we change .__init__()
> to declare the attribute without initializing it.  Use before it becomes
> valid now certainly crashes, which is an improvement.  Why can't we do
> the same here?

Didn't occur to me to add a crash on purpose... In the other cases, I
think I didn't have any suitable value to add at all, but in this case
I can use an empty list.

(I have a kind of distaste for relying on Python-level exceptions like
AttributeError to indicate that a stateful error has occurred - i.e.
that this attribute doesn't exist yet, but it will. I usually aim for
having all of the attributes defined at initialization time. The error
is misleading, semantically.

In our case, full initialization directly at init time is not ...
quite possible without some vigorous reworking of the known universe,
so I capitulated and allowed some "very late initialization" which
causes some friction for static typing between pre- and post- "delayed
initialization" callsites. It's still very much something I look at
and regard as a code smell. The entire point of declaring all state in
the init method is to centralize that state so it's finite and
knowable, both to static analysis tools and to humans. Punting the
initialization to arbitrary points later in the object's lifetime
feels like lying: we promise this value is here after initialization,
except not really, have a nice day.)

*cough* that said, I can also use that same trick here. I just want to
whine about it. (I guess I don't want to teach folks what I consider
to be a bad habit just because it makes the linters shut up, but
realize the sausage has to get made.)

... I *just* now saw you had more ideas on how to approach this in the
last series, I will go back and read them. I didn't mean to skip past
them. Lemme investigate.

>
> > Signed-off-by: John Snow <jsnow@redhat.com>
> > ---
> >  scripts/qapi/schema.py | 24 +++++++++++++++---------
> >  1 file changed, 15 insertions(+), 9 deletions(-)
> >
> > diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
> > index eefa58a798b..0d9a70ab4cb 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,22 +457,24 @@ 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):
> >          # 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
> > +        assert self._checked and not self.members
> >
> >          seen = OrderedDict()
> >          if self._base_name:
> > @@ -489,13 +491,17 @@ 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)
> >              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
>
> I think you missed these:
>
>        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)
>
> The assertions no longer work.  I figure you want to assert .checked
> instead.
>
> Consider splitting the patch: first add .checking, and replace use of
> .members by use of .checking where possible.  Then change .members.  The
> split may or may not be easier to describe and digest.  Use your
> judgement.

Ah, oops.

(I wish mypy would bark about this, actually: if members is a List,
then surely it can't ever be None, right? It's definitely a smell. I
wonder if there is a reason why it doesn't, or if this is a valid
feature request.)

--js

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

[John Snow]

On Thu, Feb 1, 2024 at 2:41 PM John Snow <jsnow@redhat.com> wrote:
>
> On Tue, Jan 16, 2024 at 9:58 AM Markus Armbruster <armbru@redhat.com> wrote:
> >
> > John Snow <jsnow@redhat.com> writes:
> >
> > > differentiate between "actively in the process of checking" and
> > > "checking has completed". This allows us to clean up the types of some
> > > internal fields such as QAPISchemaObjectType's members field which
> > > currently uses "None" as a test for determining if check has been run
> > > already or not.
> > >
> > > This simplifies the typing from a cumbersome Optional[List[T]] to merely
> > > a List[T], which is more pythonic: it is safe to iterate over an empty
> > > list with "for x in []" whereas with an Optional[List[T]] you have to
> > > rely on the more cumbersome "if L: for x in L: ..."
> >
> > Does this cumbersome form exist?
>
> I thought it should, but I suppose it shouldn't given that you have
> been relying on None to cause a crash.
>
> Here's where that pattern *would* be used if None was legitimate:
>
> qapi/schema.py:604: error: Item "None" of
> "Optional[List[QAPISchemaObjectTypeMember]]" has no attribute
> "__iter__" (not iterable)  [union-attr]
> qapi/schema.py:626: error: Item "None" of
> "Optional[List[QAPISchemaObjectTypeMember]]" has no attribute
> "__iter__" (not iterable)  [union-attr]
> qapi/gen.py:122: error: Item "None" of
> "Optional[List[QAPISchemaObjectTypeMember]]" has no attribute
> "__iter__" (not iterable)  [union-attr]
> qapi/commands.py:68: error: Item "None" of
> "Optional[List[QAPISchemaObjectTypeMember]]" has no attribute
> "__iter__" (not iterable)  [union-attr]
> qapi/events.py:60: error: Item "None" of
> "Optional[List[QAPISchemaObjectTypeMember]]" has no attribute
> "__iter__" (not iterable)  [union-attr]
>
> but, I suppose your argument is that it just literally never is. So
> never mind the callsite argument - though mypy still wants its
> guarantee that it will never be None here.
>
> >
> > > Note that it is valid to have an empty members list, see the internal
> > > q_empty object as an example.
> >
> > Yes.
> >
> > .members becomes valid only in .check().  Before the patch, .__init__()
> > initializes it to None, and .check() sets it to the real value.  We use
> > assert .members is not None to catch invalid use.  We can also hope
> > invalid use without an assert would crash.  for m in .members would.
> >
>
> Mmm, I see. (I very often just literally don't account for you relying
> on invalid types being load-bearing ... Seeing a stack trace where
> you're told that "None" does not have such-and-such property usually
> feels about as helpful as getting kicked out of a moving car on the
> freeway.)
>
> > We've seen this pattern before: PATCH 4+5.  There, we change .__init__()
> > to declare the attribute without initializing it.  Use before it becomes
> > valid now certainly crashes, which is an improvement.  Why can't we do
> > the same here?
>
> Didn't occur to me to add a crash on purpose... In the other cases, I
> think I didn't have any suitable value to add at all, but in this case
> I can use an empty list.
>
> (I have a kind of distaste for relying on Python-level exceptions like
> AttributeError to indicate that a stateful error has occurred - i.e.
> that this attribute doesn't exist yet, but it will. I usually aim for
> having all of the attributes defined at initialization time. The error
> is misleading, semantically.
>
> In our case, full initialization directly at init time is not ...
> quite possible without some vigorous reworking of the known universe,
> so I capitulated and allowed some "very late initialization" which
> causes some friction for static typing between pre- and post- "delayed
> initialization" callsites. It's still very much something I look at
> and regard as a code smell. The entire point of declaring all state in
> the init method is to centralize that state so it's finite and
> knowable, both to static analysis tools and to humans. Punting the
> initialization to arbitrary points later in the object's lifetime
> feels like lying: we promise this value is here after initialization,
> except not really, have a nice day.)
>
> *cough* that said, I can also use that same trick here. I just want to
> whine about it. (I guess I don't want to teach folks what I consider
> to be a bad habit just because it makes the linters shut up, but
> realize the sausage has to get made.)
>
> ... I *just* now saw you had more ideas on how to approach this in the
> last series, I will go back and read them. I didn't mean to skip past
> them. Lemme investigate.
>
> >
> > > Signed-off-by: John Snow <jsnow@redhat.com>
> > > ---
> > >  scripts/qapi/schema.py | 24 +++++++++++++++---------
> > >  1 file changed, 15 insertions(+), 9 deletions(-)
> > >
> > > diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
> > > index eefa58a798b..0d9a70ab4cb 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,22 +457,24 @@ 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):
> > >          # 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
> > > +        assert self._checked and not self.members
> > >
> > >          seen = OrderedDict()
> > >          if self._base_name:
> > > @@ -489,13 +491,17 @@ 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)
> > >              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
> >
> > I think you missed these:
> >
> >        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)
> >
> > The assertions no longer work.  I figure you want to assert .checked
> > instead.
> >
> > Consider splitting the patch: first add .checking, and replace use of
> > .members by use of .checking where possible.  Then change .members.  The
> > split may or may not be easier to describe and digest.  Use your
> > judgement.
>
> Ah, oops.

Oh, I got it in the assertions patch, but have since moved it forward.

>
> (I wish mypy would bark about this, actually: if members is a List,
> then surely it can't ever be None, right? It's definitely a smell. I
> wonder if there is a reason why it doesn't, or if this is a valid
> feature request.)
>
> --js

[PATCH v2 14/19] 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 0d9a70ab4cb..6afafdb8b0d 100644
--- a/scripts/qapi/schema.py
+++ b/scripts/qapi/schema.py
@@ -637,25 +637,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
@@ -675,11 +689,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

Re: [PATCH v2 14/19] qapi/schema: fix typing for QAPISchemaVariants.tag_member

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> 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>

I explored another solution, and posted it in reply to v2.  If we decide
not to like it better, I guess we'll go with this one.

[PATCH v2 15/19] 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 6afafdb8b0d..f2271dc7cbd 100644
--- a/scripts/qapi/schema.py
+++ b/scripts/qapi/schema.py
@@ -725,7 +725,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 v2 16/19] 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 bf31018aef0..b7f08cf36f2 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 v2 17/19] 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 | 568 ++++++++++++++++++++++++++++-------------
 1 file changed, 396 insertions(+), 172 deletions(-)

diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
index f2271dc7cbd..8aa27ddb1fe 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)
 
 
@@ -63,8 +77,8 @@ class QAPISchemaEntity:
     include directives. Entities with names are represented by the
     more specific sub-class `QAPISchemaDefinition` instead.
     """
-    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
@@ -73,37 +87,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 []:
@@ -114,88 +137,146 @@ 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:
         # pylint: disable=unused-argument
         assert not self._checked
-        seen = {}
+        seen: Dict[str, QAPISchemaMember] = {}
         for f in self.features:
             f.check_clash(self.info, seen)
         self._checked = True
 
-    def connect_doc(self, doc=None):
+    def connect_doc(self, doc: Optional[QAPIDoc] = None) -> None:
         doc = doc or self.doc
         if doc:
             for f in self.features:
                 doc.connect_feature(f)
 
-    def check_doc(self):
+    def check_doc(self) -> None:
         if self.doc:
             self.doc.check()
 
     @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)
 
@@ -262,18 +343,18 @@ 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) -> 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,34 +626,34 @@ 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:
         assert self.members is not None
         return not self.members and not self.variants
 
-    def has_conditional_members(self):
+    def has_conditional_members(self) -> bool:
         assert self.members is not None
         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,
@@ -561,7 +666,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
@@ -569,7 +682,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
@@ -577,8 +690,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()
@@ -607,26 +720,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.
@@ -637,11 +756,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 "
@@ -649,11 +768,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))
@@ -722,7 +843,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.
@@ -736,18 +861,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(
@@ -756,11 +890,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
@@ -792,14 +926,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:
@@ -809,12 +949,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)
@@ -826,19 +974,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:
@@ -848,24 +996,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
@@ -873,7 +1039,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:
@@ -909,14 +1075,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,
@@ -928,14 +1094,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(
@@ -957,14 +1132,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,
@@ -972,7 +1147,7 @@ def visit(self, visitor):
 
 
 class QAPISchema:
-    def __init__(self, fname):
+    def __init__(self, fname: str):
         self.fname = fname
 
         try:
@@ -984,9 +1159,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)
@@ -996,10 +1171,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, ent):
+    def _def_definition(self, ent: QAPISchemaDefinition) -> None:
         # Only the predefined types are allowed to not have info
         assert ent.info or self._predefining
         self._def_entity(ent)
@@ -1016,18 +1191,27 @@ def _def_definition(self, ent):
                 ent.info, "%s is already defined" % other_ent.describe())
         self._entity_dict[ent.name] = ent
 
-    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
@@ -1042,23 +1226,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
@@ -1066,7 +1252,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'),
@@ -1095,30 +1281,51 @@ 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()
@@ -1134,7 +1341,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')
@@ -1145,7 +1352,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:]
@@ -1156,13 +1370,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']
@@ -1174,13 +1392,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']
@@ -1205,7 +1429,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)
@@ -1223,7 +1447,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')
@@ -1248,7 +1472,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)
@@ -1262,7 +1486,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)
@@ -1281,7 +1505,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()
@@ -1289,7 +1513,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 v2 18/19] 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 v2 19/19] 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 | 27 ---------------------------
 1 file changed, 27 deletions(-)

diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
index 8aa27ddb1fe..c1f030fce25 100644
--- a/scripts/qapi/schema.py
+++ b/scripts/qapi/schema.py
@@ -127,10 +127,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
@@ -632,11 +620,9 @@ def is_implicit(self) -> bool:
         return self.name.startswith('q_')
 
     def is_empty(self) -> bool:
-        assert self.members is not None
         return not self.members and not self.variants
 
     def has_conditional_members(self) -> bool:
-        assert self.members is not None
         return any(m.ifcond.is_present() for m in self.members)
 
     def c_name(self) -> str:
@@ -676,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)
@@ -752,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
@@ -867,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()
@@ -935,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 []
 
@@ -964,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()
@@ -975,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:
@@ -1026,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
@@ -1067,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(
@@ -1105,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