Re: [PATCH v4 41/46] qapi/introspect.py: create a typed 'Node' data structure

[John Snow]

On 9/30/20 2:39 PM, Eduardo Habkost wrote:
> On Wed, Sep 30, 2020 at 12:31:45AM -0400, John Snow wrote:
> > This replaces _make_tree with Node.__init__(), effectively. By creating
> > it as a generic container, we can more accurately describe the exact
> > nature of this particular Node.
> >
> > Signed-off-by: John Snow <jsnow@redhat.com>
> > ---
> >   scripts/qapi/introspect.py | 77 +++++++++++++++++++-------------------
> >   1 file changed, 38 insertions(+), 39 deletions(-)
> >
> > diff --git a/scripts/qapi/introspect.py b/scripts/qapi/introspect.py
> > index 43b6ba5df1f..86286e755ca 100644
> > --- a/scripts/qapi/introspect.py
> > +++ b/scripts/qapi/introspect.py
> > @@ -12,11 +12,12 @@
> >   
> >   from typing import (
> >       Dict,
> > +    Generic,
> > +    Iterable,
> >       List,
> >       Optional,
> >       Sequence,
> > -    Tuple,
> > -    Union,
> > +    TypeVar,
> >   )
> >   
> >   from .common import (
> > @@ -43,42 +44,42 @@
> >   
> >   
> >   # The correct type for TreeNode is actually:
> > -# Union[AnnotatedNode, List[TreeNode], Dict[str, TreeNode], str, bool]
> > +# Union[Node[TreeNode], List[TreeNode], Dict[str, TreeNode], str, bool]
> >   # but mypy does not support recursive types yet.
> >   TreeNode = object
> > +_NodeType = TypeVar('_NodeType', bound=TreeNode)
> >   _DObject = Dict[str, object]
> > -Extra = Dict[str, object]
> > -AnnotatedNode = Tuple[TreeNode, Extra]
>
> Do you have plans to make Node replace TreeNode completely?
>
> I'd understand this patch as a means to reach that goal, but I'm
> not sure the intermediate state of having both Node and TreeNode
> types (that can be confused with each other) is desirable.

The problem is that _tree_to_qlit still accepts a broad array of types. 
The "TreeNode" comment above explains that those types are:

Node[TreeNode], List[TreeNode], Dict[str, TreeNode], str, bool

Three are containers, two are leaf values.
of the containers, the Node container is special in that it houses 
explicitly one of the four other types (but never itself.)

Even if I somehow always enforced Node[T] heading into _tree_to_qlit, I 
would still need to describe what 'T' is, which is another recursive 
type that I cannot exactly describe with mypy's current descriptive power:

INNER_TYPE = List[Node[INNER_TYPE]], Dict[str, Node[INNER_TYPE]], str, bool

And that's not really a huge win, or indeed any different to the 
existing TreeNode being an "object".

So ... no, I felt like I was going to stop here, where we have 
fundamentally:

1. Undecorated nodes (list, dict, str, bool) ("TreeNode")
2. Decorated nodes (Node[T])                 ("Node")

which leads to the question: Why bother swapping Tuple for Node at that 
point?

My answer is simply that having a strong type name allows us to 
distinguish this from garden-variety Tuples that might sneak in for 
other reasons in other data types.

Maybe we want a different nomenclature though, like Node vs AnnotatedNode?

--js

(Also: 'TreeNode' is just an alias for object, it doesn't mean anything 
grammatically. I could just as soon erase it entirely if you felt it 
provided no value. It doesn't enforce that it only takes objects we 
declared were 'TreeNode' types, for instance. It's just a preprocessor 
name, basically.)

> >   
> >   
> > -def _make_tree(obj: Union[_DObject, str], ifcond: List[str],
> > -               comment: Optional[str] = None) -> AnnotatedNode:
> > -    extra: Extra = {
> > -        'if': ifcond,
> > -        'comment': comment,
> > -    }
> > -    return (obj, extra)
> > +class Node(Generic[_NodeType]):
> > +    """
> > +    Node generally contains a SchemaInfo-like type (as a dict),
> > +    But it also used to wrap comments/ifconds around leaf value types.
> > +    """
> > +    # Remove after 3.7 adds @dataclass:
> > +    # pylint: disable=too-few-public-methods
> > +    def __init__(self, data: _NodeType, ifcond: Iterable[str],
> > +                 comment: Optional[str] = None):
> > +        self.data = data
> > +        self.comment: Optional[str] = comment
> > +        self.ifcond: Sequence[str] = tuple(ifcond)
> >   
> >   
> > -def _tree_to_qlit(obj: TreeNode,
> > -                  level: int = 0,
> > +def _tree_to_qlit(obj: TreeNode, level: int = 0,
> >                     suppress_first_indent: bool = False) -> str:
> >   
> >       def indent(level: int) -> str:
> >           return level * 4 * ' '
> >   
> > -    if isinstance(obj, tuple):
> > -        ifobj, extra = obj
> > -        ifcond = extra.get('if')
> > -        comment = extra.get('comment')
> > +    if isinstance(obj, Node):
> >           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.data, level)
> > +        if obj.ifcond:
> > +            ret += '\n' + gen_endif(obj.ifcond)
> >           return ret
> >   
> >       ret = ''
> > @@ -125,7 +126,7 @@ def __init__(self, prefix: str, unmask: bool):
> >               ' * QAPI/QMP schema introspection', __doc__)
> >           self._unmask = unmask
> >           self._schema: Optional[QAPISchema] = None
> > -        self._trees: List[AnnotatedNode] = []
> > +        self._trees: List[Node[_DObject]] = []
> >           self._used_types: List[QAPISchemaType] = []
> >           self._name_map: Dict[str, str] = {}
> >           self._genc.add(mcgen('''
> > @@ -192,9 +193,8 @@ def _use_type(self, typ: QAPISchemaType) -> str:
> >   
> >       @classmethod
> >       def _gen_features(cls,
> > -                      features: List[QAPISchemaFeature]
> > -                      ) -> List[AnnotatedNode]:
> > -        return [_make_tree(f.name, f.ifcond) for f in features]
> > +                      features: List[QAPISchemaFeature]) -> List[Node[str]]:
> > +        return [Node(f.name, f.ifcond) for f in features]
> >   
> >       def _gen_tree(self, name: str, mtype: str, obj: _DObject,
> >                     ifcond: List[str],
> > @@ -210,10 +210,10 @@ def _gen_tree(self, name: str, mtype: str, obj: _DObject,
> >           obj['meta-type'] = mtype
> >           if features:
> >               obj['features'] = self._gen_features(features)
> > -        self._trees.append(_make_tree(obj, ifcond, comment))
> > +        self._trees.append(Node(obj, ifcond, comment))
> >   
> >       def _gen_member(self,
> > -                    member: QAPISchemaObjectTypeMember) -> AnnotatedNode:
> > +                    member: QAPISchemaObjectTypeMember) -> Node[_DObject]:
> >           obj: _DObject = {
> >               'name': member.name,
> >               'type': self._use_type(member.type)
> > @@ -222,19 +222,19 @@ def _gen_member(self,
> >               obj['default'] = None
> >           if member.features:
> >               obj['features'] = self._gen_features(member.features)
> > -        return _make_tree(obj, member.ifcond)
> > +        return Node(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) -> AnnotatedNode:
> > +    def _gen_variant(self, variant: QAPISchemaVariant) -> Node[_DObject]:
> >           obj: _DObject = {
> >               'case': variant.name,
> >               'type': self._use_type(variant.type)
> >           }
> > -        return _make_tree(obj, variant.ifcond)
> > +        return Node(obj, variant.ifcond)
> >   
> >       def visit_builtin_type(self, name: str, info: Optional[QAPISourceInfo],
> >                              json_type: str) -> None:
> > @@ -245,8 +245,7 @@ def visit_enum_type(self, name: str, info: QAPISourceInfo,
> >                           members: List[QAPISchemaEnumMember],
> >                           prefix: Optional[str]) -> None:
> >           self._gen_tree(name, 'enum',
> > -                       {'values': [_make_tree(m.name, m.ifcond, None)
> > -                                   for m in members]},
> > +                       {'values': [Node(m.name, m.ifcond) for m in members]},
> >                          ifcond, features)
> >   
> >       def visit_array_type(self, name: str, info: Optional[QAPISourceInfo],
> > @@ -274,9 +273,9 @@ def visit_alternate_type(self, name: str, info: 
> > QAPISourceInfo,
> >                                variants: QAPISchemaVariants) -> None:
> >           self._gen_tree(name, 'alternate',
> >                          {'members': [
> > -                           _make_tree({'type': self._use_type(m.type)},
> > -                                      m.ifcond, None)
> > -                           for m in variants.variants]},
> > +                           Node({'type': self._use_type(m.type)}, m.ifcond)
> > +                           for m in variants.variants
> > +                       ]},
> >                          ifcond, features)
> >   
> >       def visit_command(self, name: str, info: QAPISourceInfo, ifcond: List[str],
> > --
> > 2.26.2