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

[Eduardo Habkost]

On Wed, Sep 30, 2020 at 02:58:04PM -0400, John Snow wrote:
> 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.

Would:
  AnnotatedNode = NewType('AnnotatedNode', Tuple[TreeNode, Extra])
be enough, then?

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

Yes, I believe having a more explicit name would be better.


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

-- 
Eduardo