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 <js...@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 <js...@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,