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,