[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH v4 08/14] qapi/introspect.py: create a typed 'Annotated' data

From: John Snow
Subject: Re: [PATCH v4 08/14] qapi/introspect.py: create a typed 'Annotated' data strutcure
Date: Wed, 3 Feb 2021 18:12:20 -0500
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.6.0

On 2/3/21 9:47 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 b82efe16f6e..2b90a52f016 100644
--- a/scripts/qapi/introspect.py
+++ b/scripts/qapi/introspect.py
@@ -13,8 +13,12 @@
  from typing import (
+    Generic,
+    Iterable,
+    Tuple,
+    TypeVar,
@@ -51,15 +55,25 @@
  _scalar = Union[str, bool, None]
  _nonscalar = Union[Dict[str, _stub], List[_stub]]
  _value = Union[_scalar, _nonscalar]
-# TreeValue = Union[_value, 'Annotated[_value]']
+TreeValue = Union[_value, 'Annotated[_value]']

You need to quote 'Annotated[_value]', because it's a forward

Dependency cycle:

         |                                               |
     TreeValue = Union[_value, 'Annotated[_value]']      |
                                    |                    |
         +--------------------------+                    |
         |                                               |
     Annotated(Generic[_NodeT])                          |
                          |                              |
        +-----------------+                              |
        |                                                |
     _NodeT = TypeVar('_NodeT', bound=TreeValue          |
                                          |              |

Meh.  We'll live.

Python 3.10 (!) will fix this sort of thing. It fixes it in a funny way, though: all annotations are treated as delayed-evaluation strings. :)

See https://www.python.org/dev/peps/pep-0563/ which now becomes the *default* behavior.

We do not gain access to this behavior at all, because it is exclusive to 3.7+. Boo.

For now, (and for the foreseeable future of the QEMU project, as Python 3.7 support will not be available to us for many, many, many more years [1]) forward references in the global scope need to be quoted.

-def _make_tree(obj, ifcond, comment=None):
-    extra = {
-        'if': ifcond,
-        'comment': comment,
-    }
-    return (obj, extra)
+_NodeT = TypeVar('_NodeT', bound=TreeValue)

Can you teach me what NodeT is about?

It's a TypeVar. If you're a TAPL sort of person, canonically you use T,U,V and so on. Relevant tutorial section for mypy is here: https://mypy.readthedocs.io/en/stable/generics.html

(Yes, a weird thing about Generics in Python is that you have to declare them. No, I don't know why. Yes, it's weird.)

I have bound it to TreeValue, indicating that this type variable may only *ever* represent something that isa TreeValue. Because of this, I use "NodeT" to indicate that it's a Generic type T, but with some constraint.

(Though, it really should have been bound to _value instead ... my mistake. I shouldn't allow for nested annotations ...!)

Using it allows me to define the Annotated class below in terms of some input parameter instead of a fixed, opaque type.

+class Annotated(Generic[_NodeT]):

Annotated here is defined as Annotated[T], but T is our special _NodeT, so Annotated may only hold other TreeValues*.

(* Again, as per above, this is an oopsie.)

+    """
+    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.
+    """
+    # Remove after 3.7 adds @dataclass:

Make this

        # TODO Remove after Python 3.7 ...

to give us a fighting chance to remember.

Having done a lot of the python 2 excision work, I only ever grepped for e.g. "3.7" or "sys.version". I was targeting that.

Adding TODO seems fine enough to do, and I'm here anyway.

+    # pylint: disable=too-few-public-methods
+    def __init__(self, value: _NodeT, ifcond: Iterable[str],
+                 comment: Optional[str] = None):

Why not simply value: _value?

This is what connects back with the Generic instantiation of this annotation. `class Annotated(Generic[_NodeT])` says that this class is a higher-kinded type parameterized on ... something. We don't know yet.

In the initializer, we use the TypeVar to describe which argument determines that parameterization.

+        self.value = value

That parameter flows down and connects to this property. Thus, this field actually has a dynamic type that will track the original type used to instantiate it. If it was a Dict, this will also be a Dict.

(Of course, it's limited to what mypy knows about the constraint of that value when passed. It isn't interrogated at runtime.)

This is a way to achieve dynamism in container-style classes without losing precision in data types. It allows us to declare and keep a more specific "inner type" that survives the boxing and unboxing process.

e.g. Annotated[Dict[str, object]] and Annotated[str] are two different types, and can be differentiated by mypy.

(*cough*, and yes, if you have a function that takes Annotated[Any], you lose that precision at that point. So, we aren't taking full advantage of that power here in introspect.py, but it still seemed like the "right" way to type something like this.)

+        self.comment: Optional[str] = comment
+        self.ifcond: Tuple[str, ...] = tuple(ifcond)
def _tree_to_qlit(obj, level=0, suppress_first_indent=False):
@@ -67,24 +81,20 @@ def _tree_to_qlit(obj, level=0, 
      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 suppress_first_indent, 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, suppress_first_indent)

Why do you need to pass suppress_first_indent now?

UH, technically I guess I don't. Holdover from when that was maybe not clear.

+        if obj.ifcond:
+            ret += '\n' + gen_endif(obj.ifcond)
          return ret
ret = ''
@@ -201,7 +211,7 @@ def _use_type(self, typ):
      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, 
          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': [

I think I'd prefer another line break after 'alternate', to get
{'members': align...

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

... with ]}.

How's about

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

(It'd look cooler if I re-arranged the other short parameters first, or I could create a temporary local. In-line objects with in-line comprehensions are bound to look sorta ugly. I'm going with what I wrote above for now, though.)

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

[1] Python 3.6 EOL is this December, but OpenSuSE Leap 15.2 was released 2020-07-02 and only offers Python 3.6. Ouch! It is not clear if they will add support for Python 3.7 at any point, but otherwise we are stuck supporting 15.2 for two years after the next OpenSUSE is released, which hasn't happened yet. Ouch!!! So we don't even have a date on the tin for when we might conceivably inch up our requirements again.

I think the situation for RHEL and Debian is also sad, IIRC.

reply via email to

[Prev in Thread] Current Thread [Next in Thread]