[PATCH v5 00/15] qapi: static typing conversion, pt2

[PATCH v5 00/15] qapi: static typing conversion, pt2

[John Snow]

Hi, this series adds static type hints to the QAPI module.
This is part two, and covers introspect.py.

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

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

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

Every commit should pass with (from ./scripts):
 - flake8 qapi/
 - pylint --rcfile=qapi/pylintrc qapi/
 - mypy --config-file=qapi/mypy.ini qapi/
 - isort -c qapi/

V5:

001/15:[----] [--] 'qapi/introspect.py: assert schema is not None'
002/15:[----] [--] 'qapi/introspect.py: use _make_tree for features nodes'
003/15:[----] [--] 'qapi/introspect.py: add _gen_features helper'
004/15:[0006] [FC] 'qapi/introspect.py: guard against ifcond/comment misuse'
005/15:[----] [-C] 'qapi/introspect.py: Unify return type of _make_tree()'
006/15:[0009] [FC] 'qapi/introspect.py: replace 'extra' dict with 'comment' argument'
007/15:[down] 'qapi/introspect.py: Always define all 'extra' dict keys'
008/15:[0002] [FC] 'qapi/introspect.py: Introduce preliminary tree typing'
009/15:[0018] [FC] 'qapi/introspect.py: create a typed 'Annotated' data strutcure'
010/15:[----] [--] 'qapi/introspect.py: improve _tree_to_qlit error message'
011/15:[0002] [FC] 'qapi/introspect.py: improve readability of _tree_to_qlit'
012/15:[0008] [FC] 'qapi/introspect.py: add type hint annotations'
013/15:[0006] [FC] 'qapi/introspect.py: add introspect.json dummy types'
014/15:[0003] [FC] 'qapi/introspect.py: Add docstring to _tree_to_qlit'
015/15:[0002] [FC] 'qapi/introspect.py: Update copyright and authors list'

004:
 - Rename 'suppress_first_indent' to 'dict_value'
   (Docstring added in 014.)
006:
 - Avoid changing the output structure of _make_tree
007:
 - Chance the structure of _make_tree 8-)
008:
 - Change commented TreeValue to include a TODO instead.
009:
 - Change NodeT bound to _value instead of TreeValue
 - Change "Remove in 3.7" text to include "TODO: "
 - Remove forwarding suppress_first_indent/dict_value in recursive cases
 - Change spacing in visit_alternate_type()
011:
 - Consequence of suppress_first_value/dict_value change
012:
 - Commit message note added
 - Changed _DObject comment
013:
 - Commit notes adjusted
 - _DObject stuff: Comment near SchemaInfo et al adjusted
014:
 - Changed docstring to reflect dict_value change
015:
 - Updated copyright year for 2021 :~)

V4:
 - Rebased on "pt1.5" v4
 - signatures updated to use Optional[QAPISourceInfo]
 - Changelog from V3 still relevant (That series went unreviewed.)

V3:
 - Dropped all the R-Bs again...
 - Re-re-ordered to put type annotations last again.
 - Rebased on top of "pt1.5".
 - Ensured compliance with strict-optional typing.
 - Forgive me if I missed a specific critique;
   I probably just lost it in the shuffle.

V2:
 - Dropped all R-B from previous series; enough has changed.
 - pt2 is now introspect.py, expr.py is pushed to pt3.
 - Reworked again to have less confusing (?) type names
 - Added an assertion to prevent future accidental breakage

John Snow (15):
  qapi/introspect.py: assert schema is not None
  qapi/introspect.py: use _make_tree for features nodes
  qapi/introspect.py: add _gen_features helper
  qapi/introspect.py: guard against ifcond/comment misuse
  qapi/introspect.py: Unify return type of _make_tree()
  qapi/introspect.py: replace 'extra' dict with 'comment' argument
  qapi/introspect.py: Always define all 'extra' dict keys
  qapi/introspect.py: Introduce preliminary tree typing
  qapi/introspect.py: create a typed 'Annotated' data strutcure
  qapi/introspect.py: improve _tree_to_qlit error message
  qapi/introspect.py: improve readability of _tree_to_qlit
  qapi/introspect.py: add type hint annotations
  qapi/introspect.py: add introspect.json dummy types
  qapi/introspect.py: Add docstring to _tree_to_qlit
  qapi/introspect.py: Update copyright and authors list

 scripts/qapi/introspect.py | 314 ++++++++++++++++++++++++++-----------
 scripts/qapi/mypy.ini      |   5 -
 scripts/qapi/schema.py     |   2 +-
 3 files changed, 223 insertions(+), 98 deletions(-)

-- 
2.29.2

[PATCH v5 01/15] qapi/introspect.py: assert schema is not None

[John Snow]

The introspect visitor is stateful, but expects that it will have a
schema to refer to. Add assertions that state this.

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

diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
index fafec94e022..43ab4be1f77 100644
--- a/scripts/qapi/introspect.py
+++ b/scripts/qapi/introspect.py
@@ -147,6 +147,8 @@ def _name(self, name):
         return self._name_map[name]
 
     def _use_type(self, typ):
+        assert self._schema is not None
+
         # Map the various integer types to plain int
         if typ.json_type() == 'int':
             typ = self._schema.lookup_type('int')
@@ -225,6 +227,8 @@ def visit_alternate_type(self, name, info, ifcond, features, variants):
     def visit_command(self, name, info, ifcond, features,
                       arg_type, ret_type, gen, success_response, boxed,
                       allow_oob, allow_preconfig, coroutine):
+        assert self._schema is not None
+
         arg_type = arg_type or self._schema.the_empty_object_type
         ret_type = ret_type or self._schema.the_empty_object_type
         obj = {'arg-type': self._use_type(arg_type),
@@ -234,6 +238,7 @@ def visit_command(self, name, info, ifcond, features,
         self._gen_tree(name, 'command', obj, ifcond, features)
 
     def visit_event(self, name, info, ifcond, features, arg_type, boxed):
+        assert self._schema is not None
         arg_type = arg_type or self._schema.the_empty_object_type
         self._gen_tree(name, 'event', {'arg-type': self._use_type(arg_type)},
                        ifcond, features)
-- 
2.29.2

[PATCH v5 02/15] qapi/introspect.py: use _make_tree for features nodes

[John Snow]

At present, we open-code this in _make_tree itself; but if the structure
of the tree changes, this is brittle. Use an explicit recursive call to
_make_tree when appropriate to help keep the interior node typing
consistent.

A consequence of doing this is that the 'ifcond' key of the features
dict will be omitted when ifcond is false-ish, just like it is omitted
in top-level calls to _make_tree. This also increases consistency in our
handling of this property.

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

diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
index 43ab4be1f77..3295a15c98e 100644
--- a/scripts/qapi/introspect.py
+++ b/scripts/qapi/introspect.py
@@ -30,7 +30,9 @@ def _make_tree(obj, ifcond, features, extra=None):
     if ifcond:
         extra['if'] = ifcond
     if features:
-        obj['features'] = [(f.name, {'if': f.ifcond}) for f in features]
+        obj['features'] = [
+            _make_tree(f.name, f.ifcond, None) for f in features
+        ]
     if extra:
         return (obj, extra)
     return obj
-- 
2.29.2

[PATCH v5 03/15] qapi/introspect.py: add _gen_features helper

[John Snow]

_make_tree might receive a dict (a SchemaInfo object) or some other type
(usually, a string) for its obj parameter. Adding features information
should arguably be performed by the caller at such a time when we know
the type of the object and don't have to re-interrogate it.

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

diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
index 3295a15c98e..4749f65ea3c 100644
--- a/scripts/qapi/introspect.py
+++ b/scripts/qapi/introspect.py
@@ -24,15 +24,11 @@
 )
 
 
-def _make_tree(obj, ifcond, features, extra=None):
+def _make_tree(obj, ifcond, extra=None):
     if extra is None:
         extra = {}
     if ifcond:
         extra['if'] = ifcond
-    if features:
-        obj['features'] = [
-            _make_tree(f.name, f.ifcond, None) for f in features
-        ]
     if extra:
         return (obj, extra)
     return obj
@@ -169,6 +165,10 @@ def _use_type(self, typ):
             return '[' + self._use_type(typ.element_type) + ']'
         return self._name(typ.name)
 
+    @staticmethod
+    def _gen_features(features):
+        return [_make_tree(f.name, f.ifcond) for f in features]
+
     def _gen_tree(self, name, mtype, obj, ifcond, features):
         extra = None
         if mtype not in ('command', 'event', 'builtin', 'array'):
@@ -179,13 +179,17 @@ def _gen_tree(self, name, mtype, obj, ifcond, features):
             name = self._name(name)
         obj['name'] = name
         obj['meta-type'] = mtype
-        self._trees.append(_make_tree(obj, ifcond, features, extra))
+        if features:
+            obj['features'] = self._gen_features(features)
+        self._trees.append(_make_tree(obj, ifcond, extra))
 
     def _gen_member(self, member):
         obj = {'name': member.name, 'type': self._use_type(member.type)}
         if member.optional:
             obj['default'] = None
-        return _make_tree(obj, member.ifcond, member.features)
+        if member.features:
+            obj['features'] = self._gen_features(member.features)
+        return _make_tree(obj, member.ifcond)
 
     def _gen_variants(self, tag_name, variants):
         return {'tag': tag_name,
@@ -193,7 +197,7 @@ def _gen_variants(self, tag_name, variants):
 
     def _gen_variant(self, variant):
         obj = {'case': variant.name, 'type': self._use_type(variant.type)}
-        return _make_tree(obj, variant.ifcond, None)
+        return _make_tree(obj, variant.ifcond)
 
     def visit_builtin_type(self, name, info, json_type):
         self._gen_tree(name, 'builtin', {'json-type': json_type}, [], None)
-- 
2.29.2

[PATCH v5 04/15] qapi/introspect.py: guard against ifcond/comment misuse

[John Snow]

_tree_to_qlit is called recursively on dict values (isolated from their
keys); at such a point in generating output it is too late to apply an
ifcond. Similarly, comments do not necessarily have a "tidy" place they
can be printed in such a circumstance.

Forbid this usage by renaming "suppress_first_indent" to "dict_value" to
emphasize that indents are suppressed only for the benefit of dict
values; then add an assertion assuring we do not pass ifcond/comments
in this case.

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

diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
index 4749f65ea3c..a7ccda5ab92 100644
--- a/scripts/qapi/introspect.py
+++ b/scripts/qapi/introspect.py
@@ -34,7 +34,7 @@ def _make_tree(obj, ifcond, extra=None):
     return obj
 
 
-def _tree_to_qlit(obj, level=0, suppress_first_indent=False):
+def _tree_to_qlit(obj, level=0, dict_value=False):
 
     def indent(level):
         return level * 4 * ' '
@@ -43,6 +43,12 @@ def indent(level):
         ifobj, extra = obj
         ifcond = extra.get('if')
         comment = extra.get('comment')
+
+        # NB: _tree_to_qlit is called recursively on the values of a key:value
+        # pair; those values can't be decorated with comments or conditionals.
+        msg = "dict values cannot have attached comments or if-conditionals."
+        assert not dict_value, msg
+
         ret = ''
         if comment:
             ret += indent(level) + '/* %s */\n' % comment
@@ -54,7 +60,7 @@ def indent(level):
         return ret
 
     ret = ''
-    if not suppress_first_indent:
+    if not dict_value:
         ret += indent(level)
     if obj is None:
         ret += 'QLIT_QNULL'
-- 
2.29.2

[PATCH v5 05/15] qapi/introspect.py: Unify return type of _make_tree()

[John Snow]

Returning two different types conditionally can be complicated to
type. Return one type for consistency.

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

diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
index a7ccda5ab92..7730d8ed6b2 100644
--- a/scripts/qapi/introspect.py
+++ b/scripts/qapi/introspect.py
@@ -29,9 +29,7 @@ def _make_tree(obj, ifcond, extra=None):
         extra = {}
     if ifcond:
         extra['if'] = ifcond
-    if extra:
-        return (obj, extra)
-    return obj
+    return (obj, extra)
 
 
 def _tree_to_qlit(obj, level=0, dict_value=False):
-- 
2.29.2

[PATCH v5 06/15] qapi/introspect.py: replace 'extra' dict with 'comment' argument

[John Snow]

This is only used to pass in a dictionary with a comment already set, so
skip the runaround and just accept the (optional) comment.

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

diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
index 7730d8ed6b2..1655a21f85b 100644
--- a/scripts/qapi/introspect.py
+++ b/scripts/qapi/introspect.py
@@ -10,6 +10,8 @@
 See the COPYING file in the top-level directory.
 """
 
+from typing import Optional
+
 from .common import (
     c_name,
     gen_endif,
@@ -24,11 +26,12 @@
 )
 
 
-def _make_tree(obj, ifcond, extra=None):
-    if extra is None:
-        extra = {}
+def _make_tree(obj, ifcond, comment=None):
+    extra = {}
     if ifcond:
         extra['if'] = ifcond
+    if comment:
+        extra['comment'] = comment
     return (obj, extra)
 
 
@@ -174,18 +177,18 @@ def _gen_features(features):
         return [_make_tree(f.name, f.ifcond) for f in features]
 
     def _gen_tree(self, name, mtype, obj, ifcond, features):
-        extra = None
+        comment: Optional[str] = None
         if mtype not in ('command', 'event', 'builtin', 'array'):
             if not self._unmask:
                 # Output a comment to make it easy to map masked names
                 # back to the source when reading the generated output.
-                extra = {'comment': '"%s" = %s' % (self._name(name), name)}
+                comment = f'"{self._name(name)}" = {name}'
             name = self._name(name)
         obj['name'] = name
         obj['meta-type'] = mtype
         if features:
             obj['features'] = self._gen_features(features)
-        self._trees.append(_make_tree(obj, ifcond, extra))
+        self._trees.append(_make_tree(obj, ifcond, comment))
 
     def _gen_member(self, member):
         obj = {'name': member.name, 'type': self._use_type(member.type)}
-- 
2.29.2

[PATCH v5 07/15] qapi/introspect.py: Always define all 'extra' dict keys

[John Snow]

This mimics how a typed object works, where 'if' and 'comment' are
always set, regardless of if they have a value set or not.

It is safe to do this because of the way that _tree_to_qlit processes
these values (using dict.get with a default of None), resulting in no
change of output from _tree_to_qlit. There are no other users of this
data.

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

diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
index 1655a21f85b..45231d2abc3 100644
--- a/scripts/qapi/introspect.py
+++ b/scripts/qapi/introspect.py
@@ -27,11 +27,10 @@
 
 
 def _make_tree(obj, ifcond, comment=None):
-    extra = {}
-    if ifcond:
-        extra['if'] = ifcond
-    if comment:
-        extra['comment'] = comment
+    extra = {
+        'if': ifcond,
+        'comment': comment
+    }
     return (obj, extra)
 
 
-- 
2.29.2

[PATCH v5 08/15] qapi/introspect.py: Introduce preliminary tree typing

[John Snow]

The types will be used in forthcoming patches to add typing. These types
describe the layout and structure of the objects passed to
_tree_to_qlit, but lack the power to describe annotations until the next
commit.

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

diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
index 45231d2abc3..8e019b4a26a 100644
--- a/scripts/qapi/introspect.py
+++ b/scripts/qapi/introspect.py
@@ -10,7 +10,13 @@
 See the COPYING file in the top-level directory.
 """
 
-from typing import Optional
+from typing import (
+    Any,
+    Dict,
+    List,
+    Optional,
+    Union,
+)
 
 from .common import (
     c_name,
@@ -26,6 +32,28 @@
 )
 
 
+# This module constructs a tree data structure that is used to
+# generate the introspection information for QEMU. It behaves similarly
+# to a JSON value.
+#
+# A complexity over JSON is that our values may or may not be annotated.
+#
+# Un-annotated values may be:
+#     Scalar: str, bool, None.
+#     Non-scalar: List, Dict
+# _value = Union[str, bool, None, Dict[str, TreeValue], List[TreeValue]]
+#
+# With optional annotations, the type of all values is:
+# TreeValue = Union[_value, Annotated[_value]]
+#
+# Sadly, mypy does not support recursive types, so we must approximate this.
+_stub = Any
+_scalar = Union[str, bool, None]
+_nonscalar = Union[Dict[str, _stub], List[_stub]]
+_value = Union[_scalar, _nonscalar]
+# TreeValue = TODO, in a forthcoming commit.
+
+
 def _make_tree(obj, ifcond, comment=None):
     extra = {
         'if': ifcond,
-- 
2.29.2

[PATCH v5 09/15] qapi/introspect.py: create a typed 'Annotated' data strutcure

[John Snow]

Presently, we use a tuple to attach a dict containing annotations
(comments and compile-time conditionals) to a tree node. This is
undesirable because dicts are difficult to strongly type; promoting it
to a real class allows us to name the values and types of the
annotations we are expecting.

In terms of typing, the Annotated<T> type serves as a generic container
where the annotated node's type is preserved, allowing for greater
specificity than we'd be able to provide without a generic.

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

diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
index 8e019b4a26a..b9427aba449 100644
--- a/scripts/qapi/introspect.py
+++ b/scripts/qapi/introspect.py
@@ -13,8 +13,12 @@
 from typing import (
     Any,
     Dict,
+    Generic,
+    Iterable,
     List,
     Optional,
+    Tuple,
+    TypeVar,
     Union,
 )
 
@@ -51,15 +55,25 @@
 _scalar = Union[str, bool, None]
 _nonscalar = Union[Dict[str, _stub], List[_stub]]
 _value = Union[_scalar, _nonscalar]
-# TreeValue = TODO, in a forthcoming commit.
+TreeValue = Union[_value, 'Annotated[_value]']
 
 
-def _make_tree(obj, ifcond, comment=None):
-    extra = {
-        'if': ifcond,
-        'comment': comment
-    }
-    return (obj, extra)
+_NodeT = TypeVar('_NodeT', bound=_value)
+
+
+class Annotated(Generic[_NodeT]):
+    """
+    Annotated generally contains a SchemaInfo-like type (as a dict),
+    But it also used to wrap comments/ifconds around scalar leaf values,
+    for the benefit of features and enums.
+    """
+    # TODO: Remove after Python 3.7 adds @dataclass:
+    # pylint: disable=too-few-public-methods
+    def __init__(self, value: _NodeT, ifcond: Iterable[str],
+                 comment: Optional[str] = None):
+        self.value = value
+        self.comment: Optional[str] = comment
+        self.ifcond: Tuple[str, ...] = tuple(ifcond)
 
 
 def _tree_to_qlit(obj, level=0, dict_value=False):
@@ -67,24 +81,20 @@ def _tree_to_qlit(obj, level=0, dict_value=False):
     def indent(level):
         return level * 4 * ' '
 
-    if isinstance(obj, tuple):
-        ifobj, extra = obj
-        ifcond = extra.get('if')
-        comment = extra.get('comment')
-
+    if isinstance(obj, Annotated):
         # NB: _tree_to_qlit is called recursively on the values of a key:value
         # pair; those values can't be decorated with comments or conditionals.
         msg = "dict values cannot have attached comments or if-conditionals."
         assert not dict_value, msg
 
         ret = ''
-        if comment:
-            ret += indent(level) + '/* %s */\n' % comment
-        if ifcond:
-            ret += gen_if(ifcond)
-        ret += _tree_to_qlit(ifobj, level)
-        if ifcond:
-            ret += '\n' + gen_endif(ifcond)
+        if obj.comment:
+            ret += indent(level) + '/* %s */\n' % obj.comment
+        if obj.ifcond:
+            ret += gen_if(obj.ifcond)
+        ret += _tree_to_qlit(obj.value, level)
+        if obj.ifcond:
+            ret += '\n' + gen_endif(obj.ifcond)
         return ret
 
     ret = ''
@@ -201,7 +211,7 @@ def _use_type(self, typ):
 
     @staticmethod
     def _gen_features(features):
-        return [_make_tree(f.name, f.ifcond) for f in features]
+        return [Annotated(f.name, f.ifcond) for f in features]
 
     def _gen_tree(self, name, mtype, obj, ifcond, features):
         comment: Optional[str] = None
@@ -215,7 +225,7 @@ def _gen_tree(self, name, mtype, obj, ifcond, features):
         obj['meta-type'] = mtype
         if features:
             obj['features'] = self._gen_features(features)
-        self._trees.append(_make_tree(obj, ifcond, comment))
+        self._trees.append(Annotated(obj, ifcond, comment))
 
     def _gen_member(self, member):
         obj = {'name': member.name, 'type': self._use_type(member.type)}
@@ -223,7 +233,7 @@ def _gen_member(self, member):
             obj['default'] = None
         if member.features:
             obj['features'] = self._gen_features(member.features)
-        return _make_tree(obj, member.ifcond)
+        return Annotated(obj, member.ifcond)
 
     def _gen_variants(self, tag_name, variants):
         return {'tag': tag_name,
@@ -231,16 +241,17 @@ def _gen_variants(self, tag_name, variants):
 
     def _gen_variant(self, variant):
         obj = {'case': variant.name, 'type': self._use_type(variant.type)}
-        return _make_tree(obj, variant.ifcond)
+        return Annotated(obj, variant.ifcond)
 
     def visit_builtin_type(self, name, info, json_type):
         self._gen_tree(name, 'builtin', {'json-type': json_type}, [], None)
 
     def visit_enum_type(self, name, info, ifcond, features, members, prefix):
-        self._gen_tree(name, 'enum',
-                       {'values': [_make_tree(m.name, m.ifcond, None)
-                                   for m in members]},
-                       ifcond, features)
+        self._gen_tree(
+            name, 'enum',
+            {'values': [Annotated(m.name, m.ifcond) for m in members]},
+            ifcond, features
+        )
 
     def visit_array_type(self, name, info, ifcond, element_type):
         element = self._use_type(element_type)
@@ -257,12 +268,12 @@ def visit_object_type_flat(self, name, info, ifcond, features,
         self._gen_tree(name, 'object', obj, ifcond, features)
 
     def visit_alternate_type(self, name, info, ifcond, features, variants):
-        self._gen_tree(name, 'alternate',
-                       {'members': [
-                           _make_tree({'type': self._use_type(m.type)},
-                                      m.ifcond, None)
-                           for m in variants.variants]},
-                       ifcond, features)
+        self._gen_tree(
+            name, 'alternate',
+            {'members': [Annotated({'type': self._use_type(m.type)}, m.ifcond)
+                         for m in variants.variants]},
+            ifcond, features
+        )
 
     def visit_command(self, name, info, ifcond, features,
                       arg_type, ret_type, gen, success_response, boxed,
-- 
2.29.2

[PATCH v5 10/15] qapi/introspect.py: improve _tree_to_qlit error message

[John Snow]

Trivial; make the error message just a pinch more explicit in case we
trip this by accident in the future.

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

diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
index b9427aba449..0146e4c6a0c 100644
--- a/scripts/qapi/introspect.py
+++ b/scripts/qapi/introspect.py
@@ -124,7 +124,9 @@ def indent(level):
     elif isinstance(obj, bool):
         ret += 'QLIT_QBOOL(%s)' % ('true' if obj else 'false')
     else:
-        assert False                # not implemented
+        raise NotImplementedError(
+            f"type '{type(obj).__name__}' not implemented"
+        )
     if level > 0:
         ret += ','
     return ret
-- 
2.29.2

[PATCH v5 11/15] qapi/introspect.py: improve readability of _tree_to_qlit

[John Snow]

Subjective, but I find getting rid of the comprehensions helps. Also,
divide the sections into scalar and non-scalar sections, and remove
old-style string formatting.

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

diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
index 0146e4c6a0c..cf0e4e05c5c 100644
--- a/scripts/qapi/introspect.py
+++ b/scripts/qapi/introspect.py
@@ -89,7 +89,7 @@ def indent(level):
 
         ret = ''
         if obj.comment:
-            ret += indent(level) + '/* %s */\n' % obj.comment
+            ret += indent(level) + f"/* {obj.comment} */\n"
         if obj.ifcond:
             ret += gen_if(obj.ifcond)
         ret += _tree_to_qlit(obj.value, level)
@@ -100,33 +100,36 @@ def indent(level):
     ret = ''
     if not dict_value:
         ret += indent(level)
+
+    # Scalars:
     if obj is None:
         ret += 'QLIT_QNULL'
     elif isinstance(obj, str):
-        ret += 'QLIT_QSTR(' + to_c_string(obj) + ')'
+        ret += f"QLIT_QSTR({to_c_string(obj)})"
+    elif isinstance(obj, bool):
+        ret += f"QLIT_QBOOL({str(obj).lower()})"
+
+    # Non-scalars:
     elif isinstance(obj, list):
-        elts = [_tree_to_qlit(elt, level + 1).strip('\n')
-                for elt in obj]
-        elts.append(indent(level + 1) + "{}")
         ret += 'QLIT_QLIST(((QLitObject[]) {\n'
-        ret += '\n'.join(elts) + '\n'
+        for value in obj:
+            ret += _tree_to_qlit(value, level + 1).strip('\n') + '\n'
+        ret += indent(level + 1) + '{}\n'
         ret += indent(level) + '}))'
     elif isinstance(obj, dict):
-        elts = []
-        for key, value in sorted(obj.items()):
-            elts.append(indent(level + 1) + '{ %s, %s }' %
-                        (to_c_string(key),
-                         _tree_to_qlit(value, level + 1, True)))
-        elts.append(indent(level + 1) + '{}')
         ret += 'QLIT_QDICT(((QLitDictEntry[]) {\n'
-        ret += ',\n'.join(elts) + '\n'
+        for key, value in sorted(obj.items()):
+            ret += indent(level + 1) + "{{ {:s}, {:s} }},\n".format(
+                to_c_string(key),
+                _tree_to_qlit(value, level + 1, dict_value=True)
+            )
+        ret += indent(level + 1) + '{}\n'
         ret += indent(level) + '}))'
-    elif isinstance(obj, bool):
-        ret += 'QLIT_QBOOL(%s)' % ('true' if obj else 'false')
     else:
         raise NotImplementedError(
             f"type '{type(obj).__name__}' not implemented"
         )
+
     if level > 0:
         ret += ','
     return ret
-- 
2.29.2

[PATCH v5 12/15] qapi/introspect.py: add type hint annotations

[John Snow]

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

---

See the next patch for an optional amendment that helps to clarify what
_DObject is meant to be.

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

diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
index cf0e4e05c5c..3afcdda7446 100644
--- a/scripts/qapi/introspect.py
+++ b/scripts/qapi/introspect.py
@@ -30,10 +30,19 @@
 )
 from .gen import QAPISchemaMonolithicCVisitor
 from .schema import (
+    QAPISchema,
     QAPISchemaArrayType,
     QAPISchemaBuiltinType,
+    QAPISchemaEntity,
+    QAPISchemaEnumMember,
+    QAPISchemaFeature,
+    QAPISchemaObjectType,
+    QAPISchemaObjectTypeMember,
     QAPISchemaType,
+    QAPISchemaVariant,
+    QAPISchemaVariants,
 )
+from .source import QAPISourceInfo
 
 
 # This module constructs a tree data structure that is used to
@@ -57,6 +66,10 @@
 _value = Union[_scalar, _nonscalar]
 TreeValue = Union[_value, 'Annotated[_value]']
 
+# This is an alias for an arbitrary JSON object, represented here as a Dict.
+# It is stricter on the value type than the recursive definition above.
+# It is used to represent SchemaInfo-related structures exclusively.
+_DObject = Dict[str, object]
 
 _NodeT = TypeVar('_NodeT', bound=_value)
 
@@ -76,9 +89,11 @@ def __init__(self, value: _NodeT, ifcond: Iterable[str],
         self.ifcond: Tuple[str, ...] = tuple(ifcond)
 
 
-def _tree_to_qlit(obj, level=0, dict_value=False):
+def _tree_to_qlit(obj: TreeValue,
+                  level: int = 0,
+                  dict_value: bool = False) -> str:
 
-    def indent(level):
+    def indent(level: int) -> str:
         return level * 4 * ' '
 
     if isinstance(obj, Annotated):
@@ -135,21 +150,21 @@ def indent(level):
     return ret
 
 
-def to_c_string(string):
+def to_c_string(string: str) -> str:
     return '"' + string.replace('\\', r'\\').replace('"', r'\"') + '"'
 
 
 class QAPISchemaGenIntrospectVisitor(QAPISchemaMonolithicCVisitor):
 
-    def __init__(self, prefix, unmask):
+    def __init__(self, prefix: str, unmask: bool):
         super().__init__(
             prefix, 'qapi-introspect',
             ' * QAPI/QMP schema introspection', __doc__)
         self._unmask = unmask
-        self._schema = None
-        self._trees = []
-        self._used_types = []
-        self._name_map = {}
+        self._schema: Optional[QAPISchema] = None
+        self._trees: List[Annotated[_DObject]] = []
+        self._used_types: List[QAPISchemaType] = []
+        self._name_map: Dict[str, str] = {}
         self._genc.add(mcgen('''
 #include "qemu/osdep.h"
 #include "%(prefix)sqapi-introspect.h"
@@ -157,10 +172,10 @@ def __init__(self, prefix, unmask):
 ''',
                              prefix=prefix))
 
-    def visit_begin(self, schema):
+    def visit_begin(self, schema: QAPISchema) -> None:
         self._schema = schema
 
-    def visit_end(self):
+    def visit_end(self) -> None:
         # visit the types that are actually used
         for typ in self._used_types:
             typ.visit(self)
@@ -182,18 +197,18 @@ def visit_end(self):
         self._used_types = []
         self._name_map = {}
 
-    def visit_needed(self, entity):
+    def visit_needed(self, entity: QAPISchemaEntity) -> bool:
         # Ignore types on first pass; visit_end() will pick up used types
         return not isinstance(entity, QAPISchemaType)
 
-    def _name(self, name):
+    def _name(self, name: str) -> str:
         if self._unmask:
             return name
         if name not in self._name_map:
             self._name_map[name] = '%d' % len(self._name_map)
         return self._name_map[name]
 
-    def _use_type(self, typ):
+    def _use_type(self, typ: QAPISchemaType) -> str:
         assert self._schema is not None
 
         # Map the various integer types to plain int
@@ -215,10 +230,13 @@ def _use_type(self, typ):
         return self._name(typ.name)
 
     @staticmethod
-    def _gen_features(features):
+    def _gen_features(features: List[QAPISchemaFeature]
+                      ) -> List[Annotated[str]]:
         return [Annotated(f.name, f.ifcond) for f in features]
 
-    def _gen_tree(self, name, mtype, obj, ifcond, features):
+    def _gen_tree(self, name: str, mtype: str, obj: _DObject,
+                  ifcond: List[str],
+                  features: Optional[List[QAPISchemaFeature]]) -> None:
         comment: Optional[str] = None
         if mtype not in ('command', 'event', 'builtin', 'array'):
             if not self._unmask:
@@ -232,47 +250,67 @@ def _gen_tree(self, name, mtype, obj, ifcond, features):
             obj['features'] = self._gen_features(features)
         self._trees.append(Annotated(obj, ifcond, comment))
 
-    def _gen_member(self, member):
-        obj = {'name': member.name, 'type': self._use_type(member.type)}
+    def _gen_member(self, member: QAPISchemaObjectTypeMember
+                    ) -> Annotated[_DObject]:
+        obj: _DObject = {
+            'name': member.name,
+            'type': self._use_type(member.type)
+        }
         if member.optional:
             obj['default'] = None
         if member.features:
             obj['features'] = self._gen_features(member.features)
         return Annotated(obj, member.ifcond)
 
-    def _gen_variants(self, tag_name, variants):
+    def _gen_variants(self, tag_name: str,
+                      variants: List[QAPISchemaVariant]) -> _DObject:
         return {'tag': tag_name,
                 'variants': [self._gen_variant(v) for v in variants]}
 
-    def _gen_variant(self, variant):
-        obj = {'case': variant.name, 'type': self._use_type(variant.type)}
+    def _gen_variant(self, variant: QAPISchemaVariant) -> Annotated[_DObject]:
+        obj: _DObject = {
+            'case': variant.name,
+            'type': self._use_type(variant.type)
+        }
         return Annotated(obj, variant.ifcond)
 
-    def visit_builtin_type(self, name, info, json_type):
+    def visit_builtin_type(self, name: str, info: Optional[QAPISourceInfo],
+                           json_type: str) -> None:
         self._gen_tree(name, 'builtin', {'json-type': json_type}, [], None)
 
-    def visit_enum_type(self, name, info, ifcond, features, members, prefix):
+    def visit_enum_type(self, name: str, info: Optional[QAPISourceInfo],
+                        ifcond: List[str], features: List[QAPISchemaFeature],
+                        members: List[QAPISchemaEnumMember],
+                        prefix: Optional[str]) -> None:
         self._gen_tree(
             name, 'enum',
             {'values': [Annotated(m.name, m.ifcond) for m in members]},
             ifcond, features
         )
 
-    def visit_array_type(self, name, info, ifcond, element_type):
+    def visit_array_type(self, name: str, info: Optional[QAPISourceInfo],
+                         ifcond: List[str],
+                         element_type: QAPISchemaType) -> None:
         element = self._use_type(element_type)
         self._gen_tree('[' + element + ']', 'array', {'element-type': element},
                        ifcond, None)
 
-    def visit_object_type_flat(self, name, info, ifcond, features,
-                               members, variants):
-        obj = {'members': [self._gen_member(m) for m in members]}
+    def visit_object_type_flat(self, name: str, info: Optional[QAPISourceInfo],
+                               ifcond: List[str],
+                               features: List[QAPISchemaFeature],
+                               members: List[QAPISchemaObjectTypeMember],
+                               variants: Optional[QAPISchemaVariants]) -> None:
+        obj: _DObject = {'members': [self._gen_member(m) for m in members]}
         if variants:
             obj.update(self._gen_variants(variants.tag_member.name,
                                           variants.variants))
 
         self._gen_tree(name, 'object', obj, ifcond, features)
 
-    def visit_alternate_type(self, name, info, ifcond, features, variants):
+    def visit_alternate_type(self, name: str, info: Optional[QAPISourceInfo],
+                             ifcond: List[str],
+                             features: List[QAPISchemaFeature],
+                             variants: QAPISchemaVariants) -> None:
         self._gen_tree(
             name, 'alternate',
             {'members': [Annotated({'type': self._use_type(m.type)}, m.ifcond)
@@ -280,27 +318,38 @@ def visit_alternate_type(self, name, info, ifcond, features, variants):
             ifcond, features
         )
 
-    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: List[str],
+                      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:
         assert self._schema is not None
 
         arg_type = arg_type or self._schema.the_empty_object_type
         ret_type = ret_type or self._schema.the_empty_object_type
-        obj = {'arg-type': self._use_type(arg_type),
-               'ret-type': self._use_type(ret_type)}
+        obj: _DObject = {
+            'arg-type': self._use_type(arg_type),
+            'ret-type': self._use_type(ret_type)
+        }
         if allow_oob:
             obj['allow-oob'] = allow_oob
         self._gen_tree(name, 'command', obj, ifcond, features)
 
-    def visit_event(self, name, info, ifcond, features, arg_type, boxed):
+    def visit_event(self, name: str, info: Optional[QAPISourceInfo],
+                    ifcond: List[str], features: List[QAPISchemaFeature],
+                    arg_type: Optional[QAPISchemaObjectType],
+                    boxed: bool) -> None:
         assert self._schema is not None
+
         arg_type = arg_type or self._schema.the_empty_object_type
         self._gen_tree(name, 'event', {'arg-type': self._use_type(arg_type)},
                        ifcond, features)
 
 
-def gen_introspect(schema, output_dir, prefix, opt_unmask):
+def gen_introspect(schema: QAPISchema, output_dir: str, prefix: str,
+                   opt_unmask: bool) -> None:
     vis = QAPISchemaGenIntrospectVisitor(prefix, opt_unmask)
     schema.visit(vis)
     vis.write(output_dir)
diff --git a/scripts/qapi/mypy.ini b/scripts/qapi/mypy.ini
index 04bd5db5278..0a000d58b37 100644
--- a/scripts/qapi/mypy.ini
+++ b/scripts/qapi/mypy.ini
@@ -13,11 +13,6 @@ disallow_untyped_defs = False
 disallow_incomplete_defs = False
 check_untyped_defs = False
 
-[mypy-qapi.introspect]
-disallow_untyped_defs = False
-disallow_incomplete_defs = False
-check_untyped_defs = False
-
 [mypy-qapi.parser]
 disallow_untyped_defs = False
 disallow_incomplete_defs = False
diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
index 353e8020a27..ff16578f6de 100644
--- a/scripts/qapi/schema.py
+++ b/scripts/qapi/schema.py
@@ -28,7 +28,7 @@
 class QAPISchemaEntity:
     meta: Optional[str] = None
 
-    def __init__(self, name, info, doc, ifcond=None, features=None):
+    def __init__(self, name: str, info, doc, ifcond=None, features=None):
         assert name is None or isinstance(name, str)
         for f in features or []:
             assert isinstance(f, QAPISchemaFeature)
-- 
2.29.2

[PATCH v5 13/15] qapi/introspect.py: add introspect.json dummy types

[John Snow]

Add some aliases that declare intent for some of the "dictly-typed"
objects we pass around in introspect.py.

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

---

This patch is optional, it can be dropped if desired. If it's taken,
it's probably best to squash it with the prior patch.  It is purely for
the sake of demonstrating what the _DObject was meant to convey: a
Python Dict that represents some JSON Object. It does not add any type
safety in and of itself, but does have some (light) annotational
benefit. In this case, it's usually a specific data structure from the
QAPI Schema we are referencing, but do not have "compile-time" access to
in Python.

These are loosely typed and don't bother reproducing the exact structure
of the real types. Python 3.6 does not have support for TypedDict
structures, so this is as good as we can do without involving a
third-party library (e.g. Pydantic), in which we might be able to say:


class SchemaMetaType(str, enum.Enum):
    BUILTIN = "builtin"
    ENUM = "enum"
    ARRAY = "array"
    OBJECT = "object"
    ALTERNATE = "alternate"
    COMMAND = "command"
    EVENT = "event"


class SchemaInfo(pydantic.BaseModel):
    name: str
    meta-type: SchemaMetaType
    features: Optional[List[str]]
    data: Union[SchemaInfoBuiltin, SchemaInfoEnum, SchemaInfoArray,
                SchemaInfoObject, SchemaInfoAlternate, SchemaInfoCommand,
                SchemaInfoEvent]


However, the cost of reproducing and keeping these structure definitions
in-sync between the user-defined portion of the schema and the code
generator is likely not worth doing any such thing. However, it does
illustrate an interesting dependency the generator has on the
user-defined schema itself in terms of types.

So, I settled on using some light types that suggest the form of the
object instead of enforcing the form.

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

diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
index 3afcdda7446..2a39726f40a 100644
--- a/scripts/qapi/introspect.py
+++ b/scripts/qapi/introspect.py
@@ -66,10 +66,15 @@
 _value = Union[_scalar, _nonscalar]
 TreeValue = Union[_value, 'Annotated[_value]']
 
-# This is an alias for an arbitrary JSON object, represented here as a Dict.
-# It is stricter on the value type than the recursive definition above.
-# It is used to represent SchemaInfo-related structures exclusively.
-_DObject = Dict[str, object]
+# These types are based on structures defined in QEMU's schema, so we lack
+# precise types for them here. Python 3.6 does not offer TypedDict constructs,
+# so they are broadly typed here as simple Python Dicts.
+SchemaInfo = Dict[str, object]
+SchemaInfoObject = Dict[str, object]
+SchemaInfoObjectVariant = Dict[str, object]
+SchemaInfoObjectMember = Dict[str, object]
+SchemaInfoCommand = Dict[str, object]
+
 
 _NodeT = TypeVar('_NodeT', bound=_value)
 
@@ -162,7 +167,7 @@ def __init__(self, prefix: str, unmask: bool):
             ' * QAPI/QMP schema introspection', __doc__)
         self._unmask = unmask
         self._schema: Optional[QAPISchema] = None
-        self._trees: List[Annotated[_DObject]] = []
+        self._trees: List[Annotated[SchemaInfo]] = []
         self._used_types: List[QAPISchemaType] = []
         self._name_map: Dict[str, str] = {}
         self._genc.add(mcgen('''
@@ -234,9 +239,18 @@ def _gen_features(features: List[QAPISchemaFeature]
                       ) -> List[Annotated[str]]:
         return [Annotated(f.name, f.ifcond) for f in features]
 
-    def _gen_tree(self, name: str, mtype: str, obj: _DObject,
+    def _gen_tree(self, name: str, mtype: str, obj: Dict[str, object],
                   ifcond: List[str],
                   features: Optional[List[QAPISchemaFeature]]) -> None:
+        """
+        Build and append a SchemaInfo object to self._trees.
+
+        :param name: The entity's name.
+        :param mtype: The entity's meta-type.
+        :param obj: Additional entity fields, as appropriate for the meta-type.
+        :param ifcond: List of conditionals that apply to this entire entity.
+        :param features: Optional features field for SchemaInfo.
+        """
         comment: Optional[str] = None
         if mtype not in ('command', 'event', 'builtin', 'array'):
             if not self._unmask:
@@ -251,8 +265,8 @@ def _gen_tree(self, name: str, mtype: str, obj: _DObject,
         self._trees.append(Annotated(obj, ifcond, comment))
 
     def _gen_member(self, member: QAPISchemaObjectTypeMember
-                    ) -> Annotated[_DObject]:
-        obj: _DObject = {
+                    ) -> Annotated[SchemaInfoObjectMember]:
+        obj: SchemaInfoObjectMember = {
             'name': member.name,
             'type': self._use_type(member.type)
         }
@@ -262,13 +276,9 @@ def _gen_member(self, member: QAPISchemaObjectTypeMember
             obj['features'] = self._gen_features(member.features)
         return Annotated(obj, member.ifcond)
 
-    def _gen_variants(self, tag_name: str,
-                      variants: List[QAPISchemaVariant]) -> _DObject:
-        return {'tag': tag_name,
-                'variants': [self._gen_variant(v) for v in variants]}
-
-    def _gen_variant(self, variant: QAPISchemaVariant) -> Annotated[_DObject]:
-        obj: _DObject = {
+    def _gen_variant(self, variant: QAPISchemaVariant
+                     ) -> Annotated[SchemaInfoObjectVariant]:
+        obj: SchemaInfoObjectVariant = {
             'case': variant.name,
             'type': self._use_type(variant.type)
         }
@@ -300,11 +310,12 @@ def visit_object_type_flat(self, name: str, info: Optional[QAPISourceInfo],
                                features: List[QAPISchemaFeature],
                                members: List[QAPISchemaObjectTypeMember],
                                variants: Optional[QAPISchemaVariants]) -> None:
-        obj: _DObject = {'members': [self._gen_member(m) for m in members]}
+        obj: SchemaInfoObject = {
+            'members': [self._gen_member(m) for m in members]
+        }
         if variants:
-            obj.update(self._gen_variants(variants.tag_member.name,
-                                          variants.variants))
-
+            obj['tag'] = variants.tag_member.name
+            obj['variants'] = [self._gen_variant(v) for v in variants.variants]
         self._gen_tree(name, 'object', obj, ifcond, features)
 
     def visit_alternate_type(self, name: str, info: Optional[QAPISourceInfo],
@@ -329,7 +340,7 @@ def visit_command(self, name: str, info: Optional[QAPISourceInfo],
 
         arg_type = arg_type or self._schema.the_empty_object_type
         ret_type = ret_type or self._schema.the_empty_object_type
-        obj: _DObject = {
+        obj: SchemaInfoCommand = {
             'arg-type': self._use_type(arg_type),
             'ret-type': self._use_type(ret_type)
         }
-- 
2.29.2

[PATCH v5 14/15] qapi/introspect.py: Add docstring to _tree_to_qlit

[John Snow]

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

diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
index 2a39726f40a..2b338abe2cf 100644
--- a/scripts/qapi/introspect.py
+++ b/scripts/qapi/introspect.py
@@ -97,6 +97,14 @@ def __init__(self, value: _NodeT, ifcond: Iterable[str],
 def _tree_to_qlit(obj: TreeValue,
                   level: int = 0,
                   dict_value: bool = False) -> str:
+    """
+    Convert the type tree into a QLIT C string, recursively.
+
+    :param obj: The value to convert.
+    :param level: The indentation level for this particular value.
+    :param dict_value: True when the value being processed belongs to a
+                       dict key; which suppresses the output indent.
+    """
 
     def indent(level: int) -> str:
         return level * 4 * ' '
-- 
2.29.2

[PATCH v5 15/15] qapi/introspect.py: Update copyright and authors list

[John Snow]

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

diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
index 2b338abe2cf..fd0ca0aba21 100644
--- a/scripts/qapi/introspect.py
+++ b/scripts/qapi/introspect.py
@@ -1,10 +1,11 @@
 """
 QAPI introspection generator
 
-Copyright (C) 2015-2018 Red Hat, Inc.
+Copyright (C) 2015-2021 Red Hat, Inc.
 
 Authors:
  Markus Armbruster <armbru@redhat.com>
+ John Snow <jsnow@redhat.com>
 
 This work is licensed under the terms of the GNU GPL, version 2.
 See the COPYING file in the top-level directory.
-- 
2.29.2

Re: [PATCH v5 08/15] qapi/introspect.py: Introduce preliminary tree typing

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> The types will be used in forthcoming patches to add typing. These types
> describe the layout and structure of the objects passed to
> _tree_to_qlit, but lack the power to describe annotations until the next
> commit.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  scripts/qapi/introspect.py | 30 +++++++++++++++++++++++++++++-
>  1 file changed, 29 insertions(+), 1 deletion(-)
>
> diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
> index 45231d2abc3..8e019b4a26a 100644
> --- a/scripts/qapi/introspect.py
> +++ b/scripts/qapi/introspect.py
> @@ -10,7 +10,13 @@
>  See the COPYING file in the top-level directory.
>  """
>  
> -from typing import Optional
> +from typing import (
> +    Any,
> +    Dict,
> +    List,
> +    Optional,
> +    Union,
> +)
>  
>  from .common import (
>      c_name,
> @@ -26,6 +32,28 @@
>  )
>  
>  
> +# This module constructs a tree data structure that is used to
> +# generate the introspection information for QEMU. It behaves similarly
> +# to a JSON value.
> +#
> +# A complexity over JSON is that our values may or may not be annotated.
> +#
> +# Un-annotated values may be:
> +#     Scalar: str, bool, None.
> +#     Non-scalar: List, Dict
> +# _value = Union[str, bool, None, Dict[str, TreeValue], List[TreeValue]]
> +#
> +# With optional annotations, the type of all values is:
> +# TreeValue = Union[_value, Annotated[_value]]
> +#
> +# Sadly, mypy does not support recursive types, so we must approximate this.
> +_stub = Any
> +_scalar = Union[str, bool, None]
> +_nonscalar = Union[Dict[str, _stub], List[_stub]]
> +_value = Union[_scalar, _nonscalar]
> +# TreeValue = TODO, in a forthcoming commit.
> +
> +
>  def _make_tree(obj, ifcond, comment=None):
>      extra = {
>          'if': ifcond,

We discussed the naming of these type aliases review of v4.  v5 arrived
before we reached a conclusion.  That's okay.  I'm still interested in
finishing the discussion, of course.

Re: [PATCH v5 09/15] qapi/introspect.py: create a typed 'Annotated' data strutcure

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> Presently, we use a tuple to attach a dict containing annotations
> (comments and compile-time conditionals) to a tree node. This is
> undesirable because dicts are difficult to strongly type; promoting it
> to a real class allows us to name the values and types of the
> annotations we are expecting.
>
> In terms of typing, the Annotated<T> type serves as a generic container
> where the annotated node's type is preserved, allowing for greater
> specificity than we'd be able to provide without a generic.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  scripts/qapi/introspect.py | 77 ++++++++++++++++++++++----------------
>  1 file changed, 44 insertions(+), 33 deletions(-)
>
> diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
> index 8e019b4a26a..b9427aba449 100644
> --- a/scripts/qapi/introspect.py
> +++ b/scripts/qapi/introspect.py
> @@ -13,8 +13,12 @@
>  from typing import (
>      Any,
>      Dict,
> +    Generic,
> +    Iterable,
>      List,
>      Optional,
> +    Tuple,
> +    TypeVar,
>      Union,
>  )
>  
> @@ -51,15 +55,25 @@
>  _scalar = Union[str, bool, None]
>  _nonscalar = Union[Dict[str, _stub], List[_stub]]
>  _value = Union[_scalar, _nonscalar]
> -# TreeValue = TODO, in a forthcoming commit.
> +TreeValue = Union[_value, 'Annotated[_value]']
>  
>  
> -def _make_tree(obj, ifcond, comment=None):
> -    extra = {
> -        'if': ifcond,
> -        'comment': comment
> -    }
> -    return (obj, extra)
> +_NodeT = TypeVar('_NodeT', bound=_value)
> +
> +
> +class Annotated(Generic[_NodeT]):

My gut feeling is "generic type is overkill for this purpose".  Let's go
with it anyway, because

1. It's not wrong.

2. I don't have enough experience with Python type hints for reliable
gut feelings.

3. I plan to overhaul the C generation part relatively soon (after your
work has landed, don't worry), and I can try to make it simpler then.

> +    """
> +    Annotated generally contains a SchemaInfo-like type (as a dict),
> +    But it also used to wrap comments/ifconds around scalar leaf values,
> +    for the benefit of features and enums.
> +    """
> +    # TODO: Remove after Python 3.7 adds @dataclass:
> +    # pylint: disable=too-few-public-methods
> +    def __init__(self, value: _NodeT, ifcond: Iterable[str],
> +                 comment: Optional[str] = None):
> +        self.value = value
> +        self.comment: Optional[str] = comment
> +        self.ifcond: Tuple[str, ...] = tuple(ifcond)
>  
>  
>  def _tree_to_qlit(obj, level=0, dict_value=False):
> @@ -67,24 +81,20 @@ def _tree_to_qlit(obj, level=0, dict_value=False):
>      def indent(level):
>          return level * 4 * ' '
>  
> -    if isinstance(obj, tuple):
> -        ifobj, extra = obj
> -        ifcond = extra.get('if')
> -        comment = extra.get('comment')
> -
> +    if isinstance(obj, Annotated):
>          # NB: _tree_to_qlit is called recursively on the values of a key:value
>          # pair; those values can't be decorated with comments or conditionals.
>          msg = "dict values cannot have attached comments or if-conditionals."
>          assert not dict_value, msg
>  
>          ret = ''
> -        if comment:
> -            ret += indent(level) + '/* %s */\n' % comment
> -        if ifcond:
> -            ret += gen_if(ifcond)
> -        ret += _tree_to_qlit(ifobj, level)
> -        if ifcond:
> -            ret += '\n' + gen_endif(ifcond)
> +        if obj.comment:
> +            ret += indent(level) + '/* %s */\n' % obj.comment
> +        if obj.ifcond:
> +            ret += gen_if(obj.ifcond)
> +        ret += _tree_to_qlit(obj.value, level)
> +        if obj.ifcond:
> +            ret += '\n' + gen_endif(obj.ifcond)
>          return ret
>  
>      ret = ''
> @@ -201,7 +211,7 @@ def _use_type(self, typ):
>  
>      @staticmethod
>      def _gen_features(features):
> -        return [_make_tree(f.name, f.ifcond) for f in features]
> +        return [Annotated(f.name, f.ifcond) for f in features]
>  
>      def _gen_tree(self, name, mtype, obj, ifcond, features):
>          comment: Optional[str] = None
> @@ -215,7 +225,7 @@ def _gen_tree(self, name, mtype, obj, ifcond, features):
>          obj['meta-type'] = mtype
>          if features:
>              obj['features'] = self._gen_features(features)
> -        self._trees.append(_make_tree(obj, ifcond, comment))
> +        self._trees.append(Annotated(obj, ifcond, comment))
>  
>      def _gen_member(self, member):
>          obj = {'name': member.name, 'type': self._use_type(member.type)}
> @@ -223,7 +233,7 @@ def _gen_member(self, member):
>              obj['default'] = None
>          if member.features:
>              obj['features'] = self._gen_features(member.features)
> -        return _make_tree(obj, member.ifcond)
> +        return Annotated(obj, member.ifcond)
>  
>      def _gen_variants(self, tag_name, variants):
>          return {'tag': tag_name,
> @@ -231,16 +241,17 @@ def _gen_variants(self, tag_name, variants):
>  
>      def _gen_variant(self, variant):
>          obj = {'case': variant.name, 'type': self._use_type(variant.type)}
> -        return _make_tree(obj, variant.ifcond)
> +        return Annotated(obj, variant.ifcond)
>  
>      def visit_builtin_type(self, name, info, json_type):
>          self._gen_tree(name, 'builtin', {'json-type': json_type}, [], None)
>  
>      def visit_enum_type(self, name, info, ifcond, features, members, prefix):
> -        self._gen_tree(name, 'enum',
> -                       {'values': [_make_tree(m.name, m.ifcond, None)
> -                                   for m in members]},
> -                       ifcond, features)
> +        self._gen_tree(
> +            name, 'enum',
> +            {'values': [Annotated(m.name, m.ifcond) for m in members]},
> +            ifcond, features
> +        )
>  
>      def visit_array_type(self, name, info, ifcond, element_type):
>          element = self._use_type(element_type)
> @@ -257,12 +268,12 @@ def visit_object_type_flat(self, name, info, ifcond, features,
>          self._gen_tree(name, 'object', obj, ifcond, features)
>  
>      def visit_alternate_type(self, name, info, ifcond, features, variants):
> -        self._gen_tree(name, 'alternate',
> -                       {'members': [
> -                           _make_tree({'type': self._use_type(m.type)},
> -                                      m.ifcond, None)
> -                           for m in variants.variants]},
> -                       ifcond, features)
> +        self._gen_tree(
> +            name, 'alternate',
> +            {'members': [Annotated({'type': self._use_type(m.type)}, m.ifcond)
> +                         for m in variants.variants]},

Slightly more readable, I think:

               {'members': [Annotated({'type': self._use_type(m.type)}, 
                                      m.ifcond)
                            for m in variants.variants]},

> +            ifcond, features
> +        )
>  
>      def visit_command(self, name, info, ifcond, features,
>                        arg_type, ret_type, gen, success_response, boxed,

Re: [PATCH v5 12/15] qapi/introspect.py: add type hint annotations

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> Signed-off-by: John Snow <jsnow@redhat.com>
>
> ---
>
> See the next patch for an optional amendment that helps to clarify what
> _DObject is meant to be.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  scripts/qapi/introspect.py | 117 ++++++++++++++++++++++++++-----------
>  scripts/qapi/mypy.ini      |   5 --
>  scripts/qapi/schema.py     |   2 +-
>  3 files changed, 84 insertions(+), 40 deletions(-)
>
> diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
> index cf0e4e05c5c..3afcdda7446 100644
> --- a/scripts/qapi/introspect.py
> +++ b/scripts/qapi/introspect.py
> @@ -30,10 +30,19 @@
>  )
>  from .gen import QAPISchemaMonolithicCVisitor
>  from .schema import (
> +    QAPISchema,
>      QAPISchemaArrayType,
>      QAPISchemaBuiltinType,
> +    QAPISchemaEntity,
> +    QAPISchemaEnumMember,
> +    QAPISchemaFeature,
> +    QAPISchemaObjectType,
> +    QAPISchemaObjectTypeMember,
>      QAPISchemaType,
> +    QAPISchemaVariant,
> +    QAPISchemaVariants,
>  )
> +from .source import QAPISourceInfo
>  
>  
>  # This module constructs a tree data structure that is used to
> @@ -57,6 +66,10 @@
   _stub = Any
   _scalar = Union[str, bool, None]
   _nonscalar = Union[Dict[str, _stub], List[_stub]]
>  _value = Union[_scalar, _nonscalar]
>  TreeValue = Union[_value, 'Annotated[_value]']
>  
> +# This is an alias for an arbitrary JSON object, represented here as a Dict.
> +# It is stricter on the value type than the recursive definition above.
> +# It is used to represent SchemaInfo-related structures exclusively.
> +_DObject = Dict[str, object]

Please work in an abridged version of your helpful reply to my ignorant
questions in review of v4.

My comments below are based on the following understanding:

_value has a Dict[str, Any] branch.

_DObject is Dict[str, object].

Both types are for the tree's dict nodes.  Both under-constrain the
dict's values in the sense that anything can go into the dict.  The
difference is static type checking on use of dict values: none with the
former, overly strict with the latter (to actually do something
interesting with a value, you usually have to narrow its static type to
a more specific one than object).

So, having _DObject in addition to the _value branch buys us a little
more static type checking.  I don't understand what type checking
exactly, and therefore can't judge whether it's worth the price in
complexity.  If you can enlighten me, I'm all ears.

>  
>  _NodeT = TypeVar('_NodeT', bound=_value)
>  
> @@ -76,9 +89,11 @@ def __init__(self, value: _NodeT, ifcond: Iterable[str],
>          self.ifcond: Tuple[str, ...] = tuple(ifcond)
>  
>  
> -def _tree_to_qlit(obj, level=0, dict_value=False):
> +def _tree_to_qlit(obj: TreeValue,
> +                  level: int = 0,
> +                  dict_value: bool = False) -> str:
>  
> -    def indent(level):
> +    def indent(level: int) -> str:
>          return level * 4 * ' '
>  
>      if isinstance(obj, Annotated):
> @@ -135,21 +150,21 @@ def indent(level):
>      return ret
>  
>  
> -def to_c_string(string):
> +def to_c_string(string: str) -> str:
>      return '"' + string.replace('\\', r'\\').replace('"', r'\"') + '"'
>  
>  
>  class QAPISchemaGenIntrospectVisitor(QAPISchemaMonolithicCVisitor):
>  
> -    def __init__(self, prefix, unmask):
> +    def __init__(self, prefix: str, unmask: bool):
>          super().__init__(
>              prefix, 'qapi-introspect',
>              ' * QAPI/QMP schema introspection', __doc__)
>          self._unmask = unmask
> -        self._schema = None
> -        self._trees = []
> -        self._used_types = []
> -        self._name_map = {}
> +        self._schema: Optional[QAPISchema] = None
> +        self._trees: List[Annotated[_DObject]] = []
> +        self._used_types: List[QAPISchemaType] = []
> +        self._name_map: Dict[str, str] = {}
>          self._genc.add(mcgen('''
>  #include "qemu/osdep.h"
>  #include "%(prefix)sqapi-introspect.h"
> @@ -157,10 +172,10 @@ def __init__(self, prefix, unmask):
>  ''',
>                               prefix=prefix))
>  
> -    def visit_begin(self, schema):
> +    def visit_begin(self, schema: QAPISchema) -> None:
>          self._schema = schema
>  
> -    def visit_end(self):
> +    def visit_end(self) -> None:
>          # visit the types that are actually used
>          for typ in self._used_types:
>              typ.visit(self)
> @@ -182,18 +197,18 @@ def visit_end(self):
>          self._used_types = []
>          self._name_map = {}
>  
> -    def visit_needed(self, entity):
> +    def visit_needed(self, entity: QAPISchemaEntity) -> bool:
>          # Ignore types on first pass; visit_end() will pick up used types
>          return not isinstance(entity, QAPISchemaType)
>  
> -    def _name(self, name):
> +    def _name(self, name: str) -> str:
>          if self._unmask:
>              return name
>          if name not in self._name_map:
>              self._name_map[name] = '%d' % len(self._name_map)
>          return self._name_map[name]
>  
> -    def _use_type(self, typ):
> +    def _use_type(self, typ: QAPISchemaType) -> str:
>          assert self._schema is not None
>  
>          # Map the various integer types to plain int
> @@ -215,10 +230,13 @@ def _use_type(self, typ):
>          return self._name(typ.name)
>  
>      @staticmethod
> -    def _gen_features(features):
> +    def _gen_features(features: List[QAPISchemaFeature]
> +                      ) -> List[Annotated[str]]:
>          return [Annotated(f.name, f.ifcond) for f in features]
>  
> -    def _gen_tree(self, name, mtype, obj, ifcond, features):
> +    def _gen_tree(self, name: str, mtype: str, obj: _DObject,
> +                  ifcond: List[str],
> +                  features: Optional[List[QAPISchemaFeature]]) -> None:

Recycling my review of v2...

* @name corresponds to SchemaInfo member name, which is indeed str.
  Okay.

* @mtype corresponds to member meta-type, which is enum SchemaMetaType.
  It's not a Python Enum, because those were off limits when this code
  was written.  We type what we have, and we have str.  Okay.

* @obj will be turned by _gen_tree() into the tree for a SchemaInfo.
  _DObject fits the bill.

* ifcond: List[str] should work, but gen_if(), gen_endif() use
  Sequence[str].  When I pointed this out for v2, you replied "should
  probable be using Sequence".

  More instances of ifcond: List[str] elsewhere; I'm not flagging them.

* features: Optional[List[QAPISchemaFeature]] is correct.  "No features"
  has two representations: None and [].  I guess we could eliminate
  None, trading a tiny bit of efficiency for simpler typing.  Not a
  demand.

>          comment: Optional[str] = None

"No annotations" is represented as None here, not {}.  I guess we could
use {} for simpler typing.  When I pointed this out for v2, you replied
it'll go away later.  Good enough for me.

>          if mtype not in ('command', 'event', 'builtin', 'array'):
>              if not self._unmask:
> @@ -232,47 +250,67 @@ def _gen_tree(self, name, mtype, obj, ifcond, features):
>              obj['features'] = self._gen_features(features)
>          self._trees.append(Annotated(obj, ifcond, comment))
>  
> -    def _gen_member(self, member):
> -        obj = {'name': member.name, 'type': self._use_type(member.type)}
> +    def _gen_member(self, member: QAPISchemaObjectTypeMember
> +                    ) -> Annotated[_DObject]:
> +        obj: _DObject = {
> +            'name': member.name,
> +            'type': self._use_type(member.type)
> +        }
>          if member.optional:
>              obj['default'] = None
>          if member.features:
>              obj['features'] = self._gen_features(member.features)
>          return Annotated(obj, member.ifcond)
>  
> -    def _gen_variants(self, tag_name, variants):
> +    def _gen_variants(self, tag_name: str,
> +                      variants: List[QAPISchemaVariant]) -> _DObject:
>          return {'tag': tag_name,
>                  'variants': [self._gen_variant(v) for v in variants]}
>  
> -    def _gen_variant(self, variant):
> -        obj = {'case': variant.name, 'type': self._use_type(variant.type)}
> +    def _gen_variant(self, variant: QAPISchemaVariant) -> Annotated[_DObject]:
> +        obj: _DObject = {
> +            'case': variant.name,
> +            'type': self._use_type(variant.type)
> +        }
>          return Annotated(obj, variant.ifcond)
>  
> -    def visit_builtin_type(self, name, info, json_type):
> +    def visit_builtin_type(self, name: str, info: Optional[QAPISourceInfo],
> +                           json_type: str) -> None:

A built-in type's info is always None.  Perhaps we should drop the
parameter.

>          self._gen_tree(name, 'builtin', {'json-type': json_type}, [], None)
>  
> -    def visit_enum_type(self, name, info, ifcond, features, members, prefix):
> +    def visit_enum_type(self, name: str, info: Optional[QAPISourceInfo],
> +                        ifcond: List[str], features: List[QAPISchemaFeature],
> +                        members: List[QAPISchemaEnumMember],
> +                        prefix: Optional[str]) -> None:
>          self._gen_tree(
>              name, 'enum',
>              {'values': [Annotated(m.name, m.ifcond) for m in members]},
>              ifcond, features
>          )
>  
> -    def visit_array_type(self, name, info, ifcond, element_type):
> +    def visit_array_type(self, name: str, info: Optional[QAPISourceInfo],
> +                         ifcond: List[str],
> +                         element_type: QAPISchemaType) -> None:
>          element = self._use_type(element_type)
>          self._gen_tree('[' + element + ']', 'array', {'element-type': element},
>                         ifcond, None)
>  
> -    def visit_object_type_flat(self, name, info, ifcond, features,
> -                               members, variants):
> -        obj = {'members': [self._gen_member(m) for m in members]}
> +    def visit_object_type_flat(self, name: str, info: Optional[QAPISourceInfo],
> +                               ifcond: List[str],
> +                               features: List[QAPISchemaFeature],
> +                               members: List[QAPISchemaObjectTypeMember],
> +                               variants: Optional[QAPISchemaVariants]) -> None:

We represent "no variants" as None, not as [].  I guess we could
eliminate use [], trading a tiny bit of efficiency for simpler typing.
When I pointed this out for v2, you replied we should turn
QAPISchemaVariants into an extension of Sequence[QAPISchemaVariant] in a
later series.  Okay.

> +        obj: _DObject = {'members': [self._gen_member(m) for m in members]}
>          if variants:
>              obj.update(self._gen_variants(variants.tag_member.name,
>                                            variants.variants))
>  
>          self._gen_tree(name, 'object', obj, ifcond, features)
>  
> -    def visit_alternate_type(self, name, info, ifcond, features, variants):
> +    def visit_alternate_type(self, name: str, info: Optional[QAPISourceInfo],
> +                             ifcond: List[str],
> +                             features: List[QAPISchemaFeature],
> +                             variants: QAPISchemaVariants) -> None:
>          self._gen_tree(
>              name, 'alternate',
>              {'members': [Annotated({'type': self._use_type(m.type)}, m.ifcond)
> @@ -280,27 +318,38 @@ def visit_alternate_type(self, name, info, ifcond, features, variants):
>              ifcond, features
>          )
>  
> -    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: List[str],
> +                      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:
>          assert self._schema is not None
>  
>          arg_type = arg_type or self._schema.the_empty_object_type
>          ret_type = ret_type or self._schema.the_empty_object_type
> -        obj = {'arg-type': self._use_type(arg_type),
> -               'ret-type': self._use_type(ret_type)}
> +        obj: _DObject = {
> +            'arg-type': self._use_type(arg_type),
> +            'ret-type': self._use_type(ret_type)
> +        }
>          if allow_oob:
>              obj['allow-oob'] = allow_oob
>          self._gen_tree(name, 'command', obj, ifcond, features)
>  
> -    def visit_event(self, name, info, ifcond, features, arg_type, boxed):
> +    def visit_event(self, name: str, info: Optional[QAPISourceInfo],
> +                    ifcond: List[str], features: List[QAPISchemaFeature],
> +                    arg_type: Optional[QAPISchemaObjectType],
> +                    boxed: bool) -> None:
>          assert self._schema is not None
> +
>          arg_type = arg_type or self._schema.the_empty_object_type
>          self._gen_tree(name, 'event', {'arg-type': self._use_type(arg_type)},
>                         ifcond, features)
>  
>  
> -def gen_introspect(schema, output_dir, prefix, opt_unmask):
> +def gen_introspect(schema: QAPISchema, output_dir: str, prefix: str,
> +                   opt_unmask: bool) -> None:
>      vis = QAPISchemaGenIntrospectVisitor(prefix, opt_unmask)
>      schema.visit(vis)
>      vis.write(output_dir)
> diff --git a/scripts/qapi/mypy.ini b/scripts/qapi/mypy.ini
> index 04bd5db5278..0a000d58b37 100644
> --- a/scripts/qapi/mypy.ini
> +++ b/scripts/qapi/mypy.ini
> @@ -13,11 +13,6 @@ disallow_untyped_defs = False
>  disallow_incomplete_defs = False
>  check_untyped_defs = False
>  
> -[mypy-qapi.introspect]
> -disallow_untyped_defs = False
> -disallow_incomplete_defs = False
> -check_untyped_defs = False
> -
>  [mypy-qapi.parser]
>  disallow_untyped_defs = False
>  disallow_incomplete_defs = False
> diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
> index 353e8020a27..ff16578f6de 100644
> --- a/scripts/qapi/schema.py
> +++ b/scripts/qapi/schema.py
> @@ -28,7 +28,7 @@
>  class QAPISchemaEntity:
>      meta: Optional[str] = None
>  
> -    def __init__(self, name, info, doc, ifcond=None, features=None):
> +    def __init__(self, name: str, info, doc, ifcond=None, features=None):
>          assert name is None or isinstance(name, str)
>          for f in features or []:
>              assert isinstance(f, QAPISchemaFeature)

Re: [PATCH v5 13/15] qapi/introspect.py: add introspect.json dummy types

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> Add some aliases that declare intent for some of the "dictly-typed"
> objects we pass around in introspect.py.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
>
> ---
>
> This patch is optional, it can be dropped if desired. If it's taken,
> it's probably best to squash it with the prior patch.  It is purely for
> the sake of demonstrating what the _DObject was meant to convey: a
> Python Dict that represents some JSON Object. It does not add any type
> safety in and of itself, but does have some (light) annotational
> benefit. In this case, it's usually a specific data structure from the
> QAPI Schema we are referencing, but do not have "compile-time" access to
> in Python.
>
> These are loosely typed and don't bother reproducing the exact structure
> of the real types. Python 3.6 does not have support for TypedDict
> structures, so this is as good as we can do without involving a
> third-party library (e.g. Pydantic), in which we might be able to say:
>
>
> class SchemaMetaType(str, enum.Enum):
>     BUILTIN = "builtin"
>     ENUM = "enum"
>     ARRAY = "array"
>     OBJECT = "object"
>     ALTERNATE = "alternate"
>     COMMAND = "command"
>     EVENT = "event"
>
>
> class SchemaInfo(pydantic.BaseModel):
>     name: str
>     meta-type: SchemaMetaType
>     features: Optional[List[str]]
>     data: Union[SchemaInfoBuiltin, SchemaInfoEnum, SchemaInfoArray,
>                 SchemaInfoObject, SchemaInfoAlternate, SchemaInfoCommand,
>                 SchemaInfoEvent]
>
>
> However, the cost of reproducing and keeping these structure definitions
> in-sync between the user-defined portion of the schema and the code
> generator is likely not worth doing any such thing. However, it does
> illustrate an interesting dependency the generator has on the
> user-defined schema itself in terms of types.
>
> So, I settled on using some light types that suggest the form of the
> object instead of enforcing the form.
>
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  scripts/qapi/introspect.py | 51 +++++++++++++++++++++++---------------
>  1 file changed, 31 insertions(+), 20 deletions(-)
>
> diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
> index 3afcdda7446..2a39726f40a 100644
> --- a/scripts/qapi/introspect.py
> +++ b/scripts/qapi/introspect.py
> @@ -66,10 +66,15 @@
>  _value = Union[_scalar, _nonscalar]
>  TreeValue = Union[_value, 'Annotated[_value]']
>  
> -# This is an alias for an arbitrary JSON object, represented here as a Dict.
> -# It is stricter on the value type than the recursive definition above.
> -# It is used to represent SchemaInfo-related structures exclusively.
> -_DObject = Dict[str, object]
> +# These types are based on structures defined in QEMU's schema, so we lack
> +# precise types for them here. Python 3.6 does not offer TypedDict constructs,
> +# so they are broadly typed here as simple Python Dicts.
> +SchemaInfo = Dict[str, object]
> +SchemaInfoObject = Dict[str, object]
> +SchemaInfoObjectVariant = Dict[str, object]
> +SchemaInfoObjectMember = Dict[str, object]
> +SchemaInfoCommand = Dict[str, object]
> +
>  
>  _NodeT = TypeVar('_NodeT', bound=_value)
>  
> @@ -162,7 +167,7 @@ def __init__(self, prefix: str, unmask: bool):
>              ' * QAPI/QMP schema introspection', __doc__)
>          self._unmask = unmask
>          self._schema: Optional[QAPISchema] = None
> -        self._trees: List[Annotated[_DObject]] = []
> +        self._trees: List[Annotated[SchemaInfo]] = []
>          self._used_types: List[QAPISchemaType] = []
>          self._name_map: Dict[str, str] = {}
>          self._genc.add(mcgen('''
> @@ -234,9 +239,18 @@ def _gen_features(features: List[QAPISchemaFeature]
>                        ) -> List[Annotated[str]]:
>          return [Annotated(f.name, f.ifcond) for f in features]
>  
> -    def _gen_tree(self, name: str, mtype: str, obj: _DObject,
> +    def _gen_tree(self, name: str, mtype: str, obj: Dict[str, object],
>                    ifcond: List[str],
>                    features: Optional[List[QAPISchemaFeature]]) -> None:
> +        """
> +        Build and append a SchemaInfo object to self._trees.
> +
> +        :param name: The entity's name.
> +        :param mtype: The entity's meta-type.
> +        :param obj: Additional entity fields, as appropriate for the meta-type.
> +        :param ifcond: List of conditionals that apply to this entire entity.
> +        :param features: Optional features field for SchemaInfo.
> +        """
>          comment: Optional[str] = None
>          if mtype not in ('command', 'event', 'builtin', 'array'):
>              if not self._unmask:
> @@ -251,8 +265,8 @@ def _gen_tree(self, name: str, mtype: str, obj: _DObject,
>          self._trees.append(Annotated(obj, ifcond, comment))
>  
>      def _gen_member(self, member: QAPISchemaObjectTypeMember
> -                    ) -> Annotated[_DObject]:
> -        obj: _DObject = {
> +                    ) -> Annotated[SchemaInfoObjectMember]:
> +        obj: SchemaInfoObjectMember = {
>              'name': member.name,
>              'type': self._use_type(member.type)
>          }
> @@ -262,13 +276,9 @@ def _gen_member(self, member: QAPISchemaObjectTypeMember
>              obj['features'] = self._gen_features(member.features)
>          return Annotated(obj, member.ifcond)
>  
> -    def _gen_variants(self, tag_name: str,
> -                      variants: List[QAPISchemaVariant]) -> _DObject:
> -        return {'tag': tag_name,
> -                'variants': [self._gen_variant(v) for v in variants]}
> -
> -    def _gen_variant(self, variant: QAPISchemaVariant) -> Annotated[_DObject]:
> -        obj: _DObject = {
> +    def _gen_variant(self, variant: QAPISchemaVariant
> +                     ) -> Annotated[SchemaInfoObjectVariant]:
> +        obj: SchemaInfoObjectVariant = {
>              'case': variant.name,
>              'type': self._use_type(variant.type)
>          }
> @@ -300,11 +310,12 @@ def visit_object_type_flat(self, name: str, info: Optional[QAPISourceInfo],
>                                 features: List[QAPISchemaFeature],
>                                 members: List[QAPISchemaObjectTypeMember],
>                                 variants: Optional[QAPISchemaVariants]) -> None:
> -        obj: _DObject = {'members': [self._gen_member(m) for m in members]}
> +        obj: SchemaInfoObject = {
> +            'members': [self._gen_member(m) for m in members]
> +        }
>          if variants:
> -            obj.update(self._gen_variants(variants.tag_member.name,
> -                                          variants.variants))
> -
> +            obj['tag'] = variants.tag_member.name
> +            obj['variants'] = [self._gen_variant(v) for v in variants.variants]
>          self._gen_tree(name, 'object', obj, ifcond, features)
>  
>      def visit_alternate_type(self, name: str, info: Optional[QAPISourceInfo],
> @@ -329,7 +340,7 @@ def visit_command(self, name: str, info: Optional[QAPISourceInfo],
>  
>          arg_type = arg_type or self._schema.the_empty_object_type
>          ret_type = ret_type or self._schema.the_empty_object_type
> -        obj: _DObject = {
> +        obj: SchemaInfoCommand = {
>              'arg-type': self._use_type(arg_type),
>              'ret-type': self._use_type(ret_type)
>          }

I see what you mean by "some (light) annotational benefit".

I figure some of is simply due to going from _DObject, a name that tells
me nothing, to SchemaInfo, which makes me go aha!

The remainder is having the subtypes of SchemaInfo, too. albeit without
actual type checking.  Worthwhile?

I agree it should be squashed if we want it, or parts of it.

Re: [PATCH v5 14/15] qapi/introspect.py: Add docstring to _tree_to_qlit

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  scripts/qapi/introspect.py | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
> index 2a39726f40a..2b338abe2cf 100644
> --- a/scripts/qapi/introspect.py
> +++ b/scripts/qapi/introspect.py
> @@ -97,6 +97,14 @@ def __init__(self, value: _NodeT, ifcond: Iterable[str],
>  def _tree_to_qlit(obj: TreeValue,
>                    level: int = 0,
>                    dict_value: bool = False) -> str:
> +    """
> +    Convert the type tree into a QLIT C string, recursively.
> +
> +    :param obj: The value to convert.
> +    :param level: The indentation level for this particular value.
> +    :param dict_value: True when the value being processed belongs to a
> +                       dict key; which suppresses the output indent.
> +    """
>  
>      def indent(level: int) -> str:
>          return level * 4 * ' '

Might want to mention @obj may not be Annotated when dict_value=True.
Not a demand.

Re: [PATCH v5 13/15] qapi/introspect.py: add introspect.json dummy types

[John Snow]

On 2/8/21 10:43 AM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
> 
>> Add some aliases that declare intent for some of the "dictly-typed"
>> objects we pass around in introspect.py.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>>
>> ---
>>
>> This patch is optional, it can be dropped if desired. If it's taken,
>> it's probably best to squash it with the prior patch.  It is purely for
>> the sake of demonstrating what the _DObject was meant to convey: a
>> Python Dict that represents some JSON Object. It does not add any type
>> safety in and of itself, but does have some (light) annotational
>> benefit. In this case, it's usually a specific data structure from the
>> QAPI Schema we are referencing, but do not have "compile-time" access to
>> in Python.
>>
>> These are loosely typed and don't bother reproducing the exact structure
>> of the real types. Python 3.6 does not have support for TypedDict
>> structures, so this is as good as we can do without involving a
>> third-party library (e.g. Pydantic), in which we might be able to say:
>>
>>
>> class SchemaMetaType(str, enum.Enum):
>>      BUILTIN = "builtin"
>>      ENUM = "enum"
>>      ARRAY = "array"
>>      OBJECT = "object"
>>      ALTERNATE = "alternate"
>>      COMMAND = "command"
>>      EVENT = "event"
>>
>>
>> class SchemaInfo(pydantic.BaseModel):
>>      name: str
>>      meta-type: SchemaMetaType
>>      features: Optional[List[str]]
>>      data: Union[SchemaInfoBuiltin, SchemaInfoEnum, SchemaInfoArray,
>>                  SchemaInfoObject, SchemaInfoAlternate, SchemaInfoCommand,
>>                  SchemaInfoEvent]
>>
>>
>> However, the cost of reproducing and keeping these structure definitions
>> in-sync between the user-defined portion of the schema and the code
>> generator is likely not worth doing any such thing. However, it does
>> illustrate an interesting dependency the generator has on the
>> user-defined schema itself in terms of types.
>>
>> So, I settled on using some light types that suggest the form of the
>> object instead of enforcing the form.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>>   scripts/qapi/introspect.py | 51 +++++++++++++++++++++++---------------
>>   1 file changed, 31 insertions(+), 20 deletions(-)
>>
>> diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
>> index 3afcdda7446..2a39726f40a 100644
>> --- a/scripts/qapi/introspect.py
>> +++ b/scripts/qapi/introspect.py
>> @@ -66,10 +66,15 @@
>>   _value = Union[_scalar, _nonscalar]
>>   TreeValue = Union[_value, 'Annotated[_value]']
>>   
>> -# This is an alias for an arbitrary JSON object, represented here as a Dict.
>> -# It is stricter on the value type than the recursive definition above.
>> -# It is used to represent SchemaInfo-related structures exclusively.
>> -_DObject = Dict[str, object]
>> +# These types are based on structures defined in QEMU's schema, so we lack
>> +# precise types for them here. Python 3.6 does not offer TypedDict constructs,
>> +# so they are broadly typed here as simple Python Dicts.
>> +SchemaInfo = Dict[str, object]
>> +SchemaInfoObject = Dict[str, object]
>> +SchemaInfoObjectVariant = Dict[str, object]
>> +SchemaInfoObjectMember = Dict[str, object]
>> +SchemaInfoCommand = Dict[str, object]
>> +
>>   
>>   _NodeT = TypeVar('_NodeT', bound=_value)
>>   
>> @@ -162,7 +167,7 @@ def __init__(self, prefix: str, unmask: bool):
>>               ' * QAPI/QMP schema introspection', __doc__)
>>           self._unmask = unmask
>>           self._schema: Optional[QAPISchema] = None
>> -        self._trees: List[Annotated[_DObject]] = []
>> +        self._trees: List[Annotated[SchemaInfo]] = []
>>           self._used_types: List[QAPISchemaType] = []
>>           self._name_map: Dict[str, str] = {}
>>           self._genc.add(mcgen('''
>> @@ -234,9 +239,18 @@ def _gen_features(features: List[QAPISchemaFeature]
>>                         ) -> List[Annotated[str]]:
>>           return [Annotated(f.name, f.ifcond) for f in features]
>>   
>> -    def _gen_tree(self, name: str, mtype: str, obj: _DObject,
>> +    def _gen_tree(self, name: str, mtype: str, obj: Dict[str, object],
>>                     ifcond: List[str],
>>                     features: Optional[List[QAPISchemaFeature]]) -> None:
>> +        """
>> +        Build and append a SchemaInfo object to self._trees.
>> +
>> +        :param name: The entity's name.
>> +        :param mtype: The entity's meta-type.
>> +        :param obj: Additional entity fields, as appropriate for the meta-type.
>> +        :param ifcond: List of conditionals that apply to this entire entity.
>> +        :param features: Optional features field for SchemaInfo.
>> +        """
>>           comment: Optional[str] = None
>>           if mtype not in ('command', 'event', 'builtin', 'array'):
>>               if not self._unmask:
>> @@ -251,8 +265,8 @@ def _gen_tree(self, name: str, mtype: str, obj: _DObject,
>>           self._trees.append(Annotated(obj, ifcond, comment))
>>   
>>       def _gen_member(self, member: QAPISchemaObjectTypeMember
>> -                    ) -> Annotated[_DObject]:
>> -        obj: _DObject = {
>> +                    ) -> Annotated[SchemaInfoObjectMember]:
>> +        obj: SchemaInfoObjectMember = {
>>               'name': member.name,
>>               'type': self._use_type(member.type)
>>           }
>> @@ -262,13 +276,9 @@ def _gen_member(self, member: QAPISchemaObjectTypeMember
>>               obj['features'] = self._gen_features(member.features)
>>           return Annotated(obj, member.ifcond)
>>   
>> -    def _gen_variants(self, tag_name: str,
>> -                      variants: List[QAPISchemaVariant]) -> _DObject:
>> -        return {'tag': tag_name,
>> -                'variants': [self._gen_variant(v) for v in variants]}
>> -
>> -    def _gen_variant(self, variant: QAPISchemaVariant) -> Annotated[_DObject]:
>> -        obj: _DObject = {
>> +    def _gen_variant(self, variant: QAPISchemaVariant
>> +                     ) -> Annotated[SchemaInfoObjectVariant]:
>> +        obj: SchemaInfoObjectVariant = {
>>               'case': variant.name,
>>               'type': self._use_type(variant.type)
>>           }
>> @@ -300,11 +310,12 @@ def visit_object_type_flat(self, name: str, info: Optional[QAPISourceInfo],
>>                                  features: List[QAPISchemaFeature],
>>                                  members: List[QAPISchemaObjectTypeMember],
>>                                  variants: Optional[QAPISchemaVariants]) -> None:
>> -        obj: _DObject = {'members': [self._gen_member(m) for m in members]}
>> +        obj: SchemaInfoObject = {
>> +            'members': [self._gen_member(m) for m in members]
>> +        }
>>           if variants:
>> -            obj.update(self._gen_variants(variants.tag_member.name,
>> -                                          variants.variants))
>> -
>> +            obj['tag'] = variants.tag_member.name
>> +            obj['variants'] = [self._gen_variant(v) for v in variants.variants]
>>           self._gen_tree(name, 'object', obj, ifcond, features)
>>   
>>       def visit_alternate_type(self, name: str, info: Optional[QAPISourceInfo],
>> @@ -329,7 +340,7 @@ def visit_command(self, name: str, info: Optional[QAPISourceInfo],
>>   
>>           arg_type = arg_type or self._schema.the_empty_object_type
>>           ret_type = ret_type or self._schema.the_empty_object_type
>> -        obj: _DObject = {
>> +        obj: SchemaInfoCommand = {
>>               'arg-type': self._use_type(arg_type),
>>               'ret-type': self._use_type(ret_type)
>>           }
> 
> I see what you mean by "some (light) annotational benefit".
> 
> I figure some of is simply due to going from _DObject, a name that tells
> me nothing, to SchemaInfo, which makes me go aha!
> 

Naming is hard! Using the *exact* names seems helpful.

> The remainder is having the subtypes of SchemaInfo, too. albeit without
> actual type checking.  Worthwhile?
> 

For referential purposes it seems useful, at least in so far as 
declaring intent. When I was trying to type this series, I was not aware 
this module was building structures defined in introspect.json until 
quite late into the series.

I kept wondering: "What are the valid keys here? Who consumes this?" -- 
It wasn't obvious.

I'm leaning towards just keeping them all for the documentation benefit, 
though I admit it *could* mislead someone into believing it's type 
checked when it isn't, really. I tried to call that out with the comment 
at the top, which I hope suffices to diffuse that potential 
faith-based-landmine.

> I agree it should be squashed if we want it, or parts of it.
> 

Squash all of it, IMO.

--js

Re: [PATCH v5 14/15] qapi/introspect.py: Add docstring to _tree_to_qlit

[John Snow]

On 2/8/21 10:45 AM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
> 
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>>   scripts/qapi/introspect.py | 8 ++++++++
>>   1 file changed, 8 insertions(+)
>>
>> diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
>> index 2a39726f40a..2b338abe2cf 100644
>> --- a/scripts/qapi/introspect.py
>> +++ b/scripts/qapi/introspect.py
>> @@ -97,6 +97,14 @@ def __init__(self, value: _NodeT, ifcond: Iterable[str],
>>   def _tree_to_qlit(obj: TreeValue,
>>                     level: int = 0,
>>                     dict_value: bool = False) -> str:
>> +    """
>> +    Convert the type tree into a QLIT C string, recursively.
>> +
>> +    :param obj: The value to convert.
>> +    :param level: The indentation level for this particular value.
>> +    :param dict_value: True when the value being processed belongs to a
>> +                       dict key; which suppresses the output indent.
>> +    """
>>   
>>       def indent(level: int) -> str:
>>           return level * 4 * ' '
> 
> Might want to mention @obj may not be Annotated when dict_value=True.
> Not a demand.
> 

No, that's a good point.

--js

Re: [PATCH v5 12/15] qapi/introspect.py: add type hint annotations

[John Snow]

On 2/8/21 10:34 AM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
> 
>> Signed-off-by: John Snow <jsnow@redhat.com>
>>
>> ---
>>
>> See the next patch for an optional amendment that helps to clarify what
>> _DObject is meant to be.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>>   scripts/qapi/introspect.py | 117 ++++++++++++++++++++++++++-----------
>>   scripts/qapi/mypy.ini      |   5 --
>>   scripts/qapi/schema.py     |   2 +-
>>   3 files changed, 84 insertions(+), 40 deletions(-)
>>
>> diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
>> index cf0e4e05c5c..3afcdda7446 100644
>> --- a/scripts/qapi/introspect.py
>> +++ b/scripts/qapi/introspect.py
>> @@ -30,10 +30,19 @@
>>   )
>>   from .gen import QAPISchemaMonolithicCVisitor
>>   from .schema import (
>> +    QAPISchema,
>>       QAPISchemaArrayType,
>>       QAPISchemaBuiltinType,
>> +    QAPISchemaEntity,
>> +    QAPISchemaEnumMember,
>> +    QAPISchemaFeature,
>> +    QAPISchemaObjectType,
>> +    QAPISchemaObjectTypeMember,
>>       QAPISchemaType,
>> +    QAPISchemaVariant,
>> +    QAPISchemaVariants,
>>   )
>> +from .source import QAPISourceInfo
>>   
>>   
>>   # This module constructs a tree data structure that is used to
>> @@ -57,6 +66,10 @@
>     _stub = Any
>     _scalar = Union[str, bool, None]
>     _nonscalar = Union[Dict[str, _stub], List[_stub]]
>>   _value = Union[_scalar, _nonscalar]
>>   TreeValue = Union[_value, 'Annotated[_value]']
>>   
>> +# This is an alias for an arbitrary JSON object, represented here as a Dict.
>> +# It is stricter on the value type than the recursive definition above.
>> +# It is used to represent SchemaInfo-related structures exclusively.
>> +_DObject = Dict[str, object]
> 
> Please work in an abridged version of your helpful reply to my ignorant
> questions in review of v4.
> 

Maybe not needed after we squash the named aliases patch in?.

> My comments below are based on the following understanding:
> 
> _value has a Dict[str, Any] branch.
> 
> _DObject is Dict[str, object].
> 
> Both types are for the tree's dict nodes.  Both under-constrain the
> dict's values in the sense that anything can go into the dict.  The
> difference is static type checking on use of dict values: none with the
> former, overly strict with the latter (to actually do something
> interesting with a value, you usually have to narrow its static type to
> a more specific one than object).
> 
> So, having _DObject in addition to the _value branch buys us a little
> more static type checking.  I don't understand what type checking
> exactly, and therefore can't judge whether it's worth the price in
> complexity.  If you can enlighten me, I'm all ears.
> 

It enables type checking on any value that a _DObject (SchemaInfo and 
its union arm types) may have.

foo: SchemaInfo = {}  # Dict[str, object]

says that the values here must be an object. Everything is an object, so 
this doesn't constrain much in the write direction, but it constrains a 
lot in the read direction.

the type of this expression:

foo.get('key')

is object, not "Any". Which means if we do this:

x = foo.get('key') + 5

that is a type error for mypy. If we pass this value on to any function 
that is looking for or expects a specific type, it will also be an 
error. If we treat this value in any way beyond the capabilities of 
Python's most basic, abstract type, it will be an error.

When you use 'Any', it actually *disables* type checking at that 
boundary instead. If you pass an "Any" on to an interface that expects 
"int", mypy will just assume you're right about that.

When I started this series I wasn't as clear on the distinction myself, 
but during review and re-iteration I've shifted to using 'object' 
absolutely whenever I can if I need to describe an abstract value.

Summary:

- I prefer "object" to "Any", when possible
- We have to use "Any" for the recursive stubs
- I use _DObject (and later SchemaInfo et al) to represent specific 
types, not abstract recursive types, so they get the stricter 'object'.

>>   
>>   _NodeT = TypeVar('_NodeT', bound=_value)
>>   
>> @@ -76,9 +89,11 @@ def __init__(self, value: _NodeT, ifcond: Iterable[str],
>>           self.ifcond: Tuple[str, ...] = tuple(ifcond)
>>   
>>   
>> -def _tree_to_qlit(obj, level=0, dict_value=False):
>> +def _tree_to_qlit(obj: TreeValue,
>> +                  level: int = 0,
>> +                  dict_value: bool = False) -> str:
>>   
>> -    def indent(level):
>> +    def indent(level: int) -> str:
>>           return level * 4 * ' '
>>   
>>       if isinstance(obj, Annotated):
>> @@ -135,21 +150,21 @@ def indent(level):
>>       return ret
>>   
>>   
>> -def to_c_string(string):
>> +def to_c_string(string: str) -> str:
>>       return '"' + string.replace('\\', r'\\').replace('"', r'\"') + '"'
>>   
>>   
>>   class QAPISchemaGenIntrospectVisitor(QAPISchemaMonolithicCVisitor):
>>   
>> -    def __init__(self, prefix, unmask):
>> +    def __init__(self, prefix: str, unmask: bool):
>>           super().__init__(
>>               prefix, 'qapi-introspect',
>>               ' * QAPI/QMP schema introspection', __doc__)
>>           self._unmask = unmask
>> -        self._schema = None
>> -        self._trees = []
>> -        self._used_types = []
>> -        self._name_map = {}
>> +        self._schema: Optional[QAPISchema] = None
>> +        self._trees: List[Annotated[_DObject]] = []
>> +        self._used_types: List[QAPISchemaType] = []
>> +        self._name_map: Dict[str, str] = {}
>>           self._genc.add(mcgen('''
>>   #include "qemu/osdep.h"
>>   #include "%(prefix)sqapi-introspect.h"
>> @@ -157,10 +172,10 @@ def __init__(self, prefix, unmask):
>>   ''',
>>                                prefix=prefix))
>>   
>> -    def visit_begin(self, schema):
>> +    def visit_begin(self, schema: QAPISchema) -> None:
>>           self._schema = schema
>>   
>> -    def visit_end(self):
>> +    def visit_end(self) -> None:
>>           # visit the types that are actually used
>>           for typ in self._used_types:
>>               typ.visit(self)
>> @@ -182,18 +197,18 @@ def visit_end(self):
>>           self._used_types = []
>>           self._name_map = {}
>>   
>> -    def visit_needed(self, entity):
>> +    def visit_needed(self, entity: QAPISchemaEntity) -> bool:
>>           # Ignore types on first pass; visit_end() will pick up used types
>>           return not isinstance(entity, QAPISchemaType)
>>   
>> -    def _name(self, name):
>> +    def _name(self, name: str) -> str:
>>           if self._unmask:
>>               return name
>>           if name not in self._name_map:
>>               self._name_map[name] = '%d' % len(self._name_map)
>>           return self._name_map[name]
>>   
>> -    def _use_type(self, typ):
>> +    def _use_type(self, typ: QAPISchemaType) -> str:
>>           assert self._schema is not None
>>   
>>           # Map the various integer types to plain int
>> @@ -215,10 +230,13 @@ def _use_type(self, typ):
>>           return self._name(typ.name)
>>   
>>       @staticmethod
>> -    def _gen_features(features):
>> +    def _gen_features(features: List[QAPISchemaFeature]
>> +                      ) -> List[Annotated[str]]:
>>           return [Annotated(f.name, f.ifcond) for f in features]
>>   
>> -    def _gen_tree(self, name, mtype, obj, ifcond, features):
>> +    def _gen_tree(self, name: str, mtype: str, obj: _DObject,
>> +                  ifcond: List[str],
>> +                  features: Optional[List[QAPISchemaFeature]]) -> None:
> 
> Recycling my review of v2...
> 
> * @name corresponds to SchemaInfo member name, which is indeed str.
>    Okay.
> 
> * @mtype corresponds to member meta-type, which is enum SchemaMetaType.
>    It's not a Python Enum, because those were off limits when this code
>    was written.  We type what we have, and we have str.  Okay.
> 

I can *make* it an enum if you'd like. I'll throw it on the end of the 
series as an optional patch that can be kept or dropped at your discretion.

> * @obj will be turned by _gen_tree() into the tree for a SchemaInfo.
>    _DObject fits the bill.
> 
> * ifcond: List[str] should work, but gen_if(), gen_endif() use
>    Sequence[str].  When I pointed this out for v2, you replied "should
>    probable be using Sequence".
> 
>    More instances of ifcond: List[str] elsewhere; I'm not flagging them.
> 

Oh, yes.

List winds up being correct, but is unnecessarily restrictive. It 
doesn't wind up mattering, but we can use Sequence[str].

I'll add a patch to fix the types-so-far at the beginning of the series, 
and then squash the other annotation changes into this patch.

> * features: Optional[List[QAPISchemaFeature]] is correct.  "No features"
>    has two representations: None and [].  I guess we could eliminate
>    None, trading a tiny bit of efficiency for simpler typing.  Not a
>    demand.
> 

This is easy enough to make happen. I didn't notice.

I'll include it as an extra patch at the end of the series.

>>           comment: Optional[str] = None
> 
> "No annotations" is represented as None here, not {}.  I guess we could
> use {} for simpler typing.  When I pointed this out for v2, you replied
> it'll go away later.  Good enough for me.
> 

Something might have gotten garbled between then and now. I consider 
"Annotations" to be both the if conditionals and comments; this field is 
just the comment. Optional[str] is the right type for it here:

self._trees.append(Annotated(obj, ifcond, comment))

>>           if mtype not in ('command', 'event', 'builtin', 'array'):
>>               if not self._unmask:
>> @@ -232,47 +250,67 @@ def _gen_tree(self, name, mtype, obj, ifcond, features):
>>               obj['features'] = self._gen_features(features)
>>           self._trees.append(Annotated(obj, ifcond, comment))
>>   
>> -    def _gen_member(self, member):
>> -        obj = {'name': member.name, 'type': self._use_type(member.type)}
>> +    def _gen_member(self, member: QAPISchemaObjectTypeMember
>> +                    ) -> Annotated[_DObject]:
>> +        obj: _DObject = {
>> +            'name': member.name,
>> +            'type': self._use_type(member.type)
>> +        }
>>           if member.optional:
>>               obj['default'] = None
>>           if member.features:
>>               obj['features'] = self._gen_features(member.features)
>>           return Annotated(obj, member.ifcond)
>>   
>> -    def _gen_variants(self, tag_name, variants):
>> +    def _gen_variants(self, tag_name: str,
>> +                      variants: List[QAPISchemaVariant]) -> _DObject:
>>           return {'tag': tag_name,
>>                   'variants': [self._gen_variant(v) for v in variants]}
>>   
>> -    def _gen_variant(self, variant):
>> -        obj = {'case': variant.name, 'type': self._use_type(variant.type)}
>> +    def _gen_variant(self, variant: QAPISchemaVariant) -> Annotated[_DObject]:
>> +        obj: _DObject = {
>> +            'case': variant.name,
>> +            'type': self._use_type(variant.type)
>> +        }
>>           return Annotated(obj, variant.ifcond)
>>   
>> -    def visit_builtin_type(self, name, info, json_type):
>> +    def visit_builtin_type(self, name: str, info: Optional[QAPISourceInfo],
>> +                           json_type: str) -> None:
> 
> A built-in type's info is always None.  Perhaps we should drop the
> parameter.
> 

We could.

For the moment, info cannot be "None" here because mypy thinks it 
*might* be something.

...but we could just remove it for clarity. (As long as we don't later 
re-add that built-in source info object style idea and then we'd want to 
put it back, but ...)

Later?

>>           self._gen_tree(name, 'builtin', {'json-type': json_type}, [], None)
>>   
>> -    def visit_enum_type(self, name, info, ifcond, features, members, prefix):
>> +    def visit_enum_type(self, name: str, info: Optional[QAPISourceInfo],
>> +                        ifcond: List[str], features: List[QAPISchemaFeature],
>> +                        members: List[QAPISchemaEnumMember],
>> +                        prefix: Optional[str]) -> None:
>>           self._gen_tree(
>>               name, 'enum',
>>               {'values': [Annotated(m.name, m.ifcond) for m in members]},
>>               ifcond, features
>>           )
>>   
>> -    def visit_array_type(self, name, info, ifcond, element_type):
>> +    def visit_array_type(self, name: str, info: Optional[QAPISourceInfo],
>> +                         ifcond: List[str],
>> +                         element_type: QAPISchemaType) -> None:
>>           element = self._use_type(element_type)
>>           self._gen_tree('[' + element + ']', 'array', {'element-type': element},
>>                          ifcond, None)
>>   
>> -    def visit_object_type_flat(self, name, info, ifcond, features,
>> -                               members, variants):
>> -        obj = {'members': [self._gen_member(m) for m in members]}
>> +    def visit_object_type_flat(self, name: str, info: Optional[QAPISourceInfo],
>> +                               ifcond: List[str],
>> +                               features: List[QAPISchemaFeature],
>> +                               members: List[QAPISchemaObjectTypeMember],
>> +                               variants: Optional[QAPISchemaVariants]) -> None:
> 
> We represent "no variants" as None, not as [].  I guess we could
> eliminate use [], trading a tiny bit of efficiency for simpler typing.
> When I pointed this out for v2, you replied we should turn
> QAPISchemaVariants into an extension of Sequence[QAPISchemaVariant] in a
> later series.  Okay.
> 

Yeah, that's a longer-term fix I think. Not for doing right now -- but 
you're right, it stands out as suspect.

I think ideally we'd have a Variants object that behaves like a 
collection where the empty collection is false-ish and we'd always 
create such a collection.

(Though, that might create problems for tag_name vs tag_member where we 
might now have circumstances where we have neither, maybe that makes 
typing harder instead of easier. I'll need to experiment ... I have some 
questions about how to achieve better typing for our collections class 
that clash with the delayed-type-evaluation mechanisms we have. Ongoing 
research.)

>> +        obj: _DObject = {'members': [self._gen_member(m) for m in members]}
>>           if variants:
>>               obj.update(self._gen_variants(variants.tag_member.name,
>>                                             variants.variants))
>>   
>>           self._gen_tree(name, 'object', obj, ifcond, features)
>>   
>> -    def visit_alternate_type(self, name, info, ifcond, features, variants):
>> +    def visit_alternate_type(self, name: str, info: Optional[QAPISourceInfo],
>> +                             ifcond: List[str],
>> +                             features: List[QAPISchemaFeature],
>> +                             variants: QAPISchemaVariants) -> None:
>>           self._gen_tree(
>>               name, 'alternate',
>>               {'members': [Annotated({'type': self._use_type(m.type)}, m.ifcond)
>> @@ -280,27 +318,38 @@ def visit_alternate_type(self, name, info, ifcond, features, variants):
>>               ifcond, features
>>           )
>>   
>> -    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: List[str],
>> +                      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:
>>           assert self._schema is not None
>>   
>>           arg_type = arg_type or self._schema.the_empty_object_type
>>           ret_type = ret_type or self._schema.the_empty_object_type
>> -        obj = {'arg-type': self._use_type(arg_type),
>> -               'ret-type': self._use_type(ret_type)}
>> +        obj: _DObject = {
>> +            'arg-type': self._use_type(arg_type),
>> +            'ret-type': self._use_type(ret_type)
>> +        }
>>           if allow_oob:
>>               obj['allow-oob'] = allow_oob
>>           self._gen_tree(name, 'command', obj, ifcond, features)
>>   
>> -    def visit_event(self, name, info, ifcond, features, arg_type, boxed):
>> +    def visit_event(self, name: str, info: Optional[QAPISourceInfo],
>> +                    ifcond: List[str], features: List[QAPISchemaFeature],
>> +                    arg_type: Optional[QAPISchemaObjectType],
>> +                    boxed: bool) -> None:
>>           assert self._schema is not None
>> +
>>           arg_type = arg_type or self._schema.the_empty_object_type
>>           self._gen_tree(name, 'event', {'arg-type': self._use_type(arg_type)},
>>                          ifcond, features)
>>   
>>   
>> -def gen_introspect(schema, output_dir, prefix, opt_unmask):
>> +def gen_introspect(schema: QAPISchema, output_dir: str, prefix: str,
>> +                   opt_unmask: bool) -> None:
>>       vis = QAPISchemaGenIntrospectVisitor(prefix, opt_unmask)
>>       schema.visit(vis)
>>       vis.write(output_dir)
>> diff --git a/scripts/qapi/mypy.ini b/scripts/qapi/mypy.ini
>> index 04bd5db5278..0a000d58b37 100644
>> --- a/scripts/qapi/mypy.ini
>> +++ b/scripts/qapi/mypy.ini
>> @@ -13,11 +13,6 @@ disallow_untyped_defs = False
>>   disallow_incomplete_defs = False
>>   check_untyped_defs = False
>>   
>> -[mypy-qapi.introspect]
>> -disallow_untyped_defs = False
>> -disallow_incomplete_defs = False
>> -check_untyped_defs = False
>> -
>>   [mypy-qapi.parser]
>>   disallow_untyped_defs = False
>>   disallow_incomplete_defs = False
>> diff --git a/scripts/qapi/schema.py b/scripts/qapi/schema.py
>> index 353e8020a27..ff16578f6de 100644
>> --- a/scripts/qapi/schema.py
>> +++ b/scripts/qapi/schema.py
>> @@ -28,7 +28,7 @@
>>   class QAPISchemaEntity:
>>       meta: Optional[str] = None
>>   
>> -    def __init__(self, name, info, doc, ifcond=None, features=None):
>> +    def __init__(self, name: str, info, doc, ifcond=None, features=None):
>>           assert name is None or isinstance(name, str)
>>           for f in features or []:
>>               assert isinstance(f, QAPISchemaFeature)

Re: [PATCH v5 09/15] qapi/introspect.py: create a typed 'Annotated' data strutcure

[John Snow]

On 2/8/21 9:36 AM, Markus Armbruster wrote:
> John Snow <jsnow@redhat.com> writes:
> 
>> Presently, we use a tuple to attach a dict containing annotations
>> (comments and compile-time conditionals) to a tree node. This is
>> undesirable because dicts are difficult to strongly type; promoting it
>> to a real class allows us to name the values and types of the
>> annotations we are expecting.
>>
>> In terms of typing, the Annotated<T> type serves as a generic container
>> where the annotated node's type is preserved, allowing for greater
>> specificity than we'd be able to provide without a generic.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
>> ---
>>   scripts/qapi/introspect.py | 77 ++++++++++++++++++++++----------------
>>   1 file changed, 44 insertions(+), 33 deletions(-)
>>
>> diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
>> index 8e019b4a26a..b9427aba449 100644
>> --- a/scripts/qapi/introspect.py
>> +++ b/scripts/qapi/introspect.py
>> @@ -13,8 +13,12 @@
>>   from typing import (
>>       Any,
>>       Dict,
>> +    Generic,
>> +    Iterable,
>>       List,
>>       Optional,
>> +    Tuple,
>> +    TypeVar,
>>       Union,
>>   )
>>   
>> @@ -51,15 +55,25 @@
>>   _scalar = Union[str, bool, None]
>>   _nonscalar = Union[Dict[str, _stub], List[_stub]]
>>   _value = Union[_scalar, _nonscalar]
>> -# TreeValue = TODO, in a forthcoming commit.
>> +TreeValue = Union[_value, 'Annotated[_value]']
>>   
>>   
>> -def _make_tree(obj, ifcond, comment=None):
>> -    extra = {
>> -        'if': ifcond,
>> -        'comment': comment
>> -    }
>> -    return (obj, extra)
>> +_NodeT = TypeVar('_NodeT', bound=_value)
>> +
>> +
>> +class Annotated(Generic[_NodeT]):
> 
> My gut feeling is "generic type is overkill for this purpose".  Let's go
> with it anyway, because
> 
> 1. It's not wrong.
> 

A famous phrase in Computer Science.

> 2. I don't have enough experience with Python type hints for reliable
> gut feelings.
> 

You are exactly correct that the power it offers us here isn't strictly 
necessary. An argument might be that removing it makes the types easier 
to read, but I think at a certain level of involvement with mypy that it 
isn't feasible to escape understanding Generics, and we are at that level.

> 3. I plan to overhaul the C generation part relatively soon (after your
> work has landed, don't worry), and I can try to make it simpler then.
> 

Yeah. The generation and typing can likely improve substantially at that 
point in time. Hopefully the type hints help guide a design that's nice 
to type and nice to read.

>> +    """
>> +    Annotated generally contains a SchemaInfo-like type (as a dict),
>> +    But it also used to wrap comments/ifconds around scalar leaf values,
>> +    for the benefit of features and enums.
>> +    """
>> +    # TODO: Remove after Python 3.7 adds @dataclass:
>> +    # pylint: disable=too-few-public-methods
>> +    def __init__(self, value: _NodeT, ifcond: Iterable[str],
>> +                 comment: Optional[str] = None):
>> +        self.value = value
>> +        self.comment: Optional[str] = comment
>> +        self.ifcond: Tuple[str, ...] = tuple(ifcond)
>>   
>>   
>>   def _tree_to_qlit(obj, level=0, dict_value=False):
>> @@ -67,24 +81,20 @@ def _tree_to_qlit(obj, level=0, dict_value=False):
>>       def indent(level):
>>           return level * 4 * ' '
>>   
>> -    if isinstance(obj, tuple):
>> -        ifobj, extra = obj
>> -        ifcond = extra.get('if')
>> -        comment = extra.get('comment')
>> -
>> +    if isinstance(obj, Annotated):
>>           # NB: _tree_to_qlit is called recursively on the values of a key:value
>>           # pair; those values can't be decorated with comments or conditionals.
>>           msg = "dict values cannot have attached comments or if-conditionals."
>>           assert not dict_value, msg
>>   
>>           ret = ''
>> -        if comment:
>> -            ret += indent(level) + '/* %s */\n' % comment
>> -        if ifcond:
>> -            ret += gen_if(ifcond)
>> -        ret += _tree_to_qlit(ifobj, level)
>> -        if ifcond:
>> -            ret += '\n' + gen_endif(ifcond)
>> +        if obj.comment:
>> +            ret += indent(level) + '/* %s */\n' % obj.comment
>> +        if obj.ifcond:
>> +            ret += gen_if(obj.ifcond)
>> +        ret += _tree_to_qlit(obj.value, level)
>> +        if obj.ifcond:
>> +            ret += '\n' + gen_endif(obj.ifcond)
>>           return ret
>>   
>>       ret = ''
>> @@ -201,7 +211,7 @@ def _use_type(self, typ):
>>   
>>       @staticmethod
>>       def _gen_features(features):
>> -        return [_make_tree(f.name, f.ifcond) for f in features]
>> +        return [Annotated(f.name, f.ifcond) for f in features]
>>   
>>       def _gen_tree(self, name, mtype, obj, ifcond, features):
>>           comment: Optional[str] = None
>> @@ -215,7 +225,7 @@ def _gen_tree(self, name, mtype, obj, ifcond, features):
>>           obj['meta-type'] = mtype
>>           if features:
>>               obj['features'] = self._gen_features(features)
>> -        self._trees.append(_make_tree(obj, ifcond, comment))
>> +        self._trees.append(Annotated(obj, ifcond, comment))
>>   
>>       def _gen_member(self, member):
>>           obj = {'name': member.name, 'type': self._use_type(member.type)}
>> @@ -223,7 +233,7 @@ def _gen_member(self, member):
>>               obj['default'] = None
>>           if member.features:
>>               obj['features'] = self._gen_features(member.features)
>> -        return _make_tree(obj, member.ifcond)
>> +        return Annotated(obj, member.ifcond)
>>   
>>       def _gen_variants(self, tag_name, variants):
>>           return {'tag': tag_name,
>> @@ -231,16 +241,17 @@ def _gen_variants(self, tag_name, variants):
>>   
>>       def _gen_variant(self, variant):
>>           obj = {'case': variant.name, 'type': self._use_type(variant.type)}
>> -        return _make_tree(obj, variant.ifcond)
>> +        return Annotated(obj, variant.ifcond)
>>   
>>       def visit_builtin_type(self, name, info, json_type):
>>           self._gen_tree(name, 'builtin', {'json-type': json_type}, [], None)
>>   
>>       def visit_enum_type(self, name, info, ifcond, features, members, prefix):
>> -        self._gen_tree(name, 'enum',
>> -                       {'values': [_make_tree(m.name, m.ifcond, None)
>> -                                   for m in members]},
>> -                       ifcond, features)
>> +        self._gen_tree(
>> +            name, 'enum',
>> +            {'values': [Annotated(m.name, m.ifcond) for m in members]},
>> +            ifcond, features
>> +        )
>>   
>>       def visit_array_type(self, name, info, ifcond, element_type):
>>           element = self._use_type(element_type)
>> @@ -257,12 +268,12 @@ def visit_object_type_flat(self, name, info, ifcond, features,
>>           self._gen_tree(name, 'object', obj, ifcond, features)
>>   
>>       def visit_alternate_type(self, name, info, ifcond, features, variants):
>> -        self._gen_tree(name, 'alternate',
>> -                       {'members': [
>> -                           _make_tree({'type': self._use_type(m.type)},
>> -                                      m.ifcond, None)
>> -                           for m in variants.variants]},
>> -                       ifcond, features)
>> +        self._gen_tree(
>> +            name, 'alternate',
>> +            {'members': [Annotated({'type': self._use_type(m.type)}, m.ifcond)
>> +                         for m in variants.variants]},
> 
> Slightly more readable, I think:
> 
>                 {'members': [Annotated({'type': self._use_type(m.type)},
>                                        m.ifcond)
>                              for m in variants.variants]},
> 

OK, but only because I am being annoying about not capitulating 
elsewhere on equally trivial matters.

>> +            ifcond, features
>> +        )
>>   
>>       def visit_command(self, name, info, ifcond, features,
>>                         arg_type, ret_type, gen, success_response, boxed,

Re: [PATCH v5 12/15] qapi/introspect.py: add type hint annotations

[Markus Armbruster]

John Snow <jsnow@redhat.com> writes:

> On 2/8/21 10:34 AM, Markus Armbruster wrote:
>> John Snow <jsnow@redhat.com> writes:
>> 
>>> Signed-off-by: John Snow <jsnow@redhat.com>
>>>
>>> ---
>>>
>>> See the next patch for an optional amendment that helps to clarify what
>>> _DObject is meant to be.
>>>
>>> Signed-off-by: John Snow <jsnow@redhat.com>
>>> ---
>>>   scripts/qapi/introspect.py | 117 ++++++++++++++++++++++++++-----------
>>>   scripts/qapi/mypy.ini      |   5 --
>>>   scripts/qapi/schema.py     |   2 +-
>>>   3 files changed, 84 insertions(+), 40 deletions(-)
>>>
>>> diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
>>> index cf0e4e05c5c..3afcdda7446 100644
>>> --- a/scripts/qapi/introspect.py
>>> +++ b/scripts/qapi/introspect.py
>>> @@ -30,10 +30,19 @@
>>>   )
>>>   from .gen import QAPISchemaMonolithicCVisitor
>>>   from .schema import (
>>> +    QAPISchema,
>>>       QAPISchemaArrayType,
>>>       QAPISchemaBuiltinType,
>>> +    QAPISchemaEntity,
>>> +    QAPISchemaEnumMember,
>>> +    QAPISchemaFeature,
>>> +    QAPISchemaObjectType,
>>> +    QAPISchemaObjectTypeMember,
>>>       QAPISchemaType,
>>> +    QAPISchemaVariant,
>>> +    QAPISchemaVariants,
>>>   )
>>> +from .source import QAPISourceInfo
>>>   
>>>   
>>>   # This module constructs a tree data structure that is used to
>>> @@ -57,6 +66,10 @@
>>     _stub = Any
>>     _scalar = Union[str, bool, None]
>>     _nonscalar = Union[Dict[str, _stub], List[_stub]]
>>>   _value = Union[_scalar, _nonscalar]
>>>   TreeValue = Union[_value, 'Annotated[_value]']
>>>   
>>> +# This is an alias for an arbitrary JSON object, represented here as a Dict.
>>> +# It is stricter on the value type than the recursive definition above.
>>> +# It is used to represent SchemaInfo-related structures exclusively.
>>> +_DObject = Dict[str, object]
>> 
>> Please work in an abridged version of your helpful reply to my ignorant
>> questions in review of v4.
>> 
>
> Maybe not needed after we squash the named aliases patch in?.
>
>> My comments below are based on the following understanding:
>> 
>> _value has a Dict[str, Any] branch.
>> 
>> _DObject is Dict[str, object].
>> 
>> Both types are for the tree's dict nodes.  Both under-constrain the
>> dict's values in the sense that anything can go into the dict.  The
>> difference is static type checking on use of dict values: none with the
>> former, overly strict with the latter (to actually do something
>> interesting with a value, you usually have to narrow its static type to
>> a more specific one than object).
>> 
>> So, having _DObject in addition to the _value branch buys us a little
>> more static type checking.  I don't understand what type checking
>> exactly, and therefore can't judge whether it's worth the price in
>> complexity.  If you can enlighten me, I'm all ears.
>> 
>
> It enables type checking on any value that a _DObject (SchemaInfo and 
> its union arm types) may have.
>
> foo: SchemaInfo = {}  # Dict[str, object]
>
> says that the values here must be an object. Everything is an object, so 
> this doesn't constrain much in the write direction, but it constrains a 
> lot in the read direction.
>
> the type of this expression:
>
> foo.get('key')
>
> is object, not "Any". Which means if we do this:
>
> x = foo.get('key') + 5
>
> that is a type error for mypy. If we pass this value on to any function 
> that is looking for or expects a specific type, it will also be an 
> error. If we treat this value in any way beyond the capabilities of 
> Python's most basic, abstract type, it will be an error.
>
> When you use 'Any', it actually *disables* type checking at that 
> boundary instead. If you pass an "Any" on to an interface that expects 
> "int", mypy will just assume you're right about that.
>
> When I started this series I wasn't as clear on the distinction myself, 
> but during review and re-iteration I've shifted to using 'object' 
> absolutely whenever I can if I need to describe an abstract value.
>
> Summary:
>
> - I prefer "object" to "Any", when possible
> - We have to use "Any" for the recursive stubs
> - I use _DObject (and later SchemaInfo et al) to represent specific 
> types, not abstract recursive types, so they get the stricter 'object'.

I understand that now.  I doubt it's worth the extra complexity here.
Being discussed in reply to PATCH 11.  Let's continue there.

>>>   
>>>   _NodeT = TypeVar('_NodeT', bound=_value)
>>>   
>>> @@ -76,9 +89,11 @@ def __init__(self, value: _NodeT, ifcond: Iterable[str],
>>>           self.ifcond: Tuple[str, ...] = tuple(ifcond)
>>>   
>>>   
>>> -def _tree_to_qlit(obj, level=0, dict_value=False):
>>> +def _tree_to_qlit(obj: TreeValue,
>>> +                  level: int = 0,
>>> +                  dict_value: bool = False) -> str:
>>>   
>>> -    def indent(level):
>>> +    def indent(level: int) -> str:
>>>           return level * 4 * ' '
>>>   
>>>       if isinstance(obj, Annotated):
>>> @@ -135,21 +150,21 @@ def indent(level):
>>>       return ret
>>>   
>>>   
>>> -def to_c_string(string):
>>> +def to_c_string(string: str) -> str:
>>>       return '"' + string.replace('\\', r'\\').replace('"', r'\"') + '"'
>>>   
>>>   
>>>   class QAPISchemaGenIntrospectVisitor(QAPISchemaMonolithicCVisitor):
>>>   
>>> -    def __init__(self, prefix, unmask):
>>> +    def __init__(self, prefix: str, unmask: bool):
>>>           super().__init__(
>>>               prefix, 'qapi-introspect',
>>>               ' * QAPI/QMP schema introspection', __doc__)
>>>           self._unmask = unmask
>>> -        self._schema = None
>>> -        self._trees = []
>>> -        self._used_types = []
>>> -        self._name_map = {}
>>> +        self._schema: Optional[QAPISchema] = None
>>> +        self._trees: List[Annotated[_DObject]] = []
>>> +        self._used_types: List[QAPISchemaType] = []
>>> +        self._name_map: Dict[str, str] = {}
>>>           self._genc.add(mcgen('''
>>>   #include "qemu/osdep.h"
>>>   #include "%(prefix)sqapi-introspect.h"
>>> @@ -157,10 +172,10 @@ def __init__(self, prefix, unmask):
>>>   ''',
>>>                                prefix=prefix))
>>>   
>>> -    def visit_begin(self, schema):
>>> +    def visit_begin(self, schema: QAPISchema) -> None:
>>>           self._schema = schema
>>>   
>>> -    def visit_end(self):
>>> +    def visit_end(self) -> None:
>>>           # visit the types that are actually used
>>>           for typ in self._used_types:
>>>               typ.visit(self)
>>> @@ -182,18 +197,18 @@ def visit_end(self):
>>>           self._used_types = []
>>>           self._name_map = {}
>>>   
>>> -    def visit_needed(self, entity):
>>> +    def visit_needed(self, entity: QAPISchemaEntity) -> bool:
>>>           # Ignore types on first pass; visit_end() will pick up used types
>>>           return not isinstance(entity, QAPISchemaType)
>>>   
>>> -    def _name(self, name):
>>> +    def _name(self, name: str) -> str:
>>>           if self._unmask:
>>>               return name
>>>           if name not in self._name_map:
>>>               self._name_map[name] = '%d' % len(self._name_map)
>>>           return self._name_map[name]
>>>   
>>> -    def _use_type(self, typ):
>>> +    def _use_type(self, typ: QAPISchemaType) -> str:
>>>           assert self._schema is not None
>>>   
>>>           # Map the various integer types to plain int
>>> @@ -215,10 +230,13 @@ def _use_type(self, typ):
>>>           return self._name(typ.name)
>>>   
>>>       @staticmethod
>>> -    def _gen_features(features):
>>> +    def _gen_features(features: List[QAPISchemaFeature]
>>> +                      ) -> List[Annotated[str]]:
>>>           return [Annotated(f.name, f.ifcond) for f in features]
>>>   
>>> -    def _gen_tree(self, name, mtype, obj, ifcond, features):
>>> +    def _gen_tree(self, name: str, mtype: str, obj: _DObject,
>>> +                  ifcond: List[str],
>>> +                  features: Optional[List[QAPISchemaFeature]]) -> None:
>> 
>> Recycling my review of v2...
>> 
>> * @name corresponds to SchemaInfo member name, which is indeed str.
>>    Okay.
>> 
>> * @mtype corresponds to member meta-type, which is enum SchemaMetaType.
>>    It's not a Python Enum, because those were off limits when this code
>>    was written.  We type what we have, and we have str.  Okay.
>> 
>
> I can *make* it an enum if you'd like. I'll throw it on the end of the 
> series as an optional patch that can be kept or dropped at your discretion.

Let's stick stick to "type what we have", and also take notes on
possible improvements for later.  We need this series to converge.

>> * @obj will be turned by _gen_tree() into the tree for a SchemaInfo.
>>    _DObject fits the bill.
>> 
>> * ifcond: List[str] should work, but gen_if(), gen_endif() use
>>    Sequence[str].  When I pointed this out for v2, you replied "should
>>    probable be using Sequence".
>> 
>>    More instances of ifcond: List[str] elsewhere; I'm not flagging them.
>> 
>
> Oh, yes.
>
> List winds up being correct, but is unnecessarily restrictive. It 
> doesn't wind up mattering, but we can use Sequence[str].
>
> I'll add a patch to fix the types-so-far at the beginning of the series, 
> and then squash the other annotation changes into this patch.

Sounds good.

>> * features: Optional[List[QAPISchemaFeature]] is correct.  "No features"
>>    has two representations: None and [].  I guess we could eliminate
>>    None, trading a tiny bit of efficiency for simpler typing.  Not a
>>    demand.
>> 
>
> This is easy enough to make happen. I didn't notice.
>
> I'll include it as an extra patch at the end of the series.

Only if it's really, really simple.  Else, stick to "type what we have".

>>>           comment: Optional[str] = None
>> 
>> "No annotations" is represented as None here, not {}.  I guess we could
>> use {} for simpler typing.  When I pointed this out for v2, you replied
>> it'll go away later.  Good enough for me.
>> 
>
> Something might have gotten garbled between then and now. I consider 
> "Annotations" to be both the if conditionals and comments; this field is 
> just the comment. Optional[str] is the right type for it here:
>
> self._trees.append(Annotated(obj, ifcond, comment))

You're right.

>>>           if mtype not in ('command', 'event', 'builtin', 'array'):
>>>               if not self._unmask:
>>> @@ -232,47 +250,67 @@ def _gen_tree(self, name, mtype, obj, ifcond, features):
>>>               obj['features'] = self._gen_features(features)
>>>           self._trees.append(Annotated(obj, ifcond, comment))
>>>   
>>> -    def _gen_member(self, member):
>>> -        obj = {'name': member.name, 'type': self._use_type(member.type)}
>>> +    def _gen_member(self, member: QAPISchemaObjectTypeMember
>>> +                    ) -> Annotated[_DObject]:
>>> +        obj: _DObject = {
>>> +            'name': member.name,
>>> +            'type': self._use_type(member.type)
>>> +        }
>>>           if member.optional:
>>>               obj['default'] = None
>>>           if member.features:
>>>               obj['features'] = self._gen_features(member.features)
>>>           return Annotated(obj, member.ifcond)
>>>   
>>> -    def _gen_variants(self, tag_name, variants):
>>> +    def _gen_variants(self, tag_name: str,
>>> +                      variants: List[QAPISchemaVariant]) -> _DObject:
>>>           return {'tag': tag_name,
>>>                   'variants': [self._gen_variant(v) for v in variants]}
>>>   
>>> -    def _gen_variant(self, variant):
>>> -        obj = {'case': variant.name, 'type': self._use_type(variant.type)}
>>> +    def _gen_variant(self, variant: QAPISchemaVariant) -> Annotated[_DObject]:
>>> +        obj: _DObject = {
>>> +            'case': variant.name,
>>> +            'type': self._use_type(variant.type)
>>> +        }
>>>           return Annotated(obj, variant.ifcond)
>>>   
>>> -    def visit_builtin_type(self, name, info, json_type):
>>> +    def visit_builtin_type(self, name: str, info: Optional[QAPISourceInfo],
>>> +                           json_type: str) -> None:
>> 
>> A built-in type's info is always None.  Perhaps we should drop the
>> parameter.
>> 
>
> We could.
>
> For the moment, info cannot be "None" here because mypy thinks it 
> *might* be something.
>
> ...but we could just remove it for clarity. (As long as we don't later 
> re-add that built-in source info object style idea and then we'd want to 
> put it back, but ...)
>
> Later?

Okay.

>>>           self._gen_tree(name, 'builtin', {'json-type': json_type}, [], None)
>>>   
>>> -    def visit_enum_type(self, name, info, ifcond, features, members, prefix):
>>> +    def visit_enum_type(self, name: str, info: Optional[QAPISourceInfo],
>>> +                        ifcond: List[str], features: List[QAPISchemaFeature],
>>> +                        members: List[QAPISchemaEnumMember],
>>> +                        prefix: Optional[str]) -> None:
>>>           self._gen_tree(
>>>               name, 'enum',
>>>               {'values': [Annotated(m.name, m.ifcond) for m in members]},
>>>               ifcond, features
>>>           )
>>>   
>>> -    def visit_array_type(self, name, info, ifcond, element_type):
>>> +    def visit_array_type(self, name: str, info: Optional[QAPISourceInfo],
>>> +                         ifcond: List[str],
>>> +                         element_type: QAPISchemaType) -> None:
>>>           element = self._use_type(element_type)
>>>           self._gen_tree('[' + element + ']', 'array', {'element-type': element},
>>>                          ifcond, None)
>>>   
>>> -    def visit_object_type_flat(self, name, info, ifcond, features,
>>> -                               members, variants):
>>> -        obj = {'members': [self._gen_member(m) for m in members]}
>>> +    def visit_object_type_flat(self, name: str, info: Optional[QAPISourceInfo],
>>> +                               ifcond: List[str],
>>> +                               features: List[QAPISchemaFeature],
>>> +                               members: List[QAPISchemaObjectTypeMember],
>>> +                               variants: Optional[QAPISchemaVariants]) -> None:
>> 
>> We represent "no variants" as None, not as [].  I guess we could
>> eliminate use [], trading a tiny bit of efficiency for simpler typing.
>> When I pointed this out for v2, you replied we should turn
>> QAPISchemaVariants into an extension of Sequence[QAPISchemaVariant] in a
>> later series.  Okay.
>> 
>
> Yeah, that's a longer-term fix I think. Not for doing right now -- but 
> you're right, it stands out as suspect.
>
> I think ideally we'd have a Variants object that behaves like a 
> collection where the empty collection is false-ish and we'd always 
> create such a collection.
>
> (Though, that might create problems for tag_name vs tag_member where we 
> might now have circumstances where we have neither, maybe that makes 
> typing harder instead of easier. I'll need to experiment ... I have some 
> questions about how to achieve better typing for our collections class 
> that clash with the delayed-type-evaluation mechanisms we have. Ongoing 
> research.)

In short: later (maybe).

[...]

Re: [PATCH v5 12/15] qapi/introspect.py: add type hint annotations

[John Snow]

On 2/9/21 4:17 AM, Markus Armbruster wrote:
> Let's stick stick to "type what we have", and also take notes on
> possible improvements for later.  We need this series to converge.

Yup. Consider the optional trailing patches my attempt at saying "Later, 
maybe?" as a genuine to-do and not dismissive "I will never do this! The 
fool! The rube!"

--js