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

[John Snow]

On 9/30/20 3:52 PM, Eduardo Habkost wrote:
> 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?

I don't think so, because the runtime check still checks for tuple. I 
like the consistency and simplicity of a named type.

> > Maybe we want a different nomenclature though, like Node vs AnnotatedNode?
>
> Yes, I believe having a more explicit name would be better.

I give up on introspect.py; I'm dropping it from my series. I cannot 
possibly justify another single second spent here.

--js