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 (
     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 = Union[_value, 'Annotated[_value]']
+TreeValue = Union[_value, 'Annotated[_value]']
 
 
-def _make_tree(obj, ifcond, comment=None):
-    extra = {
-        'if': ifcond,
-        'comment': comment,
-    }
-    return (obj, extra)
+_NodeT = TypeVar('_NodeT', bound=TreeValue)
+
+
+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.
+    """
+    # Remove after 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, suppress_first_indent=False):
@@ -67,24 +81,20 @@ def _tree_to_qlit(obj, level=0, suppress_first_indent=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 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)
+        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

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 (
>      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 = Union[_value, 'Annotated[_value]']
> +TreeValue = Union[_value, 'Annotated[_value]']

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

Dependency cycle:

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

Meh.  We'll live.

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

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

Make this

       # TODO Remove after Python 3.7 ...

to give us a fighting chance to remember.

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

Why not simply value: _value?

> +        self.value = value
> +        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, suppress_first_indent=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 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?

> +        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': [

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 ]}.

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

On Wed, Feb 03, 2021 at 03:47:36PM +0100, 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>
[...]
> > +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.
> > +    """
> > +    # Remove after 3.7 adds @dataclass:
> 
> Make this
> 
>        # TODO Remove after Python 3.7 ...
> 
> to give us a fighting chance to remember.
> 
> > +    # pylint: disable=too-few-public-methods
> > +    def __init__(self, value: _NodeT, ifcond: Iterable[str],
> > +                 comment: Optional[str] = None):
> 
> Why not simply value: _value?

Example:
  x = C(1)
  y: C[int]
  y = C('x')  # mistake

Declaring value as _NodeT does:
- Make the inferred type of x be Annotated[int].
- Catch the mistake above.

-- 
Eduardo

Eduardo Habkost <ehabkost@redhat.com> writes:

> On Wed, Feb 03, 2021 at 03:47:36PM +0100, 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>
> [...]
>> > +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.
>> > +    """
>> > +    # Remove after 3.7 adds @dataclass:
>> 
>> Make this
>> 
>>        # TODO Remove after Python 3.7 ...
>> 
>> to give us a fighting chance to remember.
>> 
>> > +    # pylint: disable=too-few-public-methods
>> > +    def __init__(self, value: _NodeT, ifcond: Iterable[str],
>> > +                 comment: Optional[str] = None):
>> 
>> Why not simply value: _value?
>
> Example:
>   x = C(1)
>   y: C[int]
>   y = C('x')  # mistake
>
> Declaring value as _NodeT does:
> - Make the inferred type of x be Annotated[int].
> - Catch the mistake above.

I smell overengineering.  I may well be wrong.

Without doubt, there are uses for using the type system for keeping
SomeGenericType[SomeType] and SomeGenericType[AnotherType] apart.

But what do we gain by keeping the Annotated[T] for the possible T
apart?

_tree_to_qlit() doesn't care: it peels off the wrapper holding ifcond
and comment, and recurses for the JSON so wrapped.  Regardless of what
was wrapped, i.e. what kind of T we got.

Heck, it works just fine even if you wrap your JSON multiple times.  It
doesn't give a hoot whether that makes sense.  Making sense is the
caller's business.

So what does care?

Or am I simply confused?


PS: As far as I can tell, _tree_to_qlit() doesn't give a hoot whether a
dictionary's values are wrapped, either.

On 2/4/21 10:37 AM, Markus Armbruster wrote:
> Eduardo Habkost <ehabkost@redhat.com> writes:
> 
>> On Wed, Feb 03, 2021 at 03:47:36PM +0100, 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>
>> [...]
>>>> +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.
>>>> +    """
>>>> +    # Remove after 3.7 adds @dataclass:
>>>
>>> Make this
>>>
>>>         # TODO Remove after Python 3.7 ...
>>>
>>> to give us a fighting chance to remember.
>>>
>>>> +    # pylint: disable=too-few-public-methods
>>>> +    def __init__(self, value: _NodeT, ifcond: Iterable[str],
>>>> +                 comment: Optional[str] = None):
>>>
>>> Why not simply value: _value?
>>
>> Example:
>>    x = C(1)
>>    y: C[int]
>>    y = C('x')  # mistake
>>
>> Declaring value as _NodeT does:
>> - Make the inferred type of x be Annotated[int].
>> - Catch the mistake above.
> 
> I smell overengineering.  I may well be wrong.
> 
> Without doubt, there are uses for using the type system for keeping
> SomeGenericType[SomeType] and SomeGenericType[AnotherType] apart.
> 
> But what do we gain by keeping the Annotated[T] for the possible T
> apart?
> 
> _tree_to_qlit() doesn't care: it peels off the wrapper holding ifcond
> and comment, and recurses for the JSON so wrapped.  Regardless of what
> was wrapped, i.e. what kind of T we got.
> 
> Heck, it works just fine even if you wrap your JSON multiple times.  It
> doesn't give a hoot whether that makes sense.  Making sense is the
> caller's business.
> 
> So what does care?
> 
> Or am I simply confused?
> 
> 
> PS: As far as I can tell, _tree_to_qlit() doesn't give a hoot whether a
> dictionary's values are wrapped, either.
> 

You're right that it offers no necessary power to the automated 
checking, I cede as much in my other replies to you.

(1) I hope the explanation was helpful, because the technique will 
return for structures like QAPISchemaMember where it does become more 
obviously useful to disambiguate the wrapped types.

(2) It still has a notational benefit to the human for documentation and 
IDE purposes, e.g. at the end of this series:

```
self._trees: List[Annotated[SchemaInfo]] = []
```

That's nice! It tells us exactly what's in there.

Does it work if I dumb it down to just:

```
self.trees: List[AnnotatedThingy] = []
```

Yes, but it's worse to the human and the IDE both. I consider type hints 
as serving a dual purpose; both provability and declaration of intent.

--js

On Thu, Feb 04, 2021 at 04:37:45PM +0100, Markus Armbruster wrote:
> Eduardo Habkost <ehabkost@redhat.com> writes:
> 
> > On Wed, Feb 03, 2021 at 03:47:36PM +0100, 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>
> > [...]
> >> > +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.
> >> > +    """
> >> > +    # Remove after 3.7 adds @dataclass:
> >> 
> >> Make this
> >> 
> >>        # TODO Remove after Python 3.7 ...
> >> 
> >> to give us a fighting chance to remember.
> >> 
> >> > +    # pylint: disable=too-few-public-methods
> >> > +    def __init__(self, value: _NodeT, ifcond: Iterable[str],
> >> > +                 comment: Optional[str] = None):
> >> 
> >> Why not simply value: _value?
> >
> > Example:
> >   x = C(1)
> >   y: C[int]
> >   y = C('x')  # mistake
> >
> > Declaring value as _NodeT does:
> > - Make the inferred type of x be Annotated[int].
> > - Catch the mistake above.
> 
> I smell overengineering.  I may well be wrong.

To me it's just regular and idiomatic use of Generic.

> 
> Without doubt, there are uses for using the type system for keeping
> SomeGenericType[SomeType] and SomeGenericType[AnotherType] apart.
> 
> But what do we gain by keeping the Annotated[T] for the possible T
> apart?

I understand this as (valid) criticism of the use of Generic.
If we don't want to make Generic[T1] and Generic[T2] be
different types, there's no point in using Generic at all.


> 
> _tree_to_qlit() doesn't care: it peels off the wrapper holding ifcond
> and comment, and recurses for the JSON so wrapped.  Regardless of what
> was wrapped, i.e. what kind of T we got.
> 
> Heck, it works just fine even if you wrap your JSON multiple times.  It
> doesn't give a hoot whether that makes sense.  Making sense is the
> caller's business.
> 
> So what does care?
> 
> Or am I simply confused?

Those are valid questions.  Maybe using Generic will be useful
in the future, but maybe we don't need it right now.

Personally, I don't care either way.  I just wish this small
choice don't became another obstacle for doing useful work.

> 
> 
> PS: As far as I can tell, _tree_to_qlit() doesn't give a hoot whether a
> dictionary's values are wrapped, either.

-- 
Eduardo

Eduardo Habkost <ehabkost@redhat.com> writes:

> On Thu, Feb 04, 2021 at 04:37:45PM +0100, Markus Armbruster wrote:
>> Eduardo Habkost <ehabkost@redhat.com> writes:
>> 
>> > On Wed, Feb 03, 2021 at 03:47:36PM +0100, 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>
>> > [...]
>> >> > +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.
>> >> > +    """
>> >> > +    # Remove after 3.7 adds @dataclass:
>> >> 
>> >> Make this
>> >> 
>> >>        # TODO Remove after Python 3.7 ...
>> >> 
>> >> to give us a fighting chance to remember.
>> >> 
>> >> > +    # pylint: disable=too-few-public-methods
>> >> > +    def __init__(self, value: _NodeT, ifcond: Iterable[str],
>> >> > +                 comment: Optional[str] = None):
>> >> 
>> >> Why not simply value: _value?
>> >
>> > Example:
>> >   x = C(1)
>> >   y: C[int]
>> >   y = C('x')  # mistake
>> >
>> > Declaring value as _NodeT does:
>> > - Make the inferred type of x be Annotated[int].
>> > - Catch the mistake above.
>> 
>> I smell overengineering.  I may well be wrong.
>
> To me it's just regular and idiomatic use of Generic.

Bear in mind that I'm (ab)using these reviews to learn Python's way of
static typing.  My ignorant questions may evolve into mere grumblings as
I learn.  Or into requests for change.

Grumbling: the notational overhead is regrettable.

>> 
>> Without doubt, there are uses for using the type system for keeping
>> SomeGenericType[SomeType] and SomeGenericType[AnotherType] apart.
>> 
>> But what do we gain by keeping the Annotated[T] for the possible T
>> apart?
>
> I understand this as (valid) criticism of the use of Generic.
> If we don't want to make Generic[T1] and Generic[T2] be
> different types, there's no point in using Generic at all.

True.

>> _tree_to_qlit() doesn't care: it peels off the wrapper holding ifcond
>> and comment, and recurses for the JSON so wrapped.  Regardless of what
>> was wrapped, i.e. what kind of T we got.
>> 
>> Heck, it works just fine even if you wrap your JSON multiple times.  It
>> doesn't give a hoot whether that makes sense.  Making sense is the
>> caller's business.
>> 
>> So what does care?
>> 
>> Or am I simply confused?
>
> Those are valid questions.  Maybe using Generic will be useful
> in the future, but maybe we don't need it right now.
>
> Personally, I don't care either way.  I just wish this small
> choice don't became another obstacle for doing useful work.

I agree this one's minor.

The general problem is not.

Some invariants can be elegantly expressed as static types.  Easier to
read than assertions and comments, and statically checkable.  Lovely, me
want.

There are also cases we can express, but the notational overhead
compromises readability.  We effectively trade readability for static
checking.  I believe this is a bad trade for the QAPI generator.

"Compromises readability" is highly subjective.  Lots of gray area.

My point is: please don't aim for maximally tight types.  Try to
optimize comprehension instead.  Be pragmatic.

There are plenty of languages "where you have to sit with a teacup of
types balanced on your knee and make polite conversation with a strict
old aunt of a compiler."[*]  Let's not press Python into that role :)

>> PS: As far as I can tell, _tree_to_qlit() doesn't give a hoot whether a
>> dictionary's values are wrapped, either.


[*] Paul Graham

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 (
>>       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 = Union[_value, 'Annotated[_value]']
>> +TreeValue = Union[_value, 'Annotated[_value]']
> 
> You need to quote 'Annotated[_value]', because it's a forward
> reference.
> 
> 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, suppress_first_indent=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 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):
>>   
>>       @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': [
> 
> 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

```
self._gen_tree(
     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.

John Snow <jsnow@redhat.com> writes:

> 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 (
>>>       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 = Union[_value, 'Annotated[_value]']
>>> +TreeValue = Union[_value, 'Annotated[_value]']
>> 
>> You need to quote 'Annotated[_value]', because it's a forward
>> reference.
>> 
>> 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.)

The notational overhead is regrettable.  Not your fault.

> 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 ...!)

Fixed in v5.  Good.

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

Done in v5.  Thanks!

>>> +    # 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.)

A generic type feels like overkill here.  This is code that fits on a
page or two.  It solves a simple problem in a simple way.  It's also
badly structured, and virtually undocumented.  I think it needs a
restructuring much more than it needs maximally tight typing.

I'm not demanding you dumb down the types now.  Let's move on.

>>> +        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, suppress_first_indent=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 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.

Dropped in v5.  Good.

>>> +        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': [
>> 
>> 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
>
> ```
> self._gen_tree(
>      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.)

I'll see how it comes out in v5.

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

Things like these are why your salary is called "compensation".  The
"for personal suffering" part you figure out quickly enough.